Task #1070
closedBugzilla importance field patch
50%
Description
Our Bugzilla instance needs a patch to restrict the ability of changing importance and severity fields to certain roles (dev, QA, UX).
The first patch has been applied and is running on the production instance now, but has unwanted side-effects, so a second patch needs to be
- developed
- tested
- documented (#1268)
- rolled out in due time to the productive instance
Although theoretically suited for EasyHacks, Robinson should work on all these steps, given this is quite urgent and we don't want to delay longer.
Related issues
Updated by Florian Effenberger over 9 years ago
Is the behaviour comparable to the old instance, or would this be a change from the former instance?
Updated by Beluga Beluga over 9 years ago
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.
Updated by Florian Effenberger over 9 years ago
If that is discussed with QA team and developers, and thus communicated
properly, it's fine for me
Updated by Florian Effenberger about 9 years ago
Robinson, what's the status here? Can you incorporate that, do you need help?
Updated by Florian Effenberger about 9 years ago
- Tracker changed from Bug to Task
Updated by Robinson Tryon about 9 years ago
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)
Updated by Florian Effenberger about 9 years ago
- Subject changed from Bugzilla: restrict the ability to change importance & severity to certain roles to Bugzilla importance field patch
- Description updated (diff)
- Status changed from New to In Progress
- Priority changed from Normal to High
- Target version set to Q3/2015
- % Done changed from 0 to 50
Updated by Florian Effenberger about 9 years ago
- Related to Task #1268: Bugzilla documentation added
Updated by Florian Effenberger about 9 years ago
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
Updated by Christian Lohmaier almost 9 years ago
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".
Updated by Robinson Tryon almost 9 years ago
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.
Updated by Robinson Tryon almost 9 years ago
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 by Christian Lohmaier almost 9 years ago
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..
Updated by Robinson Tryon almost 9 years ago
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.
Updated by Florian Effenberger almost 9 years ago
Status quo is that Priority/Importance is reflected in the patch and deployed to production, but Severity isn't reflected, which needs doing.
Updated by Robinson Tryon almost 9 years ago
- Status changed from In Progress to Resolved
Florian Effenberger wrote:
Status quo is that Priority/Importance is reflected in the patch and deployed to production, but Severity isn't reflected, which needs doing.
Access control on Severity has been implemented and deployed. Results discussed with QA Team (we'll consider further restrictions/tweaks in the future), and I'll mark this as Resolved.
Updated by Florian Effenberger almost 9 years ago
- Status changed from Resolved to Closed