cms-PdmV / cmsPdmV

CERN CMS McM repository
4 stars 10 forks source link

Delete multiple requests at the same time #1111

Open ggonzr opened 8 months ago

ggonzr commented 8 months ago

At the bottom panel located in the request view (‘Multiple selection buttons’), this includes a new button to delete all the requests already selected. In case of error, display a panel describing the issue. This button is only shown if the user has the role generator_contact or higher.

sihyunjeon commented 8 months ago

Hi @ggonzr could we make this deleting functionality to be executed "only for prepid author or user involved in the history" and return error that the person is not authorized? otherwise there is a danger of, even mc contacts, making a mistake and removing other people's requests.

If one wants super power to remove other people's request one can press register button and get involved in the prepid and then remove the prepid.

IIRC back when i talked to justinas, this "removing all at once" was not supported from web api for such safety issues that people can make mistakes easily and it's a massive destruction.

At least having such safety trigger might be good to implement if it's possible.

tvami commented 7 months ago

hi @sihyunjeon

This request came from me in https://github.com/cms-PdmV/cmsPdmV/issues/1110

In that issue you can see I asked this to be possible on requests that are in new. In that case there is not much destruction one can do accidentally. I'm not against your proposal if it's easy to implement, but if it is not easy then the condition for the request to be in new state should be similarly safe.

sihyunjeon commented 7 months ago

@tvami i don't think so.

  1. prepids are already "removable" only if they are in status=new. so allowing multiple prepids removal could only increases danger. (danger of accidentally removing one prepid one-by-one by hand) << (danger of accidentally removing 20 prepids all at once)

  2. ongoing validations also have "approval = validation" and "status = new" which means it could still be removed. e.g. these in the link could be removed. https://cms-pdmv-prod.web.cern.ch/mcm/requests?prepid=*Run3*wmLHE*&approval=validation&status=new&page=0&shown=127

  3. and then failures for validation in the end return back to "approval = none" and "status = new" so these will have the potential to be removed accidentally.

  4. it already happened during our run3 sample submission to me and @agrohsje. Some of our VV prepids got lost for Run3 by unknown people/person. of course you might say, "one can make prepids again if that happens" but obviously this is not a solution.

DickyChant commented 7 months ago

@tvami I think it should be simple and straightforward to implement, at least it doesn't seem complicated for me

@sihyunjeon what's your point exactly? I think having safety control is nice, while the issue you mentioned seems to be a different thing? Deleting a request during its validation should be banned or at least sending a killing signal to the validation job no?

sihyunjeon commented 7 months ago

Ideally, deleting request should be only allowed for ones in history (one by one and multiple)

At least multiple deletion should be allowed for ones in the history for safety if it is controllable separately for one by one and multiple seletions. And if not, multiple deletion should be not allowed.

What tamas was saying was "there is not that much destruction for samples with new state" and this is not true since "new" samples with ongoing validation and failing validation are included in this category

DickyChant commented 7 months ago

So why shouldn't we ban deletion of "validation-new"? It is definitely a much simpler task than reading history (which is already not complicated in my opinion after all)

sihyunjeon commented 7 months ago

That is sth else to be discussed

Still, validation failed prepids (that goes back to none/new) are prone to accident unless you run an autobot that triggers mcm validation after validation failure

So again, "new" includes "validation/new" and "none/new" And "none/new" includes both very premature prepids and mature prepids that failed validations and waiting for contacts to retrigger the button.

So it should not be like "there isnt that much danger". Allowing multiple deletion is definitely more dangerous than allowing one by one removal at least to my senses

DickyChant commented 7 months ago

I don't see a logic, allowing multiple deletion doesn't mean you have to a. select all things in "new" status and b. delete them at once.

And don't get me wrong, I think both are needed.

The only possible reason for not allowing multiple deletion is you need a price discrimination-ish strategy, saying if you are smart enough you can write a script that deletes multiple request, and if you are not smart enough to write that script you should delete one by one.

Also, I wouldn't call a request that is matured enough if it cannot pass validation. If a request is in none-new state, what is the difference between "inject a new one and trigger validation" and "directly trigger validation"? This two should be identical if McM provides informative enough log email. So, I won't worry about accidental deletion of such a request.

sihyunjeon commented 7 months ago

Discussed through offline.

Which I don't agree is that I believe "triggering validation" means the prepid is mature enough in terms of "physics content". So I don't agree that "not passing validation = not mature = ok to be removed by random user". And if somebody accidentally removes my prepid, whether i do it through script or not, it still wastes my time. This already happened to me and Alexander twice this year, unknown person removing our prepids carelessly. And to me I feel "multiple deletion" will only increase the possibility for this to happen if one scans mcm web without too much care.

@DickyChant not sure if you recall but I didn't give prepid removing script anyone else except you :) during my EXO L3 term for similar reason, didn't want careless people making mistakes. Of course you are much smarter than I first knew so you could've made yourself script easily but hope this tells you how much "careful" I was trying to be with prepid removals in general.

At least common consensus was that Allowing "deletion" by random users who do not own the prepid is already dangerous whether through multiple easy click or one-by-one hard way which then means that we need prepids to be removed only by authorized users (and the only way to do this is as far as I can think of is finding user in the history log, and if the person exists in the log -> allow removal).

@ggonzr I know you are on vacation (and sorry for missing the chance to have coffee with you on tuesday :(), when you come back, can you provide the timeline on when these can be done (allowing prepid removal only for users in the history log)?

DickyChant commented 7 months ago

@sihyunjeon I think we are on the same page for having a safety trigger.

In fact, the script thing is not hard to implement. Anyone with basic knowledge on shell scripting could manage it within 5 mins if they spent another 5 minutes on searching Indico for McM scripting, or github.

I think this thread should be viewed as an example on how user feedback & discussions can help improving McM getting better.

But still, I think it would be much better if we spend more time coding, actually we type more than needed for implementing that desired function.

sihyunjeon commented 7 months ago

if you coded the safety trigger i would've not objected ...

DickyChant commented 7 months ago

https://github.com/cms-PdmV/cmsPdmV/compare/master...DickyChant:cmsPdmV:safety_trigger?expand=1 sth like this is what needed for safety trigger (was in a bar outside so take longer)