-
Notifications
You must be signed in to change notification settings - Fork 254
Fix/hardcoded currency 135 #3891
Fix/hardcoded currency 135 #3891
Conversation
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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf 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:
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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
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 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> |
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.
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?
@feanil: Is there something we can do to merge this before Sumac? Or, considering it will be archived anyway, should we close the PR? |
@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. |
Description
Supporting information
openedx/wg-build-test-release#135
Testing instructions
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)')
orgettext('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?