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 #3946: Fix solution automatically revealing after first hint reveal #3955

Merged
merged 6 commits into from
Oct 21, 2021

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented Oct 20, 2021

Explanation

Fix #3946

Per #3946 there's a bug in some contexts where, after revealing a hint, the solution may automatically be revealed as well when the user reopens the solution dialog.

The root cause of this is that the hint handler decides whether to show solutions based on the presence of a correct answer, not based on the presence of a solution itself. This presents a problem specifically for proto-based lessons because in some cases correct_answer will be absent (if the converter can't properly convert the answer into an app-acceptable format, e.g. text or fraction). This is technically a valid case to introduce a failure, but due to #1050 we can't actually assume that correct_answer is properly defined. Thus, the solution is to change the check to instead check for whether solution is present. This does allow for an empty solution object to be added to the proto, but this shouldn't realistically happen in practice so it should be fine.

To repro (on this branch):

  • Enable proto loading in CachingModule
  • Build & install a dev flavor of the app
  • Start a new Prototype Exploration play session
  • Play to the numeric expression state
  • Enter 2 wrong answers
  • Reveal the hint
  • Close & reopen the dialog
  • (If you want to repro the issue, you'll need to replace the implementation of HintHandlerProdImpl with the current develop copy)
  • Without the fix, the solution will automatically be revealed. With the fix, the solution won't yet be available to reveal.

Note that this doesn't affect json since we've taken some measure to ensure that correct_answer is filled with something even if that value isn't quite correct.

To ensure this regression is properly caught, tests were added both in StateFragmentLocalTest and HintHandlerProdImplTest. Note that the latter required introducing a new test exploration that specifically tests a missing correct_answer, and the former required changes to the prototype exploration to introduce a hint & solution in the numeric expression state to more easily repro this issue. Test results:

Other miscellaneous changes:

  • StateFragmentLocalTest had some cleaning up since its helpers were a bit confusing referencing states for different explorations (now that the test suite uses at least 2 explorations for testing)
  • StateFragmentLocalTest was updated to depend on textproto assets in Bazel for better coverage, and so that the failure case could be triggered per the new regression tests
  • StateRetriever was updated to allow a missing correct_answer in solutions (for parity with the textproto variant, but the implementation is a bit more permissive and actually results in passing tests regardless of the fix)
  • StateRetriever was also changed so that solutions are not set if they aren't loaded (to avoid an empty solution being included which shouldn't actually ever happen for loaded binary protos). This ensures that the updated check (hasCorrectAnswer -> hasSolution) results in consistent & correct behavior for both JSON & textproto runs of the hint tests/app builds.
  • HintHandlerProdImpl was updated to use an AtomicInteger for hint sequencing. This isn't related to the original issue, but there's a likely race condition in the current implementation that I'm fixing at the same time. No regression test is reasonably possible to actually test this, so it needs to be a verify-on-inspection type change.

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

N/A -- this is fixing a UI-affecting issue, but doesn't actually change the UI.

@BenHenning BenHenning added this to the Alpha MR3 milestone Oct 21, 2021
@BenHenning BenHenning added the PR: Cherrypick requested Indicates that a PR is being requested for being cherrypicked into the ongoing release branch. label Oct 21, 2021
@BenHenning BenHenning marked this pull request as ready for review October 21, 2021 06:52
@BenHenning BenHenning requested a review from rt4914 as a code owner October 21, 2021 06:52
@BenHenning
Copy link
Member Author

@rt4914 PTAL (for codeowners + other changes if you can since the PR isn't affecting too many files).

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 assigned BenHenning and unassigned rt4914 Oct 21, 2021
@oppiabot oppiabot bot added the PR: LGTM label Oct 21, 2021
@oppiabot
Copy link

oppiabot bot commented Oct 21, 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!

@BenHenning
Copy link
Member Author

Thanks @rt4914!

@BenHenning
Copy link
Member Author

SIGSEGV in StateFragmentTest for the Gradle run. Re-running since that test should pass in this PR.

@BenHenning
Copy link
Member Author

Looks like everything is passing, so merging now.

@BenHenning BenHenning merged commit 8fb0a85 into develop Oct 21, 2021
@BenHenning BenHenning deleted the fix-automatic-solution-issue branch October 21, 2021 20:22
BenHenning added a commit that referenced this pull request Oct 27, 2021
…al (#3955)

* Fix solution auto showing after hint reveal.

See #3946 & PR for specifics.

* Add translations for new hint/solution.

* Lint fixes.

* Add TODO.

* Fix Gradle-variant tests.
BenHenning added a commit that referenced this pull request Oct 29, 2021
#3975)

* Fix #3946: Fix solution automatically revealing after first hint reveal (#3955)

* Fix solution auto showing after hint reveal.

See #3946 & PR for specifics.

* Add translations for new hint/solution.

* Lint fixes.

* Add TODO.

* Fix Gradle-variant tests.

* Fix #3937: Ensure ViewEventLogsViewModel builds for alpha builds (#3957)

* Ensure ViewEventLogsViewModel builds for alpha.

* BUILD file lint fix.

* Fix #3939 & #3938: Fix KitKat crash & SVG rendering issues (#3963)

* Fix KitKat crash when opening Help menu.

* Fix SVG rendering on SDKs 19-23 (incl).

* Add regex check to prohibit Delegates.

* Lint fixes.

* Add exemption for regex script test.

* Update file_content_validation_checks.textproto

Grammar fix in error.

* Update RegexPatternValidationCheckTest.kt

Copy grammar fix to test copy of error.

* Update version.bzl (#3964)

Bump version codes for another RC of release-0.6.

* Embed proguard.map in optimized AAB builds. (#3973)
@BenHenning BenHenning added the PR: Cherrypick completed Indicates a cherrypick request was approved & completed for a PR. label Oct 30, 2021
@BenHenning
Copy link
Member Author

This is confirmed as fixed in the release branch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Cherrypick completed Indicates a cherrypick request was approved & completed for a PR. PR: Cherrypick requested Indicates that a PR is being requested for being cherrypicked into the ongoing release branch. PR: LGTM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Solutions seem to immediately appear without prompting
2 participants