Skip to content

[OSPR-5979] Port "LANGUAGE_CODE" site configuration option to Lilac - #28502

Merged
1 commit merged into
openedx:open-release/lilac.masterfrom
open-craft:0x29a/bb4415/port-language-config-option-to-lilac
Sep 28, 2021
Merged

[OSPR-5979] Port "LANGUAGE_CODE" site configuration option to Lilac#28502
1 commit merged into
openedx:open-release/lilac.masterfrom
open-craft:0x29a/bb4415/port-language-config-option-to-lilac

Conversation

@0x29a

@0x29a 0x29a commented Aug 20, 2021

Copy link
Copy Markdown
Contributor

Description

This PR contains changes from https://github.com/edx/edx-platform/pull/27696 PR cherry-picked to open-release/lilac.master branch.

(cherry picked from commit b01544d)
@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Aug 20, 2021
@openedx-webhooks

Copy link
Copy Markdown

Thanks for the pull request, @0x29a! I've created OSPR-5979 to keep track of it in JIRA, where we prioritize reviews. 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.

@0x29a 0x29a changed the title Port "LANGUAGE_CODE" site configuration option to Lilac [OSPR-5979] Port "LANGUAGE_CODE" site configuration option to Lilac Aug 20, 2021
@pomegranited

Copy link
Copy Markdown
Contributor

FYI @arbrandes :)

@cmltaWt0

Copy link
Copy Markdown
Contributor

@pomegranited Going to test it.

Diff is simple and clear - I want to test some corner cases to ensure it works as expected.

@cmltaWt0

Copy link
Copy Markdown
Contributor

Notes so far

  • legacy UI is translated from the first point of view
  • MFE Account application doesn't get new language prefs

image

image

So presumably some effort should be dedicated to MFEs to work with this change.

P.S. it's a quick review and I'll will do some deeper tests:

  1. Check possible BackboneJS + Underscore templates parts to be compatible with the changes.
  2. Review the MFE logic a bit closer to ensure it's not a configuration issue.

@cmltaWt0

cmltaWt0 commented Aug 29, 2021

Copy link
Copy Markdown
Contributor

Additional finding.

Proposed port is not complete and doesn't change some JS (Backbone) default language logic:
Screenshot 2021-08-29 at 21 03 49

In case we change site language account settings we will get the correct behaviour:

image

Screenshot 2021-08-29 at 21 00 09

So as a conclusion- this PR should be enhanced a bit to be a complete solution.
However we can merge it and get almost complete default language behaviour but we must add Known issue as a part a this PR as I describer above.

@arbrandes @BbrSofiane what do you think?

@natabene

natabene commented Sep 1, 2021

Copy link
Copy Markdown
Contributor

@0x29a Thank you for your contribution. I see it is already assigned to @arbrandes .

@arbrandes

Copy link
Copy Markdown
Contributor

@cmltaWt0, thanks a lot for the detailed review, including testing on Lilac. Here's what I think, though:

  1. We need to post this to the forum and wait to see if there are any objections to us merging this to Lilac. It is a new feature, after all, and I fear it falls under the "useful but too few users" category, which means somebody might object to it.

  2. It looks like it might indeed be incomplete - or that the platform itself has some unrelated translation bugs. I've asked the original authors to investigate.

@arbrandes

Copy link
Copy Markdown
Contributor

Opened discussion about this in the forum, as per the BTR group's ADR on merging backports.

@BbrSofiane

Copy link
Copy Markdown
Contributor

@edx-community-bot merge

@ghost ghost added the automerge label Sep 28, 2021
@ghost
ghost merged commit d18656e into openedx:open-release/lilac.master Sep 28, 2021
@openedx-webhooks

Copy link
Copy Markdown

@0x29a 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge merged 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