write xmlrpc function disableAccount that is to be contributed to the upstream. Please refer to the following url for details on Bugzilla::RPC 2.18 functions mapped to upstream Bugzilla::WebService and for 2.18 xmlrpc functions sorted by percentage of calls: https://engineering.redhat.com/trac/bugzilla-3.0-rh/wiki/XmlrpcCrossComparison#a2.18xmlrpcfunctionssortedbypercentageofcalls
*** 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.