Merge pull request #913 from MichaelMure/comment-id-fix

Michael Muré created

core: bubble up the comment ID when created, or edited the first comment

Change summary

api/graphql/resolvers/mutation.go      |   6 
bridge/core/export.go                  |  92 ++++++++++++------------
bridge/core/import.go                  | 107 ++++++++++++++-------------
bridge/github/export.go                |  14 +-
bridge/github/export_test.go           |   7 -
bridge/github/import.go                |  22 +++--
bridge/gitlab/export.go                |  16 ++--
bridge/gitlab/export_test.go           |   7 -
bridge/gitlab/import.go                |  16 ++--
bridge/jira/export.go                  |  14 +-
bridge/jira/import.go                  |  24 +++--
bridge/launchpad/import.go             |   4 
cache/bug_cache.go                     |  31 ++++---
commands/comment_add.go                |   2 
entities/bug/op_add_comment.go         |   6 
entities/bug/op_edit_comment.go        |   8 +-
entities/bug/op_label_change.go        |   1 
entities/bug/op_set_status.go          |   1 
entities/bug/op_set_title.go           |   6 -
misc/random_bugs/create_random_bugs.go |   2 
termui/termui.go                       |   2 
21 files changed, 196 insertions(+), 192 deletions(-)

Detailed changes

api/graphql/resolvers/mutation.go 🔗

@@ -78,7 +78,7 @@ func (r mutationResolver) AddComment(ctx context.Context, input models.AddCommen
 		return nil, err
 	}
 
-	op, err := b.AddCommentRaw(author,
+	_, op, err := b.AddCommentRaw(author,
 		time.Now().Unix(),
 		text.Cleanup(input.Message),
 		input.Files,
@@ -110,7 +110,7 @@ func (r mutationResolver) AddCommentAndClose(ctx context.Context, input models.A
 		return nil, err
 	}
 
-	opAddComment, err := b.AddCommentRaw(author,
+	_, opAddComment, err := b.AddCommentRaw(author,
 		time.Now().Unix(),
 		text.Cleanup(input.Message),
 		input.Files,
@@ -148,7 +148,7 @@ func (r mutationResolver) AddCommentAndReopen(ctx context.Context, input models.
 		return nil, err
 	}
 
-	opAddComment, err := b.AddCommentRaw(author,
+	_, opAddComment, err := b.AddCommentRaw(author,
 		time.Now().Unix(),
 		text.Cleanup(input.Message),
 		input.Files,

bridge/core/export.go 🔗

@@ -42,39 +42,39 @@ const (
 // allow calling code to report on what is happening, collect metrics or
 // display meaningful errors if something went wrong.
 type ExportResult struct {
-	Err    error
-	Event  ExportEvent
-	ID     entity.Id
-	Reason string
+	Err      error
+	Event    ExportEvent
+	EntityId entity.Id // optional for err, warning
+	Reason   string
 }
 
 func (er ExportResult) String() string {
 	switch er.Event {
 	case ExportEventBug:
-		return fmt.Sprintf("new issue: %s", er.ID)
+		return fmt.Sprintf("[%s] new issue: %s", er.EntityId.Human(), er.EntityId)
 	case ExportEventComment:
-		return fmt.Sprintf("new comment: %s", er.ID)
+		return fmt.Sprintf("[%s] new comment", er.EntityId.Human())
 	case ExportEventCommentEdition:
-		return fmt.Sprintf("updated comment: %s", er.ID)
+		return fmt.Sprintf("[%s] updated comment", er.EntityId.Human())
 	case ExportEventStatusChange:
-		return fmt.Sprintf("changed status: %s", er.ID)
+		return fmt.Sprintf("[%s] changed status", er.EntityId.Human())
 	case ExportEventTitleEdition:
-		return fmt.Sprintf("changed title: %s", er.ID)
+		return fmt.Sprintf("[%s] changed title", er.EntityId.Human())
 	case ExportEventLabelChange:
-		return fmt.Sprintf("changed label: %s", er.ID)
+		return fmt.Sprintf("[%s] changed label", er.EntityId.Human())
 	case ExportEventNothing:
-		if er.ID != "" {
-			return fmt.Sprintf("no actions taken for event %s: %s", er.ID, er.Reason)
+		if er.EntityId != "" {
+			return fmt.Sprintf("no actions taken on entity %s: %s", er.EntityId, er.Reason)
 		}
 		return fmt.Sprintf("no actions taken: %s", er.Reason)
 	case ExportEventError:
-		if er.ID != "" {
-			return fmt.Sprintf("export error at %s: %s", er.ID, er.Err.Error())
+		if er.EntityId != "" {
+			return fmt.Sprintf("export error on entity %s: %s", er.EntityId, er.Err.Error())
 		}
 		return fmt.Sprintf("export error: %s", er.Err.Error())
 	case ExportEventWarning:
-		if er.ID != "" {
-			return fmt.Sprintf("warning at %s: %s", er.ID, er.Err.Error())
+		if er.EntityId != "" {
+			return fmt.Sprintf("warning on entity %s: %s", er.EntityId, er.Err.Error())
 		}
 		return fmt.Sprintf("warning: %s", er.Err.Error())
 	case ExportEventRateLimiting:
@@ -85,69 +85,69 @@ func (er ExportResult) String() string {
 	}
 }
 
-func NewExportError(err error, id entity.Id) ExportResult {
+func NewExportError(err error, entityId entity.Id) ExportResult {
 	return ExportResult{
-		ID:    id,
-		Err:   err,
-		Event: ExportEventError,
+		EntityId: entityId,
+		Err:      err,
+		Event:    ExportEventError,
 	}
 }
 
-func NewExportWarning(err error, id entity.Id) ExportResult {
+func NewExportWarning(err error, entityId entity.Id) ExportResult {
 	return ExportResult{
-		ID:    id,
-		Err:   err,
-		Event: ExportEventWarning,
+		EntityId: entityId,
+		Err:      err,
+		Event:    ExportEventWarning,
 	}
 }
 
-func NewExportNothing(id entity.Id, reason string) ExportResult {
+func NewExportNothing(entityId entity.Id, reason string) ExportResult {
 	return ExportResult{
-		ID:     id,
-		Reason: reason,
-		Event:  ExportEventNothing,
+		EntityId: entityId,
+		Reason:   reason,
+		Event:    ExportEventNothing,
 	}
 }
 
-func NewExportBug(id entity.Id) ExportResult {
+func NewExportBug(entityId entity.Id) ExportResult {
 	return ExportResult{
-		ID:    id,
-		Event: ExportEventBug,
+		EntityId: entityId,
+		Event:    ExportEventBug,
 	}
 }
 
-func NewExportComment(id entity.Id) ExportResult {
+func NewExportComment(entityId entity.Id) ExportResult {
 	return ExportResult{
-		ID:    id,
-		Event: ExportEventComment,
+		EntityId: entityId,
+		Event:    ExportEventComment,
 	}
 }
 
-func NewExportCommentEdition(id entity.Id) ExportResult {
+func NewExportCommentEdition(entityId entity.Id) ExportResult {
 	return ExportResult{
-		ID:    id,
-		Event: ExportEventCommentEdition,
+		EntityId: entityId,
+		Event:    ExportEventCommentEdition,
 	}
 }
 
-func NewExportStatusChange(id entity.Id) ExportResult {
+func NewExportStatusChange(entityId entity.Id) ExportResult {
 	return ExportResult{
-		ID:    id,
-		Event: ExportEventStatusChange,
+		EntityId: entityId,
+		Event:    ExportEventStatusChange,
 	}
 }
 
-func NewExportLabelChange(id entity.Id) ExportResult {
+func NewExportLabelChange(entityId entity.Id) ExportResult {
 	return ExportResult{
-		ID:    id,
-		Event: ExportEventLabelChange,
+		EntityId: entityId,
+		Event:    ExportEventLabelChange,
 	}
 }
 
-func NewExportTitleEdition(id entity.Id) ExportResult {
+func NewExportTitleEdition(entityId entity.Id) ExportResult {
 	return ExportResult{
-		ID:    id,
-		Event: ExportEventTitleEdition,
+		EntityId: entityId,
+		Event:    ExportEventTitleEdition,
 	}
 }
 

bridge/core/import.go 🔗

@@ -45,43 +45,45 @@ const (
 // allow calling code to report on what is happening, collect metrics or
 // display meaningful errors if something went wrong.
 type ImportResult struct {
-	Err    error
-	Event  ImportEvent
-	ID     entity.Id
-	Reason string
+	Err         error
+	Event       ImportEvent
+	EntityId    entity.Id         // optional for err, warnings
+	OperationId entity.Id         // optional
+	ComponentId entity.CombinedId // optional
+	Reason      string
 }
 
 func (er ImportResult) String() string {
 	switch er.Event {
 	case ImportEventBug:
-		return fmt.Sprintf("new issue: %s", er.ID)
+		return fmt.Sprintf("[%s] new issue: %s", er.EntityId.Human(), er.EntityId)
 	case ImportEventComment:
-		return fmt.Sprintf("new comment: %s", er.ID)
+		return fmt.Sprintf("[%s] new comment: %s", er.EntityId.Human(), er.ComponentId)
 	case ImportEventCommentEdition:
-		return fmt.Sprintf("updated comment: %s", er.ID)
+		return fmt.Sprintf("[%s] updated comment: %s", er.EntityId.Human(), er.ComponentId)
 	case ImportEventStatusChange:
-		return fmt.Sprintf("changed status: %s", er.ID)
+		return fmt.Sprintf("[%s] changed status with op: %s", er.EntityId.Human(), er.OperationId)
 	case ImportEventTitleEdition:
-		return fmt.Sprintf("changed title: %s", er.ID)
+		return fmt.Sprintf("[%s] changed title with op: %s", er.EntityId.Human(), er.OperationId)
 	case ImportEventLabelChange:
-		return fmt.Sprintf("changed label: %s", er.ID)
+		return fmt.Sprintf("[%s] changed label with op: %s", er.EntityId.Human(), er.OperationId)
 	case ImportEventIdentity:
-		return fmt.Sprintf("new identity: %s", er.ID)
+		return fmt.Sprintf("[%s] new identity: %s", er.EntityId.Human(), er.EntityId)
 	case ImportEventNothing:
-		if er.ID != "" {
-			return fmt.Sprintf("no action taken for event %s: %s", er.ID, er.Reason)
+		if er.EntityId != "" {
+			return fmt.Sprintf("no action taken on entity %s: %s", er.EntityId, er.Reason)
 		}
 		return fmt.Sprintf("no action taken: %s", er.Reason)
 	case ImportEventError:
-		if er.ID != "" {
-			return fmt.Sprintf("import error at id %s: %s", er.ID, er.Err.Error())
+		if er.EntityId != "" {
+			return fmt.Sprintf("import error on entity %s: %s", er.EntityId, er.Err.Error())
 		}
 		return fmt.Sprintf("import error: %s", er.Err.Error())
 	case ImportEventWarning:
 		parts := make([]string, 0, 4)
 		parts = append(parts, "warning:")
-		if er.ID != "" {
-			parts = append(parts, fmt.Sprintf("at id %s", er.ID))
+		if er.EntityId != "" {
+			parts = append(parts, fmt.Sprintf("on entity %s", er.EntityId))
 		}
 		if er.Reason != "" {
 			parts = append(parts, fmt.Sprintf("reason: %s", er.Reason))
@@ -98,76 +100,81 @@ func (er ImportResult) String() string {
 	}
 }
 
-func NewImportError(err error, id entity.Id) ImportResult {
+func NewImportError(err error, entityId entity.Id) ImportResult {
 	return ImportResult{
-		Err:   err,
-		ID:    id,
-		Event: ImportEventError,
+		Err:      err,
+		EntityId: entityId,
+		Event:    ImportEventError,
 	}
 }
 
-func NewImportWarning(err error, id entity.Id) ImportResult {
+func NewImportWarning(err error, entityId entity.Id) ImportResult {
 	return ImportResult{
-		Err:   err,
-		ID:    id,
-		Event: ImportEventWarning,
+		Err:      err,
+		EntityId: entityId,
+		Event:    ImportEventWarning,
 	}
 }
 
-func NewImportNothing(id entity.Id, reason string) ImportResult {
+func NewImportNothing(entityId entity.Id, reason string) ImportResult {
 	return ImportResult{
-		ID:     id,
-		Reason: reason,
-		Event:  ImportEventNothing,
+		EntityId: entityId,
+		Reason:   reason,
+		Event:    ImportEventNothing,
 	}
 }
 
-func NewImportBug(id entity.Id) ImportResult {
+func NewImportBug(entityId entity.Id) ImportResult {
 	return ImportResult{
-		ID:    id,
-		Event: ImportEventBug,
+		EntityId: entityId,
+		Event:    ImportEventBug,
 	}
 }
 
-func NewImportComment(id entity.Id) ImportResult {
+func NewImportComment(entityId entity.Id, commentId entity.CombinedId) ImportResult {
 	return ImportResult{
-		ID:    id,
-		Event: ImportEventComment,
+		EntityId:    entityId,
+		ComponentId: commentId,
+		Event:       ImportEventComment,
 	}
 }
 
-func NewImportCommentEdition(id entity.Id) ImportResult {
+func NewImportCommentEdition(entityId entity.Id, commentId entity.CombinedId) ImportResult {
 	return ImportResult{
-		ID:    id,
-		Event: ImportEventCommentEdition,
+		EntityId:    entityId,
+		ComponentId: commentId,
+		Event:       ImportEventCommentEdition,
 	}
 }
 
-func NewImportStatusChange(id entity.Id) ImportResult {
+func NewImportStatusChange(entityId entity.Id, opId entity.Id) ImportResult {
 	return ImportResult{
-		ID:    id,
-		Event: ImportEventStatusChange,
+		EntityId:    entityId,
+		OperationId: opId,
+		Event:       ImportEventStatusChange,
 	}
 }
 
-func NewImportLabelChange(id entity.Id) ImportResult {
+func NewImportLabelChange(entityId entity.Id, opId entity.Id) ImportResult {
 	return ImportResult{
-		ID:    id,
-		Event: ImportEventLabelChange,
+		EntityId:    entityId,
+		OperationId: opId,
+		Event:       ImportEventLabelChange,
 	}
 }
 
-func NewImportTitleEdition(id entity.Id) ImportResult {
+func NewImportTitleEdition(entityId entity.Id, opId entity.Id) ImportResult {
 	return ImportResult{
-		ID:    id,
-		Event: ImportEventTitleEdition,
+		EntityId:    entityId,
+		OperationId: opId,
+		Event:       ImportEventTitleEdition,
 	}
 }
 
-func NewImportIdentity(id entity.Id) ImportResult {
+func NewImportIdentity(entityId entity.Id) ImportResult {
 	return ImportResult{
-		ID:    id,
-		Event: ImportEventIdentity,
+		EntityId: entityId,
+		Event:    ImportEventIdentity,
 	}
 }
 

bridge/github/export.go 🔗

@@ -318,7 +318,7 @@ func (ge *githubExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 				return
 			}
 
-			out <- core.NewExportComment(op.Id())
+			out <- core.NewExportComment(b.Id())
 
 			// cache comment id
 			ge.cachedOperationIDs[op.Id()] = id
@@ -334,7 +334,7 @@ func (ge *githubExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 					return
 				}
 
-				out <- core.NewExportCommentEdition(op.Id())
+				out <- core.NewExportCommentEdition(b.Id())
 
 				id = bugGithubID
 				url = bugGithubURL
@@ -354,7 +354,7 @@ func (ge *githubExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 					return
 				}
 
-				out <- core.NewExportCommentEdition(op.Id())
+				out <- core.NewExportCommentEdition(b.Id())
 
 				// use comment id/url instead of issue id/url
 				id = eid
@@ -368,7 +368,7 @@ func (ge *githubExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 				return
 			}
 
-			out <- core.NewExportStatusChange(op.Id())
+			out <- core.NewExportStatusChange(b.Id())
 
 			id = bugGithubID
 			url = bugGithubURL
@@ -380,7 +380,7 @@ func (ge *githubExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 				return
 			}
 
-			out <- core.NewExportTitleEdition(op.Id())
+			out <- core.NewExportTitleEdition(b.Id())
 
 			id = bugGithubID
 			url = bugGithubURL
@@ -392,7 +392,7 @@ func (ge *githubExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 				return
 			}
 
-			out <- core.NewExportLabelChange(op.Id())
+			out <- core.NewExportLabelChange(b.Id())
 
 			id = bugGithubID
 			url = bugGithubURL
@@ -659,7 +659,7 @@ func (ge *githubExporter) createGithubIssue(ctx context.Context, gc *rateLimitHa
 	return issue.ID, issue.URL, nil
 }
 
-// add a comment to an issue and return it ID
+// add a comment to an issue and return its ID
 func (ge *githubExporter) addCommentGithubIssue(ctx context.Context, gc *rateLimitHandlerClient, subjectID string, body string) (string, string, error) {
 	m := &addCommentToIssueMutation{}
 	input := githubv4.AddCommentInput{

bridge/github/export_test.go 🔗

@@ -41,7 +41,7 @@ func testCases(t *testing.T, repo *cache.RepoCache) []*testCase {
 	bugWithComments, _, err := repo.NewBug("bug with comments", "new bug")
 	require.NoError(t, err)
 
-	_, err = bugWithComments.AddComment("new comment")
+	_, _, err = bugWithComments.AddComment("new comment")
 	require.NoError(t, err)
 
 	// bug with label changes
@@ -71,11 +71,10 @@ func testCases(t *testing.T, repo *cache.RepoCache) []*testCase {
 		entity.CombineIds(bugWithCommentEditions.Id(), createOp.Id()), "first comment edited")
 	require.NoError(t, err)
 
-	commentOp, err := bugWithCommentEditions.AddComment("first comment")
+	commentId, _, err := bugWithCommentEditions.AddComment("first comment")
 	require.NoError(t, err)
 
-	_, err = bugWithCommentEditions.EditComment(
-		entity.CombineIds(bugWithCommentEditions.Id(), commentOp.Id()), "first comment edited")
+	_, err = bugWithCommentEditions.EditComment(commentId, "first comment edited")
 	require.NoError(t, err)
 
 	// bug status changed

bridge/github/import.go 🔗

@@ -274,7 +274,7 @@ func (gi *githubImporter) ensureTimelineItem(ctx context.Context, repo *cache.Re
 			return err
 		}
 
-		gi.out <- core.NewImportLabelChange(op.Id())
+		gi.out <- core.NewImportLabelChange(b.Id(), op.Id())
 		return nil
 
 	case "UnlabeledEvent":
@@ -304,7 +304,7 @@ func (gi *githubImporter) ensureTimelineItem(ctx context.Context, repo *cache.Re
 			return err
 		}
 
-		gi.out <- core.NewImportLabelChange(op.Id())
+		gi.out <- core.NewImportLabelChange(b.Id(), op.Id())
 		return nil
 
 	case "ClosedEvent":
@@ -330,7 +330,7 @@ func (gi *githubImporter) ensureTimelineItem(ctx context.Context, repo *cache.Re
 			return err
 		}
 
-		gi.out <- core.NewImportStatusChange(op.Id())
+		gi.out <- core.NewImportStatusChange(b.Id(), op.Id())
 		return nil
 
 	case "ReopenedEvent":
@@ -356,7 +356,7 @@ func (gi *githubImporter) ensureTimelineItem(ctx context.Context, repo *cache.Re
 			return err
 		}
 
-		gi.out <- core.NewImportStatusChange(op.Id())
+		gi.out <- core.NewImportStatusChange(b.Id(), op.Id())
 		return nil
 
 	case "RenamedTitleEvent":
@@ -392,7 +392,7 @@ func (gi *githubImporter) ensureTimelineItem(ctx context.Context, repo *cache.Re
 			return err
 		}
 
-		gi.out <- core.NewImportTitleEdition(op.Id())
+		gi.out <- core.NewImportTitleEdition(b.Id(), op.Id())
 		return nil
 	}
 
@@ -425,11 +425,13 @@ func (gi *githubImporter) ensureCommentEdit(ctx context.Context, repo *cache.Rep
 		return nil
 	}
 
+	commentId := entity.CombineIds(b.Id(), target)
+
 	// comment edition
-	op, err := b.EditCommentRaw(
+	_, err = b.EditCommentRaw(
 		editor,
 		edit.CreatedAt.Unix(),
-		entity.CombineIds(b.Id(), target),
+		commentId,
 		text.Cleanup(string(*edit.Diff)),
 		map[string]string{
 			metaKeyGithubId: parseId(edit.Id),
@@ -440,7 +442,7 @@ func (gi *githubImporter) ensureCommentEdit(ctx context.Context, repo *cache.Rep
 		return err
 	}
 
-	gi.out <- core.NewImportCommentEdition(op.Id())
+	gi.out <- core.NewImportCommentEdition(b.Id(), commentId)
 	return nil
 }
 
@@ -469,7 +471,7 @@ func (gi *githubImporter) ensureComment(ctx context.Context, repo *cache.RepoCac
 	}
 
 	// add comment operation
-	op, err := b.AddCommentRaw(
+	commentId, _, err := b.AddCommentRaw(
 		author,
 		comment.CreatedAt.Unix(),
 		text.Cleanup(textInput),
@@ -483,7 +485,7 @@ func (gi *githubImporter) ensureComment(ctx context.Context, repo *cache.RepoCac
 		return err
 	}
 
-	gi.out <- core.NewImportComment(op.Id())
+	gi.out <- core.NewImportComment(b.Id(), commentId)
 	return nil
 }
 

bridge/gitlab/export.go 🔗

@@ -288,7 +288,7 @@ func (ge *gitlabExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 				return
 			}
 
-			out <- core.NewExportComment(op.Id())
+			out <- core.NewExportComment(b.Id())
 
 			idString = strconv.Itoa(id)
 			// cache comment id
@@ -307,7 +307,7 @@ func (ge *gitlabExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 					return
 				}
 
-				out <- core.NewExportCommentEdition(op.Id())
+				out <- core.NewExportCommentEdition(b.Id())
 				id = bugGitlabID
 
 			} else {
@@ -315,13 +315,13 @@ func (ge *gitlabExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 				// case comment edition operation: we need to edit the Gitlab comment
 				commentID, ok := ge.cachedOperationIDs[targetId]
 				if !ok {
-					out <- core.NewExportError(fmt.Errorf("unexpected error: comment id not found"), op.Target)
+					out <- core.NewExportError(fmt.Errorf("unexpected error: comment id not found"), b.Id())
 					return
 				}
 
 				commentIDint, err := strconv.Atoi(commentID)
 				if err != nil {
-					out <- core.NewExportError(fmt.Errorf("unexpected comment id format"), op.Target)
+					out <- core.NewExportError(fmt.Errorf("unexpected comment id format"), b.Id())
 					return
 				}
 
@@ -331,7 +331,7 @@ func (ge *gitlabExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 					return
 				}
 
-				out <- core.NewExportCommentEdition(op.Id())
+				out <- core.NewExportCommentEdition(b.Id())
 				id = commentIDint
 			}
 
@@ -342,7 +342,7 @@ func (ge *gitlabExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 				return
 			}
 
-			out <- core.NewExportStatusChange(op.Id())
+			out <- core.NewExportStatusChange(b.Id())
 			id = bugGitlabID
 
 		case *bug.SetTitleOperation:
@@ -352,7 +352,7 @@ func (ge *gitlabExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 				return
 			}
 
-			out <- core.NewExportTitleEdition(op.Id())
+			out <- core.NewExportTitleEdition(b.Id())
 			id = bugGitlabID
 
 		case *bug.LabelChangeOperation:
@@ -378,7 +378,7 @@ func (ge *gitlabExporter) exportBug(ctx context.Context, b *cache.BugCache, out
 				return
 			}
 
-			out <- core.NewExportLabelChange(op.Id())
+			out <- core.NewExportLabelChange(b.Id())
 			id = bugGitlabID
 		default:
 			panic("unhandled operation type case")

bridge/gitlab/export_test.go 🔗

@@ -44,7 +44,7 @@ func testCases(t *testing.T, repo *cache.RepoCache) []*testCase {
 	bugWithComments, _, err := repo.NewBug("bug with comments", "new bug")
 	require.NoError(t, err)
 
-	_, err = bugWithComments.AddComment("new comment")
+	_, _, err = bugWithComments.AddComment("new comment")
 	require.NoError(t, err)
 
 	// bug with label changes
@@ -68,11 +68,10 @@ func testCases(t *testing.T, repo *cache.RepoCache) []*testCase {
 		entity.CombineIds(bugWithCommentEditions.Id(), createOp.Id()), "first comment edited")
 	require.NoError(t, err)
 
-	commentOp, err := bugWithCommentEditions.AddComment("first comment")
+	commentId, _, err := bugWithCommentEditions.AddComment("first comment")
 	require.NoError(t, err)
 
-	_, err = bugWithCommentEditions.EditComment(
-		entity.CombineIds(bugWithCommentEditions.Id(), commentOp.Id()), "first comment edited")
+	_, err = bugWithCommentEditions.EditComment(commentId, "first comment edited")
 	require.NoError(t, err)
 
 	// bug status changed

bridge/gitlab/import.go 🔗

@@ -178,7 +178,7 @@ func (gi *gitlabImporter) ensureIssueEvent(repo *cache.RepoCache, b *cache.BugCa
 			return err
 		}
 
-		gi.out <- core.NewImportStatusChange(op.Id())
+		gi.out <- core.NewImportStatusChange(b.Id(), op.Id())
 
 	case EventReopened:
 		if errResolve == nil {
@@ -196,7 +196,7 @@ func (gi *gitlabImporter) ensureIssueEvent(repo *cache.RepoCache, b *cache.BugCa
 			return err
 		}
 
-		gi.out <- core.NewImportStatusChange(op.Id())
+		gi.out <- core.NewImportStatusChange(b.Id(), op.Id())
 
 	case EventDescriptionChanged:
 		firstComment := b.Snapshot().Comments[0]
@@ -219,7 +219,7 @@ func (gi *gitlabImporter) ensureIssueEvent(repo *cache.RepoCache, b *cache.BugCa
 				return err
 			}
 
-			gi.out <- core.NewImportTitleEdition(op.Id())
+			gi.out <- core.NewImportTitleEdition(b.Id(), op.Id())
 		}
 
 	case EventComment:
@@ -229,7 +229,7 @@ func (gi *gitlabImporter) ensureIssueEvent(repo *cache.RepoCache, b *cache.BugCa
 		if errResolve == cache.ErrNoMatchingOp {
 
 			// add comment operation
-			op, err := b.AddCommentRaw(
+			commentId, _, err := b.AddCommentRaw(
 				author,
 				event.CreatedAt().Unix(),
 				cleanText,
@@ -241,7 +241,7 @@ func (gi *gitlabImporter) ensureIssueEvent(repo *cache.RepoCache, b *cache.BugCa
 			if err != nil {
 				return err
 			}
-			gi.out <- core.NewImportComment(op.Id())
+			gi.out <- core.NewImportComment(b.Id(), commentId)
 			return nil
 		}
 
@@ -256,7 +256,7 @@ func (gi *gitlabImporter) ensureIssueEvent(repo *cache.RepoCache, b *cache.BugCa
 		// compare local bug comment with the new event body
 		if comment.Message != cleanText {
 			// comment edition
-			op, err := b.EditCommentRaw(
+			_, err := b.EditCommentRaw(
 				author,
 				event.(NoteEvent).UpdatedAt.Unix(),
 				comment.CombinedId(),
@@ -267,7 +267,7 @@ func (gi *gitlabImporter) ensureIssueEvent(repo *cache.RepoCache, b *cache.BugCa
 			if err != nil {
 				return err
 			}
-			gi.out <- core.NewImportCommentEdition(op.Id())
+			gi.out <- core.NewImportCommentEdition(b.Id(), comment.CombinedId())
 		}
 
 		return nil
@@ -290,7 +290,7 @@ func (gi *gitlabImporter) ensureIssueEvent(repo *cache.RepoCache, b *cache.BugCa
 			return err
 		}
 
-		gi.out <- core.NewImportTitleEdition(op.Id())
+		gi.out <- core.NewImportTitleEdition(b.Id(), op.Id())
 
 	case EventAddLabel:
 		_, err = b.ForceChangeLabelsRaw(

bridge/jira/export.go 🔗

@@ -315,7 +315,7 @@ func (je *jiraExporter) exportBug(ctx context.Context, b *cache.BugCache, out ch
 		if err != nil {
 			out <- core.NewExportError(
 				fmt.Errorf("missing operation author credentials for user %.8s",
-					author.Id().String()), op.Id())
+					author.Id().String()), b.Id())
 			continue
 		}
 
@@ -330,7 +330,7 @@ func (je *jiraExporter) exportBug(ctx context.Context, b *cache.BugCache, out ch
 				return err
 			}
 			id = comment.ID
-			out <- core.NewExportComment(op.Id())
+			out <- core.NewExportComment(b.Id())
 
 			// cache comment id
 			je.cachedOperationIDs[op.Id()] = id
@@ -345,7 +345,7 @@ func (je *jiraExporter) exportBug(ctx context.Context, b *cache.BugCache, out ch
 					out <- core.NewExportError(err, b.Id())
 					return err
 				}
-				out <- core.NewExportCommentEdition(op.Id())
+				out <- core.NewExportCommentEdition(b.Id())
 				id = bugJiraID
 			} else {
 				// Otherwise it's an edit to an actual comment. A comment cannot be
@@ -363,7 +363,7 @@ func (je *jiraExporter) exportBug(ctx context.Context, b *cache.BugCache, out ch
 					out <- core.NewExportError(err, b.Id())
 					return err
 				}
-				out <- core.NewExportCommentEdition(op.Id())
+				out <- core.NewExportCommentEdition(b.Id())
 				// JIRA doesn't track all comment edits, they will only tell us about
 				// the most recent one. We must invent a consistent id for the operation
 				// so we use the comment ID plus the timestamp of the update, as
@@ -384,7 +384,7 @@ func (je *jiraExporter) exportBug(ctx context.Context, b *cache.BugCache, out ch
 					// update. In this case, just don't export the operation.
 					continue
 				}
-				out <- core.NewExportStatusChange(op.Id())
+				out <- core.NewExportStatusChange(b.Id())
 				id = bugJiraID
 			} else {
 				out <- core.NewExportError(fmt.Errorf(
@@ -398,7 +398,7 @@ func (je *jiraExporter) exportBug(ctx context.Context, b *cache.BugCache, out ch
 				out <- core.NewExportError(err, b.Id())
 				return err
 			}
-			out <- core.NewExportTitleEdition(op.Id())
+			out <- core.NewExportTitleEdition(b.Id())
 			id = bugJiraID
 
 		case *bug.LabelChangeOperation:
@@ -409,7 +409,7 @@ func (je *jiraExporter) exportBug(ctx context.Context, b *cache.BugCache, out ch
 				out <- core.NewExportError(err, b.Id())
 				return err
 			}
-			out <- core.NewExportLabelChange(op.Id())
+			out <- core.NewExportLabelChange(b.Id())
 			id = bugJiraID
 
 		default:

bridge/jira/import.go 🔗

@@ -286,7 +286,7 @@ func (ji *jiraImporter) ensureComment(repo *cache.RepoCache, b *cache.BugCache,
 		}
 
 		// add comment operation
-		op, err := b.AddCommentRaw(
+		commentId, op, err := b.AddCommentRaw(
 			author,
 			item.Created.Unix(),
 			cleanText,
@@ -299,7 +299,7 @@ func (ji *jiraImporter) ensureComment(repo *cache.RepoCache, b *cache.BugCache,
 			return err
 		}
 
-		ji.out <- core.NewImportComment(op.Id())
+		ji.out <- core.NewImportComment(b.Id(), commentId)
 		targetOpID = op.Id()
 	}
 
@@ -329,11 +329,13 @@ func (ji *jiraImporter) ensureComment(repo *cache.RepoCache, b *cache.BugCache,
 		return err
 	}
 
+	commentId := entity.CombineIds(b.Id(), targetOpID)
+
 	// comment edition
-	op, err := b.EditCommentRaw(
+	_, err = b.EditCommentRaw(
 		editor,
 		item.Updated.Unix(),
-		entity.CombineIds(b.Id(), targetOpID),
+		commentId,
 		text.Cleanup(item.Body),
 		map[string]string{
 			metaKeyJiraId: derivedID,
@@ -344,7 +346,7 @@ func (ji *jiraImporter) ensureComment(repo *cache.RepoCache, b *cache.BugCache,
 		return err
 	}
 
-	ji.out <- core.NewImportCommentEdition(op.Id())
+	ji.out <- core.NewImportCommentEdition(b.Id(), commentId)
 
 	return nil
 }
@@ -510,7 +512,7 @@ func (ji *jiraImporter) ensureChange(repo *cache.RepoCache, b *cache.BugCache, e
 				return err
 			}
 
-			ji.out <- core.NewImportLabelChange(op.Id())
+			ji.out <- core.NewImportLabelChange(b.Id(), op.Id())
 
 		case "status":
 			statusStr, hasMap := statusMap[item.To]
@@ -528,7 +530,7 @@ func (ji *jiraImporter) ensureChange(repo *cache.RepoCache, b *cache.BugCache, e
 					if err != nil {
 						return err
 					}
-					ji.out <- core.NewImportStatusChange(op.Id())
+					ji.out <- core.NewImportStatusChange(b.Id(), op.Id())
 
 				case common.ClosedStatus.String():
 					op, err := b.CloseRaw(
@@ -542,7 +544,7 @@ func (ji *jiraImporter) ensureChange(repo *cache.RepoCache, b *cache.BugCache, e
 					if err != nil {
 						return err
 					}
-					ji.out <- core.NewImportStatusChange(op.Id())
+					ji.out <- core.NewImportStatusChange(b.Id(), op.Id())
 				}
 			} else {
 				ji.out <- core.NewImportError(
@@ -567,12 +569,12 @@ func (ji *jiraImporter) ensureChange(repo *cache.RepoCache, b *cache.BugCache, e
 				return err
 			}
 
-			ji.out <- core.NewImportTitleEdition(op.Id())
+			ji.out <- core.NewImportTitleEdition(b.Id(), op.Id())
 
 		case "description":
 			// NOTE(josh): JIRA calls it "description", which sounds more like the
 			// title but it's actually the body
-			op, err := b.EditCreateCommentRaw(
+			commentId, _, err := b.EditCreateCommentRaw(
 				author,
 				entry.Created.Unix(),
 				text.Cleanup(item.ToString),
@@ -585,7 +587,7 @@ func (ji *jiraImporter) ensureChange(repo *cache.RepoCache, b *cache.BugCache, e
 				return err
 			}
 
-			ji.out <- core.NewImportCommentEdition(op.Id())
+			ji.out <- core.NewImportCommentEdition(b.Id(), commentId)
 
 		default:
 			ji.out <- core.NewImportWarning(

bridge/launchpad/import.go 🔗

@@ -131,7 +131,7 @@ func (li *launchpadImporter) ImportAll(ctx context.Context, repo *cache.RepoCach
 
 					// This is a new comment, we can add it.
 					createdAt, _ := time.Parse(time.RFC3339, lpMessage.CreatedAt)
-					op, err := b.AddCommentRaw(
+					commentId, _, err := b.AddCommentRaw(
 						owner,
 						createdAt.Unix(),
 						text.Cleanup(lpMessage.Content),
@@ -144,7 +144,7 @@ func (li *launchpadImporter) ImportAll(ctx context.Context, repo *cache.RepoCach
 						return
 					}
 
-					out <- core.NewImportComment(op.Id())
+					out <- core.NewImportComment(b.Id(), commentId)
 				}
 
 				if !b.NeedCommit() {

cache/bug_cache.go 🔗

@@ -70,27 +70,27 @@ func (c *BugCache) ResolveOperationWithMetadata(key string, value string) (entit
 	return matching[0], nil
 }
 
-func (c *BugCache) AddComment(message string) (*bug.AddCommentOperation, error) {
+func (c *BugCache) AddComment(message string) (entity.CombinedId, *bug.AddCommentOperation, error) {
 	return c.AddCommentWithFiles(message, nil)
 }
 
-func (c *BugCache) AddCommentWithFiles(message string, files []repository.Hash) (*bug.AddCommentOperation, error) {
+func (c *BugCache) AddCommentWithFiles(message string, files []repository.Hash) (entity.CombinedId, *bug.AddCommentOperation, error) {
 	author, err := c.repoCache.GetUserIdentity()
 	if err != nil {
-		return nil, err
+		return entity.UnsetCombinedId, nil, err
 	}
 
 	return c.AddCommentRaw(author, time.Now().Unix(), message, files, nil)
 }
 
-func (c *BugCache) AddCommentRaw(author *IdentityCache, unixTime int64, message string, files []repository.Hash, metadata map[string]string) (*bug.AddCommentOperation, error) {
+func (c *BugCache) AddCommentRaw(author *IdentityCache, unixTime int64, message string, files []repository.Hash, metadata map[string]string) (entity.CombinedId, *bug.AddCommentOperation, error) {
 	c.mu.Lock()
-	op, err := bug.AddComment(c.bug, author, unixTime, message, files, metadata)
+	commentId, op, err := bug.AddComment(c.bug, author, unixTime, message, files, metadata)
 	c.mu.Unlock()
 	if err != nil {
-		return nil, err
+		return entity.UnsetCombinedId, nil, err
 	}
-	return op, c.notifyUpdated()
+	return commentId, op, c.notifyUpdated()
 }
 
 func (c *BugCache) ChangeLabels(added []string, removed []string) ([]bug.LabelChangeResult, *bug.LabelChangeOperation, error) {
@@ -189,24 +189,24 @@ func (c *BugCache) SetTitleRaw(author *IdentityCache, unixTime int64, title stri
 }
 
 // EditCreateComment is a convenience function to edit the body of a bug (the first comment)
-func (c *BugCache) EditCreateComment(body string) (*bug.EditCommentOperation, error) {
+func (c *BugCache) EditCreateComment(body string) (entity.CombinedId, *bug.EditCommentOperation, error) {
 	author, err := c.repoCache.GetUserIdentity()
 	if err != nil {
-		return nil, err
+		return entity.UnsetCombinedId, nil, err
 	}
 
 	return c.EditCreateCommentRaw(author, time.Now().Unix(), body, nil)
 }
 
 // EditCreateCommentRaw is a convenience function to edit the body of a bug (the first comment)
-func (c *BugCache) EditCreateCommentRaw(author *IdentityCache, unixTime int64, body string, metadata map[string]string) (*bug.EditCommentOperation, error) {
+func (c *BugCache) EditCreateCommentRaw(author *IdentityCache, unixTime int64, body string, metadata map[string]string) (entity.CombinedId, *bug.EditCommentOperation, error) {
 	c.mu.Lock()
-	op, err := bug.EditCreateComment(c.bug, author.Identity, unixTime, body, nil, metadata)
+	commentId, op, err := bug.EditCreateComment(c.bug, author.Identity, unixTime, body, nil, metadata)
 	c.mu.Unlock()
 	if err != nil {
-		return nil, err
+		return entity.UnsetCombinedId, nil, err
 	}
-	return op, c.notifyUpdated()
+	return commentId, op, c.notifyUpdated()
 }
 
 func (c *BugCache) EditComment(target entity.CombinedId, message string) (*bug.EditCommentOperation, error) {
@@ -225,11 +225,14 @@ func (c *BugCache) EditCommentRaw(author *IdentityCache, unixTime int64, target
 	}
 
 	c.mu.Lock()
-	op, err := bug.EditComment(c.bug, author.Identity, unixTime, comment.TargetId(), message, nil, metadata)
+	commentId, op, err := bug.EditComment(c.bug, author.Identity, unixTime, comment.TargetId(), message, nil, metadata)
 	c.mu.Unlock()
 	if err != nil {
 		return nil, err
 	}
+	if commentId != target {
+		panic("EditComment returned unexpected comment id")
+	}
 	return op, c.notifyUpdated()
 }
 

commands/comment_add.go 🔗

@@ -69,7 +69,7 @@ func runCommentAdd(env *Env, opts commentAddOptions, args []string) error {
 		}
 	}
 
-	_, err = b.AddComment(text.Cleanup(opts.message))
+	_, _, err = b.AddComment(text.Cleanup(opts.message))
 	if err != nil {
 		return err
 	}

entities/bug/op_add_comment.go 🔗

@@ -83,14 +83,14 @@ type AddCommentTimelineItem struct {
 func (a *AddCommentTimelineItem) IsAuthored() {}
 
 // AddComment is a convenience function to add a comment to a bug
-func AddComment(b Interface, author identity.Interface, unixTime int64, message string, files []repository.Hash, metadata map[string]string) (*AddCommentOperation, error) {
+func AddComment(b Interface, author identity.Interface, unixTime int64, message string, files []repository.Hash, metadata map[string]string) (entity.CombinedId, *AddCommentOperation, error) {
 	op := NewAddCommentOp(author, unixTime, message, files)
 	for key, val := range metadata {
 		op.SetMetadata(key, val)
 	}
 	if err := op.Validate(); err != nil {
-		return nil, err
+		return entity.UnsetCombinedId, nil, err
 	}
 	b.Append(op)
-	return op, nil
+	return entity.CombineIds(b.Id(), op.Id()), op, nil
 }

entities/bug/op_edit_comment.go 🔗

@@ -111,20 +111,20 @@ func NewEditCommentOp(author identity.Interface, unixTime int64, target entity.I
 }
 
 // EditComment is a convenience function to apply the operation
-func EditComment(b Interface, author identity.Interface, unixTime int64, target entity.Id, message string, files []repository.Hash, metadata map[string]string) (*EditCommentOperation, error) {
+func EditComment(b Interface, author identity.Interface, unixTime int64, target entity.Id, message string, files []repository.Hash, metadata map[string]string) (entity.CombinedId, *EditCommentOperation, error) {
 	op := NewEditCommentOp(author, unixTime, target, message, files)
 	for key, val := range metadata {
 		op.SetMetadata(key, val)
 	}
 	if err := op.Validate(); err != nil {
-		return nil, err
+		return entity.UnsetCombinedId, nil, err
 	}
 	b.Append(op)
-	return op, nil
+	return entity.CombineIds(b.Id(), target), op, nil
 }
 
 // EditCreateComment is a convenience function to edit the body of a bug (the first comment)
-func EditCreateComment(b Interface, author identity.Interface, unixTime int64, message string, files []repository.Hash, metadata map[string]string) (*EditCommentOperation, error) {
+func EditCreateComment(b Interface, author identity.Interface, unixTime int64, message string, files []repository.Hash, metadata map[string]string) (entity.CombinedId, *EditCommentOperation, error) {
 	createOp := b.FirstOp().(*CreateOperation)
 	return EditComment(b, author, unixTime, createOp.Id(), message, files, metadata)
 }

entities/bug/op_label_change.go 🔗

@@ -105,7 +105,6 @@ func NewLabelChangeOperation(author identity.Interface, unixTime int64, added, r
 }
 
 type LabelChangeTimelineItem struct {
-	// id         entity.Id
 	combinedId entity.CombinedId
 	Author     identity.Interface
 	UnixTime   timestamp.Timestamp

entities/bug/op_set_status.go 🔗

@@ -58,7 +58,6 @@ func NewSetStatusOp(author identity.Interface, unixTime int64, status common.Sta
 }
 
 type SetStatusTimelineItem struct {
-	// id         entity.Id
 	combinedId entity.CombinedId
 	Author     identity.Interface
 	UnixTime   timestamp.Timestamp

entities/bug/op_set_title.go 🔗

@@ -30,7 +30,6 @@ func (op *SetTitleOperation) Apply(snapshot *Snapshot) {
 
 	id := op.Id()
 	item := &SetTitleTimelineItem{
-		id:         id,
 		combinedId: entity.CombineIds(snapshot.Id(), id),
 		Author:     op.Author(),
 		UnixTime:   timestamp.Timestamp(op.UnixTime),
@@ -70,7 +69,6 @@ func NewSetTitleOp(author identity.Interface, unixTime int64, title string, was
 }
 
 type SetTitleTimelineItem struct {
-	id         entity.Id
 	combinedId entity.CombinedId
 	Author     identity.Interface
 	UnixTime   timestamp.Timestamp
@@ -78,10 +76,6 @@ type SetTitleTimelineItem struct {
 	Was        string
 }
 
-func (s SetTitleTimelineItem) Id() entity.Id {
-	return s.id
-}
-
 func (s SetTitleTimelineItem) CombinedId() entity.CombinedId {
 	return s.combinedId
 }

misc/random_bugs/create_random_bugs.go 🔗

@@ -144,7 +144,7 @@ func paragraphs() string {
 }
 
 func comment(b bug.Interface, p identity.Interface, timestamp int64) {
-	_, _ = bug.AddComment(b, p, timestamp, paragraphs(), nil, nil)
+	_, _, _ = bug.AddComment(b, p, timestamp, paragraphs(), nil, nil)
 }
 
 func title(b bug.Interface, p identity.Interface, timestamp int64) {

termui/termui.go 🔗

@@ -239,7 +239,7 @@ func addCommentWithEditor(bug *cache.BugCache) error {
 	if err == input.ErrEmptyMessage {
 		ui.msgPopup.Activate(msgPopupErrorTitle, "Empty message, aborting.")
 	} else {
-		_, err := bug.AddComment(text.Cleanup(message))
+		_, _, err := bug.AddComment(text.Cleanup(message))
 		if err != nil {
 			return err
 		}