-
Notifications
You must be signed in to change notification settings - Fork 86
Workitem Type Change #2202
Workitem Type Change #2202
Conversation
[test] |
06d3b8d
to
68b44d2
Compare
Codecov Report
@@ Coverage Diff @@
## master #2202 +/- ##
==========================================
- Coverage 69.46% 69.35% -0.11%
==========================================
Files 170 170
Lines 15800 15995 +195
==========================================
+ Hits 10975 11094 +119
- Misses 3797 3855 +58
- Partials 1028 1046 +18
Continue to review full report at Codecov.
|
delete(ctx.Payload.Data.Attributes, workitem.SystemVersion) | ||
ctx.Payload.Data.Relationships.BaseType = nil | ||
|
||
// Ensure we do not have any other change in payload except type change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jarifibrahim Why do we ensure that we do not have any other change in payload except type change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DhritiShikhar we have already discussed why this is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jarifibrahim I'm not sure if it is really necessary but in terms of work item revisions we should really have two revisions: One for the work item type change itself and then another one for the made changes to the work item fields. The reason being that in an workitem.event.Event
we only know about one work item type and not many. So if a field X was of type string
before the type change occurs and of type integer
afterwards, this would be problematic. I mean we could change the event system to always know from which WIT a field originated from but this is an overkill I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One for the work item type change itself and then another one for the made changes to the work item fields.
When a workitem is changed from Type A
to Type B
.
In state 1 - Workitem revision has fields of type A and actual type is A
In state 2 - Workitem revision has fields of type A and actual type is B
In state 3 - Workitem revision has fields of type B and actual type is B
Even in this way of saving 2 revisions
we will have an inconsistent state. State 2 has different fields as compared to its actual type. The Event
system has to deal with state 2 even if we store 2 revisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kwk @jarifibrahim if the workitem update payload includes update to field + update to workitem type, then it could be handled in two parts:
- Update the fields first
- Update the WIT
I am not really sure about imposing a restriction that field update cannot happen with WIT update.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the discussion with @DhritiShikhar and Nimisha, I've created a separate task for this https://openshift.io/openshiftio/Openshift_io/plan/detail/433
…c8-wit into osio-story-43
@jarifibrahim this is the request I sent to create WI: {
"data": {
"type": "workitems",
"attributes": {
"system.owner": "testuser2",
"system.state": "Open",
"system.title": "workitem1"
},
"relationships": {
"baseType": {
"data": {
"id": "03b9bb64-4f65-4fa7-b165-494cd4f01401",
"type": "workitemtypes"
}
},
"space": {
"data": {
"id": "a1f53fe3-abac-4b0d-80d4-1a4e2e69b509",
"type": "spaces"
}
}
}
}
} This is the response I got: {
"data": {
"attributes": {
"resolution": "None",
"system.created_at": "2018-08-17T16:09:30.833094+05:30",
"system.metastate": null,
"system.number": 1,
"system.order": 1000,
"system.remote_item_id": null,
"system.state": "Open",
"system.title": "workitem1",
"system.updated_at": "2018-08-17T16:09:30.833094+05:30",
"version": 0
},
"id": "8c9478b9-39ea-4a17-a7cf-6ac078f72c16",
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16",
"self": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16"
},
"relationships": {
"area": {
"data": {
"id": "c2d53f76-a6d3-4e34-85dd-8c3dadaf7674",
"type": "areas"
},
"links": {
"related": "http://localhost:8080/api/areas/c2d53f76-a6d3-4e34-85dd-8c3dadaf7674",
"self": "http://localhost:8080/api/areas/c2d53f76-a6d3-4e34-85dd-8c3dadaf7674"
}
},
"assignees": {},
"baseType": {
"data": {
"id": "03b9bb64-4f65-4fa7-b165-494cd4f01401",
"type": "workitemtypes"
},
"links": {
"self": "http://localhost:8080/api/workitemtypes/03b9bb64-4f65-4fa7-b165-494cd4f01401"
}
},
"children": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/children"
},
"meta": {
"hasChildren": false
}
},
"comments": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/comments",
"self": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/relationships/comments"
}
},
"creator": {
"data": {
"id": "48e26f62-71db-4cba-9859-af951192f728",
"type": "users"
},
"links": {
"related": "http://localhost:8080/api/users/48e26f62-71db-4cba-9859-af951192f728",
"self": "http://localhost:8080/api/users/48e26f62-71db-4cba-9859-af951192f728"
}
},
"events": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/events"
}
},
"iteration": {
"data": {
"id": "b9670061-86c7-4282-aa0c-da7b9e4a3dff",
"type": "iterations"
},
"links": {
"related": "http://localhost:8080/api/iterations/b9670061-86c7-4282-aa0c-da7b9e4a3dff",
"self": "http://localhost:8080/api/iterations/b9670061-86c7-4282-aa0c-da7b9e4a3dff"
}
},
"labels": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/labels"
}
},
"space": {
"data": {
"id": "a1f53fe3-abac-4b0d-80d4-1a4e2e69b509",
"type": "spaces"
},
"links": {
"related": "http://localhost:8080/api/spaces/a1f53fe3-abac-4b0d-80d4-1a4e2e69b509",
"self": "http://localhost:8080/api/spaces/a1f53fe3-abac-4b0d-80d4-1a4e2e69b509"
}
},
"system.boardcolumns": {},
"workItemLinks": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/links"
}
}
},
"type": "workitems"
},
"links": {
"self": "http://localhost:8080/api/spaces/a1f53fe3-abac-4b0d-80d4-1a4e2e69b509/workitems"
}
} Notice, the Here is my request to update the workitem: {
"data": {
"type": "workitems",
"attributes": {
"version": 0
},
"relationships": {
"baseType": {
"data": {
"id": "fce0921f-ea70-4513-bb91-31d3aa8017f1",
"type": "workitemtypes"
}
}
},
"id": "8c9478b9-39ea-4a17-a7cf-6ac078f72c16"
}
} This is the update response: {
"data": {
"attributes": {
"effort": null,
"environment": null,
"priority": "P1 - Critical",
"repro_steps": null,
"resolution": "Done",
"severity": "SEV1 - Urgent",
"system.created_at": "2018-08-17T10:39:30.833094Z",
"system.description": "```\n\n======= Missing fields in workitem type: Defect =======\n\nResolution : None\n\n\n================================================\n```\n\n",
"system.description.markup": "Markdown",
"system.description.rendered": "<pre><code class=\"prettyprint\">\n<span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span> <span class=\"typ\">Missing</span> <span class=\"pln\">fields</span> <span class=\"kwd\">in</span> <span class=\"pln\">workitem</span> <span class=\"kwd\">type</span><span class=\"pun\">:</span> <span class=\"typ\">Defect</span> <span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span>\n\n<span class=\"typ\">Resolution</span> <span class=\"pun\">:</span> <span class=\"kwd\">None</span>\n\n\n<span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span><span class=\"pun\">=</span>\n</code></pre>\n",
"system.metastate": null,
"system.number": 1,
"system.order": 1000,
"system.remote_item_id": null,
"system.state": "Open",
"system.title": "workitem1",
"system.updated_at": "2018-08-17T16:11:36.968752+05:30",
"version": 1
},
"id": "8c9478b9-39ea-4a17-a7cf-6ac078f72c16",
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16",
"self": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16"
},
"relationships": {
"area": {
"data": {
"id": "c2d53f76-a6d3-4e34-85dd-8c3dadaf7674",
"type": "areas"
},
"links": {
"related": "http://localhost:8080/api/areas/c2d53f76-a6d3-4e34-85dd-8c3dadaf7674",
"self": "http://localhost:8080/api/areas/c2d53f76-a6d3-4e34-85dd-8c3dadaf7674"
}
},
"assignees": {},
"baseType": {
"data": {
"id": "fce0921f-ea70-4513-bb91-31d3aa8017f1",
"type": "workitemtypes"
},
"links": {
"self": "http://localhost:8080/api/workitemtypes/fce0921f-ea70-4513-bb91-31d3aa8017f1"
}
},
"children": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/children"
},
"meta": {
"hasChildren": false
}
},
"comments": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/comments",
"self": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/relationships/comments"
}
},
"creator": {
"data": {
"id": "48e26f62-71db-4cba-9859-af951192f728",
"type": "users"
},
"links": {
"related": "http://localhost:8080/api/users/48e26f62-71db-4cba-9859-af951192f728",
"self": "http://localhost:8080/api/users/48e26f62-71db-4cba-9859-af951192f728"
}
},
"events": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/events"
}
},
"iteration": {
"data": {
"id": "b9670061-86c7-4282-aa0c-da7b9e4a3dff",
"type": "iterations"
},
"links": {
"related": "http://localhost:8080/api/iterations/b9670061-86c7-4282-aa0c-da7b9e4a3dff",
"self": "http://localhost:8080/api/iterations/b9670061-86c7-4282-aa0c-da7b9e4a3dff"
}
},
"labels": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/labels"
}
},
"space": {
"data": {
"id": "a1f53fe3-abac-4b0d-80d4-1a4e2e69b509",
"type": "spaces"
},
"links": {
"related": "http://localhost:8080/api/spaces/a1f53fe3-abac-4b0d-80d4-1a4e2e69b509",
"self": "http://localhost:8080/api/spaces/a1f53fe3-abac-4b0d-80d4-1a4e2e69b509"
}
},
"system.boardcolumns": {},
"workItemLinks": {
"links": {
"related": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16/links"
}
}
},
"type": "workitems"
},
"links": {
"self": "http://localhost:8080/api/workitems/8c9478b9-39ea-4a17-a7cf-6ac078f72c16"
}
} Notice the |
@DhritiShikhar The
while that of
The resolution values aren't compatible and hence the old value is added to the description and a default value is assigned. |
@jarifibrahim I can recall our meeting in which we said that a user's expectation would be to set any enum field in a new type if the value exists in the allowed enum values of the new type. And here @jarifibrahim I just want to be sure that if the new enum field contained a |
Correct. I verified this. If both enums have |
@kwk @jarifibrahim Yes I agree. The response is fine and expected. I mistook |
An update through |
It shouldn't work. See this https://github.com/fabric8-services/fabric8-wit/pull/2202/files#diff-02845b093c442bf1ae790a6ebb6c8da8R153 Do you have a test/sample request to reproduce this? |
test.UpdateWorkitemConflict(t, svc.Context, svc, s.workitemCtrl, fxt.WorkItems[0].ID, &u) | ||
}) | ||
|
||
s.T().Run("unauthorized", func(t *testing.T) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SUGGESTION] There should be a test to capture this scenario -> Space Admin which is not workitem creator is successfully able to change WIT of workitem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How do I make an identity a space owner? Can you point me to a similar test?
@jarifibrahim This is my request payload
This is response:
You can see the area is updated under Is this expected @jarifibrahim ? |
@@ -969,6 +969,139 @@ func (s *WorkItem2Suite) TestWI2UpdateSetReadOnlyFields() { | |||
}) | |||
} | |||
|
|||
func (s *WorkItem2Suite) TestWI2UpdateWorkItemType() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[SUGGESTION] There should be a test case to capture multiple times work item type change on same workitem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DhritiShikhar the test here https://github.com/fabric8-services/fabric8-wit/pull/2202/files#diff-1999ee68a49d5d171f3f4b3e1c4ccd04R155 is not the same but it shows that we essentially can convert back and forth. It doesn't use the same work item for this though.
workitem/workitem_repository.go
Outdated
} | ||
|
||
// CheckWITypeBelongsToSpace returns true if the given workitem type (wit) belongs to the same space template as the space (spaceID) | ||
func (r *GormWorkItemRepository) CheckWITypeBelongsToSpace(ctx context.Context, wit *WorkItemType, spaceID uuid.UUID) (bool, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[MINOR] Function name should be corrected. Work Item Type does not "belong to space"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@DhritiShikhar I've addressed that in 038c56e and renamed the function to CheckTypeAndSpaceShareTemplate
.
@DhritiShikhar I see this in your request: "area": {
"data": {
"id": "a430b960-1436-4a66-bfe3-d4628f88e646",
"type": "areas",
"links": {
"self": "http://localhost:8080/api/areas/a430b960-1436-4a66-bfe3-d4628f88e646"
}
}
} Please move the "area": {
"data": {
"id": "a430b960-1436-4a66-bfe3-d4628f88e646",
"type": "areas"
}
"links": {
"self": "http://localhost:8080/api/areas/a430b960-1436-4a66-bfe3-d4628f88e646"
}
} Only then it is correct JSONAPI. Apart from that, the "area": {
"data": {
"id": "a430b960-1436-4a66-bfe3-d4628f88e646",
"type": "areas"
},
"links": {
"related": "http://localhost:8080/api/areas/a430b960-1436-4a66-bfe3-d4628f88e646",
"self": "http://localhost:8080/api/areas/a430b960-1436-4a66-bfe3-d4628f88e646"
}
}, The area ID is still the same, but the backend response with proper JSONAPI in this area. So yes, this is expected. |
workitem/workitem_repository.go
Outdated
if ok { | ||
_, err = newField.Type.ConvertToModel(interfaceArray) | ||
var convertedValue interface{} | ||
convertedValue, err = newField.Type.ConvertToModel([]interface{}{wiStorage.Fields[oldFieldName]}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kwk Why did you cast the value here
[]interface{}{wiStorage.Fields[oldFieldName]}
?
I had done a type assertion in my code.
So the question is when do we type assert and when do we type cast?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on https://jimsaunders.net/2015/04/23/type-conversion-vs-type-assertion-in-go.html
convertedValue, err = newField.Type.ConvertToModel([]interface{}{wiStorage.Fields[oldFieldName]})
should be
a, _ := wiStorage.Fields[oldFieldName].([]interface{})
convertedValue, err = newField.Type.ConvertToModel(a)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jarifibrahim No, it shouldn't IMHO. And I'm not doing a cast here. I'm creating a new array of interface objects and stuff the old value in it so it becomes a list. This is useful when we want to convert from a field that was a single value field to a field that is a list. I've updated the tests now (692cce4) to include negative tests as well that check if the description is properly updated if a field couldn't be converted. If all passes I'm fine with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kwk I don't completely understand your answer but we can deal with it later. I am fine with your changes. Thank you :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why did you cast the value here
[]interface{}{wiStorage.Fields[oldFieldName]} ?
I thought this is type casting but you're creating a new array here. This makes sense now.
I got confused between type casting and type assertion but this is creating a new array.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jarifibrahim Thank you for addressing my comments. The PR looks good to be merged now. Improvements/ pending tasks could be done in another PR.
This PR depends on #2202 and #2239 Once the above mentioned PRs are merged I will rebase this pull request. With this PR merged, the response for workitem type change event would look like ``` { "data": [ { "attributes": { "name": "workitemtype", "timestamp": "0001-01-01T00:00:00Z" }, "id": "00000000-0000-0000-0000-000000000001", "relationships": { "modifier": { .... }, "newValue": { "data": [ { "id": "00000000-0000-0000-0000-000000000003", "type": "workitemtypes" } ] }, "oldValue": { "data": [ { "id": "00000000-0000-0000-0000-000000000004", "type": "workitemtypes" } ] }, "workItemType": { .... }, }, "type": "events" } ] } ```
---- **commit** fabric8-services/fabric8-wit@182f480 **Author:** Konrad Kleine <193408+kwk@users.noreply.github.com> **Date:** Thu Aug 16 21:53:58 2018 +0200 Remove unused migration test function (fabric8-services/fabric8-wit#2240) The same thing is tested in migration 98 function. ---- **commit** fabric8-services/fabric8-wit@2b00c92 **Author:** Ruchir Garg <rgarg@redhat.com> **Date:** Fri Aug 17 15:37:15 2018 +0530 Add E2E API tests to PRs (fabric8-services/fabric8-wit#2197) With this change the end to end tests are being executed on every PR for the fabric8-wit repo. Ref: fabric8-services/fabric8-wit#2164 Co-authored-by: Konrad Kleine 193408+kwk@users.noreply.github.com ---- **commit** fabric8-services/fabric8-wit@95a5119 **Author:** Ibrahim Jarif <jarifibrahim@gmail.com> **Date:** Fri Aug 17 16:34:58 2018 +0530 Prevent double escaping of comments (fabric8-services/fabric8-wit#2236) ---- **commit** fabric8-services/fabric8-wit@b688f8f **Author:** Ibrahim Jarif <jarifibrahim@gmail.com> **Date:** Mon Aug 20 12:23:21 2018 +0530 Workitem Type Change (fabric8-services/fabric8-wit#2202) Implements story https://openshift.io/openshiftio/Openshift_io/plan/detail/43 This PR enables user to change workitem type. A `PATCH` request on `/api/workitem/:uuid` with the following payload ``` "data": { "attributes": { "version": 1 }, "relationships": { "baseType": { "data": { "id": "00000000-0000-0000-0000-000000000001", "type": "workitemtypes" } } } } ``` would change the type of workitem to `00000000-0000-0000-0000-000000000001` **NOTE** - If the payload contains anything apart from the above fields the request will be rejected. We do not allow change of any attribute on a type change request. There's a followup story to address this: https://openshift.io/openshiftio/Openshift_io/plan/detail/433. **Details** - Consider the following two workitem types - 1. Type A has following fields ``` fooo - KindFloat fooBar - KindEnum { KindString } Values { "open", "close", "done" } assigned-to - KindList { KindUser } bar - KindString reporter - KindUser ``` 2. Type B has following fields ``` fooo - KindFloat bar - KindInteger foobar - KindEnum { KindString } Values { "alpha", "beta", "gamma" } ``` If we have a workitem of `Type A` with following fields ``` WorkItem.Fields["fooo"] = 2.5 WorkItem.Fields["fooBar"] = "open" WorkItem.Fields["bar"] = "hello" WorkItem.Fields["reporter"] = "Jon Doe" WorkItem.Fields["assigned-to"] = []string{"Jon Doe", "Lorem Ipsum"} ``` When the type of workitem is changed from `Type A` to `Type B`, the workitem will have the following fields ``` WorkItem.Fields["fooo"] = 2.5 WorkItem.Fields["fooBar"] = "alpha" WorkItem.Fields["bar"] = null WorkItem.Fields["system.description"] = " ======= Missing fields in workitem type: work item type Type A ======= Type1 Assigned To : Jon Doe, Lorem Ipsum Type1 bar : hello Type1 fooBar : open Type1 reporter : Jon Doe ================================================" ``` 1. Field `fooo` is present in `Type A` and `Type B` and that's why the new workitem has the same value set. 2. Field `bar` is present in `Type A` and `Type B` but the type is different (Type A has `string`, Type B has `int`). So, it is prepended to the description. 3. Field `fooBar` exists in both the types but the `Enum value` cannot be assigned to the new type. Hence it is prepended to the description. 4. Field `reporter` and `assigned-to` didn't exist in `Type B` and that's why they are added to the description. **NOTE** - 1. ~~ All fields in the new workitem get a default value. ~~ 2. All the relational fields (area, iteration, user) will be resolved. `UUIDs` will be resolved to get the actual value of the field and the actual value will be added to the description. 3. All the existing links will be preserved. We do not change/modify any existing links. TODO - [x] Disallow attribute change when workitem type is changed - [x] Events API supports type change event (This will be done as a separate PR fabric8-services/fabric8-wit#2234) ## Additional notes We support conversion of these field types as long as the individual field kinds match: * simple to simple * simple type to list * simple type to enum * list to list * list to simple type * only when list contains just one element * list to enum * only when list contains just one element * enum to enum * enum to simple type * enum to list That being said, we currently don't allow conversion from string to int fields for example. Co-authored-by: Konrad Kleine 193408+kwk@users.noreply.github.com ---- **commit** fabric8-services/fabric8-wit@09d3745 **Author:** Michael Kleinhenz <kleinhenz@redhat.com> **Date:** Mon Aug 20 13:13:31 2018 +0200 feat(actions): Actions system infra The actions system is a key component for process automation in WIT. It provides a way of executing user-configurable, dynamic process steps depending on user settings, schema settings and events in the WIT. The current PR provides the basic actions infrastructure and an implementation of an example action rules for testing. ---- **commit** fabric8-services/fabric8-wit@87fdcec **Author:** Ibrahim Jarif <jarifibrahim@gmail.com> **Date:** Mon Aug 20 17:30:06 2018 +0530 Add NotNull and NotEmpty constraint to Users.email column (fabric8-services/fabric8-wit#2248) See fabric8-services/fabric8-wit#2237 ---- **commit** fabric8-services/fabric8-wit@81c9706 **Author:** Michael Kleinhenz <kleinhenz@redhat.com> **Date:** Mon Aug 20 23:35:50 2018 +0200 Fixed migration test numbering and func order. (fabric8-services/fabric8-wit#2250) This fixes the numbering of the migration tests where somehow, we started not using the migration order number from 100 to 102. This is somewhat confusing when adding new tests there.This change addresses this.
---- **commit** fabric8-services/fabric8-wit@182f480 **Author:** Konrad Kleine <193408+kwk@users.noreply.github.com> **Date:** Thu Aug 16 21:53:58 2018 +0200 Remove unused migration test function (fabric8-services/fabric8-wit#2240) The same thing is tested in migration 98 function. ---- **commit** fabric8-services/fabric8-wit@2b00c92 **Author:** Ruchir Garg <rgarg@redhat.com> **Date:** Fri Aug 17 15:37:15 2018 +0530 Add E2E API tests to PRs (fabric8-services/fabric8-wit#2197) With this change the end to end tests are being executed on every PR for the fabric8-wit repo. Ref: fabric8-services/fabric8-wit#2164 Co-authored-by: Konrad Kleine 193408+kwk@users.noreply.github.com ---- **commit** fabric8-services/fabric8-wit@95a5119 **Author:** Ibrahim Jarif <jarifibrahim@gmail.com> **Date:** Fri Aug 17 16:34:58 2018 +0530 Prevent double escaping of comments (fabric8-services/fabric8-wit#2236) ---- **commit** fabric8-services/fabric8-wit@b688f8f **Author:** Ibrahim Jarif <jarifibrahim@gmail.com> **Date:** Mon Aug 20 12:23:21 2018 +0530 Workitem Type Change (fabric8-services/fabric8-wit#2202) Implements story https://openshift.io/openshiftio/Openshift_io/plan/detail/43 This PR enables user to change workitem type. A `PATCH` request on `/api/workitem/:uuid` with the following payload ``` "data": { "attributes": { "version": 1 }, "relationships": { "baseType": { "data": { "id": "00000000-0000-0000-0000-000000000001", "type": "workitemtypes" } } } } ``` would change the type of workitem to `00000000-0000-0000-0000-000000000001` **NOTE** - If the payload contains anything apart from the above fields the request will be rejected. We do not allow change of any attribute on a type change request. There's a followup story to address this: https://openshift.io/openshiftio/Openshift_io/plan/detail/433. **Details** - Consider the following two workitem types - 1. Type A has following fields ``` fooo - KindFloat fooBar - KindEnum { KindString } Values { "open", "close", "done" } assigned-to - KindList { KindUser } bar - KindString reporter - KindUser ``` 2. Type B has following fields ``` fooo - KindFloat bar - KindInteger foobar - KindEnum { KindString } Values { "alpha", "beta", "gamma" } ``` If we have a workitem of `Type A` with following fields ``` WorkItem.Fields["fooo"] = 2.5 WorkItem.Fields["fooBar"] = "open" WorkItem.Fields["bar"] = "hello" WorkItem.Fields["reporter"] = "Jon Doe" WorkItem.Fields["assigned-to"] = []string{"Jon Doe", "Lorem Ipsum"} ``` When the type of workitem is changed from `Type A` to `Type B`, the workitem will have the following fields ``` WorkItem.Fields["fooo"] = 2.5 WorkItem.Fields["fooBar"] = "alpha" WorkItem.Fields["bar"] = null WorkItem.Fields["system.description"] = " ======= Missing fields in workitem type: work item type Type A ======= Type1 Assigned To : Jon Doe, Lorem Ipsum Type1 bar : hello Type1 fooBar : open Type1 reporter : Jon Doe ================================================" ``` 1. Field `fooo` is present in `Type A` and `Type B` and that's why the new workitem has the same value set. 2. Field `bar` is present in `Type A` and `Type B` but the type is different (Type A has `string`, Type B has `int`). So, it is prepended to the description. 3. Field `fooBar` exists in both the types but the `Enum value` cannot be assigned to the new type. Hence it is prepended to the description. 4. Field `reporter` and `assigned-to` didn't exist in `Type B` and that's why they are added to the description. **NOTE** - 1. ~~ All fields in the new workitem get a default value. ~~ 2. All the relational fields (area, iteration, user) will be resolved. `UUIDs` will be resolved to get the actual value of the field and the actual value will be added to the description. 3. All the existing links will be preserved. We do not change/modify any existing links. TODO - [x] Disallow attribute change when workitem type is changed - [x] Events API supports type change event (This will be done as a separate PR fabric8-services/fabric8-wit#2234) ## Additional notes We support conversion of these field types as long as the individual field kinds match: * simple to simple * simple type to list * simple type to enum * list to list * list to simple type * only when list contains just one element * list to enum * only when list contains just one element * enum to enum * enum to simple type * enum to list That being said, we currently don't allow conversion from string to int fields for example. Co-authored-by: Konrad Kleine 193408+kwk@users.noreply.github.com ---- **commit** fabric8-services/fabric8-wit@09d3745 **Author:** Michael Kleinhenz <kleinhenz@redhat.com> **Date:** Mon Aug 20 13:13:31 2018 +0200 feat(actions): Actions system infra The actions system is a key component for process automation in WIT. It provides a way of executing user-configurable, dynamic process steps depending on user settings, schema settings and events in the WIT. The current PR provides the basic actions infrastructure and an implementation of an example action rules for testing. ---- **commit** fabric8-services/fabric8-wit@87fdcec **Author:** Ibrahim Jarif <jarifibrahim@gmail.com> **Date:** Mon Aug 20 17:30:06 2018 +0530 Add NotNull and NotEmpty constraint to Users.email column (fabric8-services/fabric8-wit#2248) See fabric8-services/fabric8-wit#2237 ---- **commit** fabric8-services/fabric8-wit@81c9706 **Author:** Michael Kleinhenz <kleinhenz@redhat.com> **Date:** Mon Aug 20 23:35:50 2018 +0200 Fixed migration test numbering and func order. (fabric8-services/fabric8-wit#2250) This fixes the numbering of the migration tests where somehow, we started not using the migration order number from 100 to 102. This is somewhat confusing when adding new tests there.This change addresses this. ---- **commit** fabric8-services/fabric8-wit@cfe4545 **Author:** Ibrahim Jarif <jarifibrahim@gmail.com> **Date:** Tue Aug 21 14:23:07 2018 +0530 Add support for workitem type change event (#2234) This PR depends on fabric8-services/fabric8-wit#2202 and fabric8-services/fabric8-wit#2239 Once the above mentioned PRs are merged I will rebase this pull request. With this PR merged, the response for workitem type change event would look like ```yaml { "data": [ { "attributes": { "name": "workitemtype", "timestamp": "0001-01-01T00:00:00Z" }, "id": "00000000-0000-0000-0000-000000000001", "relationships": { "modifier": { .... }, "newValue": { "data": [ { "id": "00000000-0000-0000-0000-000000000003", "type": "workitemtypes" } ] }, "oldValue": { "data": [ { "id": "00000000-0000-0000-0000-000000000004", "type": "workitemtypes" } ] }, "workItemType": { .... }, }, "type": "events" } ] } ``` ---- **commit** fabric8-services/fabric8-wit@69ce1ac **Author:** Konrad Kleine <193408+kwk@users.noreply.github.com> **Date:** Tue Aug 21 12:44:24 2018 +0200 Default value (fabric8-services/fabric8-wit#2247) ## Defaults for fields This change introduces the ability for any field type to OPTIONALLY specify a custom default value. See also https://openshift.io/openshiftio/Openshift_io/plan/detail/35 and openshiftio/openshift.io#3832. For lists and simple types, the only restriction is that the default value is of the same kind as the list or simple type (e.g. integer, float, string, user, label, ...). For enum fields the custom default value needs to be in the list of allowed values. ### Example usage of the feature As an example, we've set the default for severity and priority fields to a medium value instead of the first on in the list, which would be a high severity/priority. (See also openshiftio/openshift.io#3832) ## Improvements to tests ### Space template import When a space template is imported it creates or overrides the work item types. With this change I've introduced a cascading validation that checks 1. the work item types, 2. the fields inside each work item type 3. the field type of each field. This ensures that people don't accidentally specify a float default (e.g. `33.45`) for an integer field. Before this change, most of the input and output validation happened at runtime when a value was converted into another representation. Now, the validation happens at import time and also when a default value for a field is calculated.
Implements story https://openshift.io/openshiftio/Openshift_io/plan/detail/43
This PR enables user to change workitem type.
A
PATCH
request on/api/workitem/:uuid
with the following payloadwould change the type of workitem to
00000000-0000-0000-0000-000000000001
NOTE - If the payload contains anything apart from the above fields the request will be rejected. We do not allow change of any attribute on a type change request. There's a followup story to address this: https://openshift.io/openshiftio/Openshift_io/plan/detail/433.
Details -
Consider the following two workitem types -
If we have a workitem of
Type A
with following fieldsWhen the type of workitem is changed from
Type A
toType B
, the workitem will have the following fieldsfooo
is present inType A
andType B
and that's why the new workitem has the same value set.bar
is present inType A
andType B
but the type is different (Type A hasstring
, Type B hasint
). So, it is prepended to the description.fooBar
exists in both the types but theEnum value
cannot be assigned to the new type. Hence it is prepended to the description.reporter
andassigned-to
didn't exist inType B
and that's why they are added to the description.NOTE -
UUIDs
will be resolved to get the actual value of the field and the actual value will be added to the description.TODO
Additional notes
We support conversion of these field types as long as the individual field kinds match:
That being said, we currently don't allow conversion from string to int fields for example.
Co-authored-by: Konrad Kleine 193408+kwk@users.noreply.github.com