Re: [pbs-devel] [PATCH proxmox-backup 0/4] ui for group-filters

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On November 22, 2021 3:20 pm, Dominik Csapak wrote:
> adds the ui for group filters for tape-backup-jobs and sync-jobs
> some labels are a bit rough ('Type'/'Options') but had no better ones
> at the moment
> 
> also one small problem, if one has many group filters for specific
> groups, a datastore change triggers an api load for every one of them
> (since thats how our normal selectors work)
> 
> if that is not acceptable, i'll find a solution on how we can only
> update it once and send a v2

thanks for working on this! :)

gave this a quick spin, some issues that probably warrant a v2 anyhow:

- in the edit dialogue the old group filter string in the first tab is 
  still there
- opening the 'add new sync job' dialogue twice in a row leads to an 
  uncaught exception and a broken/empty dialogue[0]
- adding a new sync job requires changing the source datastore after 
  initially setting it, else no 'remote groups' scan is happening
- no remote group scan happens when opening the editor for an existing 
  sync job after initially selecting a source datastore (requires a 
  second change to the datastore selector, like when editing)

the latter two mean the selection for remote groups is empty.

I'd suggest 'Filter type' and 'Filter value' for the headings, then the 
filter type selector can have 'type', 'group' (or ID? or group ID?), 
'regex' as values like the API/config file/CLI? I think it's clear 
from the context that a 'type' filter refers to the backup group type..

IMHO ideally we'd only scan once (well, once initially and then once for 
each change of remote or remote datastore) and cache that for ALL the 
group selectors. especially since the list groups API call is not that 
inexpensive at the moment, since it also returns the number of 
snapshots and the file list of the last one, so has to list all the 
snapshot dirs and their contents (and stat the protected file for each 
snapshot as well).. we could make that part optional maybe (add a new 
parameter that skips the snapshot querying and returns backup_count = 0 
and empty files - or make them optional, since we know a client 
requesting this mode must understand the missing fields)? also just 
realized the groups API call doesn't return empty groups, not sure 
whether that's ideal..

0: Uncaught TypeError: Cannot read property 'each' of null
    at ctor.cleanupReferences (proxmox-backup-gui.js:1354)
    at ctor.fire (ext-all.js:22)
    at ctor.dispatch (ext-all.js:22)
    at ctor.dispatch (ext-all.js:22)
    at ctor.b.doFireEvent (ext-all.js:22)
    at ctor.fireEventArgs (ext-all.js:22)
    at ctor.fireEvent (ext-all.js:22)
    at ctor.destroy (ext-all.js:22)
    at ctor.callParent (ext-all.js:22)
    at ctor.<anonymous> (ext-all.js:22)

> 
> Dominik Csapak (4):
>   ui: add GroupSelector
>   ui: add GroupFilter form field(container)
>   ui: tape/BackupJobEdit: add second tab with group filters
>   ui: SyncJobEdit: add second tab with group filters
> 
>  www/Makefile                     |   2 +
>  www/css/ext6-pbs.css             |   5 +
>  www/form/GroupFilter.js          | 334 +++++++++++++++++++++++++++++++
>  www/form/GroupSelector.js        |  83 ++++++++
>  www/tape/window/TapeBackupJob.js | 231 +++++++++++----------
>  www/window/SyncJobEdit.js        | 242 ++++++++++++----------
>  6 files changed, 691 insertions(+), 206 deletions(-)
>  create mode 100644 www/form/GroupFilter.js
>  create mode 100644 www/form/GroupSelector.js
> 
> -- 
> 2.30.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
> 
>