-
Notifications
You must be signed in to change notification settings - Fork 716
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
move MeteredConnectionNotificationModal to Library Page & conditionally render "Other Libraries" #11683
move MeteredConnectionNotificationModal to Library Page & conditionally render "Other Libraries" #11683
Conversation
…p; add and evaluate allowDownloadOnMeteredConnection logic; add conditions for rendering OtherLibraries
Build Artifacts
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code here makes sense, although I did make one nice-to-have/non-blocking comment suggestion.
I have had some trouble running in app mode that I have been unable to resolve so far, but I will come back to this in the afternoon and see if I can troubleshoot to finish the manual QA portion. Otherwise will ask QA team to review
if (this.allowDownloadOnMeteredConnection) { | ||
return true; | ||
} | ||
return !this.usingMeteredConnection; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It took me a bit to understand why !this.usingMeteredConnection
was the default return here, even though when I realized, it felt "oh, duh!". The other conditional checks are very easy to follow just by reading through, and I think a very brief comment above would make this easier to understand... even something as basic (and somewhat redundant) as
// other libraries shouldn't display on metered connections unless explicitly permitted
or something along these lines
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Following up to confirm that modal pops up correctly on Library page as it should, and that the studio library doesn't load if "do not allow mobile data" selection is made. going back to the device settings and updating, the studio library then appears
f730c3d
into
learningequality:release-v0.16.x
Hi @rtibbles, initially if I select 2024-01-11_16-03-23.mp4@marcellamaki - I was not able to replicate the issue mentioned by you in Slack, but I am happy to retest this using actual devices once we have this feature enabled. :) |
This PR supersedes #11602 for more convenient rebasing.
This PR is based off of
release-v0.16.x
and is intended for the planned patch 1 releaseSummary
This PR relocates the
MeteredConnectionNotificationModal
that was previously added to the codebase and adjusts the logic for rendering the "Other Libraries" component inLearn > Library
.With these changes, "Other Libraries" will not render for a user on a metered connection unless they choose "Allow download on metered connection" on the
MeteredConnectionNotificationModal
that now appears on theLearn > Library
page. If the user chooses "Do not allow download..." theOtherLibraries
component should not render unless & until the user on a metered connection later changes this choice in their device settings. After dismissing the modal, it should not pop up for users again.This PR represents a departure from the initial issue spec, as it was determined that prior plans to have the
MeteredConnectionNotificationModal
appear at point of download would not actually save users any data, as the content had already been loaded on their device for viewing. The solution presented here appeared to be the swiftest way to avoid unnecessarily expending a user's data without their explicit agreement.References
Reviewer guidance
There are several conditions to explore once the testing setup steps are followed:
MeteredConnectionNotificationModal
:"Allow download on metered connection"
:OtherLibraries
component renders & is navigable"Do not allow download on metered connection"
:OtherLibraries
component does not renderMeteredConnectionNotificationModal
:OtherLibraries
only visible if"Allow download on metered connection"
previously selectedMeteredConnectionNotificationModal
does not renderOtherLibraries
renders (when applicable/available to user)Testing as a dev -- instructions borrowed from #10624
Run
yarn app-devserver
to run Kolibri in the app mode. Be sure to click theinitialize/
url that will come up in your console. If you want to test with the connection not being metered, update therun_kolibri_app_mode.py
file'scheck_is_metered
function to returnFalse
and restart theyarn app-devserver
.Log in as a superuser and you should see the modal the first time and never again after. The key is stored in session storage, clear it and you'll see it again if you selected to not allow connections. As is, if you click "Allow..." then you'll never see the modal again no matter what per the expected logic of the modal not showing whenever they've already said it was allowed. Until the Device Settings page is updated you won't be able to change this directly without going into the Kolibri shell and running:
Testing checklist
If there are any front-end changes, before/after screenshots are includedCritical user journeys are covered by Gherkin storiesCritical and brittle code paths are covered by unit testsPR process
If this is an important user-facing change, PR or related issue has a 'changelog' labelIf this includes an internal dependency change, a link to the diff is providedReviewer checklist
yarn
andpip
)