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

Don't always force a rescan when page is refreshed #856

Open
jonespm opened this issue Oct 25, 2022 · 7 comments
Open

Don't always force a rescan when page is refreshed #856

jonespm opened this issue Oct 25, 2022 · 7 comments

Comments

@jonespm
Copy link
Contributor

jonespm commented Oct 25, 2022

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);

@webchuckweb
Copy link
Contributor

webchuckweb commented Oct 26, 2022

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
Copy link
Contributor

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

udoit_scan_btn

@zqian
Copy link
Contributor

zqian commented Oct 26, 2022

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

@jonespm
Copy link
Contributor Author

jonespm commented Nov 1, 2022

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
Copy link
Contributor Author

jonespm commented Nov 2, 2022

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.

@lsloan
Copy link

lsloan commented Nov 8, 2022

UMich decided to put this feature on hold for now.

@jonespm
Copy link
Contributor Author

jonespm commented Nov 8, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants