-
Notifications
You must be signed in to change notification settings - Fork 98
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
fix: URLs get the current value of LMS_BASE_URL from getConfig() #269 #273
Conversation
Thanks for the pull request, @hinakhadim! 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:
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. Once you've signed the CLA, please allow 1 business day for it to be processed. After this time, you can re-run the CLA check by adding a comment here that you have signed it. If the problem persists, you can tag the |
Hey @hinakhadim, thank you for this contribution! As the bot says, the next step will be for you to submit a signed CLA. Once that's done and the build is green, we can line this PR up for engineering review. |
Signing of CLA has been done. |
@hinakhadim Great! I'm lining the changes up for a test run. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #273 +/- ##
=======================================
Coverage 96.04% 96.04%
=======================================
Files 194 194
Lines 1844 1844
Branches 322 322
=======================================
Hits 1771 1771
Misses 68 68
Partials 5 5 ☔ View full report in Codecov by Sentry. |
Thank you! This change will also fix openedx/wg-build-test-release#336, so we'll need to backport it to quince once it's merged here. I tested the unenroll changes locally in my Tutor nightly environment, and now it's working as expected. Thank you again! |
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.
Looks great, thanks!
Works on my devstack as well, thanks for pinging me @arbrandes ! |
@hinakhadim, do you mind rebasing and resolving conflicts, so we can merge your PR? |
34832a2
to
a09966f
Compare
Thanks everyone for your attention. I've rebased the PR, feel free to have a look on it. |
a6d3d01
to
c73c0fe
Compare
I went ahead and created the backport for Quince. Once this is merged, we can review/merge that one. Thanks! |
Unenroll URL was picking the stale value of
getConfig().LMS_BASE_URL
. After the following changes, it'll pick the current value ofgetConfig().LMS_BASE_URL
. This is done on the basis ofopenedx/frontend-platform
. This PR will fix the #269 .