Skip to content
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

doc: design for atlas pull for the edx-platform and its plugins FC-0012 #33166

Merged
merged 1 commit into from
Oct 25, 2023

Conversation

OmarIthawi
Copy link
Member

@OmarIthawi OmarIthawi commented Sep 4, 2023

Atlas integration proposal

This is a design document which include a couple of decisions related to the XBlockI18nService and the way translations are going to be refactored using the openedx-atlas new pull command.

Please review the decision document in this pull request by checking the file tab or using the updated preview from the branch:

TODO

Deadline

The deadline for this design is September 22nd due in order to meet the overall deadline for the FC-0012 project.

References

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

Up-to-date project overview and details are available in the Approach Memo and Technical Discovery: Translations Infrastructure Implementation document.

Join the conversation on Open edX Slack #translations-project-fc-0012.

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

Internalization is being re-architected 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, plugins and the edX Platform:

  • The standard XBlock translations 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.
  • A feature flag will be created to use both atlas and non-atlas method of pulling translations in the edX Platform.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 4, 2023
@openedx-webhooks
Copy link

openedx-webhooks commented Sep 4, 2023

Thanks for the pull request, @OmarIthawi! 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.

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

Thank you so much for putting this together! I did a big review pass on the documentation. Overall it's looking really great! All of my suggestions are just suggestions. Hopefully some of them will make this clearer for people unfamiliar to OEP-58!

docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
Comment on lines 21 to 22
- Connect the `openedx-translations repo`_ to the
`openedx-translations project`_
Copy link
Contributor

Choose a reason for hiding this comment

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

It's not clear from this what the openedx-translations project is/that it is on Transifex.

Copy link
Member Author

Choose a reason for hiding this comment

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

done

to openedx Organization
- Connect the `openedx-translations repo`_ to the
`openedx-translations project`_
- Copy Transifex's Translation Memory and Combine Translators
Copy link
Contributor

Choose a reason for hiding this comment

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

Couple things here:

  • It's probably worth linking to Transifex docs for people unfamiliar with these terms
  • It's not clear where things are being copied from and where they are being copied to

Copy link
Member Author

Choose a reason for hiding this comment

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

done

docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
@brian-smith-tcril
Copy link
Contributor

@OmarIthawi it looks like this has a couple of pycodestyle errors https://github.com/openedx/edx-platform/actions/runs/6171614902/job/16750213428?pr=33166 and commits that don't pass the linter. I think it should be good to go once those get cleaned up and all the checks pass.

@OmarIthawi
Copy link
Member Author

@OmarIthawi it looks like this has a couple of pycodestyle errors https://github.com/openedx/edx-platform/actions/runs/6171614902/job/16750213428?pr=33166 and commits that don't pass the linter. I think it should be good to go once those get cleaned up and all the checks pass.

I've fixed the commit lint PRs and moved all the code into another PR:

This should be ready for review.

@brian-smith-tcril
Copy link
Contributor

@OmarIthawi commitlint still failed, looks like you used doc instead of docs

Copy link
Contributor

@brian-smith-tcril brian-smith-tcril left a comment

Choose a reason for hiding this comment

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

Thank you so much for getting this all written up! I left a couple small grammar nits/wording suggestions, but assuming those are addressed this LGTM!

docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
docs/decisions/0019-oep-58-atlas-translations-design.rst Outdated Show resolved Hide resolved
@OmarIthawi OmarIthawi force-pushed the atlas-proposal branch 5 times, most recently from b5a6dcc to 2eef4e9 Compare September 25, 2023 19:46
Status
======

Accepted
Copy link
Member Author

Choose a reason for hiding this comment

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

right?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe so, yes.

@OmarIthawi OmarIthawi force-pushed the atlas-proposal branch 2 times, most recently from 6d5661d to 655f43d Compare September 28, 2023 07:43
@OmarIthawi OmarIthawi changed the title doc: design for atlas pull for the edx-platform and its plugins doc: design for atlas pull for the edx-platform and its plugins FC-0012 Oct 13, 2023
@OmarIthawi OmarIthawi requested a review from jmbowman October 13, 2023 06:56
@OmarIthawi
Copy link
Member Author

@jmbowman would you mind checking this design decision?

cc: @brian-smith-tcril

Copy link
Contributor

@dianakhuang dianakhuang left a comment

Choose a reason for hiding this comment

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

This looks reasonable to me, and I'd like to see it go ahead so we can iron out the specifics. I think the only thing I'm concerned about is how operators (like edx.org) are supposed to handle plugins that are not installed by default, but maybe that will become clearer as it's implemented.

@OmarIthawi
Copy link
Member Author

This looks reasonable to me, and I'd like to see it go ahead so we can iron out the specifics.

Thanks @dianakhuang.

I think the only thing I'm concerned about is how operators (like edx.org) are supposed to handle plugins that are not installed by default, but maybe that will become clearer as it's implemented.

The design is mostly concerned about the plugins which aren't installed by default. Embdedded plugins like the Discussion XBlock/XModule and the video XBlock has been and will be treated similar to native Open edX code like courseware and student modules.

Most of the design here is focused on both external plugins such as Drag and Drop XBlock, LTI Consumer XBlock, Scorm XBlock. The design is intended to be indfifferent to the method of installation such as:

  • base.in
  • github.in
  • private.txt
  • manual pip install ...

I'm sure there's a blind spot somewhere, therefore backward compatibility will be maintained to avoid imposing a sudden change on operators.

@rgraber
Copy link
Contributor

rgraber commented Oct 20, 2023

Do you need someone from our team to merge this?

@OmarIthawi
Copy link
Member Author

Do you need someone from our team to merge this?

That would be great @rgraber! I don't have merge access to this repository.

@OmarIthawi
Copy link
Member Author

Do you need someone from our team to merge this?

That would be great @rgraber! I don't have merge access to this repository.

@brian-smith-tcril if this is mergeable, would you mind merging it?

@brian-smith-tcril brian-smith-tcril merged commit 086834a into openedx:master Oct 25, 2023
63 checks passed
@openedx-webhooks
Copy link

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

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

2U Release Notice: This PR has been deployed to the edX production environment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants