Closed Bug 1196618 Opened 9 years ago Closed 9 years ago

add support for group owners

Categories

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

Production
defect

Tracking

()

RESOLVED FIXED

People

(Reporter: glob, Assigned: dkl)

References

(Blocks 3 open bugs)

Details

Attachments

(2 files, 3 obsolete files)

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 nobody@mozilla.org initially
- add a "groups owned" row to editusers, below "product responsibilities"
- update the "group admins" report to show the owner as well as admins
Blocks: 1196620
Blocks: 1196623
Blocks: 1196627
Assignee: nobody → dkl
Status: NEW → ASSIGNED
Attached patch 1196618_1.patch (obsolete) — Splinter Review
Attachment #8651942 - Flags: review?(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
Attachment #8651942 - Flags: review?(glob) → review-
Depends on: 1177911
No longer depends on: 1177911
Attached patch 1196618_2.patch (obsolete) — Splinter Review
Misread the original requirements and got the wires crossed. All fixed up now.

dkl
Attachment #8652411 - Flags: review?(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('nobody@mozilla.org');
> +    $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 == 'nobody@mozilla.org' %]

'nobody' shouldn't ever be a group admin, so it's important to list them if it's been incorrect set up as one
Attachment #8651942 - Attachment is obsolete: true
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
Attachment #8652411 - Flags: review?(glob) → review-
Attached patch 1196618_3.patch (obsolete) — Splinter Review
Attachment #8652411 - Attachment is obsolete: true
Attachment #8655033 - Flags: review?(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 == 'nobody@mozilla.org' %]

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.
Attachment #8655033 - Flags: review?(glob) → review+
schema only

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   7c39ca2..f072be2  master -> master
Attachment #8655033 - Attachment is obsolete: true
Attachment #8655611 - Flags: review+
Attachment #8655612 - Flags: review+
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.
Attachment #8655612 - Flags: review+ → review-
Blocks: 1200958
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.
Attachment #8655612 - Flags: review- → review+
To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   e1c42c8..3d463f7  master -> master
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
fix test failures:

To ssh://gitolite3@git.mozilla.org/webtools/bmo/bugzilla.git
   6a66781..3cc9c73  master -> master
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: