From 152f697c1a1e6f1908b6ba912a1da30aad8f652c Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Tue, 14 Aug 2018 17:07:36 +0530 Subject: [PATCH 1/6] Add Users to test fixture (#2238) With this PR, we now have a `tf.Users(n)` function that creates user entities. --- test/testfixture/make_functions.go | 25 +++++++++++++ test/testfixture/recipe_funcs.go | 52 ++++++++++++++++++++++------ test/testfixture/testfixture.go | 7 ++-- test/testfixture/testfixture_test.go | 21 ++++++++--- 4 files changed, 88 insertions(+), 17 deletions(-) diff --git a/test/testfixture/make_functions.go b/test/testfixture/make_functions.go index c0fc82146d..a21c4b914d 100644 --- a/test/testfixture/make_functions.go +++ b/test/testfixture/make_functions.go @@ -25,6 +25,30 @@ import ( uuid "github.com/satori/go.uuid" ) +func makeUsers(fxt *TestFixture) error { + if fxt.info[kindUsers] == nil { + return nil + } + userRepo := account.NewUserRepository(fxt.db) + fxt.Users = make([]*account.User, fxt.info[kindUsers].numInstances) + for i := range fxt.Users { + id := uuid.NewV4() + fxt.Users[i] = &account.User{ + ID: id, + Email: fmt.Sprintf("%s@example.com", id), + FullName: testsupport.CreateRandomValidTestName("user"), + } + if err := fxt.runCustomizeEntityFuncs(i, kindUsers); err != nil { + return errs.WithStack(err) + } + err := userRepo.Create(fxt.ctx, fxt.Users[i]) + if err != nil { + return errs.Wrapf(err, "failed to create user: %+v", fxt.Users[i]) + } + } + return nil +} + func makeIdentities(fxt *TestFixture) error { if fxt.info[kindIdentities] == nil { return nil @@ -34,6 +58,7 @@ func makeIdentities(fxt *TestFixture) error { fxt.Identities[i] = &account.Identity{ Username: testsupport.CreateRandomValidTestName("John Doe "), ProviderType: account.KeycloakIDP, + User: *fxt.Users[0], } if err := fxt.runCustomizeEntityFuncs(i, kindIdentities); err != nil { return errs.WithStack(err) diff --git a/test/testfixture/recipe_funcs.go b/test/testfixture/recipe_funcs.go index b652517928..0bfb5f9f13 100644 --- a/test/testfixture/recipe_funcs.go +++ b/test/testfixture/recipe_funcs.go @@ -22,28 +22,55 @@ func (fxt *TestFixture) deps(fns ...RecipeFunction) error { return nil } -// CustomizeIdentityFunc is directly compatible with CustomizeEntityFunc -// but it can only be used for the Identites() recipe-function. -type CustomizeIdentityFunc CustomizeEntityFunc +// CustomizeUserFunc is directly compatible with CustomizeEntityFunc +// but it can only be used for the Users() recipe-function. +type CustomizeUserFunc CustomizeEntityFunc -// Identities tells the test fixture to create at least n identity objects. +// Users tells the test fixture to create at least n user objects. // // If called multiple times with differently n's, the biggest n wins. All // customize-entitiy-functions fns from all calls will be respected when // creating the test fixture. // -// Here's an example how you can create 42 identites and give them a numbered -// user name like "John Doe 0", "John Doe 1", and so forth: -// Identities(42, func(fxt *TestFixture, idx int) error{ -// fxt.Identities[idx].Username = "Jane Doe " + strconv.FormatInt(idx, 10) +// Here's an example how you can create 10 users and give them a numbered +// fullname like "John Doe 0", "John Doe 1", and so forth: +// Users(10, func(fxt *TestFixture, idx int) error{ +// fxt.Users[idx].FullName = "Jane Doe " + strconv.FormatInt(idx, 10) // return nil // }) // Notice that the index idx goes from 0 to n-1 and that you have to manually -// lookup the object from the test fixture. The identity object referenced by -// fxt.Identities[idx] +// lookup the object from the test fixture. The User object referenced by +// fxt.Users[idx] // is guaranteed to be ready to be used for creation. That means, you don't // necessarily have to touch it to avoid unique key violation for example. This // is totally optional. +func Users(n int, fns ...CustomizeUserFunc) RecipeFunction { + return func(fxt *TestFixture) error { + fxt.checkFuncs = append(fxt.checkFuncs, func() error { + l := len(fxt.Users) + if l < n { + return errs.Errorf(checkStr, n, kindUsers, l) + } + return nil + }) + // Convert fns to []CustomizeEntityFunc + customFuncs := make([]CustomizeEntityFunc, len(fns)) + for idx := range fns { + customFuncs[idx] = CustomizeEntityFunc(fns[idx]) + } + return fxt.setupInfo(n, kindUsers, customFuncs...) + } +} + +// CustomizeIdentityFunc is directly compatible with CustomizeEntityFunc +// but it can only be used for the Identites() recipe-function. +type CustomizeIdentityFunc CustomizeEntityFunc + +// Identities tells the test fixture to create at least n identity objects. +// See also the Users() function for more general information on n and fns. +// When called in NewFixture() this function call will also call +// Users(1) +// but with NewFixtureIsolated(), no other objects will be created. func Identities(n int, fns ...CustomizeIdentityFunc) RecipeFunction { return func(fxt *TestFixture) error { fxt.checkFuncs = append(fxt.checkFuncs, func() error { @@ -58,7 +85,10 @@ func Identities(n int, fns ...CustomizeIdentityFunc) RecipeFunction { for idx := range fns { customFuncs[idx] = CustomizeEntityFunc(fns[idx]) } - return fxt.setupInfo(n, kindIdentities, customFuncs...) + if err := fxt.setupInfo(n, kindIdentities, customFuncs...); err != nil { + return err + } + return fxt.deps(Users(1)) } } diff --git a/test/testfixture/testfixture.go b/test/testfixture/testfixture.go index 8a3ce0b796..7209c77549 100644 --- a/test/testfixture/testfixture.go +++ b/test/testfixture/testfixture.go @@ -38,7 +38,8 @@ type TestFixture struct { customLinkCreation bool // on when you've used WorkItemLinksCustom in your recipe normalLinkCreation bool // on when you've used WorkItemLinks in your recipe - Identities []*account.Identity // Itentities (if any) that were created for this test fixture. + Users []*account.User // Users (if any) that were created for this test fixture. + Identities []*account.Identity // Identities (if any) that were created for this test fixture. Iterations []*iteration.Iteration // Iterations (if any) that were created for this test fixture. Areas []*area.Area // Areas (if any) that were created for this test fixture. Spaces []*space.Space // Spaces (if any) that were created for this test fixture. @@ -154,6 +155,7 @@ func (fxt *TestFixture) Check() error { type kind string const ( + kindUsers kind = "user" kindIdentities kind = "identity" kindIterations kind = "iteration" kindAreas kind = "area" @@ -221,11 +223,12 @@ func newFixture(db *gorm.DB, isolatedCreation bool, recipeFuncs ...RecipeFunctio } makeFuncs := []func(fxt *TestFixture) error{ // make the objects that DON'T have any dependency - makeIdentities, + makeUsers, makeTrackers, makeWorkItemLinkCategories, makeSpaceTemplates, // actually make the objects that DO have dependencies + makeIdentities, makeSpaces, makeLabels, makeQueries, diff --git a/test/testfixture/testfixture_test.go b/test/testfixture/testfixture_test.go index 83364ce91d..8c31dfabf8 100644 --- a/test/testfixture/testfixture_test.go +++ b/test/testfixture/testfixture_test.go @@ -98,13 +98,13 @@ func checkNewFixture(t *testing.T, db *gorm.DB, n int, isolated bool) { } } - // identity and work item link categories will always work - - t.Run("identities", func(t *testing.T) { + // user and work item link categories will always work + t.Run("users", func(t *testing.T) { // given - c, err := fxtCtor(db, tf.Identities(n)) + c, err := fxtCtor(db, tf.Users(n)) // then require.NoError(t, err) + require.NotNil(t, c) require.Nil(t, c.Check()) // manual checking require.Len(t, c.Identities, n) @@ -118,6 +118,19 @@ func checkNewFixture(t *testing.T, db *gorm.DB, n int, isolated bool) { // manual checking require.Len(t, c.WorkItemLinkCategories, n) }) + + t.Run("identities", func(t *testing.T) { + // given + c, err := fxtCtor(db, tf.Identities(n)) + // then + require.NoError(t, err) + require.Nil(t, c.Check()) + // manual checking + require.Len(t, c.Identities, n) + if !isolated { + require.Len(t, c.Users, 1) + } + }) t.Run("space_templates", func(t *testing.T) { // given c, err := fxtCtor(db, tf.SpaceTemplates(n)) From 19c0e19355ac3b0aa8bdd67e8a9e0854a3843ed2 Mon Sep 17 00:00:00 2001 From: Konrad Kleine <193408+kwk@users.noreply.github.com> Date: Tue, 14 Aug 2018 15:35:16 +0200 Subject: [PATCH 2/6] Restore event cleanup (code+golden files) (#2239) See #2226 for the original change that was reverted later in #2227. ---- There were places inside of the event system that dealt with work item fields by name and not by their type. Here's what's done by this change. 1. We only distinguish between single-value (`workitem.SimpleType` and `workitem.EnumType`) and multi-value (`workitem.ListType`) fields in the work item repository. 2. We use the existing `workitem.FieldType.ConvertFromModel` function to convert stored values from the DB into the model space. Previously this was done manually and everything was converted to a string. 3. The events JSONAPI no longer expects old and new values to be strings all the time. Instead values can be of any type. Have a look at the `controller/test-files/event/list/ok-kindFloat.res.payload.golden.json` and `controller/test-files/event/list/ok-kindInt.res.payload.golden.json` files to see that effect. 4. Added event system tests for simple values (those that are not relationships) in a list or a single field. This work in this change was initially done in #2212 and #2213 to split up code and golden file changes. It has been reviewed there. --- controller/codebase.go | 15 + controller/label.go | 4 +- .../list/ok-area.res.payload.golden.json | 17 +- .../list/ok-assignees.res.payload.golden.json | 12 +- .../ok-description.res.payload.golden.json | 15 +- .../list/ok-iteration.res.payload.golden.json | 17 +- .../list/ok-kindFloat.res.payload.golden.json | 13 +- .../list/ok-kindInt.res.payload.golden.json | 13 +- .../list/ok-labels.res.payload.golden.json | 12 +- .../list/ok-state.res.payload.golden.json | 9 + .../list/ok-title.res.payload.golden.json | 9 + .../list/ok.bool_list.res.payload.golden.json | 74 +++++ .../ok.bool_single.res.payload.golden.json | 65 ++++ .../ok.float_list.res.payload.golden.json | 74 +++++ .../ok.float_single.res.payload.golden.json | 65 ++++ .../ok.integer_list.res.payload.golden.json | 74 +++++ .../ok.integer_single.res.payload.golden.json | 65 ++++ .../ok.markup_list.res.payload.golden.json | 92 ++++++ .../ok.markup_single.res.payload.golden.json | 74 +++++ .../ok.string_list.res.payload.golden.json | 74 +++++ .../ok.string_single.res.payload.golden.json | 65 ++++ .../list/ok.url_list.res.payload.golden.json | 74 +++++ .../ok.url_single.res.payload.golden.json | 65 ++++ controller/work_item_events.go | 297 ++++++++++-------- controller/work_item_events_test.go | 204 +++++++++++- design/work_item_event.go | 15 +- workitem/enum_type.go | 2 +- workitem/event/event.go | 13 +- workitem/event/event_repository.go | 245 ++++++--------- .../event/event_repository_blackbox_test.go | 87 +++-- workitem/field_definition.go | 38 ++- workitem/field_definition_blackbox_test.go | 23 ++ workitem/field_test_data.go | 2 +- 33 files changed, 1523 insertions(+), 400 deletions(-) create mode 100755 controller/test-files/event/list/ok.bool_list.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.bool_single.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.float_list.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.float_single.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.integer_list.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.integer_single.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.markup_list.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.markup_single.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.string_list.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.string_single.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.url_list.res.payload.golden.json create mode 100755 controller/test-files/event/list/ok.url_single.res.payload.golden.json diff --git a/controller/codebase.go b/controller/codebase.go index 67e4496060..9c98ec30dd 100644 --- a/controller/codebase.go +++ b/controller/codebase.go @@ -474,6 +474,21 @@ func getBranch(projects []che.WorkspaceProject, codebaseURL string) string { return "" } +// ConvertCodebaseSimple converts a simple codebase ID into a Generic Relationship +func ConvertCodebaseSimple(request *http.Request, id interface{}) (*app.GenericData, *app.GenericLinks) { + i := fmt.Sprint(id) + data := &app.GenericData{ + Type: ptr.String(APIStringTypeCodebase), + ID: &i, + } + relatedURL := rest.AbsoluteURL(request, app.CodebaseHref(i)) + links := &app.GenericLinks{ + Self: &relatedURL, + Related: &relatedURL, + } + return data, links +} + // ConvertCodebase converts between internal and external REST representation func ConvertCodebase(request *http.Request, codebase codebase.Codebase, options ...CodebaseConvertFunc) *app.Codebase { relatedURL := rest.AbsoluteURL(request, app.CodebaseHref(codebase.ID)) diff --git a/controller/label.go b/controller/label.go index fc14a90908..3fda67facc 100644 --- a/controller/label.go +++ b/controller/label.go @@ -11,6 +11,7 @@ import ( "github.com/fabric8-services/fabric8-wit/jsonapi" "github.com/fabric8-services/fabric8-wit/label" "github.com/fabric8-services/fabric8-wit/login" + "github.com/fabric8-services/fabric8-wit/ptr" "github.com/fabric8-services/fabric8-wit/rest" "github.com/fabric8-services/fabric8-wit/space" "github.com/goadesign/goa" @@ -164,10 +165,9 @@ func ConvertLabelsSimple(request *http.Request, labelIDs []interface{}) []*app.G // ConvertLabelSimple converts a Label ID into a Generic Relationship func ConvertLabelSimple(request *http.Request, labelID interface{}) *app.GenericData { - t := label.APIStringTypeLabels i := fmt.Sprint(labelID) return &app.GenericData{ - Type: &t, + Type: ptr.String(label.APIStringTypeLabels), ID: &i, } } diff --git a/controller/test-files/event/list/ok-area.res.payload.golden.json b/controller/test-files/event/list/ok-area.res.payload.golden.json index d7c771214c..8912ec5895 100755 --- a/controller/test-files/event/list/ok-area.res.payload.golden.json +++ b/controller/test-files/event/list/ok-area.res.payload.golden.json @@ -3,8 +3,6 @@ { "attributes": { "name": "system.area", - "newValue": null, - "oldValue": null, "timestamp": "0001-01-01T00:00:00Z" }, "id": "00000000-0000-0000-0000-000000000001", @@ -27,13 +25,14 @@ } ] }, - "oldValue": { - "data": [ - { - "id": "", - "type": "areas" - } - ] + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000004", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000004" + } } }, "type": "events" diff --git a/controller/test-files/event/list/ok-assignees.res.payload.golden.json b/controller/test-files/event/list/ok-assignees.res.payload.golden.json index d8eae8db09..eca63d59a8 100755 --- a/controller/test-files/event/list/ok-assignees.res.payload.golden.json +++ b/controller/test-files/event/list/ok-assignees.res.payload.golden.json @@ -3,8 +3,6 @@ { "attributes": { "name": "system.assignees", - "newValue": null, - "oldValue": null, "timestamp": "0001-01-01T00:00:00Z" }, "id": "00000000-0000-0000-0000-000000000001", @@ -27,7 +25,15 @@ } ] }, - "oldValue": {} + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } }, "type": "events" } diff --git a/controller/test-files/event/list/ok-description.res.payload.golden.json b/controller/test-files/event/list/ok-description.res.payload.golden.json index a39dc4169a..348ca4f130 100755 --- a/controller/test-files/event/list/ok-description.res.payload.golden.json +++ b/controller/test-files/event/list/ok-description.res.payload.golden.json @@ -3,8 +3,10 @@ { "attributes": { "name": "system.description", - "newValue": null, - "oldValue": null, + "newValue": { + "content": "# Description is modified1", + "markup": "Markdown" + }, "timestamp": "0001-01-01T00:00:00Z" }, "id": "00000000-0000-0000-0000-000000000001", @@ -18,6 +20,15 @@ "related": "http:///api/users/00000000-0000-0000-0000-000000000002", "self": "http:///api/users/00000000-0000-0000-0000-000000000002" } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } } }, "type": "events" diff --git a/controller/test-files/event/list/ok-iteration.res.payload.golden.json b/controller/test-files/event/list/ok-iteration.res.payload.golden.json index 916df8e482..3b38532632 100755 --- a/controller/test-files/event/list/ok-iteration.res.payload.golden.json +++ b/controller/test-files/event/list/ok-iteration.res.payload.golden.json @@ -3,8 +3,6 @@ { "attributes": { "name": "system.iteration", - "newValue": null, - "oldValue": null, "timestamp": "0001-01-01T00:00:00Z" }, "id": "00000000-0000-0000-0000-000000000001", @@ -27,13 +25,14 @@ } ] }, - "oldValue": { - "data": [ - { - "id": "", - "type": "iterations" - } - ] + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000004", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000004" + } } }, "type": "events" diff --git a/controller/test-files/event/list/ok-kindFloat.res.payload.golden.json b/controller/test-files/event/list/ok-kindFloat.res.payload.golden.json index fb57563133..c908311315 100755 --- a/controller/test-files/event/list/ok-kindFloat.res.payload.golden.json +++ b/controller/test-files/event/list/ok-kindFloat.res.payload.golden.json @@ -3,8 +3,8 @@ { "attributes": { "name": "myFloatType", - "newValue": "2.99", - "oldValue": "1.99", + "newValue": 2.99, + "oldValue": 1.99, "timestamp": "0001-01-01T00:00:00Z" }, "id": "00000000-0000-0000-0000-000000000001", @@ -18,6 +18,15 @@ "related": "http:///api/users/00000000-0000-0000-0000-000000000002", "self": "http:///api/users/00000000-0000-0000-0000-000000000002" } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } } }, "type": "events" diff --git a/controller/test-files/event/list/ok-kindInt.res.payload.golden.json b/controller/test-files/event/list/ok-kindInt.res.payload.golden.json index 5e527b55a3..1e40fe9e36 100755 --- a/controller/test-files/event/list/ok-kindInt.res.payload.golden.json +++ b/controller/test-files/event/list/ok-kindInt.res.payload.golden.json @@ -3,8 +3,8 @@ { "attributes": { "name": "myIntType", - "newValue": "4235", - "oldValue": "200", + "newValue": 4235, + "oldValue": 200, "timestamp": "0001-01-01T00:00:00Z" }, "id": "00000000-0000-0000-0000-000000000001", @@ -18,6 +18,15 @@ "related": "http:///api/users/00000000-0000-0000-0000-000000000002", "self": "http:///api/users/00000000-0000-0000-0000-000000000002" } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } } }, "type": "events" diff --git a/controller/test-files/event/list/ok-labels.res.payload.golden.json b/controller/test-files/event/list/ok-labels.res.payload.golden.json index 0619561160..a8d3d9e061 100755 --- a/controller/test-files/event/list/ok-labels.res.payload.golden.json +++ b/controller/test-files/event/list/ok-labels.res.payload.golden.json @@ -3,8 +3,6 @@ { "attributes": { "name": "system.labels", - "newValue": null, - "oldValue": null, "timestamp": "0001-01-01T00:00:00Z" }, "id": "00000000-0000-0000-0000-000000000001", @@ -31,7 +29,15 @@ } ] }, - "oldValue": {} + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000005", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000005" + } + } }, "type": "events" } diff --git a/controller/test-files/event/list/ok-state.res.payload.golden.json b/controller/test-files/event/list/ok-state.res.payload.golden.json index 75801cf193..2ca0facef7 100755 --- a/controller/test-files/event/list/ok-state.res.payload.golden.json +++ b/controller/test-files/event/list/ok-state.res.payload.golden.json @@ -18,6 +18,15 @@ "related": "http:///api/users/00000000-0000-0000-0000-000000000002", "self": "http:///api/users/00000000-0000-0000-0000-000000000002" } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } } }, "type": "events" diff --git a/controller/test-files/event/list/ok-title.res.payload.golden.json b/controller/test-files/event/list/ok-title.res.payload.golden.json index 2edf31bb0d..cff4a3f909 100755 --- a/controller/test-files/event/list/ok-title.res.payload.golden.json +++ b/controller/test-files/event/list/ok-title.res.payload.golden.json @@ -18,6 +18,15 @@ "related": "http:///api/users/00000000-0000-0000-0000-000000000003", "self": "http:///api/users/00000000-0000-0000-0000-000000000003" } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000004", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000004" + } } }, "type": "events" diff --git a/controller/test-files/event/list/ok.bool_list.res.payload.golden.json b/controller/test-files/event/list/ok.bool_list.res.payload.golden.json new file mode 100755 index 0000000000..6957208e6a --- /dev/null +++ b/controller/test-files/event/list/ok.bool_list.res.payload.golden.json @@ -0,0 +1,74 @@ +{ + "data": [ + { + "attributes": { + "name": "bool_list", + "newValue": [ + true, + false + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "bool_list", + "newValue": [ + false, + true + ], + "oldValue": [ + true, + false + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.bool_single.res.payload.golden.json b/controller/test-files/event/list/ok.bool_single.res.payload.golden.json new file mode 100755 index 0000000000..f43e89a1da --- /dev/null +++ b/controller/test-files/event/list/ok.bool_single.res.payload.golden.json @@ -0,0 +1,65 @@ +{ + "data": [ + { + "attributes": { + "name": "bool_single", + "newValue": true, + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "bool_single", + "newValue": false, + "oldValue": true, + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.float_list.res.payload.golden.json b/controller/test-files/event/list/ok.float_list.res.payload.golden.json new file mode 100755 index 0000000000..33a67b2a50 --- /dev/null +++ b/controller/test-files/event/list/ok.float_list.res.payload.golden.json @@ -0,0 +1,74 @@ +{ + "data": [ + { + "attributes": { + "name": "float_list", + "newValue": [ + 0.1, + -1111.1 + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "float_list", + "newValue": [ + -1111.1, + 0.1 + ], + "oldValue": [ + 0.1, + -1111.1 + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.float_single.res.payload.golden.json b/controller/test-files/event/list/ok.float_single.res.payload.golden.json new file mode 100755 index 0000000000..f022cf6883 --- /dev/null +++ b/controller/test-files/event/list/ok.float_single.res.payload.golden.json @@ -0,0 +1,65 @@ +{ + "data": [ + { + "attributes": { + "name": "float_single", + "newValue": 0.1, + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "float_single", + "newValue": -1111.1, + "oldValue": 0.1, + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.integer_list.res.payload.golden.json b/controller/test-files/event/list/ok.integer_list.res.payload.golden.json new file mode 100755 index 0000000000..281c0a881e --- /dev/null +++ b/controller/test-files/event/list/ok.integer_list.res.payload.golden.json @@ -0,0 +1,74 @@ +{ + "data": [ + { + "attributes": { + "name": "integer_list", + "newValue": [ + 0, + 333 + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "integer_list", + "newValue": [ + 333, + 0 + ], + "oldValue": [ + 0, + 333 + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.integer_single.res.payload.golden.json b/controller/test-files/event/list/ok.integer_single.res.payload.golden.json new file mode 100755 index 0000000000..52e28659fd --- /dev/null +++ b/controller/test-files/event/list/ok.integer_single.res.payload.golden.json @@ -0,0 +1,65 @@ +{ + "data": [ + { + "attributes": { + "name": "integer_single", + "newValue": 0, + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "integer_single", + "newValue": 333, + "oldValue": 0, + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.markup_list.res.payload.golden.json b/controller/test-files/event/list/ok.markup_list.res.payload.golden.json new file mode 100755 index 0000000000..53b4e34b69 --- /dev/null +++ b/controller/test-files/event/list/ok.markup_list.res.payload.golden.json @@ -0,0 +1,92 @@ +{ + "data": [ + { + "attributes": { + "name": "markup_list", + "newValue": [ + { + "content": "plain text", + "markup": "PlainText" + }, + { + "content": "default", + "markup": "PlainText" + } + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "markup_list", + "newValue": [ + { + "content": "default", + "markup": "PlainText" + }, + { + "content": "plain text", + "markup": "PlainText" + } + ], + "oldValue": [ + { + "content": "plain text", + "markup": "PlainText" + }, + { + "content": "default", + "markup": "PlainText" + } + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.markup_single.res.payload.golden.json b/controller/test-files/event/list/ok.markup_single.res.payload.golden.json new file mode 100755 index 0000000000..09b3e49148 --- /dev/null +++ b/controller/test-files/event/list/ok.markup_single.res.payload.golden.json @@ -0,0 +1,74 @@ +{ + "data": [ + { + "attributes": { + "name": "markup_single", + "newValue": { + "content": "plain text", + "markup": "PlainText" + }, + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "markup_single", + "newValue": { + "content": "default", + "markup": "PlainText" + }, + "oldValue": { + "content": "plain text", + "markup": "PlainText" + }, + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.string_list.res.payload.golden.json b/controller/test-files/event/list/ok.string_list.res.payload.golden.json new file mode 100755 index 0000000000..9d7b6932b8 --- /dev/null +++ b/controller/test-files/event/list/ok.string_list.res.payload.golden.json @@ -0,0 +1,74 @@ +{ + "data": [ + { + "attributes": { + "name": "string_list", + "newValue": [ + "foo", + "bar" + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "string_list", + "newValue": [ + "bar", + "foo" + ], + "oldValue": [ + "foo", + "bar" + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.string_single.res.payload.golden.json b/controller/test-files/event/list/ok.string_single.res.payload.golden.json new file mode 100755 index 0000000000..fb6c30bf63 --- /dev/null +++ b/controller/test-files/event/list/ok.string_single.res.payload.golden.json @@ -0,0 +1,65 @@ +{ + "data": [ + { + "attributes": { + "name": "string_single", + "newValue": "foo", + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "string_single", + "newValue": "bar", + "oldValue": "foo", + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.url_list.res.payload.golden.json b/controller/test-files/event/list/ok.url_list.res.payload.golden.json new file mode 100755 index 0000000000..44e953389f --- /dev/null +++ b/controller/test-files/event/list/ok.url_list.res.payload.golden.json @@ -0,0 +1,74 @@ +{ + "data": [ + { + "attributes": { + "name": "url_list", + "newValue": [ + "127.0.0.1", + "http://www.openshift.io" + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "url_list", + "newValue": [ + "http://www.openshift.io", + "127.0.0.1" + ], + "oldValue": [ + "127.0.0.1", + "http://www.openshift.io" + ], + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/test-files/event/list/ok.url_single.res.payload.golden.json b/controller/test-files/event/list/ok.url_single.res.payload.golden.json new file mode 100755 index 0000000000..1819ed75d1 --- /dev/null +++ b/controller/test-files/event/list/ok.url_single.res.payload.golden.json @@ -0,0 +1,65 @@ +{ + "data": [ + { + "attributes": { + "name": "url_single", + "newValue": "127.0.0.1", + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000001", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + }, + { + "attributes": { + "name": "url_single", + "newValue": "http://www.openshift.io", + "oldValue": "127.0.0.1", + "timestamp": "0001-01-01T00:00:00Z" + }, + "id": "00000000-0000-0000-0000-000000000004", + "relationships": { + "modifier": { + "data": { + "id": "00000000-0000-0000-0000-000000000002", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000002", + "self": "http:///api/users/00000000-0000-0000-0000-000000000002" + } + }, + "workItemType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + } + }, + "type": "events" + } + ] +} \ No newline at end of file diff --git a/controller/work_item_events.go b/controller/work_item_events.go index c6d9e1292e..e9c088aa83 100644 --- a/controller/work_item_events.go +++ b/controller/work_item_events.go @@ -1,11 +1,14 @@ package controller import ( + "context" "net/http" "github.com/fabric8-services/fabric8-wit/app" "github.com/fabric8-services/fabric8-wit/application" "github.com/fabric8-services/fabric8-wit/jsonapi" + "github.com/fabric8-services/fabric8-wit/ptr" + "github.com/fabric8-services/fabric8-wit/rest" "github.com/fabric8-services/fabric8-wit/workitem" "github.com/fabric8-services/fabric8-wit/workitem/event" "github.com/goadesign/goa" @@ -41,165 +44,189 @@ func (c *EventsController) List(ctx *app.ListWorkItemEventsContext) error { eventList, err = appl.Events().List(ctx, ctx.WiID) return errs.Wrap(err, "list events model failed") }) + if err != nil { return jsonapi.JSONErrorResponse(ctx, err) } + + var convertedEvents []*app.Event return ctx.ConditionalEntities(eventList, c.config.GetCacheControlEvents, func() error { - res := &app.EventList{} - res.Data = ConvertEvents(c.db, ctx.Request, eventList, ctx.WiID) - return ctx.OK(res) + convertedEvents, err = ConvertEvents(ctx, c.db, ctx.Request, eventList, ctx.WiID) + if err != nil { + return jsonapi.JSONErrorResponse(ctx, errs.Wrapf(err, "failed to convert events")) + } + return ctx.OK(&app.EventList{ + Data: convertedEvents, + }) }) } // ConvertEvents from internal to external REST representation -func ConvertEvents(appl application.Application, request *http.Request, eventList []event.Event, wiID uuid.UUID) []*app.Event { +func ConvertEvents(ctx context.Context, appl application.Application, request *http.Request, eventList []event.Event, wiID uuid.UUID) ([]*app.Event, error) { var ls = []*app.Event{} for _, i := range eventList { - ls = append(ls, ConvertEvent(appl, request, i, wiID)) + converted, err := ConvertEvent(ctx, appl, request, i, wiID) + if err != nil { + return nil, errs.Wrapf(err, "failed to convert event: %+v", i) + } + ls = append(ls, converted) } - return ls + return ls, nil } // ConvertEvent converts from internal to external REST representation -func ConvertEvent(appl application.Application, request *http.Request, wiEvent event.Event, wiID uuid.UUID) *app.Event { - modifierData, modifierLinks := ConvertUserSimple(request, wiEvent.Modifier) - modifier := &app.RelationGeneric{ - Data: modifierData, - Links: modifierLinks, +func ConvertEvent(ctx context.Context, appl application.Application, req *http.Request, wiEvent event.Event, wiID uuid.UUID) (*app.Event, error) { + // find out about background details on the field that was modified + wit, err := appl.WorkItemTypes().Load(ctx, wiEvent.WorkItemTypeID) + if err != nil { + return nil, errs.Wrapf(err, "failed to load work item type: %s", wiEvent.WorkItemTypeID) } - - var e *app.Event - switch wiEvent.Name { - case workitem.SystemState, workitem.SystemTitle: - e = &app.Event{ - Type: event.APIStringTypeEvents, - ID: &wiEvent.ID, - Attributes: map[string]interface{}{ - "name": wiEvent.Name, - "newValue": wiEvent.New, - "oldValue": wiEvent.Old, - "timestamp": wiEvent.Timestamp, - }, - - Relationships: &app.EventRelations{ - Modifier: modifier, - }, - } - case workitem.SystemDescription: - e = &app.Event{ - Type: event.APIStringTypeEvents, - ID: &wiEvent.ID, - Attributes: map[string]interface{}{ - "name": wiEvent.Name, - "newValue": nil, - "oldValue": nil, - "timestamp": wiEvent.Timestamp, - }, - - Relationships: &app.EventRelations{ - Modifier: modifier, - }, - } - case workitem.SystemArea: - old, _ := ConvertAreaSimple(request, wiEvent.Old) - new, _ := ConvertAreaSimple(request, wiEvent.New) - e = &app.Event{ - Type: event.APIStringTypeEvents, - ID: &wiEvent.ID, - Attributes: map[string]interface{}{ - "name": wiEvent.Name, - "newValue": nil, - "oldValue": nil, - "timestamp": wiEvent.Timestamp, + fieldName := wiEvent.Name + fieldDef, ok := wit.Fields[fieldName] + if !ok { + return nil, errs.Errorf("failed to find field \"%s\" in work item type: %s (%s)", fieldName, wit.Name, wit.ID) + } + modifierData, modifierLinks := ConvertUserSimple(req, wiEvent.Modifier) + e := app.Event{ + Type: event.APIStringTypeEvents, + ID: wiEvent.ID, + Attributes: &app.EventAttributes{ + Name: wiEvent.Name, + Timestamp: wiEvent.Timestamp, + }, + Relationships: &app.EventRelations{ + Modifier: &app.RelationGeneric{ + Data: modifierData, + Links: modifierLinks, }, - - Relationships: &app.EventRelations{ - Modifier: modifier, - OldValue: &app.RelationGenericList{ - Data: []*app.GenericData{old}, + WorkItemType: &app.RelationGeneric{ + Links: &app.GenericLinks{ + Self: ptr.String(rest.AbsoluteURL(req, app.WorkitemtypeHref(wit.ID))), }, - NewValue: &app.RelationGenericList{ - Data: []*app.GenericData{new}, + Data: &app.GenericData{ + ID: ptr.String(wit.ID.String()), + Type: ptr.String(APIStringTypeWorkItemType), }, }, + }, + } + + // convertVal returns the given value converted from storage space to + // JSONAPI space. If the given value is supposed to be stored as a + // relationship in JSONAPI, the second return value will be true. + convertVal := func(kind workitem.Kind, val interface{}) (interface{}, bool) { + switch kind { + case workitem.KindString, + workitem.KindInteger, + workitem.KindFloat, + workitem.KindBoolean, + workitem.KindURL, + workitem.KindMarkup, + workitem.KindDuration, // TODO(kwk): get rid of duration + workitem.KindInstant: + return val, false + case workitem.KindIteration: + data, _ := ConvertIterationSimple(req, val) + return data, true + case workitem.KindUser: + data, _ := ConvertUserSimple(req, val) + return data, true + case workitem.KindLabel: + data := ConvertLabelSimple(req, val) + return data, true + case workitem.KindBoardColumn: + data := ConvertBoardColumnSimple(req, val) + return data, true + case workitem.KindArea: + data, _ := ConvertAreaSimple(req, val) + return data, true + case workitem.KindCodebase: + data, _ := ConvertCodebaseSimple(req, val) + return data, true } - case workitem.SystemIteration: - old, _ := ConvertIterationSimple(request, wiEvent.Old) - new, _ := ConvertIterationSimple(request, wiEvent.New) - e = &app.Event{ - Type: event.APIStringTypeEvents, - ID: &wiEvent.ID, - Attributes: map[string]interface{}{ - "name": wiEvent.Name, - "newValue": nil, - "oldValue": nil, - "timestamp": wiEvent.Timestamp, - }, + return nil, false + } - Relationships: &app.EventRelations{ - Modifier: modifier, - OldValue: &app.RelationGenericList{ - Data: []*app.GenericData{old}, - }, - NewValue: &app.RelationGenericList{ - Data: []*app.GenericData{new}, - }, - }, + kind := fieldDef.Type.GetKind() + if kind == workitem.KindEnum { + enumType, ok := fieldDef.Type.(workitem.EnumType) + if !ok { + return nil, errs.Errorf("failed to convert field \"%s\" to enum type: %+v", fieldName, fieldDef) } - case workitem.SystemAssignees: - e = &app.Event{ - Type: event.APIStringTypeEvents, - ID: &wiEvent.ID, - Attributes: map[string]interface{}{ - "name": wiEvent.Name, - "newValue": nil, - "oldValue": nil, - "timestamp": wiEvent.Timestamp, - }, - Relationships: &app.EventRelations{ - Modifier: modifier, - OldValue: &app.RelationGenericList{ - Data: ConvertUsersSimple(request, wiEvent.Old.([]interface{})), - }, - NewValue: &app.RelationGenericList{ - Data: ConvertUsersSimple(request, wiEvent.New.([]interface{})), - }, - }, + kind = enumType.BaseType.GetKind() + } + + // handle all single value fields (including enums) + if kind != workitem.KindList { + oldVal, useRel := convertVal(kind, wiEvent.Old) + newVal, _ := convertVal(kind, wiEvent.New) + if useRel { + if wiEvent.Old != nil { + e.Relationships.OldValue = &app.RelationGenericList{Data: []*app.GenericData{oldVal.(*app.GenericData)}} + } + if wiEvent.New != nil { + e.Relationships.NewValue = &app.RelationGenericList{Data: []*app.GenericData{newVal.(*app.GenericData)}} + } + } else { + if oldVal != nil { + e.Attributes.OldValue = &oldVal + } + if newVal != nil { + e.Attributes.NewValue = &newVal + } } - case workitem.SystemLabels: - e = &app.Event{ - Type: event.APIStringTypeEvents, - ID: &wiEvent.ID, - Attributes: map[string]interface{}{ - "name": wiEvent.Name, - "newValue": nil, - "oldValue": nil, - "timestamp": wiEvent.Timestamp, - }, - Relationships: &app.EventRelations{ - Modifier: modifier, - OldValue: &app.RelationGenericList{ - Data: ConvertLabelsSimple(request, wiEvent.Old.([]interface{})), - }, - NewValue: &app.RelationGenericList{ - Data: ConvertLabelsSimple(request, wiEvent.New.([]interface{})), - }, - }, + return &e, nil + } + + // handle multi-value fields + listType, ok := fieldDef.Type.(workitem.ListType) + if !ok { + return nil, errs.Errorf("failed to convert field \"%s\" to list type: %+v", fieldName, fieldDef) + } + componentTypeKind := listType.ComponentType.GetKind() + + arrOld, ok := wiEvent.Old.([]interface{}) + if !ok { + return nil, errs.Errorf("failed to convert old value of field \"%s\" to []interface{}: %+v", fieldName, wiEvent.Old) + } + arrNew, ok := wiEvent.New.([]interface{}) + if !ok { + return nil, errs.Errorf("failed to convert new value of field \"%s\" to []interface{}: %+v", fieldName, wiEvent.New) + } + + for i, v := range arrOld { + oldVal, useRel := convertVal(componentTypeKind, v) + if useRel { + if i == 0 { + e.Relationships.OldValue = &app.RelationGenericList{ + Data: make([]*app.GenericData, len(arrOld)), + } + } + e.Relationships.OldValue.Data[i] = oldVal.(*app.GenericData) + } else { + if i == 0 { + e.Attributes.OldValue = ptr.Interface(make([]interface{}, len(arrOld))) + } + (*e.Attributes.OldValue).([]interface{})[i] = oldVal } - default: - e = &app.Event{ - Type: event.APIStringTypeEvents, - ID: &wiEvent.ID, - Attributes: map[string]interface{}{ - "name": wiEvent.Name, - "newValue": wiEvent.New, - "oldValue": wiEvent.Old, - "timestamp": wiEvent.Timestamp, - }, - Relationships: &app.EventRelations{ - Modifier: modifier, - }, + } + + for i, v := range arrNew { + newVal, useRel := convertVal(componentTypeKind, v) + if useRel { + if i == 0 { + e.Relationships.NewValue = &app.RelationGenericList{ + Data: make([]*app.GenericData, len(arrNew)), + } + } + e.Relationships.NewValue.Data[i] = newVal.(*app.GenericData) + } else { + if i == 0 { + e.Attributes.NewValue = ptr.Interface(make([]interface{}, len(arrNew))) + } + (*e.Attributes.NewValue).([]interface{})[i] = newVal } } - return e + + return &e, nil } diff --git a/controller/work_item_events_test.go b/controller/work_item_events_test.go index 749adca5bf..47112f4112 100644 --- a/controller/work_item_events_test.go +++ b/controller/work_item_events_test.go @@ -10,6 +10,7 @@ import ( . "github.com/fabric8-services/fabric8-wit/controller" "github.com/fabric8-services/fabric8-wit/gormtestsupport" "github.com/fabric8-services/fabric8-wit/label" + "github.com/fabric8-services/fabric8-wit/rendering" "github.com/fabric8-services/fabric8-wit/resource" "github.com/fabric8-services/fabric8-wit/rest" testsupport "github.com/fabric8-services/fabric8-wit/test" @@ -38,7 +39,7 @@ func (s *TestEvent) SetupTest() { func (s *TestEvent) TestListEvent() { s.T().Run("event list ok - state", func(t *testing.T) { - fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) + fxt := tf.NewTestFixture(t, s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) svc := testsupport.ServiceAsSpaceUser("Event-Service", *fxt.Identities[0], &TestSpaceAuthzService{*fxt.Identities[0], ""}) EventCtrl := NewEventsController(svc, s.GormDB, s.Configuration) workitemCtrl := NewWorkitemController(svc, s.GormDB, s.Configuration) @@ -66,7 +67,7 @@ func (s *TestEvent) TestListEvent() { }) s.T().Run("event list ok - title", func(t *testing.T) { - fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) + fxt := tf.NewTestFixture(t, s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) svc := testsupport.ServiceAsSpaceUser("Event-Service", *fxt.Identities[0], &TestSpaceAuthzService{*fxt.Identities[0], ""}) EventCtrl := NewEventsController(svc, s.GormDB, s.Configuration) workitemCtrl := NewWorkitemController(svc, s.GormDB, s.Configuration) @@ -186,18 +187,23 @@ func (s *TestEvent) TestListEvent() { }) s.T().Run("event list ok - description", func(t *testing.T) { - fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) + fxt := tf.NewTestFixture(t, s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) svc := testsupport.ServiceAsSpaceUser("Event-Service", *fxt.Identities[0], &TestSpaceAuthzService{*fxt.Identities[0], ""}) EventCtrl := NewEventsController(svc, s.GormDB, s.Configuration) workitemCtrl := NewWorkitemController(svc, s.GormDB, s.Configuration) spaceSelfURL := rest.AbsoluteURL(&http.Request{Host: "api.service.domain.org"}, app.SpaceHref(fxt.Spaces[0].ID.String())) + + modifiedDescription := "# Description is modified1" + modifiedMarkup := rendering.SystemMarkupMarkdown + payload := app.UpdateWorkitemPayload{ Data: &app.WorkItem{ Type: APIStringTypeWorkItem, ID: &fxt.WorkItems[0].ID, Attributes: map[string]interface{}{ - workitem.SystemDescription: "New Description", - workitem.SystemVersion: fxt.WorkItems[0].Version, + workitem.SystemDescription: modifiedDescription, + workitem.SystemDescriptionMarkup: modifiedMarkup, + workitem.SystemVersion: fxt.WorkItems[0].Version, }, Relationships: &app.WorkItemRelationships{ Space: app.NewSpaceRelation(fxt.Spaces[0].ID, spaceSelfURL), @@ -214,7 +220,7 @@ func (s *TestEvent) TestListEvent() { }) s.T().Run("event list ok - assigned", func(t *testing.T) { - fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) + fxt := tf.NewTestFixture(t, s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) svc := testsupport.ServiceAsSpaceUser("Event-Service", *fxt.Identities[0], &TestSpaceAuthzService{*fxt.Identities[0], ""}) EventCtrl := NewEventsController(svc, s.GormDB, s.Configuration) assignee := []string{fxt.Identities[0].ID.String()} @@ -288,7 +294,7 @@ func (s *TestEvent) TestListEvent() { }) s.T().Run("event list ok - iteration", func(t *testing.T) { - fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) + fxt := tf.NewTestFixture(t, s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) svc := testsupport.ServiceAsSpaceUser("Event-Service", *fxt.Identities[0], &TestSpaceAuthzService{*fxt.Identities[0], ""}) EventCtrl := NewEventsController(svc, s.GormDB, s.Configuration) workitemCtrl := NewWorkitemController(svc, s.GormDB, s.Configuration) @@ -316,7 +322,7 @@ func (s *TestEvent) TestListEvent() { }) s.T().Run("event list ok - area", func(t *testing.T) { - fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) + fxt := tf.NewTestFixture(t, s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) svc := testsupport.ServiceAsSpaceUser("Event-Service", *fxt.Identities[0], &TestSpaceAuthzService{*fxt.Identities[0], ""}) EventCtrl := NewEventsController(svc, s.GormDB, s.Configuration) workitemCtrl := NewWorkitemController(svc, s.GormDB, s.Configuration) @@ -344,7 +350,7 @@ func (s *TestEvent) TestListEvent() { }) s.T().Run("event list - empty", func(t *testing.T) { - fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) + fxt := tf.NewTestFixture(t, s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) svc := testsupport.ServiceAsSpaceUser("Event-Service", *fxt.Identities[0], &TestSpaceAuthzService{*fxt.Identities[0], ""}) EventCtrl := NewEventsController(svc, s.GormDB, s.Configuration) res, eventList := test.ListWorkItemEventsOK(t, svc.Context, svc, EventCtrl, fxt.WorkItems[0].ID, nil, nil) @@ -354,7 +360,7 @@ func (s *TestEvent) TestListEvent() { }) s.T().Run("many events", func(t *testing.T) { - fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1), tf.Iterations(2)) + fxt := tf.NewTestFixture(t, s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1), tf.Iterations(2)) svc := testsupport.ServiceAsSpaceUser("Event-Service", *fxt.Identities[0], &TestSpaceAuthzService{*fxt.Identities[0], ""}) EventCtrl := NewEventsController(svc, s.GormDB, s.Configuration) workitemCtrl := NewWorkitemController(svc, s.GormDB, s.Configuration) @@ -416,4 +422,182 @@ func (s *TestEvent) TestListEvent() { require.NotEmpty(t, eventList) require.Len(t, eventList.Data, 3) }) + + s.T().Run("non-relational field kinds", func(t *testing.T) { + testData := workitem.GetFieldTypeTestData(t) + for _, kind := range testData.GetKinds() { + if !kind.IsSimpleType() || kind.IsRelational() { + continue + } + + // TODO(kwk): Once we got rid of the duration kind remove this skip + if kind == workitem.KindDuration { + continue + } + + // TODO(kwk): Once the new type system enhancements are in, also + // test instant fields + if kind == workitem.KindInstant { + continue + } + + fieldNameSingle := kind.String() + "_single" + fieldNameList := kind.String() + "_list" + // NOTE(kwk): Leave this commented out until we have proper test data + // fieldNameEnum := kind.String() + "_enum" + + fxt := tf.NewTestFixture(t, s.DB, + tf.CreateWorkItemEnvironment(), + tf.WorkItemTypes(2, func(fxt *tf.TestFixture, idx int) error { + switch idx { + case 0: + fxt.WorkItemTypes[idx].Fields[fieldNameSingle] = workitem.FieldDefinition{ + Label: fieldNameSingle, + Description: "A single value of a " + kind.String() + " object", + Type: workitem.SimpleType{Kind: kind}, + } + case 1: + fxt.WorkItemTypes[idx].Fields[fieldNameList] = workitem.FieldDefinition{ + Label: fieldNameList, + Description: "An array of " + kind.String() + " objects", + Type: workitem.ListType{ + SimpleType: workitem.SimpleType{Kind: workitem.KindList}, + ComponentType: workitem.SimpleType{Kind: kind}, + }, + } + // NOTE(kwk): Leave this commented out until we have proper test data + // case 3: + // fxt.WorkItemTypes[idx].Fields[fieldNameEnum] = workitem.FieldDefinition{ + // Label: fieldNameEnum, + // Description: "An enum value of a " + kind.String() + " object", + // Type: workitem.EnumType{ + // SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, + // BaseType: workitem.SimpleType{Kind: kind}, + // Values: []interface{}{ + // testData[kind].Valid[0], + // testData[kind].Valid[1], + // }, + // }, + // } + } + return nil + }), + tf.WorkItems(2, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Type = fxt.WorkItemTypes[idx].ID + return nil + }), + ) + svc := testsupport.ServiceAsSpaceUser("Event-Service", *fxt.Identities[0], &TestSpaceAuthzService{*fxt.Identities[0], ""}) + EventCtrl := NewEventsController(svc, s.GormDB, s.Configuration) + workitemCtrl := NewWorkitemController(svc, s.GormDB, s.Configuration) + spaceSelfURL := rest.AbsoluteURL(&http.Request{Host: "api.service.domain.org"}, app.SpaceHref(fxt.Spaces[0].ID.String())) + + t.Run(fieldNameSingle, func(t *testing.T) { + // NOTE(kwk): Leave this commented out until we have proper test data + // fieldDef := fxt.WorkItemTypes[0].Fields[fieldNameSingle] + // val, err := fieldDef.ConvertFromModel(fieldNameSingle, testData[kind].Valid[0]) + // require.NoError(t, err) + newValue := testData[kind].Valid[0] + payload := app.UpdateWorkitemPayload{ + Data: &app.WorkItem{ + Type: APIStringTypeWorkItem, + ID: &fxt.WorkItems[0].ID, + Attributes: map[string]interface{}{ + fieldNameSingle: newValue, + workitem.SystemVersion: fxt.WorkItems[0].Version, + }, + Relationships: &app.WorkItemRelationships{ + Space: app.NewSpaceRelation(fxt.Spaces[0].ID, spaceSelfURL), + }, + }, + } + // update work item once + test.UpdateWorkitemOK(t, svc.Context, svc, workitemCtrl, fxt.WorkItems[0].ID, &payload) + // update it twice + payload.Data.Attributes[workitem.SystemVersion] = fxt.WorkItems[0].Version + 1 + payload.Data.Attributes[fieldNameSingle] = testData[kind].Valid[1] + test.UpdateWorkitemOK(t, svc.Context, svc, workitemCtrl, fxt.WorkItems[0].ID, &payload) + + res, eventList := test.ListWorkItemEventsOK(t, svc.Context, svc, EventCtrl, fxt.WorkItems[0].ID, nil, nil) + safeOverriteHeader(t, res, app.ETag, "1GmclFDDPcLR1ZWPZnykWw==") + require.NotEmpty(t, eventList) + require.Len(t, eventList.Data, 2) + compareWithGoldenAgnostic(t, filepath.Join(s.testDir, "list", "ok."+fieldNameSingle+".res.payload.golden.json"), eventList) + }) + t.Run(fieldNameList, func(t *testing.T) { + // NOTE(kwk): Leave this commented out until we have proper test data + // listDef := fxt.WorkItemTypes[0].Fields[fieldNameList] + // fieldDef, ok := listDef.Type.(workitem.ListType) + // require.True(t, ok, "failed to cast %+v (%[1]T) to workitem.ListType", listDef) + // vals, err := fieldDef.ConvertFromModel([]interface{}{testData[kind].Valid[0], testData[kind].Valid[1]}) + // require.NoError(t, err) + newValue := []interface{}{testData[kind].Valid[0], testData[kind].Valid[1]} + payload := app.UpdateWorkitemPayload{ + Data: &app.WorkItem{ + Type: APIStringTypeWorkItem, + ID: &fxt.WorkItems[1].ID, + Attributes: map[string]interface{}{ + fieldNameList: newValue, + workitem.SystemVersion: fxt.WorkItems[1].Version, + }, + Relationships: &app.WorkItemRelationships{ + Space: app.NewSpaceRelation(fxt.Spaces[0].ID, spaceSelfURL), + }, + }, + } + // update work item once + test.UpdateWorkitemOK(t, svc.Context, svc, workitemCtrl, fxt.WorkItems[1].ID, &payload) + // update it twice + payload.Data.Attributes[workitem.SystemVersion] = fxt.WorkItems[1].Version + 1 + payload.Data.Attributes[fieldNameList] = []interface{}{testData[kind].Valid[1], testData[kind].Valid[0]} + test.UpdateWorkitemOK(t, svc.Context, svc, workitemCtrl, fxt.WorkItems[1].ID, &payload) + res, eventList := test.ListWorkItemEventsOK(t, svc.Context, svc, EventCtrl, fxt.WorkItems[1].ID, nil, nil) + safeOverriteHeader(t, res, app.ETag, "1GmclFDDPcLR1ZWPZnykWw==") + require.NotEmpty(t, eventList) + require.Len(t, eventList.Data, 2) + compareWithGoldenAgnostic(t, filepath.Join(s.testDir, "list", "ok."+fieldNameList+".res.payload.golden.json"), eventList) + }) + // NOTE(kwk): Leave this commented out until we have proper test data + // TODO(kwk): Once the new type system enhancements are in, also + // test for enum fields here. + + // t.Run(fieldNameEnum, func(t *testing.T) { + // // NOTE(kwk): Leave this commented out until we have proper test data + // // listDef := fxt.WorkItemTypes[0].Fields[fieldNameEnum] + // // fieldDef, ok := listDef.Type.(workitem.EnumType) + // // require.True(t, ok, "failed to cast %+v (%[1]T) to workitem.EnumType", listDef) + // // val, err := fieldDef.ConvertFromModel(testData[kind].Valid[0]) + // // require.NoError(t, err) + + // // we have to use the second value because we default to the + // // first one upon creation of the work item. + // newValue := testData[kind].Valid[1] + // payload := app.UpdateWorkitemPayload{ + // Data: &app.WorkItem{ + // Type: APIStringTypeWorkItem, + // ID: &fxt.WorkItems[2].ID, + // Attributes: map[string]interface{}{ + // fieldNameEnum: newValue, + // workitem.SystemVersion: fxt.WorkItems[2].Version, + // }, + // Relationships: &app.WorkItemRelationships{ + // Space: app.NewSpaceRelation(fxt.Spaces[0].ID, spaceSelfURL), + // }, + // }, + // } + // // update work item once + // test.UpdateWorkitemOK(t, svc.Context, svc, workitemCtrl, fxt.WorkItems[2].ID, &payload) + // // update it twice + // payload.Data.Attributes[workitem.SystemVersion] = fxt.WorkItems[2].Version + 1 + // payload.Data.Attributes[fieldNameEnum] = testData[kind].Valid[1] + // test.UpdateWorkitemOK(t, svc.Context, svc, workitemCtrl, fxt.WorkItems[2].ID, &payload) + // res, eventList := test.ListWorkItemEventsOK(t, svc.Context, svc, EventCtrl, fxt.WorkItems[2].ID, nil, nil) + // safeOverriteHeader(t, res, app.ETag, "1GmclFDDPcLR1ZWPZnykWw==") + // require.NotEmpty(t, eventList) + // require.Len(t, eventList.Data, 2) + // compareWithGoldenAgnostic(t, filepath.Join(s.testDir, "list", "ok."+fieldNameEnum+".res.payload.golden.json"), eventList) + // // compareWithGoldenAgnostic(t, filepath.Join(s.testDir, "list", "ok."+fieldNameEnum+".res.headers.golden.json"), res.Header()) + // }) + } + }) } diff --git a/design/work_item_event.go b/design/work_item_event.go index c5e2c85cb8..9c3c7a389a 100644 --- a/design/work_item_event.go +++ b/design/work_item_event.go @@ -13,12 +13,10 @@ var event = a.Type("Event", func() { a.Attribute("id", d.UUID, "ID of event", func() { a.Example("40bbdd3d-8b5d-4fd6-ac90-7236b669af04") }) - a.Attribute("attributes", a.HashOf(d.String, d.Any), func() { - a.Example(map[string]interface{}{"version": "1", "system.state": "new", "system.title": "Example story"}) - }) + a.Attribute("attributes", eventAttributes) a.Attribute("relationships", eventRelationships) a.Attribute("links", genericLinks) - a.Required("type") + a.Required("type", "relationships", "attributes", "id") }) var eventAttributes = a.Type("EventAttributes", func() { @@ -27,12 +25,12 @@ var eventAttributes = a.Type("EventAttributes", func() { a.Example("2016-11-29T23:18:14Z") }) a.Attribute("name", d.String, "The name of the event occured", func() { - a.Example("closed") + a.Example("system.title") }) - a.Attribute("oldValue", d.String, "The user who was assigned to (or unassigned from). Only for 'assigned' and 'unassigned' events.", func() { + a.Attribute("oldValue", d.Any, "The user who was assigned to (or unassigned from). Only for 'assigned' and 'unassigned' events.", func() { a.Example("813a456e-1c8a-48df-ac15-84065ee039f7") }) - a.Attribute("newValue", d.String, "The user who performed the assignment (or unassignment). Only for 'assigned' and 'unassigned' events..", func() { + a.Attribute("newValue", d.Any, "The user who performed the assignment (or unassignment). Only for 'assigned' and 'unassigned' events..", func() { a.Example("813a456e-1c8a-48df-ac15-84065ee039f7") }) a.Required("timestamp", "name") @@ -42,6 +40,9 @@ var eventRelationships = a.Type("EventRelations", func() { a.Attribute("modifier", relationGeneric, "This defines the modifier of the event") a.Attribute("oldValue", relationGenericList) a.Attribute("newValue", relationGenericList) + a.Attribute("workItemType", relationGeneric, "The type of the work item at the event's point in time") + + a.Required("workItemType", "modifier") }) var eventList = JSONList( diff --git a/workitem/enum_type.go b/workitem/enum_type.go index a0853cbced..fbc7a1eb46 100644 --- a/workitem/enum_type.go +++ b/workitem/enum_type.go @@ -80,7 +80,7 @@ func (t EnumType) ConvertToModel(value interface{}) (interface{}, error) { } if !contains(t.Values, converted) { - return nil, fmt.Errorf("not an enum value: %v", value) + return nil, fmt.Errorf("value: %+v (%[1]T) is not part of allowed enum values: %+v", value, t.Values) } return converted, nil } diff --git a/workitem/event/event.go b/workitem/event/event.go index 57bfee74d0..546f12fb0a 100644 --- a/workitem/event/event.go +++ b/workitem/event/event.go @@ -8,12 +8,13 @@ import ( // Event represents work item event type Event struct { - ID uuid.UUID - Name string - Timestamp time.Time - Modifier uuid.UUID - Old interface{} - New interface{} + ID uuid.UUID + Name string + WorkItemTypeID uuid.UUID + Timestamp time.Time + Modifier uuid.UUID + Old interface{} + New interface{} } // GetETagData returns the field values to use to generate the ETag diff --git a/workitem/event/event_repository.go b/workitem/event/event_repository.go index 2bb6e25403..6adfab9255 100644 --- a/workitem/event/event_repository.go +++ b/workitem/event/event_repository.go @@ -2,7 +2,6 @@ package event import ( "context" - "fmt" "reflect" "github.com/jinzhu/gorm" @@ -11,7 +10,6 @@ import ( "github.com/fabric8-services/fabric8-wit/account" "github.com/fabric8-services/fabric8-wit/errors" - "github.com/fabric8-services/fabric8-wit/rendering" "github.com/fabric8-services/fabric8-wit/workitem" ) @@ -48,181 +46,124 @@ type GormEventRepository struct { func (r *GormEventRepository) List(ctx context.Context, wiID uuid.UUID) ([]Event, error) { revisionList, err := r.wiRevisionRepo.List(ctx, wiID) if err != nil { - return nil, errs.Wrapf(err, "error during fetching event list") + return nil, errs.Wrapf(err, "failed to list revisions for work item: %s", wiID) } if revisionList == nil { return []Event{}, nil } - wi, err := r.workItemRepo.LoadByID(ctx, wiID) - if err != nil { - return nil, errs.Wrapf(err, "error during fetching event list") - } - wiType, err := r.workItemTypeRepo.Load(ctx, wi.Type) - if err != nil { - return nil, errs.Wrapf(err, "error during fetching event list") + if err = r.workItemRepo.CheckExists(ctx, wiID); err != nil { + return nil, errs.Wrapf(err, "failed to find work item: %s", wiID) } eventList := []Event{} for k := 1; k < len(revisionList); k++ { - modifierID, err := r.identityRepo.Load(ctx, revisionList[k].ModifierIdentity) - if err != nil { - return nil, errs.Wrapf(err, "error during fetching event list") - } - for fieldName, field := range wiType.Fields { - switch fieldType := field.Type.(type) { - case workitem.ListType: - switch fieldType.ComponentType.Kind { - case workitem.KindLabel, workitem.KindUser, workitem.KindBoardColumn: - var p []interface{} - var n []interface{} - - previousValues := revisionList[k-1].WorkItemFields[fieldName] - newValues := revisionList[k].WorkItemFields[fieldName] - switch previousValues.(type) { - case nil: - p = []interface{}{} - case []interface{}: - for _, v := range previousValues.([]interface{}) { - p = append(p, v) - } - } - switch newValues.(type) { - case nil: - n = []interface{}{} - case []interface{}: - for _, v := range newValues.([]interface{}) { - n = append(n, v) - } + oldRev := revisionList[k-1] + newRev := revisionList[k] - } + // If the new and old work item type are different, we're skipping this + // revision because it denotes the change of a work item type. + // + // TODO(kwk): make sure we have a proper "changed work item type" + // revision entry in one way or another. + if oldRev.WorkItemTypeID != newRev.WorkItemTypeID { + continue + } - // Avoid duplicate entries for empty labels or assignees - if reflect.DeepEqual(p, n) == false { - wie := Event{ - ID: revisionList[k].ID, - Name: fieldName, - Timestamp: revisionList[k].Time, - Modifier: modifierID.ID, - Old: p, - New: n, - } - eventList = append(eventList, wie) - } - default: - return nil, errors.NewNotFoundError("Unknown field:", fieldName) - } - case workitem.EnumType: - var p string - var n string + wit, err := r.workItemTypeRepo.Load(ctx, oldRev.WorkItemTypeID) + if err != nil { + return nil, errs.Wrapf(err, "failed to load old work item type: %s", oldRev.WorkItemTypeID) + } - previousValue := revisionList[k-1].WorkItemFields[fieldName] - newValue := revisionList[k].WorkItemFields[fieldName] + modifierID, err := r.identityRepo.Load(ctx, newRev.ModifierIdentity) + if err != nil { + return nil, errs.Wrapf(err, "failed to load modifier identity %s", newRev.ModifierIdentity) + } - switch previousValue.(type) { - case nil: - p = "" - case interface{}: - p, _ = previousValue.(string) - } + for fieldName, fieldDef := range wit.Fields { - switch newValue.(type) { - case nil: - n = "" - case interface{}: - n, _ = newValue.(string) + oldVal := oldRev.WorkItemFields[fieldName] + newVal := newRev.WorkItemFields[fieldName] - } - if p != n { - wie := Event{ - ID: revisionList[k].ID, - Name: fieldName, - Timestamp: revisionList[k].Time, - Modifier: modifierID.ID, - Old: p, - New: n, - } - eventList = append(eventList, wie) - } - case workitem.SimpleType: - switch fieldType.Kind { - case workitem.KindMarkup: - var p string - var n string - - previousValue := revisionList[k-1].WorkItemFields[fieldName] - newValue := revisionList[k].WorkItemFields[fieldName] - - switch previousValue.(type) { - case nil: - p = "" - case map[string]interface{}: - pv := rendering.NewMarkupContentFromMap(previousValue.(map[string]interface{})) - p = pv.Content - } + event := Event{ + ID: newRev.ID, + Name: fieldName, + WorkItemTypeID: newRev.WorkItemTypeID, + Timestamp: newRev.Time, + Modifier: modifierID.ID, + Old: oldVal, + New: newVal, + } - switch newValue.(type) { - case nil: - n = "" - case map[string]interface{}: - nv := rendering.NewMarkupContentFromMap(newValue.(map[string]interface{})) - n = nv.Content + // The enum type can be handled by the simple type since it's just a + // single value after all. + ft := fieldDef.Type + enumType, isEnumType := ft.(workitem.EnumType) + if isEnumType { + ft = enumType.BaseType + } - } + switch fieldType := ft.(type) { + case workitem.ListType: + var p, n []interface{} + var ok bool - if p != n { - wie := Event{ - ID: revisionList[k].ID, - Name: fieldName, - Timestamp: revisionList[k].Time, - Modifier: modifierID.ID, - Old: p, - New: n, - } - eventList = append(eventList, wie) + switch t := oldVal.(type) { + case nil: + p = []interface{}{} + case []interface{}: + converted, err := fieldType.ConvertFromModel(t) + if err != nil { + return nil, errs.Wrapf(err, "failed to convert old value for field %s from storage representation: %+v", fieldName, t) } - case workitem.KindString, workitem.KindIteration, workitem.KindArea, workitem.KindFloat, workitem.KindInteger: - var p string - var n string - - previousValue := revisionList[k-1].WorkItemFields[fieldName] - newValue := revisionList[k].WorkItemFields[fieldName] - - switch v := previousValue.(type) { - case nil: - p = "" - case float32, float64, int: - p = fmt.Sprintf("%g", previousValue) - case string: - p = v - default: - return nil, errors.NewConversionError("Failed to convert") + p, ok = converted.([]interface{}) + if !ok { + return nil, errs.Errorf("failed to convert old value for field %s from to []interface{}: %+v", fieldName, t) } + } - switch v := newValue.(type) { - case nil: - n = "" - case float32, float64, int: - n = fmt.Sprintf("%g", newValue) - case string: - n = v - default: - return nil, errors.NewConversionError("Failed to convert") + switch t := newVal.(type) { + case nil: + n = []interface{}{} + case []interface{}: + converted, err := fieldType.ConvertFromModel(t) + if err != nil { + return nil, errs.Wrapf(err, "failed to convert new value for field %s from storage representation: %+v", fieldName, t) } - if p != n { - wie := Event{ - ID: revisionList[k].ID, - Name: fieldName, - Timestamp: revisionList[k].Time, - Modifier: modifierID.ID, - Old: p, - New: n, - } - eventList = append(eventList, wie) + n, ok = converted.([]interface{}) + if !ok { + return nil, errs.Errorf("failed to convert new value for field %s from to []interface{}: %+v", fieldName, t) } } + + // Avoid duplicate entries for empty labels or assignees, etc. + if !reflect.DeepEqual(p, n) { + event.Old = p + event.New = n + eventList = append(eventList, event) + } + case workitem.SimpleType: + // compensate conversion from storage if this really was an enum field + converter := fieldType.ConvertFromModel + if isEnumType { + converter = enumType.ConvertFromModel + } + + p, err := converter(oldVal) + if err != nil { + return nil, errs.Wrapf(err, "failed to convert old value for field %s from storage representation: %+v", fieldName, oldVal) + } + n, err := converter(newVal) + if err != nil { + return nil, errs.Wrapf(err, "failed to convert new value for field %s from storage representation: %+v", fieldName, newVal) + } + if !reflect.DeepEqual(p, n) { + event.Old = p + event.New = n + eventList = append(eventList, event) + } default: - return nil, errors.NewNotFoundError("Unknown field:", fieldName) + return nil, errors.NewNotFoundError("unknown field type", fieldType.GetKind().String()) } } } diff --git a/workitem/event/event_repository_blackbox_test.go b/workitem/event/event_repository_blackbox_test.go index cc7bd88076..a98fec992e 100644 --- a/workitem/event/event_repository_blackbox_test.go +++ b/workitem/event/event_repository_blackbox_test.go @@ -1,7 +1,6 @@ package event_test import ( - "strconv" "testing" "github.com/fabric8-services/fabric8-wit/gormtestsupport" @@ -10,6 +9,7 @@ import ( tf "github.com/fabric8-services/fabric8-wit/test/testfixture" "github.com/fabric8-services/fabric8-wit/workitem" "github.com/fabric8-services/fabric8-wit/workitem/event" + uuid "github.com/satori/go.uuid" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "github.com/stretchr/testify/suite" @@ -59,7 +59,7 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, workitem.SystemAssignees) + assert.Equal(t, workitem.SystemAssignees, eventList[0].Name) assert.Empty(t, eventList[0].Old) assert.Equal(t, fxt.Identities[0].ID.String(), eventList[0].New.([]interface{})[0]) @@ -72,7 +72,7 @@ func (s *eventRepoBlackBoxTest) TestList() { eventList, err = s.wiEventRepo.List(s.Ctx, fxt.WorkItems[0].ID) require.NotEmpty(t, eventList) require.Len(t, eventList, 2) - assert.Equal(t, eventList[1].Name, workitem.SystemAssignees) + assert.Equal(t, workitem.SystemAssignees, eventList[1].Name) assert.NotEmpty(t, eventList[1].Old) assert.NotEmpty(t, eventList[1].New) assert.Equal(t, fxt.Identities[0].ID.String(), eventList[0].New.([]interface{})[0]) @@ -93,7 +93,7 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, workitem.SystemAssignees) + assert.Equal(t, workitem.SystemAssignees, eventList[0].Name) assert.Empty(t, eventList[0].Old) assert.Equal(t, fxt.Identities[0].ID.String(), eventList[0].New.([]interface{})[0]) }) @@ -112,9 +112,9 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - require.Equal(t, "description1", eventList[0].Old) - require.Equal(t, "description2", eventList[0].New) - require.Equal(t, wiNew.Fields[workitem.SystemDescription], newDescription) + require.Equal(t, oldDescription, eventList[0].Old) + require.Equal(t, newDescription, eventList[0].New) + require.Equal(t, newDescription, wiNew.Fields[workitem.SystemDescription]) }) s.T().Run("event assignee - new assignee nil", func(t *testing.T) { @@ -130,7 +130,7 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, workitem.SystemAssignees) + assert.Equal(t, workitem.SystemAssignees, eventList[0].Name) assert.Empty(t, eventList[0].Old) assert.Equal(t, fxt.Identities[0].ID.String(), eventList[0].New.([]interface{})[0]) @@ -142,7 +142,7 @@ func (s *eventRepoBlackBoxTest) TestList() { eventList, err = s.wiEventRepo.List(s.Ctx, fxt.WorkItems[0].ID) require.NotEmpty(t, eventList) require.Len(t, eventList, 2) - assert.Equal(t, eventList[1].Name, workitem.SystemAssignees) + assert.Equal(t, workitem.SystemAssignees, eventList[1].Name) assert.Empty(t, eventList[1].New) }) @@ -159,7 +159,7 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, workitem.SystemAssignees) + assert.Equal(t, workitem.SystemAssignees, eventList[0].Name) assert.Empty(t, eventList[0].Old) assert.Equal(t, fxt.Identities[0].ID.String(), eventList[0].New.([]interface{})[0]) @@ -171,7 +171,7 @@ func (s *eventRepoBlackBoxTest) TestList() { eventList, err = s.wiEventRepo.List(s.Ctx, fxt.WorkItems[0].ID) require.NotEmpty(t, eventList) require.Len(t, eventList, 2) - assert.Equal(t, eventList[1].Name, workitem.SystemAssignees) + assert.Equal(t, workitem.SystemAssignees, eventList[1].Name) assert.Equal(t, fxt.Identities[1].ID.String(), eventList[1].New.([]interface{})[0]) }) @@ -185,7 +185,7 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, workitem.SystemState) + assert.Equal(t, workitem.SystemState, eventList[0].Name) assert.Equal(t, workitem.SystemStateResolved, eventList[0].New) }) @@ -193,9 +193,10 @@ func (s *eventRepoBlackBoxTest) TestList() { fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1)) - label := []string{"label1"} + labelID1 := uuid.NewV4() + labels := []string{labelID1.String()} - fxt.WorkItems[0].Fields[workitem.SystemLabels] = label + fxt.WorkItems[0].Fields[workitem.SystemLabels] = labels wiNew, err := s.wiRepo.Save(s.Ctx, fxt.WorkItems[0].SpaceID, *fxt.WorkItems[0], fxt.Identities[0].ID) require.NoError(t, err) require.Len(t, wiNew.Fields[workitem.SystemLabels].([]interface{}), 1) @@ -203,12 +204,13 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, workitem.SystemLabels) + assert.Equal(t, workitem.SystemLabels, eventList[0].Name) assert.Empty(t, eventList[0].Old) - assert.Equal(t, "label1", eventList[0].New.([]interface{})[0]) + assert.Equal(t, labelID1.String(), eventList[0].New.([]interface{})[0]) - label = []string{"label2"} - wiNew.Fields[workitem.SystemLabels] = label + labelID2 := uuid.NewV4() + labels = []string{labelID2.String()} + wiNew.Fields[workitem.SystemLabels] = labels wiNew.Version = fxt.WorkItems[0].Version + 1 wiNew, err = s.wiRepo.Save(s.Ctx, fxt.WorkItems[0].SpaceID, *wiNew, fxt.Identities[0].ID) require.NoError(t, err) @@ -216,20 +218,21 @@ func (s *eventRepoBlackBoxTest) TestList() { eventList, err = s.wiEventRepo.List(s.Ctx, fxt.WorkItems[0].ID) require.NotEmpty(t, eventList) require.Len(t, eventList, 2) - assert.Equal(t, eventList[1].Name, workitem.SystemLabels) + assert.Equal(t, workitem.SystemLabels, eventList[1].Name) assert.NotEmpty(t, eventList[1].Old) assert.NotEmpty(t, eventList[1].New) - assert.Equal(t, "label1", eventList[0].New.([]interface{})[0]) - assert.Equal(t, "label2", eventList[1].New.([]interface{})[0]) + assert.Equal(t, labelID1.String(), eventList[0].New.([]interface{})[0]) + assert.Equal(t, labelID2.String(), eventList[1].New.([]interface{})[0]) }) s.T().Run("event label - previous label nil", func(t *testing.T) { fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1)) - label := []string{"label1"} + labelID1 := uuid.NewV4() + labels := []string{labelID1.String()} - fxt.WorkItems[0].Fields[workitem.SystemLabels] = label + fxt.WorkItems[0].Fields[workitem.SystemLabels] = labels wiNew, err := s.wiRepo.Save(s.Ctx, fxt.WorkItems[0].SpaceID, *fxt.WorkItems[0], fxt.Identities[0].ID) require.NoError(t, err) require.Len(t, wiNew.Fields[workitem.SystemLabels].([]interface{}), 1) @@ -237,7 +240,7 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, workitem.SystemLabels) + assert.Equal(t, workitem.SystemLabels, eventList[0].Name) assert.Empty(t, eventList[0].Old) }) @@ -245,9 +248,10 @@ func (s *eventRepoBlackBoxTest) TestList() { fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1)) - label := []string{"label1"} + labelID1 := uuid.NewV4() + labels := []string{labelID1.String()} - fxt.WorkItems[0].Fields[workitem.SystemLabels] = label + fxt.WorkItems[0].Fields[workitem.SystemLabels] = labels wiNew, err := s.wiRepo.Save(s.Ctx, fxt.WorkItems[0].SpaceID, *fxt.WorkItems[0], fxt.Identities[0].ID) require.NoError(t, err) require.Len(t, wiNew.Fields[workitem.SystemLabels].([]interface{}), 1) @@ -255,7 +259,7 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, workitem.SystemLabels) + assert.Equal(t, workitem.SystemLabels, eventList[0].Name) assert.Empty(t, eventList[0].Old) wiNew.Fields[workitem.SystemLabels] = []string{} @@ -266,7 +270,7 @@ func (s *eventRepoBlackBoxTest) TestList() { eventList, err = s.wiEventRepo.List(s.Ctx, fxt.WorkItems[0].ID) require.NotEmpty(t, eventList) require.Len(t, eventList, 2) - assert.Equal(t, eventList[1].Name, workitem.SystemLabels) + assert.Equal(t, workitem.SystemLabels, eventList[1].Name) assert.Empty(t, eventList[1].New) }) @@ -280,7 +284,7 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) require.NotEmpty(t, eventList) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, workitem.SystemIteration) + assert.Equal(t, workitem.SystemIteration, eventList[0].Name) assert.Empty(t, eventList[0].Old) wiNew.Fields[workitem.SystemIteration] = fxt.Iterations[1].ID.String() @@ -289,7 +293,7 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) eventList, err = s.wiEventRepo.List(s.Ctx, fxt.WorkItems[0].ID) require.Len(t, eventList, 2) - assert.Equal(t, eventList[1].Name, workitem.SystemIteration) + assert.Equal(t, workitem.SystemIteration, eventList[1].Name) }) s.T().Run("Field with Kind", func(t *testing.T) { @@ -317,13 +321,9 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) eventList, err := s.wiEventRepo.List(s.Ctx, fxt.WorkItems[0].ID) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, fieldName) - oldStr, _ := eventList[0].Old.(string) - old, _ := strconv.ParseFloat(oldStr, 64) - assert.Equal(t, old, initialValue) - newStr, _ := eventList[0].New.(string) - new, _ := strconv.ParseFloat(newStr, 64) - assert.Equal(t, new, updatedValue) + assert.Equal(t, fieldName, eventList[0].Name) + assert.Equal(t, initialValue, eventList[0].Old) + assert.Equal(t, updatedValue, eventList[0].New) }) t.Run("Int", func(t *testing.T) { @@ -350,21 +350,16 @@ func (s *eventRepoBlackBoxTest) TestList() { require.NoError(t, err) eventList, err := s.wiEventRepo.List(s.Ctx, fxt.WorkItems[0].ID) require.Len(t, eventList, 1) - assert.Equal(t, eventList[0].Name, fieldName) - oldStr, _ := eventList[0].Old.(string) - old, _ := strconv.ParseInt(oldStr, 10, 0) - assert.EqualValues(t, old, initialValue) - newStr, _ := eventList[0].New.(string) - new, _ := strconv.ParseInt(newStr, 10, 0) - assert.EqualValues(t, new, updatedValue) + assert.Equal(t, fieldName, eventList[0].Name) + assert.EqualValues(t, initialValue, eventList[0].Old) + assert.EqualValues(t, updatedValue, eventList[0].New) }) }) s.T().Run("multiple events", func(t *testing.T) { fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1)) - label := []string{"label1"} - fxt.WorkItems[0].Fields[workitem.SystemLabels] = label + fxt.WorkItems[0].Fields[workitem.SystemLabels] = []string{uuid.NewV4().String()} fxt.WorkItems[0].Fields[workitem.SystemState] = workitem.SystemStateResolved _, err := s.wiRepo.Save(s.Ctx, fxt.WorkItems[0].SpaceID, *fxt.WorkItems[0], fxt.Identities[0].ID) require.NoError(t, err) diff --git a/workitem/field_definition.go b/workitem/field_definition.go index 94576a9e63..413e2cc0b6 100644 --- a/workitem/field_definition.go +++ b/workitem/field_definition.go @@ -13,22 +13,25 @@ import ( // constants for describing possible field types const ( - KindString Kind = "string" - KindInteger Kind = "integer" - KindFloat Kind = "float" - KindBoolean Kind = "bool" - KindInstant Kind = "instant" - KindDuration Kind = "duration" - KindURL Kind = "url" + // non-relational + KindString Kind = "string" + KindInteger Kind = "integer" + KindFloat Kind = "float" + KindBoolean Kind = "bool" + KindInstant Kind = "instant" + KindDuration Kind = "duration" + KindURL Kind = "url" + KindMarkup Kind = "markup" + // relational KindIteration Kind = "iteration" KindUser Kind = "user" KindLabel Kind = "label" KindBoardColumn Kind = "boardcolumn" - KindEnum Kind = "enum" - KindList Kind = "list" - KindMarkup Kind = "markup" KindArea Kind = "area" KindCodebase Kind = "codebase" + // composite + KindEnum Kind = "enum" + KindList Kind = "list" ) // Kind is the kind of field type @@ -39,6 +42,21 @@ func (k Kind) IsSimpleType() bool { return k != KindEnum && k != KindList } +// IsRelational returns 'true' if the kind must be represented with a +// relationship. +func (k Kind) IsRelational() bool { + switch k { + case KindIteration, + KindUser, + KindLabel, + KindBoardColumn, + KindArea, + KindCodebase: + return true + } + return false +} + // String implements the Stringer interface and returns the kind as a string // object. func (k Kind) String() string { diff --git a/workitem/field_definition_blackbox_test.go b/workitem/field_definition_blackbox_test.go index 926736176b..b582c0e126 100644 --- a/workitem/field_definition_blackbox_test.go +++ b/workitem/field_definition_blackbox_test.go @@ -7,6 +7,8 @@ import ( "github.com/fabric8-services/fabric8-wit/resource" "github.com/fabric8-services/fabric8-wit/workitem" + uuid "github.com/satori/go.uuid" + "github.com/stretchr/testify/require" ) func testFieldDefinitionMarshalUnmarshal(t *testing.T, def workitem.FieldDefinition) { @@ -74,3 +76,24 @@ func TestFieldDefinition_Marshalling(t *testing.T) { testFieldDefinitionMarshalUnmarshal(t, def) }) } + +func TestFieldDefinition_IsRelational(t *testing.T) { + // relational kinds + require.True(t, workitem.KindLabel.IsRelational()) + require.True(t, workitem.KindArea.IsRelational()) + require.True(t, workitem.KindIteration.IsRelational()) + require.True(t, workitem.KindBoardColumn.IsRelational()) + require.True(t, workitem.KindUser.IsRelational()) + require.True(t, workitem.KindCodebase.IsRelational()) + // composite kinds + require.False(t, workitem.KindList.IsRelational()) + require.False(t, workitem.KindEnum.IsRelational()) + // non-relational kinds + require.False(t, workitem.KindString.IsRelational()) + require.False(t, workitem.KindInteger.IsRelational()) + require.False(t, workitem.KindInstant.IsRelational()) + require.False(t, workitem.KindFloat.IsRelational()) + require.False(t, workitem.KindBoolean.IsRelational()) + // random + require.False(t, workitem.Kind(uuid.NewV4().String()).IsRelational()) +} diff --git a/workitem/field_test_data.go b/workitem/field_test_data.go index e1e678d157..e075115afe 100644 --- a/workitem/field_test_data.go +++ b/workitem/field_test_data.go @@ -161,7 +161,7 @@ func GetFieldTypeTestData(t *testing.T) FieldTypeTestDataMap { return int(v) }, Valid: []interface{}{ - 0, + int(0), 333, -100, }, From 182f4802909cfdd860a60b4488902cb8add0f7ca Mon Sep 17 00:00:00 2001 From: Konrad Kleine <193408+kwk@users.noreply.github.com> Date: Thu, 16 Aug 2018 21:53:58 +0200 Subject: [PATCH 3/6] Remove unused migration test function (#2240) The same thing is tested in migration 98 function. --- migration/migration_blackbox_test.go | 6 ------ 1 file changed, 6 deletions(-) diff --git a/migration/migration_blackbox_test.go b/migration/migration_blackbox_test.go index 36b41bef9c..5b7aa66531 100644 --- a/migration/migration_blackbox_test.go +++ b/migration/migration_blackbox_test.go @@ -1235,12 +1235,6 @@ func executeSQLTestFile(filename string, args ...string) fn { } } -func testMigration95Boards(t *testing.T) { - migrateToVersion(t, sqlDB, migrations[:95], 95) - assert.True(t, dialect.HasTable("work_item_boards")) - assert.True(t, dialect.HasTable("work_item_board_columns")) -} - // test that the userspace_data table no longer exists - previously // used as a temporary solution to get data from tenant jenkins func testDropUserspacedataTable(t *testing.T) { From 2b00c92ddc078569b20016e45a283ea196006cba Mon Sep 17 00:00:00 2001 From: Ruchir Garg Date: Fri, 17 Aug 2018 15:37:15 +0530 Subject: [PATCH 4/6] Add E2E API tests to PRs (#2197) With this change the end to end tests are being executed on every PR for the fabric8-wit repo. Ref: https://github.com/fabric8-services/fabric8-wit/issues/2164 Co-authored-by: Konrad Kleine 193408+kwk@users.noreply.github.com --- .make/test.mk | 56 ++++++++++++++++++++++++++++++++++++++++++- cico_run_e2e_tests.sh | 19 +++++++++++---- cico_setup.sh | 3 ++- 3 files changed, 71 insertions(+), 7 deletions(-) diff --git a/.make/test.mk b/.make/test.mk index 622384532a..37129d135e 100644 --- a/.make/test.mk +++ b/.make/test.mk @@ -124,6 +124,12 @@ ALL_PKGS_EXCLUDE_PATTERN = "vendor\|account\/tenant\|app\'\|tool\/cli\|design\|c GOANALYSIS_PKGS_EXCLUDE_PATTERN="vendor|account/tenant|app|client|tool/cli" GOANALYSIS_DIRS=$(shell go list -f {{.Dir}} ./... | grep -v -E $(GOANALYSIS_PKGS_EXCLUDE_PATTERN)) +# temporary directory for fabric8-test +FABRIC8_E2E_TEST_DIR = $(TMP_PATH)/fabric8-test + +# fabric8-test reporsitory +FABRIC8_E2E_TEST_REPO = "https://github.com/fabric8io/fabric8-test.git" + #------------------------------------------------------------------------------- # Normal test targets # @@ -187,13 +193,50 @@ test-remote-no-coverage: prebuild-check $(SOURCES) test-migration: prebuild-check migration/sqlbindata.go migration/sqlbindata_test.go F8_RESOURCE_DATABASE=1 F8_LOG_LEVEL=$(F8_LOG_LEVEL) go test $(GO_TEST_VERBOSITY_FLAG) github.com/fabric8-services/fabric8-wit/migration +# Starts the WIT server and waits until its running +define start-wit + echo "Starting WIT and ensuring that it is running..."; \ + F8_LOG_LEVEL=ERROR ./wit+pmcd.sh & + while ! nc -z localhost 8080 < /dev/null; do \ + printf .; \ + sleep 5 ; \ + done; \ + echo "WIT is RUNNING!"; +endef + +.PHONY: test-e2e +## Runs the end-to-end tests WITHOUT producing coverage files for each package. +test-e2e: $(BINARY_SERVER_BIN) + $(call log-info,"Running tests: $@") +ifeq ($(OS),Windows_NT) + $(error End to end tests currently cannot run on Windows based operating systems.) +endif + $(call download-docker-compose) +ifeq ($(UNAME_S),Darwin) + @echo "Running docker-compose with macOS network settings" + $(DOCKER_COMPOSE_BIN_ALT) -f docker-compose.macos.yml up -d db auth +else + @echo "Running docker-compose with Linux network settings" + $(DOCKER_COMPOSE_BIN_ALT) up -d db auth +endif + # Start the WIT server + $(call start-wit) + # Clone the fabric8-test repo + @if [ "$(FABRIC8_E2E_TEST_DIR)" ]; then \ + echo "Removing any existing dir $(FABRIC8_E2E_TEST_DIR)"; \ + rm -rf $(FABRIC8_E2E_TEST_DIR); \ + fi + $(GIT_BIN_NAME) clone --depth=1 $(FABRIC8_E2E_TEST_REPO) $(FABRIC8_E2E_TEST_DIR) + # Install e2e test deps and run the tests + $(FABRIC8_E2E_TEST_DIR)/EE_API_automation/cico_run_EE_tests_wit.sh + # Downloads docker-compose to tmp/docker-compose if it does not already exist. define download-docker-compose @if [ ! -f "$(DOCKER_COMPOSE_BIN_ALT)" ]; then \ echo "Downloading docker-compose to $(DOCKER_COMPOSE_BIN_ALT)"; \ UNAME_S=$(shell uname -s); \ UNAME_M=$(shell uname -m); \ - URL="https://github.com/docker/compose/releases/download/1.11.2/docker-compose-$${UNAME_S}-$${UNAME_M}"; \ + URL="https://github.com/docker/compose/releases/download/1.22.0/docker-compose-$${UNAME_S}-$${UNAME_M}"; \ curl --silent -L $${URL} > $(DOCKER_COMPOSE_BIN_ALT); \ chmod +x $(DOCKER_COMPOSE_BIN_ALT); \ fi @@ -542,3 +585,14 @@ CLEAN_TARGETS += clean-coverage-remote ## Removes remote test coverage file clean-coverage-remote: -@rm -f $(COV_PATH_REMOTE) + +CLEAN_TARGETS += clean-e2e +.PHONY: clean-e2e +## Cleans up environment of e2e tests +clean-e2e: + ## Kills the WIT process + $(shell ps aux | grep 'bin/[w]it' | awk '{print $$2}' | xargs kill) + ## Removes the end-to-end (e2e) test directory + -rm -rf $(FABRIC8_E2E_TEST_DIR) + + diff --git a/cico_run_e2e_tests.sh b/cico_run_e2e_tests.sh index b6ee4586a3..67d6783c29 100755 --- a/cico_run_e2e_tests.sh +++ b/cico_run_e2e_tests.sh @@ -1,8 +1,17 @@ #!/bin/bash -echo "The end to end tests check will be implemented in" -echo "https://github.com/fabric8-services/fabric8-wit/pull/2197 " -echo "but for the CI to execute the task in an individual job we need" -echo "to have an \".sh\" file in place." +. cico_setup.sh -exit 1 +load_jenkins_vars; + +install_deps; + +make docker-start + +make docker-build + +trap "make clean-e2e" EXIT + +make test-e2e + +echo "CICO: ran e2e-tests" \ No newline at end of file diff --git a/cico_setup.sh b/cico_setup.sh index 59673f42be..f00af10ffd 100644 --- a/cico_setup.sh +++ b/cico_setup.sh @@ -38,7 +38,8 @@ function install_deps() { docker \ make \ git \ - curl + curl \ + nc service docker start From 95a5119028db31ce8b78531a08d076df909b8830 Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Fri, 17 Aug 2018 16:34:58 +0530 Subject: [PATCH 5/6] Prevent double escaping of comments (#2236) --- controller/comments.go | 3 +-- controller/comments_blackbox_test.go | 18 ++++++++++++++++-- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/controller/comments.go b/controller/comments.go index 93038d3268..5cc7d00790 100644 --- a/controller/comments.go +++ b/controller/comments.go @@ -3,7 +3,6 @@ package controller import ( "context" "fmt" - "html" "net/http" "github.com/fabric8-services/fabric8-wit/app" @@ -216,7 +215,7 @@ func ConvertComment(request *http.Request, comment comment.Comment, additional . ID: &comment.ID, Attributes: &app.CommentAttributes{ Body: &comment.Body, - BodyRendered: ptr.String(rendering.RenderMarkupToHTML(html.EscapeString(comment.Body), comment.Markup)), + BodyRendered: ptr.String(rendering.RenderMarkupToHTML(comment.Body, comment.Markup)), Markup: ptr.String(rendering.NilSafeGetMarkup(&comment.Markup)), CreatedAt: &comment.CreatedAt, UpdatedAt: &comment.UpdatedAt, diff --git a/controller/comments_blackbox_test.go b/controller/comments_blackbox_test.go index 48c2a427c1..e93dab962c 100644 --- a/controller/comments_blackbox_test.go +++ b/controller/comments_blackbox_test.go @@ -3,7 +3,6 @@ package controller_test import ( "context" "fmt" - "html" "strings" "testing" "time" @@ -155,7 +154,7 @@ func assertComment(t *testing.T, resultData *app.Comment, expectedIdentity accou assert.Equal(t, expectedBody, *resultData.Attributes.Body) require.NotNil(t, resultData.Attributes.Markup) assert.Equal(t, expectedMarkup, *resultData.Attributes.Markup) - assert.Equal(t, rendering.RenderMarkupToHTML(html.EscapeString(expectedBody), expectedMarkup), *resultData.Attributes.BodyRendered) + assert.Equal(t, rendering.RenderMarkupToHTML(expectedBody, expectedMarkup), *resultData.Attributes.BodyRendered) require.NotNil(t, resultData.Relationships) require.NotNil(t, resultData.Relationships.Creator) require.NotNil(t, resultData.Relationships.Creator.Data) @@ -302,6 +301,21 @@ func (s *CommentsSuite) TestShowCommentWithEscapedScriptInjection() { assertComment(s.T(), result.Data, s.testIdentity, "", rendering.SystemMarkupPlainText) } +func (s *CommentsSuite) TestShowCommentWithTextAndCodeblock() { + // given + fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) + wiID := fxt.WorkItems[0].ID + body := "Hello, World \n```\n { \"foo\":\"bar\" } \n``` " + expectedBody := "

Hello, World

\n\n
 { "foo":"bar" } \n
\n" + c := s.createWorkItemComment(s.testIdentity, wiID, body, &markdownMarkup, nil) + // when + userSvc, _, _, _, commentsCtrl := s.securedControllers(s.testIdentity) + _, result := test.ShowCommentsOK(s.T(), userSvc.Context, userSvc, commentsCtrl, *c.Data.ID, nil, nil) + // then + assert.Equal(s.T(), body, *result.Data.Attributes.Body) + assert.Equal(s.T(), expectedBody, *result.Data.Attributes.BodyRendered) +} + func (s *CommentsSuite) TestUpdateCommentWithoutAuth() { // given fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1)) From b688f8f5c16431f1da129ab5492ef096c7ad0ccf Mon Sep 17 00:00:00 2001 From: Ibrahim Jarif Date: Mon, 20 Aug 2018 12:23:21 +0530 Subject: [PATCH 6/6] Workitem Type Change (#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 https://github.com/fabric8-services/fabric8-wit/pull/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 --- .../workitem_type.res.payload.golden.json | 95 +++++ controller/workitem.go | 59 ++- controller/workitem_blackbox_test.go | 137 ++++++- workitem/simple_type.go | 6 + workitem/workitem_repository.go | 342 ++++++++++++++++-- workitem/workitem_repository_blackbox_test.go | 224 +++++++++++- workitem/workitemtype.go | 1 + 7 files changed, 820 insertions(+), 44 deletions(-) create mode 100755 controller/test-files/work_item/update/workitem_type.res.payload.golden.json diff --git a/controller/test-files/work_item/update/workitem_type.res.payload.golden.json b/controller/test-files/work_item/update/workitem_type.res.payload.golden.json new file mode 100755 index 0000000000..58462c84e9 --- /dev/null +++ b/controller/test-files/work_item/update/workitem_type.res.payload.golden.json @@ -0,0 +1,95 @@ +{ + "data": { + "attributes": { + "bar": null, + "fooBar": "alpha", + "fooo": 2.5, + "integer-or-float-list": [], + "system.created_at": "0001-01-01T00:00:00Z", + "system.description": "```\nMissing fields in workitem type: Second WorkItem Type\n\nType1 Assigned To : First User (jon_doe), Second User (lorem_ipsum)\nType1 bar : hello\nType1 fooBar : open\nType1 integer-or-float-list : 101\nType1 reporter : First User (jon_doe)\n```\ndescription1\n", + "system.description.markup": "Markdown", + "system.description.rendered": "\u003cpre\u003e\u003ccode class=\"prettyprint\"\u003e\u003cspan class=\"typ\"\u003eMissing\u003c/span\u003e \u003cspan class=\"pln\"\u003efields\u003c/span\u003e \u003cspan class=\"kwd\"\u003ein\u003c/span\u003e \u003cspan class=\"pln\"\u003eworkitem\u003c/span\u003e \u003cspan class=\"kwd\"\u003etype\u003c/span\u003e\u003cspan class=\"pun\"\u003e:\u003c/span\u003e \u003cspan class=\"typ\"\u003eSecond\u003c/span\u003e \u003cspan class=\"typ\"\u003eWorkItem\u003c/span\u003e \u003cspan class=\"typ\"\u003eType\u003c/span\u003e\n\n\u003cspan class=\"typ\"\u003eType1\u003c/span\u003e \u003cspan class=\"typ\"\u003eAssigned\u003c/span\u003e \u003cspan class=\"typ\"\u003eTo\u003c/span\u003e \u003cspan class=\"pun\"\u003e:\u003c/span\u003e \u003cspan class=\"typ\"\u003eFirst\u003c/span\u003e \u003cspan class=\"typ\"\u003eUser\u003c/span\u003e \u003cspan class=\"pun\"\u003e(\u003c/span\u003e\u003cspan class=\"pln\"\u003ejon_doe\u003c/span\u003e\u003cspan class=\"pun\"\u003e)\u003c/span\u003e\u003cspan class=\"pun\"\u003e,\u003c/span\u003e \u003cspan class=\"typ\"\u003eSecond\u003c/span\u003e \u003cspan class=\"typ\"\u003eUser\u003c/span\u003e \u003cspan class=\"pun\"\u003e(\u003c/span\u003e\u003cspan class=\"pln\"\u003elorem_ipsum\u003c/span\u003e\u003cspan class=\"pun\"\u003e)\u003c/span\u003e\n\u003cspan class=\"typ\"\u003eType1\u003c/span\u003e \u003cspan class=\"pln\"\u003ebar\u003c/span\u003e \u003cspan class=\"pun\"\u003e:\u003c/span\u003e \u003cspan class=\"pln\"\u003ehello\u003c/span\u003e\n\u003cspan class=\"typ\"\u003eType1\u003c/span\u003e \u003cspan class=\"pln\"\u003efooBar\u003c/span\u003e \u003cspan class=\"pun\"\u003e:\u003c/span\u003e \u003cspan class=\"pln\"\u003eopen\u003c/span\u003e\n\u003cspan class=\"typ\"\u003eType1\u003c/span\u003e \u003cspan class=\"pln\"\u003einteger\u003c/span\u003e\u003cspan class=\"pun\"\u003e-\u003c/span\u003e\u003cspan class=\"kwd\"\u003eor\u003c/span\u003e\u003cspan class=\"pun\"\u003e-\u003c/span\u003e\u003cspan class=\"kwd\"\u003efloat\u003c/span\u003e\u003cspan class=\"pun\"\u003e-\u003c/span\u003e\u003cspan class=\"pln\"\u003elist\u003c/span\u003e \u003cspan class=\"pun\"\u003e:\u003c/span\u003e \u003cspan class=\"dec\"\u003e101\u003c/span\u003e\n\u003cspan class=\"typ\"\u003eType1\u003c/span\u003e \u003cspan class=\"pln\"\u003ereporter\u003c/span\u003e \u003cspan class=\"pun\"\u003e:\u003c/span\u003e \u003cspan class=\"typ\"\u003eFirst\u003c/span\u003e \u003cspan class=\"typ\"\u003eUser\u003c/span\u003e \u003cspan class=\"pun\"\u003e(\u003c/span\u003e\u003cspan class=\"pln\"\u003ejon_doe\u003c/span\u003e\u003cspan class=\"pun\"\u003e)\u003c/span\u003e\n\u003c/code\u003e\u003c/pre\u003e\n\n\u003cp\u003edescription1\u003c/p\u003e\n", + "system.metastate": null, + "system.number": 1, + "system.order": 1000, + "system.remote_item_id": null, + "system.state": "new", + "system.title": "work item 00000000-0000-0000-0000-000000000001", + "system.updated_at": "0001-01-01T00:00:00Z", + "version": 1 + }, + "id": "00000000-0000-0000-0000-000000000002", + "links": { + "related": "http:///api/workitems/00000000-0000-0000-0000-000000000002", + "self": "http:///api/workitems/00000000-0000-0000-0000-000000000002" + }, + "relationships": { + "area": {}, + "assignees": {}, + "baseType": { + "data": { + "id": "00000000-0000-0000-0000-000000000003", + "type": "workitemtypes" + }, + "links": { + "self": "http:///api/workitemtypes/00000000-0000-0000-0000-000000000003" + } + }, + "children": { + "links": { + "related": "http:///api/workitems/00000000-0000-0000-0000-000000000002/children" + }, + "meta": { + "hasChildren": false + } + }, + "comments": { + "links": { + "related": "http:///api/workitems/00000000-0000-0000-0000-000000000002/comments", + "self": "http:///api/workitems/00000000-0000-0000-0000-000000000002/relationships/comments" + } + }, + "creator": { + "data": { + "id": "00000000-0000-0000-0000-000000000004", + "type": "users" + }, + "links": { + "related": "http:///api/users/00000000-0000-0000-0000-000000000004", + "self": "http:///api/users/00000000-0000-0000-0000-000000000004" + } + }, + "events": { + "links": { + "related": "http:///api/workitems/00000000-0000-0000-0000-000000000002/events" + } + }, + "iteration": {}, + "labels": { + "links": { + "related": "http:///api/workitems/00000000-0000-0000-0000-000000000002/labels" + } + }, + "space": { + "data": { + "id": "00000000-0000-0000-0000-000000000005", + "type": "spaces" + }, + "links": { + "related": "http:///api/spaces/00000000-0000-0000-0000-000000000005", + "self": "http:///api/spaces/00000000-0000-0000-0000-000000000005" + } + }, + "system.boardcolumns": {}, + "workItemLinks": { + "links": { + "related": "http:///api/workitems/00000000-0000-0000-0000-000000000002/links" + } + } + }, + "type": "workitems" + }, + "links": { + "self": "http:///api/workitems/00000000-0000-0000-0000-000000000002" + } +} \ No newline at end of file diff --git a/controller/workitem.go b/controller/workitem.go index d14e028345..88c2161ca6 100644 --- a/controller/workitem.go +++ b/controller/workitem.go @@ -71,6 +71,25 @@ func NewNotifyingWorkitemController(service *goa.Service, db application.DB, not config: config} } +// authorizeWorkitemTypeEditor returns true if the modifier is allowed to change +// workitem type else it returns false. +// Only space owner and workitem creator are allowed to change workitem type +func (c *WorkitemController) authorizeWorkitemTypeEditor(ctx context.Context, spaceID uuid.UUID, creatorID string, editorID string) (bool, error) { + // check if workitem editor is same as workitem creator + if editorID == creatorID { + return true, nil + } + space, err := c.db.Spaces().Load(ctx, spaceID) + if err != nil { + return false, errors.NewNotFoundError("space", spaceID.String()) + } + // check if workitem editor is same as space owner + if space != nil && editorID == space.OwnerID.String() { + return true, nil + } + return false, errors.NewUnauthorizedError("user is not allowed to change workitem type") +} + // Returns true if the user is the work item creator or space collaborator func authorizeWorkitemEditor(ctx context.Context, db application.DB, spaceID uuid.UUID, creatorID string, editorID string) (bool, error) { if editorID == creatorID { @@ -111,18 +130,48 @@ func (c *WorkitemController) Update(ctx *app.UpdateWorkitemContext) error { if !authorized { return jsonapi.JSONErrorResponse(ctx, errors.NewForbiddenError("user is not authorized to access the space")) } + + if ctx.Payload.Data.Relationships != nil && ctx.Payload.Data.Relationships.BaseType != nil && + ctx.Payload.Data.Relationships.BaseType.Data != nil && ctx.Payload.Data.Relationships.BaseType.Data.ID != wi.Type { + + authorized, err := c.authorizeWorkitemTypeEditor(ctx, wi.SpaceID, creator.(string), currentUserIdentityID.String()) + if err != nil { + return jsonapi.JSONErrorResponse(ctx, err) + } + if !authorized { + return jsonapi.JSONErrorResponse(ctx, errors.NewForbiddenError("user is not authorized to change the workitemtype")) + } + // Store new values of type and version + newType := ctx.Payload.Data.Relationships.BaseType + newVersion := ctx.Payload.Data.Attributes[workitem.SystemVersion] + + // Remove version and base type from payload + 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 + if (app.WorkItemRelationships{}) != *ctx.Payload.Data.Relationships || len(ctx.Payload.Data.Attributes) > 0 { + // Todo(ibrahim) - Change this error to 422 Unprocessable entity + // error once we have this error in our error package. Please see + // https://github.com/fabric8-services/fabric8-wit/pull/2202#discussion_r208842063 + return jsonapi.JSONErrorResponse(ctx, errors.NewBadParameterErrorFromString("cannot update type along with other fields")) + } + + // Restore the original values + ctx.Payload.Data.Relationships.BaseType = newType + ctx.Payload.Data.Attributes[workitem.SystemVersion] = newVersion + + } err = application.Transactional(c.db, func(appl application.Application) error { - // The Number and Type of a work item are not allowed to be changed - // which is why we overwrite those values with their old value after the - // work item was converted. + // The Number of a work item is not allowed to be changed which is why + // we overwrite the values with its old value after the work item was + // converted. oldNumber := wi.Number - oldType := wi.Type err = ConvertJSONAPIToWorkItem(ctx, ctx.Method, appl, *ctx.Payload.Data, wi, wi.Type, wi.SpaceID) if err != nil { return err } wi.Number = oldNumber - wi.Type = oldType wi, err = appl.WorkItems().Save(ctx, wi.SpaceID, *wi, *currentUserIdentityID) if err != nil { return errs.Wrap(err, "Error updating work item") diff --git a/controller/workitem_blackbox_test.go b/controller/workitem_blackbox_test.go index 3ca62f4198..7b7f57e3e2 100644 --- a/controller/workitem_blackbox_test.go +++ b/controller/workitem_blackbox_test.go @@ -947,7 +947,7 @@ func (s *WorkItem2Suite) TestWI2UpdateWithNonExistentID() { func (s *WorkItem2Suite) TestWI2UpdateSetReadOnlyFields() { // given - fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1), tf.WorkItemTypes(2)) + fxt := tf.NewTestFixture(s.T(), s.DB, tf.CreateWorkItemEnvironment(), tf.WorkItems(1), tf.WorkItemTypes(1)) u := minimumRequiredUpdatePayload() u.Data.Attributes[workitem.SystemTitle] = "Test title" @@ -955,7 +955,7 @@ func (s *WorkItem2Suite) TestWI2UpdateSetReadOnlyFields() { u.Data.Attributes[workitem.SystemNumber] = fxt.WorkItems[0].Number + 666 u.Data.ID = &fxt.WorkItems[0].ID u.Data.Relationships = &app.WorkItemRelationships{ - BaseType: newRelationBaseType(fxt.WorkItemTypes[1].ID), + BaseType: newRelationBaseType(fxt.WorkItemTypes[0].ID), } // when @@ -969,6 +969,139 @@ func (s *WorkItem2Suite) TestWI2UpdateSetReadOnlyFields() { }) } +func (s *WorkItem2Suite) TestWI2UpdateWorkItemType() { + userFullName := []string{"First User", "Second User"} + userUserName := []string{"jon_doe", "lorem_ipsum"} + fxt := tf.NewTestFixture(s.T(), s.DB, + tf.CreateWorkItemEnvironment(), + tf.Users(2, func(fxt *tf.TestFixture, idx int) error { + fxt.Users[idx].FullName = userFullName[idx] + return nil + }), + tf.Identities(2, func(fxt *tf.TestFixture, idx int) error { + fxt.Identities[idx].Username = userUserName[idx] + fxt.Identities[idx].User = *fxt.Users[idx] + return nil + }), + tf.WorkItemTypes(2, func(fxt *tf.TestFixture, idx int) error { + switch idx { + case 0: + fxt.WorkItemTypes[idx].Name = "First WorkItem Type" + fxt.WorkItemTypes[idx].Fields = map[string]workitem.FieldDefinition{ + "fooo": { + Label: "Type1 fooo", + Type: &workitem.SimpleType{Kind: workitem.KindFloat}, + }, + "fooBar": { + Label: "Type1 fooBar", + Type: workitem.EnumType{ + BaseType: workitem.SimpleType{Kind: workitem.KindString}, + SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, + Values: []interface{}{"open", "done", "closed"}, + }, + }, + "assigned-to": { + Label: "Type1 Assigned To", + Type: workitem.ListType{ + SimpleType: workitem.SimpleType{Kind: workitem.KindList}, + ComponentType: workitem.SimpleType{Kind: workitem.KindUser}, + }, + }, + "bar": { + Label: "Type1 bar", + Type: &workitem.SimpleType{Kind: workitem.KindString}, + }, + "reporter": { + Label: "Type1 reporter", + Type: &workitem.SimpleType{Kind: workitem.KindUser}, + }, + "integer-or-float-list": { + Label: "Type1 integer-or-float-list", + Type: workitem.ListType{ + SimpleType: workitem.SimpleType{Kind: workitem.KindList}, + ComponentType: workitem.SimpleType{Kind: workitem.KindInteger}, + }, + }, + } + case 1: + fxt.WorkItemTypes[idx].Name = "Second WorkItem Type" + fxt.WorkItemTypes[idx].Fields = map[string]workitem.FieldDefinition{ + "fooo": { + Label: "Type2 fooo", + Type: &workitem.SimpleType{Kind: workitem.KindFloat}, + }, + "bar": { + Label: "Type2 bar", + Type: &workitem.SimpleType{Kind: workitem.KindInteger}, + }, + "fooBar": { + Label: "Type2 fooBar", + Type: workitem.EnumType{ + BaseType: workitem.SimpleType{Kind: workitem.KindString}, + SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, + Values: []interface{}{"alpha", "beta", "gamma"}, + }, + }, + "integer-or-float-list": { + Label: "Type2 integer-or-float-list", + Type: workitem.ListType{ + SimpleType: workitem.SimpleType{Kind: workitem.KindList}, + ComponentType: workitem.SimpleType{Kind: workitem.KindFloat}, + }, + }, + } + } + return nil + }), + tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Type = fxt.WorkItemTypes[0].ID + fxt.WorkItems[idx].Fields["integer-or-float-list"] = []int{101} + fxt.WorkItems[idx].Fields["fooo"] = 2.5 + fxt.WorkItems[idx].Fields["fooBar"] = "open" + fxt.WorkItems[idx].Fields["bar"] = "hello" + fxt.WorkItems[idx].Fields["reporter"] = fxt.Identities[0].ID.String() + fxt.WorkItems[idx].Fields["assigned-to"] = []string{fxt.Identities[0].ID.String(), fxt.Identities[1].ID.String()} + fxt.WorkItems[idx].Fields[workitem.SystemDescription] = rendering.NewMarkupContentFromLegacy("description1") + return nil + }), + ) + // when + u := minimumRequiredUpdatePayload() + u.Data.Attributes[workitem.SystemVersion] = fxt.WorkItems[0].Version + u.Data.ID = &fxt.WorkItems[0].ID + u.Data.Relationships = &app.WorkItemRelationships{ + BaseType: newRelationBaseType(fxt.WorkItemTypes[1].ID), + } + svc := testsupport.ServiceAsUser("TypeChangeService", *fxt.Identities[0]) + s.T().Run("ok", func(t *testing.T) { + _, newWI := test.UpdateWorkitemOK(t, svc.Context, svc, s.workitemCtrl, fxt.WorkItems[0].ID, &u) + + assert.Equal(t, fxt.WorkItemTypes[1].ID, newWI.Data.Relationships.BaseType.Data.ID) + newDescription := newWI.Data.Attributes[workitem.SystemDescription] + assert.NotNil(t, newDescription) + // Type of old and new field is same + assert.NotContains(t, newDescription, fxt.WorkItemTypes[0].Fields["fooo"].Label) + assert.Contains(t, newDescription, fxt.WorkItemTypes[0].Fields["bar"].Label) + assert.Contains(t, newDescription, fxt.WorkItemTypes[0].Fields["fooBar"].Label) + assert.Equal(t, "alpha", newWI.Data.Attributes["fooBar"]) // First value of enum for field foobar + compareWithGoldenAgnostic(t, filepath.Join(s.testDir, "update", "workitem_type.res.payload.golden.json"), newWI) + }) + + s.T().Run("disallow update of field along with type", func(t *testing.T) { + u.Data.Attributes[workitem.SystemTitle] = "xyz" + // TODO (ibrahim) - Check type of error once error 422 has been added. + //https://github.com/fabric8-services/fabric8-wit/pull/2202#discussion_r210184092 + test.UpdateWorkitemConflict(t, svc.Context, svc, s.workitemCtrl, fxt.WorkItems[0].ID, &u) + }) + + s.T().Run("unauthorized", func(t *testing.T) { + // Only Space owner and workitem creator is allowed to change type + svcNotAuthorized := testsupport.ServiceAsSpaceUser("TypeChange-Service", *fxt.Identities[1], &TestSpaceAuthzService{*fxt.Identities[0], ""}) + workitemCtrlNotAuthorized := NewWorkitemController(svcNotAuthorized, s.GormDB, s.Configuration) + test.UpdateWorkitemForbidden(t, svcNotAuthorized.Context, svcNotAuthorized, workitemCtrlNotAuthorized, fxt.WorkItems[0].ID, &u) + }) +} + func (s *WorkItem2Suite) TestWI2UpdateFieldOfDifferentSimpleTypes() { s.T().Run("field types", func(t *testing.T) { vals := workitem.GetFieldTypeTestData(t) diff --git a/workitem/simple_type.go b/workitem/simple_type.go index a636e6bfc8..60e95fa9a4 100644 --- a/workitem/simple_type.go +++ b/workitem/simple_type.go @@ -111,6 +111,12 @@ func (t SimpleType) ConvertToModel(value interface{}) (interface{}, error) { return nil, errs.Errorf("value %v (%[1]T) has no valid markup type %s", value, markupContent.Markup) } return markupContent.ToMap(), nil + case map[string]interface{}: + markupContent := rendering.NewMarkupContentFromValue(value) + if !rendering.IsMarkupSupported(markupContent.Markup) { + return nil, errs.Errorf("value %v (%[1]T) has no valid markup type %s", value, markupContent.Markup) + } + return markupContent.ToMap(), nil default: return nil, errs.Errorf("value %v (%[1]T) should be rendering.MarkupContent, but is %s", value, valueType) } diff --git a/workitem/workitem_repository.go b/workitem/workitem_repository.go index 448565025e..a375297d0d 100644 --- a/workitem/workitem_repository.go +++ b/workitem/workitem_repository.go @@ -1,16 +1,22 @@ package workitem import ( + "bytes" "context" "fmt" + "sort" "strconv" "strings" + "text/template" "time" - "github.com/fabric8-services/fabric8-wit/closeable" + "github.com/fabric8-services/fabric8-wit/label" "github.com/fabric8-services/fabric8-wit/account" "github.com/fabric8-services/fabric8-wit/application/repository" + "github.com/fabric8-services/fabric8-wit/area" + "github.com/fabric8-services/fabric8-wit/closeable" + "github.com/fabric8-services/fabric8-wit/codebase" "github.com/fabric8-services/fabric8-wit/criteria" "github.com/fabric8-services/fabric8-wit/errors" "github.com/fabric8-services/fabric8-wit/iteration" @@ -97,25 +103,28 @@ type WorkItemRepository interface { GetCountsPerIteration(ctx context.Context, spaceID uuid.UUID) (map[string]WICountsPerIteration, error) GetCountsForIteration(ctx context.Context, itr *iteration.Iteration) (map[string]WICountsPerIteration, error) Count(ctx context.Context, spaceID uuid.UUID, criteria criteria.Expression) (int, error) + ChangeWorkItemType(ctx context.Context, wiStorage *WorkItemStorage, oldWIType *WorkItemType, newWIType *WorkItemType, spaceID uuid.UUID) error } // NewWorkItemRepository creates a GormWorkItemRepository func NewWorkItemRepository(db *gorm.DB) *GormWorkItemRepository { repository := &GormWorkItemRepository{ - db: db, - winr: numbersequence.NewWorkItemNumberSequenceRepository(db), - witr: &GormWorkItemTypeRepository{db}, - wirr: &GormRevisionRepository{db}, + db: db, + winr: numbersequence.NewWorkItemNumberSequenceRepository(db), + witr: &GormWorkItemTypeRepository{db}, + wirr: &GormRevisionRepository{db}, + space: space.NewRepository(db), } return repository } // GormWorkItemRepository implements WorkItemRepository using gorm type GormWorkItemRepository struct { - db *gorm.DB - winr *numbersequence.GormWorkItemNumberSequenceRepository - witr *GormWorkItemTypeRepository - wirr *GormRevisionRepository + db *gorm.DB + winr *numbersequence.GormWorkItemNumberSequenceRepository + witr *GormWorkItemTypeRepository + wirr *GormRevisionRepository + space *space.GormRepository } // ************************************************ @@ -578,9 +587,7 @@ func (r *GormWorkItemRepository) Save(ctx context.Context, spaceID uuid.UUID, up return nil, errors.NewVersionConflictError("version conflict") } wiStorage.Version = wiStorage.Version + 1 - wiStorage.Type = updatedWorkItem.Type wiStorage.Fields = Fields{} - for fieldName, fieldDef := range wiType.Fields { if fieldDef.ReadOnly { continue @@ -606,6 +613,18 @@ func (r *GormWorkItemRepository) Save(ctx context.Context, spaceID uuid.UUID, up return nil, errors.NewBadParameterError(fieldName, fieldValue) } } + // Change of Work Item Type + if wiStorage.Type != updatedWorkItem.Type { + newWiType, err := r.witr.Load(ctx, updatedWorkItem.Type) + if err != nil { + return nil, errs.Wrapf(err, "failed to load workitemtype: %s ", updatedWorkItem.Type) + } + if err := r.ChangeWorkItemType(ctx, wiStorage, wiType, newWiType, spaceID); err != nil { + return nil, errs.Wrapf(err, "unable to change workitem type from %s (ID: %s) to %s (ID: %s)", wiType.Name, wiType.ID, newWiType.Name, newWiType.ID) + } + // This will be used by the ConvertWorkItemStorageToModel function + wiType = newWiType + } tx := r.db.Where("Version = ?", updatedWorkItem.Version).Save(&wiStorage) if err := tx.Error; err != nil { log.Error(ctx, map[string]interface{}{ @@ -631,42 +650,58 @@ func (r *GormWorkItemRepository) Save(ctx context.Context, spaceID uuid.UUID, up return ConvertWorkItemStorageToModel(wiType, wiStorage) } -// Create creates a new work item in the repository -// returns BadParameterError, ConversionError or InternalError -func (r *GormWorkItemRepository) Create(ctx context.Context, spaceID uuid.UUID, typeID uuid.UUID, fields map[string]interface{}, creatorID uuid.UUID) (*WorkItem, error) { - defer goa.MeasureSince([]string{"goa", "db", "workitem", "create"}, time.Now()) - - wiType, err := r.witr.Load(ctx, typeID) - if err != nil { - return nil, errors.NewBadParameterError("typeID", typeID) - } - +// CheckTypeAndSpaceShareTemplate returns true if the given workitem type (wit) +// belongs to the same space template as the space (spaceID); otherwise false is +// returned +func (r *GormWorkItemRepository) CheckTypeAndSpaceShareTemplate(ctx context.Context, wit *WorkItemType, spaceID uuid.UUID) (bool, error) { // Prohibit creation of work items from a base type. - if !wiType.CanConstruct { - return nil, errors.NewForbiddenError(fmt.Sprintf("cannot construct work items from \"%s\" (%s)", wiType.Name, wiType.ID)) + if !wit.CanConstruct { + return false, errors.NewForbiddenError(fmt.Sprintf("cannot construct work items from \"%s\" (%s)", wit.Name, wit.ID)) } var exists bool // Prohibit creation of work items from a type that doesn't belong to current space template query := fmt.Sprintf(` - SELECT EXISTS ( - SELECT 1 from %[1]s WHERE id=$1 AND space_template_id = ( - SELECT space_template_id FROM %[2]s WHERE id=$2 - ) - )`, wiType.TableName(), space.Space{}.TableName()) - err = r.db.Raw(query, wiType.ID, spaceID).Row().Scan(&exists) + SELECT EXISTS ( + SELECT 1 from %[1]s WHERE id=$1 AND space_template_id = ( + SELECT space_template_id FROM %[2]s WHERE id=$2 + ) + )`, wit.TableName(), space.Space{}.TableName()) + err := r.db.Raw(query, wit.ID, spaceID).Row().Scan(&exists) if err == nil && !exists { - return nil, errors.NewBadParameterErrorFromString( - fmt.Sprintf("Workitem Type \"%s\" (ID: %s) does not belong to the current space template", wiType.Name, wiType.ID), + return false, errors.NewBadParameterErrorFromString( + fmt.Sprintf("Workitem Type \"%s\" (ID: %s) does not belong to the current space template", wit.Name, wit.ID), ) } if err != nil { log.Error(ctx, map[string]interface{}{ "space_id": spaceID, - "workitem_type_id": wiType.ID, + "workitem_type_id": wit.ID, "err": err, }, "unable to fetch workitem types related to current space") - return nil, errors.NewInternalError(ctx, errs.Wrapf(err, "unable to verify if %s exists", wiType.ID)) + return false, errors.NewInternalError(ctx, errs.Wrapf(err, "unable to verify if %s exists", wit.ID)) + } + return true, nil +} + +// Create creates a new work item in the repository +// returns BadParameterError, ConversionError or InternalError +func (r *GormWorkItemRepository) Create(ctx context.Context, spaceID uuid.UUID, typeID uuid.UUID, fields map[string]interface{}, creatorID uuid.UUID) (*WorkItem, error) { + defer goa.MeasureSince([]string{"goa", "db", "workitem", "create"}, time.Now()) + + wiType, err := r.witr.Load(ctx, typeID) + if err != nil { + return nil, errors.NewBadParameterError("typeID", typeID) + } + + allowedWIT, err := r.CheckTypeAndSpaceShareTemplate(ctx, wiType, spaceID) + if err != nil { + return nil, err + } + if !allowedWIT { + return nil, err + } + // The order of workitems are spaced by a factor of 1000. pos, err := r.LoadHighestOrder(ctx, spaceID) if err != nil { @@ -1131,3 +1166,244 @@ func (r *GormWorkItemRepository) LoadByIteration(ctx context.Context, iterationI } return workitems, nil } + +// ChangeWorkItemType changes the workitem in wiStorage to newWIType. Returns +// error if the operation fails +func (r *GormWorkItemRepository) ChangeWorkItemType(ctx context.Context, wiStorage *WorkItemStorage, oldWIType *WorkItemType, newWIType *WorkItemType, spaceID uuid.UUID) error { + allowedWIT, err := r.CheckTypeAndSpaceShareTemplate(ctx, newWIType, spaceID) + if err != nil { + return errs.Wrap(err, "failed to check workitem type") + } + if !allowedWIT { + return errors.NewBadParameterError("typeID", oldWIType.ID) + } + var fieldDiff = Fields{} + // Loop through old workitem type + for oldFieldName := range oldWIType.Fields { + // Temporary workaround to not add metastates to the field diff. We need + // to have a special handling for fields that shouldn't be set by user + // (or affected by type change) MetaState is a system level detail and + // that shouldn't be affected by type change, even if it is affected, it + // shouldn't show up in the field diff. The purpose of + // fieldDiff is to get the list of fields that should be added to the + // description. Metastate shouldn't show up in the description + if oldFieldName == SystemMetaState { + continue + } + // The field exists in old type and new type + if newField, ok := newWIType.Fields[oldFieldName]; ok { + // Try to assign the old value to the new field + _, err := newField.Type.ConvertToModel(wiStorage.Fields[oldFieldName]) + if err != nil { + // if the new type is a list, stuff the old value in a list and + // try to assign it + if newField.Type.GetKind() == KindList { + var convertedValue interface{} + convertedValue, err = newField.Type.ConvertToModel([]interface{}{wiStorage.Fields[oldFieldName]}) + if err == nil { + wiStorage.Fields[oldFieldName] = convertedValue + } + } + // if the old type is a list but the new one isn't check that + // the list contains only one element and assign that + if oldWIType.Fields[oldFieldName].Type.GetKind() == KindList && newField.Type.GetKind() != KindList { + ifArr, ok := wiStorage.Fields[oldFieldName].([]interface{}) + if !ok { + return errs.Errorf("failed to convert field \"%s\" to interface array: %+v", oldFieldName, wiStorage.Fields[oldFieldName]) + } + if len(ifArr) == 1 { + var convertedValue interface{} + convertedValue, err = newField.Type.ConvertToModel(ifArr[0]) + if err == nil { + wiStorage.Fields[oldFieldName] = convertedValue + } + } + } + } + // Failed to assign the old value to the new field. Add the field to + // the diff and remove it from the old workitem. + if err != nil { + fieldDiff[oldFieldName] = wiStorage.Fields[oldFieldName] + delete(wiStorage.Fields, oldFieldName) + } + } else { // field doesn't exist in new type + if wiStorage.Fields[oldFieldName] != nil { + fieldDiff[oldFieldName] = wiStorage.Fields[oldFieldName] + delete(wiStorage.Fields, oldFieldName) + } + } + } + // We need fieldKeys to show field diff in a defined order. Golang maps + // aren't ordered by default. + var fieldKeys []string + for fieldName := range fieldDiff { + fieldKeys = append(fieldKeys, fieldName) + } + // Sort the field keys to prevent random order of fields + sort.Strings(fieldKeys) + // Append diff (fields along with their values) between the workitem types + // to the description + if len(fieldDiff) > 0 { + // If description doesn't exists, assign it empty value + if wiStorage.Fields[SystemDescription] == nil { + wiStorage.Fields[SystemDescription] = "" + } + originalDescription := rendering.NewMarkupContentFromValue(wiStorage.Fields[SystemDescription]) + // TemplateData holds the information to be added to the description + templateData := struct { + NewTypeName string + FieldNameValues map[string]string + OriginalDescription string + }{ + NewTypeName: newWIType.Name, + FieldNameValues: make(map[string]string), + OriginalDescription: originalDescription.Content, + } + + for _, fieldName := range fieldKeys { + fieldDef := oldWIType.Fields[fieldName] + oldKind := fieldDef.Type.GetKind() + oldValue := fieldDiff[fieldName] + + if oldKind == KindEnum { + enumType, ok := fieldDef.Type.(EnumType) + if !ok { + return errs.Errorf("failed to convert field \"%s\" to enum type: %+v", fieldName, fieldDef) + } + oldKind = enumType.BaseType.GetKind() + } + // handle all single value fields (including Enums) + if oldKind != KindList { + var val string + if oldKind.IsRelational() { + val, err = getValueOfRelationalKind(r.db, oldValue, oldKind) + if err != nil { + return errs.Wrapf(err, "failed to get relational value for field %s", fieldName) + } + } else { + val = fmt.Sprint(oldValue) + } + // Add field information to the description + templateData.FieldNameValues[oldWIType.Fields[fieldName].Label] = val + continue + } + + // Deal with multi value field (KindList) + listType, ok := fieldDef.Type.(ListType) + if !ok { + return errs.Errorf("failed to convert field \"%s\" to list type: %+v", fieldName, fieldDef) + } + oldKind = listType.ComponentType.GetKind() + valList, ok := fieldDiff[fieldName].([]interface{}) + if !ok { + return errs.Errorf("failed to convert list value of field \"%s\" to []interface{}: %+v", fieldName, fieldDiff[fieldName]) + } + + var tempList []string + for _, v := range valList { + val := fmt.Sprint(v) + if oldKind.IsRelational() { + val, err = getValueOfRelationalKind(r.db, v, oldKind) + if err != nil { + return errs.Wrapf(err, "failed to get relational value for field %s", fieldName) + } + } + tempList = append(tempList, val) + } + // Convert []string to comma seperated strings and add it to the + // description. + templateData.FieldNameValues[oldWIType.Fields[fieldName].Label] = strings.Join(tempList, ", ") + } + descriptionTemplate := template.Must(template.New("test").Parse("```" + + ` +Missing fields in workitem type: {{ .NewTypeName }} +{{range $index, $element := .FieldNameValues }} +{{$index}} : {{$element}}{{end}} +` + "```" + ` +{{.OriginalDescription}} +`)) + var newDescription bytes.Buffer + if err := descriptionTemplate.Execute(&newDescription, templateData); err != nil { + return errs.Wrap(err, "failed to populate description template") + } + wiStorage.Fields[SystemDescription] = rendering.NewMarkupContent(newDescription.String(), rendering.SystemMarkupMarkdown) + } + // Set default values for all field in newWIType + for fieldName, fieldDef := range newWIType.Fields { + fieldValue := wiStorage.Fields[fieldName] + // Do not assign default value to metastate + if fieldName == SystemMetaState { + continue + } + // Assign default only if fieldValue is nil + wiStorage.Fields[fieldName], err = fieldDef.ConvertToModel(fieldName, fieldValue) + if err != nil { + return errs.Wrapf(err, "failed to convert field \"%s\"", fieldName) + } + } + wiStorage.Type = newWIType.ID + return nil +} + +// getValueOfRelationKind resolves the relational value stored in val to it's +// verbose value. Eg: UUID of kind User to username. +func getValueOfRelationalKind(db *gorm.DB, val interface{}, kind Kind) (string, error) { + var result string + switch kind { + case KindList, KindEnum: + return result, errors.NewInternalErrorFromString("cannot resolve relational value for KindList or KindEnum") + case KindUser: + var identity account.Identity + tx := db.Model(&account.Identity{}).Where("id = ?", val).Find(&identity) + if tx.Error != nil { + return result, errs.Wrap(tx.Error, "failed to find identity") + } + var user account.User + tx = db.Model(&account.User{}).Where("id = ?", identity.UserID).Find(&user) + if tx.Error != nil { + return result, errs.Wrap(tx.Error, "failed to find user") + } + result = fmt.Sprintf("%s (%s)", user.FullName, identity.Username) + case KindArea: + var area area.Area + tx := db.Model(area.TableName()).Where("id = ?", val).First(&area) + if tx.Error != nil { + return result, errs.Wrap(tx.Error, "failed to find area") + } + result = fmt.Sprintf("%s (%s)", area.Name, area.Path) + case KindBoardColumn: + var column BoardColumn + tx := db.Model(column.TableName()).Where("id = ?", val).First(&column) + if tx.Error != nil { + return result, errs.Wrap(tx.Error, "failed to find boardcolumn") + } + result = column.Name + + case KindIteration: + var iteration iteration.Iteration + tx := db.Model(iteration.TableName()).Where("id = ?", val).First(&iteration) + if tx.Error != nil { + return result, errs.Wrap(tx.Error, "failed to find iteration") + } + result = fmt.Sprintf("%s (%s)", iteration.Name, iteration.Path) + + case KindCodebase: + var codebase codebase.Codebase + tx := db.Model(codebase.TableName()).Where("id = ?", val).First(&codebase) + if tx.Error != nil { + return result, errs.Wrap(tx.Error, "failed to find codebase") + } + result = codebase.URL // TODO(ibrahim): Figure out what we should be here. Codebase does not have a name. + + case KindLabel: + var label label.Label + tx := db.Model(label.TableName()).Where("id = ?", val).First(&label) + if tx.Error != nil { + return result, errs.Wrap(tx.Error, "failed to find area") + } + result = label.Name + default: + return result, errors.NewInternalErrorFromString("unknown field Kind") + } + return result, nil +} diff --git a/workitem/workitem_repository_blackbox_test.go b/workitem/workitem_repository_blackbox_test.go index cc79f171b4..37235571db 100644 --- a/workitem/workitem_repository_blackbox_test.go +++ b/workitem/workitem_repository_blackbox_test.go @@ -3,6 +3,7 @@ package workitem_test import ( "context" "fmt" + "strings" "sync" "testing" "time" @@ -108,16 +109,231 @@ func (s *workItemRepoBlackBoxTest) TestSave() { }) s.T().Run("change is not prohibited", func(t *testing.T) { - // tests that you can change the type of a work item. NOTE: This - // functionality only works on the DB layer and is not exposed to REST. // given fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1), tf.WorkItemTypes(2)) // when fxt.WorkItems[0].Type = fxt.WorkItemTypes[1].ID newWi, err := s.repo.Save(s.Ctx, fxt.WorkItems[0].SpaceID, *fxt.WorkItems[0], fxt.Identities[0].ID) // then - require.NoError(s.T(), err) - assert.Equal(s.T(), fxt.WorkItemTypes[1].ID, newWi.Type) + require.NoError(t, err) + assert.Equal(t, fxt.WorkItemTypes[1].ID, newWi.Type) + }) + + s.T().Run("change of type along with field is not prohibited", func(t *testing.T) { + // tests that you can change the type of a work item and its fields at the same time. + // NOTE: This functionality only works on the DB layer and is not exposed to REST. + // given + fxt := tf.NewTestFixture(t, s.DB, tf.WorkItems(1, func(fxt *tf.TestFixture, idx int) error { + fxt.WorkItems[idx].Fields[workitem.SystemTitle] = "foo" + return nil + }), tf.WorkItemTypes(2)) + // when + fxt.WorkItems[0].Fields[workitem.SystemTitle] = "bar" + fxt.WorkItems[0].Type = fxt.WorkItemTypes[1].ID + newWi, err := s.repo.Save(s.Ctx, fxt.WorkItems[0].SpaceID, *fxt.WorkItems[0], fxt.Identities[0].ID) + // then + require.NoError(t, err) + assert.Equal(t, fxt.WorkItemTypes[1].ID, newWi.Type) + assert.Equal(t, "bar", newWi.Fields[workitem.SystemTitle]) + }) + + s.T().Run("type change", func(t *testing.T) { + + type testData struct { + name string + + initialValue interface{} + targetValue interface{} + + initialFieldType workitem.FieldType + targetFieldType workitem.FieldType + + fieldConvertible bool + } + + k := workitem.KindString + + td := []testData{ + // valid conversions + {"ok - simple type to simple type", + "foo1", + "foo1", + workitem.SimpleType{Kind: k}, + workitem.SimpleType{Kind: k}, + true}, + {"ok - simple type to list", + "foo2", + []interface{}{"foo2"}, + workitem.SimpleType{Kind: k}, + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: k}}, + true}, + {"ok - simple type to enum", + "foo3", + "foo3", + workitem.SimpleType{Kind: k}, + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: k}, Values: []interface{}{"red", "foo3", "blue"}}, + true}, + {"ok - list to list", + []interface{}{"foo4", "foo5"}, + []interface{}{"foo4", "foo5"}, + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: k}}, + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: k}}, + true}, + {"ok - list to simple type", + []interface{}{"foo6"}, + "foo6", + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: k}}, + workitem.SimpleType{Kind: k}, + true}, + {"ok - list to enum", + []interface{}{"foo7"}, + "foo7", + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: k}}, + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: k}, Values: []interface{}{"yellow", "foo7", "cyan"}}, + true}, + {"ok - enum to enum", + "foo8", + "foo8", + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: k}, Values: []interface{}{"Bach", "foo8", "Chapdelaine"}}, + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: k}, Values: []interface{}{"Kant", "Hume", "foo8", "Aristoteles"}}, + true}, + {"ok - enum to simple type", + "foo9", + "foo9", + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: k}, Values: []interface{}{"Schopenhauer", "foo9", "Duerer"}}, + workitem.SimpleType{Kind: k}, + true}, + {"ok - enum to list", + "foo10", + []interface{}{"foo10"}, + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: k}, Values: []interface{}{"Sokrates", "foo10", "Fromm"}}, + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: k}}, + true}, + // invalid conversions + {"err - simple type (string) to simple type (int)", + "foo11", + nil, + workitem.SimpleType{Kind: workitem.KindString}, + workitem.SimpleType{Kind: workitem.KindInteger}, + false}, + {"err - simple type (string) to list (integer)", + "foo2", + ([]interface{})(nil), + workitem.SimpleType{Kind: k}, + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: workitem.KindInteger}}, + false}, + {"err - simple type (string) to enum (float)", + "foo3", + 11.1, + workitem.SimpleType{Kind: k}, + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: workitem.KindFloat}, Values: []interface{}{11.1, 22.2, 33.3}}, + false}, + {"err - list (string) to list (float)", + []interface{}{"foo4", "foo5"}, + ([]interface{})(nil), + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: k}}, + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: workitem.KindFloat}}, + false}, + {"err - list (string) to simple type (int)", + []interface{}{"foo6"}, + nil, + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: k}}, + workitem.SimpleType{Kind: workitem.KindInteger}, + false}, + {"err - list (string) to enum (float)", + []interface{}{"foo7"}, + 11.1, + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: k}}, + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: workitem.KindFloat}, Values: []interface{}{11.1, 22.2, 33.3}}, + false}, + {"err - enum (string) to enum (float)", + "foo8", + 11.1, + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: k}, Values: []interface{}{"Bach", "foo8", "Chapdelaine"}}, + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: workitem.KindFloat}, Values: []interface{}{11.1, 22.2, 33.3}}, + false}, + {"err - enum (string) to simple type (float)", + "foo9", + nil, + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: k}, Values: []interface{}{"Schopenhauer", "foo9", "Duerer"}}, + workitem.SimpleType{Kind: workitem.KindFloat}, + false}, + {"err - enum (string) to list (float)", + "foo10", + ([]interface{})(nil), + workitem.EnumType{SimpleType: workitem.SimpleType{Kind: workitem.KindEnum}, BaseType: workitem.SimpleType{Kind: k}, Values: []interface{}{"Sokrates", "foo10", "Fromm"}}, + workitem.ListType{SimpleType: workitem.SimpleType{Kind: workitem.KindList}, ComponentType: workitem.SimpleType{Kind: workitem.KindFloat}}, + false}, + } + for _, d := range td { + t.Run(d.name, func(t *testing.T) { + fieldName := d.name + + fxt := tf.NewTestFixture(t, s.DB, + tf.WorkItemTypes(2, func(fxt *tf.TestFixture, idx int) error { + wit := fxt.WorkItemTypes[idx] + switch idx { + case 0: + wit.Fields = workitem.FieldDefinitions{ + fieldName: workitem.FieldDefinition{ + Label: "source field", + Required: false, + Type: d.initialFieldType, + }, + } + case 1: + wit.Fields = workitem.FieldDefinitions{ + fieldName: workitem.FieldDefinition{ + Label: "target field", + Required: false, + Type: d.targetFieldType, + }, + } + } + return nil + }), + tf.WorkItems(1, tf.SetWorkItemField(fieldName, d.initialValue)), + ) + + // Load the work item from the DB and check that the + // initial value was set correctly. We have some special + // treatment for lists here. + loadedWorkItem, err := s.repo.LoadByID(s.Ctx, fxt.WorkItems[0].ID) + require.NoError(t, err) + require.Equal(t, d.initialValue, loadedWorkItem.Fields[fieldName]) + + // when we update the work item type + loadedWorkItem.Type = fxt.WorkItemTypes[1].ID + updatedWorkItem, err := s.repo.Save(s.Ctx, fxt.WorkItems[0].SpaceID, *loadedWorkItem, fxt.Identities[0].ID) + require.NoError(t, err) + + // then check that the error is as expected or that the + // value in the new field type is what we expected. + if !d.fieldConvertible { + rendered := d.initialValue + if d.initialFieldType.GetKind() == workitem.KindList { + ifArr := d.initialValue.(interface{}).([]interface{}) + strArr := make([]string, len(ifArr)) + for i := range ifArr { + strArr[i] = ifArr[i].(string) + } + rendered = strings.Join(strArr, ", ") + } + require.Contains(t, updatedWorkItem.Fields[workitem.SystemDescription].(rendering.MarkupContent).Content, fmt.Sprintf("source field : %+v", rendered)) + } else { + require.NotNil(t, updatedWorkItem) + require.Equal(t, fxt.WorkItemTypes[1].ID, updatedWorkItem.Type) + require.Equal(t, d.targetValue, updatedWorkItem.Fields[fieldName]) + } + + // also check if the values are the same when work item is + // loaded + loadedWorkItem, err = s.repo.LoadByID(s.Ctx, fxt.WorkItems[0].ID) + require.NoError(t, err) + require.Equal(t, fxt.WorkItemTypes[1].ID, loadedWorkItem.Type) + require.Equal(t, d.targetValue, loadedWorkItem.Fields[fieldName]) + }) + } }) } diff --git a/workitem/workitemtype.go b/workitem/workitemtype.go index 491d315da0..3635b51669 100644 --- a/workitem/workitemtype.go +++ b/workitem/workitemtype.go @@ -36,6 +36,7 @@ const ( SystemCodebase = "system.codebase" SystemLabels = "system.labels" SystemBoardcolumns = "system.boardcolumns" + SystemMetaState = "system.metastate" SystemBoard = "Board"