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

Add missing default_currency field on admin/stores #2091

Merged
merged 1 commit into from
Jul 17, 2017

Conversation

oeN
Copy link

@oeN oeN commented Jul 14, 2017

The Spree::Store have a default_currency field that is used in
core/lib/spree/core/controller_helpers/pricing.rb but never valorized
because the field was missing from the store form.

Copy link
Member

@kennyadsl kennyadsl left a comment

Choose a reason for hiding this comment

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

Left a small note but overall it works for me, thanks!


@store = Spree::Store.last

expect(@store.default_currency).to eq 'EUR'
Copy link
Member

Choose a reason for hiding this comment

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

@oeN maybe we should also test that the currency wasn't EUR before selecting/saving it?

The Spree::Store have a default_currency field that is used in
`core/lib/spree/core/controller_helpers/pricing.rb` but never valorized
because the field was missing from the store form.
@oeN oeN force-pushed the add-store-default-currency branch from 54708fd to e1cb3b3 Compare July 14, 2017 13:18
Copy link
Contributor

@mamhoff mamhoff left a comment

Choose a reason for hiding this comment

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

Great, thank you!

Copy link
Contributor

@cbrunsdon cbrunsdon 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 to me too, thanks

@jhawthorn jhawthorn merged commit f832463 into solidusio:master Jul 17, 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.

5 participants