mrc-443 Added uploadANC front end #38

EmmaLRussell · 2019-09-03T10:27:35Z

No description provided.

EmmaLRussell · 2019-09-03T10:28:26Z

src/app/static/src/app/main.ts

@@ -14,7 +14,7 @@ export interface RootState {
 }

 export interface StepState {
-    complete: boolean
+    complete: () => boolean


I wondered if this would work better as a function, since it's likely to be a value derived from other properties in the state.

Will this work? I don't think the function will be automatically executed when the underlying data changes. This could/should be tested by instantiating the component with incomplete data, then triggering the action/mutation which makes the stage complete, then checking whether the continue button is enabled.

It looks like vue does have a concept of "getters" aka computed properties for the store which I think are what you're after here: https://vuex.vuejs.org/guide/getters.html

codecov · 2019-09-03T10:31:44Z

Codecov Report

Merging #38 into master will increase coverage by 0.24%.
The diff coverage is 100%.

@@             Coverage Diff             @@
##             master     #38      +/-   ##
===========================================
+ Coverage     96.46%   96.7%   +0.24%     
  Complexity       73      73              
===========================================
  Files            51      51              
  Lines           565     607      +42     
  Branches         55      62       +7     
===========================================
+ Hits            545     587      +42     
  Misses           15      15              
  Partials          5       5

Impacted Files	Coverage Δ	Complexity Δ
src/app/static/src/app/store/baseline/mutations.ts	`100% <ø> (ø)`	`0 <0> (ø)`	⬇️
src/app/static/src/app/main.ts	`100% <ø> (ø)`	`0 <0> (ø)`	⬇️
...in/kotlin/org/imperial/mrc/hint/models/Response.kt	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...static/src/app/store/surveyAndProgram/mutations.ts	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
src/app/static/src/app/apiService.ts	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
src/app/static/src/app/store/baseline/baseline.ts	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
src/app/static/src/app/components/Stepper.vue	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...c/tests/components/surveyAndProgram/fileUploads.ts	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...p/static/src/app/store/surveyAndProgram/actions.ts	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
...src/app/store/surveyAndProgram/surveyAndProgram.ts	`100% <100%> (ø)`	`0 <0> (ø)`	⬇️
... and 1 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 91f449e...e5b42b0. Read the comment docs.

hillalex · 2019-09-03T11:59:16Z

src/app/static/src/tests/components/surveyAndProgram/fileUploads.ts

@@ -49,8 +53,10 @@ export function testUploadComponent(name: string, position: number) {

        const successState = name == "survey" ? {
            survey: response
-        } : {
+        } : name =="program" ? {


I guess a switch statement would be more readable here now! And above

hillalex · 2019-09-03T12:05:40Z

src/app/static/src/app/main.ts

@@ -14,7 +14,7 @@ export interface RootState {
 }

 export interface StepState {
-    complete: boolean
+    complete: () => boolean


Will this work? I don't think the function will be automatically executed when the underlying data changes. This could/should be tested by instantiating the component with incomplete data, then triggering the action/mutation which makes the stage complete, then checking whether the continue button is enabled.

It looks like vue does have a concept of "getters" aka computed properties for the store which I think are what you're after here: https://vuex.vuejs.org/guide/getters.html

…ility

EmmaLRussell · 2019-09-03T17:28:01Z

I've tried using getters, which turned out to be slightly painful, primarily because I couldn't get mapGetters to work (compile kept failing as it wouldn't recognise the mapped getters as computed properties). Updated Babel to no avail. Possible that this is a bug and related to the fact that we're using modules. There's an open PR which may be relevant. vuejs/vuex#1121
Would require declaring further interfaces. Anyway, I worked around it, but not sure how much it adds really!

EmmaLRussell · 2019-09-03T17:30:30Z

src/app/static/src/tests/baseline/mutations.test.ts

        mutations.PJNZUploaded(testState, {payload: mockPJNZResponse({data: {country: "Malawi"}}), type: "PJNZLoaded"});
-        expect(testState.complete()).toBe(true);
+
+        expect(complete(testState, null, testRootState, null)).toBe(true);


Getters really needs four parameters, but ours only use the first one!

…onnection rather than openStream

EmmaLRussell · 2019-09-04T09:21:53Z

src/app/src/test/kotlin/org/imperial/mrc/hint/helpers/JSONValidator.kt

-        return url.openStream().use {
+
+        val conn = url.openConnection() as HttpURLConnection
+        return BufferedReader(InputStreamReader(conn.getInputStream())).use {


Holy hell, this was annoying. This randomly started throwing errors when running locally, when attempting to use the blameless ValidateInputResponse schema (it was claiming the schema was invalid). Eventually worked out it appeared to always fail at the same character index, so presumably it wasn't managing to read the whole file over a certain length). Changing from url.openStream to url.openConnection seemed to fix this...

EmmaLRussell · 2019-09-04T09:22:51Z

I regenerated the types from HINTR, as latest master is now returning different response to before (errors as array rather than object).

hillalex

Wow, what a faff, sorry! If it's any consolation I really like the use of getters here, I think it does make things easier to reason about

hillalex · 2019-09-04T12:07:43Z

src/app/static/src/tests/components/stepper.test.ts

@@ -111,4 +114,23 @@ describe("Stepper component", () => {
        expect(steps.at(1).props().active).toBe(true);
    });

+    it("updates from completed state when active step data is populated", (done) => {


Added uploadANC front end

f300b87

EmmaLRussell commented Sep 3, 2019

View reviewed changes

EmmaLRussell requested a review from hillalex September 3, 2019 10:35

hillalex suggested changes Sep 3, 2019

View reviewed changes

Use getters for local state complete and switch statements for readab…

6b615a7

…ility

EmmaLRussell commented Sep 3, 2019

View reviewed changes

EmmaLRussell added 3 commits September 3, 2019 18:43

Updated expected error message

b66d27b

Regenerated types from HINTR and change to JSONValidator to use openC…

03265a5

…onnection rather than openStream

Removed unused import

c453dd9

EmmaLRussell commented Sep 4, 2019

View reviewed changes

Mocks need to use latest Response type

e5b42b0

EmmaLRussell requested a review from hillalex September 4, 2019 10:05

hillalex approved these changes Sep 4, 2019

View reviewed changes

hillalex merged commit d4ec0fd into master Sep 4, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

mrc-443 Added uploadANC front end #38

mrc-443 Added uploadANC front end #38

EmmaLRussell commented Sep 3, 2019

EmmaLRussell Sep 3, 2019

hillalex Sep 3, 2019

codecov bot commented Sep 3, 2019 •

edited

Loading

hillalex Sep 3, 2019

hillalex Sep 3, 2019

EmmaLRussell commented Sep 3, 2019

EmmaLRussell Sep 3, 2019

EmmaLRussell Sep 4, 2019 •

edited

Loading

EmmaLRussell commented Sep 4, 2019

hillalex left a comment

hillalex Sep 4, 2019

mrc-443 Added uploadANC front end #38

mrc-443 Added uploadANC front end #38

Conversation

EmmaLRussell commented Sep 3, 2019

EmmaLRussell Sep 3, 2019

Choose a reason for hiding this comment

hillalex Sep 3, 2019

Choose a reason for hiding this comment

codecov bot commented Sep 3, 2019 • edited Loading

Codecov Report

hillalex Sep 3, 2019

Choose a reason for hiding this comment

hillalex Sep 3, 2019

Choose a reason for hiding this comment

EmmaLRussell commented Sep 3, 2019

EmmaLRussell Sep 3, 2019

Choose a reason for hiding this comment

EmmaLRussell Sep 4, 2019 • edited Loading

Choose a reason for hiding this comment

EmmaLRussell commented Sep 4, 2019

hillalex left a comment

Choose a reason for hiding this comment

hillalex Sep 4, 2019

Choose a reason for hiding this comment

codecov bot commented Sep 3, 2019 •

edited

Loading

EmmaLRussell Sep 4, 2019 •

edited

Loading