Merge pull request #631 from MichaelMure/comment-edit

Michael Muré created 4 years ago

Fix ID string in order to find correct bug instance on comment edit operation

Change summary

bug/comment.go              |  7 ++++---
bug/op_add_comment.go       |  5 ++---
bug/op_create.go            |  5 ++---
bug/op_create_test.go       |  6 ++----
bug/op_edit_comment.go      | 16 +++++++++++-----
bug/op_edit_comment_test.go | 15 ++++++---------
bug/snapshot.go             |  5 ++---
bug/timeline.go             |  5 +++--
8 files changed, 32 insertions(+), 32 deletions(-)

Detailed changes

bug/comment.go

@@ -11,6 +11,8 @@ import (
 
 // Comment represent a comment in a Bug
 type Comment struct {
+	// id should be the result of entity.CombineIds with the Bug id and the id
+	// of the Operation that created the comment
 	id      entity.Id
 	Author  identity.Interface
 	Message string
@@ -24,9 +26,8 @@ type Comment struct {
 // Id return the Comment identifier
 func (c Comment) Id() entity.Id {
 	if c.id == "" {
-		// simply panic as it would be a coding error
-		// (using an id of an identity not stored yet)
-		panic("no id yet")
+		// simply panic as it would be a coding error (no id provided at construction)
+		panic("no id")
 	}
 	return c.id
 }

bug/op_add_comment.go

@@ -31,9 +31,8 @@ func (op *AddCommentOperation) Apply(snapshot *Snapshot) {
 	snapshot.addActor(op.Author_)
 	snapshot.addParticipant(op.Author_)
 
-	commentId := entity.CombineIds(snapshot.Id(), op.Id())
 	comment := Comment{
-		id:       commentId,
+		id:       entity.CombineIds(snapshot.Id(), op.Id()),
 		Message:  op.Message,
 		Author:   op.Author_,
 		Files:    op.Files,
@@ -43,7 +42,7 @@ func (op *AddCommentOperation) Apply(snapshot *Snapshot) {
 	snapshot.Comments = append(snapshot.Comments, comment)
 
 	item := &AddCommentTimelineItem{
-		CommentTimelineItem: NewCommentTimelineItem(commentId, comment),
+		CommentTimelineItem: NewCommentTimelineItem(comment),
 	}
 
 	snapshot.Timeline = append(snapshot.Timeline, item)

bug/op_create.go

@@ -55,9 +55,8 @@ func (op *CreateOperation) Apply(snapshot *Snapshot) {
 
 	snapshot.Title = op.Title
 
-	commentId := entity.CombineIds(snapshot.Id(), op.Id())
 	comment := Comment{
-		id:       commentId,
+		id:       entity.CombineIds(snapshot.Id(), op.Id()),
 		Message:  op.Message,
 		Author:   op.Author_,
 		UnixTime: timestamp.Timestamp(op.UnixTime),
@@ -69,7 +68,7 @@ func (op *CreateOperation) Apply(snapshot *Snapshot) {
 
 	snapshot.Timeline = []TimelineItem{
 		&CreateTimelineItem{
-			CommentTimelineItem: NewCommentTimelineItem(commentId, comment),
+			CommentTimelineItem: NewCommentTimelineItem(comment),
 		},
 	}
 }

bug/op_create_test.go

@@ -30,10 +30,8 @@ func TestCreate(t *testing.T) {
 	id := create.Id()
 	require.NoError(t, id.Validate())
 
-	commentId := entity.CombineIds(create.Id(), create.Id())
-
 	comment := Comment{
-		id:       commentId,
+		id:       entity.CombineIds(create.Id(), create.Id()),
 		Author:   rene,
 		Message:  "message",
 		UnixTime: timestamp.Timestamp(create.UnixTime),
@@ -51,7 +49,7 @@ func TestCreate(t *testing.T) {
 		CreateTime:   create.Time(),
 		Timeline: []TimelineItem{
 			&CreateTimelineItem{
-				CommentTimelineItem: NewCommentTimelineItem(commentId, comment),
+				CommentTimelineItem: NewCommentTimelineItem(comment),
 			},
 		},
 	}

bug/op_edit_comment.go

@@ -34,12 +34,12 @@ func (op *EditCommentOperation) Apply(snapshot *Snapshot) {
 	// Todo: currently any message can be edited, even by a different author
 	// crypto signature are needed.
 
-	snapshot.addActor(op.Author_)
+	// Recreate the Comment Id to match on
+	commentId := entity.CombineIds(snapshot.Id(), op.Target)
 
 	var target TimelineItem
-
 	for i, item := range snapshot.Timeline {
-		if item.Id() == op.Target {
+		if item.Id() == commentId {
 			target = snapshot.Timeline[i]
 			break
 		}
@@ -51,7 +51,7 @@ func (op *EditCommentOperation) Apply(snapshot *Snapshot) {
 	}
 
 	comment := Comment{
-		id:       op.Target,
+		id:       commentId,
 		Message:  op.Message,
 		Files:    op.Files,
 		UnixTime: timestamp.Timestamp(op.UnixTime),
@@ -62,12 +62,18 @@ func (op *EditCommentOperation) Apply(snapshot *Snapshot) {
 		target.Append(comment)
 	case *AddCommentTimelineItem:
 		target.Append(comment)
+	default:
+		// somehow, the target matched on something that is not a comment
+		// we make the op a no-op
+		return
 	}
 
+	snapshot.addActor(op.Author_)
+
 	// Updating the corresponding comment
 
 	for i := range snapshot.Comments {
-		if snapshot.Comments[i].Id() == op.Target {
+		if snapshot.Comments[i].Id() == commentId {
 			snapshot.Comments[i].Message = op.Message
 			snapshot.Comments[i].Files = op.Files
 			break

bug/op_edit_comment_test.go

@@ -24,14 +24,12 @@ func TestEdit(t *testing.T) {
 	create := NewCreateOp(rene, unix, "title", "create", nil)
 	create.Apply(&snapshot)
 
-	id1 := create.Id()
-	require.NoError(t, id1.Validate())
+	require.NoError(t, create.Id().Validate())
 
 	comment1 := NewAddCommentOp(rene, unix, "comment 1", nil)
 	comment1.Apply(&snapshot)
 
-	id2 := comment1.Id()
-	require.NoError(t, id2.Validate())
+	require.NoError(t, comment1.Id().Validate())
 
 	// add another unrelated op in between
 	setTitle := NewSetTitleOp(rene, unix, "edited title", "title")
@@ -40,10 +38,9 @@ func TestEdit(t *testing.T) {
 	comment2 := NewAddCommentOp(rene, unix, "comment 2", nil)
 	comment2.Apply(&snapshot)
 
-	id3 := comment2.Id()
-	require.NoError(t, id3.Validate())
+	require.NoError(t, comment2.Id().Validate())
 
-	edit := NewEditCommentOp(rene, unix, snapshot.Comments[0].Id(), "create edited", nil)
+	edit := NewEditCommentOp(rene, unix, create.Id(), "create edited", nil)
 	edit.Apply(&snapshot)
 
 	require.Len(t, snapshot.Timeline, 4)
@@ -54,7 +51,7 @@ func TestEdit(t *testing.T) {
 	require.Equal(t, snapshot.Comments[1].Message, "comment 1")
 	require.Equal(t, snapshot.Comments[2].Message, "comment 2")
 
-	edit2 := NewEditCommentOp(rene, unix, snapshot.Comments[1].Id(), "comment 1 edited", nil)
+	edit2 := NewEditCommentOp(rene, unix, comment1.Id(), "comment 1 edited", nil)
 	edit2.Apply(&snapshot)
 
 	require.Len(t, snapshot.Timeline, 4)
@@ -65,7 +62,7 @@ func TestEdit(t *testing.T) {
 	require.Equal(t, snapshot.Comments[1].Message, "comment 1 edited")
 	require.Equal(t, snapshot.Comments[2].Message, "comment 2")
 
-	edit3 := NewEditCommentOp(rene, unix, snapshot.Comments[2].Id(), "comment 2 edited", nil)
+	edit3 := NewEditCommentOp(rene, unix, comment2.Id(), "comment 2 edited", nil)
 	edit3.Apply(&snapshot)
 
 	require.Len(t, snapshot.Timeline, 4)

bug/snapshot.go

@@ -29,9 +29,8 @@ type Snapshot struct {
 // Return the Bug identifier
 func (snap *Snapshot) Id() entity.Id {
 	if snap.id == "" {
-		// simply panic as it would be a coding error
-		// (using an id of a bug not stored yet)
-		panic("no id yet")
+		// simply panic as it would be a coding error (no id provided at construction)
+		panic("no id")
 	}
 	return snap.id
 }

bug/timeline.go

@@ -26,6 +26,7 @@ type CommentHistoryStep struct {
 
 // CommentTimelineItem is a TimelineItem that holds a Comment and its edition history
 type CommentTimelineItem struct {
+	// id should be the same as in Comment
 	id        entity.Id
 	Author    identity.Interface
 	Message   string
@@ -35,9 +36,9 @@ type CommentTimelineItem struct {
 	History   []CommentHistoryStep
 }
 
-func NewCommentTimelineItem(ID entity.Id, comment Comment) CommentTimelineItem {
+func NewCommentTimelineItem(comment Comment) CommentTimelineItem {
 	return CommentTimelineItem{
-		id:        ID,
+		id:        comment.id,
 		Author:    comment.Author,
 		Message:   comment.Message,
 		Files:     comment.Files,