658485 - WebService function to update groups (Group.update)

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[pryfer]

User-Agent:       Mozilla/5.0 (X11; Linux i686; rv:2.0.1) Gecko/20100101 Firefox/4.0.1
Build Identifier: 3.0.3

Implement the Group.update webservice v1

Hi,
This is the first version of the Group.update Webservice.
Thanks to review when you can.

ps : I wrote the same function _translate than Julien Heyman in the product.update Webservice (bug 647980). Perhaps this function could be implemted into Util.pm Webservice.

Reproducible: Always

Comment 1

[pryfer]

Attachment: Implement the Group.update Webservice V1

Comment 2

[Max Kanat-Alexander]

Comment on attachment 533906 [details] [diff] [review]
Implement the Group.update Webservice V1

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

::: Bugzilla/WebService/Group.pm
@@ +24,5 @@
>  
> +use constant MAPPED_RETURNS => {
> +    userregexp => 'user_regexp',
> +    isactive => 'is_active'
> +};

I think we should rename user_regexp to just "regex". We should do this in the existing Group.create function, too.

@@ +72,5 @@
> +    @group_objects = grep { !$seen{$_->id}++} @group_objects;
> +
> +    my %values = %$params;
> +    
> +    #we delete names and ids to keep only new values to set

Nit: Space after # and capital letter for first letter. Also, period at the end. (All comments should generally be complete sentences.)

@@ +80,5 @@
> +
> +    my %changes;
> +    foreach my $group (@group_objects) {
> +        $group->set_all(\%values);
> +        my $returned_changes = $group->update();

You need to call $group->update() in a separate loop outside of this loop. All set_all calls need to happen before *any* update() call happens. Also, probably all of this should be wrapped in a transaction, starting before the set_all loop starts and committing after the update loop completes.

@@ +81,5 @@
> +    my %changes;
> +    foreach my $group (@group_objects) {
> +        $group->set_all(\%values);
> +        my $returned_changes = $group->update();
> +        $changes{$group->id} = {_translate($returned_changes,MAPPED_RETURNS)};

_translate should be called as a method ($self->_translate), and it should simply return a hashref.

@@ +93,5 @@
> +    my $mapped = shift;
> +    my %changes;
> +    while ( my ($key,$value) = each (%$params)) {
> +        my $new_field = $mapped->{$key} || $key;
> +        $changes{$new_field} = $value;

You should explicitly type each return value as a string.

@@ +196,5 @@
>  =back 
>  
> +=head2 update
> +
> +B<EXPERIMENTAL>

This should be UNSTABLE.

@@ +221,5 @@
> +B<Required> C<array> Contain names of groups to update.
> +
> +=item C<name>
> +
> +C<string> A new name for groups. Must be unique.

You cannot set this on multiple groups, you can only set it on one group at a time. So you probably shouldn't talk about it in the plural, here.

@@ +225,5 @@
> +C<string> A new name for groups. Must be unique.
> +
> +=item C<description>
> +
> +C<string> A new description for groups. This is what will appear in the UI as the name of the groups.

Nit: All lines should be no longer than 80 characters.

@@ +234,5 @@
> +ular expression
> +
> +=item C<is_active>
> +
> +C<boolean> Set if groups updates are active and eligible to be used for bugs.

True if bugs can be restricted to this group, false otherwise.

@@ +238,5 @@
> +C<boolean> Set if groups updates are active and eligible to be used for bugs.
> +
> +=item C<icon_url>
> +
> +C<string> The new URL pointing to the icon used to identify the group.

"A URL pointing to an icon that will appear next to the names of users who are in this group."

@@ +246,5 @@
> +=item B<Returns>
> +
> +A hash with item C<changes>, containing all C<ids> for groups updated.
> +Each C<id> is a hash, containing one hash by element updated
> +containing new and old values for this element.

You should instead be returning a similar format to what Bug.update returns, and it should be documented similarly.

Comment 3

[pryfer]

Attachment: Implement the Group.update Webservice V2

Hi,

it's a new version with your comments.

It supposed that methods "translate" and "params_to_objects" are defined into Util.pm via the bug 647980.

Please review when you can.

Thanks.

Comment 4

[Max Kanat-Alexander]

Comment on attachment 543949 [details] [diff] [review]
Implement the Group.update Webservice V2

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

::: Bugzilla/WebService/Group.pm
@@ +62,5 @@
> +    defined($params->{names}) || defined($params->{ids})
> +        || ThrowCodeError('params_required',
> +               { function => 'Group.update', params => ['ids', 'names'] });
> +
> +    my $group_objects =  params_to_objects($params, 'Bugzilla::Group');

Nit: extra space after =

@@ +68,5 @@
> +    my %values = %$params;
> +    
> +    # We delete names and ids to keep only new values to set.
> +    delete $values{names};
> +    delete $values{ids};

You never call translate, so "regex" won't be mapped to "user_regexp".

@@ +70,5 @@
> +    # We delete names and ids to keep only new values to set.
> +    delete $values{names};
> +    delete $values{ids};
> +
> +

Nit: Extra newline.

@@ +79,5 @@
> +
> +    my %changes;
> +    foreach my $group (@$group_objects) {
> +        my $returned_changes = $group->update();
> +        $changes{$group->id} = translate($returned_changes,MAPPED_RETURNS);

Nit: Space after comma.

@@ +87,5 @@
> +    my @result;
> +    foreach my $group (@$group_objects) {
> +        my %hash = (
> +            id               => $group->id,
> +            changes          => {},

Nit: Way too much space before =>.

Comment 5

[pryfer]

Attachment: Implement the Group.update Webservice V3

Hi,

It's a new version with your comments(with translation of userregexp to user_regexp).

I use methods "translate" and "params_to_objects" defined into Util.pm via the bug 647980.

Please review when you can.

Thanks.

Comment 6

[Max Kanat-Alexander]

Comment on attachment 555078 [details] [diff] [review]
Implement the Group.update Webservice V3

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

Looks right to me! I will fix up the POD myself a bit on checkin.

Comment 7

[Frédéric Buclin]

Carole: I suggest you update your real name to "Carole Pryfer" to make our life easier when adding your real name to the commit message. Thanks for your contribution! :)

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/
modified Bugzilla/WebService/Group.pm
Committed revision 7979.

Comment 8

[Frédéric Buclin]

Added to relnotes for 4.4.