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

enables sync button in sync modal #10765

Merged

Conversation

akolson
Copy link
Member

@akolson akolson commented Jun 1, 2023

Summary

This pr enables the previously disabled Sync button in the facility syncing modal. It also corrects the display of the You must be connected to the internet message to only show when an internet connection is not available. The sync button is additionally disabled when there is no internet connection available. Finally, the tooltip has been removed.

Before:
image

After:
Screenshot 2023-06-01 at 12 53 22

Screenshot 2023-06-01 at 12 54 10

References

Fixes #10758

Reviewer guidance

  • Navigate to Device > Facilities
  • Select Sync all
  • A syncing modal will open. Proceed to click the Sync button to start the syncing process
  • You must be connected to the internet message is displayed when you set your connection to offline

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

@github-actions github-actions bot added the APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) label Jun 1, 2023
@akolson akolson removed the request for review from jtamiace June 1, 2023 09:59
@akolson
Copy link
Member Author

akolson commented Jun 1, 2023

Tagging @jtamiace @tomiwaoLE to advise on
image
IMO, it probably doesn't make sense to to display the You must be connected to the internet message and disable the since we already have a global snackbar handling connectivity announcements that also prevents interaction with any interfaces underneath.

@pcenov pcenov self-requested a review June 1, 2023 12:38
@bjester
Copy link
Member

bjester commented Jun 1, 2023

IMO, it probably doesn't make sense to to display the You must be connected to the internet message and disable the since we already have a global snackbar handling connectivity announcements that also prevents interaction with any interfaces underneath.

@akolson The client browser's connectivity isn't the same as Kolibri's connectivity. You of course could run Kolibri locally, be connected to Kolibri, but have no internet connection. That said, the string could/should be reworded along those lines for clarification.

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Hi @akolson, my understanding is that the message You must be connected to the internet. should be displayed not when the server is in a disconnected state but when the server is running while the person's device is with no internet access and therefore cannot sync with KDP.
See the following video:

2023-06-01_17-46-45.mp4

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Code LGTM!

@akolson
Copy link
Member Author

akolson commented Jun 1, 2023

@bjester @jredrejo do we have an implementation that checks for the availability of KDP? There's a ToDo comment regarding this in the modal, just wondering if anything was ever implemented

@jredrejo
Copy link
Member

jredrejo commented Jun 1, 2023

@bjester @jredrejo do we have an implementation that checks for the availability of KDP? There's a ToDo comment regarding this in the modal, just wondering if anything was ever implemented

No, we don't, as far as i'm aware

@bjester
Copy link
Member

bjester commented Jun 1, 2023

do we have an implementation that checks for the availability of KDP?

We don't currently but with the new is_local field and when #10431 is completed, we can optimize this and use the existing network location API

@akolson
Copy link
Member Author

akolson commented Jun 1, 2023

Changes have been made following @pcenov 's feedback:

Screen.Recording.2023-06-02.at.00.51.10.mov

As a follow up to fully resolve this issue, there is need to check for;

  1. Device's internet connectivity(implemented with this pr)
  2. KDP's availability, a task that will follow once this comment has been addressed

Copy link
Member

@pcenov pcenov left a comment

Choose a reason for hiding this comment

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

Excellent @akolson, this is implemented as specified now.

@akolson akolson merged commit 3db06b5 into learningequality:develop Jun 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) SIZE: small TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update Sync All Facilities Modal to allow unregistered facilities to sync
5 participants