Skip to content
This repository has been archived by the owner on Mar 11, 2021. It is now read-only.

WIP: update event system #2189

Closed
wants to merge 13 commits into from
Closed

Conversation

kwk
Copy link
Collaborator

@kwk kwk commented Jul 27, 2018

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.

@kwk kwk self-assigned this Jul 27, 2018
@alien-ike alien-ike changed the title WIP: update events system update events system Jul 27, 2018
@alien-ike alien-ike changed the title WIP: update events system update events system Jul 27, 2018
@codecov-io
Copy link

codecov-io commented Jul 27, 2018

Codecov Report

Merging #2189 into master will decrease coverage by 0.45%.
The diff coverage is 72.2%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2189      +/-   ##
==========================================
- Coverage   69.84%   69.39%   -0.46%     
==========================================
  Files         170      170              
  Lines       15826    15778      -48     
==========================================
- Hits        11054    10949     -105     
- Misses       3760     3802      +42     
- Partials     1012     1027      +15
Impacted Files Coverage Δ
workitem/field_test_data.go 94.28% <ø> (ø) ⬆️
workitem/event/event.go 100% <ø> (ø) ⬆️
controller/codebase.go 45.8% <0%> (-1.13%) ⬇️
controller/label.go 86.25% <100%> (-0.11%) ⬇️
workitem/field_definition.go 80.7% <100%> (+0.88%) ⬆️
controller/work_item_board.go 100% <100%> (ø) ⬆️
controller/area.go 92.3% <100%> (ø) ⬆️
controller/iteration.go 78.27% <100%> (ø) ⬆️
controller/users.go 79.16% <100%> (ø) ⬆️
workitem/enum_type.go 88.23% <100%> (ø) ⬆️
... and 11 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6687a95...30ad4c8. Read the comment docs.

@kwk
Copy link
Collaborator Author

kwk commented Jul 27, 2018

[test]

@kwk kwk changed the title update events system update event system Jul 27, 2018
@kwk kwk changed the title update event system WIP: update event system Jul 27, 2018
@kwk kwk force-pushed the update-event-system branch from e548d0a to 7849d0e Compare July 30, 2018 15:22
@kwk kwk requested a review from jarifibrahim July 31, 2018 13:31
"relationships": {
"modifier": {
"data": {
"id": "00000000-0000-0000-0000-000000000002",
"links": {
"related": "http:///api/users/00000000-0000-0000-0000-000000000002"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please note that JSONAPI doesn't allow both to be specified: type+id and links. Instead only one is allowed.

"oldValue": null,
"timestamp": "0001-01-01T00:00:00Z"
"timestamp": "0001-01-01T00:00:00Z",
"workItemTypeID": "00000000-0000-0000-0000-000000000001"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note, that an event needs to know about the work item type at the given point int type. This is important so that the UI can find out how to resolve the name (e.g. system.description) into a field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I should make this a relationships really. Let me do that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in 6020b73

@kwk
Copy link
Collaborator Author

kwk commented Aug 3, 2018

@kwk kwk closed this Aug 3, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants