Skip to content
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

feat: Idempotency support #552

Merged
merged 6 commits into from
Aug 17, 2023
Merged

feat: Idempotency support #552

merged 6 commits into from
Aug 17, 2023

Conversation

nickevansuk
Copy link
Collaborator

Reintroduce commented out idempotency testing, in parallel to its support within reference implementation (openactive/OpenActive.Server.NET#100, openactive/OpenActive.Server.NET#207).

Note these tests are failing due to the following issues:

  1. idempotentRepeatBAfterBook needs to be wired up to work with initialiseSimpleC1C2BookFlow
  2. payment.identifier is currently randomly generated for each B request, which makes the request body unique. payment.identifier needs to be consistent across the two B calls in order for the second call to trigger the idempotency logic, which is based on two identical requests.

The reference implementation support for idempotency has been tested independently of the above and seems to work as expected.

orderItemCriteriaList,
bookRecipe,
defaultFlowStageParams,
omit(bookRecipeArgs, ['prerequisite']),
Copy link
Contributor

@lukehesluke lukehesluke Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One issue was this: Idempotent B wasn't getting the same starting args as B, which was messing up its expectations for what capacity should be asserted for after the 2nd B. This fixes that

@@ -52,6 +67,7 @@ async function runB({ templateRef, accessPass, brokerRole, uuid, sellerConfig, c
brokerRole,
positionOrderIntakeFormMap,
customer,
paymentIdentifier: paymentIdentifierIfPaid,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other issue was, as you mentioned, Nick, that paymentIdentifier was being re-generated for each B/P call. Rather than using stage input/output, I've just put it as an arg to the B / P stages, which should make it very easy to do things like test what happens if the repeated B call has a different payment identifier, if a B-after-P call has a different payment identifier, etc

Copy link
Collaborator Author

@nickevansuk nickevansuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One small comment on this

@nickevansuk nickevansuk merged commit ad0c68c into master Aug 17, 2023
19 of 31 checks passed
@nickevansuk nickevansuk deleted the coverage/idempotency branch August 17, 2023 15:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants