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

Added get_order_idempotency_key function #492

Closed
wants to merge 1 commit into from
Closed

Added get_order_idempotency_key function #492

wants to merge 1 commit into from

Conversation

Copy link
Contributor

@ChaseWiseman ChaseWiseman left a comment

Choose a reason for hiding this comment

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

thanks for the PR, @TimBHowe !

It seems like something like this would be better served by using the raw order ID, no? Considering that ID won't ever change. What do you think?

@TimBHowe
Copy link
Author

TimBHowe commented Jun 3, 2020

After reviewing some additional material on the subject, I agree the raw order ID would be better.

As it is just an incremented post ID I have also added a unique int based on the WordPress site URL to make it a bit more unique.

Please review and let me know if there are any other changes that should be made.

https://multithreaded.stitchfix.com/blog/2017/06/26/patterns-of-soa-idempotency-key/
https://medium.com/airbnb-engineering/avoiding-double-payments-in-a-distributed-payments-system-2981f6b070bb
https://brandur.org/idempotency-keys
https://stripe.com/blog/idempotency
https://developer.squareup.com/docs/working-with-apis/idempotency
https://developer.squareup.com/blog/understanding-the-essentials-idempotency/

@TimBHowe
Copy link
Author

#442

Copy link
Contributor

@ChaseWiseman ChaseWiseman left a comment

Choose a reason for hiding this comment

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

@TimBHowe apologies for missing your followup ping for review!

Regarding this PR itself, we'll certainly consider it for a future release of the framework. The only tweak we might make is switching from using the crc32() function to something like md5. We don't usually merge directly into master, but I'll retarget this branch once we spin up a new release branch.

That said, regarding the overall issue - I see that this is a support request with the Square plugin. Please note that while it uses this framework, I don't know whether or not the development team that is maintaining that plugin is planning on keeping it updated to the latest versions of this framework, so it may not see a change like this right away.

Secondly, the plugin itself really needs to determine when and when not to use this key. Simply switching to using this new value in the API request (instead of the unique_transaction_ref) would not be the way to go, because it would also be used in situations where cards are declined. In that case, you definitely wouldn't want to resend the exact same request if the customer retries.

In the case you mention in the linked tickets where Square is responding with a 500 error, you may not want to send the same key. One important distinction for using the idempotency key like this is that you only want to send it in times when you know you should retry a request. The 500 error may indicate that, but it also may not.

Rather, you'd want to resend the exact same request in cases where the API never responds and you aren't sure if it even went through, like a timeout. From that Square article:

It is important to note that getting back a “successful” response in the context of idempotency doesn’t necessarily mean that you are getting a 200 OK. Even if you get back a 401 or 500 or something else indicating that your requested operation didn’t work, you managed to get a response indicating that whatever was sent did not succeed. The intention here is to handle ambiguous moments where an operation was attempted but its unclear what the result was, so you retry it with the same idempotency_key.

Thanks again for the PR! I'm also going to make the Square plugin's team aware of this conversation so they know it's not necessarily resolved here and may require a bit more handling adjustments. I hope Square will be able to get to the bottom of the 500 error for you soon! 🍻

@TimBHowe
Copy link
Author

Hey, I'm going to review and redo the pull request using the notes and information you have supplied. Thanks

@TimBHowe TimBHowe closed this Oct 27, 2020
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