ecommerce icon indicating copy to clipboard operation
ecommerce copied to clipboard

Fix/hardcoded currency 135

Open wasuregusa18 opened this issue 2 years ago • 12 comments

Description

  1. Adds a context processor based on the settings variables outlined in this comment.
  2. Saves the localised currency code + symbol as global javascript variables in base template
  3. Utilizes either django template variable or javascript variable to replace hardcoded symbol with dynamic localized symbol
  4. Adds a test that context dictionary is correctly instantiated

Supporting information

https://github.com/openedx/build-test-release-wg/issues/135

Testing instructions

  1. Change the currency code value in the configuration
  2. 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?

wasuregusa18 avatar Jan 11 '23 05:01 wasuregusa18

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.

openedx-webhooks avatar Jan 11 '23 05:01 openedx-webhooks

I have submitted a signed contributor agreement.

wasuregusa18 avatar Jan 11 '23 06:01 wasuregusa18

Hi @wasuregusa18! Thank you for your submission! I'll have my team authorize tests for this pull request.

mphilbrick211 avatar Jan 11 '23 22:01 mphilbrick211

@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?

e0d avatar Jan 12 '23 16:01 e0d

@wasuregusa18 it looks like there are some linter errors: https://github.com/openedx/ecommerce/actions/runs/3890110722/jobs/6668323621#step:5:1939

e0d avatar Jan 12 '23 16:01 e0d

Hey @e0d! The work is scheduled for sometime within our next 2 sprints. Ideally, this would mean before March.

pshiu avatar Jan 12 '23 19:01 pshiu

Hi @wasuregusa18! Just checking in on this to follow up on Ed's comment and the failed checks.

mphilbrick211 avatar Jan 23 '23 19:01 mphilbrick211

@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?

wasuregusa18 avatar Jan 28 '23 02:01 wasuregusa18

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.

mphilbrick211 avatar Jan 30 '23 22:01 mphilbrick211

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?

mariajgrimaldi avatar Mar 20 '23 14:03 mariajgrimaldi

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!

mphilbrick211 avatar Mar 21 '23 18:03 mphilbrick211