Introduce /api/workitems/:ID/relationships/links #525

kwk · 2016-11-29T15:56:44Z

This change will introduce the endpoint /api/workitems/:ID/relationships/links for work item links that lives under the work item endpoint and automatically filters work item links associated with the work item on the backend. You can also use this endpoint as you would use the /api/workitemlinks/ endpoint.

Also, this change will introduce the included top-level JSONAPI element for responses of /api/workitemlinks/ that return work item links or arrays of work item links. Currently this included element will contain only objects of type work item link type.

Currently the UI has to do the filtering of links on a particular work item. This should be no longer needed.

Checks

In the WorkItemRelationshipsLinksController we perform the following
checks for the listed actions.

Create action

We check that the current work item (:id) does exist.
Check that the source ID of the link is the same as the current work
item ID (:id).
If no source is specified we pre-fill the source field of the payload
with the current work item ID from the URL. This is for convenience.

Delete action

We check that the work item (:linkid) does exist.
Check that the source ID of the link to be deleted is the same as the
current work item ID (:id).

List action

No checks are done

Show action

We check that the work item (:linkid) does exist.
Check that the source ID or target ID of the link to be shown is the
same as the current work item ID (:id).

Update action

We check that the work item (:linkid) does exist.
Check that the source ID of the link to be updated is the same as the
current work item ID (:id).
Check that the source ID of the update payload is the same as the
current work item ID (:id).

Current coverage is 66.37% (diff: 26.66%)

Merging #525 into master will decrease coverage by 0.36%

@@             master       #525   diff @@
==========================================
  Files            62         63     +1   
  Lines          3286       3316    +30   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2193       2201     +8   
- Misses          857        878    +21   
- Partials        236        237     +1

Powered by Codecov. Last update 9bd5c82...b55718a

aslakknutsen · 2016-11-29T21:44:50Z

There's only one action for this resource: list (in http: GET).

Are you considering POST to this relation as well for Adding links to the WorkItem?
http://jsonapi.org/format/#crud-updating-relationships

aslakknutsen · 2016-11-29T21:48:24Z

Should we consider this PR the fix for #475 ?

kwk · 2016-11-30T12:11:23Z

Are you considering POST to this relation as well for Adding links to the WorkItem?
http://jsonapi.org/format/#crud-updating-relationships

I am considering POST but this PR is supposed to stay small and make its way into master quicker than the last one ;)

Should we consider this PR the fix for #475 ?

It is a part #475 but the workitems endpoint needs to include the relationships as well. That I want to do in another PR as well.

aslakknutsen · 2016-11-30T12:29:45Z

I am considering POST but this PR is supposed to stay small and make its way into master quicker than the last one ;)

Sure. hehe. Just didn't see any Task|Issue related to it, was just wondering.

codecov-io · 2016-11-30T16:50:16Z

Current coverage is 68.40% (diff: 69.29%)

Merging #525 into master will increase coverage by 0.02%

@@             master       #525   diff @@
==========================================
  Files            70         71     +1   
  Lines          3644       3754   +110   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2492       2568    +76   
- Misses          889        915    +26   
- Partials        263        271     +8

Powered by Codecov. Last update 90158fd...64c90c7

aslakknutsen · 2016-12-06T00:17:04Z

[test]

kwk · 2016-12-06T08:08:13Z

[test]

This change will introduce the endpoint `/api/workitems/:ID/relationships/links` for work item links that lives under the work item endpoint and automatically filters work item links associated with the work item on the backend. You can also use this endpoint as you would use the `/api/workitemlinks/` endpoint. Also, this change will introduce the `included` top-level JSONAPI element for responses of `/api/workitemlinks/` that return work item links or arrays of work item links. Currently this `included` element will contain only objects of type work item link type. Currently the UI has to do the filtering of links on a particular work item. This should be no longer needed. == Checks In the WorkItemRelationshipsLinksController we perform the following checks for the listed actions. === Create action * We check that the current work item (:id) does exist. * Check that the source ID of the link is the same as the current work item ID (:id). * If no source is specified we pre-fill the source field of the payload with the current work item ID from the URL. This is for convenience. === Delete action * We check that the work item (:linkid) does exist. * Check that the source ID of the link to be deleted is the same as the current work item ID (:id). === List action * No checks are done === Show action * We check that the work item (:linkid) does exist. * Check that the source ID or target ID of the link to be shown is the same as the current work item ID (:id). === Update action * We check that the work item (:linkid) does exist. * Check that the source ID of the link to be updated is the same as the current work item ID (:id). * Check that the source ID of the update payload is the same as the current work item ID (:id). See also fabric8-services#307

aslakknutsen · 2016-12-08T23:53:28Z

I would like to see include of source/target workitems (where source/target != this) as well, to avoid the UI having to do n backend calls per link to display the basic information like Title, State, ID.

Doesn't have to be part of this PR tho. Just in general :)

kwk · 2016-12-09T07:57:27Z

@aslakknutsen thanks for approving.

I would like to see include of source/target workitems (where source/target != this) as well, to avoid the UI having to do n backend calls per link to display the basic information like Title, State, ID.

Doesn't have to be part of this PR tho. Just in general :)

Yes, that's what I intended with this issue: "Include related resources by default in all actions and endpoints that deal with work item links." #533

tsmaeder · 2016-12-09T08:35:09Z

design/resources.go

 	})
 })
+
+// listWorkItemLinks defines the list action for endpoints that return an array


Unfortunately, this refactoring makes it impossible to see if you have made any changes to this code. Generally, I would do such mechanical but sweeping changes in a separate PR.

Sorry about that. Will try to not do it again.

tsmaeder · 2016-12-09T08:44:55Z

workitem/workitem.go

+func ParseWorkItemIDToUint64(wiIDStr string) (uint64, error) {
+	wiID, err := strconv.ParseUint(wiIDStr, 10, 64)
+	if err != nil {
+		return 0, errors.NewBadParameterError("work item ID", wiIDStr)


I don't think "BadParameter" is correct. In the API, the ID is just a string, so if it is "abcd", that's not a wrong request, just an id that we happen to know we won't find in the db.

Agreed. Fixed in local branch.

tsmaeder · 2016-12-09T08:47:09Z

design/resources.go

@@ -535,7 +535,7 @@ var _ = a.Resource("work-item-link-type", func() {
 		a.Routing(
 			a.GET("/:id"),
 		)
-		a.Description("Retrieve work item link type (as JSONAPI) for the given ID.")
+		a.Description("Retrieve work item link type (as JSONAPI) for the given link ID.")


Link id or link type id?

@tsmaeder good catch. It is supposed to be "link type id". Fixed in local branch.

tsmaeder · 2016-12-09T08:52:38Z

workitem/workitem_repository.go

+// CheckWorkItemExists returns nil if no work item ID string is given or if work
+// item ID string could be converted into a number and looked up in the
+// database; otherwise it returns an error.
+func CheckWorkItemExists(db *gorm.DB, wiIDStr string) (*WorkItem, error) {


If you add this as a method in the gorm work item repo, you don't need to pass in the db.

I found I can use WorkItemRepository.LoadFromDB instead. It does the same thing.

tsmaeder · 2016-12-09T08:53:15Z

workitem/workitem_repository.go

+// item ID string could be converted into a number and looked up in the
+// database; otherwise it returns an error.
+func CheckWorkItemExists(db *gorm.DB, wiIDStr string) (*WorkItem, error) {
+	if db == nil {


It's a programming error to pass in a nil db here. I don't think we should check invariants of the code, just let it crash.

tsmaeder · 2016-12-09T08:56:46Z

workitem/link/link-repository.go

-	if db.Error != nil {
-		return nil, db.Error
+	db := r.db
+	if wiIDStr == nil {


I don't like dispatching on a nil parameter. These are really two different methods. We can reuse the common parts of the code in different ways.
At the API level, it's fine to introduce parameters as optional first in order to facilitate migration.

Okay, I agree. Will outsource into separate function.

tsmaeder · 2016-12-09T09:57:24Z

design/resources.go

+var _ = a.Resource("work-item-relationships-links", func() {
+	a.BasePath("/relationships/links")
+	a.Parent("workitem")
+	a.Action("show", showWorkItemLink)


The more I think about it, the less I think treating link object like a simple relationship makes sense. When I create a link, it is necessarily always related to two work items (and vice versa). A link is not a relationship between two work items. It is a resource object in it's own right. We cannot remove or add relationships here without deleting or creating new resource objects. That is not what jsonapi relationships are.
Therefore, I don't think having create, update and delete here makes sense, only list (and perhaps show).

Agreed. Thank you for sharing your thoughts. @aslakknutsen I'd like to remove the CUD operations from the /api/workitems/:id/relationships/links/:linkid endpoint. Such operations have to made on the actual resource endpoint /api/work-item-links/:linkid.

@tsmaeder

According to @tsmaeder 's comments on fabric8-services#525 (after it was being merged), this change includes some cleanups. * Return NotFoundError instead of BadParameterError when parsing a WI ID * Split WorkItemLinkRepository.List into List and ListByWorkItemID * Removed this generic List method from the WorkItemLinkRepository List(ctx context.Context, wiIDStr *string) (*app.WorkItemLinkArray, error) and replaced it with these less generic methods: List(ctx context.Context) (*app.WorkItemLinkArray, error) ListByWorkItemID(ctx context.Context, wiIDStr string) (*app.WorkItemLinkArray, error) * Let crash if db is nil * No need for workitem.CheckWorkItemExists() * There's a WorkItemRepository.LoadFromDB that basically does the same thing. * Remove obsolete comment

@tsmaeder

According to @tsmaeder 's comments on fabric8-services#525 (after it was being merged), this change includes some cleanups. * Return NotFoundError instead of BadParameterError when parsing a WI ID * Split WorkItemLinkRepository.List into List and ListByWorkItemID * Removed this generic List method from the WorkItemLinkRepository List(ctx context.Context, wiIDStr *string) (*app.WorkItemLinkArray, error) and replaced it with these less generic methods: List(ctx context.Context) (*app.WorkItemLinkArray, error) ListByWorkItemID(ctx context.Context, wiIDStr string) (*app.WorkItemLinkArray, error) * Let crash if db is nil * No need for workitem.CheckWorkItemExists() * There's a WorkItemRepository.LoadFromDB that basically does the same thing. * Remove obsolete comment

@tsmaeder

According to @tsmaeder 's comments on #525 (after it was being merged), this change includes some cleanups. * Return NotFoundError instead of BadParameterError when parsing a WI ID * Split WorkItemLinkRepository.List into List and ListByWorkItemID * Removed this generic List method from the WorkItemLinkRepository List(ctx context.Context, wiIDStr *string) (*app.WorkItemLinkArray, error) and replaced it with these less generic methods: List(ctx context.Context) (*app.WorkItemLinkArray, error) ListByWorkItemID(ctx context.Context, wiIDStr string) (*app.WorkItemLinkArray, error) * Let crash if db is nil * No need for workitem.CheckWorkItemExists() * There's a WorkItemRepository.LoadFromDB that basically does the same thing. * Remove obsolete comment

kwk added jsonapi 😴 REST labels Nov 29, 2016

kwk self-assigned this Nov 29, 2016

michaelkleinhenz mentioned this pull request Nov 29, 2016

Add Work Item Association by Id [13] [13] [13] [5] #307

Closed

21 tasks

kwk force-pushed the show-links-for-wi branch from b55718a to 285f530 Compare November 30, 2016 16:49

kwk changed the title ~~WIP introduce /api/workitem/:ID/relationships/links~~ WIP introduce GET /api/workitem/:ID/relationships/links API Nov 30, 2016

kwk changed the title ~~WIP introduce GET /api/workitem/:ID/relationships/links API~~ WIP Introduce List action (GET) on /api/workitem/:ID/relationships/links Nov 30, 2016

kwk force-pushed the show-links-for-wi branch from 2799659 to 70fb0d0 Compare December 2, 2016 09:37

kwk changed the title ~~WIP Introduce List action (GET) on /api/workitem/:ID/relationships/links~~ WIP Introduce /api/workitem/:ID/relationships/links Dec 2, 2016

kwk changed the title ~~WIP Introduce /api/workitem/:ID/relationships/links~~ WIP Introduce /api/workitems/:ID/relationships/links Dec 2, 2016

kwk changed the title ~~WIP Introduce /api/workitems/:ID/relationships/links~~ Introduce /api/workitems/:ID/relationships/links Dec 2, 2016

kwk changed the title ~~Introduce /api/workitems/:ID/relationships/links~~ WIP Introduce /api/workitems/:ID/relationships/links Dec 2, 2016

kwk force-pushed the show-links-for-wi branch from 4d25300 to 3895561 Compare December 2, 2016 17:59

kwk changed the title ~~WIP Introduce /api/workitems/:ID/relationships/links~~ Introduce /api/workitems/:ID/relationships/links Dec 2, 2016

kwk force-pushed the show-links-for-wi branch 2 times, most recently from 803911b to e15cc61 Compare December 6, 2016 11:35

kwk changed the title ~~Introduce /api/workitems/:ID/relationships/links~~ WIP Introduce /api/workitems/:ID/relationships/links Dec 6, 2016

kwk mentioned this pull request Dec 6, 2016

WIP Cleanup linking #552

Closed

kwk force-pushed the show-links-for-wi branch from e15cc61 to d71c3e0 Compare December 6, 2016 15:13

sanbornsen mentioned this pull request Dec 7, 2016

Removing workitem filter logic from FE fabric8-ui/fabric8-planner#381

Closed

kwk force-pushed the show-links-for-wi branch from d71c3e0 to 64c90c7 Compare December 8, 2016 07:56

kwk requested review from aslakknutsen and tsmaeder December 8, 2016 08:04

kwk changed the title ~~WIP Introduce /api/workitems/:ID/relationships/links~~ Introduce /api/workitems/:ID/relationships/links Dec 8, 2016

Merge branch 'master' into show-links-for-wi

1f342a5

aslakknutsen approved these changes Dec 8, 2016

View reviewed changes

aslakknutsen merged commit 3d7da8c into fabric8-services:master Dec 9, 2016

kwk deleted the show-links-for-wi branch December 9, 2016 07:57

tsmaeder reviewed Dec 9, 2016

View reviewed changes

kwk mentioned this pull request Dec 9, 2016

Followup on #525 #563

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce /api/workitems/:ID/relationships/links #525

Introduce /api/workitems/:ID/relationships/links #525

kwk commented Nov 29, 2016 •

edited

codecov-io commented Nov 29, 2016

aslakknutsen commented Nov 29, 2016

aslakknutsen commented Nov 29, 2016

kwk commented Nov 30, 2016

aslakknutsen commented Nov 30, 2016

codecov-io commented Nov 30, 2016 •

edited

aslakknutsen commented Dec 6, 2016

kwk commented Dec 6, 2016

aslakknutsen commented Dec 8, 2016

kwk commented Dec 9, 2016

tsmaeder Dec 9, 2016

kwk Dec 9, 2016

tsmaeder Dec 9, 2016

kwk Dec 9, 2016

tsmaeder Dec 9, 2016

kwk Dec 9, 2016

tsmaeder Dec 9, 2016

kwk Dec 9, 2016

tsmaeder Dec 9, 2016

tsmaeder Dec 9, 2016

kwk Dec 9, 2016

tsmaeder Dec 9, 2016

kwk Dec 9, 2016

Introduce /api/workitems/:ID/relationships/links #525

Introduce /api/workitems/:ID/relationships/links #525

Conversation

kwk commented Nov 29, 2016 • edited

Checks

Create action

Delete action

List action

Show action

Update action

codecov-io commented Nov 29, 2016

Current coverage is 66.37% (diff: 26.66%)

aslakknutsen commented Nov 29, 2016

aslakknutsen commented Nov 29, 2016

kwk commented Nov 30, 2016

aslakknutsen commented Nov 30, 2016

codecov-io commented Nov 30, 2016 • edited

Current coverage is 68.40% (diff: 69.29%)

aslakknutsen commented Dec 6, 2016

kwk commented Dec 6, 2016

aslakknutsen commented Dec 8, 2016

kwk commented Dec 9, 2016

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 Nov 29, 2016 •

edited

codecov-io commented Nov 30, 2016 •

edited