feat: FC-0012 - add atlas support for cookiecutter-xblock #382

shadinaif · 2023-08-14T11:05:09Z

feat: add atlas support for cookiecutter-xblock

This PR prepares the repository to comply with openedx-translations by doing the following:

General:

Add an optional argument to post_gen_project named symlink_translation to create a symbolic link from ./conf/locale to translation

Only for cookiecutter-xblock:

Use i18_tool extract to extract transactions
Move local translations from locale to conf/locale directory. This is the default used one in openedx-translations
Remove symlink_translations command from Makefile and use the new symlink_translation argument in post-generation hook
Ask the user to fill i18n_namespace instead of filling fixed name by the cookiecutter (default provided of course)

How it was tested?

Create an XBlock using the cookiecutter cookiecutter -o testdir cookiecutter-xblock
go to the new XBlock project directory
Create a virtualenv and upgrade requirements using make upgrade
Install the requirements make requirements
Apply git init && git add -A && git commit -m 'First commit' to pass validation tests
try make extract_translations: everything looks fine
try make validate_translations: no errors

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 notice 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 translation directories are removed from all repos

openedx-webhooks · 2023-08-14T11:05:13Z

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.

cookiecutter-xblock/{{cookiecutter.repo_name}}/Makefile

shadinaif · 2023-08-21T12:26:24Z

...utter-xblock/{{cookiecutter.repo_name}}/{{cookiecutter.package_name}}/conf/locale/en/text.po

@@ -0,0 +1 @@
+django.po


@OmarIthawi we have an issue here. Is it good enough to add the link in the template like this? or should we dig deeper to change the dependency of XBlocks on text domain and change it to django ?

I keep forgetting this @shadinaif, so it seems that you do too :)

In XBlocks we plan to store no po/mo files at all. The LMS will completely manage the translation e.g. edx-platform:conf/plugins-locale/drag-and-drop/conf/locale/django.po.

So this link is not needed and could be harmful.

@shadinaif In case I didn't make myself clear, let's keep text.po and textjs.po` as-is because those files will be irrelevant to all of the OEP-58 work and will eventually be removed from all XBlocks.

@OmarIthawi thank you. What about developers creating new XBlocks? we've removed the part that converts the partial file into text.po. Should we just add a comment to clarify that?

or maybe better: add an option in the cookiecutter creation to ask if the package is to be integrated with atlas, with default No (because most usage would be from the community)

@shadinaif if you must implement choices I recommend the following two options:

Option 1

Support both Atlas OEP-58 translation mechanism and the old way (like most XBlocks have been converted). The default choice.

The consequences here that we need make extract... to support two modes:

make extract_translations that produces text.po and textjs.po and make extract_translations that produces django.po and djangojs.po.

Option 2

Support only Atlas and OEP-58. An optional but acceptable choice.

In my opinion Option 1 should be the only one available. After OEP-58 is finalized, we can move to Option 2 removing Option 1 altogether unless someone really wants to. Having options can be a bad Developer Experience especially when the consequences are not immediately obvious.

In either cases, we should enable an Atlas workflow as a mandatory feature in all new XBlocks.

Thank you @OmarIthawi , the case is confusing! the problem with the opposite is that openedx-tranlsations will fail when it runs git add when django.po is the link. So, looks like we now have three options:

Option1: (the one I like): add an option in the cookiecutter creation to ask if the package is to be integrated with atlas, with default No (because most usage would be from the community). This supports two modes when creating the XBlock. U2 and partner developers who create XBlocks integrated with Atlas should simply be aware of the option

Default (No): for the community, the translation is hosted somewhere else. make extract_translation command will look like this:
cd $(PACKAGE_NAME) && i18n_tool extract --merge-js --combine-partial --text-domain
(it means we add a new option --text-domain to i18n_tools to rename django.po to text.po on extraction)
the extracted file name will be text.po

Support atlas (Yes): for atlas-supported XBlocks. No links are added too because text.po will not be used anyway. make extract_translation command will look like the code we already have here:
cd $(PACKAGE_NAME) && i18n_tool extract --merge-js --combine-partial
the extracted file name will be django.po

Option2: add the link -s text.po django.po and update openedx-translations to deal with it. (complexity added to the script)

Option3: (simple but lazy from our side, we put the burden on others). always add the link -s django.po text.po and let developers remove it manually when they know that the XBlock supports atlas

What do you think?

@shadinaif Atlas is not for 2U although they'll be using Atlas for sure.

OEP-58 is meant to benefit the wider community of Open edX. Therefore, we should make it enabled by default because the in-repo committed pofiles will be deprecated and removed. Therefore an option to enable Atlas doesn't make sense.

Thank you @OmarIthawi , the case is confusing! the problem with the opposite is that openedx-tranlsations will fail when it runs git add when django.po is the link. So, looks like we now have three options:

Yeah, you're right. We're dealing with conflicting requirements here.

Both Option 2 and Option 3 are good.

@shadinaif Atlas is not for 2U although they'll be using Atlas for sure.

this is the main point I was missing! thank you @OmarIthawi

I'll go with Option2 then 👍🏼

@OmarIthawi Update on Option2:
~~add the link -s text.po django.po and~~ update openedx-translations to deal with text.po and textjs.po files when used by XBlocks. No links are added to the cookiecutter

Thanks! @shadinaif please remove the link from this PR.

OmarIthawi · 2023-09-14T18:31:27Z

@shadinaif https://pypi.org/project/edx-i18n-tools/1.2.0/ has been released so this PR is no longer blocked.

shadinaif · 2023-09-15T09:30:04Z

linter is failing when run locally, I'll fix it in a few

shadinaif · 2023-09-18T12:44:39Z

please review @OmarIthawi . See (How it was tested?) section in the PR description

OmarIthawi · 2023-09-19T11:33:31Z

@brian-smith-tcril @e0d would you mind approving this PR workflows to run tests?

OmarIthawi · 2023-09-22T12:02:28Z

cookiecutter-xblock/{{cookiecutter.repo_name}}/manage.py

@@ -13,7 +13,7 @@
 if __name__ == "__main__":
    os.environ.setdefault(
        "DJANGO_SETTINGS_MODULE",
-        "{{cookiecutter.package_name}}.locale.settings"
+        "{{cookiecutter.package_name}}.conf.locale.settings"


@shadinaif Is it mandatory to have the settings.py in locale? Usually the locale dir isn't expected to have any Python code.

My expectation is to find the setting in {{cookiecutter.package_name}}.dev_settings or {{cookiecutter.package_name}}.locale_settings if you must.

I understand that this pattern is already set, but it appears to be incorrect. Not a blocker though.

@shadinaif please take a look at this note.

I used .dev_settings not {{cookiecutter.package_name}}.dev_settings, to place the file next to manage.py (which uses the settings file)

OmarIthawi · 2023-09-30T11:28:00Z

Thanks @shadinaif. The pull request looks great! It looks good to me now.

@itsjeyd @brian-smith-tcril please enable tests and review this pull request.

itsjeyd · 2023-10-10T07:02:51Z

@OmarIthawi Thanks for the ping! Looks like we've got a passing build 🙂

@shadinaif As far as I can tell from the PR description, this PR is part of FC-0012. Would you mind updating the title to reflect that?

@brian-smith-tcril This is ready for final review/merge. Could you please take it from here?

OmarIthawi

It looks good to me. Thanks @shadinaif!

I've tested an earlier version of this PR and it worked. All my notes has been addressed :)

shadinaif · 2023-10-10T07:29:02Z

Thank you @itsjeyd , I changed the title

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

@brian-smith-tcril copying from openedx/RecommenderXBlock#70 .

@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.

brian-smith-tcril · 2023-10-10T15:49:26Z

I think my concern with dev_settings.py comes from creating what feels like a "kitchen sink" settings file. I would lean towards using different files to handle different types of settings instead of having one file for everything. I understand the desire to not paint ourselves into a corner by using locale_setttings as a name, but it does feel the most accurate at the moment.

shadinaif · 2023-10-12T06:45:02Z

Thank you @OmarIthawi and @brian-smith-tcril . I'll vote for test_settings.py for the following reasons:

cookiecutter for django apps use that for both tests and manage.py
We can do the same here and add dummy unit tests as a starting point like the one we find in django-app cookiecutter

But actually, if I should decide that alone; I would name it translation_settings.py or keep it inside conf.locale as settings.py and use it explicitly with the manage command:

DJANGO_SETTINGS_MODULE=translation_settings python manage.py compilejsi18n --namespace MyXblockI18n --output $(JS_TARGET)

It is confusing in all cases in my opinion. What do you think?

brian-smith-tcril · 2023-10-12T15:38:34Z

I think I still prefer locale_settings (or translation_settings, no strong feelings between those 2 names).

It seems it should be easy to get manage.py to load both DJANGO_SETTINGS_MODULE and the additional locale/translation settings files (as it was doing before with settings in conf.locale.

I'm not sure what would be required to get the tests to do a similar thing, but I stand by not wanting to combine locale/dev/test settings together into one file.

Update cookiecutter-xblock to support atlas for all new XBlocks Refs: FC-0012 OEP-58

shadinaif · 2023-10-16T05:23:38Z

Thank you @brian-smith-tcril and @OmarIthawi . I changed it to translation_settings

dianakhuang

I think this looks good to me.

brian-smith-tcril

I think this looks good. I left 2 comments with questions about the changes in the json, but nothing about those changes feels particularly problematic.

brian-smith-tcril · 2023-10-16T14:10:33Z

cookiecutter-xblock/cookiecutter.json

  "github_org": "openedx",
-  "repo_name": "{{cookiecutter.package_name}}-xblock",
-  "tag_name": "myxblock",
+  "repo_name": "{{cookiecutter.package_name.replace('_', '-')}}",


this is changing behavior

before this change, the value of repo_name would be myxblock-xblock, now it's my-xblock

is this change intentional?

it is intentional, I see it as more convenient to have the repo name the same as the package name unless the creator decides otherwise. No technical reason or implication for that

Note: the template will not enforce anything. It'll just propose these as defaults, and the XBlock creator can change them on the spot

another thing is that we have a new field i18n_namespace that will look nicer by this change

brian-smith-tcril · 2023-10-16T14:15:04Z

cookiecutter-xblock/cookiecutter.json

-  "repo_name": "{{cookiecutter.package_name}}-xblock",
-  "tag_name": "myxblock",
+  "repo_name": "{{cookiecutter.package_name.replace('_', '-')}}",
+  "tag_name": "{{cookiecutter.package_name|lower}}",


similar to the comment about repo_name - this will force _xblock into tag names (considering there is now an expectation that _xblock will be in the package name).

is this intentional?

brian-smith-tcril · 2023-10-18T21:04:36Z

@jmbowman you're listed as "please inform" on the spreadsheet for this repo. I plan on merging this by EOD tomorrow. Please let me know if you have any objections. Thanks!

dianakhuang · 2023-10-19T13:21:47Z

@brian-smith-tcril I'm basically here as Jeremy's arch-bom representative on these translations/atlas tickets, just for future reference.

openedx-webhooks · 2023-10-19T20:31:52Z

@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 Aug 14, 2023

shadinaif force-pushed the shadinaif/atlas-support branch from 6dea480 to ee8e819 Compare August 15, 2023 07:53

OmarIthawi reviewed Aug 15, 2023

View reviewed changes

cookiecutter-xblock/{{cookiecutter.repo_name}}/Makefile Outdated Show resolved Hide resolved

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Aug 15, 2023

shadinaif force-pushed the shadinaif/atlas-support branch 2 times, most recently from 779c81c to e11f8f6 Compare August 21, 2023 08:34

OmarIthawi reviewed Aug 21, 2023

View reviewed changes

cookiecutter-xblock/{{cookiecutter.repo_name}}/Makefile Outdated Show resolved Hide resolved

shadinaif force-pushed the shadinaif/atlas-support branch from e11f8f6 to a738b8c Compare August 21, 2023 12:24

shadinaif commented Aug 21, 2023

View reviewed changes

itsjeyd added the blocked by other work PR cannot be finished until other work is complete label Aug 22, 2023

shadinaif requested a review from OmarIthawi August 23, 2023 08:25

shadinaif mentioned this pull request Aug 24, 2023

feat: Special support for XBlocks openedx/openedx-translations#593

Merged

1 task

shadinaif changed the title ~~feat: stardandize templates to support atlas~~ feat: standardize templates to support Atlas Sep 15, 2023

shadinaif force-pushed the shadinaif/atlas-support branch 2 times, most recently from a4e7dc8 to 4feaa3f Compare September 15, 2023 08:53

shadinaif marked this pull request as ready for review September 15, 2023 08:54

shadinaif force-pushed the shadinaif/atlas-support branch 2 times, most recently from 8597543 to d55bab4 Compare September 15, 2023 09:06

shadinaif marked this pull request as draft September 15, 2023 09:29

shadinaif force-pushed the shadinaif/atlas-support branch from d55bab4 to 975eb64 Compare September 15, 2023 13:05

shadinaif marked this pull request as ready for review September 18, 2023 12:43

shadinaif force-pushed the shadinaif/atlas-support branch from 975eb64 to 2665a4d Compare September 19, 2023 05:16

itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Sep 21, 2023

OmarIthawi removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Sep 22, 2023

OmarIthawi reviewed Sep 22, 2023

View reviewed changes

itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Sep 26, 2023

shadinaif force-pushed the shadinaif/atlas-support branch from 8df4f21 to 1a702c2 Compare September 30, 2023 10:34

shadinaif requested a review from OmarIthawi September 30, 2023 10:36

itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Oct 10, 2023

itsjeyd added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Oct 10, 2023

itsjeyd requested a review from brian-smith-tcril October 10, 2023 07:04

itsjeyd mentioned this pull request Oct 10, 2023

feat: FC-0012 - add atlas support for cookiecutter-django-app #392

Merged

OmarIthawi approved these changes Oct 10, 2023

View reviewed changes

shadinaif changed the title ~~feat: add atlas support for cookiecutter-xblock~~ feat: FC-0012 - add atlas support for cookiecutter-xblock Oct 10, 2023

brian-smith-tcril mentioned this pull request Oct 10, 2023

feat: Add gettext for JS translation openedx/RecommenderXBlock#70

Merged

1 task

feat: add atlas support for cookiecutter-xblock

043e2e6

Update cookiecutter-xblock to support atlas for all new XBlocks Refs: FC-0012 OEP-58

shadinaif force-pushed the shadinaif/atlas-support branch from 1a702c2 to 043e2e6 Compare October 16, 2023 05:20

dianakhuang approved these changes Oct 18, 2023

View reviewed changes

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

View reviewed changes

brian-smith-tcril merged commit c07da33 into openedx:master Oct 19, 2023
5 checks passed

itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Oct 24, 2023

feat: FC-0012 - add atlas support for cookiecutter-xblock #382

feat: FC-0012 - add atlas support for cookiecutter-xblock #382

Conversation

shadinaif commented Aug 14, 2023 • edited Loading

References

Breaking Changes

openedx-webhooks commented Aug 14, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi Aug 22, 2023 • edited Loading

Choose a reason for hiding this comment

Option 1

Option 2

Choose a reason for hiding this comment

OmarIthawi Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi commented Sep 14, 2023

shadinaif commented Sep 15, 2023

shadinaif commented Sep 18, 2023

OmarIthawi commented Sep 19, 2023

OmarIthawi Sep 22, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

OmarIthawi commented Sep 30, 2023

itsjeyd commented Oct 10, 2023

OmarIthawi left a comment

Choose a reason for hiding this comment

shadinaif commented Oct 10, 2023

OmarIthawi commented Oct 10, 2023 • edited Loading

brian-smith-tcril commented Oct 10, 2023

shadinaif commented Oct 12, 2023

brian-smith-tcril commented Oct 12, 2023

shadinaif commented Oct 16, 2023

dianakhuang left a comment

Choose a reason for hiding this comment

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-smith-tcril commented Oct 18, 2023

dianakhuang commented Oct 19, 2023

openedx-webhooks commented Oct 19, 2023

shadinaif commented Aug 14, 2023 •

edited

Loading

openedx-webhooks commented Aug 14, 2023 •

edited

Loading

OmarIthawi Aug 22, 2023 •

edited

Loading

OmarIthawi Aug 23, 2023 •

edited

Loading

OmarIthawi Sep 22, 2023 •

edited

Loading

OmarIthawi commented Oct 10, 2023 •

edited

Loading