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

Update device settings page to better handle server reload and force page refresh for settings changes #11403

Merged

Conversation

rtibbles
Copy link
Member

@rtibbles rtibbles commented Oct 12, 2023

Summary

  • Updates the useDeviceRestart composable to play better with the Kolibri global disconnection state checking
  • Prevent users from editing paths based on whether the device can be restarted, as well as whether content is remotely managed
  • Remove restriction for content path editing for non-loopback IPs
  • Do a proper PATCH in the device setting api.js to prevent unnecessary saves
  • Display a modal to the user while the device is restarting
  • If a save has happened, refresh the page - if the device was restarted, wait until the restart happened before refreshing.

References

Fixes #11400

Reviewer guidance

Go to the device settings page.
Press the save button, see that nothing happens (except a pop up - maybe we should stop that too?)
Make a change to the settings that doesn't require a restart - see that the page refreshes after.
Make a change to the settings that does require a restart - see that a modal displays while the restart is happening, and that the page refreshes after.

Screencast.from.10-12-2023.04.08.02.PM.webm

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

@rtibbles rtibbles added the TODO: needs review Waiting for review label Oct 12, 2023
@github-actions github-actions bot added APP: Device Re: Device App (content import/export, facility-syncing, user permissions, etc.) SIZE: medium labels Oct 12, 2023
@radinamatic
Copy link
Member

Tested on Ubuntu 20.04:

Press the save button, see that nothing happens (except a pop up - maybe we should stop that too?) ✔️

Agreed on removing the snackbar notification that the settings have been updated when no changes were made. On another hand, snackbar notification is not appearing when other changes are made (like for External devices or Default landing page).

Make a change to the settings that doesn't require a restart - see that the page refreshes after. ✔️

Make a change to the settings that does require a restart - see that a modal displays while the restart is happening, and that the page refreshes after. ✔️

Will do the test on Android next.

@radinamatic
Copy link
Member

radinamatic commented Oct 13, 2023

snackbar notification is not appearing when other changes are made (like for External devices or Default landing page).

I did not see snackbar notifications for these 2 options on Ubuntu, but I do see a brief flash of them on Android, too brief to actually be readable... 🤔

Implemented fixes are working on Android too otherwise, My downloads option appears in the menu after the reload. 👍🏽
One thing I'm still unclear is if there's a plan to have the sticky bar at the bottom for the Save changes button on Android? On narrow screens it's a lot of scrolling and sometimes user may even forget to do it (happened to me too) 😅

@rtibbles
Copy link
Member Author

snackbar notification is not appearing when other changes are made (like for External devices or Default landing page).

Interesting - I don't think anything I did would have caused that, but I'll fix it anyway, along with not showing the notification when no changes have actually been made.

@rtibbles
Copy link
Member Author

One thing I'm still unclear is if there's a plan to have the sticky bar at the bottom for the Save changes button on Android? On narrow screens it's a lot of scrolling and sometimes user may even forget to do it (happened to me too) 😅

I think that should be addressed in ongoing Bottom Bar vs Bottom Navigation Bar discussions/fixes that @marcellamaki is doing in collaboration with @jtamiace

@rtibbles
Copy link
Member Author

I did not see snackbar notifications for these 2 options on Ubuntu, but I do see a brief flash of them on Android, too brief to actually be readable... 🤔

I am seeing these - so I think it may just be a case of the refresh happening too fast before the snackbar appears.

@LianaHarris360
Copy link
Member

LianaHarris360 commented Oct 13, 2023

One thing I noticed is that whenever I add a new storage location and save it, after the device restarts, the new storage location is shown. But if I navigate away from the page and go back, the storage location is no longer there.

I did not press the save changes button in the video but I do want to note that this happens even if that button is pressed after adding a new storage location.

devicesettings.mov

@rtibbles
Copy link
Member Author

OK, behaviour should be updated now to make the snackbar work as expected, to prevent the issues that @LianaHarris360 reported which were caused by the cache object not being up to date.

@radinamatic
Copy link
Member

Now we are talking, snackbars visible (and long enough to be read) on each settings change (and not appearing when no changes were made), prior to page reload. 👍🏽 Actually the latter could also be avoided by making the Save changes button disabled until there is an actual change in a setting 😉

Issue noted by @LianaHarris360 also confirmed as fixed.

Would it be possible to sneak in one more change, I'm guessing it's not a big push: when disabling/enabling pages the same message appears in the modal after unchecking and checking. Maybe when the user is re-enabling (checking) we could skip the first phrase: When you uncheck a page, it will become invisible to users even if they have permission to access it. ?

check-uncheck.mp4

@rtibbles
Copy link
Member Author

Would it be possible to sneak in one more change, I'm guessing it's not a big push: when disabling/enabling pages the same message appears in the modal after unchecking and checking. Maybe when the user is re-enabling (checking) we could skip the first phrase: When you uncheck a page, it will become invisible to users even if they have permission to access it. ?

This would require a bit more of a rework than first appears, because we'd have to have the conditional for when plugins are disabled, plugins are enabled, and when plugins are being both enabled and disabled.

I also think the text is insufficient if we just remove the uncheck warning, and the unused string that is available is too verbose. I think let's file a follow up issue for this instead.

@radinamatic
Copy link
Member

I also think the text is insufficient if we just remove the uncheck warning, and the unused string that is available is too verbose. I think let's file a follow up issue for this instead.

Agreed that it is not ideal, just enough for a temporary change that does not require a new string. All is good with this PR then, I'll open the issue for follow-up! 💯

Copy link
Member

@radinamatic radinamatic left a comment

Choose a reason for hiding this comment

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

Manual QA passes, good to go! 💯 🎉 :shipit:

Copy link
Member

@marcellamaki marcellamaki left a comment

Choose a reason for hiding this comment

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

Code read through makes sense to me. Thanks @LianaHarris360 and @radinamatic for the reviews! Since manual QA passes, I think we can go ahead and merge this.

@marcellamaki marcellamaki merged commit 994fc31 into learningequality:release-v0.16.x Oct 17, 2023
34 checks passed
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: medium TODO: needs review Waiting for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updating device settings does not update behaviour when the device settings are bootstrapped into the page
4 participants