Closed
Bug 1195952
Opened 9 years ago
Closed 9 years ago
Please add the following choices to the CAB review drop down area of bugzilla
Categories
(bugzilla.mozilla.org :: Administration, task)
Tracking
()
RESOLVED
FIXED
People
(Reporter: lypulong, Assigned: dkl)
Details
Attachments
(2 files, 4 obsolete files)
I would like 3 choices added to the CAB review drop down table ! emergency % not successful # routine The symbols are just a suggestion Please let me know if any questions
Assignee | ||
Comment 1•9 years ago
|
||
Can you remind me which form this is by providing a link for me to look at? dkl
Flags: needinfo?(lypulong)
Reporter | ||
Comment 2•9 years ago
|
||
If you look on the right hand side of the screenshot in the flags section that is where I need the dropdowns added - not sure where the link to that would be
Flags: needinfo?(lypulong)
ah, that's a flag, which only supports ? + - as values. we'll have to migrate this to a custom field, which is a major change. it will break all existing queries as well as requiring development effort to migrate existing data. do you have a timeframe around when you'd like to see this done?
Component: Custom Bug Entry Forms → Administration
Flags: needinfo?(lypulong)
for values we aren't limited to single character symbols, so something like the following may work: ? approved denied emergency not successful routine
Reporter | ||
Comment 5•9 years ago
|
||
(In reply to Byron Jones ‹:glob› from comment #4) > for values we aren't limited to single character symbols, so something like > the following may work: > > ? > approved > denied > emergency > not successful > routine MEH I thought it was selecting from a table and a simple change to add additional values - can you give me LOE to do so and which quarter seems to work for you - I think it has value but not a drop everything you have planned and reallocate resources if that makes sense
Flags: needinfo?(lypulong)
(In reply to Linda Ypulong [:unixfairy] from comment #5) > can you give me LOE to do so and which quarter seems to work for you it isn't a huge amount of work -- it's probably 2 to 4 of hours of code, plus testing and qa on top of that. we aren't responsible for updating any of your saved searches, or any other systems that interface with bugzilla that rely on this field (eg. dashboards). be aware that tracking this in a single field may be problematic in itself - for example you wouldn't be able to set a bug as "emergency" and "not successful". i'm not sure what your reasons are for requesting this change, but if you're only interested in using these to facilitate tracking and reporting i instead recommend we add new keywords and leave the flag as is. we could add "cab-emergency" etc very quickly and you'll be able to tag a single bug with multiple attributes.
Flags: needinfo?(lypulong)
Reporter | ||
Comment 7•9 years ago
|
||
I think the drop downs will be more useful and provide history - because once we get something flagged emergency it would then be moved to approved - if it failed then it would be be changed to not successful at that moment. The IT department is really good about using the dropdown - I think the keywords would need more of a culture change. We are also looking at integrating this into SN workflow and the drop down would be a better trigger to integrate
Flags: needinfo?(lypulong)
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → dkl
Status: NEW → ASSIGNED
Assignee | ||
Comment 8•9 years ago
|
||
Working on this now. Just have a question about migration. What would you like the current cab-review flag values (?, +, -) to map to in the new drop down? Are the values that glob mentioned in comment 4 sufficient for what you need? dkl
Flags: needinfo?(lypulong)
Reporter | ||
Comment 9•9 years ago
|
||
approved = + denied = - to be reviewed = ? and then need to add ! emergency % not successful # routine But the symbol used was simply a suggestion THANK YOU!!!
Flags: needinfo?(lypulong)
Assignee | ||
Comment 10•9 years ago
|
||
(In reply to Linda Ypulong [:unixfairy] from comment #9) > approved = + > denied = - > to be reviewed = ? > > and then need to add > ! emergency > % not successful > # routine > > But the symbol used was simply a suggestion > > THANK YOU!!! Yeah we will leave out the symbols in the new drop down if thats OK. So it would be to be reviewed approved denied emergency not successful routine I assume you want this enabled for 'Infrastructure & Operations'. Any other products would this need to show up for as well? dkl
Flags: needinfo?(lypulong)
Reporter | ||
Comment 11•9 years ago
|
||
No I believe these all come in as Infrastructure & Operations and the words make it much clearer - Thank you
Flags: needinfo?(lypulong)
Assignee | ||
Comment 12•9 years ago
|
||
Attachment #8671082 -
Flags: review?(glob)
Comment 13•9 years ago
|
||
Comment on attachment 8671082 [details] [diff] [review] 1195952_1.patch Review of attachment 8671082 [details] [diff] [review]: ----------------------------------------------------------------- - all our other fields use ? as the request value -- it would be a mistake to not use ? in this field - there isn't any code for requiring 'infra' group membership to set it to a value other than '?' ::: extensions/BMO/lib/Data.pm @@ +144,5 @@ > "Firefox" => [], > "Toolkit" => [], > }, > + qr/^cf_cab_review$/ => { > + "Infrastructure & Operations" => [], this doesn't match the cab-review flag's visibility: Developer Services:__Any__ Infrastructure & Operations Graveyard:__Any__ Infrastructure & Operations:__Any__ ::: scripts/migrate-cab-review.pl @@ +24,5 @@ > + > +my $dbh = Bugzilla->dbh; > + > +my $auto_user = Bugzilla::User->check({ name => 'automation@bmo.tld' }); > +Bugzilla->set_user($auto_user); add this user into all groups to ensure it has permissions to update the bugs: $user->{groups} = [ Bugzilla::Group->get_all ]; $user->{bless_groups} = [ Bugzilla::Group->get_all ]; @@ +26,5 @@ > + > +my $auto_user = Bugzilla::User->check({ name => 'automation@bmo.tld' }); > +Bugzilla->set_user($auto_user); > + > +my $cab_field = Bugzilla::Field->check({ name => 'cf_cab_review' }); i'd like to see a sanity check that the required values ('?', 'approved', and 'denied') are available before running this script @@ +40,5 @@ > +my $sql = <<EOF; > + SELECT bugs.bug_id > + FROM bugs JOIN flags ON bugs.bug_id = flags.bug_id > + JOIN flagtypes ON flags.type_id = flagtypes.id > + WHERE CONCAT(flagtypes.name, flags.status) REGEXP '^cab-review[\\?\\+\\-]\$' there's no need for a concat and a regex here .. WHERE flagtypes.name = 'cab-review' AND flags.status IN ('?', '+', '-') @@ +87,5 @@ > + > + $dbh->bz_commit_transaction; > + $updated++; > + > + undef $bug; this line does nothing
Attachment #8671082 -
Flags: review?(glob) → review-
Assignee | ||
Comment 14•9 years ago
|
||
Attachment #8671082 -
Attachment is obsolete: true
Attachment #8671497 -
Flags: review?(glob)
Comment 15•9 years ago
|
||
Comment on attachment 8671497 [details] [diff] [review] 1195952_2.patch Review of attachment 8671497 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/BMO/Extension.pm @@ +551,5 @@ > if ($field =~ /^cf/ && !@$priv_results && $new_value ne '---') { > + # Cannot use the standard %cf_setter mapping as we want anyone > + # to be able to set ?, just not the other values. > + if ($field eq 'cf_cab_review') { > + if ($new_value !~ /^(1|\?|---)$/ && !$user->in_group('infra', $bug->product_id)) { firing up the regex engine to perform simple string comparison is costly
Attachment #8671497 -
Flags: feedback+
Assignee | ||
Comment 16•9 years ago
|
||
- Removed use of regular expression in check_can_change_field
Attachment #8671497 -
Attachment is obsolete: true
Attachment #8671497 -
Flags: review?(glob)
Attachment #8671737 -
Flags: review?(glob)
Comment 17•9 years ago
|
||
Comment on attachment 8671737 [details] [diff] [review] 1195952_3.patch Review of attachment 8671737 [details] [diff] [review]: ----------------------------------------------------------------- the migration code looks good, but the placement of the field is wrong on the modal form - it should be under 'tracking', not under 'details'. also the modal view is showing all values, even if the user can't set it to that value. this is likely a bug in that view, but it's a blocker for this change so let's fix it as part of this work. ::: extensions/BMO/Extension.pm @@ +553,5 @@ > + # to be able to set ?, just not the other values. > + if ($field eq 'cf_cab_review') { > + if ($new_value ne '1' > + && $new_value ne '?' > + && $new_value ne '---' there's no need to check $new_value ne '---' again (this is already checked in the parent 'if') @@ +563,2 @@ > # "other" custom field setters restrictions > + } elsif (exists $cf_setters->{$field}) { nit: missing \n between } and elsif ::: template/en/default/bug/field.html.tmpl @@ +130,4 @@ > [% IF field.name.match("^cf_blocking_") OR > field.name.match("^cf_status_") OR > field.name.match("^cf_tracking_") OR > + field.name.match("^cf_cab_review$") OR this should be: field.name == "cf_cab_review"
Attachment #8671737 -
Flags: review?(glob) → review-
Assignee | ||
Comment 18•9 years ago
|
||
Feel free to commit if r+ so as to make it in the next push. dkl
Attachment #8671737 -
Attachment is obsolete: true
Attachment #8672708 -
Flags: review?(glob)
Comment 19•9 years ago
|
||
Comment on attachment 8672708 [details] [diff] [review] 1195952_4.patch Review of attachment 8672708 [details] [diff] [review]: ----------------------------------------------------------------- ::: extensions/BugModal/lib/WebService.pm @@ +95,5 @@ > + grep { $bug->$field_name eq $_->name || $_->is_active } > + @{ $field->legal_values }; > + if ($field_name eq 'cf_cab_review') { > + @values = grep { $bug->check_can_change_field($field_name, '---', $_->{name}) } @values; > + } this is a generic method -- do not hard code field names here. i suspect it'll be good enough to always call check_can_change_field, but you'll need to verify that. the "from value" should be the field's current value, not '---'. ::: extensions/BugModal/template/en/default/bug_modal/field.html.tmpl @@ +173,4 @@ > [% IF values.defined %] > [% FOREACH v IN values %] > [% NEXT IF NOT v.is_active AND NOT value.contains(v.name).size %] > + [% NEXT IF name == "cf_cab_review" AND NOT bug.check_can_change_field(name, '---', v.name) %] field.html.tmpl is a generic template -- do not hard code field names here. (the other issues from the WebService changes also apply here)
Attachment #8672708 -
Flags: review?(glob) → review-
Assignee | ||
Comment 20•9 years ago
|
||
Thanks. Tested with always using check_can_change_field and did not seem to cause issue with other custom fields. dkl
Attachment #8672708 -
Attachment is obsolete: true
Attachment #8673198 -
Flags: review?(glob)
Attachment #8673198 -
Flags: feedback+
Comment 21•9 years ago
|
||
Comment on attachment 8673198 [details] [diff] [review] 1195952_5.patch Review of attachment 8673198 [details] [diff] [review]: ----------------------------------------------------------------- r=glob with the fix below. please leave this bug open as a reminder to run the migration script and disable the old flag. i've created the cf_cab_review flag on production, please configure it appropriately but leave it obsolete. ::: extensions/BugModal/lib/WebService.pm @@ +92,5 @@ > foreach my $field (@custom_fields) { > my $field_name = $field->name; > + my @values = map { { name => $_->name } } > + grep { $bug->$field_name eq $_->name || $_->is_active } > + @{ $field->legal_values }; there's no need for the intermediate @values array - add the new grep above the map in the original code.
Attachment #8673198 -
Flags: review?(glob) → review+
Assignee | ||
Comment 22•9 years ago
|
||
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git 9c79219..0e11b4a master -> master Leaving open until the code is live and I have ran the migration script. dkl
Assignee | ||
Comment 23•9 years ago
|
||
Migration script has been ran and 759 bugs were migrated. The cab review field is now active and the old flag retired. Let me know if you need anything else. dkl
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 24•9 years ago
|
||
(In reply to David Lawrence [:dkl] from comment #23) > Migration script has been ran and 759 bugs were migrated. The cab review > field is now active and the old flag retired. Let me know if you need > anything else. > > dkl Looks great, works great - thank you so much!!!!
You need to log in
before you can comment on or make changes to this bug.
Description
•