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

[Feature Request]: Make Changes to Download Script #5365

Open
adhiamboperes opened this issue Mar 20, 2024 · 1 comment
Open

[Feature Request]: Make Changes to Download Script #5365

adhiamboperes opened this issue Mar 20, 2024 · 1 comment
Assignees
Labels
enhancement End user-perceivable enhancements. Impact: High High perceived user impact (breaks a critical feature or blocks a release). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.

Comments

@adhiamboperes
Copy link
Collaborator

Is your feature request related to a problem? Please describe.

There are changes required in the download script in #4885 to incorporate classroom-specific workflows. Because the script relies on farmiliarity with multiple repositories, we want to streamline some parts of the script in order to simplify work for the GSoC multiple classrooms project.

Describe the solution you'd like

We want to modify the classroom structure so that the topic dependecy graph is baked in to it. We want to make changes to the TopicDto to pass in classroom name and other known information like the dependency graph.

This issue tracks defining the classroom structure and incorporating the topic dependency list into the download script.

Describe alternatives you've considered

N/A

Additional context

N/A

@adhiamboperes adhiamboperes added enhancement End user-perceivable enhancements. Impact: High High perceived user impact (breaks a critical feature or blocks a release). Work: High It's not clear what the solution is. labels Mar 20, 2024
@adhiamboperes adhiamboperes added Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. and removed Work: High It's not clear what the solution is. labels Mar 20, 2024
@BenHenning
Copy link
Sponsor Member

So I've been working on addressing this. While the changes aren't yet final, I think my work will cover all of the following:

  • Introducing a new ClassroomRecord to include the topic dependencies graph & other classroom information.
  • Remove TopicIdList in favor of the above.
  • Updating TopicController and TopicListController to use this dependency graph, and to assume exactly one classroom by default (to load the list of topic IDs).
  • Updating the JSON loading pipeline & affected tests, where needed, to load correctly.
  • Update the download asset script to include:
    • A proper GAE DTO for classroom data.
    • A v2 representation (as part of oppia-proto-api) that correctly encapsulates classroom changes and topic dependencies (this is a WIP and not yet pushed).
    • Correct conversion to the new v1 protos being introduced in the Oppia Android proto folder (see above).

What's NOT being included:

  • Localization work for classroom data (the structures are being added for v2 for correctness, but nothing extra is being done for v1 objects).
  • Classroom-specific tests for the topic loading pipeline.

Note also that the topic dependency graph for test topics will be hardcoded in the script as we don't yet have a way to load them from a local instance of Oppia web (though this is a long-term plan).

The above should remove any need to make changes to the download script, ensure a working app version with classroom data, and remove a pretty large chunk of the classroom changes in the domain and data layers (but not all of it).

BenHenning added a commit that referenced this issue May 29, 2024
… for (multiple) classrooms & topic dependencies, and prepare for #4885 (#5398)

## 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:

```proto
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:
- The recommended topics structure has been updated to use this new
``topic_prerequisites`` value being loaded from classrooms. This will
also extend to production assets since the asset download script from
#4885 is also being updated to include support for this multiple
classrooms structure to address the remainder of #5365.
- To minimize domain changes, the new loading code assumes only **one**
classroom is present. TODOs have been added on #5344 to extend this to
support multiple classrooms.
- Current loading code (including for JSON) is ignoring all but:
``classrooms``, ``id``, and ``topic_prerequisites`` (including
``topic_ids``) from the protos above. These other fields are expected to
have supported added as part of #5344.
- There were some color simplifications made in ``TopicListController``.
These largely shouldn't have a major impact outside of developer assets.
These changes were made to ensure non-specificity to the previous lesson
classroom. In general, all of this should eventually be removed in favor
of loading colors from lesson assets, but that will need to wait until
the JSON pipeline is completely removed.

### 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
oppia/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 #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
- [x] 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: ...".)
- [x] Any changes to
[scripts/assets](https://github.com/oppia/oppia-android/tree/develop/scripts/assets)
files have their rationale included in the PR explanation.
- [x] The PR follows the [style
guide](https://github.com/oppia/oppia-android/wiki/Coding-style-guide).
- [x] The PR does not contain any unnecessary code changes from Android
Studio
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#undo-unnecessary-changes)).
- [x] The PR is made from a branch that's **not** called "develop" and
is up-to-date with "develop".
- [x] The PR is **assigned** to the appropriate reviewers
([reference](https://github.com/oppia/oppia-android/wiki/Guidance-on-submitting-a-PR#clarification-regarding-assignees-and-reviewers-section)).

## 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement End user-perceivable enhancements. Impact: High High perceived user impact (breaks a critical feature or blocks a release). Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet.
Development

No branches or pull requests

2 participants