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/intl renewal #17

Merged
merged 5 commits into from
Aug 15, 2017
Merged

Fix/intl renewal #17

merged 5 commits into from
Aug 15, 2017

Conversation

papayaah
Copy link

@papayaah papayaah commented Jul 18, 2017

@hugobast @sherryson

this is ready for CR. the problem earlier is renewal isn't working for Canadian subscriptions because the order's currency is being set to USD.

The changes here will set the subsequent orders to follow the last completed order's currency and store id.

I also surfaced the currency in the admin subscription listing page, similar to our admin orders listing.

papayaah added 2 commits July 19, 2017 04:41
…he shipping rates

are being calculated for US addresses by default, if the currency is not set in the order.

Setting the currency for new orders generated ensures the right shipping methods are pulled
to match the customer's address
@papayaah papayaah self-assigned this Jul 18, 2017
@papayaah papayaah requested review from hugobast and sherryson July 19, 2017 15:20
Copy link

@hugobast hugobast left a comment

Choose a reason for hiding this comment

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

Looks good @mickeyren, I noted maybe one small improvement if you want to give that a go.

)
# use the subscription's email if present.
# we are doing it here because the order's callback associate_user! will
# override the order's email even if we set it during creation

Choose a reason for hiding this comment

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

What about instead of a comment you extract this part into a well named method?

Copy link

@sherryson sherryson left a comment

Choose a reason for hiding this comment

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

looks good! i agree with hugo's comment, and in my PR for adding items to subscriptions i do something similar if you want to take a look #18

@papayaah
Copy link
Author

@hugobast @sherryson updated - moved it into a callback instead, but i still left a comment. :)

@papayaah papayaah merged commit b970223 into v2 Aug 15, 2017
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