-
Notifications
You must be signed in to change notification settings - Fork 57
feat(quotes): B2B-3491 consume and propagate quote UUIDs #592
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
|
Wondering if we could replace the |
|
@icatalina as we discussed, we will go ahead with the UUID solution, with notice to be given to merchants with custom buyer portals before rolling out the changes. |
icatalina
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.
Do we have tests covering these scenarios? should we?
4cf0b97 to
2750b19
Compare
|
Good call out, have added tests, as well as refactored the name of the parameter to be just |
icatalina
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.
Looks good, thanks 🙇
| await userEvent.click(within(table).getByRole('row', { name: /Quote with UUID/ })); | ||
|
|
||
| expect(navigation).toHaveBeenCalledWith( | ||
| `/quoteDetail/123456?date=${getUnixTime(new Date('1 January 2025'))}&uuid=abc-123-def-456`, |
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.
if there's an issue with getUnixTime, this test won't catch it. Imagine:
const getUnixTime = () => 'potato';it is better not to rely on code for the expectations. Simply hard-code the expected value.
| const data = { | ||
| id: Number(id), | ||
| date: date.toString(), | ||
| ...(uuid && { uuid: uuid.toString() }), |
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.
🤔 undefined uuids are already handled in the get*QuoteDetails methods. Might be easier to pass it as undefined instead of doing this conditional spread:
const data = {
id: Number(id),
date: date.toString(),
uuid: uuid ? uuid.toString() : undefined,
};
Jira: B2B-3491
What/Why?
Consumes the UUIDs added to quote links in this backend PR so that they are used in graphql calls to retrieve quotes.
Rollout/Rollback
Revert this commit. This PR is intended to be backwards compatible with existing quotes, so this shouldn't need to be reverted, but the backend change rollout is controlled by feature flag B2B-3491.quote_uuid_generation_and_access
Testing
See backend PR for testing screenshots.