1173442 - Implement admin UI changes to allow selecting default product security group instead of editing code

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[David Lawrence [:dkl]]

Currently when we need to assign a default security group (other than core-security) we have to edit the Data.pm file and wait for the next code push. We should do the following instead:

- add new products.default_security_group_id column which is a foreign key to groups.id.
- value for the new column can be NULL meaning default core-security will be used.
- change the monkey patches methods in BMO extension to look for the new column
- add a dropdown to editproducts.cgi to select an existing group other than the default save the choice in the new column

dkl

Comment 1

[:glob ✱]

> - value for the new column can be NULL meaning default core-security will be
> used.

i think it should be non-null and require explicit selection rather than defaulting to core-security -- this ensures thought has been put into selecting the group, and slightly simplifies the code.

Comment 2

[David Lawrence [:dkl]]

Attachment: 1173442_1.patch

Comment 3

[:glob ✱]

Comment on attachment 8628518 [details] [diff] [review]
1173442_1.patch

Review of attachment 8628518 [details] [diff] [review]:
-----------------------------------------------------------------

::: extensions/BMO/Extension.pm
@@ +1989,5 @@
>  }
>  
>  sub _default_security_group_obj {
> +    return unless my $group_id = $_[0]->{security_group_id};
> +    return Bugzilla::Group->new($group_id);

this needs to use the cache

::: extensions/GuidedBugEntry/Extension.pm
@@ +107,2 @@
>  
>      return unless $page eq 'guided_products.js';

there's probably no need to import Bugzilla::Extension::BMO::Data anymore

@@ +114,1 @@
>      $vars->{'product_sec_groups'} = \%product_sec_groups;

there's no need to do the convoluted product_sec_groups dance anymore.

update the product.get webservice call to return the product's default security group and use that in guided's setName()

Comment 4

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #3)
> ::: extensions/GuidedBugEntry/Extension.pm
> @@ +107,2 @@
> >  
> >      return unless $page eq 'guided_products.js';
> 
> there's probably no need to import Bugzilla::Extension::BMO::Data anymore

Still need it for %create_bug_formats.

dkl

Comment 5

[David Lawrence [:dkl]]

Attachment: 1173442_2.patch

Comment 6

[:glob ✱]

Comment on attachment 8628915 [details] [diff] [review]
1173442_2.patch

Review of attachment 8628915 [details] [diff] [review]:
-----------------------------------------------------------------

this looks sane, full review next week.

::: extensions/BMO/Extension.pm
@@ +1989,5 @@
>  }
>  
>  sub _default_security_group_obj {
> +    return unless my $group_id = $_[0]->{security_group_id};
> +    return Bugzilla::Group->new({ id => $group_id, cache => 1 });

currently it's impossible for a product to not have a default secgroup, so there's code that assumes it's always set, including your _default_security_group().

we should fall back to the insidergroup when security_group_id is null (eg. after a group is deleted).

Comment 7

[:glob ✱]

Comment on attachment 8628915 [details] [diff] [review]
1173442_2.patch

Review of attachment 8628915 [details] [diff] [review]:
-----------------------------------------------------------------

in addition to comment 6..

if a sec-group doesn't exist, checksetup throws:
> Adding new column 'security_group_id' to the 'products' table...
> Can't call method "id" on an undefined value at ./extensions/BMO/Extension.pm line 1143.
when this happens we should display a warning and leave the group set to core-security.
this is particularly problematic as stops the migration midway and doesn't reattempt on subsequent checksetup runs.

::: extensions/BMO/Extension.pm
@@ +158,5 @@
>              if $attachid;
>      }
> +
> +    if ($file =~ /^admin\/products\/(create|edit)\./) {
> +        $vars->{security_groups} = Bugzilla::Group->match({ isbuggroup => 1 });

this also needs to match on isactive => 1

you need to check if the product's current group isn't matched by this filter and insert it into the list.

::: extensions/BMO/template/en/default/hook/admin/products/edit-common-rows.html.tmpl
@@ +9,5 @@
>  <tr>
> +  <th align="right">Default Security Group:</th>
> +  <td>
> +    <select required name="security_group_id">
> +      <option value="">Select one...</option>

"select one.." is odd.  just leave it blank.

@@ +17,5 @@
> +          [% g.name FILTER html %]
> +        </option>
> +      [% END %]
> +    </select>
> +  </td>

as the default group should always be shown/shown i'd like to see a warning here if this isn't the case.

Comment 8

[David Lawrence [:dkl]]

Attachment: 1173442_3.patch

Comment 9

[:glob ✱]

Comment on attachment 8631228 [details] [diff] [review]
1173442_3.patch

Review of attachment 8631228 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob, with fixes on commit.

just commit the schema changes only, and attach a patch with the fixes minus the schema changes for me to commit during the push.

::: extensions/BMO/Extension.pm
@@ +1154,5 @@
> +        foreach my $prod_name (keys %product_sec_groups) {
> +            my $group_name = $product_sec_groups{$prod_name};
> +            next if $group_name eq 'core-security'; # already done
> +            my $group = Bugzilla::Group->new({ name => $group_name, cache => 1 });
> +            my $group_id = $group ? $group->id : $core_sec_group->id;

we need to display a warning if we can't find the group so we know to manually fix it after migration.

nit: at this point security_group_id is already core_sec_group->id, so you can just show the warning and skip the UPDATE.

::: extensions/BMO/template/en/default/hook/admin/products/edit-common-rows.html.tmpl
@@ +9,5 @@
> +[%
> +  group_correct_visibility = {};
> +  FOREACH g = product.group_controls.values;
> +    IF (g.membercontrol == constants.CONTROLMAPSHOWN)
> +        && (g.othercontrol == constants.CONTROLMAPSHOWN);

nit: no need to wrap this line

@@ +36,5 @@
> +    </span>
> +    <script type="text/javascript">
> +      var toggleGroupWarning = function() {
> +        var correct_shown = $('#security_group_id option:selected')
> +          .data('group-correct-visibility');

nit: don't wrap this line either

Comment 10

[David Lawrence [:dkl]]

Attachment: 1173442_no_schema_4.patch

Patch (without schema) with suggested changes that will be committed as a secondary push. Moving r+ forward.

Comment 11

[David Lawrence [:dkl]]

Schema only patch committed:

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   c230ed4..5cc7059  master -> master

Please close this bug after the next prod push happens and the rest of the code is committed.

dkl

Comment 12

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   940d6d4..425d780  master -> master