-
Notifications
You must be signed in to change notification settings - Fork 521
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 part of #5025: App and OS Deprecation Milestone 4 - Gate the new Deprecation Dialogs and Add Logic to Display Them #5249
Fix part of #5025: App and OS Deprecation Milestone 4 - Gate the new Deprecation Dialogs and Add Logic to Display Them #5249
Conversation
Hi @adhiamboperes. This PR is ready for review. PTAL |
Hi @kkmurerwa, 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. |
Hi @kkmurerwa, 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 @kkmurerwa!. And apologies for the delay in review.
I have left comments, I think this needs two more review passes before it is merge ready.
app/src/main/java/org/oppia/android/app/notice/DeprecationNoticeActionListener.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/splash/SplashActivityPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/splash/SplashActivityPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/splash/SplashActivityPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/org/oppia/android/app/splash/SplashActivityPresenter.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/onboarding/AppStartupStateController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/onboarding/AppStartupStateController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/onboarding/AppStartupStateController.kt
Outdated
Show resolved
Hide resolved
@adhiamboperes I have made a lot of changes including taking consideration of previous responses when showing the app deprecation dialog. PTAL |
Unassigning @kkmurerwa since a re-review was requested. @kkmurerwa, please make sure you have addressed all review comments. Thanks! |
.../main/java/org/oppia/android/app/notice/ForcedAppDeprecationNoticeDialogFragmentPresenter.kt
Outdated
Show resolved
Hide resolved
app/src/sharedTest/java/org/oppia/android/app/administratorcontrols/AppVersionActivityTest.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/onboarding/AppStartupStateController.kt
Outdated
Show resolved
Hide resolved
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 @kkmurerwa! left some comments especially about test formats. Please see inline.
domain/src/test/java/org/oppia/android/domain/onboarding/AppStartupStateControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/onboarding/AppStartupStateControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/onboarding/DeprecationControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/onboarding/DeprecationControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/onboarding/DeprecationControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/onboarding/DeprecationControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/onboarding/DeprecationControllerTest.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/onboarding/DeprecationControllerTest.kt
Outdated
Show resolved
Hide resolved
@adhiamboperes I have made the suggested changes. PTAL |
Unassigning @kkmurerwa since a re-review was requested. @kkmurerwa, please make sure you have addressed all review comments. 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 @kkmurerwa! This LGTM.
@BenHenning, PTAL for codeowners.
Hi @kkmurerwa, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to ask someone to merge your PR once the CI checks pass and you're happy with it. 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.
As far as I can tell, this doesn't seem to require any review from me. I did take a quick scan and didn't notice anything to note, so going ahead and skipping review.
Deferring to you @adhiamboperes for merging in case I missed something.
Merging this since it LGTM. |
Explanation
Fix part of #5025 - When this PR is merged, it will;
Essential Checklist
For UI-specific PRs only
If your PR includes UI-related changes, then: