Adds WIT parameter in TrackerQuery payload #2360

DhritiShikhar · 2018-12-03T07:03:54Z

[1] Adds WIT parameter in TrackerQuery Payload

Task -> #2361

remoteworkitem/trackerquery_repository_test.go

baijum · 2018-12-03T08:43:21Z

controller/trackerquery.go

+			WorkItemTypeID: ctx.Payload.Data.Relationships.BaseType.Data.ID,
+		}
+		if ctx.Payload.Data.ID != nil {
+			trackerQuery.ID = *ctx.Payload.Data.ID


I wonder whether the UUID should be validated.

@baijum are you talking about a check if the id contains string or uuid?

Check whether the value of *ctx.Payload.Data.ID is real UUID (It's not a string type)

I mean what if the value is some random string. It will reach the database layer and fail. How about failing early?

@baijum At design level itself its specified that the value should be a UUID (https://github.com/fabric8-services/fabric8-wit/blob/master/design/tracker_queries.go#L13)

design/tracker_queries.go

codecov · 2018-12-03T09:04:56Z

Codecov Report

Merging #2360 into master will increase coverage by 0.09%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #2360      +/-   ##
==========================================
+ Coverage   67.65%   67.75%   +0.09%     
==========================================
  Files         182      182              
  Lines       17908    17922      +14     
==========================================
+ Hits        12116    12143      +27     
+ Misses       4622     4610      -12     
+ Partials     1170     1169       -1

Impacted Files	Coverage Δ
remoteworkitem/remoteworkitem.go	`78.87% <ø> (ø)`	⬆️
remoteworkitem/trackerquery.go	`100% <ø> (ø)`	⬆️
migration/migration.go	`77.93% <100%> (+0.07%)`	⬆️
controller/trackerquery.go	`61.47% <100%> (+3.97%)`	⬆️
remoteworkitem/scheduler.go	`60.97% <80%> (+7.31%)`	⬆️
remoteworkitem/trackerquery_repository.go	`66.15% <83.33%> (+3.19%)`	⬆️
remoteworkitem/trackeritem_repository.go	`74% <91.66%> (ø)`	⬆️
remoteworkitem/github.go	`100% <0%> (+22.85%)`	⬆️

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 5c95c04...9fd13d7. Read the comment docs.

kwk · 2018-12-03T08:56:56Z

controller/trackerquery.go

@@ -248,6 +254,9 @@ func validateCreateTrackerQueryPayload(ctx *app.CreateTrackerqueryContext) error
 	if *ctx.Payload.Data.Relationships.Space.Data.ID == uuid.Nil {
 		return errors.NewBadParameterError("SpaceID", nil).Expected("not nil")
 	}
+	if ctx.Payload.Data.Relationships.BaseType.Data.ID == uuid.Nil {


You hat the same check on line 163, no?

@kwk the check here is for validating create action. The check on line 163 is for validating update action.

However, there are some duplicate checks for update action. Made changes here bba6209

controller/trackerquery_blackbox_test.go

design/tracker_queries.go

remoteworkitem/scheduler.go

migration/sql-files/111-add-wit-to-trackerquery.sql

kwk · 2018-12-03T09:15:03Z

remoteworkitem/trackeritem_repository.go

 	if err != nil {
 		return nil, ConversionError{simpleError{message: fmt.Sprintf("Error mapping to local work item: %s", err.Error())}}
 	}
-	workItem, err := lookupIdentities(ctx, db, remoteWorkItem, providerType, spaceID)
+	workItem, err := lookupIdentities(ctx, db, remoteWorkItem, tq)
 	if err != nil {
 		return nil, InternalError{simpleError{message: fmt.Sprintf("Error bind assignees: %s", err.Error())}}
 	}
 	return upsert(ctx, db, *workItem)
 }

 // lookupIdentities looks up creator and assignee remote identities to local identities (already existing or to be created)


Please create a better documentation line. It makes no sense to me :)

The function did much more than lookupIdentities. Changed the function name here - 6648376

@DhritiShikhar // setLocalWorkItemFields sets fields of local workitem isn't really a good explanation either.

@kwk renamed here
6ddb69d

remoteworkitem/trackerquery.go

baijum

LGTM

…ries table

kwk · 2018-12-11T09:16:50Z

controller/trackerquery_blackbox_test.go

-	assert.Equal(t, "is:open", updated.Data.Attributes.Query)
-	assert.Equal(t, "* * * * * *", updated.Data.Attributes.Schedule)
+	_, updated := test.UpdateTrackerqueryOK(s.T(), svc.Context, svc, trackerQueryCtrl, tqr.Data.ID.String(), &payload2)
+	assert.NotNil(s.T(), tqr)


This needs to be a require.NotNil

kwk · 2018-12-11T09:25:02Z

controller/trackerquery_blackbox_test.go

+		svc, _, trackerQueryCtrl := s.SecuredController()
+
+		tqpayload := newCreateTrackerQueryPayload(fxt.Spaces[0].ID, fxt.Trackers[0].ID, fxt.WorkItemTypes[0].ID)
+		test.CreateTrackerqueryBadRequest(t, svc.Context, svc, trackerQueryCtrl, &tqpayload)


Don't these test methods have return values that you can check?

kwk · 2018-12-11T09:25:54Z

controller/trackerquery_blackbox_test.go

 		invalidID := uuid.Nil
 		tqpayload.Data.ID = &invalidID
 		test.CreateTrackerqueryBadRequest(t, svc.Context, svc, trackerQueryCtrl, &tqpayload)
 	})
 }

-func newCreateTrackerQueryPayload(spaceID uuid.UUID, trackerID uuid.UUID) app.CreateTrackerqueryPayload {
+func (s *TestTrackerQueryREST) TestInvalidWITinTrackerQuery() {


Why not have one func (s *TestTrackerQuerySuite) TestCreate() and then test for all cases in there?

kwk · 2018-12-11T11:57:35Z

remoteworkitem/trackeritem_repository.go

@@ -77,7 +77,7 @@ func lookupIdentities(ctx context.Context, db *gorm.DB, remoteWorkItem RemoteWor
 			}
 			creatorLogin := fieldValue.(string)
 			creatorProfileURL := remoteWorkItem.Fields[remoteCreatorProfileURL].(string)
-			identity, err := identityRepository.Lookup(context.Background(), creatorLogin, creatorProfileURL, providerType)
+			identity, err := identityRepository.Lookup(context.Background(), creatorLogin, creatorProfileURL, tq.TrackerType)


I'm a bit sceptical for why and how the tracker type corresponds to the provider type in the identity repo. Can you please explain what both are and if it is only by accident that you can use the tracker type for it?

kwk · 2018-12-11T11:58:27Z

remoteworkitem/trackeritem_test.go

@@ -22,40 +23,44 @@ func (test *TestTrackerItemRepository) TestUpload() {
 	resource.Require(t, resource.Database)

 	test.DB.Exec(`DELETE FROM "tracker_items"`)


This seems a bit drastic. Why do you need to delete all tracker items? And why can't you use the db test suite to delete all that it has created after a test?

This is an old piece of code. remoteworkitem package needs a lot of refactoring.

baijum

LGTM

This PR needs to be merged for further work on remoteworkitems. I will address other remarks in another PR.

DhritiShikhar added 2 commits December 3, 2018 02:30

Adds WIT to TQ payload

f0fc73c

Merge remote-tracking branch 'upstream/master' into tq-wit-1

65b3643

DhritiShikhar requested review from baijum and kwk December 3, 2018 07:03

golangcibot reviewed Dec 3, 2018

View reviewed changes

remoteworkitem/trackerquery_repository_test.go Show resolved Hide resolved

remoteworkitem/trackerquery_repository_test.go Show resolved Hide resolved

DhritiShikhar force-pushed the tq-wit-1 branch from c1513f2 to 02dca24 Compare December 3, 2018 08:28

adds check for WIT; adds tests

e4f054c

DhritiShikhar force-pushed the tq-wit-1 branch from 02dca24 to e4f054c Compare December 3, 2018 08:42

DhritiShikhar added the enhancement label Dec 3, 2018

adds require.NoError

21ee0b5

baijum reviewed Dec 3, 2018

View reviewed changes

kwk suggested changes Dec 3, 2018

View reviewed changes

DhritiShikhar added 8 commits December 3, 2018 14:58

removes duplicate checks for TrackerQuery update action

bba6209

replaces rest with s

0524fee

uses app.NewSpaceRelation(....)

5c24211

changes baseType to workItemType in TrackerQuery payload

4e28161

removes unnecessary spacetemplate creation

7eafcf3

uses app.NewSpaceRelation(....) for space

936cfc9

Adds work_item_type_id to existing tracker_queries; Adds tests;

68498a6

Merge remote-tracking branch 'upstream/master' into tq-wit-1

3646533

baijum approved these changes Dec 3, 2018

View reviewed changes

DhritiShikhar added 8 commits December 3, 2018 20:33

Adds struct tags

72bc5c5

changes name of a function

6648376

removes t:=s.T() line controller level test

071fe92

deletes old data from tracker_queries; adds wit column in tracker_que…

97f2247

…ries table

removes unnecessary code from migration black box tests

6f48605

Merge branch 'master' into tq-wit-1

e358263

renames function

6ddb69d

Merge remote-tracking branch 'origin/tq-wit-1' into tq-wit-1

92a0e2c

DhritiShikhar added 2 commits December 10, 2018 18:25

Merge remote-tracking branch 'upstream/master' into tq-wit-1

f5c9755

Merge branch 'master' into tq-wit-1

fc1a8ba

kwk previously requested changes Dec 11, 2018

View reviewed changes

DhritiShikhar added 4 commits December 17, 2018 12:44

changes assert to require

1b36616

adds return values for tests

09a0e75

puts tests under one function

a59a40b

Merge remote-tracking branch 'upstream/master' into tq-wit-1

9fd13d7

baijum approved these changes Dec 21, 2018

View reviewed changes

DhritiShikhar merged commit ac1460f into fabric8-services:master Dec 21, 2018

jarifibrahim mentioned this pull request Feb 21, 2019

Update core.yaml openshiftio/saas-openshiftio#1226

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adds WIT parameter in TrackerQuery payload #2360

Adds WIT parameter in TrackerQuery payload #2360

DhritiShikhar commented Dec 3, 2018 •

edited

Loading

baijum Dec 3, 2018

DhritiShikhar Dec 3, 2018

baijum Dec 3, 2018

baijum Dec 3, 2018

DhritiShikhar Dec 3, 2018

codecov bot commented Dec 3, 2018 •

edited

Loading

kwk Dec 3, 2018

DhritiShikhar Dec 3, 2018

kwk Dec 3, 2018

DhritiShikhar Dec 3, 2018

kwk Dec 5, 2018

DhritiShikhar Dec 11, 2018

baijum left a comment

kwk Dec 11, 2018

DhritiShikhar Dec 17, 2018

kwk Dec 11, 2018

DhritiShikhar Dec 17, 2018

kwk Dec 11, 2018

DhritiShikhar Dec 17, 2018

kwk Dec 11, 2018

kwk Dec 11, 2018

DhritiShikhar Dec 17, 2018

baijum left a comment

		@@ -22,40 +23,44 @@ func (test *TestTrackerItemRepository) TestUpload() {
		resource.Require(t, resource.Database)

		test.DB.Exec(`DELETE FROM "tracker_items"`)

Adds WIT parameter in TrackerQuery payload #2360

Adds WIT parameter in TrackerQuery payload #2360

Conversation

DhritiShikhar commented Dec 3, 2018 • edited Loading

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

codecov bot commented Dec 3, 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

baijum 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baijum left a comment

Choose a reason for hiding this comment

DhritiShikhar commented Dec 3, 2018 •

edited

Loading

codecov bot commented Dec 3, 2018 •

edited

Loading