Fix/hardcoded currency 135
Description
- Adds a context processor based on the settings variables outlined in this comment.
- Saves the localised currency code + symbol as global javascript variables in base template
- Utilizes either django template variable or javascript variable to replace hardcoded symbol with dynamic localized symbol
- Adds a test that context dictionary is correctly instantiated
Supporting information
https://github.com/openedx/build-test-release-wg/issues/135
Testing instructions
- Change the currency code value in the configuration
- Check that in the views the currency symbol/code has been correctly updated
Other information
Instructions here for setting up the repo, didn't work for me, so I just tested the core functionality on a separate basic django project.
Ideally, one would also want to confirm that template variables in the js templates are correctly replaced (otherwise one could use <%=currencySymbol%>) And add a test of the above.
Lastly, there are several places in the code where gettext('Price (USD)') or gettext('Fixed ($USD)'). I could also make these dynamic, but I'm assuming this serves as a key for a translation lookup and having it dynamic may cause issues. Would a maintainer please advise on best way to handle this?
Thanks for the pull request, @wasuregusa18! Please note that it may take us up to several weeks or months to complete a review and merge your PR.
Feel free to add as much of the following information to the ticket as you can:
- supporting documentation
- Open edX discussion forum threads
- timeline information ("this must be merged by XX date", and why that is)
- partner information ("this is a course on edx.org")
- any other information that can help Product understand the context for the PR
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.
Please let us know once your PR is ready for our review and all tests are green.
I have submitted a signed contributor agreement.
Hi @wasuregusa18! Thank you for your submission! I'll have my team authorize tests for this pull request.
@pshiu I know that 2U are planning to start deploying from a branch. This would allow us to review and merge PRs that the community needs without impacting you. What's the time line for that?
@wasuregusa18 it looks like there are some linter errors: https://github.com/openedx/ecommerce/actions/runs/3890110722/jobs/6668323621#step:5:1939
Hey @e0d! The work is scheduled for sometime within our next 2 sprints. Ideally, this would mean before March.
Hi @wasuregusa18! Just checking in on this to follow up on Ed's comment and the failed checks.
@mphilbrick211 apologies for the delay was swamped with work. So I was able to get the devstack working and can probably fix the tests. However, going through the site I think the scope of this issue may need to be reevaluated. The issue mentions only hardcoded currency values in templates which this pr should solve.
However, in most templates this currency filter is used and it has no dependency on the two setting variables I was directed to use in the issue thread. Should I also be handling these cases?
At the same time, there seems to be a way of defining currency of a product within the website. For example the Refund model has a currency field and StockRecord appears to have a price_currency field. Is handling of these dynamic currencies also within the scope?
Hi @colinbrash and @nedbat - flagging the above question. I'm not able to tag Kelly.
@wasuregusa18 - thank you for your update - I'm hoping someone tagged above can address your question.
This issue should fix https://github.com/openedx/wg-build-test-release/issues/135, as indicated in the PR's cover letter.
@wasuregusa18: thanks for the PR with the solution! Now, can you attach a before and after? Thanks!
Regarding your questions, the BTR issue scope is to solve what was reported. Now, regarding the other issues you mentioned, we need to find out with the repo's maintainers if that also needs to be fixed. Can we consider that a different issue? Dynamic currencies? So it doesn't block this PR any further.
Hi @mphilbrick211! Who can we talk to about those issues reported above?
This issue should fix openedx/wg-build-test-release#135, as indicated in the PR's cover letter.
@wasuregusa18: thanks for the PR with the solution! Now, can you attach a before and after? Thanks!
Regarding your questions, the BTR issue scope is to solve what was reported. Now, regarding the other issues you mentioned, we need to find out with the repo's maintainers if that also needs to be fixed. Can we consider that a different issue? Dynamic currencies? So it doesn't block this PR any further.
Hi @mphilbrick211! Who can we talk to about those issues reported above?
@colinbrash @nedbat would one of you be able to help with this? Thanks!