[pve-devel] applied: [PATCH v11 manager 0/3] fix#1710: add download from url button

[Thomas Lamprecht]

On 01.07.21 10:50, Lorenz Stechauner wrote:
> changes to v10:
> * dropped already applied patches
> * added "check" button - the gui now does not automatically send the metadata request anymore
> * removed (visible) content type selector, because there was only one hard-coded option every time
> * added loading mask while the metadata check is in progress
> 
> Lorenz Stechauner (3):
>   api: nodes: add query_url_metadata method
>   ui: Utils: change download task format
>   fix #1710: ui: storage: add download from url button
> 
>  PVE/API2/Nodes.pm                   |  96 ++++++++++
>  www/manager6/Utils.js               |   2 +-
>  www/manager6/storage/Browser.js     |   8 +
>  www/manager6/storage/ContentView.js | 262 +++++++++++++++++++++++++---
>  4 files changed, 343 insertions(+), 25 deletions(-)
> 


applied series, thanks! Did some followups though: 

- I moved out the download window into its own file, was a bit out of place in the storage
  content view and adding 200+ lines code is just better down in a separate file, if not
  really general code to the existing content view components
- I found the event logic a bit hard to grasp, reworked it with a view model
- Renamed `Check` to `Query URL`, as when trying to think as user I had no idea what
  `Check` would actually do.
- disabled the `Query URL` button when done so successfully, any change in URL or verify
  state re-enables it, it stays also enabled if the query itself fails, as that could 
  have been a (network) hiccup, where allowing one to requery makes sense from UX POV.
- do not invalidate size/mime on verify cert check toggle, makes no sense to me, the
  last OK queried info will still be valid.

Above could have been noticed earlier on review, sorry for that, but as I was pretty
swamped to take a closer look and there are others which can review such things I'm
so free to not take the whole blame ;-) 

In general I wonder if we could do the metadata query also client side, we could
just do a HEAD request from there which would be much nicer, but there are also
cases where an admin may enter an internal URL or the DNS the browsers queries is
another than the PVE hosts have, so while I think it would cover most of the
actual cases in practice (as even with internal URLs, the admin often access PVE
also from an internal network), it's still not always doable. On the other hand,
it would drop any remaining concern about doing some http request from the PVE
side.

just noting for the record, we can always switch to that in the future and sunset
or further-restrict the backend querier if we want...