Skip to content
This repository has been archived by the owner on Nov 4, 2024. It is now read-only.

Fix/hardcoded currency 135 #3891

Conversation

wasuregusa18
Copy link

@wasuregusa18 wasuregusa18 commented Jan 11, 2023

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

openedx/wg-build-test-release#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?

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 11, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Jan 11, 2023

Thanks for the pull request, @wasuregusa18!

What's next?

Please work through the following steps to get your changes ready for engineering review:

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

  • If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
    • This process (including the steps you'll need to take) is documented here.
  • If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

  • Dependencies

    This PR must be merged before / after / at the same time as ...

  • Blockers

    This PR is waiting for OEP-1234 to be accepted.

  • Timeline information

    This PR must be merged by XX date because ...

  • Partner information

    This is for a course on edx.org.

  • Supporting documentation
  • Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently unmaintained.

To get help with finding a technical reviewer, tag the community contributions project manager for this PR in a comment and let them know that your changes are ready for review:

  1. On the right-hand side of the PR, find the Contributions project, click the caret in the top right corner to expand it, and check the "Primary PM" field for the name of your PM.
  2. Find their GitHub handle here.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

  • The size and impact of the changes that it introduces
  • The need for product review
  • Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

@wasuregusa18
Copy link
Author

I have submitted a signed contributor agreement.

@mphilbrick211
Copy link

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

@mphilbrick211 mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 11, 2023
@e0d e0d removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 12, 2023
@e0d
Copy link

e0d commented Jan 12, 2023

@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
Copy link

e0d commented Jan 12, 2023

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

@e0d e0d added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jan 12, 2023
@pshiu
Copy link
Contributor

pshiu commented Jan 12, 2023

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

@mphilbrick211
Copy link

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

@wasuregusa18
Copy link
Author

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

@mphilbrick211 mphilbrick211 removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Jan 30, 2023
@mphilbrick211
Copy link

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.

@mariajgrimaldi
Copy link

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?

@mphilbrick211
Copy link

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!

@@ -35,7 +35,12 @@
<script src="{{ optimizely_snippet_src }}"></script>
{% endif %}

{% block extrahead %}{% endblock %}
{% block extrahead %}
<script>
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the reason for putting this in the extrahead block? Would this mean it would get lost if someone replaces this block during template inheritance?

@mariajgrimaldi
Copy link

@feanil: Is there something we can do to merge this before Sumac? Or, considering it will be archived anyway, should we close the PR?

@feanil
Copy link
Contributor

feanil commented Nov 4, 2024

@mariajgrimaldi sorry for getting back to you late on this but we will be archiving this today hopefully so I think we'll just close this.

@feanil feanil closed this Nov 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants