-
Notifications
You must be signed in to change notification settings - Fork 536
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
Fixes #5572 : Softer redirection for learners who need to revisit earlier cards #5626
base: develop
Are you sure you want to change the base?
Fixes #5572 : Softer redirection for learners who need to revisit earlier cards #5626
Conversation
Develop (fork) Branch Update
Develop (fork) Branch Update #2
Develop (fork) Branch Update
Develop(fork) branch update
Develop (fork) Branch Update
Develop (fork) Branch Update
…ies (oppia#5526) <!-- READ ME FIRST: Please fill in the explanation section below and check off every point from the Essential Checklist! --> ## Explanation <!-- - Explain what your PR does. If this PR fixes an existing bug, please include - "Fixes #bugnum:" in the explanation so that GitHub can auto-close the issue - when this PR is merged. --> Fixes oppia#5404 This PR migrates deprecated `onBackPressed` usage to `OnBackPressedDispatcher` callback in the following activities and presenters. - ProfileEditActivity - ProfileEditActivityPresenter - QuestionPlayerActivityPresenter - WalkthroughFinalFragmentPresenter ## 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 <!-- Delete these section if this PR does not include UI-related changes. --> If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing --------- Co-authored-by: Adhiambo Peres <[email protected]> Co-authored-by: Mr. 17 <[email protected]>
…ity (oppia#5548) <!-- READ ME FIRST: Please fill in the explanation section below and check off every point from the Essential Checklist! --> ## Explanation <!-- - Explain what your PR does. If this PR fixes an existing bug, please include - "Fixes #bugnum:" in the explanation so that GitHub can auto-close the issue - when this PR is merged. --> Fixes oppia#5404 This PR migrates deprecated `onBackPressed `usage to `OnBackPressedDispatcher` callback in the RevisonCardActivity and RevisionCardActivityPresenter. ## 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 <!-- Delete these section if this PR does not include UI-related changes. --> If your PR includes UI-related changes, then: - Add screenshots for portrait/landscape for both a tablet & phone of the before & after UI changes - For the screenshots above, include both English and pseudo-localized (RTL) screenshots (see [RTL guide](https://github.com/oppia/oppia-android/wiki/RTL-Guidelines)) - Add a video showing the full UX flow with a screen reader enabled (see [accessibility guide](https://github.com/oppia/oppia-android/wiki/Accessibility-A11y-Guide)) - For PRs introducing new UI elements or color changes, both light and dark mode screenshots must be included - Add a screenshot demonstrating that you ran affected Espresso tests locally & that they're passing
Develop (fork) Branch Update
Develop (fork) branch update
Develop (fork) branch update
Develop (fork) branch update
…omments (oppia#5580) <!-- READ ME FIRST: Please fill in the explanation section below and check off every point from the Essential Checklist! --> ## Explanation <!-- - Explain what your PR does. If this PR fixes an existing bug, please include - "Fixes #bugnum:" in the explanation so that GitHub can auto-close the issue - when this PR is merged. --> Fix oppia#5508 TODO: - [Done] - ~~Update Indents and latest upstream changes~~ ### This PR includes **1. Code Coverage Comment:** - In previous implementations, redundant code coverage reports could accumulate, even when they provided no additional information, leading to cluttered PR threads. - To address this, a comparison step has been added to **check the newly generated code coverage report against the latest posted code coverage comment**. - If the current report is identical to the latest comment, the script will skip posting a new comment. This ensures that the last coverage comment in the PR thread accurately reflects the latest report, preventing unnecessary repetitions. **2. Stats Comment:** - The Stat reports were being generated even when no new changes were made to the PR, causing repetitive APK/AAB report comments and hindering the stale comment checks. - A new step is added to track the presence of new commits. Now, the **stats analysis only triggers if there has been a new commit since the latest APK/AAB report comment**. - This approach reduces redundant analysis, ensuring that builds are only processed when relevant *PR changes are made. ***Limitation:** - These changes aim to help the stale comment checks. However, the trade-off is: merge commits to the `develop` branch will still generate new build reports. Allowing these reports would negate the benefits of stale comment checks, as weekly or bi-weekly merges can cause build variations. - Consequently, the [older method](oppia#5532) of comparing previous and new reports has been removed due to flakiness in the stat.yml. While it is technically possible to use the currently generated report for comparison with the latest comment, it would include variations from merge changes, thus failing to prevent stale comments. Including the comparison step source for reference: ```sh - name: Compare Generated APK & AAB Analysis with the Previous Report if: ${{ steps.track_commits.outputs.new_commits == 'true' }} run: | if [ -f latest_aab_comment_body.log ]; then sed -i -e '$a\' ./develop/brief_build_summary.log sed -i -e '$a\' latest_aab_comment_body.log if diff -B ./develop/brief_build_summary.log latest_aab_comment_body.log > /dev/null; then echo "No significant changes detected; skipping apk aab analysis comment." echo "skip_apk_aab_comment=true" >> $GITHUB_ENV else echo "Changes detected; proceeding with the apk aab analysis comment." diff ./develop/brief_build_summary.log latest_aab_comment_body.log || true echo "skip_apk_aab_comment=false" >> $GITHUB_ENV fi else echo "No previous APK & AAB report posted; Commenting analysed APK & AAB report." echo "skip_apk_aab_comment=false" >> $GITHUB_ENV fi ``` # ### Demonstration >Demonstrated PR: Rd4dev/Oppia-Android-Fork-from-Fork#44 Tested with souce code - [comment_code_coverage.yml](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/blob/4492ab36dc1efbcbec9cf0b7c164d24f9a5d4511/.github/workflows/comment_coverage_report.yml#L3) and [stats.yml](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/blob/4492ab36dc1efbcbec9cf0b7c164d24f9a5d4511/.github/workflows/stats.yml#L3) - Initial Code Coverage Comment | [Comment](Rd4dev/Oppia-Android-Fork-from-Fork#44 (comment)) | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11957616687/job/33335730714#step:5:20) - Initial APK & AAB Analysis Comment | [Comment](Rd4dev/Oppia-Android-Fork-from-Fork#44 (comment)) | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11957621776/job/33337727450#step:20:348) - Redundant Code Coverage Comment Skipped | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11958563394/job/33338783923?pr=44#step:5:20) - Varying Code Coverage Comment Posted | [Comment](Rd4dev/Oppia-Android-Fork-from-Fork#44 (comment)) | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11959332332/job/33341699858#step:5:20) - APK & AAB Analysis Posted with follow up commits | [Comment](Rd4dev/Oppia-Android-Fork-from-Fork#44 (comment)) | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11959338918/job/33340923675#step:20:348) - APK & AAB Analysis Skipped with no follow up commits | [Stack Trace](https://github.com/Rd4dev/Oppia-Android-Fork-from-Fork/actions/runs/11959461908/job/33341312780#step:3:33) ## 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)). --------- Co-authored-by: Ben Henning <[email protected]>
…nter (oppia#5596) <!-- READ ME FIRST: Please fill in the explanation section below and check off every point from the Essential Checklist! --> ## Explanation ### Fixes part of oppia#4865 This is a step towards the whole migration, as proposed we should address this by each feature to make PRs smaller and easy to review. - The changes here are only scoped to classroom. - routes to other features maintain `internalProfileId` until the destination parameter is migrated to `profileId` ## 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)). ## Screen record https://github.com/user-attachments/assets/c474a78f-0331-4b0e-918b-abc07df380c4
APK & AAB differences analysisNote that this is a summarized snapshot. See the CI artifacts for detailed differences. DevExpand to see flavor specificsUniversal APKAPK file size: 19 MiB (old), 19 MiB (new), 1372 bytes (Added) APK download size (estimated): 17 MiB (old), 17 MiB (new), 2101 bytes (Added) Method count: 260202 (old), 260206 (new), 4 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6818 (old), 6818 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 18 MiB (old), 18 MiB (new), 1372 bytes (Added) Configuration hdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 49 KiB (old), 49 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 45 KiB (old), 45 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 86 KiB (old), 86 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 57 KiB (old), 57 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 63 KiB (old), 63 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 48 bytes (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 457 bytes (Added) Method count: 116280 (old), 116284 (new), 4 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 44 bytes (Removed) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 500 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 4259 bytes (Added) Method count: 116286 (old), 116290 (new), 4 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 504 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 368 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 1641 bytes (Added) Method count: 116286 (old), 116290 (new), 4 (Added) Features: 2 (old), 2 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 5786 (old), 5786 (new), 0 (No change)
Lesson assets: 111 (old), 111 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 368 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 44 KiB (old), 44 KiB (new), 0 bytes (No change) Configuration mdpiAPK file size: 38 KiB (old), 38 KiB (new), 0 bytes (No change) Configuration tvdpiAPK file size: 73 KiB (old), 73 KiB (new), 0 bytes (No change) Configuration xhdpiAPK file size: 50 KiB (old), 50 KiB (new), 0 bytes (No change) Configuration xxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) Configuration xxxhdpiAPK file size: 55 KiB (old), 55 KiB (new), 0 bytes (No change) |
Hi @subhajitxyz, 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@subhajitxyz Thanks for this. I think the behaviour looks fine from a product perspective. Only one note: at 0:34 when returning to the card that brought the learner back, shouldn't the previous answer/response be hidden under the "previous answers/responses dropdown"?
@seanlip Apologies for the delayed response. I’ve attempted to replicate the behavior shown in the web PR and video as closely as possible. (it is not hiding the most recent previous answer and response) Currently, the implementation displays only the most recent previous answer and response, while hiding all other incorrect answers and responses. This approach allows the user to focus on their last attempt and understand where they went wrong. Please share your thoughts on this. |
@seanlip , PTAL. |
Unassigning @subhajitxyz since a re-review was requested. @subhajitxyz, please make sure you have addressed all review comments. Thanks! |
@subhajitxyz Sorry for the delay -- just a note that this is on my radar but I am swamped with review requests. Will try to look into it this weekend, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@subhajitxyz Ah yes, you're right. I think the behaviour you have is fine then, thanks! Dismissing my review as not blocking, and confirming this looks good from a product perspective.
Thanks |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @subhajitxyz! I have left a minor suggestion on naming.
} | ||
|
||
/** Sets whether the user should revisit an earlier card. */ | ||
fun turnOnRevisitEarlierCard(value: Boolean) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about: enableRevisitEarlierCard
?
Unassigning @adhiamboperes since the review is done. |
Hi @subhajitxyz, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks! |
@BenHenning, I am assigning this to you for review because I am confused by changes made here in |
Hi @subhajitxyz, 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @subhajitxyz! This is quite an interesting problem, so I have several thoughts on the current approach and some suggestions on how it might be able to be done in a more robust way (and address a part of #5572 that I think might've been missed in this implementation). PTAL.
@@ -699,6 +699,14 @@ class ExplorationProgressController @Inject constructor( | |||
when { | |||
answerOutcome.destinationCase == AnswerOutcome.DestinationCase.STATE_NAME -> { | |||
endState() | |||
// Determines if a revision is required for the user based on the answer outcome. | |||
if (!answerOutcome.labelledAsCorrectAnswer && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that, while this works, I worry the mechanism is potentially fragile because it seems that it's essentially detecting a scenario where we want to redirect back to an earlier state and then triggering an override behavior in StateDeck
to 'catch' this before a real state is pushed onto the stack.
I wonder if this could be done more simply. We could, for example, update the AnswerOutcome
proto to have a new destination such as string previous_state_name
and update computeAnswerOutcomeForResult
to compute this scenario (i.e. the detection is moved to that method, instead).
That would mean a new case needs to be handled here, which actually should allow us to simplify the state deck behavior as well. StateDeck
already supports navigation to/from specific cards. Could we, perhaps, add a method to StateDeck
that's essentially "navigateToMostRecentState()" that takes a state name? That seems like it could be much simpler to manage.
Finally, I think the original issue also asks that we introduce a new blue "Learn Again" button. This would need to replace "Continue" which, in turn, means updating EphemeralState
to include a new type, probably something like CompletedState need_to_revisit_old_card
which would signal to the UI that a different button other than 'Continue' needs to be shown.
I have some other thoughts on how to manage replacing the latest state so that it can be tried again, and possibly some thoughts on the specific state computation. However, I'll leave that for a review after the above is addressed since those will involve rearranging and, hopefully, simplifying much of the logic here (plus adding the necessary new pieces for the button change).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, @BenHenning , for the suggestion. I think it could be done more simply in the way you mentioned above. I will try introducing a new "Learn Again" button.
I would love to hear your thoughts on how to manage replacing the latest state so that it can be tried again, as well as the specific state computation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
. Could we, perhaps, add a method to
StateDeck
that's essentially "navigateToMostRecentState()" that takes a state name? That seems like it could be much simpler to manage.
Actually, I am not fully able to understand how this navigateToMostRecentState() will help us.
After reaching to the old card, Do we want to skip all the previously seen states with one click ?
Explanation
Fixes #5572 :
This PR adds functionality for redirecting learners to previously added card for revision, add softer redirection for learners to get to where they left off, and re-attempt the question.
Essential Checklist
For UI-specific PRs only
Record_2024-12-27-18-18-57_943a62cb4c6fb83e010e1c2e82766a17.mp4