Skip to content
This repository has been archived by the owner on Feb 26, 2024. It is now read-only.

[MCKIN-7023] Improve XBlock i18n by allowing template translations #1032

Merged
merged 1 commit into from
Apr 5, 2018

Conversation

pomegranited
Copy link

@pomegranited pomegranited commented Mar 12, 2018

Uses temporary branches for XBlock, xblock-utils, and DnDv2-new to provide fixes for XBlock template translations.

JIRA tickets: MCKIN-7023

Discussions: WL-230, YONK-879

Dependencies:

Screenshots: See openedx/xblock-image-explorer#37 and openedx/xblock-drag-and-drop-v2#154 for before and after screenshots using dummy translations.

Merge deadline: ASAP

Testing instructions:

See openedx/xblock-image-explorer#37 for test instructions.

Author notes and concerns:

  1. When the dependent PRs merge, we'll submit a follow-up PR to use the newly tagged versions of the dependent libraries.

Reviewers

@nedbat
Copy link

nedbat commented Mar 12, 2018

@pomegranited Am I right in thinking that any PR with "[MCKIN]" in the title isn't an OSPR?

@pomegranited
Copy link
Author

@nedbat Yep totally -- though some people might omit the [], so if you're updating the OSPR trigger, then maybe just look for MCKIN-?

@nedbat
Copy link

nedbat commented Mar 12, 2018

Just manual for now.. :)

@pomegranited pomegranited force-pushed the MCKIN-7023-xblock-i18n-templates branch 2 times, most recently from 251cdb1 to b862eb6 Compare March 15, 2018 08:04
@bradenmacdonald
Copy link

@nedbat @pomegranited It's a little more complicated than that. If we submit work like this upstream to edx/edx-platform or edx/XBlock, then it would need an OSPR, even though it might have a MCKIN ticket associated with it. (Because edX is not paying us for this work.)

In general, OSPRs are never used for this repository/organization though, so a good rule of thumb is to not expect OSPRs for anything in the edx-solutions organization, with the only exception I know of being https://github.com/edx-solutions/xblock-drag-and-drop-v2 (which should really be moved to the edx org now anyways) and on that one I usually just ping Sandy Student for a review directly, since he's faster than the OSPR process :-p

@bradenmacdonald
Copy link

@pomegranited Note that in preparation for a Ginkgo rebase, this shouldn't be merged this week and we may need to target the integration branch instead of master. I'll send details tomorrow.

@pomegranited
Copy link
Author

pomegranited commented Mar 19, 2018

Thanks @bradenmacdonald edit saw your message, so changed the target to integration and rebased.

@pomegranited pomegranited changed the base branch from master to integration March 19, 2018 06:50
@pomegranited pomegranited force-pushed the MCKIN-7023-xblock-i18n-templates branch from b862eb6 to 924d890 Compare March 19, 2018 06:56
@bradenmacdonald
Copy link

Perfect, thanks @pomegranited !

@bradenmacdonald
Copy link

Note that we need to get a green CI build before merging.

@pomegranited pomegranited force-pushed the MCKIN-7023-xblock-i18n-templates branch 2 times, most recently from 351b88d to 15f681b Compare March 23, 2018 01:56
@mtyaka
Copy link

mtyaka commented Mar 27, 2018

Rebased on top of 'integration' from 291fa53e8b0f465cfc976903498b79bf9fbb4a59.

@mtyaka mtyaka force-pushed the MCKIN-7023-xblock-i18n-templates branch 2 times, most recently from 291fa53 to d669319 Compare March 29, 2018 07:04
@mtyaka
Copy link

mtyaka commented Mar 29, 2018

Removed 291fa53e8b0f465cfc976903498b79bf9fbb4a59 to see if it has any effect on tests which are still failing.

@pomegranited pomegranited force-pushed the MCKIN-7023-xblock-i18n-templates branch 5 times, most recently from 24ac919 to d86c03f Compare April 3, 2018 11:24
@pomegranited
Copy link
Author

@mtyaka Ok, the only remaining issues are quality failures, which don't seem to be related to this ticket. What should I do?

@bradenmacdonald
Copy link

@pomegranited It's fine to ignore unrelated quality issues; another team is working on those things as part of the overall ginkgo upgrade. See #1040 (comment)

@pomegranited
Copy link
Author

@bradenmacdonald Should I revert the quality-related commits, 2f5db08 and d86c03f?

@pomegranited pomegranited requested a review from mtyaka April 4, 2018 00:22
@bradenmacdonald
Copy link

@pomegranited Probably, to make the team that's doing the rebase have an easier time, with fewer commits to deal with.

@pomegranited pomegranited force-pushed the MCKIN-7023-xblock-i18n-templates branch from 87cfb2e to 2805cce Compare April 5, 2018 08:31
@pomegranited
Copy link
Author

@bradenmacdonald done.

@mtyaka
Copy link

mtyaka commented Apr 5, 2018

👍

  • I tested this: I installed this branch on the solutions devstack and verified both versions of DnDv2 still work correctly.
  • I read through the code
  • I checked for accessibility issues n/a
  • Includes documentation n/a

to allow XBlock template translations.

Will need to replace these with proper version bumps once the relevant PRs are merged.
@pomegranited pomegranited force-pushed the MCKIN-7023-xblock-i18n-templates branch 2 times, most recently from 5213d1f to a71bcd0 Compare April 5, 2018 10:51
@pomegranited
Copy link
Author

Thanks @mtyaka !

Merging now.

@pomegranited pomegranited changed the title WIP: [MCKIN-7023] Improve XBlock i18n by allowing template translations [MCKIN-7023] Improve XBlock i18n by allowing template translations Apr 5, 2018
@pomegranited pomegranited merged commit 6a04bb5 into integration Apr 5, 2018
@pomegranited pomegranited deleted the MCKIN-7023-xblock-i18n-templates branch April 5, 2018 13:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants