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)

Production
task
Not set
normal

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
Can you remind me which form this is by providing a link for me to look at?

dkl
Flags: needinfo?(lypulong)
Attached image cabreview.png
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
(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)
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: nobody → dkl
Status: NEW → ASSIGNED
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)
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)
(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)
No I believe these all come in as Infrastructure & Operations and the words make it much clearer - Thank you
Flags: needinfo?(lypulong)
Attached patch 1195952_1.patch (obsolete) — Splinter Review
Attachment #8671082 - Flags: review?(glob)
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-
Attached patch 1195952_2.patch (obsolete) — Splinter Review
Attachment #8671082 - Attachment is obsolete: true
Attachment #8671497 - Flags: review?(glob)
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+
Attached patch 1195952_3.patch (obsolete) — Splinter Review
- 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 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-
Attached patch 1195952_4.patch (obsolete) — Splinter Review
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 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-
Attached patch 1195952_5.patchSplinter Review
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 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+
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
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
(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.

Attachment

General

Created:
Updated:
Size: