Bug 427912 - XMLRPC function disableAccount will be replaced with User.update()
XMLRPC function disableAccount will be replaced with User.update()
Status: CLOSED NEXTRELEASE
Product: Bugzilla
Classification: Community
Component: WebService (Show other bugs)
3.2
All Linux
high Severity medium (vote)
: ---
: ---
Assigned To: Noura El hawary
:
: 406211 (view as bug list)
Depends On:
Blocks: RHBZ30UpgradeTracker 406131 427053
  Show dependency treegraph
 
Reported: 2008-01-07 21:03 EST by Noura El hawary
Modified: 2013-06-24 00:18 EDT (History)
1 user (show)

See Also:
Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of:
Environment:
Last Closed: 2008-03-18 16:39:32 EDT
Type: ---
Regression: ---
Mount Type: ---
Documentation: ---
CRM:
Verified Versions:
Category: ---
oVirt Team: ---
RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: ---


Attachments (Terms of Use)
v1 for function Bugzilla::WebService::User::update (3.80 KB, patch)
2008-02-22 00:51 EST, Noura El hawary
no flags Details | Diff
WebService function User.update (8.58 KB, patch)
2008-02-26 09:02 EST, Noura El hawary
no flags Details | Diff
Bugzilla::WebService::User::update() (8.26 KB, patch)
2008-02-28 09:35 EST, Noura El hawary
dkl: review-
Details | Diff
Bugzlla::WebService::User::update() (7.61 KB, patch)
2008-02-29 00:46 EST, Noura El hawary
no flags Details | Diff


External Trackers
Tracker ID Priority Status Summary Last Updated
Mozilla Foundation 416137 None None None Never

  None (edit)
Description Noura El hawary 2008-01-07 21:03:47 EST
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
Comment 1 Noura El hawary 2008-02-18 23:51:42 EST
*** Bug 406211 has been marked as a duplicate of this bug. ***
Comment 2 Noura El hawary 2008-02-22 00:51:00 EST
Created attachment 295589 [details]
v1 for function Bugzilla::WebService::User::update

patch for xmlrpc function User.update() ,, submitted the patch upstream for
review.
Comment 3 Noura El hawary 2008-02-22 00:52:07 EST
worked 5 hours , analysis, design, coding and testing
Comment 4 Noura El hawary 2008-02-25 11:36:22 EST
modified function code to make the API consistent with User.get, also added POD 

submitted new version of the code upstream for review.
Comment 5 Noura El hawary 2008-02-26 09:02:48 EST
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 6 David Lawrence 2008-02-26 16:18:47 EST
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
Comment 7 Noura El hawary 2008-02-28 09:35:38 EST
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 8 David Lawrence 2008-02-28 11:55:25 EST
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.
Comment 9 Noura El hawary 2008-02-29 00:46:49 EST
Created attachment 296309 [details]
Bugzlla::WebService::User::update()

applied Dave's suggestions. Patch committed to cvs now
Comment 10 David Lawrence 2008-03-18 16:33:49 EDT
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.

Note You need to log in before you can comment on or make changes to this bug.