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 MissingResourceAlert to report sync status #11426

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented Oct 17, 2023

Summary

After much discussion, adds syncing status to the MissingResourceAlert UI banner

References

Fixes #11243

Fixes #11444

missing-resources.mp4

Reviewer guidance

Run two kolibris, one with full facility and one as LOD. Previously, when the "some resources are missing or unsupported" banner would display, you should now see either a waiting to sync or syncing message. The missing or unsupported message should only happen if the resources are indeed missing, and if the sync is failed or errored in some way.

Please note that since these strings were pulled in using createTranslator, they will not be translated until make i18n-download branch=release-v0.16.x is run


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

@radinamatic
Copy link
Member

radinamatic commented Oct 18, 2023

The missing or unsupported message should only happen if the resources are indeed missing, and if the sync is failed or errored in some way.

Tested as per the reviewer guidance, but I can still see the missing or unsupported resources notification during the normal sync that was eventually completed successfully.

Technically, while the sync is happening, the resources are missing, right? Maybe this message should only appear if the sync indeed just failed or errored, not while it is currently underway.

@marcellamaki
Copy link
Member Author

@radinamatic - can you clarify about seeing the message? What you experienced was seeing
"Some resources are missing or not supported" during the sync?

Maybe this message should only appear if the sync indeed just failed or errored, not while it is currently underway.

Yes that is the intention. The purpose of this update would be two things.

  1. By indicating the "The device is currently syncing" when it is, the user would have an explanation for why some lessons might not have all of the resources, such as if you click into a lesson and see nothing (as you and others experienced during one of the bug bashes)
  2. Then, only if the sync had failed or errored, and resources were STILL missing or incompatible, would the "Some resources are missing or not supported" be displayed.

@radinamatic
Copy link
Member

seeing "Some resources are missing or not supported" during the sync?

Yes, as you can see in the screencast below the notification is appearing while the sync is still in progress (thumbnails are loading), and it does not fail at any point (I can open all the videos and both the LoD and coach are displaying green checkmark for syncing).

resources-unsupported.mp4

If it matters, the LoD is in Ubuntu 18.04 VM, not on Android, and browser is set for Slow 3G throttling.

@rtibbles
Copy link
Member

From the screencast, it looks like at least one resource was still being loaded when the page first loaded, and by the time you had viewed the first resource and returned to the playlist page, it was finished?

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

Needs to be updated so as not to cause odd behaviour where this is used in the Coach plugin.

@radinamatic
Copy link
Member

it looks like at least one resource was still being loaded when the page first loaded, and by the time you had viewed the first resource and returned to the playlist page, it was finished?

Github is again messing with my mind, I wrote the reply to this yesterday, and now it's gone... 🙄

Yes, judging by the thumbnails the sync (loading) was happening, and while it is in progress, the user should see the message 'device is syncing', not 'resources missing or unsupported'.

@marcellamaki marcellamaki force-pushed the intermediate-lesson-loading-state branch from a6d9925 to 32a030b Compare October 19, 2023 18:02
@marcellamaki
Copy link
Member Author

@rtibbles @radinamatic this should be ready for re-review now

Copy link
Member

@rtibbles rtibbles left a comment

Choose a reason for hiding this comment

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

A couple more changes needed, I think.

@marcellamaki
Copy link
Member Author

@radinamatic - I lied, it will be ready for you tomorrow. I'll ping you in the morning :D

…e made in backend in follow up, that will prevent over-reporting on the frontend
rtibbles
rtibbles previously approved these changes Oct 20, 2023
@marcellamaki
Copy link
Member Author

Follow up issue here: #11444

@marcellamaki
Copy link
Member Author

marcellamaki commented Oct 20, 2023

@radinamatic this can be re-reviewed on Monday, but importantly, the issues you noticed will not be entirely resolved until the follow up issue is finished, as this requires some additional backend changes. You can let me and Richard know if you have any questions

@github-actions github-actions bot added the DEV: backend Python, databases, networking, filesystem... label Oct 22, 2023
@rtibbles rtibbles dismissed their stale review October 22, 2023 22:27

Updates made since.

@rtibbles
Copy link
Member

To ensure comprehensive QA, I have pushed some updates to this PR to fix the follow up issue as well. @marcellamaki this felt easier than merging this PR and then opening a follow up.

@marcellamaki
Copy link
Member Author

Thanks @rtibbles!

@pcenov
Copy link
Member

pcenov commented Oct 23, 2023

Hi @marcellamaki, @rtibbles, @radinamatic, I was able to identify a couple of minor issues:

  1. Once the sync has happened the text 'Some resources are missing or not supported Learn more' remains visible until I manually refresh the page (I waited several minutes but shortened the video). To replicate it assign a lesson with several resources and wait for the sync to happen on the LOD then on the learn-only device delete several resources and wait for the sync tor restore them on the LOD.
2023-10-23_16-08-40.mp4
  1. If there are missing resources from a quiz we are explaining that within the quiz and not at the Home page so we have a bit of an inconsistent behavior:

2023-10-23_17-20-09

@rtibbles
Copy link
Member

Hrm, I'm not able to replicate 2 - quizzes with missing resources are also showing an alert on the home page.

@marcellamaki
Copy link
Member Author

code review and manual QA on Richard's most recent changes - I can confirm that the banner does disappear after the sync completes (if all goes well)

@rtibbles rtibbles merged commit 6b3d577 into learningequality:release-v0.16.x Oct 23, 2023
35 checks passed
@pcenov
Copy link
Member

pcenov commented Oct 24, 2023

@rtibbles I confirm that the first issue is fixed now:

2023-10-24_11-06-28.mp4

I've reported the 2nd one here as it's still happening on my end.

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: medium SIZE: small TODO: needs review Waiting for review
Projects
None yet
4 participants