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

GaeSolution's 'correct_answer' field should be 'Any?' instead of 'String?' #1547

Closed
miaboloix opened this issue Jul 30, 2020 · 5 comments · Fixed by #5398
Closed

GaeSolution's 'correct_answer' field should be 'Any?' instead of 'String?' #1547

miaboloix opened this issue Jul 30, 2020 · 5 comments · Fixed by #5398
Assignees
Labels
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.

Comments

@miaboloix
Copy link
Contributor

Is your feature request related to a problem? Please describe.
According to state_domain.py, correct_answer in GaeSolution.kt should not be typecast as 'String' but as 'Any'.

Describe the solution you'd like
The type declared in GaeSolution.kt for correct_answer must be changed to Any?. On top of this, in exploration.proto we must cast correct_answer to be an InteractionObject and must change StateRetreiver.kt in order to parse the field correctly.

@miaboloix miaboloix self-assigned this Jul 30, 2020
@miaboloix miaboloix added this to the Stage 2 of Bazel Migration milestone Aug 10, 2020
@BenHenning BenHenning assigned BenHenning and unassigned miaboloix Feb 19, 2021
@Broppia Broppia added issue_type_bug Impact: Low Low perceived user impact (e.g. edge cases). labels Jul 29, 2022
@BenHenning BenHenning added issue_type_infrastructure Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Z-ibt Temporary label for Ben to keep track of issues he's triaged. and removed issue_type_bug labels Sep 15, 2022
@BenHenning BenHenning removed this from the Stage 2 of Bazel Migration milestone Sep 16, 2022
@seanlip seanlip added bug End user-perceivable behaviors which are not desirable. and removed issue_type_infrastructure labels Mar 28, 2023
@seanlip
Copy link
Member

seanlip commented Mar 28, 2023

Actually, it should be cast to a union of possible answer types -- Any is a bit broad. Note that the typing in Oppia-Web (see link in the issue description) has been made stricter since this issue was filed.

@BenHenning
Copy link
Sponsor Member

This will actually become obsolete in #4885 because that will be removing the JSON endpoints from the app proper and basically shift the app entirely over to using protos only (which also are a bit non-specific in terms of solution typing, but that's fully fixed in oppia/oppia-proto-api#1).

@seanlip
Copy link
Member

seanlip commented Apr 4, 2023

@BenHenning #4885 is closed, is that intended?

Also, based on your comment, can we just close this issue, or add "Fix #1547" to the PR title of #4885 if we still want to keep it open?

@BenHenning
Copy link
Sponsor Member

BenHenning commented Apr 6, 2023

@seanlip #4885 will be reopened once I have time to focus on it again, but I won't lose track of it (I'd rather not keep it open only to have Oppiabot close it again, though; currently load-balancing 7 PRs & counting).

I added a reference in the PR to this issue, and I will for sure update it once I finalize the PR to make it more clear (as I've done with past issues).

@seanlip
Copy link
Member

seanlip commented Apr 6, 2023

OK, just having the reference is good enough so that this gets auto-closed at the right time. Thanks!

@MohitGupta121 MohitGupta121 added the Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. label Jun 16, 2023
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
bug End user-perceivable behaviors which are not desirable. Impact: Low Low perceived user impact (e.g. edge cases). Issue: Needs Clarification Indicates that an issue needs more detail in order to be able to be acted upon. Work: Medium The means to find the solution is clear, but it isn't at good-first-issue level yet. Z-ibt Temporary label for Ben to keep track of issues he's triaged.
5 participants