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 console errors after placing an order for a single shop in multi-shop mode #3056

Closed
wants to merge 2 commits into from

Conversation

kieha
Copy link
Contributor

@kieha kieha commented Oct 6, 2017

Resolves #3024

How to test

  • Create a marketplace shop and create a product for that shop
  • In admin shop, place only that product in the cart and checkout
  • Observe no server errors

Copy link
Contributor

@spencern spencern left a comment

Choose a reason for hiding this comment

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

@kieha this does fix the console errors, but it introduces some issues in the email that gets sent to the customer, so my guess is that you've solved some symptoms rather than the core problem here.

image

@zenweasel
Copy link
Collaborator

I think this is a complicated problem that I am not immediately sure what the solution is, because when we had only one payment we knew what to look for but when we have multiple shops we can't just use getShopId because the current shop might not be the shop that the charge was against. Theoretically we should be passing in paymentMethod to this like we do with orders/refunds/create and then grabbing the information based on that rather than just assuming we can get the payment method by current shop.

@zenweasel
Copy link
Collaborator

Looking at the code this gets called a couple of times in two different contexts. In the context of the user it should cycle through all payments and return an array of refunds for all shops because a user doesn't care which shop, just wants the total (and that's what's throwing the error). The code is already expecting an array of refunds.

Working out how it will work in the order dashboard might be something we could figure out post-alpha release.

I do think we need to back out some of this code where we are failing silently.

@zenweasel
Copy link
Collaborator

Ok, last thought. If we use the order passed in only (don't re-grab it from the DB), which should be filtered already, then we should just be able to always query for all the billing records on the order and return an array because orders from dashboard should already be filtered so that should fix both situations I think.

@zenweasel
Copy link
Collaborator

Rather than modify @kieha's PR I opened a new PR with an alternative fix here: #3064

@kieha
Copy link
Contributor Author

kieha commented Oct 9, 2017

Closing this in favor of #3064

@kieha kieha closed this Oct 9, 2017
@spencern spencern deleted the kieha-fix-issue-3024 branch November 9, 2017 06:45
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.

None yet

3 participants