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

docs: decision to standardize django/djangojs po files FC-0012 #32994

Merged
merged 1 commit into from
Oct 20, 2023

Conversation

OmarIthawi
Copy link
Member

@OmarIthawi OmarIthawi commented Aug 13, 2023

This a decision to make the edx-platform uses django.po and djangojs.po files exclusively without splitting/segmenting the files into multiple Transifex resources.

Preview the document via the link below:

Check the decision document https://github.com/openedx/edx-platform/pull/32994/files and write inline comments, but if you're in a hurry here's the quick trade-off we're trying to make:

Keep as-is Combine everything Combine everything, except for Studio
Number of translation resources 11 2 4
Benefits The edx-platform splits its translations files to studio.po, django-partial.po among 11 files in total to make it possible for translators to focus on learner-facing text and de-prioritize educator-facing text. The idea was that LMS would be translated into more languages than CMS (Studio) would. More standard Django setup. Split into edx-platform.po, edx-platform-js.po, cms.po, cms-js.po
Named releases Needs to be combined into two files Same 2 file setup for both master and named releases. Ideally, same 4 files for both master and named releases
Developer UX More things to worry about Less resources to worry about 4 resources to manage, while translators can focus on LMS

Supporting information

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

Checkout the related pull request:

Deadline

As soon as possible to finish the OEP-58: September 13th, 2023.

@openedx-webhooks
Copy link

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.

@nedbat
Copy link
Contributor

nedbat commented Aug 14, 2023

The original reason for the split was to make it possible for translators to focus on learner-facing text and de-prioritize educator-facing text. Do we no longer need that? The idea was that LMS would be translated into more languages than Studio would.

The decision doc doesn't make any reference to this original goal.

@sambapete
Copy link
Contributor

The original reason for the split was to make it possible for translators to focus on learner-facing text and de-prioritize educator-facing text. Do we no longer need that? The idea was that LMS would be translated into more languages than Studio would.

The decision doc doesn't make any reference to this original goal.

To be honest @nedbat most reviewers probably don't know that or don't know which files are for what purposes in Transifex. Could this be part of a future project to separate the LMS from the CMS (I read somewhere we should not refer to it as Studio anymore) ?

@OmarIthawi
Copy link
Member Author

The original reason for the split was to make it possible for translators to focus on learner-facing text and de-prioritize educator-facing text. Do we no longer need that? The idea was that LMS would be translated into more languages than Studio would.

Thanks @nedbat for adding the specific reason, I tried to capture the context I know.

I'll update the decision with this context.

To be honest @nedbat most reviewers probably don't know that or don't know which files are for what purposes in Transifex. Could this be part of a future project to separate the LMS from the CMS?

I think so as well. Having worked on Open edX translations for a long time I also seem to forget this issue. The best shot for any language team is to translate 100% of the translations.

I'll wait for input from others after updating the context.

@ghassanmas
Copy link
Member

I agree with @sambapete that mostly translators/reviewers mightn't be aware of the difference of files/resources. I peronsally priori to a release would always foucs on learner facing UX, for example, (learning, account, profile, authn..etc MFEs).

Which bring us to the next point I want to mention, as the UX/UI platform is transforming into MFEs of which it's strings are stored in .json files rather than .po, so I would say whatever we end up choosen here, in long term it might not be that relevant.

Lastly, assuming we moved to a new simple process, would the translations/reviews from older resources be saved? This I bet the main thing reviewers/transltaors would be concerned about...

@OmarIthawi
Copy link
Member Author

OmarIthawi commented Aug 16, 2023

Thanks @ghassanmas for your input.

Lastly, assuming we moved to a new simple process, would the translations/reviews from older resources be saved? This I bet the main thing reviewers/transltaors would be concerned about...

I agree. In a best case scenario we'd copy the following:

  • Translation for each entry in every language (easy effort)
  • Reviewed/unreviewed status for every translation (requires more effort)

The same process would be followed regardless of whether we combine resources or not. I need to double check the Transifex API for controlling the Reviewed/Unreviewed status when moving entries between projects.

@OmarIthawi
Copy link
Member Author

OmarIthawi commented Aug 23, 2023

@nedbat I've updated this pull request to add another method of which preserves the Studio vs Platform separation but in 4 files instead of 11.

@nedbat Please let me know if this makes it a more acceptable decision.

@brian-smith-tcril Please let me know if this work should block the OEP-58 edx-platform atlas integration or not? I can move on and implement Option 1 by adding 11 .po files into the new openedx-translations project. While I don't prefer that, I could use your recommendation for timeline in terms of FC-0012 project.

@brian-smith-tcril
Copy link
Contributor

brian-smith-tcril commented Aug 23, 2023

@OmarIthawi I don't think it makes sense to move forward with openedx/openedx-translations#541 while there are still 11 resource files. I think we should decide if we want to go the "combine everything" or "combine everything, except for Studio" route first.

@OmarIthawi
Copy link
Member Author

@nedbat Thanks for the feedback. Would you mind checking the new option which suggests splitting into edx-platform.po, edx-platform-js.po, cms.po and cms-js.po?

@brian-smith-tcril
Copy link
Contributor

Has this been discussed in the translations working group? I generally lean towards less complex solutions (Option 1), so if that's OK with the people doing translation work in transifex I'd prefer we go that route.

@OmarIthawi
Copy link
Member Author

OmarIthawi commented Sep 1, 2023

Has this been discussed in the translations working group?

Not yet. The closest Transifex Working Group meeting will be held on Sept 20th, 2023. I think we should aim for async resolution of this decision.

I generally lean towards less complex solutions (Option 1), so if that's OK with the people doing translation work in transifex I'd prefer we go that route.

The 4-files certainly will require more effort to accomplish. So Option 1 is indeed easier to implement and manage.

@brian-smith-tcril
Copy link
Contributor

I think we should aim for async resolution of this decision.

Works for me! Would you mind starting a thread in the #wg-translations channel?

@OmarIthawi
Copy link
Member Author

Works for me! Would you mind starting a thread in the #wg-translations channel?

Done: https://openedx.slack.com/archives/C037XDB9KN1/p1692577800473309

@sambapete
Copy link
Contributor

I have a small concern as a reviewer in Transifex.

When you will be combining the files in a single django.po file and a single djangojs.po file, will we lose the strings review status and need to review them all over again?

This is what currently happens under the Open edX Releases project when the new releases are created (see release-palm and release-palm-js under fr_CA for example). As a result, I personally never review these files when a new release is created and only review the original po files under the edx-platform project.

I am concerned that the first time the files will be standardized we will have no choice but to review the strings again. Or is there somethings that will keep the review status in the new django.po and djangojs.po files?

@brian-smith-tcril
Copy link
Contributor

I am concerned that the first time the files will be standardized we will have no choice but to review the strings again.

Thank you for raising that concern @sambapete! This shouldn't impact the decision about combining into 2 or 4 files though, as both scenarios involve combining the existing 11 files.

is there somethings that will keep the review status in the new django.po and djangojs.po files?

I believe so. The /resource_translations/{resource_translation_id} transifex API endpoint does allow us to get and set the reviewed status of any given resource translation. It may take a bit of effort to ensure we match up all the strings in the old files with the new files properly, but it is definitely something we should try to solve for.

@OmarIthawi
Copy link
Member Author

I have a small concern as a reviewer in Transifex.

When you will be combining the files in a single django.po file and a single djangojs.po file, will we lose the strings review status and need to review them all over again?

This is what currently happens under the Open edX Releases project when the new releases are created (see release-palm and release-palm-js under fr_CA for example). As a result, I personally never review these files when a new release is created and only review the original po files under the edx-platform project.

I am concerned that the first time the files will be standardized we will have no choice but to review the strings again. Or is there somethings that will keep the review status in the new django.po and djangojs.po files?

I'm working on a solution for that. It's yet to be done but so far I was able to copy review status:

It's promising so far and thanks for your input @sambapete :)

@mphilbrick211
Copy link

Hi @OmarIthawi! Just flagging the new tests that have popped up, per the recent update: https://discuss.openedx.org/t/minor-change-to-edx-platform-check-statuses/11131

@OmarIthawi
Copy link
Member Author

@brian-smith-tcril I think it's now ready for merge. I hope we've left a very good trail for future maintainers of edx-platform to understand the decision and its consequences.

@brian-smith-tcril
Copy link
Contributor

@jristau1984 you're listed as "Review required from" for this repo on the spreadsheet, would you mind taking a look at this one?

@jristau1984
Copy link
Contributor

@jristau1984 you're listed as "Review required from" for this repo on the spreadsheet, would you mind taking a look at this one?

I am listed as a reviewer for edx-platform? Can you please point me to where that is logged, I should update it. I have no more context here than anyone who is involved, my review should not be required.

@OmarIthawi
Copy link
Member Author

@jristau1984 you're listed as "Review required from" for this repo on the spreadsheet, would you mind taking a look at this one?

I am listed as a reviewer for edx-platform? Can you please point me to where that is logged, I should update it. I have no more context here than anyone who is involved, my review should not be required.

I believe that would be @jmbowman. So I'm guessing it's a typo. Sorry @jristau1984!!

@brian-smith-tcril
Copy link
Contributor

So I'm guessing it's a typo

That cell was updated in the most recent edit to the spreadsheet (about 20 minutes after my ping). Glad that spreadsheet is in order now.

@OmarIthawi OmarIthawi changed the title docs: decision to standardize django/djangojs po files docs: decision to standardize django/djangojs po files FC-0012 Oct 13, 2023


This option adds more complexity, but retain the Translator-friendly resource
split.
Copy link
Contributor

Choose a reason for hiding this comment

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

After reading this discussion, I think this rejected alternative section could also have an explanation for why it was rejected (still too much complexity, wanting to simplify as much as possible, etc).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks @dianakhuang. I'll expand this paragraph.

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 @dianakhuang, would you mind taking another look?

@OmarIthawi OmarIthawi force-pushed the pofiles branch 2 times, most recently from 0ad647d to ceb0d8d Compare October 17, 2023 16:52
@rgraber
Copy link
Contributor

rgraber commented Oct 20, 2023

do you need someone from our team to merge this?

@brian-smith-tcril
Copy link
Contributor

@rgraber I can merge it. Thanks for asking!

@brian-smith-tcril brian-smith-tcril merged commit 09a6523 into openedx:master Oct 20, 2023
62 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.

1 similar comment
@edx-pipeline-bot
Copy link
Contributor

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

@OmarIthawi
Copy link
Member Author

OmarIthawi commented Oct 21, 2023

Great news!! This is now merged :)

Thanks everyone!

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.