feat: Add gettext for JS translation #70

shadinaif · 2023-05-27T06:48:34Z

feat: Add gettext for JS translation

JS translations will not be fetched without directing gettext to the correct javascript file

Tested on openedx-translation fork. https://github.com/Zeit-Labs/openedx-translations/pull/76/files#r1351711701

References

This pull request is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.

Check the links above for full information about the overall project.

Internalization is being rearchitected in Open edX Python, XBlock, Micro-frontend, and other projects. There are a number of immediately visible changes:

Remove source and language translations from the repositories, hence no .json, .po or .mo files will be committed into the repos.
Add standardized make extract_translations in all repositories
Push user-facing messages strings into openedx/openedx-translations.
Integrate root repositories with openedx/openedx-atlas to pull translations on build/deploy time

Breaking Changes

One of the primary goals of the project is to avoid breaking changes. If you noticed any suspicious code, please raise your concern. But before that, please know the strategy we're following to avoid breaking changes

For XBlocks:

The standard translation path must be conf/locale. Therefore, we are creating a link from conf/locale to translations so Atlas can find the path without disrupting the current way of having translations locally within the XBlocks
openedx-translations will have a related PR that adds the XBlock to the pipeline. This will not affect the current way of managing/updating translations
At the end of the project, a clear change log will be added and all translations will be handled by Atlas. Thus, the local translation will be removed from the repo within the version bump
A notification for the community will be published to ensure that everyone knows why translations directories are removed from all repos

openedx-webhooks · 2023-05-27T06:48:39Z

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:

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.

OmarIthawi · 2023-07-22T17:26:07Z

@shadinaif what's the status of this PR? Do we still need this fix after the recent work you've done?

mphilbrick211 · 2023-09-12T02:30:28Z

Hi @shadinaif and @OmarIthawi - just checking to see if this can be closed?

OmarIthawi · 2023-09-12T06:25:27Z

@shadinaif I think we should add the following note to the PR description:

This contribution is part of the FC-0012 project which is sparked by the Translation Infrastructure update OEP-58.

Is this a follow up to the PR below?

fix: django.po contains duplicate entries #69

OmarIthawi · 2023-09-12T06:25:52Z

Hi @shadinaif and @OmarIthawi - just checking to see if this can be closed?

It seems to be pending work that we forgot to follow up on.

shadinaif · 2023-09-13T06:25:18Z

setup.py

@@ -78,10 +78,10 @@ def get_version(file_path):
    license='AGPL 3.0',
    entry_points={
        'xblock.v1': [
-            'recommender = recommender:RecommenderXBlock',
+            'recommender = recommender.recommender:RecommenderXBlock',


because we're not importing RecommenderXBlock in __init__.py

shadinaif · 2023-09-13T07:08:02Z

@OmarIthawi, @brian-smith-tcril please review. This one is ready

OmarIthawi

sorry, this is an old review that I haven't posted it.

OmarIthawi · 2023-09-13T08:00:46Z

Makefile

@@ -31,3 +32,7 @@ extract_translations: ## extract strings to be translated, outputting .po files
 	fi


@shadinaif now the i18n-tools==1.2.0 package has been released. Would you mind using it here?

I suppose this XBlock won't use the --merge-po-files option, right?

it will use the option @OmarIthawi , but this goes into another PR

Upgrade requirements

Use new options --no-segment --merge-po-files

Remove django.po and the text.po symlink

OmarIthawi

A minor change before this is ready to go.

manage.py

recommender/conf/locale/settings.py

OmarIthawi

Thanks @shadinaif! Please test it on the openedx-translations repo and it's should be ready to get merged.

shadinaif · 2023-10-10T07:25:47Z

all good @OmarIthawi , please take a final look

OmarIthawi

Thanks @shadinaif!! Looks great!

brian-smith-tcril · 2023-10-10T13:48:07Z

@OmarIthawi I see that you recommended dev_settings.py as a filename. I don't see that filename used in other XBlocks throughout the org. I do see test_settings.py used quite often, would that name be inappropriate here?

brian-smith-tcril · 2023-10-10T14:56:51Z

I see that dev_settings.py is being added here too openedx/edx-cookiecutters#382. Is the expectation that this file would contain more than just locale settings? If not, would locale_settings.py work? It's just not clear to me what dev_settings entails.

OmarIthawi · 2023-10-10T15:36:28Z

@OmarIthawi I see that you recommended dev_settings.py as a filename. I don't see that filename used in other XBlocks throughout the org. I do see test_settings.py used quite often, would that name be inappropriate here?

Good question @brian-smith-tcril . Here's why I recommended dev_settings.py.

test_settings.py: implies testing and unit tests, but we need it for locale and development.
locale_settings.py: is accurate in this context, but the file might be repurposed in the future, so the name wouldn't be helpful.
dev_settings.py: is indeed a new name, which servers a shared for testing, locale and development.

Please let me know what's your thoughts. I'm okay with reverting back to locale or even test if you think it's more appropriate.

I'll copy this comment into openedx/edx-cookiecutters#382.

brian-smith-tcril · 2023-10-18T16:20:57Z

For consistency with the naming decided on in openedx/edx-cookiecutters#382 (comment), we should rename dev_settings.py to translation_settings.py

JS translations will not be shown fetched without directing (gettext) to the correct javascript file Refs: FC-0012 OEP-58

shadinaif · 2023-10-18T17:20:51Z

For consistency with the naming decided on in openedx/edx-cookiecutters#382 (comment), we should rename dev_settings.py to translation_settings.py

Thank you @brian-smith-tcril , Done

brian-smith-tcril

This looks great!

brian-smith-tcril · 2023-10-18T22:34:15Z

requirements/common_constraints.txt

-# edx-sphinx-theme is not compatible with latest Sphinx==6.0.0 version 
-# Pinning Sphinx version unless the compatibility issue gets resolved
-# For details, see issue https://github.com/openedx/edx-sphinx-theme/issues/197
-sphinx<6.0.0


This stood out to me when I was looking through this PR so I'm leaving a comment to provide context

This is the only mention of sphinx in this repo https://github.com/search?q=repo%3Aopenedx%2FRecommenderXBlock%20sphinx&type=code

edx-sphinx-theme is deprecated [DEPR]: edx-sphinx-theme public-engineering#200

latest sphinx on pypi as of writing is 7.2.6 https://pypi.org/project/Sphinx/

Removing this is definitely the right call.

openedx-webhooks · 2023-10-18T22:39:24Z

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

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 27, 2023

shadinaif force-pushed the shadinaif/FC-0012-OEP-58.05 branch from 503724b to 33d3014 Compare September 12, 2023 07:16

shadinaif changed the title ~~WIP - Fix JS translation~~ feat: Add gettext for JS translation Sep 13, 2023

shadinaif force-pushed the shadinaif/FC-0012-OEP-58.05 branch from 33d3014 to 0372561 Compare September 13, 2023 06:24

shadinaif commented Sep 13, 2023

View reviewed changes

shadinaif marked this pull request as ready for review September 13, 2023 06:35

OmarIthawi reviewed Sep 21, 2023

View reviewed changes

OmarIthawi self-requested a review September 21, 2023 09:55

OmarIthawi reviewed Sep 21, 2023

View reviewed changes

manage.py Outdated Show resolved Hide resolved

recommender/conf/locale/settings.py Outdated Show resolved Hide resolved

OmarIthawi self-requested a review September 21, 2023 14:37

mphilbrick211 added waiting for eng review PR is ready for review. Review and merge it, or suggest changes. and removed waiting for eng review PR is ready for review. Review and merge it, or suggest changes. labels Oct 3, 2023

shadinaif force-pushed the shadinaif/FC-0012-OEP-58.05 branch 3 times, most recently from 43374da to 95fdb79 Compare October 10, 2023 07:04

OmarIthawi approved these changes Oct 10, 2023

View reviewed changes

shadinaif force-pushed the shadinaif/FC-0012-OEP-58.05 branch from 95fdb79 to b6827a1 Compare October 10, 2023 07:12

shadinaif mentioned this pull request Oct 10, 2023

chore: add updated translation source files Zeit-Labs/openedx-translations#76

Closed

shadinaif requested a review from OmarIthawi October 10, 2023 07:25

OmarIthawi approved these changes Oct 10, 2023

View reviewed changes

OmarIthawi mentioned this pull request Oct 10, 2023

feat: FC-0012 - add atlas support for cookiecutter-xblock openedx/edx-cookiecutters#382

Merged

feat: Add gettext for JS translation

dc013ff

JS translations will not be shown fetched without directing (gettext) to the correct javascript file Refs: FC-0012 OEP-58

shadinaif force-pushed the shadinaif/FC-0012-OEP-58.05 branch from b6827a1 to dc013ff Compare October 18, 2023 17:20

brian-smith-tcril approved these changes Oct 18, 2023

View reviewed changes

brian-smith-tcril merged commit d32e7e4 into openedx:master Oct 18, 2023
3 checks passed

shadinaif deleted the shadinaif/FC-0012-OEP-58.05 branch October 19, 2023 11:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Add gettext for JS translation #70

feat: Add gettext for JS translation #70

shadinaif commented May 27, 2023 •

edited

Loading

openedx-webhooks commented May 27, 2023 •

edited

Loading

OmarIthawi commented Jul 22, 2023

mphilbrick211 commented Sep 12, 2023

OmarIthawi commented Sep 12, 2023

OmarIthawi commented Sep 12, 2023

shadinaif Sep 13, 2023

shadinaif commented Sep 13, 2023

OmarIthawi left a comment

OmarIthawi Sep 13, 2023

shadinaif Sep 21, 2023

OmarIthawi left a comment

OmarIthawi left a comment •

edited

Loading

shadinaif commented Oct 10, 2023

OmarIthawi left a comment

brian-smith-tcril commented Oct 10, 2023

brian-smith-tcril commented Oct 10, 2023

OmarIthawi commented Oct 10, 2023

brian-smith-tcril commented Oct 18, 2023

shadinaif commented Oct 18, 2023

brian-smith-tcril left a comment

brian-smith-tcril Oct 18, 2023 •

edited

Loading

openedx-webhooks commented Oct 18, 2023

		@@ -31,3 +32,7 @@ extract_translations: ## extract strings to be translated, outputting .po files
		fi

feat: Add gettext for JS translation #70

feat: Add gettext for JS translation #70

Conversation

shadinaif commented May 27, 2023 • edited Loading

References

Breaking Changes

openedx-webhooks commented May 27, 2023 • edited Loading

OmarIthawi commented Jul 22, 2023

mphilbrick211 commented Sep 12, 2023

OmarIthawi commented Sep 12, 2023

OmarIthawi commented Sep 12, 2023

shadinaif Sep 13, 2023

Choose a reason for hiding this comment

shadinaif commented Sep 13, 2023

OmarIthawi left a comment

Choose a reason for hiding this comment

OmarIthawi Sep 13, 2023

Choose a reason for hiding this comment

shadinaif Sep 21, 2023

Choose a reason for hiding this comment

OmarIthawi left a comment

Choose a reason for hiding this comment

OmarIthawi left a comment • edited Loading

Choose a reason for hiding this comment

shadinaif commented Oct 10, 2023

OmarIthawi left a comment

Choose a reason for hiding this comment

brian-smith-tcril commented Oct 10, 2023

brian-smith-tcril commented Oct 10, 2023

OmarIthawi commented Oct 10, 2023

brian-smith-tcril commented Oct 18, 2023

shadinaif commented Oct 18, 2023

brian-smith-tcril left a comment

Choose a reason for hiding this comment

brian-smith-tcril Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

openedx-webhooks commented Oct 18, 2023

shadinaif commented May 27, 2023 •

edited

Loading

openedx-webhooks commented May 27, 2023 •

edited

Loading

OmarIthawi left a comment •

edited

Loading

brian-smith-tcril Oct 18, 2023 •

edited

Loading