Do not generate event log for reorder event #2196

jarifibrahim · 2018-07-30T10:00:14Z

Part of #2192

The reorder event updated the workitem such that the fields with kind list get a value of [] (empty list) and when the next Patch request is made these empty values are removed which creates an event log.
This PR fixes this issue.

codecov-io · 2018-07-30T10:19:48Z

Codecov Report

❗ No coverage uploaded for pull request base (master@1ca1860). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master    #2196   +/-   ##
=========================================
  Coverage          ?   69.38%           
=========================================
  Files             ?      173           
  Lines             ?    16393           
  Branches          ?        0           
=========================================
  Hits              ?    11375           
  Misses            ?     3924           
  Partials          ?     1094

Impacted Files	Coverage Δ
workitem/workitem_repository.go	`68.66% <100%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1ca1860...052f980. Read the comment docs.

jarifibrahim · 2018-07-30T10:31:32Z

workitem/workitem_repository.go

@@ -545,6 +545,21 @@ func (r *GormWorkItemRepository) Reorder(ctx context.Context, spaceID uuid.UUID,
 			continue
 		}
 		fieldValue := wi.Fields[fieldName]
+		// Remove Assignee/Labels/Boardcolumns (fields with kind list) if they do not have a value
+		if fieldDef.Type.GetKind() == "list" {


I cannot use fieldDef.Type.GetKind() == workitem.KindList because it would cause a cyclic dependency.

But you can use fieldDef.Type.GetKind() == KindList. I mean you're already in the workitem package. So no need to import it again ;)

See ee41af2

I should've seen that earlier :)

jarifibrahim · 2018-07-30T10:31:46Z

workitem/workitem_repository.go

@@ -587,7 +602,8 @@ func (r *GormWorkItemRepository) Save(ctx context.Context, spaceID uuid.UUID, up
 		}
 		fieldValue := updatedWorkItem.Fields[fieldName]
 		var err error
-		if fieldName == SystemAssignees || fieldName == SystemLabels || fieldName == SystemBoardcolumns {
+		// Remove fields with type list if there are no elements (eg: Assignee, Labels)
+		if fieldDef.Type.GetKind() == "list" {


I cannot use fieldDef.Type.GetKind() == workitem.KindList because it would cause a cyclic dependency.

But you can use fieldDef.Type.GetKind() == KindList. I mean you're already in the workitem package. So no need to import it again ;)

See ee41af2

I should've seen that earlier :)

kwk

Please wait with this PR until my changes to the field types are being made.

kwk · 2018-07-30T10:45:09Z

workitem/workitem_repository.go

+		if fieldDef.Type.GetKind() == "list" {
+			switch fieldValue.(type) {
+			case []string:
+				if len(fieldValue.([]string)) == 0 {


This will change soon and you will no longer be able to case to []string.

kwk

@jarifibrahim it looks like the changes you've made only proof that a reorder has no effect on the event list. Is that true?

kwk · 2018-08-23T13:47:13Z

workitem/event/event_repository_blackbox_test.go

@@ -379,6 +379,14 @@ func (s *eventRepoBlackBoxTest) TestList() {
 		assert.Equal(t, 2, c)
 	})

+	s.T().Run("reorder event shouldn't be logged", func(t *testing.T) {
+		fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(2))
+		s.wiRepo.Reorder(s.Ctx, fxt.Spaces[0].ID, workitem.DirectionBelow, &fxt.WorkItems[0].ID, *fxt.WorkItems[1], fxt.Identities[0].ID)


Can you please create 3 events and then move item with ID 1 to down or up? I don't know straight away if item 0 is at the top or the bottom of a list. And I bet there will not be an event if the order has no effect, say because the item is already at the very end of the list.

I hope this makes it more clear 74feeca

kwk · 2018-08-23T13:47:52Z

workitem/workitem_repository.go

@@ -554,6 +554,21 @@ func (r *GormWorkItemRepository) Reorder(ctx context.Context, spaceID uuid.UUID,
 			continue
 		}
 		fieldValue := wi.Fields[fieldName]
+		// Remove Assignee/Labels/Boardcolumns (fields with kind list) if they do not have a value


Why is that needed here?

Added the comment just for clarity.

kwk · 2018-08-23T13:49:05Z

workitem/workitem_repository.go

@@ -594,7 +609,8 @@ func (r *GormWorkItemRepository) Save(ctx context.Context, spaceID uuid.UUID, up
 		}
 		fieldValue := updatedWorkItem.Fields[fieldName]
 		var err error
-		if fieldName == SystemAssignees || fieldName == SystemLabels || fieldName == SystemBoardcolumns {
+		// Remove fields with type list if there are no elements (eg: Assignee, Labels)
+		if fieldDef.Type.GetKind() == KindList {


Thank you for making this change. Yesterday I've talked about this exact line with @xcoulon and we temporarily changed it to the exact line.

You're welcome

jarifibrahim · 2018-08-27T07:12:01Z

it looks like the changes you've made only proof that a reorder has no effect on the event list. Is that true?

No. We were generating invalid events when a workitem was reordered. With this change https://github.com/fabric8-services/fabric8-wit/pull/2196/files#diff-55f2da878ed2fcb42e5373010f02ff92R558 we will no longer have invalid events.

kwk · 2018-08-27T07:23:56Z

workitem/workitem_repository.go

+		if fieldDef.Type.GetKind() == KindList {
+			switch fieldValue.(type) {
+			case []string:
+				if len(fieldValue.([]string)) == 0 {


Try switch t := fieldValue.(type) { and then use if len(t) == 0 { here so you can avoid the type assertion.

Done in 075a72c

jarifibrahim · 2018-08-30T06:35:24Z

The actual issue will be fixed by #2261 .
Closing this.

Do not generate event log for reorder event

feea930

jarifibrahim self-assigned this Jul 30, 2018

jarifibrahim requested review from baijum, kwk, DhritiShikhar, michaelkleinhenz and tinakurian July 30, 2018 10:00

jarifibrahim commented Jul 30, 2018

View reviewed changes

kwk suggested changes Jul 30, 2018

View reviewed changes

jarifibrahim added 2 commits July 30, 2018 16:27

Use KindList instead of 'list'

ee41af2

Merge branch 'master' into issue-2192

d2e713f

jarifibrahim mentioned this pull request Aug 21, 2018

Workitem timeline shows empty values openshiftio/openshift.io#3737

Closed

Merge branch 'master' into issue-2192

389e093

kwk suggested changes Aug 23, 2018

View reviewed changes

fixup

74feeca

kwk reviewed Aug 27, 2018

View reviewed changes

jarifibrahim added 2 commits August 27, 2018 12:56

update switch block

075a72c

Merge branch 'master' into issue-2192

052f980

jarifibrahim closed this Aug 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not generate event log for reorder event #2196

Do not generate event log for reorder event #2196

jarifibrahim commented Jul 30, 2018

codecov-io commented Jul 30, 2018 •

edited

Loading

jarifibrahim Jul 30, 2018

kwk Jul 30, 2018

jarifibrahim Jul 30, 2018

jarifibrahim Jul 30, 2018

kwk Jul 30, 2018

jarifibrahim Jul 30, 2018

kwk left a comment

kwk Jul 30, 2018

kwk left a comment

kwk Aug 23, 2018

jarifibrahim Aug 27, 2018

kwk Aug 23, 2018

jarifibrahim Aug 27, 2018

kwk Aug 23, 2018

jarifibrahim Aug 27, 2018

jarifibrahim commented Aug 27, 2018

kwk Aug 27, 2018 •

edited

Loading

jarifibrahim Aug 27, 2018

jarifibrahim commented Aug 30, 2018

Do not generate event log for reorder event #2196

Do not generate event log for reorder event #2196

Conversation

jarifibrahim commented Jul 30, 2018

codecov-io commented Jul 30, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarifibrahim commented Aug 27, 2018

kwk Aug 27, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jarifibrahim commented Aug 30, 2018

codecov-io commented Jul 30, 2018 •

edited

Loading

kwk Aug 27, 2018 •

edited

Loading