search

dlandon / unassigned.devices

Unassigned Devices plugin for unRAID
Other
85 stars 40 forks source link

Better handling of form events #73

Closed bavis-m closed 3 years ago

bavis-m commented 3 years ago

When submitting the EditSettings.php main

element:

  • call preventDefault() right away, then manually submit the form after we've $.post-ed the changes we want
  • this prevents us from discarding the XHR request if the form submit request goes through before the XHR goes through (causes the page to reload and not see the new updates yet)
dlandon commented 3 years ago

What is it that you are trying to accomplish? This PR causes issues with the settings update. In one case I was getting a missing CSRF token error. I am reverting this PR.

bavis-m commented 3 years ago

I found that occasionally when I submit the form (choose a new device script and hit "Save") it would make the XHR request to unassigned devices to update stuff, and then make the actual form submit to update.php, the update.php request would come in first and reload the page, showing the old info (the XHR request does go through, although it shows "Cancelled" in the network tab of chrome dev tools, you just have to refresh the page to see updated values). I'm not sure what is causing the CSRF errors... I hadn't seen those in my testing.

bavis-m commented 3 years ago

Just out of curiosity, what are the exact CSRF errors you're getting? This shouldn't cause any changes to the actual requests made, just the ordering. The CSRF shouldn't be changing between requests anyways (at least I haven't seen that in the unraid web ui)

dlandon commented 3 years ago

It was a missing csrf token and occurred on the page that shows the apfs volume.

I still don't understand the issue you are trying to solve. What sequence of events do you do on the settings page that causes the issue?

I've released a new version of UD. See if it is still happening.

bavis-m commented 3 years ago

It looked to me like a race between the form submitting to update.php (which reloads the page) and the XHR to UnassignedDevices.php which updates the config of the mount. This may only have happened once I added the mount options. That would cause (for cifs mounts, for example) set_samba_config to be called twice from UnassignedDevices.php, once to set the user command and once to set the mount options. The timing on my particular machine was such that the user command would get set in time for the form submit to update.php to reload and show the updated user command, but the next set_samba_config call to set the mount options wouldn't have run by the time the page refreshed, and it would show the previously set mount options (despite them being updated in the config file). Delaying the form submit until the XHR request goes through guarantees that when the page refresh happens it shows all the updated information.

(If I'm misunderstanding any of the flow here, please let me know, obviously you have a much better understanding of the setup here!)

On Thu, Nov 4, 2021 at 10:33 AM dlandon @.***> wrote:

It was a missing csrf token and occurred on the page that shows the apfs volume.

I still don't understand the issue you are trying to solve. What sequence of events do you do on the settings page that causes the issue?

I've released a new version of UD. See if it is still happening.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dlandon/unassigned.devices/pull/73#issuecomment-961265585, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABXE2KKPDIAJD4IU5JO4OODUKLGYDANCNFSM5HJRWAIQ . Triage notifications on the go with GitHub Mobile for iOS https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675 or Android https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub.

dlandon commented 3 years ago

There are several calls to the set_samba_config() function to set different config options for the remote share.  This is normal.

When you say mount options, you mean the switches at the top?  Those are updated as soon as the switch changes state.  They are not changed when 'Save' is clicked.

I don't know how you are saving the device script file and getting the switches to not show properly.  You may have actually found a bug.

Can you give me a sequence of events that demonstrates the issue you are describing?  Or a video showing the problem?

bavis-m commented 3 years ago

Here's a video showing what happens:

https://user-images.githubusercontent.com/7228713/140544004-4e420088-e0e5-4fc8-9526-61526d36f819.mp4

I change the name of the script from rtorrent_plex2.sh -> rtorrent_plex.sh and save, but when the page reloads (from the form submit to update.php), the script reverts to rtorrent_plex2.sh. A reload, however, shows that the value was actually changed in the config file. As best as I can tell, the page is reloading before the XHR call to UnassignedDevices.php can finish running.

Here's the network trace: image

UnassignedDevices.php starts at 2.65s, and runs for 507ms until it's cancelled at ~3.15s. The page reload of EditSettings.page (triggered by updatePage() in update.php, loaded into the progressFrame iframe) happens at 3.07s.

NOTE: this only seems to happen about 50% of the time for me. With the changes to event.preventDefault from this PR however, it has never happened to me. I've also never gotten the CSRF token error though, so possibly my workflow or setup is different?

dlandon commented 3 years ago

I can't reproduce your issue. Please install the released version of UD and try to reproduce the problem. You have a custom line about additional mount options that might be interfering,

The latest version of UD has the vers set to 3.1.1. You no longer need that.

If we do custom mount options, this is not how I'd do it.