Bug 427912

*** Bug 406211 has been marked as a duplicate of this bug. ***

Created attachment 295589 [details]
v1 for function Bugzilla::WebService::User::update

patch for xmlrpc function User.update() ,, submitted the patch upstream for
review.

worked 5 hours , analysis, design, coding and testing

modified function code to make the API consistent with User.get, also added POD 

submitted new version of the code upstream for review.

Created attachment 295909 [details]
WebService function User.update

Please review the function when you can so we can get it into milestone 2.

Thanks,
Noura

Comment on attachment 295909 [details]
WebService function User.update

>+sub update {
>+    my ($self, $params) = @_; 
>+
>+    my $user = Bugzilla->login(LOGIN_REQUIRED);
>+    my $editusers = $user->in_group('editusers');
>+
>+    # Reject access if there is no sense in continuing.
>+    $editusers
>+        || ThrowUserError("auth_failure", {group  => "editusers",
>+                                           action => "edit",
>+                                           object => "users"});
>+

If the plan is to eventually allow permission group changes also with
User.update(), then the above check will need to be changed to allow people who
have ability to bless other users to continue if the 'update' value contains a
group change for groups they can bless.
For now it is fine to leave it this way.

>+
>+    my @user_objects;
>+    @user_objects = map { Bugzilla::User->check($_) } @{ $params->{names} }
>+                    if $params->{names};
>+
>+    my $obj_by_ids;
>+    $obj_by_ids = Bugzilla::User->new_from_list($params->{ids}) if $params->{ids};
>+     
>+    my %unique_users;
>+    map { $unique_users{$_->id} = $_ } @user_objects;
>+
>+    # obj_by_ids are only visible to the user if he can see 
>+    # the otheruser, for non visible otheruser throw an error

Move this comment from here down to the section where you are executing
$user->can_see_user().

>+    foreach my $obj (@$obj_by_ids){
>+        push (@user_objects, $obj) if !$unique_users{$obj->id};
>+    }
>+
>+    my %changes;
>+
>+    my %methods = (
>+        name => 'set_login',
>+        real_name => 'set_name',
>+        disabled_text => 'set_disabledtext',
>+        email_enabled => 'set_disable_mail',
>+    );
>+
>+    my %mapped_returns = (
>+        realname => 'real_name',
>+        login_name => 'name',
>+        disable_mail => 'email_enabled',
>+        disabledtext => 'disabled_text',
>+
>+    );

Do we need to change the returned labels here? They are similar enough already
to what you are changing to that we could just leave them as-is and keep this
simple.

>+
>+    # throw an error is user tries to update password or login for several users 
>+    if (scalar @user_objects > 1 && ( (grep {$_ =~ /name/ } keys %{$params->{update}}) || (grep {$_ =~ /password/ } keys %{$params->{update}}) ))
>+    {
>+        ThrowUserError("multiple_users_update_not_allowed");
>+    }
>+    else{
>+        foreach my $otheruser (@user_objects){
>+            $user->can_see_user($otheruser)
>+                || ThrowUserError('auth_failure', {reason => "not_visible",
>+                                                   action => "modify",
>+                                                   object => "user",
>+                                                   userid => $otheruser->id});
>+
>+            if ($editusers) {

Is this above even necessary? We already check for 'editusers' in the beginning
and exit if the user is not permitted. Anyone in 'editusers' in theory should
be able to see all users anyway so we probably do not need to the
can_see_user() checks.

Eventually this could be $user->can_bless($some_group) if the related change
being executed is a group change.

Unless upstream or us decide to have a separate function called
User.update_perms() or something like that then the can_bless would be there
instead.


>+                foreach my $field (keys %{$params->{update}}) {
>+                    my $method = $methods{$field};
>+                    $method ||= "set_" . $field;
>+                    $otheruser->$method($params->{update}{$field});
>+                }
>+            }
>+        }
>+        foreach my $otheruser (@user_objects){
>+            my $returned_changes = $otheruser->update();
>+            foreach my $changed_field (keys %{$returned_changes}){
>+                my $mapped_field = $mapped_returns{$changed_field};
>+                $mapped_field ||= $changed_field;

		  my $mapped_field = $mapped_returns{$changed_field}
		      || $changed_field;

>+                $changes{$otheruser->id}{$mapped_field} = $returned_changes->{$changed_field};
>+            }
>+        }
>+    }
>+
>+    return { users_updates => \%changes };
>+}
>+
> 1;


Everything else looks fine.

Dave

Created attachment 296211 [details]
Bugzilla::WebService::User::update()

Hi Dave,

This is a new patch for the function, added your suggestions, also allowed
passing single login names and user ids to this function as well. 
with regards to the mapping of the field names , actually I think we will have
to keep them there caze upstream is very concerned that the field names have to
be consistent and similar through the API, for example realname and real_name
makes a big difference to them!

Noura

Comment on attachment 296211 [details]
Bugzilla::WebService::User::update()

>+sub update {
>+    my ($self, $params) = @_; 
>+
>+    my $user = Bugzilla->login(LOGIN_REQUIRED);
>+    my $editusers = $user->in_group('editusers');
>+
>+    # Reject access if there is no sense in continuing.
>+    $editusers
>+        || ThrowUserError("auth_failure", {group  => "editusers",
>+                                           action => "edit",
>+                                           object => "users"});
>+
>+    my @name_list = ref($params->{names}) ? @{$params->{names}} :
>+                    ($params->{names});
>+
>+    my @user_objects;
>+    @user_objects = map { Bugzilla::User->check($_) } @name_list
>+		    if $params->{names};
>+
>+    my @ids = ref($params->{ids}) ? @{$params->{ids}} : ($params->{ids});

Nit-Pick: Either change @ids to @id_list or change @name_list to @names.

>+    my $obj_by_ids;
>+    $obj_by_ids = Bugzilla::User->new_from_list(\@ids) if $params->{ids};
>+     
>+    my %unique_users;
>+    map { $unique_users{$_->id} = $_ } @user_objects;
>+
>+    foreach my $obj (@$obj_by_ids){
>+        push (@user_objects, $obj) if !$unique_users{$obj->id};
>+    }
>+

If I pass in { ids => [6, 6] } I will get that account twice. So change to
the following:

foreach my $obj (@$obj_by_ids){
    if ( !$unique_user{$obj->id} ) {
	push (@user_objects, $obj);
	$unique_user{$obj->id} = $obj;
    }
}

The rest is ok as long as upstream wants the returned field names to be what
they are. 
Fix the above and r=dkl.

Created attachment 296309 [details]
Bugzlla::WebService::User::update()

applied Dave's suggestions. Patch committed to cvs now

This feature is included now in Milestone 2. Any bugs found with this feature
should be file as a new report and set to block the 3.2 final release tracker.