Skip to content
This repository was archived by the owner on May 6, 2024. It is now read-only.

feat: add SIMPLETHEME_I18N_DJANGO variable - #6679

Merged
nadeemshahzad merged 1 commit into
openedx-unsupported:masterfrom
open-craft:mtyaka/SE-5140-simple-theme-i18n
Apr 6, 2022
Merged

feat: add SIMPLETHEME_I18N_DJANGO variable#6679
nadeemshahzad merged 1 commit into
openedx-unsupported:masterfrom
open-craft:mtyaka/SE-5140-simple-theme-i18n

Conversation

@mtyaka

@mtyaka mtyaka commented Feb 2, 2022

Copy link
Copy Markdown
Contributor

This makes it possible to add or override django translations via the simple-theme role.

This adds a new SIMPLETHEME_I18N_DJANGO configuration variable to the simple_theme role, which makes it possible to set or override theme translations via configuration variables without having to check po/mo files into any theme repo. When combined with the EDXAPP_PREPEND_LOCALE_PATHS variable, it can also be used to override default edx-platform translations.

Test instructions

  1. Create an account and log into the sandbox.
  2. Enroll into the Demo course.
  3. Go to https://mtyaka.opencraft.hosting/update_lang/ and switch the language to Arabic (ar).
  4. Go to the dashboard and verify that the "View Course" button contains the following custom translation: "XXX استعراض المحتوى"
  5. Go to the forum, click the button to create a new post, and verify that the submit button at the bottom contains the custom translation: "إضافة منشور ZZZ"

Note: the sandbox is running lilac. It was provisioned with this patch cherry-picked into the lilac branch (without any conflicts).

Sandbox:

The sandbox was provisioned with the following settings:

EDXAPP_DEFAULT_SITE_THEME: simple-theme
SIMPLETHEME_ENABLE_DEPLOY: true

EDXAPP_COMPREHENSIVE_THEME_LOCALE_PATHS:
  - "{{ EDXAPP_COMPREHENSIVE_THEME_DIRS[0] }}/conf/locale"

SIMPLETHEME_I18N_DJANGO:
  - lang: ar
    domain: django
    headers: |
      "Plural-Forms: nplurals=6; plural=n==0 ? 0 : n==1 ? 1 : n==2 ? 2 : n%100>=3 && n%100<=10 ? 3 : n%100>=11 && n%100<=99 ? 4 : 5;\n"
    messages: |
      msgid "View Course"
      msgstr "XXX استعراض المحتوى"
  - lang: ar
    domain: djangojs
    headers: |
      "Plural-Forms: nplurals=6; plural=n==0 ? 0 : n==1 ? 1 : n==2 ? 2 : n%100>=3 && n%100<=10 ? 3 : n%100>=11 && n%100<=99 ? 4 : 5;\n"
    messages: |
      msgid "Submit"
      msgstr "تقديم YYY"
      msgid "Add a Post"
      msgstr "إضافة منشور ZZZ"

Configuration Pull Request

Make sure that the following steps are done before merging:

  • A SRE team member has approved the PR if it is code shared across multiple services and you don't own all of the services.
  • Are you adding any new default values that need to be overridden when this change goes live? If so:
    • Update the appropriate internal repo (be sure to update for all our environments)
    • If you are updating a secure value rather than an internal one, file a SRE ticket with details.
    • Add an entry to the CHANGELOG.
  • If you are making a complicated change, have you performed the proper testing specified on the Ops Ansible Testing Checklist? Adding a new variable does not require the full list (although testing on a sandbox is a great idea to ensure it links with your downstream code changes).
  • Think about how this change will affect Open edX operators. Have you updated the wiki page for the next Open edX release?

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Feb 2, 2022
@openedx-webhooks

openedx-webhooks commented Feb 2, 2022

Copy link
Copy Markdown

Thanks for the pull request, @mtyaka! I've created OSPR-6430 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.

@openedx-webhooks openedx-webhooks removed the open-source-contribution PR author is not from Axim or 2U label Feb 2, 2022
@mtyaka
mtyaka force-pushed the mtyaka/SE-5140-simple-theme-i18n branch from 9cc33f3 to 41b5644 Compare February 2, 2022 08:59
@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Feb 2, 2022
@mtyaka
mtyaka force-pushed the mtyaka/SE-5140-simple-theme-i18n branch from 41b5644 to bed978c Compare February 2, 2022 08:59
@natabene

natabene commented Feb 7, 2022

Copy link
Copy Markdown
Contributor

@mtyaka Thank you for your contribution. Is this ready for our review?

@mtyaka

mtyaka commented Feb 7, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for checking @natabene! I'm waiting for a couple of dependencies to get merged before I'll mark this ready for review. I'll ping you when it's ready.

@mtyaka
mtyaka force-pushed the mtyaka/SE-5140-simple-theme-i18n branch from bed978c to c84cfd2 Compare March 8, 2022 07:26
@mtyaka mtyaka changed the title WIP: feat: add SIMPLETHEME_I18N_DJANGO variable feat: add SIMPLETHEME_I18N_DJANGO variable Mar 8, 2022
@openedx-webhooks openedx-webhooks added needs triage waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. needs triage labels Mar 8, 2022

@farhaanbukhsh farhaanbukhsh left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

👍

  • ✅ I tested this on the sandbox and looked at the configuration and translations.
  • ✅ I read through the code
  • ❌ I checked for accessibility issues
  • ❌ Includes documentation
  • ❌ I made sure any change in configuration variables is reflected in the corresponding client's configuration-secure repository.

@mtyaka

mtyaka commented Mar 24, 2022

Copy link
Copy Markdown
Contributor Author

@natabene This is now ready for review.

@openedx-webhooks openedx-webhooks added awaiting prioritization and removed waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. labels Mar 25, 2022
@adzuci

adzuci commented Apr 4, 2022

Copy link
Copy Markdown
Contributor

@mtyaka I've skimmed this PR and believe it looks reasonable, however I'm not familiar with the pros and cons of this approach over other approaches, so I'd like to discuss it with our the edX SRE team.

Would you also mind rebasing it to resolve the conflicts if you have a moment to do so?

This makes it possible to add or override django translations via the
simple-theme role.
@mtyaka
mtyaka force-pushed the mtyaka/SE-5140-simple-theme-i18n branch from c84cfd2 to 1e55e8a Compare April 6, 2022 05:46
@mtyaka

mtyaka commented Apr 6, 2022

Copy link
Copy Markdown
Contributor Author

Thanks for looking at this @adzuci. We use this in Ocim to be able to make small adjustments to text for some of our clients without having to maintain a custom repo of gettext PO files.

I rebased the code and resolved the conflicts.

@nadeemshahzad
nadeemshahzad merged commit ae8100e into openedx-unsupported:master Apr 6, 2022
@openedx-webhooks

Copy link
Copy Markdown

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

@Kelketek
Kelketek deleted the mtyaka/SE-5140-simple-theme-i18n branch April 8, 2022 23:09
0x29a pushed a commit to open-craft/configuration that referenced this pull request May 4, 2022
…-5140-simple-theme-i18n

feat: add SIMPLETHEME_I18N_DJANGO variable
(cherry picked from commit ae8100e)
0x29a pushed a commit to open-craft/configuration that referenced this pull request May 28, 2022
…-5140-simple-theme-i18n

feat: add SIMPLETHEME_I18N_DJANGO variable
(cherry picked from commit ae8100e)
0x29a pushed a commit to open-craft/configuration that referenced this pull request May 30, 2022
…-5140-simple-theme-i18n

feat: add SIMPLETHEME_I18N_DJANGO variable
(cherry picked from commit ae8100e)
kaustavb12 pushed a commit to open-craft/configuration that referenced this pull request Aug 29, 2022
…-5140-simple-theme-i18n

feat: add SIMPLETHEME_I18N_DJANGO variable
(cherry picked from commit ae8100e)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

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.

6 participants