Fix #4833, #4834, #4835, #4838, #1050, #4519, #4522, #4837, #4836, #4855, #4856: Assorted alpha MR6 fixes #4846
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes a bunch of Swahili string bugs by stripping out unnecessary prefixed whitespace. This also introduces a fast language switch bar between English and Swahili. It's not currently tested or gated behind a platform flag, but both of these will be changed if the team decides to proceed with this solution.
This adds support for opening concept cards from hints, solutions, and other concept cards.
Conflicts: app/src/main/java/org/oppia/android/app/player/state/StateFragmentPresenter.kt app/src/main/res/layout/state_fragment.xml app/src/main/res/values-sw/strings.xml app/src/main/res/values/untranslated_strings.xml
Fixes: - Removed extra spacing around a few Swahili strings (all should now be fixed). - Replaced 'reveal' English wording with 'show'. - Added a new event for logging voiceover pausing. - Updated the play/pause voiceover events to include language codes. Documentation and/or tests may not yet be completed.
Conflicts: app/src/main/java/org/oppia/android/app/hintsandsolution/HintsAndSolutionDialogFragmentPresenter.kt app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivity.kt app/src/main/java/org/oppia/android/app/topic/conceptcard/ConceptCardFragmentPresenter.kt domain/src/main/assets/test_exp_id_2.json domain/src/main/assets/test_exp_id_2.textproto
With the learner study parameter enabled, admins can now force-complete lessons on a per-profile basis for all profiles in the app. This leverages the existing developer-only options menu so this isn't something we would want to incorporate in the long-term without more refinement (none of the UI strings are even translated).
This also adds Swahili translations for test topic 0. Some style changes including support for a secondary button type. Updated the in-lesson language switcher to use this style & to be gated behind the learner study platform parameter. And, fixes hint & solution header expanding to be the whole header and not the whole card.
This also includes a rebuild of much of the hint/solution data flow since the previous version had a lot of unnecessary complexity. It also redefines the internal representation for solution answers (which makes them type-safe during data retrieval time).
(The intent of this issue is being tracked separately and will be addressed on this branch in a follow-up commit).
Conflicts: app/src/main/res/drawable/secondary_button_background.xml app/src/main/res/layout/state_fragment.xml app/src/main/res/values/component_colors.xml app/src/main/res/values/styles.xml
This includes: - Post-merge fixes - A change to make the switch lang button container transparent so that the continue & submit buttons are still clickable through it - A fix to work around a databinding bug that can sometimes cause a crash that would be 100% reproducible for a specific build of the app (since it's a bytecode-level issue)
Also, clean up component_colors.xml (noticed during PR self-review) and miscellaneous other code health adjustments.
BenHenning
commented
Jan 19, 2023
seanlip
reviewed
Jan 19, 2023
There was a problem hiding this comment.
Thanks @BenHenning, took a pass and commented on what I could.
app/src/main/java/org/oppia/android/app/hintsandsolution/SolutionViewModel.kt
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/hintsandsolution/SolutionViewModel.kt
Show resolved
Hide resolved
Also, fix a few issues that the new tests discovered.
BenHenning
commented
Jan 20, 2023
- Fixes a test broken in Gradle. - Repurposes a now unused color for the new secondary button style. - Fixes landscape support in profile_edit_fragment for the new modify progress flow.
6 tasks
…roid into assorted-alpha-mr6-fixes
BenHenning
changed the base branch from
develop
to
adjust-version-codes-based-on-flavors
oppiabot
bot
removed
the
stale
BenHenning
changed the title
|
Hi @BenHenning, I'm going to mark this PR as stale because it hasn't had any updates for 7 days. If no further activity occurs within 7 days, it will be automatically closed so that others can take up the issue. |
oppiabot
bot
added
the
stale
oppiabot
bot
removed
the
stale
Base automatically changed from
adjust-version-codes-based-on-flavors
to
develop
March 10, 2023 07:04
BenHenning
changed the title
BenHenning
changed the title
|
PTAL @seanlip for the final follow-up changes (as explained in the latest changes to the PR description in 2 recent edits). I've self-reviewed this code, and recently worked through multiple areas (for analytics and the learner study screen) as part of #4896. @MohitGupta121 PTAL as well since I think you have open conversation threads (please verify & mark them as resolved if you're happy with the latest changes). |
MohitGupta121
approved these changes
Mar 10, 2023
There was a problem hiding this comment.
@BenHenning LGTM,
I think I not see "mark resolved" in conversations due to permission issue. So you can mark them as resolved.
Thanks.
|
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! |
|
No problem, thanks @MohitGupta121! @seanlip since this is set to auto-merge, I'm going to wait to resolve @MohitGupta121's comments until after you approve (or, you're welcome to resolve them after approving if you'd like to merge the PR). |
|
Whoops, I didn't realize resolving the comments would result in the PR getting merged. That said, I'm fine with merging. I read through the comment updates but I didn't look at the code carefully, and the edited description looks fine to me. Thanks! |
6 tasks
BenHenning
added a commit
that referenced
this pull request
Mar 10, 2023
## Explanation Fixes #4897 ### Overview of changes This PR follows up from the changes introduced in #4846 by adding new functionality to help improve event reliability investigations, as well as telemetry diagnostics for research project facilitators that need to ensure events have been successfully uploaded for processing. To help with this, the following features have been introduced: - A new "force upload" button that immediately uploads any pending cached events. - Event counts to show both the number of events waiting to be uploaded & the events that are successfully uploaded. - An improvement on the 'share' button to include a Base64 deflated binary representation of the entire event log store on the device. Leading up to, and during, development, a number of theoretical issues were encountered that were also fixed in case they are playing a role in some of the ongoing concerns around Firebase event delivery reliability: - The sync status reported on the learner analytics screen can be wrong in some cases, leading to the assumption that events have been uploaded when they may not have been. - Events can, in some cases, be lost from previous app instances if they haven't been uploaded yet and, in other cases, duplicated. The former is much more likely to happen in practice, particularly with the connectivity patterns being used in the research project. ### Technical specifics - ``FakeSyncStatusManager`` was renamed to ``TestSyncStatusManager`` since it's actually augmenting the production implementation of ``SyncStatusManager``, not replacing it. - Events are now being cached indefinitely if the learner study feature is enabled. This allows for some robustness improvements (see below), and the ability to share the events themselves if the team suspects events are not arriving to Firebase. - The issue of sync status being incorrect came from an unreliable means of reporting sync status during different event scenarios. Specifically, if the ``LogUploadWorker`` doesn't kick off and there's network connectivity, a new event being logged could trigger the sync status to become "data uploaded" even though there are still pending events. The new solution is to leverage the now-tracked uploaded events to determine whether there are still events yet waiting to upload. In this way, the sync status should always be the most up-to-date information with one caveat: network connectivity changes won't reflect until a new log is requested (this is a limitation in how the app tracks network connectivity, and it didn't seem very important to fix it). - The issue of possible event duplication comes from ``PersistentCacheStore`` using ``mergeFrom`` with the *current* cached state and the on-disk proto being loaded. I originally thought that all pathways to loading the proto from disk could only execute once, but there's at least one pathway where it's possible to chain multiple calls into the cache store such that the load happens twice (and both results end up being merged together, hence the event duplication). This has been fixed by never reloading the cache store if it's already been loaded which should always be correct. - The issue if possible events being lost comes from ``AnalyticsController`` previously not priming the event log cache. This means that if a new event is logged before the cache is read (e.g. early events like "open profile screen" can race against ``LogUploadWorker`` kicking off on app startup) then the cache will be completely reset and, once written, will overwrite any events currently cached on disk. This has been fixed by ensuring that the event log cache has been primed before any interactions with it (read or write) occur. Note that all of the above has been thoroughly tested through automated testing to ensure that these issues have been properly fixed, and stay that way in the long-term. ### Test notes & exemptions - In order to simplify testing ``SyncStatusManagerImpl`` and ``TestSyncStatusManager``, they ought to both pass a common test suite specific to ``SyncStatusManager`` itself. This has been done by introducing a ``SyncStatusManagerTestBase`` class that's inherited by both implementation test suites so that they inherit common API tests. This pattern is used in one other location in the codebase currently: https://github.com/oppia/oppia-android/blob/0290ef8037f798d8ba2721441612a80dec4a1f1a/testing/src/test/java/org/oppia/android/testing/threading/TestCoroutineDispatcherTestBase.kt#L33 - ``CircularProgressIndicatorAdaptersTestActivity`` has been exempted for an a11y label for the same reason as all other test activities: it technically doesn't need one. - ``TestSyncStatusManagerTest`` has been allow-listed to use parameterized tests since it's aggressively verifying many different force/override sync status cases. Making sure that the test utility works correctly is extremely important to avoid false failures/passes of dependent tests (which are, in turn, guarding against regressions for the core issues covered by this PR, among other event system behaviors). - ``SyncStatusManagerTestBase`` has been exempted from requiring KDocs since it's technically a test suite, so KDocs aren't required. - A number of classes have been exempted from having new test suites as per existing conventions and requirements: ``ControlButtonsViewModel`` (renamed from ``ShareIdsViewModel``), ``CircularProgressIndicatorAdaptersTestActivity`` (simple test activities don't require their own tests), ``CircularProgressIndicatorAdaptersTestViewModel`` (view models aren't tested directly), ``SyncStatusManagerTestBase`` (the test base is more or less a test suite itself, so it doesn't make sense for it to have tests as well). - ``TestSyncStatusManagerTest`` has been disabled in Gradle since it depends on ``SyncStatusManagerTestBase`` which is part of a different module. To make this work, the test base would have to be moved to src/main of the testing module which isn't a very clean approach. The current setup works fine with Bazel, and longer term this will get cleaner by moving testing utilities like ``TestSyncStatusManager`` to be near their corresponding production implementations (we just can't do this today because of Gradle limitations). - Due to the asynchronous and data race nature of some of the issues discovered during test writing, changed & added tests were sometimes found to be flaky (and, in some cases, very rarely, e.g. 1/50). To ensure that all of the new & changed tests are stable, I made sure to run all 8 affected test suites 128 times to ensure stability. Here are the results of that run ([buildbuddy link](https://app.buildbuddy.io/invocation/5da310e1-3cc9-451b-a1d0-951a3322c8d7)):  ## Essential Checklist <!-- Please tick the relevant boxes by putting an "x" in them. --> - [x] 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: ...".) - [x] Any changes to [scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets) files have their rationale included in the PR explanation. - [x] The PR follows the [style guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide). - [x] The PR does not contain any unnecessary code changes from Android Studio ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)). - [x] The PR is made from a branch that's **not** called "develop" and is up-to-date with "develop". - [x] The PR is **assigned** to the appropriate reviewers ([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)). ## For UI-specific PRs only Following is a video demonstrating the new functionality introduced in this PR: https://user-images.githubusercontent.com/12983742/224289397-73c5e3a7-4f7a-4116-8e71-3316e30fa9d7.mp4 Note that affected UI tests that could run on Espresso were **not** verified in this PR similar for the reasons listed in #4846 (specifically, the ongoing issue of Espresso tests being time consuming to investigate, often broken, and this PR being high priority). Localization & accessibility changes aren't relevant for this change, and any tablet-specific issues aren't important since the affected screen is only expected to be accessed on handset devices.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Explanation
Fixes #4833
Fixes #4834
Fixes #4835
Fixes #4838
Fixes #1050
Fixes #4519
Fixes #4522
Fixes #4837
Fixes #4836
Fixes #4855
Fixes #4856
This PR introduces a variety of fixes and user quality-of-life improvements in preparation for the start of the upcoming multi-week user research project in Kenya. This PR covers 9 different issues in one shot to keep the PR management simpler due to the time sensitive nature of these changes, so it's on the larger side. Please note that:
The specifics of the PR will be split across the different issues being addressed, in categories below.
Improvements to solutions
#4833 needed to be addressed because the previous terminology ("reveal") has been found to be confusing to learners who might not yet have a strong grasp on English. This string update will, unfortunately, not be translated for the upcoming study (though the study predominantly uses English). This change affects all users of the app.
#1050 is a long-standing issue wherein solution answers would only show up correctly if they were either text or a fraction. This PR fixes the issue by introducing generic answer support, expanding it to include: numeric input, numeric expression, algebraic expression, algebraic equation, and ratio expression. The only missing interaction that web supports is drag & drop, but this has been found to not be needed for the current shipped list of topics (as now enforced by an update to the content import pipeline) and introducing support for it would nontrivially increase the complexity of that side of the PR. This is something that, like other interactions, will be added in the future once needed. This fix will affect all users of the app.
Improvements to voiceover telemetry
#4834 was addressed by introducing a new event to track explicit voiceover pauses (i.e. those that require clicking the 'pause' button and not the ones that are automatic, such as autoplay ending or the user navigating away from the lesson player). Because of the nature of the 'play voiceover' event (that it is logged both for clicking the 'play' button and when expanding the audio bar due to autoplay), it's possible for the play & pause event counts to not be equal. The intent behind these events is to track explicit user actions, not the occurrences of voiceovers playing/not playing. This event will be logged for all users, though sensitive identifiers will only be logged if the user study feature is enabled.
#4835 was addressed by updating both the existing 'play voiceover' and new 'pause vocieover' events to include the lesson player-reported language code (that is, the language code originating from Oppia web). We may change this to be something more strongly ensured (since Oppia Android is a bit more careful in its language code management due to Android complexities), but the code should be generally usable as-is for data purposes as it's implemented. This change will affect telemetry for all users of the app.
Improvements to concept card availability
#4519 and #4522 were addressed by adding support for opening concept cards both in concept cards themselves, and from hints. As of this PR, just about all cases of lesson HTML rendering should now include support for linkifying and opening concept card references. Since both hints and concept cards are dialogs, the behavior is that the newly opened concept card dialog "stacks" on top of the existing, open dialog (i.e. closing the concept card will show the original dialog from which it was opened). This change affects all users of the app.
Miscellaneous language improvements
#4837 was addressed to clean up a variety of extraneous spaces & newlines that were unintentionally added by translators for Swahili strings. These have a direct user benefit but are otherwise innocuous, non-semantic changes. This fix technically affects all users of the app, but only those who use the Swahili translations will actually interact with the changed strings.
Study-specific improvements
#4838 was addressed by introducing a new link in the profile edit flow for administrators to access the previously developer-only flow for manually marking certain chapters as completed. This newly exposed mechanism allows study facilitators to set up profiles so that learners can begin at a specific point (which is particularly helpful if the data on the device ever needs to be reset, such as in the case where the administrator forgot their PIN). This change will only affect user study administrators as it's locked both behind the admin account and the learner study flag. The functionality will be available on production-optimized builds (that is, it won't be restricted to developer-only build flavors of the app).
#4836 was addressed by introducing a floating button within the lesson player for lesson cards whose content have available Swahili translations to quickly switch the content language between English and Swahili. This doesn't change app strings, only content strings, and only temporarily for the lifetime of that specific play session (that is, if they exit the lesson & return immediately they will still be in the switched language until the app itself restarts). This button only appears if the study feature is enabled and has been enabled for that user by the administrator profile (via a new setting on the "Edit Profile" screen). This button also has its own new event to track its usage.
A "share IDs" button was added to make it easier to share the device & learner IDs by collating the IDs into a single blob of text that can be shared to any app that accepts text intents. This is meant to help reduce potential error by study coordinators.
Content display improvements
#4855 was addressed by changing a lot of how custom LI/OL tags are handled. Specifically, Android will automatically shift the leading span of cascading bullets/numbers, but the previous implementation was using the wrong value for each nested span's parent's leading margin (which is needed to adjust what leading margin to use for the child span). This was fixed by introducing more thorough tracking (which also included a bit of robustness in ensuring larger numbers can be aligned correctly). This doesn't quite match HTML rendering, but it's much closer. Bullet lists were also updated to use squares in addition to filled and open circles (for parity with web HTML), and now uses sizes & spacing that are a bit cleaner to see.
There's still more work to improve edge cases for bullet list rendering (see #4859).
#4856 was addressed by introducing a separate SVG "render size" that, unlike the intrinsic size, is based on the image filename. Since the image filename is given in pixels, there needs to be a translation to dp to ensure consistent viewing across devices. This computation has been implemented with three calculation passes:
There's still more work to improve image rendering (see #4093).
General technical details
Design choices & other details of note:
HelpIndexas it greatly reduces the complexity of managing UI-side hint state. Since solutions were being updated, anyway, this unrelated complexity has been reduced (and other issues like directly constructing an injectable view model were addressed).AnalyticsController).Noteworthy test & exemption changes:
StateRetrieverTestis largely not updated since all of the JSON changes in this PR only affect local development (all Bazel tests & production builds use textproto or binary proto versions of lessons). The tests locally passing plus some basic local ad hoc testing is considered sufficient for the JSON loading pipeline as it will, eventually, go away in favor of textproto (once Gradle is removed since textproto -> binary proto conversion is not easily implemented in the Gradle build environment).Essential Checklist
For UI-specific PRs only
Screenshots showing some of the new functionality
A couple things to note:
Videos demonstrating different changed/new flows
Espresso test results
The following tests have been modified as a result of this PR:
I have not run the Espresso tests for these suites. I ran into some issues getting the suites to kick off locally, and I won't have time to investigate this for a bit due to other competing priorities. Reviewers: please let me know if you would like these run and I will prioritize accordingly (I'm leaning toward not running them since many of the Espresso tests are already failing on develop, and we have no viable way to keep them passing until we can run them in CI).