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

Add caching on client id for purchases #1551

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

koutst
Copy link
Contributor

@koutst koutst commented Apr 25, 2024

  • To prevent accidental double purchases such as tipping.
  • All requests to this endpoint will require a client id.

@koutst koutst changed the title Add caching on client id Add caching on client id for purchases Apr 25, 2024
@koutst koutst marked this pull request as ready for review April 25, 2024 14:53
@koutst koutst requested a review from a team as a code owner April 25, 2024 14:53
Copy link
Contributor

@yattias yattias left a comment

Choose a reason for hiding this comment

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

LGTM

Comment on lines 81 to 90
cached_client_id = cache.get(f"purchase_client_id_{client_id}")
if cached_client_id:
return Response(
{
"detail": "This purchase has already been processed",
},
status=400,
)

cache.set(f"purchase_client_id_{client_id}", True, timeout=60)
Copy link
Member

Choose a reason for hiding this comment

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

Nit: It might be better to implement idempotent behavior and return the original transaction with a 2xx series response code.

Copy link
Member

@gzurowski gzurowski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@koutst koutst force-pushed the check-client-id-on-create-purchase branch from 8ccf11c to 0112cfc Compare April 30, 2024 14:46
Copy link
Member

@gzurowski gzurowski left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Contributor

@yattias yattias left a comment

Choose a reason for hiding this comment

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

LGTM

@koutst
Copy link
Contributor Author

koutst commented May 1, 2024

Paper contributions front end needs to be updated before deploying

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants