Skip to content
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 #1547, parts of #169, #5344, #5365, #5411: Add data layer support for (multiple) classrooms & topic dependencies, and prepare for #4885 #5398

Merged
merged 10 commits into from
May 29, 2024

Conversation

BenHenning
Copy link
Member

@BenHenning BenHenning commented May 17, 2024

Explanation

Fixes #1547
Fixes part of #169
Fixes part of #5344
Fixes part of #5365
Fixes part of #5411

The main purpose of this PR is to introduce support for multiple classrooms in the data layer of the app (with minimal domain integration to avoid the change extending beyond the lesson structures). However, the PR is also doing a few more things including preparing the Android codebase for introducing an asset download script (#4885) and other peripheral cleanups of code (rather than updating it) that was noticed along the way.

Preparing for multiple classrooms

This addresses part of #5365.

#5344 is introducing support for multiple classrooms in the app. To help prepare for those changes, this PR introduces the necessary data structure and domain loading changes to load a new proto structure:

message ClassroomList {
  repeated ClassroomRecord classrooms = 1;
}
message ClassroomRecord {
  string id = 1;
  map<string, TranslationMapping> written_translations = 2;
  SubtitledHtml translatable_title = 3;
  LessonThumbnail classroom_thumbnail = 4;
  map<string, TopicIdList> topic_prerequisites = 5;
  message TopicIdList {
    repeated string topic_ids = 1;
  }
}

Rather than just a flat topic list. Some important details to note:

Asset priming removal

This addresses part of #169.

PrimeTopicAssetsController and its implementation were responsible for hackily pre-loading all lesson images and audio to be on-device to enable offline support. This was the first attempt at offline support early in the app's development, but it had a few significant drawbacks:

  • It required preloading everything upon first app open. Since it can take a while for loading to occur, some robustness needed to be built in for pausing, cancelling, and resuming. I'm not certain if these were even 100% handled in the current implementation.
  • It doesn't perform strong compatibility checks until you're in the app which means lesson incompatibilities would just cause the app to get stuck rather than addressing it during lesson import time (e.g. via an asset downloader script).
  • It required very significant workarounds to existing loading pipelines that aren't ideal to keep in the codebase long-term (code smell).
  • There's no guarantee the user even has enough disk space to download all the needed assets (particularly audio), or if they'll have sufficient internet connectivity & bandwidth to perform those downloads upon first app open.

This approach was abandoned after the earliest alpha releases for an asset download script (which is now being migrated over to this codebase per #5411.

This removal unfortunately required removing a module that was referenced in a lot of tests throughout the codebase. While the removal itself was fairly simple, it does affect a lot of files.

Other areas changed (but unaffected by tests since these flows didn't have automated tests):

  • SplashActivityPresenter for enabling the downloader to start and block the UI using a dialog box while the downloading occurred.
  • ``AudioPlayerController` for removing the special loading logic for primed audio files (the app now no longer supports loading audio files from disk as we don't yet have a good long-term solution for offline-available audio files due to their significant size).
  • GlideImageLoader for removing support for loading locally cached images (only through this flow; the flow we use for the asset download script uses a different annotation and is still supported).

As alluded to above, two annotations were removed as part of this cleanup:

  • CacheAssetsLocally: a boolean indicating whether the prime download flow should be enabled.
  • TopicListToCache: this specified the list of topics to pre-download if the flow was enabled.

GAE structure cleanup & preparing for asset script

Preparing for #4885 included a few other changes, some of which were nice-to-have:

  • Introducing support for incorporating the protos from https://github.com/oppia/oppia-proto-api (specifically Introduce Oppia proto API v1: Android oppia-proto-api#1 since they are still a work-in-progress).
  • Adding java_proto versions for many of the app's proto structures (since the download script runs in the JVM and doesn't use the javalite proto environment).
  • Removed all of the unused GAE services and models (addressing GaeSolution's 'correct_answer' field should be 'Any?' instead of 'String?' #1547 by essentially making it obsolete), plus their mocks. These were never hooked up, and they're never going to be: the long-term plan for the codebase is to use new proto endpoints that will be added to Oppia web. These Retrofit endpoints have actually been rebuilt and repurposed to be used in the asset download script as a medium-term stop-gap until the permanent proto endpoints can be added to Oppia web.
  • RemoteAuthNetworkInterceptorTest was updated to use a different example service since TopicService has been removed. The services for platform parameters and user feedback are being kept.
  • Some test file & KDoc exemptions have been removed since their corresponding files have been deleted.

Note that the new Java proto targets & oppia-proto-api targets aren't being used yet, but they will be in future PRs. This just provides the basis of support for the asset download script while also helping to split up the code to review across multiple PRs.

Essential Checklist

  • 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: ...".)
  • Any changes to scripts/assets files have their rationale included in the PR explanation.
  • The PR follows the style guide.
  • The PR does not contain any unnecessary code changes from Android Studio (reference).
  • The PR is made from a branch that's not called "develop" and is up-to-date with "develop".
  • The PR is assigned to the appropriate reviewers (reference).

For UI-specific PRs only

This is essentially only data infrastructural except for a couple of points:

  • Topic loading is now happening through a classrooms structure rather than a tropic ID list. Since there's only one test & one production classroom, this shouldn't actually change the UX.
  • An at-app-open flow for predownloading image & audio assets has been removed. This hasn't been used since the very earliest alpha releases of the app, so it won't actually affect any users.
  • Some colors for developer lesson and topic thumbnails were updated--see above.

This doesn't actually finish the new data definitions or properly
integrate them, nor is it updating any local development assets.
Copy link

oppiabot bot commented May 24, 2024

Hi @BenHenning, 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.
If you are still working on this PR, please make a follow-up commit within 3 days (and submit it for review, if applicable). Please also let us know if you are stuck so we can help you!

@oppiabot oppiabot bot added the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 24, 2024
@oppiabot oppiabot bot removed the stale Corresponds to items that haven't seen a recent update and may be automatically closed. label May 26, 2024
@BenHenning BenHenning changed the title Fix part of #5344, part of #5365: Add data layer support for (multiple) classrooms & topic dependencies Fix part of #5344, part of #5365, part of #5411: Add data layer support for (multiple) classrooms & topic dependencies May 26, 2024
Also, prep for other proto dependencies that will be needed by the asset
download script.
This functionality hasn't been used in years, and it's unnecessary.
Plus, some lint fixes.
Copy link
Member Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-reviewed all changes.

Conflicts:
	third_party/BUILD.bazel
	utility/src/main/java/org/oppia/android/util/caching/BUILD.bazel
Copy link
Member Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-reviewed 2 changed files after merge (due to conflicts from develop changes).

@BenHenning BenHenning changed the title Fix part of #5344, part of #5365, part of #5411: Add data layer support for (multiple) classrooms & topic dependencies Fix #1547, parts of #169, #5344, #5365, #5411: Add data layer support for (multiple) classrooms & topic dependencies, and prepare for #4885 May 27, 2024
Copy link
Member Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-reviewed latest changes.

This ensures the fixes explained in
bazelbuild/rules_proto#117 and
bazelbuild/bazel#16192 are included to fix CI
breakages on this branch.
Copy link
Member Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Self-reviewed latest changes (bumping oppia-proto-api version so that CI doesn't try to pull a non-existent version of zlib).

BenHenning added a commit that referenced this pull request May 27, 2024
This pulls in the GCS endpoint from #5398 as part of a broader effort of
splitting it up.
@BenHenning BenHenning marked this pull request as ready for review May 27, 2024 22:46
@BenHenning BenHenning requested review from a team as code owners May 27, 2024 22:46
@BenHenning
Copy link
Member Author

Ah excellent. CI is now passing! @adhiamboperes PTAL.

Copy link
Collaborator

@adhiamboperes adhiamboperes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @BenHenning! The cleanup looks spiff and I have no concerns with the new additions.

@oppiabot oppiabot bot added the PR: LGTM label May 29, 2024
Copy link

oppiabot bot commented May 29, 2024

Hi @BenHenning, this PR is ready to be merged. Please address any remaining comments prior to merging, and feel free to merge this PR once the CI checks pass and you're happy with it. Thanks!

@BenHenning
Copy link
Member Author

Thanks @adhiamboperes! I really appreciate the review. :)

Updating to latest develop & enabling auto-merge.

@BenHenning
Copy link
Member Author

Also @theMr17 I forgot to ask you to review this, so I suggest taking a look at it post-merge and let me know if there are any additions needed for multiple classrooms that won't already be included in your project.

@BenHenning BenHenning enabled auto-merge (squash) May 29, 2024 19:00
@BenHenning BenHenning merged commit 18fa030 into develop May 29, 2024
43 checks passed
@BenHenning BenHenning deleted the update-data-layer-to-support-classrooms branch May 29, 2024 19:42
@BenHenning BenHenning restored the update-data-layer-to-support-classrooms branch June 21, 2024 02:43
@BenHenning BenHenning deleted the update-data-layer-to-support-classrooms branch June 21, 2024 02:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GaeSolution's 'correct_answer' field should be 'Any?' instead of 'String?'
2 participants