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

Fix #3729, #3730, #3777, #91, #20, part of #3625: Introduce support for localizing lesson content & multi-lingual answer submission #3796

Conversation

BenHenning
Copy link
Sponsor Member

@BenHenning BenHenning commented Sep 15, 2021

Fix #3729
Fix #3730
Fix #3777
Fix #91
Fix #20
Fix part of #3625

Explanation

This PR introduces the integration pathways necessary for all currently localizable content strings to be properly translated using the infrastructure introduced in #3794. The current localization support per Oppia lesson structures (that are also being introduced in this PR):

  • Translating content & feedback strings in explorations & questions
  • Translating options for multiple choice, item selection, and drag & drop
  • Translating explanations for hints & solutions
  • Translating explanations for concept cards
  • Translating concepts for revision cards
  • Matching drag & drop, multiple choice, and item selection answers in a language-agnostic way
  • Matching text input answers correctly based on the language
  • Translating interaction placeholder texts

Note that some of the above aren't yet supported in Oppia's editor experience, so they aren't actually translated in production assets (concept & revision cards). Further, there are some parts of lesson structures that don't yet support translations (titles, descriptions, and outlines), so the app experience will be partly translated.

Finally, see #3794 for context on the overall design of the infrastructural side of this work, and for a link to the (now outdated) design document for the project.

Design overview

At a high-level, content & related strings (such as answer options or interaction placeholders) are translated by switching out the default content string with a translated version via its content ID using a translation context that's computed by TranslationController (the string swapping bit is also done via convenience helpers from TranslationController). This context is computed to contain the exact strings needed for each situation based on the user's current content language.

To implement the above, the translation context is passed along in all ephemeral structures (i.e. EphemeralState and EphemeralQuestion). Two new ephemeral structures were also added for similar purpose: EphemeralConceptCard and EphemeralRevisionCard since translation context is short-lived data and the contained concept/revision cards are persisted. Note that one consequence of this is that all ephemeral structure data providers now depend on various language data providers (including the system data provider) to ensure automatic updating works properly.

Users cannot select their content language today (#3797 is disabling the current UI, and #52 will include the work to properly re-enable it). The default content language is to use whatever app strings are using (which is currently to default to the system language), or fall back to the default content strings.

The other part of content localization is supporting non-English answers. Java/Kotlin + Android handle the encoding part automatically, so all the app needs to do is match the answer to the correct corresponding localized answer. Most interactions are either language-agnostic (e.g. fractions, numeric input, ratios) or leverage other indicators like indexes or content IDs to perform matching (e.g. multiple choice, item selection, drag and drop). The only interaction actually needing work is text input. In order for this (& future interactions) to properly localize, the classification framework was updated to also take the current translation context so that the correct input can be picked when matching against the answer.

Note that from a UX perspective, content strings almost always match the exact current language. That means if the user were to change their language mid-lesson, nearly all the strings will update. The single exception is submitted answers since those had a localized meaning at the time of submission, so they retain their translation. All other content strings currently will switch (including feedback).

Other miscellaneous considerations for the design:

  • The language definition for Portuguese was updated to include a content string ID since content currently uses 'pt' for Brazilian Portuguese, so the app will leverage its fallback mechanism when retrieving content strings
  • As indicated above, tests from TopicControllerTest, Exploration/QuestionAssessmentProgressControllerTest, QuestionTrainingControllerTest, and others led to a change in content language representation: before, retrieving the content locale for an unknown language would fail, but now it falls back to built-in strings. This enables tests & the app to work in the Gradle environment when there are no supported languages, and it also produces a more robust system in the event the configuration actually has an issue.

Changes to test assets

This PR required updating the structures of all test assets (both JSON & textproto) to support a relatively recent change to localization in explorations. Rule inputs now support content-ID based input types (which, for text input, required support in the translation substructure to support a list of translated strings rather than a single). Unfortunately, it results in multiple thousands of lines of affected changes. Note that most of the JSON & textproto changes are automatically generated by a import/conversion script, so I suggest lightly reviewing those files. Our tests cover the content of the test lessons quite thoroughly at this point, as well.

Beyond that, it was also desirable to translated at least one example of each structure type (except questions) to verify this PR. To do that, the asset download script that we use was updated to support filling in automatic translations by scraping Google Translate on web. Many of the translations may be a bit nonsensical, but they are reasonable placeholders to verify translation functionality for both Arabic & Brazilian Portuguese.

Note that a new subtopic was added for the base test topic so that revision card translations could be tested. I suspect we'll find other good uses out of this new test subtopic, as well.

Finally, the changes to the proto structures were meant to be backward compatible with previous saved versions of explorations or saved state (though these aren't expected to realistically hit users other than developers with saved checkpoints, so little effort was made to actually verify the support this time).

Issues fixed in this PR

There were a few issues discovered and/or fixed as part of this PR:

Changes to the core language selection mechanism

This PR also changes the core language selection mechanism to no longer update the application context, also changing the meaning of the system data provider to always be tied to the actual current system locale. This fixes an issue noticed earlier that broke automatic content string updates when the system locale changes, and is actually conceptually more consistent since it ensures the app can always keep track of the actual system locale.

While this improves tracking & fixes the observed issue, it also introduces some new inconsistencies (such as the navigation drawer not updating when the system locale changes). There are a few edge cases where the mechanism breaks down a bit, so more robustness work will be needed in the future. #3840 is partly tracking the infrastructural side of this, but additional user-facing localization issues are likely to be filed in future app test passes.

Misceallaneous changes

Miscellaneous changes in the PR & why they were needed:

  • The Oppia logger refactor was needed in order to avoid a circular dependency that cropped up due to new dependencies on the logger (& note that sub-parts must also be migrated if a parent package is migrated otherwise they'll be excluded from the build per expected Bazel behavior)
  • The Bazel app flavors were updated such that dev-only is also testonly (so it could, in the future, depend on test-only libraries)
  • oppia_android_application was updated to properly output an error when part of the AAB creation process fails (rather than hiding the error), and to use xargs in a way that's more compatible with OSX
  • The compute affected tests script was updated out of necessity. https://github.com/oppia/oppia-android/pull/3796/checks?check_run_id=3707485282 failed because this PR as the third of three very large PRs had such a large changeset (>700 files) that one of the Bazel query commands outright timed out. The script needed to be updated to split things for more robustness at the expense of performance. No tests were changed/added since this is really an implementation detail, and testing it would be a bit tricky (& probably not worth it).
  • For most cases, no new test suites were introduced in this PR even though there are changes to files that don't have tested. In these cases, the class changes were low-risk (such as retriever classes) where we can introduce test coverage as part of future code coverage efforts.
  • AsyncDataSubscriptionManager was reworked in its entirety to enable two things: (1) the ability to catch cycles, and (2) to de-duplicate subscriptions. Both of these were observed as issues (the former only in an odd edge case with checkpoints) which led to stability issues. These are long-standing issues that have gotten worse now that we're working with larger data provider graphs, so it's timely to fix them. A new test suite was also added to verify correctness, and even run against the old implementation to demonstrate the new functionality improvements: https://app.buildbuddy.io/invocation/f11e143b-a010-4ea5-8fa7-94284fb6ea98. The utility also has specific support for better debuggability by printing out the subscription dependency graphs when a cycle is detected.
  • QuestionAssessmentProgressControllerTest & others (including for explorations) needed to be reworked a bit due to incorrectly relying on getCurrentQuestion/State earlier than was valid (i.e. before starting the training/play sessions) which broke a new expectation that the profile ID is properly initialized in each controller prior to that point. The changes should be both more correct and more robust. Some actual behaviors in the controllers were also changed (with corresponding test updates) since there are fewer pending states possible than before (versus outright failures).
  • Some minor changes were made to RecyclerViewMatcher to turn an NPE into a failed match when the actual recycler view is missing from the view hierarchy
  • Note that no image region selection tests were added in StateFragmentTest since they're current flaky. A TODO was added to track adding these in the future, instead. This seems low risk since image region selection shouldn't really be affected by the translation infrastructure & has been manually verified.
  • No question tests were added since there isn't a proper test suite for them, but a TODO was added to track adding them in the future. This seems low risk since the state fragment tests comprehensively cover the new language support in the shared questions/exploration UI infrastructure.
  • InitializeDefaultLocaleRule & its corresponding annotation were updated to support defining a region. Due to the changes to how locale selection works (see corresponding section above), some tests (language watcher mixin + HomeActivityTest) started failing due to an unexpected need to recreate the activity. The former was fixed by defining a more specific initial locale definition (to avoid relying on the rotation), and the latter by ensuring the system locale actually matches the initial context. Both seem more correct overall.
  • Note that all of the new tests were disabled on Espresso due to the difficulty in getting these tests to pass, and that many of the existing Espresso tests are inconsistently failing. Without having Espresso tests running in CI, it's getting increasingly more difficult to keep them up-to-date. Enable localization tests in Espresso [Blocked: #973] #3858 is tracking migrating these new tests over to Espresso once it's running in CI. Further, many of the tests can't be run on Espresso since only Gradle can be used for instrumentation tests today, and Gradle doesn't load in the supported languages configuration.
  • DataProviderTestMonitor was changed slightly to become Espresso-compatible (in particular, it observes changes using the main thread correctly for both Robolectric & Espresso now). No new tests were added for this behavior since it's a bit tricky to test, and simply seeing the tests that use it pass is sufficient to know that it's working (it's extremely unlikely to result in falsely passing tests).

Essential Checklist

  • The PR title and explanation each start with "Fix #bugnum: " (If this PR fixes part of an issue, prefix the title with "Fix part of #bugnum: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

Overview screenshots / video

Since this PR covers multiple screens, these screenshots will cover specific cases of content. Also, a video will be provided at the end demonstrating the app's behavior with automatic selection.

Portrait -- exploration player (English/Arabic):
exp_player_translated_portrait

Landscape -- exploration player (English/Arabic):
exp_player_translated_landscape

Portrait -- revision card (English/Arabic):
revision_card_translated_portrait

Landscape -- revision card (English/Arabic):
revision_card_translated_landscape

Video demonstrating automatic switching between English, Arabic, and Brazilian Portuguese (& other content screens from the above):
https://user-images.githubusercontent.com/12983742/135427506-286ffaad-fa86-4275-b260-02fdfd9afb61.mp4

Note that tablet screenshots are not included since there aren't any layout changes made by this PR.

Accessibility

See video for how Talkback behaves with content string translations:

exploration_player_translated_content_strings_talkback.mp4

(Note that RTL switches properly for Talkback when Arabic is selected)

Espresso tests

Per above, no new Espresso tests were added in this PR. Instead, the following suites were tested against develop & this branch to ensure no regressions were introduced:

  • org.oppia.android.app.player.exploration.ExplorationActivityTest
  • org.oppia.android.app.topic.questionplayer.QuestionPlayerActivityTest
  • org.oppia.android.app.topic.conceptcard.ConceptCardFragmentTest
  • org.oppia.android.app.topic.revisioncard.RevisionCardFragmentTest
  • org.oppia.android.app.topic.revisioncard.RevisionCardActivityTest
  • org.oppia.android.app.topic.TopicActivityTest
  • org.oppia.android.app.player.state.StateFragmentTest

All test suites except StateFragmentTest & ExplorationActivityTest passed on both develop & this branch. See the following screenshot for test results on this branch:
gradle_run_results

For ExplorationActivityTest, I observed the following tests failing on Espresso:

  • testAudioCellular_ratioExp_audioIcon_clickPositive_checkAudioFragmentIsVisible
  • testAudioCellular_ratioExp_checkPositive_audioIconTwice_audioFragVisDialogNotDisplay
  • testAudioWifi_fractionsExp_changeLang_next_langIsHinglish
  • testExploration_clickAudioIconTwice_contentDescription_changesToDefault
  • testExploration_clickAudioIcon_contentDescription_changesCorrectly

Locally, I noticed the following additional failures (note that some of the above passed for me locally):

  • testExpActivity_progressSaved_onBackPress_checkNoProgressDeleted
  • testExpActivity_showUnsavedExpDialog_cancel_checkOldestProgressIsSaved
  • testExpActivity_showUnsavedExpDialog_cancel_dismissesDialog
  • testExpActivity_showUnsavedExpDialog_leave_checkOldestProgressIsSaved
  • testExplorationActivity_hasCorrectActivityLabel
  • testExplorationActivity_loadExplorationFragment_hasDummyString

All of these came from an instrumentation process crash which seems to be unrelated to this PR.

Regarding StateFragmentTest, I observed the following failures on this branch:

  • testStateFragment_forMisconception_clickLinkText_opensConceptCard
  • testStateFragment_interactions_numericInputInteraction_hasCorrectHint
  • testStateFragment_interactions_radioItemSelection_hasCorrectAccessibilityAttributes
  • testStateFragment_landscape_forMisconception_clickLinkText_opensConceptCard
  • testStateFragment_loadExp_changeConfig_continueToEnd_clickReturnToTopic_destroysActivity
  • testStateFragment_loadExp_continueToEndExploration_hasReturnToTopicButton
  • testStateFragment_loadExp_land_secondState_invalidAnswer_submitAnswerIsNotEnabled
  • testStateFragment_loadExp_landscape_secondState_submitAnswer_submitButtonIsEnabled
  • testStateFragment_loadExp_secondState_hasSubmitButton
  • testStateFragment_loadExp_secondState_invalidAnswer_updated_submitAnswerIsEnabled
  • testStateFragment_loadExp_secondState_submitAnswer_submitButtonIsEnabled
  • testStateFragment_loadExp_secondState_submitWrongAnswer_contentDescriptionIsCorrect*
  • testStateFragment_loadExp_submitAnswer_clickContinueThenPrevious_onlyNextButtonIsShown

* was observed as failing on develop, plus another test that didn't fail in this run.

Essentially all of these failed due to the TestCoroutineDispatchers idling resource not finishing. While there are some changes in this PR that could affect that (e.g. DataProviderMonitor), they aren't actually being used by Espresso (those tests are disabled), and it oddly only affects some of the tests. This is also the same error for the failed tests on develop, and I don't see a reason why this branch would make it worse.

BenHenning and others added 30 commits August 31, 2021 23:33
There are a lot of details to cover here--see the PR for the complete
context.
- Add missing codeowner
- Add support for configuring base branch reference
- Update CI for dev/alpha AAB builds to use 'develop' since there's no
  origin configured by default in the workflows
This is needed to open a PR on GitHub. This commit is being made so that
the PR can start off in a broken Actions state.

This also initially disables most non-Bazel workflows to make workflow
iteration faster and less impacting on other team members.
This introduces a new mechanism for passing lists of tests to sharded
test targets in CI, and hooks it up. No actual sharding is occurring
yet. This led to some simplifications in the CI workflow since the
script can be more dynamic in computing the full list of targets (which
also works around a previous bug with instrumentation tests being run).
Java proto lite also needed to be upgraded for the scripts to be able to
use it.

More testing/documentation needed as this functionality continues to
expand.
This simply partitions bucketed groups of targets into chunks of 10 for
each run. Only 3 buckets are currently retained to test sharding in CI
before introducing full support.
Fixes some caching bucket and output bugs. Also, introduces while loop &
keep_going to introduce resilience against app test build failures (or
just test failures in general).
Also, enable other workflows.

Note that CI shouldn't fully pass yet since some documentation and
testing needs to be added yet, but this is meant to be a more realistic
test of the CI environment before the PR is finished.
Adds a human-readable prefix to make the shards look a bit nicer.

Also, adds more fine-tuned partitioning to bucket & reduce shard counts
to improve overall timing. Will need to be tested in CI.
A newly computed variable wasn't updated to be used in an earlier
change.
Add docstrings for proto.
…/oppia-android into add-bundles-proguard-build-flavors
Neither 'mv -t' nor piping to mv work on OSX so we needed to find an
alternative (in this case just trying to move everything). This actually
works a bit better since it's doing a per-file move rather than
accommodating for files that shouldn't be moved (which isn't an issue
since the destination directory is different than the one containing the
AAB file).
Documentation, thorough tests, and detailed description of these changes
are still needed.
This demonstrates working string selection for system-based and
overwritten app languages, including necessary activity recreation &
layout direction overwriting.

This also includes a bunch of Dagger infra refactoring so that some app
layer packages can now be modularized (including the new packages).
…o localization-part5-introduce-app-string-translations-support-and-refactor
This involves MANY broad changes to ensure consistent string retrieval
(for arrays and plurals), formatting, and string transformations
throughout the codebase. Some extra patterns to added to fix things that
were needed, and a few issues were fixed along the way.
@BenHenning
Copy link
Sponsor Member Author

@rt4914 PTAL for codeowners

@vinitamurthi PTAL for codeowners. Also, while updating QuestionAssessmentProgressControllerTest for correctness some of the mastery values changed. I'm having difficulty determining if the new calculations are actually correct (indicating the old ones were wrong), though from what I can tell they seem correct now. Can you please check & confirm? It's unexpected that these changes would result in a difference--I'm not entirely sure what's going on there. See:

  • someWrongAnswersSubmittedWithTaggedMisconceptionSkillId
  • wrongAnswersAllSubmittedWithMisconception_onlyMisconceptionSkillIdMasteryDegreesAffected

@anandwana001 PTAL for codeowners & also as a second reviewer for domain/utility changes.

Copy link
Contributor

@rt4914 rt4914 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks.

@rt4914 rt4914 removed their assignment Sep 30, 2021
Copy link
Contributor

@vinitamurthi vinitamurthi left a comment

Choose a reason for hiding this comment

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

Changes LGTM but I am not sure why the mastery values changed. @TheRealJessicaLi can you take a look at the two tests Ben mentioned?

@oppiabot
Copy link

oppiabot bot commented Oct 5, 2021

Unassigning @vinitamurthi since they have already approved the PR.

Copy link
Contributor

@anandwana001 anandwana001 left a comment

Choose a reason for hiding this comment

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

LGTM

Adding a screenshot to confirm the correctness of the language.

@oppiabot oppiabot bot added the PR: LGTM label Oct 5, 2021
@oppiabot
Copy link

oppiabot bot commented Oct 5, 2021

Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

…answer-translations-support

Conflicts:
	domain/BUILD.bazel
@BenHenning
Copy link
Sponsor Member Author

Thanks all.

@TheRealJessicaLi I followed up with you over chat. To unblock downstream work & the MR4 launch, I'm going ahead and merging this. Please follow up if there's actually a mistake that needs to be fixed here.

@BenHenning BenHenning merged commit 5801df0 into develop Oct 6, 2021
@BenHenning BenHenning deleted the localization-part6-introduce-content-and-answer-translations-support branch October 6, 2021 21:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment