-
Notifications
You must be signed in to change notification settings - Fork 4k
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: session_language
should be renamed to update_language
#33234
Conversation
Thanks for the pull request, @shadinaif! 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. |
can you please review this @OmarIthawi ? |
Thanks @shadinaif.
|
3f9c1b0
to
4aa1aa9
Compare
This is a patch to pull request openedx#32578 because the `session_language` was renamed to `update_language` except in three locations causing a 500 error: Reverse for 'session_language' not found. 'session_language' is not a valid view function or pattern name
4aa1aa9
to
fabb18c
Compare
Thank you @OmarIthawi . I made a search all over the code, I found another missing one. So we have three now:
Done
It's not covered because the template parts are rendered iff header language selector is enabled. We can add tests, but I don't see it worth the trouble |
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.
@shadinaif the pull request title is still on the old patch
one. Please update it.
Otherwise it looks good!
As for tests, I'll see what other reviewers think, but my recommendation is to add them. With tests setup, this pull request wouldn't be needed.
session_language
should be renamed to update_language
Hi @OmarIthawi and @shadinaif! Can this PR be closed? |
@mphilbrick211 I think the PR is still valid, but we need tests for it. |
Hi @OmarIthawi - just checking in on this |
@mphilbrick211 I think this one is safe to merge. I've faced this bug multiple times already in devstack. Please consider merging it, even if has no tests. |
I'm not able to merge, so I'll see if @brian-smith-tcril can maybe do it? |
@brian-smith-tcril @shadinaif @mphilbrick211 this pull request is no longer needed because another similar fix has already been merged: I was trying to rebase to fix tests and found out. |
@shadinaif Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future. |
fix:
session_language
should be renamed toupdate_language
Description
fix:
session_language
should be renamed toupdate_language
This is a patch to pull request #32578 because the
session_language
was renamedto
update_language
except in three two locations causing a 500 errorNote: updating assets is needed after the fix
Supporting information
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.
Testing instructions
Please provide detailed step-by-step instructions for testing this change.
Deadline
"None" if there's no rush, or provide a specific date or event (and reason) if there is one.
Other information
Include anything else that will help reviewers and consumers understand the change.