UDOIT icon indicating copy to clipboard operation
UDOIT copied to clipboard

Don't always force a rescan when page is refreshed

Open jonespm opened this issue 1 year ago • 7 comments

In courses with a lot of content and issues, rescanning can take up to a minute before any user action can take place.

This also can make debugging issues with the tool where large courses are needed for testing more time consuming waiting for the rescan to free up the action. It's seems fine to always run a scan the first time if the course has never scanned content before but shouldn't always run a scan. This is similar to how the previous version of UDOIT ran. A scan can always manually be triggered off of the kebab menu.

I think there should be an config that could either be set in the environment or possibly per course to turn off the auto scan feature, but it ideally would have to check if the course has been scanned at all yet.

I manually disabled this locally for testing a different issue by removing this and resetting the state but a better fix would toggle this.

diff --git a/assets/js/Components/App.js b/assets/js/Components/App.js
index 6985d55b..efcb8fc6 100644
--- a/assets/js/Components/App.js
+++ b/assets/js/Components/App.js
@@ -120,15 +120,22 @@ class App extends React.Component {
     if (this.settings.user && Array.isArray(this.settings.user.roles)) {
       if (this.settings.user.roles.includes('ROLE_ADVANCED_USER')) {
         if (this.initialReport) {
-          this.setState({report: this.initialReport, navigation: 'summary'})
+          this.setState({
+            report: this.initialReport,
+            navigation: 'summary',
+            syncComplete: true,
+            hasNewReport: false,
+            disableReview: false
+          })
+        }
+        else {
+          this.scanCourse()
+            .then((response) => response.json())
+            .then(this.handleNewReport)
         }
       }
     }

-    this.scanCourse()
-      .then((response) => response.json())
-      .then(this.handleNewReport)
-
       // update iframe height on resize
       window.addEventListener("resize", this.resizeFrame);

jonespm avatar Oct 25 '22 14:10 jonespm

I've actually thought about this recently, and agree with the idea. When we originally built it to scan on each page load the idea was that since it was only scanning content that changed it would be a very fast update to the content. However, the reality is that it can take a while to get through the content just to check whether changes have occurred.

So I like the idea of not waiting for the scan, but that alone isn't sufficient. I think if we disable the auto-scan we need to make it very obvious to run a scan, not just a link hidden in the kebab menu. And it needs to be clear when the last time a scan was made. So maybe we have a bar across the top that says "Last scanned: 10/21/2022" with a "Scan Now" button next to it.

If everyone agrees this is a good direction I can claim this.

webchuckweb avatar Oct 26 '22 04:10 webchuckweb

To add on to that idea, here's a rough mock of what that might look like.

udoit_scan_btn

webchuckweb avatar Oct 26 '22 04:10 webchuckweb

@webchuckweb you got my vote! The last scan result status line is really helpful.

zqian avatar Oct 26 '22 22:10 zqian

Yeah, that all looks like a good idea and I like the screenshot and the explicit information. I updated my original comment as I think it should scan if there isn't an initial report but just not force a rescan after that.

It makes sense to me to move it out of the menu and have it more obvious.

jonespm avatar Nov 01 '22 19:11 jonespm

That all sounds good, maybe we could split this up into two issues? Have just a PR for now to have a configuration option to disable the auto rescan then get this improved UI fix in there.

jonespm avatar Nov 02 '22 19:11 jonespm

UMich decided to put this feature on hold for now.

lsloan avatar Nov 08 '22 17:11 lsloan

I think we thought initially it was a good idea to be able to turn off, especially for working in development mode.

But to turn off for everything introduces the new problem where UDOIT would be out of sync with the course. And we didn't test to see what happens if for instance

  1. A course with multiple instructors, one instructors scans in UDOIT
  2. Then another course adds, removes or modifies content
  3. Then an instructor goes back to UDOIT later without rescanning and tries to change content that was modified in step 2

It seems like having it updated is the ideal. We were thinking it might be better if some better progress indicator was shown to the users similar to the old UDOIT rather than disabling the scan. It might be nice to have this optionally able to be disabled though either on a per course or per instance basis with the UX change as suggested.

jonespm avatar Nov 08 '22 20:11 jonespm