-
Notifications
You must be signed in to change notification settings - Fork 534
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 2 - Add protos and the DeprecationController #4999
Fix part of #5025: App and OS Deprecation Milestone 2 - Add protos and the DeprecationController #4999
Conversation
…on controller and a test for the controller
Hey @BenHenning and @adhiamboperes. Here is the PR for the second milestone of the App/OS Deprecation Feature. PTAL. |
Thanks @kkmurerwa! I'm going to be delegating this over to @adhiamboperes to approve since she'll be stepping in for me for codeowners starting this week through all of June. |
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.
I have only reviewed the protos since there are changes that may cause you to update some parts of your implementation. I have suggested a way to structure your deprecation responses, and also requested changes to the field documentation since some of the comments are not clear to me. I think we should aim for clarity on how these models are used because this is impotant for maintenability.
Unassigning @adhiamboperes since the review is done. |
Hi @kkmurerwa, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks! |
…n the function of the different models and enums
Hey @adhiamboperes. This is ready for another pass. PTAL. |
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.
Hi @kkmurerwa, I have requested some changes, most of which are suggestins for improvements. PTAL, thanks.
domain/src/main/java/org/oppia/android/domain/onboarding/DeprecationController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/onboarding/DeprecationController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/onboarding/DeprecationController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/onboarding/DeprecationController.kt
Outdated
Show resolved
Hide resolved
Unassigning @adhiamboperes since the review is done. |
Hi @kkmurerwa, it looks like some changes were requested on this pull request by @adhiamboperes. PTAL. Thanks! |
…erwa/oppia-android into app-and-os-deprecation-milestone-2
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.
Hi @kkmurerwa, I have left a few further comments+some previoujs comments are not yet resolved. PTAL!
domain/src/main/java/org/oppia/android/domain/onboarding/DeprecationController.kt
Outdated
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/onboarding/DeprecationController.kt
Outdated
Show resolved
Hide resolved
domain/src/test/java/org/oppia/android/domain/onboarding/DeprecationControllerTest.kt
Show resolved
Hide resolved
domain/src/main/java/org/oppia/android/domain/onboarding/DeprecationController.kt
Outdated
Show resolved
Hide resolved
Hi @adhiamboperes. I have pushed an updated commit. Let me know if something else is remaining. |
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.
Hi @kkmurerwa I have left one additional comment, but otherwise this LGTM.
domain/src/main/java/org/oppia/android/domain/onboarding/DeprecationController.kt
Outdated
Show resolved
Hide resolved
Unassigning @adhiamboperes since they have already approved the PR. |
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! |
Explanation
Fix part of #5025: When this PR is merged, it will;
deprecation.proto
file that will allow the storage of deprecation responses as well as provide the various deprecation types.OPTIONAL_UPDATE_AVAILABLE
and theOS_IS_DEPRECATED
startup modes on theonboarding.proto
file for the two new startup modes being introduced.DeprecationController
class and add tests for the class in theDeprecationControllerTest
.DeprecationControllerTest
to theOppiaParameterizedTestRunner
file exemptions on thefile_content_validation_checks.textproto
.Essential Checklist