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 new param current_status to /api/order cancel #1325

Conversation

jerryfletcher21
Copy link
Contributor

@jerryfletcher21 jerryfletcher21 commented Jun 15, 2024

What does this PR do?

This PR introduces a new optional parameter current_status that can be used when cancelling an order. The server checks if current_status is the correct status of the order and returns an error without cancelling the order if it is not.
This way a client can be sure to cancel an order just if the status is the one it specified.
Fixes backed and tests of #1311.

Checklist before merging

  • Install pre-commit and initialize it: pip install pre-commit, then pre-commit install. Pre-commit installs git hooks that automatically check the codebase. If pre-commit fails when you commit your changes, please fix the problems it points out.

also added ENUM_NAME_OVERRIDES in SPECTACULAR_SETTINGS, because without
it when generating api-latest.yaml this warning is returned:
Warning: encountered multiple names for the same choice set
(CurrentStatusEnum). This may be unwanted even though the generated
schema is technically correct. Add an entry to ENUM_NAME_OVERRIDES to
fix the naming.
Copy link
Collaborator

@Reckless-Satoshi Reckless-Satoshi left a comment

Choose a reason for hiding this comment

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

This PR is solid and complete. We will be merging #1326 instead given that it implements the frontend fixes and has integration tests as well. But a few things from this PR can be implemented there for the best of both worlds 😄

Makes sense to split the reward as discussed on #1326. Please post an invoice for 40K Sats (+ week expiration if possible) 🚀

Closing.

@@ -146,6 +146,9 @@
}
},
"REDOC_DIST": "SIDECAR",
"ENUM_NAME_OVERRIDES": {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Aha! TIL 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I also learned it today, well yesterday, while searching how to remove that warning ahahaha

@jerryfletcher21
Copy link
Contributor Author

This PR is solid and complete. We will be merging #1326 instead given that it implements the frontend fixes and has integration tests as well. But a few things from this PR can be implemented there for the best of both worlds 😄

Makes sense to split the reward as discussed on #1326. Please post an invoice for 40K Sats (+ week expiration if possible) 🚀

Closing.

Sure, perfectly fine with me, I left some comments in #1326

@Reckless-Satoshi
Copy link
Collaborator

40K Sats (+ week expiration if possible) 🚀

Feel free to post the invoice in this threat or in the issue. Thank you!

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