feat: add atlas pull for the communications app FC-0012 #149

OmarIthawi · 2023-09-29T09:25:04Z

Adds atlas pull to the Communications MFE. Support for other MFEs will come.

Previously open issues

Here's a couple issues that needs to be tackled:

Issue: How to enable the atlas_pull conditionally?
- Old solution: CORE_MFE_APPS feature.
- Updated solution: Hard-coded if app_name == communications
Issue: How to install openedx-atlas and platform-frontend which are required for atlas pull?
- Solution: In the package.json of the communications MFE.
Issue: Should we use make pull translations? or hand-craft an atlas pull command?
- For discovery Regis recommended to avoid using make and writing an atlas command instead.
- For MFEs, it's better to use the Makefile step to allow each frontend to control its own plugins.
Issue: After changing directory to cd src/i18n/messages the node_modules/.bin is no longer available.
- Suggested solution: Use ENV PATH /openedx/app/node_modules/.bin:${PATH} in base instead of the relative PATH.
Issue: atlas pull is only available for master because feat: use atlas in make pull_translations openedx/frontend-app-communications#124 is a post-Palm pull request.
- Patch has been done done on palm.master: feat: use atlas in make pull_translations on palm FC-0012 openedx/frontend-app-communications#156

TODO before merge

Test the translations and post a screenshot
Screenshot: Je parle français!
Backport the atlas pull to the Palm: feat: use atlas in make pull_translations on palm FC-0012 openedx/frontend-app-communications#156
Wait for Quince support and merge into Quince: feat: upgrade to quince #156
Verify that atlas pull and intl-import.js are both working as expected:

Debug atlas pull and the generated src/i18n/index.js

RUN ls -R /openedx/app/src/i18n/; cat /openedx/app/src/i18n/index.js; exit 1         0.4s
------                                                                                                                        
 > [communications-i18n 4/8] RUN ls -R /openedx/app/src/i18n/; cat /openedx/app/src/i18n/index.js; exit 1:                    
#0 0.385 /openedx/app/src/i18n/:                                                                                              
#0 0.385 index.js                                                                                                             
#0 0.385 messages                                                                                                             
#0 0.385                                                                                                                      
#0 0.385 /openedx/app/src/i18n/messages:
#0 0.385 frontend-app-communications
#0 0.385 frontend-component-footer
#0 0.385 frontend-component-header
#0 0.385 paragon
#0 0.385 
#0 0.385 /openedx/app/src/i18n/messages/frontend-app-communications:
#0 0.385 fr_CA.json
#0 0.385 index.js
#0 0.385 
#0 0.385 /openedx/app/src/i18n/messages/frontend-component-footer:
#0 0.385 ar.json
#0 0.385 de.json
#0 0.385 fr_CA.json
#0 0.385 index.js
#0 0.385 
#0 0.385 /openedx/app/src/i18n/messages/frontend-component-header:
#0 0.385 ar.json
#0 0.385 de.json
#0 0.385 fr_CA.json
#0 0.385 index.js
#0 0.385 
#0 0.385 /openedx/app/src/i18n/messages/paragon:
#0 0.385 ar.json
#0 0.385 de.json
#0 0.385 fr_CA.json
#0 0.385 index.js
#0 0.385 // This file is generated by the openedx/frontend-platform's "intl-import.js" script.
#0 0.385 //
#0 0.385 // Refer to the i18n documents in https://docs.openedx.org/en/latest/developers/references/i18n.html to update
#0 0.385 // the file and use the Micro-frontend i18n pattern in new repositories.
#0 0.385 //
#0 0.385 
#0 0.385 import messagesFromFrontendComponentHeader from './messages/frontend-component-header';
#0 0.385 import messagesFromFrontendComponentFooter from './messages/frontend-component-footer';
#0 0.385 import messagesFromParagon from './messages/paragon';
#0 0.385 import messagesFromFrontendAppCommunications from './messages/frontend-app-communications';
#0 0.385 
#0 0.385 export default [
#0 0.385   messagesFromFrontendComponentHeader,
#0 0.385   messagesFromFrontendComponentFooter,
#0 0.385   messagesFromParagon,
#0 0.385   messagesFromFrontendAppCommunications,
#0 0.385 ];
------

Nightly vs Master

All atlas integration is going to target nightly branches so it's included in Quince but not Palm.

The exception is for the Communications MFE because it has not translations and needs atlas in Palm, therefore it's being used on master.

References

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

tutormfe/plugin.py

OmarIthawi

I'm adding notes to myself from my call with Adolfo.

OmarIthawi · 2023-10-17T14:06:47Z

tutormfe/templates/mfe/build/mfe/Dockerfile

@@ -17,7 +17,7 @@ RUN apt update \

 RUN mkdir -p /openedx/app /openedx/env
 WORKDIR /openedx/app
-ENV PATH ./node_modules/.bin:${PATH}
+ENV PATH /openedx/app/node_modules/.bin:${PATH}


make pull_translations performs a cd therefore the relative directory PATH won't work.

hopefully it doesn't break anything else 😅

tutormfe/templates/mfe/build/mfe/Dockerfile

tutormfe/plugin.py

OmarIthawi · 2023-10-18T07:25:20Z

@regisb @arbrandes I've tested this pull request which requires the following plugin:

from tutormfe.hooks import MFE_APPS

@MFE_APPS.add()
def _use_comm_mfe_zeitlabs_fork(mfes):
    mfes["communications"] = {
        "repository": "https://github.com/Zeit-Labs/frontend-app-communications",
        "refs": "https://api.github.com/repos/Zeit-Labs/frontend-app-communications/git/refs/heads",
        "version": "atlas-palm",
        "port": 1984,
    }
    return mfes

So it pulls the right repository until the palm.master pull request is merged.

Please review it and let me know what do you think.

OmarIthawi · 2023-10-28T08:09:45Z

@arbrandes would you mind giving this pull request one more look? Thanks!!

cc: @regisb

regisb · 2023-10-30T08:56:34Z

tutormfe/templates/mfe/build/mfe/Dockerfile

+# Whenever a new MFE supports Atlas, it should be added to this list.
+# When all MFEs support Atlas, this if-statement should be removed.
+{% if app_name in ["communications"] %}
+RUN make OPENEDX_ATLAS_PULL=true pull_translations


Are you familiar with the i18-merge.js script from line 53? It's a script that allows anyone to override translation strings. If I understand correctly, running pull_translations here would override this behaviour, thus preventing users from adding their custom translations. Can you confirm? Can we run Atlas in a way that is compatible with this existing feature?

Also, this change is going to break the build for all users who do not run your fork of the communications app, right? We should definitely avoid this, one way or another.

Are you familiar with the i18-merge.js script from line 53? It's a script that allows anyone to override translation strings. If I understand correctly, running pull_translations here would override this behaviour, thus preventing users from adding their custom translations. Can you confirm? Can we run Atlas in a way that is compatible with this existing feature?

I'm familiar with the script and I should have been clearer in the pull request description that we're going to intentionally not support i18n-merge.js and atlas pull at the same time in this iteration. This will affect the communications MFE which has no translations at the moment so there's no step backward.

Once this PR is merged and used, we'll work on supporting the i18n-merge.js if needed. Our understanding that Atlas provides a superior workflow which makes i18n-merge.js not needed -- at least as we understand it.

This has been confirmed with @arbrandes -- tagging him just in case I missed anything.

Is what I'm mentioning a blocker to merging this PR?

Also, this change is going to break the build for all users who do not run your fork of the communications app, right? We should definitely avoid this, one way or another.

No, of course not. The fork is being used merely for reviewing this pull request, we are fixing this on upstream and it's pending merge:

Palm: feat: use atlas in make pull_translations on palm FC-0012 openedx/frontend-app-communications#156

Quince: Pull request will be created.

@regisb @arbrandes The openedx/frontend-app-communications#156 pull request has been merged into palm.master. We need the following updates to make Tutor MFE support Atlas without needing a fork:

Tag open-release/palm.4 on the communications MFE.

Update Tutor open edx version open-release/palm.2 --> open-release/palm.4

This is needed to fix the i18n for the communications app on Palm.

Once this is merged, it will also cause Quince to be supported. The good news that Atlas support already exists in the quince branch: https://github.com/openedx/frontend-app-communications/blob/3e7ea6a3e5d5c35bf508fc7fab067fc5be836e75/Makefile#L52-L62

Until the tags are ready, we'll need the following plugin to make atlas work:

from tutormfe.hooks import MFE_APPS @MFE_APPS.add() def _use_comm_mfe_zeitlabs_fork(mfes): mfes["communications"]["version"] = "open-release/palm.master" mfes["communications"]["refs"] = "https://api.github.com/repos/openedx/frontend-app-communications/git/refs/heads" return mfes

This will affect the communications MFE which has no translations at the moment so there's no step backward.

Well, it will be a breaking change for users who implemented their own translation strings for the communications MFE. It's a niche use case though, so maybe we can just ignore it.

I agree that Atlas should be a much better solution than that weird i18s-merge.js script. Still, we need to provide users a way to customise translation strings. How are we going to do that in the tutor-mfe plugin?

Tag open-release/palm.4 on the communications MFE.

Publishing a new release tag is a big deal. Do you really need Atlas in Palm? If yes we can ask the BTR to publish a new tag. But maybe it would be sufficient to support Atlas in Quince? You tell me.

Well, it will be a breaking change for users who implemented their own translation strings for the communications MFE. It's a niche use case though, so maybe we can just ignore it.
I agree that Atlas should be a much better solution than that weird i18s-merge.js script. Still, we need to provide users a way to customise translation strings. How are we going to do that in the tutor-mfe plugin?

For the communications MFE i18n-merge.js is mostly unused due to completely missing translations. Based on the feedback we've had, no one translated it except for Pierre Mailhot which doesn't use the i18n-merge. For other MFEs, yes it makes sense to add this support.

We've discussed this issue in call with Adolfo and we decided that this iteration won't support i18n merge.

Please let me know if that's still a blocker.

Publishing a new release tag is a big deal. Do you really need Atlas in Palm? If yes we can ask the BTR to publish a new tag. But maybe it would be sufficient to support Atlas in Quince? You tell me.

It's a big deal indeed. Let's refocus our efforts into Quince. I'll check with @sambapete on the idea of skipping Palm for Tutor MFE support, which would leave the Palm users no way to support Atlas except by cherry-picking this pull request. Which isn't pretty bad as long as we land it into Quince.

That's the keyword "as long as we land it into Quince". Unfortunately, translators translate strings before the cut of Quince.master that took place a month ago. They were never able to translate strings for the communications MFE because it wasn't part any section in the edx-platform project. I only translated it manually for Palm by extracting the strings from the source code because I couldn't leave it untranslated for my users.

Thanks @sambapete, Quince it is! We're close to do so -- hopefully :)

+1 for Quince.

We can still do Palm.4, but it should come after getting Quince up and running.

OmarIthawi · 2023-11-09T18:30:57Z

Status: This is pending the Quince upgrade #156 so I'm presuming that this PR is blocked until the following PR is merged:

feat: upgrade to quince #156

cc: @regisb @sambapete @arbrandes

regisb · 2023-11-14T14:05:11Z

Any change that would added to nightly now would also go to Quince. Do you want to merge this now in nightly?

arbrandes · 2023-11-14T16:24:26Z

+1 for getting this into nightly.

OmarIthawi · 2023-11-15T08:47:02Z

Nightly it is! I'll rebase and get back to you!

OmarIthawi · 2023-11-15T20:32:15Z

@regisb @arbrandes @brian-smith-tcril I've tested this plugin and it worked well without plugins or settings.

The only remaining pull request has been merged into master:

feat!: remove broken transifex and use atlas exclusively | FC-0012 openedx/frontend-app-communications#164 (comment)

Please let me know if there's anything blocking this PR from getting merged.

arbrandes

Works for me!

arbrandes · 2023-11-17T15:36:06Z

@OmarIthawi, we're missing openedx-atlas in frontend-app-communications as it stands in Quince. It was introduced in master via openedx/frontend-app-communications#164. I created a backport: openedx/frontend-app-communications#176. Objections?

arbrandes self-requested a review September 29, 2023 13:55

This was referenced Sep 29, 2023

Adolfo's reviews openedx/axim-engineering#696

Open

[frontend-app-communications] Localization issues openedx/frontend-app-communications#148

Closed

regisb assigned OmarIthawi Oct 2, 2023

This comment was marked as resolved.

Sign in to view

OmarIthawi mentioned this pull request Oct 10, 2023

feat: use atlas in make pull_translations on palm FC-0012 openedx/frontend-app-communications#156

Merged

1 task

OmarIthawi force-pushed the atlas branch from 8ab7148 to ce0ee3f Compare October 12, 2023 12:03

OmarIthawi changed the title ~~feat: add atlas pull for the communications app~~ feat: add atlas pull for the communications app FC-0012 Oct 13, 2023

regisb reviewed Oct 16, 2023

View reviewed changes

tutormfe/plugin.py Outdated Show resolved Hide resolved

OmarIthawi force-pushed the atlas branch from 0d47b04 to 2ce3474 Compare October 16, 2023 16:28

OmarIthawi commented Oct 18, 2023

View reviewed changes

OmarIthawi force-pushed the atlas branch from 2ce3474 to 53558b1 Compare October 18, 2023 07:23

OmarIthawi marked this pull request as ready for review October 18, 2023 07:23

regisb reviewed Oct 30, 2023

View reviewed changes

OmarIthawi requested a review from regisb November 4, 2023 06:39

OmarIthawi marked this pull request as draft November 4, 2023 14:14

feat: add atlas pull for the communications app

8d7dbca

OmarIthawi changed the base branch from master to nightly November 15, 2023 16:03

OmarIthawi force-pushed the atlas branch from 53558b1 to 8d7dbca Compare November 15, 2023 16:03

OmarIthawi marked this pull request as ready for review November 15, 2023 20:28

This comment was marked as duplicate.

Sign in to view

arbrandes approved these changes Nov 16, 2023

View reviewed changes

regisb merged commit 0202379 into overhangio:nightly Nov 16, 2023

arbrandes mentioned this pull request Nov 17, 2023

[Quince backport] feat!: remove broken transifex and use atlas exclusively openedx/frontend-app-communications#176

Merged

brian-smith-tcril mentioned this pull request Mar 22, 2024

fix: re-pull Atlas translations for mounted edx-platform overhangio/tutor#1024

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add atlas pull for the communications app FC-0012 #149

feat: add atlas pull for the communications app FC-0012 #149

OmarIthawi commented Sep 29, 2023 •

edited

This comment was marked as resolved.

OmarIthawi left a comment

OmarIthawi Oct 17, 2023

OmarIthawi commented Oct 18, 2023

OmarIthawi commented Oct 28, 2023 •

edited

regisb Oct 30, 2023 •

edited

OmarIthawi Nov 2, 2023

OmarIthawi Nov 4, 2023 •

edited

regisb Nov 7, 2023

OmarIthawi Nov 8, 2023

sambapete Nov 8, 2023

OmarIthawi Nov 8, 2023

arbrandes Nov 14, 2023

OmarIthawi commented Nov 9, 2023

regisb commented Nov 14, 2023

arbrandes commented Nov 14, 2023

OmarIthawi commented Nov 15, 2023 •

edited

This comment was marked as duplicate.

OmarIthawi commented Nov 15, 2023

arbrandes left a comment

arbrandes commented Nov 17, 2023

feat: add atlas pull for the communications app FC-0012 #149

feat: add atlas pull for the communications app FC-0012 #149

Conversation

OmarIthawi commented Sep 29, 2023 • edited

Previously open issues

TODO before merge

Nightly vs Master

References

This comment was marked as resolved.

OmarIthawi left a comment

Choose a reason for hiding this comment

OmarIthawi Oct 17, 2023

Choose a reason for hiding this comment

OmarIthawi commented Oct 18, 2023

OmarIthawi commented Oct 28, 2023 • edited

regisb Oct 30, 2023 • edited

Choose a reason for hiding this comment

OmarIthawi Nov 2, 2023

Choose a reason for hiding this comment

OmarIthawi Nov 4, 2023 • edited

Choose a reason for hiding this comment

regisb Nov 7, 2023

Choose a reason for hiding this comment

OmarIthawi Nov 8, 2023

Choose a reason for hiding this comment

sambapete Nov 8, 2023

Choose a reason for hiding this comment

OmarIthawi Nov 8, 2023

Choose a reason for hiding this comment

arbrandes Nov 14, 2023

Choose a reason for hiding this comment

OmarIthawi commented Nov 9, 2023

regisb commented Nov 14, 2023

arbrandes commented Nov 14, 2023

OmarIthawi commented Nov 15, 2023 • edited

This comment was marked as duplicate.

OmarIthawi commented Nov 15, 2023

arbrandes left a comment

Choose a reason for hiding this comment

arbrandes commented Nov 17, 2023

OmarIthawi commented Sep 29, 2023 •

edited

OmarIthawi commented Oct 28, 2023 •

edited

regisb Oct 30, 2023 •

edited

OmarIthawi Nov 4, 2023 •

edited

OmarIthawi commented Nov 15, 2023 •

edited