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 9 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
30 changes: 30 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,36 @@ 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.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
21 changes: 4 additions & 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,11 @@ 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).Select("execution_order", "version").Updates(WorkItemStorage{
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't this have a where clause?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because I pass .Model instead of .Where

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

ExecutionOrder: order,
Version: res.Version + 1,
})
if err := tx.Error; err != nil {
return nil, errors.NewInternalError(ctx, err)
}
Expand Down