Infrastructure

Is the behaviour comparable to the old instance, or would this be a change from the former instance?

Florian Effenberger wrote:

Is the behaviour comparable to the old instance, or would this be a change from the former instance?

This is a change compared to the old instance, universally wanted by all QA team members.

If that is discussed with QA team and developers, and thus communicated
properly, it's fine for me

Robinson, what's the status here? Can you incorporate that, do you need help?

Florian Effenberger wrote:

Robinson, what's the status here? Can you incorporate that, do you need help?

(We still need to update the patch for the Importance fields; I can look into this further, or happy to have more input on patch changes from others, and I can then test and merge)

Robinson is working on a patch he tested locally but it has side-effects
He will deploy it on the test VM after having informed the website & QA list (so nobody is testing in parallel)
Code of the patch will be attached to this issue for others to have a look as well

patch works for limiting at bug creation time, but demoting severity to critical is done in the wrong place in Bug.pm and thus has no effect. (as seen on bugzilla-test, that btw still has sending mails to libreoffice-bugs@lists.freedesktop.org enabled)

It is set to the lower value in a function that receives the parameters by value, and is supposed to give a "yes/no" result only ("yes, the user is allowed to change the field" or "no, the user is not allowed to change that field" - the check_can_change_field only has the reason passed in as reference to return the reason to the calling function (the predefined "need to be reporter", "need to be assignee (or empored)", "need to be empowered user").

So doesn't work for the most important usecase I think.

Even if it worked, it would not account for just rejecting the change but instead would still send notification email to users (and probably multiple times, since people might thing "I must have not selected the value properly, let's set blocker again".

Christian Lohmaier wrote:

patch works for limiting at bug creation time, but demoting severity to critical is done in the wrong place in Bug.pm and thus has no effect.

Demoting severity seemed to work for me at bug creation time, but later edits did show the wrong effect.

(as seen on bugzilla-test, that btw still has sending mails to libreoffice-bugs@lists.freedesktop.org enabled)

Yeah, two hurdles here:
1) Currently we have to set the default assignee for each component separately, so that takes a little time whenever we update the db
2) Ideally we'd have a a TDF email (e.g. no-reply@tdf.org etc..)

It is set to the lower value in a function that receives the parameters by value, and is supposed to give a "yes/no" result only ("yes, the user is allowed to change the field" or "no, the user is not allowed to change that field" - the check_can_change_field only has the reason passed in as reference to return the reason to the calling function (the predefined "need to be reporter", "need to be assignee (or empored)", "need to be empowered user").

I was trying to route-around some of the limitations on the way that Bugzilla couches the errors, but it's probably easier for me to tweak the user-error template instead so that it spits-out something more accurate when a user is prevented from setting particular values.

Even if it worked, it would not account for just rejecting the change but instead would still send notification email to users (and probably multiple times, since people might thing "I must have not selected the value properly, let's set blocker again".

The initial goal there was to simplify the process, so that users wouldn't have to go back and fix the field, and instead would have the edit go ahead and be done with the bug. I agree that even with an added notification, this behaviour might be confusing for some people, so let's just go with an error message for now.

Robinson Tryon wrote:

The initial goal there was to simplify the process, so that users wouldn't have to go back and fix the field, and instead would have the edit go ahead and be done with the
bug. I agree that even with an added notification, this behaviour might be confusing for some people, so let's just go with an error message for now.

New patch in place and updated error message. Priority is only editable by admins/users in group 'contributors'. Admins can set Severity to 'blocker', everyone else gets an error when they try to change the value to 'blocker'. If a non-admin tries to create the bug with 'blocker' state, it will silently drop Severity to 'critical', but subsequent attempts to change the field will give them the regular can't-change-Severity-to-blocker-value error message (this seemed like a ok compromise between efficiency and clarity).

Cloph: Feedback would be appreciated. Thanks!

updated change is OK, but please pay attention to tab vs space indents (you used tabs occasionally, where the rest uses spaces, this messes up the indents)

functionality-wise it now works as expected/described. Althought one could be more explicit in the error message ("you tried to set the field severity to blocker, but only a user with the required permissions may change that field to that value" → why not write "... may change that field to 'blocker'") - minor thing, but as the check already explicitly checks for that value..

Christian Lohmaier wrote:

updated change is OK, but please pay attention to tab vs space indents (you used tabs occasionally, where the rest uses spaces, this messes up the indents)

yeah, something I'll clean up before committing those patches.

Althought one could be more explicit in the error message ("you tried to set the field severity to blocker, but only a user with the required permissions may change that field to that value" → why not write "... may change that field to 'blocker'") - minor thing, but as the check already explicitly checks for that value..

the size of the diff in the error template was originally very short, but to fix string concatenation issues I had to expand it, so a more tailored error message makes good sense.

functionality-wise it now works as expected/described.

Okay, I'll improve cosmetics and then get ready for it to go live.

Status quo is that Priority/Importance is reflected in the patch and deployed to production, but Severity isn't reflected, which needs doing.