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: use XBlockI18NService js translations | FC-0012 #365

Merged
merged 1 commit into from
Jan 15, 2024
Merged

feat: use XBlockI18NService js translations | FC-0012 #365

merged 1 commit into from
Jan 15, 2024

Conversation

OmarIthawi
Copy link
Member

@OmarIthawi OmarIthawi commented Nov 12, 2023 •
edited
Loading

Implement OEP-58 JavaScript translations

Testing

Tested with results posted openedx/edx-platform#33698 (comment)

Reference

This pull request is part of the FC-0012 project which implements the Translation Infrastructure update OEP-58.

@openedx-webhooks
Copy link

openedx-webhooks commented Nov 12, 2023 •
edited
Loading

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.

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

This is currently a draft pull request. When it is ready for our review and all tests are green, click "Ready for Review", or remove "WIP" from the title, as appropriate.

@openedx-webhooks openedx-webhooks added open-source-contribution PR author is not from Axim or 2U labels Nov 12, 2023
@itsjeyd
Copy link
Contributor

itsjeyd commented Nov 17, 2023

Thanks for the PR @OmarIthawi!

Let us know when it's ready for review.

@itsjeyd itsjeyd added the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Nov 17, 2023
@itsjeyd itsjeyd added the core contributor PR author is a Core Contributor (who may or may not have write access to this repo). label Nov 23, 2023
@itsjeyd itsjeyd removed the waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. label Dec 14, 2023
@itsjeyd
Copy link
Contributor

itsjeyd commented Dec 14, 2023

Hey @OmarIthawi, any updates on when you'll be able to come back to this PR?

@OmarIthawi OmarIthawi requested a review from itsjeyd December 22, 2023 20:31
@OmarIthawi
Copy link
Member Author

OmarIthawi commented Dec 22, 2023 •
edited
Loading

@itsjeyd this is ready and tested. Please let me know if this warrants test cases or not. It'll need substantial mocking, but probably won't be practically useful.

Sorry for the radio silence, I've been focusing on finishing the edx-platform work for this to be useful.

@OmarIthawi OmarIthawi marked this pull request as ready for review December 22, 2023 20:35
@OmarIthawi OmarIthawi mentioned this pull request Dec 22, 2023
Merged
@OmarIthawi OmarIthawi requested a review from Agrendalath January 1, 2024 15:50
@OmarIthawi OmarIthawi marked this pull request as draft January 1, 2024 16:32
@OmarIthawi OmarIthawi force-pushed the atlas-translations branch 4 times, most recently from 3a499d9 to bef499a Compare January 3, 2024 19:35
@OmarIthawi OmarIthawi marked this pull request as ready for review January 3, 2024 19:36
@itsjeyd
Copy link
Contributor

itsjeyd commented Jan 11, 2024

@OmarIthawi Thanks for the update, and no worries!

I'll let @Agrendalath answer your question about whether this change warrants test cases or not. He's your technical reviewer here and can provide final approval for this PR.

There is no need for me to have a look the changes; feel free to cancel the request for review that you sent earlier.

@itsjeyd itsjeyd added the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jan 11, 2024
@OmarIthawi OmarIthawi removed the request for review from itsjeyd January 12, 2024 11:45
@OmarIthawi
Copy link
Member Author

Thanks @itsjeyd. @Agrendalath this is ready for merge as far as our testing tells us.

Also, the existing test cases already covers the new code and failed a couple of times when there was an error.

Agrendalath
Agrendalath reviewed Jan 12, 2024
View reviewed changes
Copy link
Member

@Agrendalath Agrendalath left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@OmarIthawi, this looks good to me. Additionally to the two minor comments, please:

  1. Drop ad6ad2b (I already merged the version update in Python Requirements Update #376).
  2. Bump the minor version.
  3. Add a CHANGELOG entry.

drag_and_drop_v2/drag_and_drop_v2.py Outdated Show resolved Hide resolved
drag_and_drop_v2/drag_and_drop_v2.py Outdated Show resolved Hide resolved
@OmarIthawi OmarIthawi force-pushed the atlas-translations branch 2 times, most recently from 69036b9 to 6309325 Compare January 12, 2024 14:26
@OmarIthawi OmarIthawi marked this pull request as draft January 12, 2024 14:26
@OmarIthawi
Copy link
Member Author

Thanks @Agrendalath. Let me test the pull request again and I'll make it ready for review once manual tests are okay. A little change here and there is prune to make a bug somewhere.

@OmarIthawi OmarIthawi force-pushed the atlas-translations branch 2 times, most recently from 87cbb98 to 0d3b858 Compare January 12, 2024 14:33
@OmarIthawi OmarIthawi marked this pull request as ready for review January 12, 2024 14:33
@OmarIthawi
Copy link
Member Author

Tested it again and it looks good.

@OmarIthawi
Copy link
Member Author

@Agrendalath please let me know if anything is needed. Otherwise, I believe this is ready for merge.

@Agrendalath
Copy link
Member

@OmarIthawi, as I mentioned here, we should bump the minor version, not patch. We're technically adding a new functionality, not fixing a bug.

@OmarIthawi
Copy link
Member Author

@Agrendalath Good catch. You're right, I've got it mixed up. Please check now.

Agrendalath
Agrendalath approved these changes Jan 15, 2024
View reviewed changes
Copy link
Member

@Agrendalath Agrendalath left a comment

Choose a reason for hiding this comment

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

👍

  • I tested this: checked that this XBlock uses the get_javascript_i18n_catalog_url method by default and the old approach as a fallback; the new functionality was already tested in the edx-platform PR
  • I read through the code
  • I checked for accessibility issues: n/a
  • Includes documentation: n/a

@Agrendalath Agrendalath merged commit ad28251 into openedx:master Jan 15, 2024
7 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.

1 similar comment
@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.

@OmarIthawi
Copy link
Member Author

Thanks a lot @Agrendalath!!

@OmarIthawi OmarIthawi mentioned this pull request Jan 22, 2024
Closed
1 task
@itsjeyd itsjeyd removed the waiting for eng review PR is ready for review. Review and merge it, or suggest changes. label Jan 25, 2024
@OmarIthawi OmarIthawi mentioned this pull request Feb 1, 2024
Closed
This was referenced Feb 28, 2024
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Agrendalath Agrendalath Agrendalath approved these changes

Assignees
No one assigned
Labels
core contributor PR author is a Core Contributor (who may or may not have write access to this repo). 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
@OmarIthawi @openedx-webhooks @itsjeyd @Agrendalath