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

(fix: 3705) Fix for "capturing bulk orders throws server side error" #3822

Merged

Conversation

5 participants
@prinzdezibel
Copy link
Contributor

prinzdezibel commented Feb 22, 2018

Resolves #3705

Impact:major
Type: bugfix

Issue

Selecting an approved order (not necessarily shipped) and clicking "Capture" on the bulk order editor throws a server error, if the order was done with the Stripe payment processor

Solution

  • Don't call server side "orders/approvePayment" and
    "orders/capturePayments" for orders that are already captured.
  • Will call it if payment was approved, but not captured yet.

Breaking changes

none expected

Testing

  1. As an admin, enable Stripe payment
  2. As a user, order an item
  3. As an admin, approve the payment
  4. Optionally also capture the payment
  5. In the bulk editor, select all orders
  6. Choose "Capture" action

No error should be shown. Action should finish successfully, but only orders that are not captured yet should be considered.

Fix for "capturing bulk orders throws server side error"
Closes issue 3705

Specifics:
- Don't call server side "orders/approvePayment" and
"orders/capturePayments" for orders that are already captured.
- Will call it if payment was approved, but not captured yet.
- Error described in issue will only be seen if Stripe is used as
payment method.

@prinzdezibel prinzdezibel requested a review from jshimko Feb 22, 2018

@zenweasel zenweasel changed the title Fix for "capturing bulk orders throws server side error" (fix: 3705) Fix for "capturing bulk orders throws server side error" Feb 22, 2018

@prinzdezibel

This comment has been minimized.

Copy link
Contributor Author

prinzdezibel commented Feb 26, 2018

@jshimko would be great if you could have a look into this one. should be straight-forward.

@zenweasel

This comment has been minimized.

Copy link
Member

zenweasel commented Feb 26, 2018

@prinzdezibel Looks like you have conflicts here

@jshimko
Copy link
Contributor

jshimko left a comment

As Brent said, you have some merge conflicts to work out, but everything appears to be working as-is. Let me know when you resolve the conflicts and I'll test once more.

@aaronjudd

This comment has been minimized.

Copy link
Contributor

aaronjudd commented Feb 26, 2018

@zenweasel @jshimko what happened to "reviewers can and should resolve conflicts, when possible?" -> and if this was from an older PR, that wasn't updated to reflect merging into the current branch, shouldn't we close/or at least WIP it?

@zenweasel

This comment has been minimized.

Copy link
Member

zenweasel commented Feb 26, 2018

@aaronjudd Is that new? I did not know about that but makes sense.

prinzdezibel added some commits Feb 27, 2018

@prinzdezibel

This comment has been minimized.

Copy link
Contributor Author

prinzdezibel commented Feb 27, 2018

Merged and also fixed another problem in a marketplace scenario, where the appropriate billing record is not necessarily positioned at index 0 of the billing array.

@prinzdezibel

This comment has been minimized.

Copy link
Contributor Author

prinzdezibel commented Feb 28, 2018

Merged and tested in a marketplace setup and it looks good. @jshimko Would be great if we can get this over the finish line.

@spencern spencern changed the base branch from master to release-1.9.0 Mar 5, 2018

@spencern spencern merged commit 5394039 into release-1.9.0 Mar 5, 2018

3 of 4 checks passed

Codacy/PR Quality Review Not so good... This pull request quality could be better.
Details
WIP ready for review
Details
ci/circleci Your tests passed on CircleCI!
Details
security/snyk No dependency changes
Details

@spencern spencern deleted the fix-3705-prinzdezibel-bulk-capture-throws-error branch Mar 5, 2018

@spencern spencern referenced this pull request Mar 8, 2018

Merged

Release 1.9.0 #3941

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.