Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

Restrict iterating over every workitem field for reorder #2261

Closed
wants to merge 11 commits into from
Closed

Restrict iterating over every workitem field for reorder #2261

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4eb8e89
restrict iterating over every workitem field for reorder
DhritiShikhar Aug 28, 2018
3ba0022
removes unnecessary lines
DhritiShikhar Aug 28, 2018
cac3e02
adds test
DhritiShikhar Aug 28, 2018
5fde63f
adds gopkg.lock
DhritiShikhar Aug 28, 2018
5a14ac8
Merge remote-tracking branch 'upstream/master' into ooe-optimize-101
DhritiShikhar Sep 3, 2018
e6c9abd
Merge remote-tracking branch 'upstream/master' into ooe-optimize-101
DhritiShikhar Sep 5, 2018
2242279
uses update instead of updateColumns
DhritiShikhar Sep 5, 2018
adccee4
removes unnecessary assert statement
DhritiShikhar Sep 5, 2018
7e9b1ad
Merge branch 'master' into ooe-optimize-101
DhritiShikhar Sep 5, 2018
ef2d346
Merge branch 'master' into ooe-optimize-101
DhritiShikhar Sep 11, 2018
a37aad6
Merge branch 'master' into ooe-optimize-101
DhritiShikhar Sep 11, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions Gopkg.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

31 changes: 31 additions & 0 deletions controller/workitem_blackbox_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -559,6 +559,37 @@ func (s *WorkItemSuite) TestUpdateWorkitemWithoutReorder() {
assert.Equal(s.T(), wi.Data.Attributes[workitem.SystemOrder], updated.Data.Attributes[workitem.SystemOrder])
}

// TestReorderDoesNotUpdateAnyOtherWIFields tests that no other workitem fields are updated while reorder
func (s *WorkItemSuite) TestReorderDoesNotUpdateOtherWIFields() {
fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment())

// create workitems
payload := minimumRequiredCreateWithTypeAndSpace(fxt.WorkItemTypes[0].ID, fxt.Spaces[0].ID)
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

_, workitem2 := test.CreateWorkitemsCreated(s.T(), s.svc.Context, s.svc, s.workitemsCtrl, *payload.Data.Relationships.Space.Data.ID, &payload)

titleNotUpdated := "This Title Should Not Be Updated"
workitem1.Data.Attributes[workitem.SystemTitle] = titleNotUpdated

payload2 := minimumRequiredReorderPayload()

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

workitem.DirectionAbove is workitem.DirectionType

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)
}

_, reorderedList := test.ReorderWorkitemsOK(s.T(), s.svc.Context, s.svc, s.workitemsCtrl, fxt.Spaces[0].ID, &payload2) // Returns the workitems which are reordered

require.Len(s.T(), reorderedList.Data, 1)
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])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed here adccee4

}

func (s *WorkItemSuite) TestCreateWorkItemWithoutContext() {
// given
s.svc = goa.New("TestCreateWorkItemWithoutContext-Service")
Expand Down
18 changes: 1 addition & 17 deletions workitem/workitem_repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -544,24 +544,8 @@ func (r *GormWorkItemRepository) Reorder(ctx context.Context, spaceID uuid.UUID,
default:
return &wi, nil
}
res.Version = res.Version + 1
res.Type = wi.Type
res.Fields = Fields{}

res.ExecutionOrder = order

for fieldName, fieldDef := range wiType.Fields {
if fieldDef.ReadOnly {
continue
}
fieldValue := wi.Fields[fieldName]
var err error
res.Fields[fieldName], err = fieldDef.ConvertToModel(fieldName, fieldValue)
if err != nil {
return nil, errors.NewBadParameterError(fieldName, fieldValue)
}
}
tx = tx.Where("Version = ?", wi.Version).Save(&res)
tx = tx.Model(&res).Where("Version = ?", wi.Version).UpdateColumns(WorkItemStorage{ExecutionOrder: order, Version: res.Version + 1})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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?

if err := tx.Error; err != nil {
return nil, errors.NewInternalError(ctx, err)
}
Expand Down