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 #4641, #4674, #4579, #4591: Assorted beta fixes #4685

Merged
merged 13 commits into from
Nov 10, 2022

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Oct 31, 2022

Explanation

Fixes #4641
Fixes #4674
Fixes #4579
Fixes #4591

Miscellaneous fixes for Beta MR2. Specifically:

  • Terms of Use needs updating #4641 is addressed by updating the terms of service wording. No test is needed here since manual verification is sufficient.
  • Hide all downloads options behind downloads flag #4674 is addressed by replacing the existing automatic topic updates platform with a generic "all downloads" one (which will be used for all downloads gating moving forward). Admin controls, add profile, and profile edit pages have been updated to only show downloads-related settings if this flag is enabled. Corresponding tests have been updated. FAQ question 8 was also removed since it asks about how to download lessons, and this support isn't in the app yet. Some strings referencing downloads have also been updated.
  • The home screen is blank on opening beta app #4579 is addressed by properly handling the case where unpublish topics with previous progress would cause the topic list to fail to load. A test was added to verify the fix, but it required a new FakeAssetRepository to be introduced.
  • Incorrect answer responses are not displayed for a question in Mixed numbers and the Number Line 1 #4591 is addressed by fixing FractionInputHasIntegerPartEqualToRuleClassifierProvider's input type (which should be SIGNED_INT but was incorrectly NON_NEGATIVE_INT before; this seems to have been wrong since the classifier was introduced).

Test exemption: FakeAssetRepository doesn't have a test added since it uses a real production AssetRepository, and tests don't yet exist for that). They ought to be tested together.

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

Screenshots showing the before & after changes due to hiding downloads options:

Without changes With changes
Profile creation image image
Profile edit image image
Admin controls image image

Espresso test rules for affected UI tests:

Affected Test Test Results
AdministratorControlsActivityTest (Skipped since there are tablet tests to also run, so the suite won't actually pass cleanly)
ProfileEditActivityTest (Same as above)
AdministratorControlsFragmentTest oppia_assorted_beta_fixes_passing_admin_controls_fragment_test
ProfileEditFragmentTest oppia_assorted_beta_fixes_passing_profile_edit_fragment_test

New Robolectric tests failing for corresponding regressions:

Issue Test Suite Failure Specifics Failure Results
#4579 TopicListControllerTest 1/36 tests fail failing_topic_list_test
#4591 FractionInputHasIntegerPartEqualToRuleClassifierProviderTest 22/23 tests fail oppia_failing_fraction_test

Missing artifacts:

  • Accessibility: not particularly relevant to these changes since the user-facing changes are either crash fixes or removing UI elements (except in the case of terms of service, but that doesn't require additional a11y testing coverage or demonstration here).
  • RTL: nothing in this change affects RTL specifically.
  • Landscape: since the main UI changes are removing elements, the changes to tablet aren't very "interesting" to show off.
  • Tablet: same rationale as for landscape.
  • Internationalization: same rationale as RTL.

This replaces the existing parameter for automatic updates with one
general to all downloads. It does not include gating the side menu as
that would require more refactoring (and will be done in a future,
non-time sensitive PR).
@BenHenning BenHenning marked this pull request as ready for review November 1, 2022 09:18
@BenHenning BenHenning requested a review from rt4914 as a code owner November 1, 2022 09:18
@BenHenning
Copy link
Member Author

The changes are fairly self explanatory, I think, but I'll follow up with a clearer description & tests in the next 1-2 days.

@BenHenning BenHenning self-assigned this Nov 1, 2022
Conflicts:
	app/src/main/java/org/oppia/android/app/profile/AddProfileViewModel.kt
	app/src/main/res/values-ar/strings.xml
	testing/src/main/java/org/oppia/android/testing/platformparameter/TestPlatformParameterModule.kt
The tests were accidentally disabled before, so they didn't reveal the
classifier itself needed to also change.
@BenHenning BenHenning assigned seanlip and unassigned BenHenning Nov 9, 2022
@BenHenning BenHenning requested a review from seanlip November 9, 2022 12:11
@BenHenning
Copy link
Member Author

@seanlip PTAL. As an FYI I have also self-reviewed the whole PR.

@seanlip seanlip assigned BenHenning and unassigned seanlip Nov 9, 2022
@seanlip
Copy link
Member

seanlip commented Nov 9, 2022

Thanks, took a pass. No concerns.

@oppiabot
Copy link

oppiabot bot commented Nov 9, 2022

Assigning @rt4914 for code owner reviews. Thanks!

@oppiabot oppiabot bot assigned rt4914 Nov 9, 2022
@BenHenning
Copy link
Member Author

Thanks @seanlip! Going ahead and merging this without @rt4914’s approval since it’s release blocking.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants