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.

Sign up for GitHub

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

Jump to bottom

feat: sync from old transifex project #1191

Merged
merged 1 commit into from
Sep 21, 2023
Merged

feat: sync from old transifex project #1191

merged 1 commit into from
Sep 21, 2023

Conversation

shadinaif
Copy link
Contributor

@shadinaif shadinaif commented Sep 18, 2023 •
edited
Loading

This is an on-demand GitHub Actions workflow which will sync traslations and their status from the open-edx/edx-platform Transifex project (old) into open-edx/openedx-translations OEP-58 project (new).

This is to address openedx/edx-platform#32994 (comment) on preserving the translated/reviewed status when migrating to the new project.

  • Merge chore: rename transifex-resources.ini file and upgrade deps #570 first, then merge this PR
  • Run make TX_LANGUAGES=ar SYNC_ARGS='--dry-run' sync_translations_github_workflow to ensure resource slugs are correct
  • Ensure that plural message translations are synced from the old project
  • Test on one resource (I chose credentials)
  • Enable the rest of the resources

This PR replaces #569

@openedx-webhooks
Copy link

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.

@openedx-webhooks openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Sep 18, 2023
@shadinaif shadinaif force-pushed the shadinaif/sync-translations branch from 4d369b8 to 567c216 Compare September 18, 2023 08:50
@OmarIthawi OmarIthawi mentioned this pull request Sep 18, 2023
Closed
5 tasks
OmarIthawi
OmarIthawi reviewed Sep 18, 2023
View reviewed changes
Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks @shadinaif I've added few notes. Please check them and let me know when it's ready for another review.

.github/workflows/sync-translations.yml Outdated Show resolved Hide resolved
Makefile Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
requirements/sync.in Outdated Show resolved Hide resolved
scripts/sync_translations.py Show resolved Hide resolved
scripts/sync_translations.py Outdated Show resolved Hide resolved
scripts/sync_translations.py Outdated Show resolved Hide resolved
.github/workflows/sync-translations.yml Outdated Show resolved Hide resolved
.github/workflows/sync-translations.yml Show resolved Hide resolved
.github/workflows/sync-translations.yml Outdated Show resolved Hide resolved
@shadinaif shadinaif force-pushed the shadinaif/sync-translations branch 2 times, most recently from 2774bca to 1e41160 Compare September 18, 2023 11:36
@OmarIthawi OmarIthawi self-requested a review September 18, 2023 15:17
@shadinaif shadinaif force-pushed the shadinaif/sync-translations branch from 1e41160 to 9ef9f27 Compare September 19, 2023 05:21
OmarIthawi
OmarIthawi previously approved these changes Sep 19, 2023
View reviewed changes
Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks @shadinaif!

This looks great now.

@brian-smith-tcril, would you mind taking a look before we merge this pull request?

brian-smith-tcril
brian-smith-tcril reviewed Sep 19, 2023
View reviewed changes
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 building this out! This will be huge for getting OEP-58 out the door!

I left a couple little comments in there, nothing too big.

Makefile Show resolved Hide resolved
scripts/sync_translations.py Show resolved Hide resolved
.github/workflows/sync-translations.yml Outdated Show resolved Hide resolved
.github/workflows/sync-translations.yml Outdated Show resolved Hide resolved
@shadinaif shadinaif force-pushed the shadinaif/sync-translations branch 2 times, most recently from 8864d6d to e64f0cc Compare September 20, 2023 12:56
@shadinaif
Copy link
Contributor Author

shadinaif commented Sep 20, 2023 •
edited
Loading

Thank you @brian-smith-tcril

it would probably be good to add a comment directly in the file documenting that

comment added to Makefile

I think we should at least add a basic test or two, even if we don't get full coverage in this PR .....

two basic tests added

cc: @OmarIthawi

@shadinaif shadinaif force-pushed the shadinaif/sync-translations branch from e64f0cc to 5da4f21 Compare September 20, 2023 12:59
OmarIthawi
OmarIthawi reviewed Sep 20, 2023
View reviewed changes
Copy link
Member

@OmarIthawi OmarIthawi left a comment

Choose a reason for hiding this comment

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

Thanks @shadinaif for adding tests.

I've added some suggestions to make tests more understandable.

scripts/tests/test_sync_translations.py Outdated Show resolved Hide resolved
scripts/tests/test_sync_translations.py Outdated Show resolved Hide resolved
scripts/tests/test_sync_translations.py Outdated Show resolved Hide resolved
scripts/tests/test_sync_translations.py Outdated Show resolved Hide resolved
@OmarIthawi @shadinaif
feat: sync from old transifex project
6bfd6e0 
This is a on-demand GitHub Actions workflow which will sync traslations
and their status from the `open-edx/edx-platform` Transifex project
(old) into `open-edx/openedx-translations` OEP-58 project (new).

Refs: FC-0012 OEP-58
@shadinaif shadinaif force-pushed the shadinaif/sync-translations branch from 5da4f21 to 6bfd6e0 Compare September 21, 2023 06:31
@shadinaif
Copy link
Contributor Author

I've added some suggestions to make tests more understandable.

Thank you @OmarIthawi , done

@OmarIthawi OmarIthawi merged commit 3829e58 into openedx:main Sep 21, 2023
4 checks passed
@openedx-webhooks
Copy link

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

@OmarIthawi
Copy link
Member

Thanks @shadinaif!! Merged :)

@shadinaif shadinaif deleted the shadinaif/sync-translations branch September 21, 2023 08:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OmarIthawi OmarIthawi Awaiting requested review from OmarIthawi

@brian-smith-tcril brian-smith-tcril Awaiting requested review from brian-smith-tcril

Assignees
No one assigned
Labels
open-source-contribution PR author is not from Axim or 2U
Projects
Contributions
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shadinaif @openedx-webhooks @OmarIthawi @brian-smith-tcril