Fix part of #6203: Migrate domain layer from LegacyProfileId to ProfileId#6254
Fix part of #6203: Migrate domain layer from LegacyProfileId to ProfileId#6254Kishan8548 wants to merge 14 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Migrates domain-layer controller APIs from deprecated LegacyProfileId to ProfileId, while keeping app/UI layer mostly on LegacyProfileId via boundary conversions (toProfileIdPreservingZero() / toLegacyProfileId()).
Changes:
- Updated
ProfileManagementController&TranslationControllerpublic APIs to acceptProfileId. - Added boundary conversions across domain/app call sites where
LegacyProfileIdis still used. - Updated Bazel targets & tests to compile against the migrated APIs.
Reviewed changes
Copilot reviewed 62 out of 62 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| testing/src/main/java/org/oppia/android/testing/profile/ProfileTestHelper.kt | Converts Legacy->ProfileId when calling migrated domain APIs in tests. |
| domain/src/test/java/org/oppia/android/domain/translation/TranslationControllerTest.kt | Updates tests to use ProfileId in translation controller calls. |
| domain/src/test/java/org/oppia/android/domain/topic/TopicControllerTest.kt | Converts Legacy->ProfileId at translation controller boundary in tests. |
| domain/src/test/java/org/oppia/android/domain/profile/ProfileManagementControllerTest.kt | Updates tests/constants to use ProfileId, adjusts analytics assertions. |
| domain/src/main/java/org/oppia/android/domain/translation/TranslationController.kt | Migrates translation controller API to ProfileId and converts to legacy for persistence. |
| domain/src/main/java/org/oppia/android/domain/translation/BUILD.bazel | Adds profile id migration util dep. |
| domain/src/main/java/org/oppia/android/domain/topic/TopicListController.kt | Converts Legacy->ProfileId when calling translation controller. |
| domain/src/main/java/org/oppia/android/domain/topic/TopicController.kt | Converts Legacy->ProfileId when calling translation controller. |
| domain/src/main/java/org/oppia/android/domain/survey/SurveyGatingController.kt | Converts Legacy->ProfileId for migrated profile management calls. |
| domain/src/main/java/org/oppia/android/domain/survey/BUILD.bazel | Adds profile id migration util dep. |
| domain/src/main/java/org/oppia/android/domain/question/QuestionAssessmentProgressController.kt | Converts Legacy->ProfileId when calling translation controller. |
| domain/src/main/java/org/oppia/android/domain/profile/ProfileManagementController.kt | Migrates profile management controller API to ProfileId; converts to legacy for analytics. |
| domain/src/main/java/org/oppia/android/domain/profile/BUILD.bazel | Adds profile id migration util dep. |
| domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/BUILD.bazel | Adds profile id migration util dep(s). |
| domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/ApplicationLifecycleObserver.kt | Converts ProfileId->LegacyProfileId for analytics logging method. |
| domain/src/main/java/org/oppia/android/domain/oppialogger/analytics/AnalyticsController.kt | Converts Legacy->ProfileId when reading translation prefs. |
| domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgressController.kt | Converts Legacy->ProfileId for migrated profile/translation calls. |
| domain/src/main/java/org/oppia/android/domain/exploration/ExplorationDataController.kt | Converts Legacy->ProfileId for translation locale retrieval. |
| domain/src/main/java/org/oppia/android/domain/classroom/ClassroomController.kt | Converts Legacy->ProfileId for translation locale retrieval. |
| domain/BUILD.bazel | Adds profile id migration util dep. |
| app/src/main/java/org/oppia/android/app/walkthrough/welcome/WalkthroughWelcomeFragmentPresenter.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardFragmentPresenter.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/topic/revisioncard/RevisionCardActivityPresenter.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/topic/questionplayer/QuestionPlayerActivityPresenter.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/survey/SurveyWelcomeDialogFragmentPresenter.kt | Converts Legacy->ProfileId for survey timestamp update. |
| app/src/main/java/org/oppia/android/app/splash/SplashActivityPresenter.kt | Converts Legacy->ProfileId for login. |
| app/src/main/java/org/oppia/android/app/settings/profile/ProfileResetPinFragmentPresenter.kt | Converts Legacy->ProfileId before calling migrated updatePin. |
| app/src/main/java/org/oppia/android/app/settings/profile/ProfileRenameFragmentPresenter.kt | Updates profile id type used for updateName call. |
| app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditViewModel.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/settings/profile/ProfileEditFragmentPresenter.kt | Mix of direct ProfileId construction and Legacy->ProfileId conversion for migrated calls. |
| app/src/main/java/org/oppia/android/app/resumelesson/ResumeLessonActivityPresenter.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/profileprogress/ProfileProgressViewModel.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/profileprogress/ProfileProgressActivityPresenter.kt | Converts Legacy->ProfileId for avatar update. |
| app/src/main/java/org/oppia/android/app/profileprogress/ProfilePictureActivityPresenter.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/profile/ResetPinDialogFragmentPresenter.kt | Converts Legacy->ProfileId before updatePin. |
| app/src/main/java/org/oppia/android/app/profile/ProfileLoginFragmentPresenter.kt | Converts Legacy->ProfileId for profile retrieval and login. |
| app/src/main/java/org/oppia/android/app/profile/ProfileChooserFragmentPresenterV1.kt | Converts Legacy->ProfileId for initializeLearnerId and login. |
| app/src/main/java/org/oppia/android/app/profile/ProfileChooserFragmentPresenter.kt | Converts Legacy->ProfileId for onboarding end, initializeLearnerId, and login. |
| app/src/main/java/org/oppia/android/app/profile/PinPasswordViewModel.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/profile/PinPasswordActivityPresenter.kt | Converts Legacy->ProfileId for login. |
| app/src/main/java/org/oppia/android/app/profile/AdminPinActivityPresenter.kt | Converts Legacy->ProfileId for updatePin. |
| app/src/main/java/org/oppia/android/app/profile/AdminAuthActivityPresenter.kt | Converts Legacy->ProfileId for login. |
| app/src/main/java/org/oppia/android/app/player/state/StateViewModel.kt | Converts Legacy->ProfileId for translation/profile calls. |
| app/src/main/java/org/oppia/android/app/player/exploration/ExplorationManagerFragmentPresenter.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/player/exploration/ExplorationFragmentPresenter.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/player/audio/AudioFragmentPresenter.kt | Converts Legacy->ProfileId for audio language retrieval. |
| app/src/main/java/org/oppia/android/app/options/OptionsFragmentPresenter.kt | Converts Legacy->ProfileId for reading size / language / audio updates. |
| app/src/main/java/org/oppia/android/app/options/OptionControlsViewModel.kt | Converts Legacy->ProfileId for translation/profile providers. |
| app/src/main/java/org/oppia/android/app/options/AudioLanguageSelectionViewModel.kt | Converts Legacy->ProfileId for audio preselection. |
| app/src/main/java/org/oppia/android/app/options/AppLanguageFragmentPresenterV1.kt | Converts Legacy->ProfileId for app language updates. |
| app/src/main/java/org/oppia/android/app/options/AppLanguageFragmentPresenter.kt | Converts Legacy->ProfileId for app language updates. |
| app/src/main/java/org/oppia/android/app/onboarding/OnboardingFragmentPresenter.kt | Converts Legacy->ProfileId for app language updates. |
| app/src/main/java/org/oppia/android/app/onboarding/IntroFragmentPresenter.kt | Converts Legacy->ProfileId for onboarding started. |
| app/src/main/java/org/oppia/android/app/onboarding/CreateProfileFragmentPresenter.kt | Converts Legacy->ProfileId for profile details update. |
| app/src/main/java/org/oppia/android/app/onboarding/AudioLanguageFragmentPresenter.kt | Converts Legacy->ProfileId for translation updates and login. |
| app/src/main/java/org/oppia/android/app/onboarding/AdminIntroFragmentPresenter.kt | Converts Legacy->ProfileId for onboarding started. |
| app/src/main/java/org/oppia/android/app/home/HomeViewModel.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/home/HomeFragmentPresenter.kt | Converts Legacy->ProfileId for profile retrieval and onboarding end. |
| app/src/main/java/org/oppia/android/app/drawer/NavigationDrawerFragmentPresenter.kt | Converts Legacy->ProfileId for profile retrieval. |
| app/src/main/java/org/oppia/android/app/classroom/ClassroomListViewModel.kt | Converts Legacy->ProfileId for profile retrieval and classroom id preference retrieval. |
| app/src/main/java/org/oppia/android/app/classroom/ClassroomListFragmentPresenter.kt | Converts Legacy->ProfileId for profile retrieval, preference update, onboarding end. |
| app/src/main/java/org/oppia/android/app/administratorcontrols/administratorcontrolsitemviewmodel/AdministratorControlsDownloadPermissionsViewModel.kt | Converts Legacy->ProfileId for permission updates. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@BenHenning , PTAL |
|
@Neer-rn, PTAL for a first pass. @Kishan8548, just FYI for the human reviews, don't resolve the reviewer comments. But it is fine to do so for the copilot reviews. |
|
@Neer-rn Fixed the failing checks/tests and pushed the latest updates. PTAL. |
|
@Neer-rn i fixed some failing tests, please give workflow approval |
|
@Kishan8548, are you running your tests locally before pushing to CI? |
@adhiamboperes, I wasn't running the full Robolectric suite locally as it's very slow with Bazel. The failing tests were in test files that called the APIs I migrated but were still passing |
@Kishan8548, you will never need to run all the tests locally. Only run the targets affected by your changes, and open a discussion if you are facing problems running tests locally. See wiki. |
…oProfileIdPreservingZero() at migrated API call sites
|
Thanks for the work @Kishan8548! I am looking at your changes form quiet few days. The code looks good, tests pass locally and app runs fine on my emulator. Few things I noticed before second pass:
Also a few migrated methods are not mentioned at all. Can you update the description?
Also if you are worried about the failing CI checks I went though them and don't worry those are because first time contributor workflows need maintainer approval. They will turn green once we pass it. After you make these changes please assign this to @adhiamboperes for the final pass. |
…ppLanguageWatcherMixinTest - AudioPlayerControllerTest: loginToProfile() now expects ProfileId; add .toProfileIdPreservingZero() conversion for rootProfileId (LegacyProfileId) and add the required import. - AppLanguageWatcherMixinTest: ProfileId.internal_id is declared as 'optional int32' in proto3. This means ProfileId.getDefaultInstance() (field unset, serializes to empty bytes) is NOT equal to ProfileId.newBuilder().setInternalId(0).build() (field explicitly set, serializes to 0x08 0x00). The mixin uses getCurrentProfileId() which calls newBuilder().setInternalId(0).build(), so the test must use the same construction to match the appLanguageCacheStoreMap key and trigger the correct cache-store observer. Part of issue oppia#6203 (migrate LegacyProfileId -> ProfileId).
Hi @Neer-rn, thanks for the detailed review! I had university exams ongoing over the past couple of weeks and wasn't able to respond sooner — I've now gone through all the points:
I'll push the fixes shortly and reassign the PR to you for a final pass. Thanks again! |
…Zero() at API boundaries ProfileRenameFragmentPresenter, ProfileEditFragmentPresenter, and ProfileEditFragmentTest were incorrectly using ProfileId.newBuilder() directly. Per the migration pattern used throughout the rest of this PR, settings/profile (Task 8) should keep LegacyProfileId construction inside the presenter and only convert at the domain-API boundary using .toProfileIdPreservingZero(). Part of issue oppia#6203.
|
@Kishan8548 left a small comment above, otherwise this PR LGTM! |
Addressed review comment: removed unnecessary blank lines inside the function call brackets around toProfileIdPreservingZero() calls. Part of issue oppia#6203.
|
@Neer-rn Done! Removed the blank lines inside the brackets in all similar spots in |
|
Thanks for the work, @Kishan8548. Just one thing again: please don’t resolve the human reviewer’s comments yourself. You should either reply to the comment or leave it as it is. The reviewer will mark it as resolved if they are satisfied. For now, this was a simple issue, but sometimes the issue might not actually be resolved and may need to be brought up again. This has already been mentioned before so please take care of this. Other than that, it LGTM from my side. Over to you, @adhiamboperes PTAL |
|
Thanks for the reviews! @Neer-rn |
| profileId | ||
| ) { translationController.getAppLanguageSelection(it.toProfileIdPreservingZero()) } | ||
| ?.let { this.appLanguageSelection = it } |
There was a problem hiding this comment.
Instead of using lambdas as a workaround to pass params in resolveProfileOperation the function itself should be migrated since it exists in the same layer and how you did it adds unnecessary same layer conversion.
There was a problem hiding this comment.
Done! Migrated resolveProfileOperation to accept ProfileId? directly. The conversion now happens once at the call site via profileId?.toProfileIdPreservingZero() before being passed in, so the lambdas no longer need the same-layer conversion.
| profileId | ||
| ) { | ||
| translationController.getWrittenTranslationContentLanguageSelection( | ||
| it.toProfileIdPreservingZero() | ||
| ) | ||
| } | ||
| ?.let { this.writtenTranslationLanguageSelection = it } |
There was a problem hiding this comment.
Fixed here as well — same approach.
| profileId | ||
| ) { | ||
| translationController.getAudioTranslationContentLanguageSelection( | ||
| it.toProfileIdPreservingZero() | ||
| ) | ||
| } | ||
| ?.let { this.audioTranslationLanguageSelection = it } |
There was a problem hiding this comment.
Fixed here as well — same approach.
|
Hi @Kishan8548, it looks like some changes were requested on this pull request by @ShankhanilSaha. PTAL. Thanks! |
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), 2752 bytes (Removed) APK download size (estimated): 18 MiB (old), 18 MiB (new), 6001 bytes (Removed) Method count: 265520 (old), 265379 (new), 141 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 7119 (old), 7119 (new), 0 (No change)
Lesson assets: 113 (old), 113 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 19 MiB (old), 19 MiB (new), 2756 bytes (Removed) 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: 46 KiB (old), 46 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: 64 KiB (old), 64 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 236 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 863 bytes (Removed) Method count: 118637 (old), 118615 (new), 22 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6054 (old), 6054 (new), 0 (No change)
Lesson assets: 114 (old), 114 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 232 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 45 KiB (old), 45 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), 180 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 989 bytes (Added) Method count: 118641 (old), 118619 (new), 22 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6054 (old), 6054 (new), 0 (No change)
Lesson assets: 114 (old), 114 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 180 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 45 KiB (old), 45 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), 216 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 122 bytes (Removed) Method count: 118641 (old), 118619 (new), 22 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6054 (old), 6054 (new), 0 (No change)
Lesson assets: 114 (old), 114 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 220 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 45 KiB (old), 45 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) |
| this.priority = priority | ||
| this.context = context | ||
| profileId?.let { this.profileId = it } | ||
| val profileIdNew = profileId?.toProfileIdPreservingZero() |
There was a problem hiding this comment.
No need to create this variable, profileId?.toProfileIdPreservingZero() can be passed directly in resolveProfileOperation(). Adding this adds more stuff to clear up during cleanup after complete migration.
There was a problem hiding this comment.
Done, removed the intermediate variable — passing profileId?.toProfileIdPreservingZero() directly at each call site now.
| profileIdNew | ||
| ) { translationController.getWrittenTranslationContentLanguageSelection(it) } |
| profileIdNew | ||
| ) { translationController.getAudioTranslationContentLanguageSelection(it) } |
| profileIdNew | ||
| ) { translationController.getAppLanguageSelection(it) } |
There was a problem hiding this comment.
You are still using the lambda, I would prefer that the params are passed how they are passed right now in the develop branch and since resolveProfileOperation() is updated in this PR it should not be an issue to use the old way.
Thus, it should look something like
resolveProfileOperation(
profileId?.toProfileIdPreservingZero(),
translationController::getAudioTranslationContentLanguageSelection
)
There was a problem hiding this comment.
Sorry for the confusion in the previous commit. Fixed now — using method references and passing profileId?.toProfileIdPreservingZero() directly as suggested.
|
Thanks @ShankhanilSaha, your comments make sense. Since you have more context on this migration, wanted to quickly check one more thing with you. I noticed the earlier Copilot comment about I checked the current call sites and most seem to use |
|
@Neer-rn leaving it for later should be fine for now. |
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), 2824 bytes (Removed) APK download size (estimated): 18 MiB (old), 18 MiB (new), 4868 bytes (Removed) Method count: 265520 (old), 265378 (new), 142 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 7119 (old), 7119 (new), 0 (No change)
Lesson assets: 113 (old), 113 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 19 MiB (old), 19 MiB (new), 2828 bytes (Removed) 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: 46 KiB (old), 46 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: 64 KiB (old), 64 KiB (new), 0 bytes (No change) AlphaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 320 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 35 bytes (Added) Method count: 118637 (old), 118614 (new), 23 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6054 (old), 6054 (new), 0 (No change)
Lesson assets: 114 (old), 114 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 320 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 45 KiB (old), 45 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), 268 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 3 bytes (Added) Method count: 118641 (old), 118618 (new), 23 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6054 (old), 6054 (new), 0 (No change)
Lesson assets: 114 (old), 114 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 264 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 45 KiB (old), 45 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), 244 bytes (Added) APK download size (estimated): 10 MiB (old), 10 MiB (new), 1075 bytes (Added) Method count: 118641 (old), 118618 (new), 23 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6054 (old), 6054 (new), 0 (No change)
Lesson assets: 114 (old), 114 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 248 bytes (Added) Configuration hdpiAPK file size: 43 KiB (old), 43 KiB (new), 0 bytes (No change) Configuration ldpiAPK file size: 45 KiB (old), 45 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) |
There was a problem hiding this comment.
Thanks @Kishan8548!
I have reviewed about 60 of the 82 files, and I had to stop becase of some confusion.
Many files in the app layer have been refactored to use the migration util, which is expected. However, some of the domain ones have too, like the AnalyticsController.kt.
Morever, some of the tests are switching to ProfileId, but as far as I could tell, their corresponding presenter/viewmodels were still using the LegacyProfileId with the migration util.
I have reread the PR description and it says that the domain layer controllers are expected to migrate away from LegacyProfileId.
Could you please clarify the changes expected in the PR?
| profileManagementController.markProfileOnboardingEnded( | ||
| profileId.toProfileIdPreservingZero() | ||
| ) |
There was a problem hiding this comment.
In this case, it is okay to use the toProfileIdUnsetIfZero util because an additional profile can never be zero(since they must be created by an existing admin).
There was a problem hiding this comment.
Ditto in the HomeFragmentPresenter
There was a problem hiding this comment.
Done! Switched to toProfileIdUnsetIfZero in both ClassroomListFragmentPresenter and HomeFragmentPresenter for markProfileOnboardingEnded calls since non-admin profiles can never have internalId=0.
There was a problem hiding this comment.
Please recheck, only the ADDITIONAL_LEARNER typecase should use toProfileIdUnsetIfZero. The SOLE_LEARNER case above is also an admin and can be zero. This is a new pathway, and is not well documented yet, so apologies for the confusion. Please fix that in the HomeFragmentPresenter as well.
- Use toProfileIdUnsetIfZero for markProfileOnboardingEnded in ClassroomListFragmentPresenter and HomeFragmentPresenter since non-admin profiles can never have internalId=0 - Revert direct ProfileId construction in AudioLanguageFragmentTest, ExplorationActivityLocalTest, ActivityLanguageLocaleHandlerTest to use LegacyProfileId + toProfileIdPreservingZero pattern Part of issue oppia#6203.
Part of issue oppia#6203.
|
@adhiamboperes Thanks for the detailed review! To clarify the PR scope:
|
adhiamboperes
left a comment
There was a problem hiding this comment.
Thanks @Kishan8548!
I reviewed the remaining files and left a correction. PTAL. Also wait to see if the current CI passes before pushing a new commit to hopefully speed up the next review cycle.
| profileManagementController.markProfileOnboardingEnded( | ||
| profileId.toProfileIdPreservingZero() | ||
| ) |
There was a problem hiding this comment.
Please recheck, only the ADDITIONAL_LEARNER typecase should use toProfileIdUnsetIfZero. The SOLE_LEARNER case above is also an admin and can be zero. This is a new pathway, and is not well documented yet, so apologies for the confusion. Please fix that in the HomeFragmentPresenter as well.
|
@adhiamboperes Done! Fixed in both |
Explanation
Fixes part of #6203
This PR migrates the domain-layer controller public APIs from the deprecated
LegacyProfileIdto the newProfileIdproto. The UI layer (ViewModels, Fragments, and Presenters) is intentionally left unchanged in this PR as part of a gradual migration strategy — boundary conversions usingtoProfileIdPreservingZero()are applied at all call sites where the UI still usesLegacyProfileId.Domain layer changes (migrated to
ProfileId)ProfileManagementController:loginToProfile,getProfile,getAudioLanguage,markProfileOnboardingStarted,markProfileOnboardingEnded,updateName,updatePin,updateProfileAvatar,updateProfileType,updateAllowDownloadAccess,updateEnableInLessonQuickLanguageSwitching,updateAudioLanguage,updateReadingTextSize,updateWifiPermissionDeviceSettings,updateTopicAutomaticallyPermissionDeviceSettings,updateNewProfileDetails,initializeLearnerId,deleteProfile,getCurrentProfileId,fetchLearnerId,fetchContinueAnimationSeenStatus,markContinueButtonAnimationSeen,retrieveSurveyLastShownTimestamp,updateSurveyLastShownTimestamp,updateLastSelectedClassroomId,retrieveLastSelectedClassroomIdTranslationController:getAppLanguage,getAppLanguageLocale,getAppLanguageSelection,getWrittenTranslationContentLanguage,getWrittenTranslationContentLocale,getWrittenTranslationContentLanguageSelection,getAudioLanguagePreselection,getAudioTranslationContentLanguage,getAudioTranslationContentLocale,getAudioTranslationContentLanguageSelection,updateAppLanguage,updateWrittenTranslationContentLanguage,updateAudioTranslationContentLanguageApp-layer boundary conversions
Applied
.toProfileIdPreservingZero()at all unmigrated call sites in:AnalyticsController,TopicController,TopicListController,ClassroomController,ExplorationDataController,ExplorationProgressController,QuestionAssessmentProgressController,SurveyGatingController,ApplicationLifecycleObserver, and ~50 UI Fragments/ViewModels/Presenters.BUILD.bazel updates
Added
//utility/src/main/java/org/oppia/android/util/profile:profile_id_migration_utildependency to all affected Bazel targets.Tests updated
Domain tests:
ProfileManagementControllerTestTranslationControllerTestTopicControllerTestExplorationProgressControllerTestAnalyticsControllerTestApplicationLifecycleObserverTestQuestionAssessmentProgressControllerTestSurveyGatingControllerTestAudioPlayerControllerTestApp tests:
ActivityLanguageLocaleHandlerTestAppLanguageLocaleHandlerTestAppLanguageResourceHandlerTestAppLanguageWatcherMixinTestExplorationActivityLocalTestStateFragmentLocalTestAudioLanguageFragmentTestExplorationActivityTestStateFragmentTestProfileEditFragmentTestConceptCardFragmentTestQuestionPlayerActivityTestRevisionCardActivityTestRevisionCardFragmentTestNote: This supersedes #6253 which was closed due to an accidental force push.
Essential Checklist
scripts/assetsfiles have their rationale included in the PR explanation.developand is up-to-date withdevelop.