mrc-2755 Move error reporting related mutations out of root mutations #611

Nicolas-Dolan · 2021-11-12T11:05:21Z

Description

Moves error reporting-related state. mutations, and actions from the root module to the errors module so they can be shared with the data exploration mode. To test, check app loads up correctly and no issues with error reporting form.

Type of version change

Patches

Checklist

I have incremented version number, or version needs no increment
The build passed successfully, or failed because of ADR tests

codecov · 2021-11-12T17:06:34Z

Codecov Report

Merging #611 (1a399b4) into master (31d1c4a) will increase coverage by 0%.
The diff coverage is 100%.

@@           Coverage Diff           @@
##             master   #611   +/-   ##
=======================================
  Coverage        98%    98%           
  Complexity      281    281           
=======================================
  Files           202    202           
  Lines          5862   5863    +1     
  Branches        812    812           
=======================================
+ Hits           5766   5767    +1     
  Misses           61     61           
  Partials         35     35

Impacted Files	Coverage Δ
src/app/static/src/app/root.ts	`100% <ø> (ø)`
...c/src/app/store/dataExploration/dataExploration.ts	`100% <ø> (ø)`
src/app/static/src/app/store/errors/errors.ts	`100% <ø> (ø)`
src/app/static/src/app/components/ErrorReport.vue	`100% <100%> (ø)`
src/app/static/src/app/hintVersion.ts	`100% <100%> (ø)`
src/app/static/src/app/store/errors/mutations.ts	`100% <100%> (ø)`
src/app/static/src/app/store/root/actions.ts	`100% <100%> (ø)`
src/app/static/src/app/store/root/mutations.ts	`100% <100%> (ø)`

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 31d1c4a...1a399b4. Read the comment docs.

EmmaLRussell · 2021-11-15T10:30:13Z

src/app/static/src/tests/integration/errors.itest.ts

-import {initialHintrVersionState} from "../../app/store/hintrVersion/hintrVersion";
+import {actions} from "../../app/store/errors/actions";
+import {ErrorReportManualDetails} from "../../app/store/errors/actions";
+import {emptyState} from "../../app/root";

 describe(`root actions`, () => {


This should be error actions now

LekanAnni

LGTM

Just a minor improvement

needs an update with master

LekanAnni · 2021-11-15T10:36:35Z

src/app/static/src/app/store/errors/mutations.ts

@@ -16,5 +18,15 @@ export const mutations: MutationTree<ErrorsState> = {

    [ErrorsMutation.DismissAll](state: ErrorsState, action: PayloadWithType<null>) {


Suggested change

[ErrorsMutation.DismissAll](state: ErrorsState, action: PayloadWithType<null>) {

[ErrorsMutation.DismissAll](state: ErrorsState) {

EmmaLRussell

I'm getting this error when I submit an error report (from a project with no errors in state, at step 2):

{
  "data": {},
  "status": "failure",
  "errors": [
    {
      "detail": "An unexpected error occurred. If you see this message while you are using Naomi at a workshop, please contact your workshop technical support and show them this code: uro-ers-iqg. Otherwise please contact support at naomi-support@imperial.ac.uk and quote this code: uro-ers-iqg",
      "error": "OTHER_ERROR",
      "trace": [
        "JSON parse error: Instantiation of [simple type, class org.imperial.mrc.hint.models.ErrorReport] value failed for JSON property errors due to missing (therefore NULL) value for creator parameter errors which is a non-nullable type; nested exception is com.fasterxml.jackson.module.kotlin.MissingKotlinParameterException: Instantiation of [simple type, class org.imperial.mrc.hint.models.ErrorReport] value failed for JSON property errors due to missing (therefore NULL) value for creator parameter errors which is a non-nullable type\n at [Source: (PushbackInputStream); line: 1, column: 430] (through reference chain: org.imperial.mrc.hint.models.ErrorReport[\"errors\"])"
      ]
    }
  ]
}

Looking at the POSTed data, the `errors' field is indeed missing.

I think this is because the action is using getters.errors to generate the errors field - but this getter has not been moved out of root. So it either needs to use the getter in root, or move it into errors - I think it would probably make for it to stay in root. You should be able to use rootGetters in the context. Should be a test for this too.

hillalex · 2021-11-15T12:39:42Z

src/app/static/src/app/store/errors/actions.ts

+        const data = {
+            email: payload.email || rootState.currentUser,
+            country: rootState.baseline.country || "no associated country",
+            projectName: rootState.projects.currentProject?.name || "no associated project",


Ah, I'm afraid this needs to remain in root actions, because it won't be re-usable - it references the projects module, which won't exist in the new app.

Right, so we'll need a separate action to generate the report for data exploration mode. I guess the alternative would be some ugly casting to any to check if state is actually a root state or not (has projects). But if we're going to have a separate dialog component for DE error reporting anyway, cleaner to have that as separate action.

hillalex

I think it should only be the mutations and the ErrorReport interface moved out. The actions and getter should remain in root.

hillalex

Looks great, thanks

LekanAnni · 2021-11-16T14:32:03Z

@Nicolas-Dolan It would be nice if this ticket can be merged after update version.

Nicolas-Dolan · 2021-11-16T16:40:30Z

@Nicolas-Dolan It would be nice if this ticket can be merged after update version.

@LekanAnni up to date now

Nicolas-Dolan added 8 commits November 11, 2021 16:58

moved error reports from root to errors module

df277a7

adjusted tests

b2ba65f

fixed unit tests

a4650df

Merge branch 'master' into mrc-2755

8c92821

incremented version number

01b0b96

fixed localstoragemanager test

3401c66

updated integration test

aa4f5b0

fixed integration test

3959bd1

Nicolas-Dolan marked this pull request as ready for review November 12, 2021 17:07

Nicolas-Dolan requested review from LekanAnni, hillalex, mwoodbri and EmmaLRussell and removed request for mwoodbri November 12, 2021 17:07

EmmaLRussell reviewed Nov 15, 2021

View reviewed changes

LekanAnni suggested changes Nov 15, 2021

View reviewed changes

EmmaLRussell requested changes Nov 15, 2021

View reviewed changes

updated errors.itest and merged latest master

a9e628f

hillalex reviewed Nov 15, 2021

View reviewed changes

hillalex requested changes Nov 15, 2021

View reviewed changes

Nicolas-Dolan added 6 commits November 15, 2021 13:07

fixed errors action

4113297

moved error alert actions to root but interfaces to types

3636a1a

merged master incl data exploration app

8551687

fixed tests

0d40889

tweaks

d59c6b3

fixed error

30ebdd6

Nicolas-Dolan requested review from hillalex, LekanAnni and EmmaLRussell November 16, 2021 09:57

hillalex approved these changes Nov 16, 2021

View reviewed changes

EmmaLRussell approved these changes Nov 16, 2021

View reviewed changes

merged latest master

1a399b4

LekanAnni approved these changes Nov 16, 2021

View reviewed changes

Nicolas-Dolan merged commit 581a615 into master Nov 17, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mrc-2755 Move error reporting related mutations out of root mutations #611

mrc-2755 Move error reporting related mutations out of root mutations #611

Nicolas-Dolan commented Nov 12, 2021 •

edited

codecov bot commented Nov 12, 2021 •

edited

EmmaLRussell Nov 15, 2021

LekanAnni left a comment •

edited

LekanAnni Nov 15, 2021

EmmaLRussell left a comment

hillalex Nov 15, 2021

EmmaLRussell Nov 16, 2021

hillalex left a comment

hillalex left a comment

LekanAnni commented Nov 16, 2021

Nicolas-Dolan commented Nov 16, 2021

		@@ -16,5 +18,15 @@ export const mutations: MutationTree<ErrorsState> = {

		[ErrorsMutation.DismissAll](state: ErrorsState, action: PayloadWithType<null>) {

mrc-2755 Move error reporting related mutations out of root mutations #611

mrc-2755 Move error reporting related mutations out of root mutations #611

Conversation

Nicolas-Dolan commented Nov 12, 2021 • edited

Description

Type of version change

Checklist

codecov bot commented Nov 12, 2021 • edited

Codecov Report

EmmaLRussell Nov 15, 2021

Choose a reason for hiding this comment

LekanAnni left a comment • edited

Choose a reason for hiding this comment

LekanAnni Nov 15, 2021

Choose a reason for hiding this comment

EmmaLRussell left a comment

Choose a reason for hiding this comment

hillalex Nov 15, 2021

Choose a reason for hiding this comment

EmmaLRussell Nov 16, 2021

Choose a reason for hiding this comment

hillalex left a comment

Choose a reason for hiding this comment

hillalex left a comment

Choose a reason for hiding this comment

LekanAnni commented Nov 16, 2021

Nicolas-Dolan commented Nov 16, 2021

Nicolas-Dolan commented Nov 12, 2021 •

edited

codecov bot commented Nov 12, 2021 •

edited

LekanAnni left a comment •

edited