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

Introduce order protection + Improved processing of the webhook #96

Merged
merged 9 commits into from
Feb 21, 2024

Conversation

BitcoinMitchell
Copy link
Collaborator

@BitcoinMitchell BitcoinMitchell commented Feb 17, 2024

Questions Answers
Description - Introduces order protection similar to what the woocommerce plugin has.
- Added payment to order for settled payments.
- Improved overall processing of an order.
- DX: Adds a simple bitcoin-cli.sh-script to interact with the bitcoind defined in the docker-compose.
Type improvement
BC breaks no
Deprecations no
Fixes #94
Depends on - #93
- #95
How to test - Mark an order as paid in the backend, mark the invoice as invalid in BTCPayServer. The order state will stay the same and a message will be added.
- For every settled payment, a payment should be added to the order.
- Re-triggering an InvoicePaymentSettled-webhook should not add an additional payment.

image

Base automatically changed from improve-configuration-page to 6.x February 19, 2024 15:43
@BitcoinMitchell BitcoinMitchell marked this pull request as ready for review February 19, 2024 15:44
@BitcoinMitchell BitcoinMitchell changed the title Draft: Introduce order protection + Improved processing of the webhook Introduce order protection + Improved processing of the webhook Feb 19, 2024
@BitcoinMitchell BitcoinMitchell linked an issue Feb 19, 2024 that may be closed by this pull request
modules/btcpay/btcpay.php Outdated Show resolved Hide resolved
Copy link
Contributor

@ndeet ndeet left a comment

Choose a reason for hiding this comment

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

Nice work and refactor, much cleaner and nice feature of the order protection 👍

Only a few nitpicks, feel free to ignore.

@BitcoinMitchell
Copy link
Collaborator Author

BitcoinMitchell commented Feb 19, 2024

Nice work and refactor, much cleaner and nice feature of the order protection 👍

Only a few nitpicks, feel free to ignore.

Thank you, I'll create PRs for the the nitpicks that I will merge into this PR, before I merge this entire PR into master.


I've noticed that maybe we are adding too many messages, will rethink that as well.

@BitcoinMitchell BitcoinMitchell added the awaiting response Awaiting extra information label Feb 19, 2024
@BitcoinMitchell BitcoinMitchell marked this pull request as draft February 19, 2024 22:16
@BitcoinMitchell BitcoinMitchell self-assigned this Feb 19, 2024
@BitcoinMitchell BitcoinMitchell force-pushed the protect-order branch 2 times, most recently from 6a2ec45 to ae49901 Compare February 20, 2024 22:41
@BitcoinMitchell BitcoinMitchell marked this pull request as ready for review February 20, 2024 22:42
@BitcoinMitchell BitcoinMitchell removed the awaiting response Awaiting extra information label Feb 20, 2024
Copy link
Contributor

@ndeet ndeet left a comment

Choose a reason for hiding this comment

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

Tested several use cases, partial payments, manually settled/invalid, expired, overpayment

Looks all good and with nice warning in case of unusual flow e.g. manually setting paid and then getting an expired / manually marked invalid webhook

Nice work 👍

@BitcoinMitchell
Copy link
Collaborator Author

BitcoinMitchell commented Feb 21, 2024

Tested several use cases, partial payments, manually settled/invalid, expired, overpayment

Looks all good and with nice warning in case of unusual flow e.g. manually setting paid and then getting an expired / manually marked invalid webhook

Nice work 👍

Awesome, glad to hear it all worked smoothly. The only thing I wanna double check now is different currencies (since I do value * rate for the payment). If that also works as expected, I'll merge and release this :)

Copy link
Contributor

@ndeet ndeet left a comment

Choose a reason for hiding this comment

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

LGTM!

@BitcoinMitchell BitcoinMitchell merged commit 5119635 into 6.x Feb 21, 2024
4 checks passed
@BitcoinMitchell BitcoinMitchell deleted the protect-order branch February 21, 2024 19:42
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.

Feature request: Add option to protect order status
2 participants