search

cms-PdmV / cmsPdmV

CERN CMS McM repository
4 stars 10 forks source link

Multiple request modification: bug with time/event #1078

Closed swertz closed 3 years ago

swertz commented 3 years ago

The ability in McM to select several requests and modify some quantities for all of those at once is very useful.

However it doesn't work well for the time/event field: I've tried editing the existing time/event entry or deleting the existing one and creating a new one, but in both cases the original time/event doesn't get removed. Instead, a new entry is added.

Worse, when applying that change to several selected requests, some requests end up with many duplicate time/event entries (and it seems that, for request validation at least, it's the sum of all those entries that is used)...

justinasr commented 3 years ago

Indeed, McM tried to merge old and new list of time/event. Multiple request edit is a tricky feature. Fixed - now time per event will be overwritten to values in editing page, i.e. if one request had 1 and 2 and other 3 and 4 and you set 5, 6 and 7 in editing page, both requests will have 5, 6 and 7 as time per event. Let me know if you have any more problems with this.

swertz commented 3 years ago

Thanks a lot, it works fine now! :+1:

FYI, I've noticed that modifying the event size also doesn't work, it results in multiple empty entries: image (but this is not somthing I need atm)

justinasr commented 3 years ago

I added both time/event and size/event into list of "exceptional lists that should not be merged" when fixing time/event for you: https://github.com/cms-PdmV/cmsPdmV/commit/c7dea8ee764a15e6a5fc8a5e6bcc90414a5c2338 so size/even should be good too.

swertz commented 3 years ago

I've tried again and I still see this list of 3 empty entries as in the screenshot above.

What's a little strange is that in the multiple edit page, the evens size doesn't appear as a list: image

While time/event does, as expected: image

Anyway, I won't need to change the event size (just the time), I just wanted to report this...

Just for my education, why are event size and time/event represented as lists instead of single entries?

justinasr commented 3 years ago

Ah, it was broken in the frontend as well, not just backend. I updated the HTML, so now size/event editing should work correctly.

Anyway, I won't need to change the event size (just the time), I just wanted to report this...

No no, that's good, thanks for detailed explanation and screenshots.

Changing size/event and time/event to a list predates me, but according to commits https://github.com/cms-PdmV/cmsPdmV/commit/ff55ebb36e42a10020e2b8a129ace9b95f4203ba and https://github.com/cms-PdmV/cmsPdmV/commit/43c33f86447f99e279a6220c4afcee85bca28acf reason is "_When generating taskchain dict each task uses it's dedicated sizeevent value" and "_When removing sequence, it removes same index timeevent". It seems that someone wanted more control over each task's time/event and size/event. When requests with multiple sequences are submitted to computing, these multiple sequences act as different tasks event though they come from the same request.

swertz commented 3 years ago

Thanks for the clarification and for the fix!