Restrict iterating over every workitem field for reorder #2261

DhritiShikhar · 2018-08-28T10:30:56Z

fixes openshiftio/openshift.io#4259

alien-ike · 2018-08-28T10:30:57Z

Ike Plugins (test-keeper)

Thank you @DhritiShikhar for this contribution!

It seems that this PR already contains some added or changed tests. Good job!

For more information please head over to official documentation. You can find there how to configure the plugin.

codecov-io · 2018-08-28T10:55:12Z

Codecov Report

Merging #2261 into master will increase coverage by 1.84%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #2261      +/-   ##
==========================================
+ Coverage   69.53%   71.37%   +1.84%     
==========================================
  Files         176      176              
  Lines       16670    17818    +1148     
==========================================
+ Hits        11591    12718    +1127     
+ Misses       3956     3940      -16     
- Partials     1123     1160      +37

Impacted Files	Coverage Δ
workitem/workitem_repository.go	`71.33% <100%> (+3.64%)`	⬆️
controller/user.go	`34.04% <0%> (-4.99%)`	⬇️
migration/migration.go	`61.11% <0%> (-0.46%)`	⬇️
controller/space_iterations.go	`84.78% <0%> (+0.51%)`	⬆️
controller/area.go	`92.78% <0%> (+0.52%)`	⬆️
controller/iteration.go	`81.12% <0%> (+2.79%)`	⬆️
workitem/expression_compiler.go	`87.29% <0%> (+3.79%)`	⬆️
path/path.go	`71.42% <0%> (+4.76%)`	⬆️
controller/space.go	`74.67% <0%> (+4.83%)`	⬆️
... and 4 more

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 a8fab53...a37aad6. Read the comment docs.

DhritiShikhar · 2018-08-29T06:36:44Z

[test]

jarifibrahim · 2018-08-29T07:09:47Z

controller/workitem_blackbox_test.go

+	originalTitle := "Original Title"
+	payload.Data.Attributes[workitem.SystemTitle] = originalTitle
+	payload.Data.Attributes[workitem.SystemState] = workitem.SystemStateClosed
+	_, workitem1 := test.CreateWorkitemsCreated(s.T(), s.svc.Context, s.svc, s.workitemsCtrl, *payload.Data.Relationships.Space.Data.ID, &payload)


You can use test fixture to create the initial work item. That would remove all the code from line 566 to 576

@jarifibrahim The test fixture gives me a workitem of workitem.Workitem type but I need app.WorkItem to create reorder payload.

jarifibrahim · 2018-08-29T07:15:30Z

workitem/workitem_repository.go

-		}
-	}
-	tx = tx.Where("Version = ?", wi.Version).Save(&res)
+	tx = tx.Model(&res).Where("Version = ?", wi.Version).UpdateColumns(WorkItemStorage{ExecutionOrder: order, Version: res.Version + 1})


Can't we use update() instead of updateColumns ?

@DhritiShikhar @jarifibrahim

When reading the gorm docs I found this:

Above updating operations will perform the model's BeforeUpdate, AfterUpdate method, update its UpdatedAt timestamp, save its Associations when updaing, if you don't want to call them, you could use UpdateColumn, UpdateColumns.

(source: http://doc.gorm.io/crud.html#update)

We certainly want to call all those functions so UpdateColumn and UpdateColumns are not an option for us to use.

Probably the safest way to update the columns is using this syntax:

tx = tx.Model(WorkItemStorage{Version:wi.Version}).Select("version").Select("execution_order").Updates(map[string]interface{}{ "execution_order": order, "version": res.Version + 1, })

Notice that by using Model() as I did we don't need the Where function anymore.

What do you think?

@kwk I am not completely sure how this works

tx = tx.Model(WorkItemStorage{Version:wi.Version}).Select("version").Select("execution_order").Updates(map[string]interface{}{ "execution_order": order, "version": res.Version + 1, })

I couldn't find examples like tx.Model(WorkItemStorage{Version:wi.Version}) . Is it equivalent to select .. where version = wi.Version?

jarifibrahim · 2018-08-29T07:16:54Z

controller/workitem_blackbox_test.go

+	dataArray = append(dataArray, workitem1.Data)
+	payload2.Data = dataArray
+	payload2.Position.ID = workitem2.Data.ID // Position.ID specifies the workitem ID above or below which the workitem(s) should be placed
+	payload2.Position.Direction = string(workitem.DirectionAbove)


workitem.DirectionAbove is already a string, you don't need the string(...)

workitem.DirectionAbove is workitem.DirectionType

Yes, but workitem.DirectionType is of type string
See https://play.golang.org/p/Nov7b0810R-

package main import ( "fmt" ) func main() { type direction string const foo direction = "above" bar := foo fmt.Println(foo, bar) }

jarifibrahim · 2018-08-29T07:17:19Z

controller/workitem_blackbox_test.go

+
+	payload2 := minimumRequiredReorderPayload()
+
+	var dataArray []*app.WorkItem // dataArray contains the workitem(s) that have to be reordered


This can also be removed when fxt.Workitems[] is used.

@jarifibrahim The test fixture gives me a workitem of workitem.Workitem type but I need app.WorkItem to create reorder payload.

jarifibrahim · 2018-08-29T07:18:42Z

controller/workitem_blackbox_test.go

+	assert.Equal(s.T(), workitem1.Data.Attributes["version"].(int)+1, reorderedList.Data[0].Attributes["version"])
+	assert.Equal(s.T(), *workitem1.Data.ID, *reorderedList.Data[0].ID)
+	assert.NotEqual(s.T(), titleNotUpdated, reorderedList.Data[0].Attributes[workitem.SystemTitle])
+	assert.Equal(s.T(), originalTitle, reorderedList.Data[0].Attributes[workitem.SystemTitle])


We don't need both, this and https://github.com/fabric8-services/fabric8-wit/pull/2261/files#diff-98914f859911696f3cdd98dd289d00faR589 . One of them should be sufficient.

fixed here adccee4

kwk · 2018-08-29T08:24:25Z

workitem/workitem_repository.go

-		}
-	}
-	tx = tx.Where("Version = ?", wi.Version).Save(&res)
+	tx = tx.Model(&res).Where("Version = ?", wi.Version).UpdateColumns(WorkItemStorage{ExecutionOrder: order, Version: res.Version + 1})


@DhritiShikhar @jarifibrahim

When reading the gorm docs I found this:

Above updating operations will perform the model's BeforeUpdate, AfterUpdate method, update its UpdatedAt timestamp, save its Associations when updaing, if you don't want to call them, you could use UpdateColumn, UpdateColumns.

(source: http://doc.gorm.io/crud.html#update)

We certainly want to call all those functions so UpdateColumn and UpdateColumns are not an option for us to use.

Probably the safest way to update the columns is using this syntax:

tx = tx.Model(WorkItemStorage{Version:wi.Version}).Select("version").Select("execution_order").Updates(map[string]interface{}{ "execution_order": order, "version": res.Version + 1, })

Notice that by using Model() as I did we don't need the Where function anymore.

What do you think?

DhritiShikhar · 2018-09-05T09:12:35Z

@kwk

I use updates instead of updateColumns

tx = tx.Model(&res).Select("execution_order", "version").Updates(WorkItemStorage{
		ExecutionOrder: order,
		Version:        res.Version + 1,
	})

tx.Model(WorkItemStorage{Version:wi.Version}) doesnot update execution order so still using tx.Model(&res)

DhritiShikhar · 2018-09-05T10:20:18Z

[test]

DhritiShikhar · 2018-09-05T11:53:50Z

[test]

jarifibrahim · 2018-09-05T13:06:01Z

workitem/workitem_repository.go

-		}
-	}
-	tx = tx.Where("Version = ?", wi.Version).Save(&res)
+	tx = tx.Model(&res).Select("execution_order", "version").Updates(WorkItemStorage{


Why doesn't this have a where clause?

because I pass .Model instead of .Where

@DhritiShikhar can you please show the .Debug(). output of this query? I think Gorm only checks for the primary key if you do .Model(&res).. And the version is not the primary field.

restrict iterating over every workitem field for reorder

4eb8e89

alien-ike added the work-in-progress label Aug 28, 2018

removes unnecessary lines

3ba0022

DhritiShikhar added 2 commits August 28, 2018 17:23

adds test

cac3e02

adds gopkg.lock

5fde63f

DhritiShikhar changed the title ~~WIP: restrict iterating over every workitem field for reorder~~ Restrict iterating over every workitem field for reorder Aug 28, 2018

alien-ike removed the work-in-progress label Aug 28, 2018

DhritiShikhar mentioned this pull request Aug 28, 2018

refactor reorder tests #2262

Open

DhritiShikhar requested review from jarifibrahim and kwk August 29, 2018 06:57

jarifibrahim suggested changes Aug 29, 2018

View reviewed changes

kwk suggested changes Aug 29, 2018

View reviewed changes

jarifibrahim mentioned this pull request Aug 30, 2018

Do not generate event log for reorder event #2196

Closed

DhritiShikhar added 3 commits September 3, 2018 11:40

Merge remote-tracking branch 'upstream/master' into ooe-optimize-101

5a14ac8

Merge remote-tracking branch 'upstream/master' into ooe-optimize-101

e6c9abd

uses update instead of updateColumns

2242279

removes unnecessary assert statement

adccee4

Merge branch 'master' into ooe-optimize-101

7e9b1ad

jarifibrahim reviewed Sep 5, 2018

View reviewed changes

DhritiShikhar added 2 commits September 11, 2018 12:01

Merge branch 'master' into ooe-optimize-101

ef2d346

Merge branch 'master' into ooe-optimize-101

a37aad6

DhritiShikhar closed this Aug 1, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Restrict iterating over every workitem field for reorder #2261

Restrict iterating over every workitem field for reorder #2261

DhritiShikhar commented Aug 28, 2018 •

edited

Loading

alien-ike commented Aug 28, 2018 •

edited

Loading

codecov-io commented Aug 28, 2018 •

edited

Loading

DhritiShikhar commented Aug 29, 2018

jarifibrahim Aug 29, 2018

DhritiShikhar Sep 3, 2018

jarifibrahim Aug 29, 2018

kwk Aug 29, 2018

jarifibrahim Sep 5, 2018

jarifibrahim Aug 29, 2018

DhritiShikhar Sep 5, 2018

jarifibrahim Sep 5, 2018

jarifibrahim Aug 29, 2018

DhritiShikhar Sep 3, 2018

jarifibrahim Aug 29, 2018

DhritiShikhar Sep 5, 2018

kwk Aug 29, 2018

DhritiShikhar commented Sep 5, 2018

DhritiShikhar commented Sep 5, 2018

DhritiShikhar commented Sep 5, 2018

jarifibrahim Sep 5, 2018

DhritiShikhar Sep 11, 2018

kwk Sep 11, 2018


		payload2 := minimumRequiredReorderPayload()

		var dataArray []*app.WorkItem // dataArray contains the workitem(s) that have to be reordered

Restrict iterating over every workitem field for reorder #2261

Restrict iterating over every workitem field for reorder #2261

Conversation

DhritiShikhar commented Aug 28, 2018 • edited Loading

alien-ike commented Aug 28, 2018 • edited Loading

Ike Plugins (test-keeper)

codecov-io commented Aug 28, 2018 • edited Loading

Codecov Report

DhritiShikhar commented Aug 29, 2018

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

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

DhritiShikhar commented Sep 5, 2018

DhritiShikhar commented Sep 5, 2018

DhritiShikhar commented Sep 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DhritiShikhar commented Aug 28, 2018 •

edited

Loading

alien-ike commented Aug 28, 2018 •

edited

Loading

codecov-io commented Aug 28, 2018 •

edited

Loading