-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
When editing prices keep the currency locked. #3150
When editing prices keep the currency locked. #3150
Conversation
6a2a94e
to
cbfad0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot, @peterberkenbosch! 🎉
seems there are some unrelated failing specs on the rails51, I do see them in master now and then too actually. @kennyadsl could you restart the circleci workflow? |
@peterberkenbosch done! |
@peterberkenbosch Thanks, I think this works, I just have a doubt about if the currency choice should be disabled in the form. Can't/Shouldn't the price currency be edited by an admin user? |
I kept the existing expected(?) functionality in tact in this PR. Noticed the same though. Wanted to keep the PR to the bugfix, but I can update it to have the currency change as well. WDYT @kennyadsl |
I'm fine with the fix, just wondering. Maybe some git blame could help us remember why currency is not changeable. |
The currency component was introduced like that. It's used on multiple places actually. |
@peterberkenbosch can you please rebase now, I hope flaky specs are fixed with #3141 . |
cbfad0d
to
9d65036
Compare
@kennyadsl indeed! Specs are passing! Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good fix for now. Thanks @peterberkenbosch!
I'd like to see the admin have the ability to change price currencies in the near future.
Thanks @jacobherrington, will make another PR for that ;) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @peterberkenbosch !!!
Description
When editing a price that has a different currency then the default currency set, the edit form defaults the currency to the default currency.
This PR fixes that by passing in the price currency to the currency form partial. It also improves the feature spec for editing prices by actually verifying that the edit worked.
before: https://cl.ly/420fb991710b
after: https://cl.ly/2b141684337f
Checklist: