Space id for wi, wit and tracker query #900

hectorj2f · 2017-02-28T15:58:17Z

related to: #685 and #766

This PR uses as base code the following PR #852

Conflicts: work-item-link-blackbox_test.go work-item-link-type.go workitem/link/type.go workitem/link/type_repository.go

Conflicts: migration/migration.go

aslakknutsen · 2017-03-01T02:01:49Z

workitem/workitem_repository.go

@@ -22,7 +24,7 @@ type WorkItemRepository interface {
 	Load(ctx context.Context, ID string) (*app.WorkItem, error)
 	Save(ctx context.Context, wi app.WorkItem) (*app.WorkItem, error)
 	Delete(ctx context.Context, ID string) error
-	Create(ctx context.Context, typeID string, fields map[string]interface{}, creator uuid.UUID) (*app.WorkItem, error)
+	Create(ctx context.Context, typeID string, fields map[string]interface{}, creator, spaceID uuid.UUID) (*app.WorkItem, error)


I think space_id should be after context as it's the highest level.

Conflicts: controller/search_blackbox_test.go design/spaces.go migration/migration.go

hectorj2f · 2017-03-03T10:46:51Z

@aslakknutsen @kwk I am still dealing with some tests failing... I ping you when it is ready

Conflicts: controller/iteration_test.go controller/workitemtype.go design/media_types.go design/user_types.go design/workitems.go migration/migration.go search/search_repository_blackbox_test.go test/work_item_repository.go workitem/undoable_wi_repo.go workitem/undoable_wi_type_repo.go workitem/workitem.go workitem/workitem_repository.go workitem/workitemtype_repository.go workitem/workitemtype_repository_blackbox_test.go workitem/workitemtype_repository_whitebox_test.go

Conflicts: controller/work-item-comments_test.go migration/migration.go search/search_repository_blackbox_test.go search/search_repository_whitebox_test.go test/work_item_repository.go workitem/workitem_repository.go workitem/workitem_repository_blackbox_test.go

xcoulon · 2017-03-06T08:27:30Z

controller/trackerquery_test.go

@@ -106,11 +100,28 @@ func TestUpdateTrackerQuery(t *testing.T) {
 		t.Errorf("Id should be %s, but is %s", tqresult.ID, tqr.ID)
 	}

+	spaceType := "spaces"


shouldn't you make a constant out of this variable ? I see it is also used in the getCreateTrackerQueryPayload function below.

yeah, I'll make it 👍.

xcoulon · 2017-03-06T08:28:10Z

controller/trackerquery_test.go

-		TrackerID: result.ID,
-	}
+	tqpayload := getCreateTrackerQueryPayload(result.ID)
+
 	_, trackerquery := test.CreateTrackerqueryCreated(t, nil, nil, &tqController, &tqpayload)
 	_, created := test.ShowTrackerqueryOK(t, nil, nil, &tqController, trackerquery.ID)
 	if created != nil && created.ID != trackerquery.ID {


what about using require.True(...) instead ?

xcoulon

This looks good, but I have a question about the data of the existing work items: I saw a lot of changes like this one in the tests:

-	c.Data.Relationships = &app.WorkItemRelationships{
-		BaseType: &app.RelationBaseType{
-			Data: &app.BaseTypeData{
-				Type: "workitemtypes",
-				ID:   workitem.SystemBug,
-			},
+	c.Data.Relationships.BaseType = &app.RelationBaseType{
+		Data: &app.BaseTypeData{
+			Type: "workitemtypes",
+			ID:   workitem.SystemBug,
 		},

and I wonder if this has any impact on the existing work item data and if so, how do you handle the change in the database ?

kwk · 2017-03-06T08:33:52Z

controller/trackerquery_blackbox_test.go

+	tqPayload := fmt.Sprintf(`{
+		"query": "abcdefgh",
+		"schedule": "1 1 * * * *",
+		"trackerID": "%s",


Please use %[1]s, %[2]s, %[3]s instead of just %s. It's easier to understand then where each parameter goes.

They are just three input arguments :)... I can understand that if I have n > 5. Anyway I will check that

The second and third parameter are the same, so why even bother passing in the third? Instead just repeat %[2]s ;)

kwk · 2017-03-06T08:37:34Z

controller/work-item-link-blackbox_test.go

+	_, space := test.CreateSpaceCreated(s.T(), s.workItemSvc.Context, s.workItemSvc, s.spaceCtrl, createSpacePayload)
+	require.NotNil(s.T(), space)
+	s.userSpaceID = *space.Data.ID
+	fmt.Printf("Created link space with ID: %s\n", *space.Data.ID)


Please use s.T().Logf("Created link space with ID: %s\n", *space.Data.ID) instead.

kwk · 2017-03-06T08:41:34Z

controller/work-item-link-blackbox_test.go

+	// Create a work item link space
+	createSpacePayload := CreateSpacePayload("test-space", "description")
+	_, space := test.CreateSpaceCreated(s.T(), s.workItemSvc.Context, s.workItemSvc, s.spaceCtrl, createSpacePayload)
+	require.NotNil(s.T(), space)


I think this check is not needed as test.CreateSpaceCreated sort of guarantees that the space it not nil because it calls the Validate() method of each media type before returning the result. I know, there're a lot of places in the code where we do check for not nil on returned objects, but I think we don't have to do that. @aslakknutsen agree?

Well, if this is done in called function then it isn't need to repeat code. I could change that for the space but I won't do those changes in the other lines

kwk · 2017-03-06T08:44:26Z

controller/work-item-link-blackbox_test.go

@@ -289,7 +314,7 @@ func CreateWorkItem(workItemType satoriuuid.UUID, title string) *app.CreateWorki
 }

 // CreateWorkItemLinkType defines a work item link type
-func CreateWorkItemLinkType(name string, sourceTypeID, targetTypeID, categoryID, spaceID satoriuuid.UUID) *app.CreateWorkItemLinkTypePayload {
+func CreateWorkItemLinkType(name string, sourceTypeID, targetTypeID, categoryID, spaceID uuid.UUID) *app.CreateWorkItemLinkTypePayload {


Next time, please keep this refactoring / renaming work out of your main PR.

Well, I agree with that cause I always ask about it. But this change was already started in many other files where I had to solve conflicts. So I had to resolve them, consequently and following the same procedure, I decided to apply those changes to the files I touched.
Maybe we should try to avoid those partial changes in the future.

kwk · 2017-03-06T08:45:33Z

controller/work-item-link-type-blackbox_test.go

+	createSpacePayload := CreateSpacePayload("test-space", "description")
+	_, space := test.CreateSpaceCreated(s.T(), s.svcSpace.Context, s.svcSpace, s.spaceCtrl, createSpacePayload)
+	s.spaceID = space.Data.ID
+	require.NotNil(s.T(), space)


If you insist to DO test that space is not nil, then please do so before dereferencing it one line above ;)

Hehehe good one :)! I don't insist I just simply followed the code structured as done in many other places.

We dereference before checking for nil in other places as well?

I changed in one more place

kwk · 2017-03-06T08:55:53Z

migration/migration.go

+		}, "space %s will be created", id)
+		_, err := spaceRepo.Create(ctx, newSpace)
+		if err != nil {
+			return errs.WithStack(err)


Please use return errs.Wrapf(err, "failed to create space %s", id) instead. It is more expressive. The WithStack calls have been put into the code automatically at one point and cannot be considered a very good practice for newly added code.

kwk · 2017-03-06T08:57:09Z

migration/sql-files/039-add-space-id-wi-wit-tq.sql

@@ -0,0 +1,27 @@
+-- Alter the table work_items
+ALTER TABLE work_items ADD space_id uuid DEFAULT '{{index . 0}}' NOT NULL;;


So this is okay, but only because you have a DEFAULT. Otherwise the NOT NULL part would have to be SET later.

Exactly, this is done like that on purpose

kwk · 2017-03-06T09:00:43Z

migration/sql-files/039-add-space-id-wi-wit-tq.sql

+ALTER TABLE work_items ADD space_id uuid DEFAULT '{{index . 0}}' NOT NULL;;
+-- Once we set the values to the default. We drop this default constraint
+ALTER TABLE work_items ALTER space_id DROP DEFAULT;
+ALTER TABLE work_items ADD FOREIGN KEY (space_id) REFERENCES spaces(id) ON DELETE CASCADE;


I prefer to add the FK when adding the column:

ALTER TABLE work_items ADD space_id uuid DEFAULT '{{index . 0}}' NOT NULL REFERENCES spaces(id) ON DELETE CASCADE;

but yours is fine, too.

kwk · 2017-03-06T09:06:34Z

remoteworkitem/trackerquery_repository.go

 	tq2 := app.TrackerQuery{
 		ID:        strconv.FormatUint(tq.ID, 10),
 		Query:     query,
 		Schedule:  schedule,
-		TrackerID: tracker}
+		TrackerID: tracker,
+		Relationships: &app.TrackerQueryRelationships{


It looks like the following code block is repeated in each repository method and I wonder if you can outsource it into a function.

You wonder well ;)

kwk · 2017-03-06T09:07:49Z

remoteworkitem/trackerquery_repository_test.go


-		trackerqueries2, _ := queryRepo.List(context.Background())
+		trackerqueries2, _ := queryRepo.List(ctx)


What exactly is the benefit of not using context.Background().

@kwk context.Backgroun is empty while ctx is initialized with some parameters that will later use.

kwk · 2017-03-06T09:12:11Z

and I wonder if this has any impact on the existing work item data and if so, how do you handle the change in the database ?

@xcoulon see the SQL migration script in this PR. @hectorj2f adds a new column to each of the affected tables and references the space there by using a foreign key.

codecov-io · 2017-03-06T09:13:09Z

Codecov Report

Merging #900 into master will decrease coverage by -0.22%.
The diff coverage is 77.98%.

@@            Coverage Diff             @@
##           master     #900      +/-   ##
==========================================
- Coverage   69.49%   69.28%   -0.22%     
==========================================
  Files          90       93       +3     
  Lines        6874     7009     +135     
==========================================
+ Hits         4777     4856      +79     
- Misses       1650     1707      +57     
+ Partials      447      446       -1

Impacted Files	Coverage Δ
rest/url.go	`100% <ø> (ø)`	✅
workitem/undoable_wi_repo.go	`0% <0%> (ø)`
main.go	`0% <0%> (ø)`	✅
workitem/undoable_wi_type_repo.go	`0% <0%> (ø)`
controller/workitemtype.go	`74.54% <100%> (ø)`	✅
workitem/workitem.go	`77.27% <100%> (+2.27%)`	✅
workitem/workitemtype_repository.go	`84.23% <100%> (+0.44%)`	✅
workitem/workitem_repository.go	`71.98% <100%> (+0.12%)`	✅
space/space.go	`70.48% <100%> (+2.09%)`	✅
controller/workitem.go	`91.37% <100%> (+2.48%)`	✅
... and 25 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 2765006...460ae4f. Read the comment docs.

xcoulon · 2017-03-06T09:16:58Z

ok, thanks for confirming. And after reading my comment again, I think that the change I was referring to made no sense with regards to the data migration. Sorry about that.

Conflicts: migration/migration.go

hectorj2f · 2017-03-06T11:51:57Z

@xcoulon @kwk I addressed your comments :). Let's see if the test pass successfully.

kwk · 2017-03-06T12:15:33Z

controller/iteration.go

@@ -215,7 +215,7 @@ func ConvertIteration(request *goa.RequestData, itr *iteration.Iteration, additi
 		Relationships: &app.IterationRelations{
 			Space: &app.RelationGeneric{


Why not use NewSpaceRelation here as well?

Because we have two type of objects for the relation, relationSpace and relationGeneric. I could unify this in a different PR.

kwk

Looking much better now.

kwk · 2017-03-06T13:03:23Z

workitem/workitemtype_repository_whitebox_test.go

@@ -101,10 +111,21 @@ func TestConvertTypeFromModels(t *testing.T) {
 					},
 				},
 			},
+			Relationships: &app.WorkItemTypeRelationships{
+				Space: &app.RelationSpaces{


Why not use NewSpaceRelation() here?

I missed that! thx

kwk

LGTM Except some minor things.

Conflicts: controller/workitem_blackbox_test.go workitem/undoable_wi_repo.go workitem/undoable_wi_type_repo.go

hectorj2f and others added 19 commits February 22, 2017 16:17

Initial commit

a56bffa

Merge branch master into space_id_wilt

e797767

Added space creation in the existing wi tests

9797405

Adapted migration to set the request context

be6e2d5

Adapted to use DELETE CASCADE

a7f2263

Added space self URL

fc3a9b0

Use secured service to create spaces

308b2db

Filled description field for spaces

0969397

Changed userSpaceID to uuid type and added tests to check include list

af5c1f5

Added test and fixed some types

8f9e63d

Merge remote master branch into space_id_wilt

7049998

Re-organized my sql script to be in order

ca1038a

Merge remote master branch into space_id_wilt

4510e7d

Conflicts: work-item-link-blackbox_test.go work-item-link-type.go workitem/link/type.go workitem/link/type_repository.go

Avoid referring to space by name. Use uuid identifier

fb6a810

Merge remote-tracking branch 'master' into space_id_wilt

cb9302c

Merge remote-tracking branch 'master' into space_id_wilt

d0a70d6

Merge remote-tracking branch 'master' into space_id_wilt

0e07c52

Conflicts: migration/migration.go

Merge remote-tracking branch 'master' into space_id_wilt

3239ea3

Conflicts: migration/migration.go

Add space_id to wi and wit

e915fd9

hectorj2f self-assigned this Feb 28, 2017

hectorj2f requested review from aslakknutsen and kwk February 28, 2017 15:58

aslakknutsen suggested changes Mar 1, 2017

View reviewed changes

hectorj2f changed the title ~~Space id for wi and wit~~ Space id for wi, wit and tracker query Mar 2, 2017

Merge remote-tracking branch 'master' into space_id_wi_wit

0a23ab2

Conflicts: controller/search_blackbox_test.go design/spaces.go migration/migration.go

hectorj2f changed the title ~~Space id for wi, wit and tracker query~~ [WIP] Space id for wi, wit and tracker query Mar 3, 2017

hectorj2f added 3 commits March 3, 2017 16:26

Fixed tests

d9c9957

Removed unnecessary locks

d461d31

hectorj2f changed the title ~~[WIP] Space id for wi, wit and tracker query~~ Space id for wi, wit and tracker query Mar 6, 2017

xcoulon reviewed Mar 6, 2017

View reviewed changes

kwk suggested changes Mar 6, 2017

View reviewed changes

Merge branch 'master' into space_id_wi_wit

9f72319

hectorj2f added 3 commits March 6, 2017 12:28

Merge remote-tracking branch 'master' into space_id_wi_wit

d7908d0

Conflicts: migration/migration.go

Added a locker for the db population function and some code polishing

413183e

Merge branch 'space_id_wi_wit' of hectorj2f into space_id_wi_wit

3ee44d9

Conflicts: migration/migration.go

kwk reviewed Mar 6, 2017

View reviewed changes

kwk approved these changes Mar 6, 2017

View reviewed changes

hectorj2f added 2 commits March 6, 2017 14:19

Addressed minor comments

9a7de8d

Merge remote-tracking branch 'master' into space_id_wi_wit

ab7b2ba

Conflicts: controller/workitem_blackbox_test.go workitem/undoable_wi_repo.go workitem/undoable_wi_type_repo.go

kwk approved these changes Mar 6, 2017

View reviewed changes

hectorj2f added 2 commits March 6, 2017 15:47

Fixed wrong function name

c8df855

Updated migration version to 40

271965b

aslakknutsen approved these changes Mar 6, 2017

View reviewed changes

hectorj2f and others added 4 commits March 6, 2017 16:17

Fixed error tracker.ID is a string object

5c9d2e9

Merge remote-tracking branch 'master' into space_id_wi_wit

4f3dc45

Adapted new test

accbfef

gofmt code

460ae4f

hectorj2f merged commit 0454ce9 into fabric8-services:master Mar 6, 2017

hectorj2f deleted the space_id_wi_wit branch March 6, 2017 16:18

		@@ -0,0 +1,27 @@
		-- Alter the table work_items
		ALTER TABLE work_items ADD space_id uuid DEFAULT '{{index . 0}}' NOT NULL;;


		trackerqueries2, _ := queryRepo.List(context.Background())
		trackerqueries2, _ := queryRepo.List(ctx)

		@@ -215,7 +215,7 @@ func ConvertIteration(request goa.RequestData, itr iteration.Iteration, additi
		Relationships: &app.IterationRelations{
		Space: &app.RelationGeneric{

Space id for wi, wit and tracker query #900

Space id for wi, wit and tracker query #900

Conversation

hectorj2f commented Feb 28, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hectorj2f commented Mar 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xcoulon left a comment • edited

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

hectorj2f Mar 6, 2017 • edited

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kwk commented Mar 6, 2017

codecov-io commented Mar 6, 2017 • edited

Codecov Report

xcoulon commented Mar 6, 2017

hectorj2f commented Mar 6, 2017

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

kwk left a comment

Choose a reason for hiding this comment

hectorj2f commented Feb 28, 2017 •

edited

xcoulon left a comment •

edited

hectorj2f Mar 6, 2017 •

edited

codecov-io commented Mar 6, 2017 •

edited