[Feat][DEVX-424] Map Error Code to commercetools HTTP Error Code #924
Conversation
- map code field to http error code
🦋 Changeset detectedLatest commit: 5c2cd15 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
- fix types in the ts-sdk-apm package
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #924 +/- ##
=======================================
Coverage 96.72% 96.73%
=======================================
Files 112 112
Lines 2139 2145 +6
Branches 628 633 +5
=======================================
+ Hits 2069 2075 +6
Misses 63 63
Partials 7 7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
ajimae
requested review from
lojzatran and
jenschude
and removed request for
a team
There was a problem hiding this comment.
Good job 👍
I'm lacking a context here. Why are we changing the error structure? Was someone complaining? There is no support issue linked to the JIRA task.
Also it's not clear what is the decision for this task in JIRA. Please add the final decision there if it has been made.
| }, | ||
| }) | ||
| .execute() | ||
| expect(productUpdateResponse.statusCode).toBe(200) |
There was a problem hiding this comment.
This expect is not being called at all. Should the test pass or fail if it reaches this point?
| @@ -238,8 +240,7 @@ describe('Concurrent Modification Middleware', () => { | |||
| }) | |||
| .execute() | |||
| } catch (e) { | |||
| console.error(e) | |||
| throw e | |||
| /** noop */ | |||
There was a problem hiding this comment.
Why did you remove this? The test should actually fail if it reaches here.
| .execute() | ||
|
|
||
| expect(productUpdateResponse.statusCode).toBe(200) | ||
| try { |
There was a problem hiding this comment.
Why did you add try/catch? The test should actually fail if there is an exception.
| message: string | ||
| code?: number | ||
| message?: string | ||
| code?: string |
There was a problem hiding this comment.
In the JIRA task this is written: We can’t change the code field to string as it would be a breaking change.
There was a problem hiding this comment.
That was my suggestion, I even went ahead to suggest we add a new field for it, but you specifically suggested that since it wasn't documented, we can go ahead and make the change. Have you forgotten? 🙃
I believe the outcome of our discussion was not properly represented in the JIRA ticket.
There was a problem hiding this comment.
Yes, I remember I said sth, but I have forgotten 🙃 Please add the outcome to the ticket so it's clear 👍
implement feedback
Not really changing, there are some duplicate fields I was trying to remove and also, it's not fully following the structure of the original response object. Anyways, I guess we will have to do this in a different task.
I will definitely update the JIRA ticket to reflect the correct context. |
Summary
Map error response code field to return commercetools HTTP error response code property
Completed Tasks