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

move MeteredConnectionNotificationModal to Library Page & conditionally render "Other Libraries" #11602

Conversation

thanksameeelian
Copy link
Contributor

@thanksameeelian thanksameeelian commented Dec 7, 2023

This PR is based off of release-v0.16.x and is intended for the planned patch 1 release

Summary

This PR relocates the MeteredConnectionNotificationModal that was previously added to the codebase and adjusts the logic for rendering the "Other Libraries" component in Learn > 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 the Learn > Library page. If the user chooses "Do not allow download..." the OtherLibraries 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:

  • user is on metered connection
    • has not yet dismissed MeteredConnectionNotificationModal:
      • chooses "Allow download on metered connection":
        • upon dismissal of the modal, OtherLibraries component renders & is navigable
      • chooses "Do not allow download on metered connection":
        • upon dismissal of the modal, OtherLibraries component does not render
    • has already dismissed MeteredConnectionNotificationModal:
      • OtherLibraries only visible if "Allow download on metered connection" previously selected
      • user does not encounter modal again after dismissing
  • user is not on metered connection
    • MeteredConnectionNotificationModal does not render
    • OtherLibraries 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 the initialize/ url that will come up in your console. If you want to test with the connection not being metered, update the run_kolibri_app_mode.py file's check_is_metered function to return False and restart the yarn 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:

from kolibri.core.device.models import DeviceSettings
settings = DeviceSettings.objects.first()
ex = settings.extra_settings
ex["allow_download_on_metered_connection"] = False
settings.extra_settings = ex
settings.save()

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

…p; add and evaluate allowDownloadOnMeteredConnection logic; add conditions for rendering OtherLibraries
@thanksameeelian thanksameeelian added the TODO: needs review Waiting for review label Dec 7, 2023
@thanksameeelian thanksameeelian added this to the 0.16 Planned Patch 1 milestone Dec 7, 2023
@github-actions github-actions bot added DEV: backend Python, databases, networking, filesystem... APP: Learn Re: Learn App (content, quizzes, lessons, etc.) SIZE: small labels Dec 7, 2023
@thanksameeelian thanksameeelian marked this pull request as draft December 7, 2023 20:17
@thanksameeelian thanksameeelian marked this pull request as ready for review December 7, 2023 20:27
@rtibbles
Copy link
Member

rtibbles commented Jan 2, 2024

Closing in favour of #11683 for rebase.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Learn Re: Learn App (content, quizzes, lessons, etc.) DEV: backend Python, databases, networking, filesystem... SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Ensure metered connection notification appears at point of bandwidth usage
2 participants