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 Cart.getShippingTotalByShop and use it in Cart.getSubTotal and in… #3214

Merged
merged 7 commits into from
Nov 20, 2017

Conversation

prosf
Copy link
Collaborator

@prosf prosf commented Oct 31, 2017

fix issue #3190

… 'cart/submitPayment' to pass correct shipping
@prosf prosf requested review from spencern and brent-hoover and removed request for brent-hoover October 31, 2017 18:44
@prosf prosf changed the base branch from release-1.5.5 to master November 3, 2017 09:36
@spencern
Copy link
Contributor

spencern commented Nov 4, 2017

@zenweasel I think you might be better suited to review this PR as it's dealing directly with your cart/order transforms

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

I tested this and it fixes the problem. Just have a couple style-type thing.

billingObject[billingItem.shopId] = billingItem.invoice.shipping;
}
return billingObject;
// The previous implementation can only be used if an invoice object already exists( while Invoice
Copy link
Collaborator

Choose a reason for hiding this comment

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

If this comment is why you are making the change, it probably should be in the PR comments and not in the code since "the previous implementation" will no longer be visible. I am also not clear what this comment means? Could you elaborate a little more?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, the right place for this comment is in the pr and not in the code.
Nevertheless what I mean here is that previously, getShippingTotalByShop couldn't be called before the buyer charging (e.g couldn't be used in order-preview of checkout) as it required access to the billing.$.invoice which gets created and is accessible only after the buyer-charging.

// for (const billingItem of this.billing) {
// billingObject[billingItem.shopId] = billingItem.invoice.shipping;
// }
// return billingObject;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Shouldn't leave commented code. Let's remove?

@prosf
Copy link
Collaborator Author

prosf commented Nov 10, 2017

@zenweasel Style changes done

@spencern spencern changed the base branch from master to release-1.5.9 November 20, 2017 23:48
@spencern spencern merged commit 18b1332 into release-1.5.9 Nov 20, 2017
@spencern spencern deleted the prosf-ShopShippingTotalFix branch November 20, 2017 23:50
@spencern spencern removed their request for review November 20, 2017 23:51
@spencern spencern mentioned this pull request Nov 21, 2017
Akarshit pushed a commit that referenced this pull request Jan 7, 2018
…lFix

fix Cart.getShippingTotalByShop and use it in Cart.getSubTotal and in…
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.

3 participants