1196618 - add support for group owners

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: General, defect, P1)

Description

[:glob ✱]

for most of the groups on bmo there isn't a clear owner - someone who is ultimately responsible for managing that group's membership, and a point of contact for questions and reports.

- add a new "owner_user_id" field to groups
- nullable but mandatory for all non-system groups (ie. mandatory in editgroups)
- set to [e-mail] initially
- add a "groups owned" row to editusers, below "product responsibilities"
- update the "group admins" report to show the owner as well as admins

Comment 1

[David Lawrence [:dkl]]

Attachment: 1196618_1.patch

Comment 2

[:glob ✱]

Comment on attachment 8651942 [details] [diff] [review]
1196618_1.patch

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

::: Bugzilla/DB/Schema.pm
@@ +1207,4 @@
>              isactive     => {TYPE => 'BOOLEAN', NOTNULL => 1,
>                               DEFAULT => 'TRUE'},
>              icon_url     => {TYPE => 'TINYTEXT'},
> +            owner_user_id => {TYPE => 'INT3'},

this is lacking REFERENCES

::: Bugzilla/Group.pm
@@ +213,5 @@
> +sub owner {
> +    my $self = shift;
> +    return $self->{owner} if exists $self->{owner};
> +    if ($self->{owner_user_id}) {
> +        $self->{owner} = Bugzilla::User->check({ id => $self->{owner_user_id} });

you need to set 'cache => 1' here

@@ +541,5 @@
>  sub _check_icon_url { return $_[1] ? clean_text($_[1]) : undef; }
>  
> +sub _check_owner {
> +    my ($invocant, $owner, undef, $params) = @_;
> +    return Bugzilla::User->check($owner)->id if $owner;

cache=>1 here as well

@@ +544,5 @@
> +    my ($invocant, $owner, undef, $params) = @_;
> +    return Bugzilla::User->check($owner)->id if $owner;
> +    # We require an owner if the group is a system group
> +    if (blessed($invocant) && !$invocant->is_bug_group) {
> +        ThrowUserError('group_needs_owner');

this check is inverted - system groups do not require owners, admin created groups do

i also suspect this check won't happen with creating a group, which would be incorrect.

::: Bugzilla/User.pm
@@ +991,5 @@
>  
> +sub groups_owned {
> +    my $self = shift;
> +    return $self->{groups_owned} if exists $self->{groups_owned};
> +    return $self->{groups_owned} = Bugzilla::Group->match({ owner_user_id => $self->id });

nit: would be easier as
return $self->{groups_owned} //= Bugzilla::Group->match(...)

::: extensions/BMO/lib/Reports/Groups.pm
@@ +48,5 @@
>        GROUP BY groups.name";
>  
>      my @groups;
> +    foreach my $row (@{ $dbh->selectall_arrayref($query) }) {
> +        my $group = Bugzilla::Group->new(shift @$row);

missing cache=>1 here

@@ +50,5 @@
>      my @groups;
> +    foreach my $row (@{ $dbh->selectall_arrayref($query) }) {
> +        my $group = Bugzilla::Group->new(shift @$row);
> +        my %admins;
> +        $admins{$group->owner->id} = $group->owner if $group->owner;

the owner is mandatory, so you don't need this check

@@ +53,5 @@
> +        my %admins;
> +        $admins{$group->owner->id} = $group->owner if $group->owner;
> +        if (my $admin_ids = shift @$row) {
> +            foreach my $id (split(/,/, $admin_ids)) {
> +                $admins{$id} = Bugzilla::User->new($id);

and here

@@ +58,5 @@
>              }
>          }
> +        push(@groups, { name        => $group->name,
> +                        description => $group->description,
> +                        admins      => [ values %admins ] });

admins are different from owners.

the owner needs to be shown in its own column, with admins separately.

::: template/en/default/admin/users/edit.html.tmpl
@@ +130,5 @@
> +      <td>
> +        <table>
> +        [% FOREACH group = otheruser.groups_owned %]
> +          <tr>
> +            <th>[% group.name FILTER html %]</th>

make this link to editgroups

::: template/en/default/global/user-error.html.tmpl
@@ +543,5 @@
>      You must enter a name for the group.
> +
> +  [% ELSIF error == "group_needs_owner" %]
> +    [% title = "System Groups Require Default Owner" %]
> +    You must enter a default owner login for system groups.

system groups do not require owners, admin created groups do

Comment 3

[David Lawrence [:dkl]]

Attachment: 1196618_2.patch

Misread the original requirements and got the wires crossed. All fixed up now.

dkl

Comment 4

[:glob ✱]

Comment on attachment 8652411 [details] [diff] [review]
1196618_2.patch

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

this looks good from quick read.  i'll do a full review later today or tomorrow.

::: Bugzilla/Group.pm
@@ +215,5 @@
> +    return $self->{owner} if exists $self->{owner};
> +    if ($self->{owner_user_id}) {
> +        $self->{owner} = Bugzilla::User->check({ id => $self->{owner_user_id}, cache => 1 });
> +    }
> +    return $self->{owner} || undef;

there's no need to cache the user object in ->{owner}, it's already cached by bugzilla::object so it's ok to call Bugzilla::User->check each time.

::: Bugzilla/Install/DB.pm
@@ +3887,5 @@
> +    my $dbh = Bugzilla->dbh;
> +    return if $dbh->bz_column_info('groups', 'owner_user_id');
> +    $dbh->bz_add_column('groups', 'owner_user_id', {TYPE => 'INT3'});
> +    my $nobody = Bugzilla::User->check('[e-mail]');
> +    $dbh->do('UPDATE groups SET owner_user_id = ?', undef, $nobody->id);

there's no need to set the owner on system groups (ie. set the owner to nobody where bug_groups=1)

::: extensions/BMO/template/en/default/pages/group_admins.html.tmpl
@@ +44,4 @@
>          </td>
>          <td>
>            [% FOREACH admin = group.admins %]
> +            [% NEXT IF admin.login == '[e-mail]' %]

'nobody' shouldn't ever be a group admin, so it's important to list them if it's been incorrect set up as one

Comment 5

[:glob ✱]

Comment on attachment 8652411 [details] [diff] [review]
1196618_2.patch

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

overall this looks good - just a few things to tweak.

- treat the group owner as privileged in extensions/BMO/template/en/default/pages/group_members.html.tmpl

::: extensions/BMO/template/en/default/pages/group_admins.html.tmpl
@@ +38,5 @@
>          <td>
> +          <span title="[% group.description FILTER html %]">
> +            [% group.name FILTER html %]</span>
> +        </td>
> +        <td>

this <td> needs a nowrap attribute to prevent aggressive wrapping of that column

@@ +39,5 @@
> +          <span title="[% group.description FILTER html %]">
> +            [% group.name FILTER html %]</span>
> +        </td>
> +        <td>
> +          [% INCLUDE global/user.html.tmpl who = group.owner %]

"nobody"'s description looks weird here .. can you display "-" if the owner is nobody please

::: template/en/default/admin/groups/create.html.tmpl
@@ +49,5 @@
>      <th>Icon URL:</th>
>      <td colspan="3"><input type="text" size="70" maxlength="255" id="icon_url" name="icon_url"></td>
>    </tr>
> +  <tr>
> +    <th>Default Owner:</th>

"default" doesn't make sense - it should just be "owner"

::: template/en/default/admin/groups/edit.html.tmpl
@@ +99,5 @@
>        </td>
>      </tr>
>  
> +    <tr>
> +      <th>Default Owner:</th>

"default" doesn't make sense - it should just be "owner"

::: template/en/default/admin/users/edit.html.tmpl
@@ +142,5 @@
> +      <td>
> +        [% can_edit_groups = user.in_group('creategroups') %]
> +        [% FOREACH group = otheruser.groups_owned %]
> +          [% IF can_edit_groups %]
> +            <a href="[% urlbase FILTER none %]editgroups.cgi?action=changeform&group=[% group.id FILTER none %]">

s/&/&amp;/

@@ +144,5 @@
> +        [% FOREACH group = otheruser.groups_owned %]
> +          [% IF can_edit_groups %]
> +            <a href="[% urlbase FILTER none %]editgroups.cgi?action=changeform&group=[% group.id FILTER none %]">
> +          [% END %]
> +          [% group.description FILTER html %]

display the group name here, not the description

Comment 6

[David Lawrence [:dkl]]

Attachment: 1196618_3.patch

Comment 7

[:glob ✱]

Comment on attachment 8655033 [details] [diff] [review]
1196618_3.patch

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

r=glob, with fixes to be made on commit.
don't forget to commit just the schema changes.

::: extensions/BMO/template/en/default/pages/group_admins.html.tmpl
@@ +48,4 @@
>          </td>
>          <td>
>            [% FOREACH admin = group.admins %]
> +            [% NEXT IF admin.login == '[e-mail]' %]

delete this line - 'nobody' shouldn't ever be a group admin.

::: extensions/BMO/template/en/default/pages/group_members.html.tmpl
@@ +11,4 @@
>    style_urls = [ "extensions/BMO/web/styles/reports.css" ]
>  %]
>  
> +[% SET privileged = (user.in_group('editusers') || user.in_group('infrasec') || user.id == group.owner.id) %]

unfortunately bug 1197073 moved this check to perl, so this is bitrotted.

Comment 8

[David Lawrence [:dkl]]

schema only

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   7c39ca2..f072be2  master -> master

Comment 9

[David Lawrence [:dkl]]

Attachment: 1196618_4_schema.patch (schema only)

Comment 10

[David Lawrence [:dkl]]

Attachment: 1196618_4.patch (the rest)

Comment 11

[:glob ✱]

Comment on attachment 8655612 [details] [diff] [review]
1196618_4.patch (the rest)

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

::: extensions/BMO/template/en/default/pages/group_members.html.tmpl
@@ +48,4 @@
>          <th>Type</th>
>          <th>Count</th>
>          <th>Members</th>
> +        [% IF privileged || (user.id == group.owner.id) %]

this won't work because a group owner who isn't an admin won't be able to select their group if it isn't a public group.

Comment 12

[:glob ✱]

Comment on attachment 8655612 [details] [diff] [review]
1196618_4.patch (the rest)

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

i need to push some other changes to production.
i've filed bug 1200958 to fix the group_members report, restoring r+ so this can land without that fix.

Comment 13

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   e1c42c8..3d463f7  master -> master

Comment 14

[:glob ✱]

fix test failures:

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   6a66781..3cc9c73  master -> master