#2 push method #8

josh-griffin · 2019-06-13T00:48:39Z

Fixes #2

Main addition:

Adds the surveyResponse method which will push data to the Tupaia server
Adds generic tests for all requests, based on status codes
Adds generic error lookup function for requests to use

Minor additions:

Adds a couple more error objects
Slightly altered the api config

Chris-Petty

Not sure if there is a such thing as too many comments... naaaahhh

I didn't review the jest tests, is that bad?

src/errors/errorLookup.js

src/utilities/requestUtilities.js

Co-Authored-By: Chris Petty <[email protected]>

…a-service into #2-push-method

josh-griffin · 2019-06-13T22:10:59Z

I wouldn't mind more thoughts on the comments if you had time - did they help you, with little understanding of much else, understand the code easily? Or were they just annoying? (I like comments for functions which are obscure/dependent on strange inputs), still trying to find a balance!

Also it's fine with the tests - I was going to add more but didn't want to make the PR too big, prob make an issue for more - i.e. with malformed input etc, rather than just status codes

josh-griffin · 2019-06-14T04:25:03Z

Just a note in case I forget. The API has changed and now returns 400 status codes when an object is invalid. Requires changes to this method that should be done in this PR. I think setting valid status codes (responses which will not throw an error) as 200 and 400 should be fine, but it has me a little worried

Chris-Petty · 2019-06-14T08:03:49Z

Just a note in case I forget. The API has changed and now returns 400 status codes when an object is invalid. Requires changes to this method that should be done in this PR. I think setting valid status codes (responses which will not throw an error) as 200 and 400 should be fine, but it has me a little worried

I'm not 100% up to speed, but maybe 422 would be accurate? This is largely internal right? So long as it's well documented it's probably not a big bother if it's not the absolute best option (if figuring that out takes excessive amounts of time)

https://www.bennadel.com/blog/2434-http-status-codes-for-invalid-data-400-vs-422.htm

src/utilities/requestUtilities.js

Co-Authored-By: Chris Petty <[email protected]>

josh-griffin · 2019-06-16T21:00:17Z

I didn't actually know about 422, that would def be ideal. The 400 response is what is sent from the API, which I don't have any control over (they're probably sick of our suggestions already), so it will just send a 400.

josh-griffin · 2019-06-17T00:23:04Z

Added some tests for the utlity method and request method.. sorry :(

andreievg · 2019-06-17T21:35:16Z

src/utilities/requestUtilities.js

+    invalidObjects.push(invalidDatum);
+  });
+  return { data: originalDataClone, invalidData: invalidObjects };
+};


can you please provide possible erroneous response in a comment below

I guess i can look it up in testData/

I'm not 100% what you want here, you mean responses from the API that are errors? Maybe best for the wiki? I haven't added any to the test cases as that case essentially is testing the helper method (Also hard to mock default import of axios twice in one test). Hard to find a balance for number of tests written - I think trying to go for 100% coverage is a bit of a trap

just needed an example of erroneous response, but found/figured it out

andreievg · 2019-06-17T21:42:42Z

src/utilities/requestUtilities.js

+    // is an error message.
+    const { row } = errorObject;
+    // If the row value is valid, remove the index from the original data
+    if (!Number.isInteger(Number(row)) || row >= invalidData.length) return;


originalData.length ?

if so, maybe should add index out of bound to removeInvalidObjectsTestData test

49509f4 have added a check here

i mean instead of row >= invalidData.length, should be row >= originalData.length ?

oh .. well the other one was needed also! heh.

Added test cases: bdab5fe 7532704

and solution 7d6e3d4

andreievg · 2019-06-17T21:49:41Z

src/utilities/requestUtilities.js

+    const { row } = errorObject;
+    // If the row value is valid, remove the index from the original data
+    if (!Number.isInteger(Number(row)) || row >= invalidData.length) return;
+    const [invalidDatum] = originalDataClone.splice(row, 1);


try this method with removeInvalidObjectsTestData like this:

TEST_FIVE: { INPUT: { originalData: [1, 2, 3], invalidData: { errors: [{ row: 0 }, { row: 2 }] } }, OUTPUT: { data: [2], invalidData: [1,3] }, },

0ba216b I added a couple more test cases - one for the above comment and also this case (Although I'm not 100% sure what it was going for?)

does that test run fine ? i think it would fail, once you splice a row from originalDataClone, it becomes length of 2 ? then try to splice an index that doesn't exist (2)

ah yeah it does run fine - it first sorts the invalid data by row in descending order. So we would splice 2 first, then 0

andreievg · 2019-06-17T21:50:42Z

src/requests/surveyResponse.js

+    // which lists the indicies which are invalid with a status code of
+    // 200.
+    const { data: response } = await Axios(apiConfig);
+    const { errors } = response;


should we check for presence of "error" (just in case API response has unknown error with still 200 reponse)

8a4efa9 added this here

andreievg

@joshxg, looks good to me, merge at will

Chris-Petty · 2019-06-18T12:02:18Z

Lol this PR doubled since my last review. Most tested utility method eva

Chris-Petty · 2019-06-18T13:06:56Z

src/tests/utilities.test.js

+ * 7 : calling with an empty, but existing, errors array
+ * 8 : calling with an error index being out of range
+ */
+test('Should pass all tests', () => {


In terms of Jest output, isn't this just going to show:
❎ Should pass all tests
i.e. no context

Ages ago I went through the Jasmine docs as I think it has a nice way of presenting making a test suite (JEST basically took the same API): https://jasmine.github.io/tutorials/your_first_suite

Or more specifically just the docs for describe in Jest here: https://jestjs.io/docs/en/api#describename-fn

In that case it'd be

describe('removeInvalidObjects', () => { test('removes all data, as all have errors', () => { const result = removeInvalidObjects([1, 2, 3], { errors: [{ row: 0 }, { row: 1 }, { row: 2 }] }) expect(result).toEqual([3, 2, 1]) } test('removing one object, as only one has an error', () => { ... } ... }

That'd make the test output a bit more useful/typical I'd presume. Also not sure what affect your forEach might have on Jest's multithread support. Ew I just went in a not so relevant rabbit hole jestjs/jest#6694

Interesting rabbit hole! (I hope this doesn't get to 7,000 tests :( )

Regarding the tests though: You're right on the description, it will just show no real context. I have been wanting to look more into testing and making suites, I've just been doing it very un-researched currently.

As for the forEach, I didn't look into any implications, it just seemed like a nice easy way to run, add, modify tests. Definitely given me lots to look into though!

josh-griffin added 10 commits June 7, 2019 13:56

Add restructure test folder

f60c902

Add update to APIConfig for surveyresponse endpoint

27b83fb

Add invalid object utility method

2bc8638

Add error lookup functionality

4a0f952

Add update to surveyResponse config and test data

b14e41b

Add tidy up of api config test data

ff3b74f

Add additional error objects

74f8df9

Add requests index export

1b8b387

Add surveyResponse method

8876b60

Add generic request tests

98703e8

josh-griffin requested review from Chris-Petty, andreievg and wlthomson June 13, 2019 00:49

Chris-Petty requested changes Jun 13, 2019

View reviewed changes

josh-griffin and others added 7 commits June 14, 2019 08:49

Add typo fix

51215e7

Co-Authored-By: Chris Petty <[email protected]>

Update src/errors/errorLookup.js

215d3da

Co-Authored-By: Chris Petty <[email protected]>

Add fix for incorrect doc string

2f2d2a4

Co-Authored-By: Chris Petty <[email protected]>

Add fix confusing conditional

285fb10

Merge branch '#2-push-method' of https://github.com/openmsupply/tupai…

8b5e813

…a-service into #2-push-method

Add fix for confusing length destructuring

64807af

Add fix array mutation indexing

6b47261

Chris-Petty requested changes Jun 15, 2019

View reviewed changes

src/utilities/requestUtilities.js Outdated Show resolved Hide resolved

src/utilities/requestUtilities.js Outdated Show resolved Hide resolved

josh-griffin and others added 3 commits June 17, 2019 08:29

Add even clearer conditional

0ef5503

Co-Authored-By: Chris Petty <[email protected]>

Add fix for array mutation

a66e902

Add 400 http status to valid responses for surveyResponse endpoint

5dedee7

Add fix for incorrect iteration

f3203a0

josh-griffin added 7 commits June 17, 2019 11:13

Add test cases for invalid objects method

eff7db2

Add fix errors test cases highlighted

297ef53

Add named export over default in utilities

4f9ba70

Add correctly import removeInvalidObjects method

a3dd684

Add simple surveyResponse test

10cc7fe

Add tidy up doc strings

32bf924

Add maintenance error object

6f03ab2

andreievg reviewed Jun 17, 2019

View reviewed changes

josh-griffin added 8 commits June 18, 2019 11:26

Add additional removeInvalidObjects test cases

0ba216b

Add check for originalData in removeINvalidObjects function

49509f4

Add updated comment for surveyResponse

74ed058

Add update to test case comments

98cf0e8

Add handling of error object, but no error

8a4efa9

Add new test case for utility method

bdab5fe

Add proper check on row value

7d6e3d4

Add additional test case

7532704

andreievg approved these changes Jun 18, 2019

View reviewed changes

josh-griffin merged commit e68f1b3 into development Jun 18, 2019

josh-griffin deleted the #2-push-method branch June 18, 2019 02:23

Chris-Petty reviewed Jun 18, 2019

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#2 push method #8

#2 push method #8

josh-griffin commented Jun 13, 2019

Chris-Petty left a comment

josh-griffin commented Jun 13, 2019

josh-griffin commented Jun 14, 2019

Chris-Petty commented Jun 14, 2019 •

edited

Loading

josh-griffin commented Jun 16, 2019

josh-griffin commented Jun 17, 2019

andreievg Jun 17, 2019

andreievg Jun 17, 2019

josh-griffin Jun 17, 2019

andreievg Jun 17, 2019

andreievg Jun 17, 2019

andreievg Jun 17, 2019

josh-griffin Jun 17, 2019

andreievg Jun 17, 2019

josh-griffin Jun 17, 2019

andreievg Jun 17, 2019

josh-griffin Jun 17, 2019

andreievg Jun 17, 2019

josh-griffin Jun 17, 2019

andreievg Jun 17, 2019

andreievg Jun 17, 2019

josh-griffin Jun 17, 2019

andreievg left a comment

Chris-Petty commented Jun 18, 2019

Chris-Petty Jun 18, 2019

josh-griffin Jun 18, 2019

#2 push method #8

#2 push method #8

Conversation

josh-griffin commented Jun 13, 2019

Chris-Petty left a comment

Choose a reason for hiding this comment

josh-griffin commented Jun 13, 2019

josh-griffin commented Jun 14, 2019

Chris-Petty commented Jun 14, 2019 • edited Loading

josh-griffin commented Jun 16, 2019

josh-griffin commented Jun 17, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreievg left a comment

Choose a reason for hiding this comment

Chris-Petty commented Jun 18, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Chris-Petty commented Jun 14, 2019 •

edited

Loading