-
Notifications
You must be signed in to change notification settings - Fork 29
REV-2575: update order status page to go through commerce-coordinator #181
REV-2575: update order status page to go through commerce-coordinator #181
Conversation
Codecov Report
@@ Coverage Diff @@
## master #181 +/- ##
==========================================
- Coverage 54.05% 48.78% -5.28%
==========================================
Files 15 15
Lines 148 164 +16
Branches 25 28 +3
==========================================
Hits 80 80
- Misses 65 79 +14
- Partials 3 5 +2
Continue to review full report at Codecov.
|
7ee0204 to
39d0d42
Compare
39d0d42 to
cb02949
Compare
cb02949 to
21a142d
Compare
| data = newData.data; | ||
| console.log('DKTEMP: CC data.data.results', newData.data.results); | ||
| console.log('DKTEMP: ecommerce data.results', data.results); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This big 'TEMPORARY CODE for rollout testing/confirmation' block is for use to sanity check using the commerce-coordinator endpoint for order history. Therefore I've included some debugging statements so our production test can be explicit about where the data came from. Is there a better way to do this, or are these statements okay for this temporary case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As long as we're not logging any PII, and assuming we'll be logged-in in our accounts anyways (so the information will be ours), we should be good!
I would change DKTEMP to something theseus/commerce-coordinator related though to be more specific, in case the temporary code is not as temporary as we imagined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good call- updated the logging prefix to 'REV-2577 LOG' (that'll be the testing ticket). There is PII in this data (name and address), but console.log should only make this appear in the user's own browser, as opposed to something like Splunk which shows server-side logging that's put there with a different/specific Logger.
| page_size: pageSize, | ||
| }, | ||
| }); | ||
| data = newData.data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here data is replaced with the newData from commerce-coordinator, but then both data and newData are logged - aren't they the same thing at this point in the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes these will get the same json results, the only difference being that one will go through commerce-coordinator. I do the replacement because even though these results aren't actually different, when the waffle flag is enabled we want the order history page to show the data coming back from commerce-coordinator. (As opposed to just calling the commerce-coordinator in the background but not actually showing the data we get from it).
julianajlk
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great Diane! Good to see how the waffle flag is being used as expected. Just some minor comments but overall makes sense (considering it was agreed that for now we will call ecommerce first to get the flag value :) )
Description: Update the order history page to be able to call the new commerce-coordinator endpoint from this PR.
JIRA: Link to JIRA ticket
Testing notes:
Result: the order history page loads the orders info (in double the time now that we're temporarily performing two sequential calls. This is around 48 seconds locally, but the prod page is under a second today so I don't expect that temporary doubling will be a problem. Will confirm with the team). Inspect in the browser's console tab to see the responses from the ecommerce and the commerce-coordianator's endpoints