Fix part of #6203: Migrate exploration and progress controllers to use ProfileId#6294
Fix part of #6203: Migrate exploration and progress controllers to use ProfileId#6294Kishan8548 wants to merge 20 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@Kishan8548 this PR is being marked as draft because the PR description must contain 'Fixes #' or 'Fixes part of #' for each issue the PR is changing, and each one on its own line with no other text. |
There was a problem hiding this comment.
Pull request overview
This PR migrates the domain-layer exploration/progress controllers from LegacyProfileId (and raw Int internal IDs) to the newer ProfileId schema, while keeping temporary compatibility conversions at app-layer call sites and maintaining the legacy analytics boundary.
Changes:
- Updated domain controllers (exploration data/progress, checkpointing, active time tracking, story progress) to accept/store per-profile state keyed by
ProfileId. - Migrated domain + app tests and app presenters to pass
ProfileId(using boundary conversions where the app layer still holdsLegacyProfileId/Int). - Updated exploration progress listener APIs and implementations to use
ProfileId.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| domain/src/main/java/org/oppia/android/domain/exploration/ExplorationDataController.kt | Migrates exploration start/resume/restart/replay APIs to take ProfileId instead of raw internal IDs. |
| domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgressController.kt | Migrates exploration session/progress flow to ProfileId; keeps analytics boundary via toLegacyProfileId(). |
| domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgress.kt | Updates stored session progress fields to use ProfileId. |
| domain/src/main/java/org/oppia/android/domain/exploration/ExplorationProgressListener.kt | Updates listener signature to accept ProfileId. |
| domain/src/main/java/org/oppia/android/domain/exploration/ExplorationActiveTimeController.kt | Migrates active time aggregation/cache to ProfileId and updates listener override. |
| domain/src/main/java/org/oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointController.kt | Migrates checkpoint persistence/retrieval APIs and cache map key to ProfileId. |
| domain/src/main/java/org/oppia/android/domain/topic/StoryProgressController.kt | Migrates story progress tracking/cache to ProfileId. |
| domain/src/test/java/org/oppia/android/domain/exploration/ExplorationDataControllerTest.kt | Updates tests to pass ProfileId into the controller APIs. |
| domain/src/test/java/org/oppia/android/domain/exploration/ExplorationProgressControllerTest.kt | Updates tests for ProfileId usage across play/replay/resume flows. |
| domain/src/test/java/org/oppia/android/domain/exploration/ExplorationActiveTimeControllerTest.kt | Updates tests to use ProfileId when recording and reading active time. |
| domain/src/test/java/org/oppia/android/domain/exploration/lightweightcheckpointing/ExplorationCheckpointControllerTest.kt | Updates checkpoint tests to ProfileId for controller calls (but still bridges to legacy in helper usage). |
| domain/src/test/java/org/oppia/android/domain/topic/StoryProgressControllerTest.kt | Updates story progress tests to use ProfileId. |
| app/src/main/java/org/oppia/android/app/player/exploration/ExplorationActivityPresenter.kt | Adds boundary conversion to pass ProfileId into domain exploration data APIs. |
| app/src/main/java/org/oppia/android/app/player/state/StateViewModel.kt | Converts legacy-held profile ID to ProfileId before calling domain progress controller. |
| app/src/main/java/org/oppia/android/app/topic/lessons/TopicLessonsFragmentPresenter.kt | Converts profile IDs at boundary for replay/restart/start calls. |
| app/src/main/java/org/oppia/android/app/home/recentlyplayed/RecentlyPlayedFragmentPresenter.kt | Converts profile IDs at boundary for restart/replay calls. |
| app/src/main/java/org/oppia/android/app/story/StoryFragmentPresenter.kt | Converts profile IDs at boundary for start/replay calls. |
| app/src/main/java/org/oppia/android/app/resumelesson/ResumeLessonFragmentPresenter.kt | Converts profile IDs at boundary for restart/resume calls. |
| app/src/main/java/org/oppia/android/app/testing/ExplorationTestActivityPresenter.kt | Wraps raw internal ID into ProfileId for replay. |
| app/src/main/java/org/oppia/android/app/player/state/testing/StateFragmentTestActivityPresenter.kt | Wraps raw internal ID into ProfileId for start/replay/delete APIs. |
| app/src/sharedTest/java/org/oppia/android/app/player/exploration/ExplorationActivityTest.kt | Wraps test-injected internal IDs into ProfileId before calling domain. |
| app/src/test/java/org/oppia/android/app/player/exploration/ExplorationActivityLocalTest.kt | Wraps test internal IDs into ProfileId for domain controller calls. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fun testController_saveCheckpoint_retrieveSavedCheckpoint_isSuccessful() { | ||
| explorationCheckpointTestHelper.saveCheckpointForFractionsStory0Exploration0( | ||
| profileId = firstTestProfile, | ||
| profileId = firstTestProfile.toLegacyProfileId(), | ||
| version = FRACTIONS_STORY_0_EXPLORATION_0_CURRENT_VERSION | ||
| ) |
| AsyncResult.Failure( | ||
| ExplorationCheckpointNotFoundException( | ||
| "Checkpoint with the explorationId $explorationId was not found " + | ||
| "for profileId ${profileId.internalId}." | ||
| "for profileId $profileId." | ||
| ) |
| AsyncResult.Failure( | ||
| ExplorationCheckpointNotFoundException( | ||
| "No saved checkpoint with explorationId ${explorationId!!} found for " + | ||
| "the profileId ${profileId!!.internalId}." | ||
| "the profileId ${profileId!!}." | ||
| ) |
| fun startPlayingNewExploration( | ||
| internalProfileId: Int, | ||
| profileId: ProfileId, | ||
| classroomId: String, | ||
| topicId: String, | ||
| storyId: String, | ||
| explorationId: String | ||
| ): DataProvider<Any?> { |
| fun resumeExploration( | ||
| internalProfileId: Int, | ||
| profileId: ProfileId, | ||
| classroomId: String, | ||
| topicId: String, | ||
| storyId: String, | ||
| explorationId: String, | ||
| explorationCheckpoint: ExplorationCheckpoint |
|
|
||
| private fun logIntoAnalyticsReadyAdminProfile() { | ||
| val rootProfileId = LegacyProfileId.getDefaultInstance() | ||
| val rootProfileId = ProfileId.getDefaultInstance() |
|
Thanks for the thorough review! All these suggestions are valid and have been addressed in the latest commit:
|
…est and ExplorationCheckpointControllerTest
|
@adhiamboperes The |
|
@ShankhanilSaha Would you mind taking a first pass on this? PTAL. |
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), 47 KiB (Removed) APK download size (estimated): 18 MiB (old), 18 MiB (new), 41 KiB (Removed) Method count: 266903 (old), 266006 (new), 897 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 7149 (old), 7119 (new), 30 (Removed)
Lesson assets: 69 (old), 69 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 19 MiB (old), 19 MiB (new), 47 KiB (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), 32 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 28 KiB (Removed) Method count: 119174 (old), 118720 (new), 454 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6084 (old), 6054 (new), 30 (Removed)
Lesson assets: 70 (old), 70 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed)
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), 4 bytes (Removed) BetaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 25 KiB (Removed) Method count: 119181 (old), 118727 (new), 454 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6084 (old), 6054 (new), 30 (Removed)
Lesson assets: 70 (old), 70 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed)
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), 4 bytes (Removed) GaExpand to see flavor specificsUniversal APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed) APK download size (estimated): 10 MiB (old), 10 MiB (new), 27 KiB (Removed) Method count: 119181 (old), 118727 (new), 454 (Removed) Features: 1 (old), 1 (new), 0 (No change) Permissions: 6 (old), 6 (new), 0 (No change) Resources: 6084 (old), 6054 (new), 30 (Removed)
Lesson assets: 70 (old), 70 (new), 0 (No change) AAB differencesExpand to see AAB specificsSupported configurations:
Base APKAPK file size: 11 MiB (old), 11 MiB (new), 32 KiB (Removed)
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), 4 bytes (Removed) |
Neer-rn
left a comment
Neer-rn
left a comment
There was a problem hiding this comment.
Thanks @Kishan8548, the migration work looks solid to me. I checked your changes and they mostly look good. A few things I wouldd like you to look into:
- Please go through all the Copilot comments and try to resolve each of them. Once you have addressed a Copilot comment, you can mark it as resolved. However, please only resolve Copilot comments yourself don’t resolve reviewer comments.
- I re-ran the CI, but it’s still failing. I haven’t got chance to closely yet, but it doesn’t seem like a flaky CI failure to me. It looks like there may be an issue with the code, so please run the tests locally and push again only once the build passes.
- I can see that Task 3 in the issue also mentions
arguments.protochanges, but I don’t see any proto changes in this PR. Is there a reason for that?
|
Hi @Kishan8548, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
… private vals, add TODOs
…ofileId - Change ExplorationActivityParams, ExplorationFragmentArguments, ResumeLessonActivityParams, and ResumeLessonFragmentArguments profile_id fields from LegacyProfileId to ProfileId as required by oppia#6203 Task 3. - Add toProfileIdPreservingZero() at write boundaries (Activity/Fragment createIntent/newInstance functions) and toLegacyProfileId() at read boundaries (onCreate/onCreateView) to preserve app-layer compatibility while the full internal migration proceeds in follow-up work. - Remove now-redundant toProfileIdPreservingZero() call in ExplorationFragmentPresenter since profileId is already ProfileId. - Update ResumeLessonFragmentTest assertion to compare against ProfileId (not LegacyProfileId) since args.profileId is now natively ProfileId.
|
@Neer-rn These two CI failures are caused by pre-existing AAPT2/DataBinding errors in the |
Got it, thanks! I will take a closer look during another review pass. Feel free to assign it back to me when you think it’s ready for review. |
|
Unassigning @Kishan8548 since a re-review was requested. @Kishan8548, please make sure you have addressed all review comments. Thanks! |
|
@Neer-rn PTAL, I think it's ready for review as the whole migration for this task is done. |
Neer-rn
left a comment
Neer-rn
left a comment
There was a problem hiding this comment.
Thanks for the changes. Mostly this PR looks great. I had few small comments. PTAL!!
|
Unassigning @Neer-rn since the review is done. |
|
Hi @Kishan8548, it looks like some changes were requested on this pull request by @Neer-rn. PTAL. Thanks! |
Coverage ReportResultsNumber of files assessed: 80 Passing coverageFiles with passing code coverage
Exempted coverageFiles exempted from coverage
|
|
@Neer-rn PTAL. I've addressed all the review comments and this pr is ready for review. |
|
Unassigning @Kishan8548 since a re-review was requested. @Kishan8548, please make sure you have addressed all review comments. Thanks! |
Neer-rn
left a comment
Neer-rn
left a comment
There was a problem hiding this comment.
LGTM!! Great work kishan. @adhiamboperes PTAL
|
Unassigning @Neer-rn since they have already approved the PR. |
|
Assigning @adhiamboperes for code owner reviews. Thanks! |
Explanation
Fixes part of #6203.
This PR completes the migration for the domain-layer exploration and progress controllers. It migrates these controllers from using
LegacyProfileId(and rawIntinternal IDs) to the newProfileIdschema.1. Domain Layer Controllers Migrated to
ProfileIdExplorationProgressController.kt&ExplorationDataController.kt: Updated all internal state maps, data classes, and method signatures to utilizeProfileIdinstead of legacy types.StoryProgressController.kt,ExplorationCheckpointController.kt, andExplorationActiveTimeController.kt: Signatures and internal progress tracking methods were migrated toProfileId.LearnerAnalyticsLogger.beginExplorationis maintained intentionally usingtoLegacyProfileId()since the logging infrastructure is not yet fully migrated.2.
arguments.protoSchema MigrationMigrated the
profile_idfield in:ExplorationActivityParamsExplorationFragmentArgumentsResumeLessonActivityParamsResumeLessonFragmentArgumentsfrom
LegacyProfileId→ProfileIdas required by Task 3.Added
toProfileIdPreservingZero()at write boundaries (Activity/FragmentcreateIntent()/newInstance()functions) andtoLegacyProfileId()at read boundaries (onCreate()/onCreateView()) in:ExplorationActivityExplorationFragmentResumeLessonActivityResumeLessonFragmentto preserve app-layer compatibility until a follow-up migration.
Removed the now-redundant
toProfileIdPreservingZero()call inExplorationFragmentPresentersinceprofileIdis alreadyProfileId.3. Domain Layer Tests Fully Migrated
ExplorationDataControllerTest.ktExplorationProgressControllerTest.ktExplorationActiveTimeControllerTest.ktExplorationCheckpointControllerTest.ktStoryProgressControllerTest.ktChanges:
Removed all instances of
.internalIdand legacy types.Updated all test helpers to instantiate and pass
ProfileIdobjects natively.ResumeLessonFragmentTest.kt: Updated assertion to compare againstProfileIdinstead ofLegacyProfileIdto reflect the proto field migration.4. App-Layer Boundary Isolation (Temporary Compatibility)
Because the App Layer is slated for migration in a future task, strict type-safe boundaries were introduced at presenter call-sites:
TopicLessonsFragmentPresenter.ktRecentlyPlayedFragmentPresenter.ktStoryFragmentPresenter.ktResumeLessonFragmentPresenter.ktAdded
toProfileIdPreservingZero()to calls accessingExplorationDataController.ExplorationActivityPresenter.kt&StateViewModel.ktRefactored:
deleteExplorationProgressByIdgetOldestExplorationDetailsDataProviderupdateWrittenTranslationContentLanguageMidLessonto convert the internally held
LegacyProfileIdat the boundary.StateFragmentTestActivityPresenter.ktWrapped raw
Intprofile values inProfileId.newBuilder().setInternalId(..).build().App Tests
ExplorationActivityTest.ktExplorationActivityLocalTest.ktSafely wrapped injected test properties (
internalProfileId) inProfileIdprior to domain controller interaction.Essential Checklist