442013 - Create Bugzilla::User->set_groups and set_bless_groups and have editusers.cgi use them

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement, P1)

Description

[Max Kanat-Alexander]

Right now we have set_ functions for all of the profiles-table fields for users, but we don't have methods that can add and remove groups. We should write them!

Comment 1

[Frédéric Buclin]

P1 as we need it for Mageia's Bugzilla.

Comment 2

[Renata Ghisloti]

Attachment: Patch with new add/remove funtion

Attached a patch with a new add_group and remove_group in User.pm. with modifications in sub update and set_all, necessary for the sake of add/remove.

Comment 3

[Frédéric Buclin]

We do not accept new methods if nothing uses them. Please use them somewhere, for instance from editusers.cgi. Also, do not forget to add POD for these new methods.

Comment 4

[Tiago Mello [:timello]]

Comment on attachment 626546 [details] [diff] [review]
Patch with new add/remove funtion

As per LpSolit last comment.

Comment 5

[Renata Ghisloti]

Attachment: Patch with new add/remove function and update_groups

Attached a patch with a new add_group and remove_group in User.pm. with modifications in sub update and set_all, necessary for the sake of add/remove and the update_groups function used via XMLRPC interface.

Comment 6

[Tiago Mello [:timello]]

Comment on attachment 626563 [details] [diff] [review]
Patch with new add/remove function and update_groups

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

::: extensions/LTC/lib/WebService/User.pm
@@ +18,4 @@
>  #
>  # Contributor(s):
>  #   Tiago Rodrigues de Mello <[e-mail]>
> +#   Renata Ghisloti <[e-mail]>

The patch should be against trunk. This does not seem to be trunk code.

Comment 7

[Frédéric Buclin]

(In reply to Tiago Mello [:timello] from comment #6)

> ::: extensions/LTC/lib/WebService/User.pm
> 
> The patch should be against trunk. This does not seem to be trunk code.

LTC is not an official extension. You must use the code in the Bugzilla core code itself, not as part of an extension you wrote.

Comment 8

[Renata Ghisloti]

Attachment: Patch with new add/remove function and update_groups

Attached a patch with a new add_group and remove_group in User.pm. with modifications in sub update and set_all, necessary for the sake of add/remove and the update_groups function used via XMLRPC interface.

Comment 9

[Tiago Mello [:timello]]

Comment on attachment 626769 [details] [diff] [review]
Patch with new add/remove function and update_groups

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

::: Bugzilla/User.pm
@@ +175,5 @@
> +
> +    # Groups
> +    my $removed_groups = $self->{_remove_groups};
> +    my $added_groups = $self->{_new_groups};
> +

You can call delete here for each line.

@@ +179,5 @@
> +
> +    my @removed_names;
> +    my @added_names;
> +    if (defined $removed_groups || defined $added_groups) {
> +        foreach my $g (@$removed_groups) {

$removed_groups can be undef and it can break the foreach, instead, you should be writing:

foreach my $g (@{ $remove_groups || [] })

@@ +183,5 @@
> +        foreach my $g (@$removed_groups) {
> +            $dbh->do("DELETE FROM user_group_map
> +                      WHERE user_id = ?
> +                      AND group_id = ?"
> +                      ,undef, $self->id, $g->id);

I think WHERE line and AND line fits in the same line... and the comma in ,undef should go in the line above. Also, undef is another param of $dbh->do function, not part of the query, so it should aligned with the ".

@@ +193,5 @@
> +                  user_id, group_id, isbless, grant_type
> +                 ) VALUES (
> +                  ?, ?, ?, ?
> +                 )
> +          });

Please, check how other places in Bugzilla indent SQL queries. Also, no need to have one char in a single line. I would write:

my $sth_insert = $dbh->prepare(qq{
    INSERT INTO user_group_map (user_id, group_id, isbless, grant_type)
    VALUES (?, ?, ?, ?)});

@@ +194,5 @@
> +                 ) VALUES (
> +                  ?, ?, ?, ?
> +                 )
> +          });
> +        foreach my $g (@$added_groups) {

Same here: foreach my $g (@{ $added_groups || [] })

@@ +211,4 @@
>      return $changes;
>  }
>  
> +sub set_all {

I'm not sure about the need of this method here. I think we can let the caller call add/remove_group method directly and then call ->update(). Maybe, when we change the editusers.cgi to use this new API method we can implement the set_all method.

@@ +753,5 @@
>  }
>  
> +sub add_group {
> +    my ($self, $params) = @_;
> +    my $user = Bugzilla->login(LOGIN_REQUIRED);

There is no need to force the login here. The caller should take care of that. Use Bugzilla->user instead.

@@ +761,5 @@
> +        || ThrowUserError('auth_failure', { action => 'add',
> +                                            name   => $group->name,
> +                                            object => 'groups' });
> +
> +    # If the bug is already in this group, then there is nothing to do.

bug? :). You meant 'user', right?

@@ +771,5 @@
> +    my $new_group = { group     => $group,
> +                      can_bless => $bless,
> +                      member    => $member};
> +
> +    push @{ $self->{_new_groups} }, $new_group;

How do you guarantee that the caller isn't passing the same group to be added more than once? Otherwise, the INSERT above will fail after the first group is inserted.

@@ +776,5 @@
> +}
> +
> +sub remove_group {
> +    my ($self, $params) = @_;
> +    my $user = Bugzilla->login(LOGIN_REQUIRED);

Bugzilla->user instead

@@ +786,5 @@
> +        if !blessed $params;
> +
> +    $user->can_bless($group->id) || ThrowUserError('auth_failure', $args);
> +
> +    $self->in_group($group) || ThrowUserError('user_group',$args);

This seems to be a new error. But I don't see where a new entry in user-error.html.tmpl file. Check if there is any existent error msg that can be used here. If not, maybe, rename the error to something like: 'user_not_in_group' or anything meaningful. Then write the message telling which user is not a member of which group.

Nit: put a space after the comma.

@@ +2719,5 @@
>  the two passwords match.
>  
> +=item C<add_group($group)>
> +
> +Adds a user in group corresponding to $group->name 

'Adds an user in a group.'

It should explain that $group is hash and state every hash required key. Maybe, give an example how $group should look like.

@@ +2723,5 @@
> +Adds a user in group corresponding to $group->name 
> +
> +=item C<remove_group($group)>
> +
> +Removes a user from group corresponding to $group->name.

'Removes an user from a group.'
It should explain what $group parameter is.

::: Bugzilla/WebService/User.pm
@@ +358,5 @@
> +                                           action => "edit",
> +                                           object => "users"});
> +
> +    # Error if parameters are wrong
> +    defined($params->{names}) || defined ($params->{groups})

I think both parameters are required, right? You must have 'names' and 'groups', otherwise the method wont work. Also, use 'logins' instead of 'names'.

@@ +365,5 @@
> +             params => ['names','groups'] });
> +
> +
> +    my $userlist = $params->{names};
> +    my $changes;

You want an array, not a reference.
my @changes;

@@ +373,5 @@
> +        my $result = $current_user->update()->{user_group};
> +
> +        foreach my $g (@$result) {
> +            push (@$changes, $self->type('string', $g));
> +        }

Please, check how other methods do the changes structure. It should contain 2 keys, 'added' and 'removed'. I think it could be something like:

{ 'someone@login' => { added   => ['group1', 'group2'],
                                   removed => ['group3', 'group4'] }
  ...
}

@@ +755,5 @@
> +Receives a hash containg the list of users and a field add or remove
> +with the group name. Parameters can_bless and in_group are optional
> +
> +Returns the changes provided by update function.
> +

The documentation should explain what exactly are the parameters and what exactly it returns. See other documentations as an example.

Comment 10

[Koosha KM]

May I upload a new patch so that we could get rid of this bug and work on bug 469196?

Comment 11

[Frédéric Buclin]

We are going to branch for Bugzilla 4.4 next week and this bug is too invasive or too risky to be accepted for 4.4 at this point. The target milestone is set to 5.0 which is our next major release.

I ask the assignee to reassign the bug to the default assignee if you don't plan to work on this bug in the near future, to make it clearer which bugs should be fixed by someone else on time for 5.0.

Comment 12

[Renata Ghisloti]

Hello, 
I am working on a patch to update this one I sent. I expect to be ready very soon. Sorry for the delay.

Comment 13

[Renata Ghisloti]

(In reply to Koosha Khajeh Moogahi [:koosha] from comment #10)
> May I upload a new patch so that we could get rid of this bug and work on
> bug 469196?

Hello Koosha,

I am sorry, I misunderstood you question before. Feel free to send your patch. :)
Thanks,

Comment 14

[Simon Green]

Attachment: v4 patch

Okay, this patch is totally different from the previous ones. I created set_groups and set_bless_groups which take a hash with the keys with set, add and remove. This will provide similar functionality as requested and also deal with the bless groups as user can have direct membership on.

As with other set_* fuctions, update needs to be called to actually commit the changes. This has the advantage of the changes being in the array that update returns.

Doing it this way also makes implementing bug 469196 very trivial.

Comment 15

[Simon Green]

Should also probably mention that "XXX: should create profiles_activity entries for blesser changes." comes from editusers.cgi. Fixing adding rows in Bugzilla::Users is outside the scope of this bug (see update too, which has the same comment).

Comment 16

[David Lawrence [:dkl]]

Comment on attachment 830629 [details] [diff] [review]
v4 patch

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

Little bit of rot:

patching file Bugzilla/User.pm
Hunk #1 succeeded at 144 (offset 1 line).
Hunk #2 succeeded at 331 (offset 1 line).
Hunk #3 succeeded at 3157 (offset 166 lines).
patching file editusers.cgi
patching file template/en/default/global/messages.html.tmpl
Hunk #2 FAILED at 51.
1 out of 2 hunks FAILED -- saving rejects to file template/en/default/global/messages.html.tmpl.rej

::: Bugzilla/User.pm
@@ +197,5 @@
> +        else {
> +            # XXX: should create profiles_activity entries for blesser changes.
> +        }
> +
> +        $changes->{$is_bless?'bless_groups':'groups'} = [

nit: add spaces. Also IMO would be more readable as:

my $type = $is_bless ? 'bless_groups' : 'groups';
$changes->{$type} = [ ... ];

@@ +356,5 @@
> +
> +    # The person making the change is $user, $self is the person being changed
> +    my $user = Bugzilla->user;
> +
> +    # Input is a hash of arrays. Key is 'set', 'add' or 'removed'. The array

s/removed/remove/

@@ +400,5 @@
> +        $self->{_group_changes}{$is_bless} = \@diffs;
> +    }
> +}
> +
> +sub _set_groups_to_ref {

nit: _set_groups_to_objects {

@@ +2991,4 @@
>  
>  =item set_disabledtext
>  
> +=item set_groups

Move all POD you are adding here into "Other methods" and out of "Methods in need of POD".

::: editusers.cgi
@@ +259,5 @@
> +    # Update groups
> +    $otherUser->set_groups({ set => [
> +        grep { s/group_// } keys %{Bugzilla->cgi->Vars}
> +    ] });
> +

nit: I would rather this be slightly more readable by using:

# Update groups
my $group_ids = grep { s/group_// } keys %{ Bugzilla->cgi->Vars };
$otherUser->set_groups({ set => $group_ids });

@@ +276,5 @@
> +
> +        # Update bless groups
> +        $otherUser->set_bless_groups({ set => [
> +            grep { s/bless_// } keys %{Bugzilla->cgi->Vars}
> +        ] });

Same here:

# Update bless groups
my $bless_ids = grep { s/bless_// } keys %{ Bugzilla->cgi->Vars };
$otherUser->set_bless_groups({ set => $bless_ids });

Comment 17

[Simon Green]

Attachment: bug442013-v5.patch

Made the changes you suggestion. Your changes for editusers.cgi were a we bit faulty "my $groups = grep ..." returns the number of matching items, not an array. Have made changes to accommodate.

  -- simon

Comment 18

[David Lawrence [:dkl]]

Comment on attachment 8426724 [details] [diff] [review]
bug442013-v5.patch

Just noticed an issue when re-verifying the functionality of the latest revised patch. When you add/remove groups using editusers.cgi, it will report in the changes box that groups that the user has inherited through regexp or group inheritance as being 'removed' from those groups. To recreate, look at the canconfirm and editbugs groups which normally all users are in due to '.*' regexp. editusers.cgi shows those as being removed even though no change to those groups were made.

dkl

Comment 19

[Simon Green]

Attachment: bug442013-v6.patch

Comment 20

[David Lawrence [:dkl]]

Comment on attachment 8429644 [details] [diff] [review]
bug442013-v6.patch

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

r=dkl

Comment 21

[Simon Green]

To ssh://git.mozilla.org/bugzilla/bugzilla.git
   2f75161..d2c2138  master -> master