416137 - WebService function to update a user's information (User.update)

Status: RESOLVED FIXED

(Bugzilla :: WebService, enhancement)

Description

[Noura Elhawary]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.3) Gecko/20070417 Fedora/2.0.0.3-4.fc7 Firefox/2.0.0.3
Build Identifier: 

Hi, we need a WebService functon that can enable us to disable user accounts. Would it be better to have a function to only disable user accounts so it would be Bugzilla::WebService::User::disable()

or to have a function that can update a user information including enabling and disabling them, I guess this one would be a preference as it can enable us to disable either sending emails to the user of disabling the whole account, and also ofcourse all the other user attributes. 

Please let me know what you think?

Thanks,
Noura

Reproducible: Always

Steps to Reproduce:
1.
2.
3.

Comment 1

[Frédéric Buclin]

We should rather implement User.update instead of User.enable_account, User.disable_account, User.enable_email, User.disable_email, ...

Comment 2

[Max Kanat-Alexander]

Yes, I agree, User.update() is what we should do.

Comment 3

[Noura Elhawary]

Hi, I have few points regarding this function:

1- I guess for this function we can only have single user update, as for example when updating user login, or name, or password must be done for a single user.

2- the function param , can either be user login or user id and the user fields that needs to be updated , something like this:
{id => 1, fields => {login => '[e-mail]', disabled_mail => 1}}
or 
{name => '[e-mail]', fields => {name => '[e-mail]'}}

3- the user must be logged in , can_see_user(otheruser) and in editusers group to be able to update user login,name,password,disabledtext and disabled_mail

4- for updating user groups, here I think that we need to move the code that does that update from editusers.cgi to object functions in Bugzilla::User.pm
I think also with that we will need to move the function userDataToVars() in editusers.cgi to Bugzilla::User.pm as well. Is there any bugs currently for doing that ? 

Please let me know what you think.

Thanks,
Noura

Comment 4

[Max Kanat-Alexander]

(In reply to comment #3)
> 1- I guess for this function we can only have single user update, as for
> example when updating user login, or name, or password must be done for a
> single user.

  No, we can do multiple updates, but login and password can only be updated for one user at a time.

> 2- the function param , can either be user login or user id and the user fields
> that needs to be updated , something like this:

  Perhaps we should make it ids =>, logins =>, and "update" =>

> 3- the user must be logged in , can_see_user(otheruser) and in editusers group
> to be able to update user login,name,password,disabledtext and disabled_mail

  That sounds right to me.

> 4- for updating user groups, here I think that we need to move the code that
> does that update from editusers.cgi to object functions in Bugzilla::User.pm
> I think also with that we will need to move the function userDataToVars() in
> editusers.cgi to Bugzilla::User.pm as well. Is there any bugs currently for
> doing that ? 

  There are bugs for those, I'm pretty sure. For now, don't worry about changing people's group permissions through the API, let's just get the basic fields first.

Comment 5

[Noura Elhawary]

Attachment: v1 for function Bugzilla::WebService::User::update

Hi, attached is patch for the User.update function, I have left out updating user groups for now as Max suggested. Please review when you can and let me know what you think.

Thanks,
Noura

Comment 6

[Max Kanat-Alexander]

Comment on attachment 304906 [details] [diff] [review]
v1 for function Bugzilla::WebService::User::update

There's no reason to allow $can_bless since we don't allow you to change group membership.

I think you should make a separate function for getting visible, unique users from a list of ids and logins, so that you can share that code between User.get and User.update.

One thing that's very important here is that we be totally consistent:

* The input parameters for this function's update argument should match the names of the parameters returned by User.get.

* The fields mentioned in the %changes hash should have the same names, also.


Finally, you can't do your loop like:

loop { set; update; }

You have to have two loops like:

loop { set }
loop { update }

Otherwise some users could get updated and then an error is thrown by one of the setters, and you'd be kind of screwed.

Comment 7

[Noura Elhawary]

> 
> One thing that's very important here is that we be totally consistent:
> 
> * The input parameters for this function's update argument should match the
> names of the parameters returned by User.get.
> 
> * The fields mentioned in the %changes hash should have the same names, also.
> 

Hey Max, Thanks for the quick review :) .. I actually thought about the consistency between the 2 functions User.get and User.update but i got a bit confused with few things ,,

1- the use of name and login and real_name ,, so shall i use name instead of login as how was agreed in User.get in everything so I will use it as the hash key for the user logins in the params hash , so the user would pass the following:

{ ids => [1,2], names => ['[e-mail]'], update => { }}

also i will use it in the update => {} ,, so for all login names we use the word name. 


2- disabledtext is not returned by User.get so i guess it is fine as is in User.update

3- disable_mail however we call it in User.get "email_enabled" in the return value. shall I go by what is in User.get ? but in this case the %changes hash returned from User.update returns it as disable_mail , so maybe i will have to overwrite that. 

4- and for user's real names shall we always use the word real_name?

Thanks,
Noura  

Comment 8

[Max Kanat-Alexander]

Perhaps we should return disabledtext in User.get also and call it disabled_text. 

Yes, you will have to override the name of disable_mail.

And yes, we should use real_name.

Comment 9

[Noura Elhawary]

Attachment: v2 for function Bugzilla::WebService::User::update + pod

Hi Max,, Attached is a patch for User.update with changes made to make it consistent with User.get ,, I will attach new patch to User.get that returns disabled_text. also i added POD , done modification to the user-error template and  Constants.pm. 

One thing i didn't do was creating a function to return the visible user objects for the passed ids and logins , I tried to implement it but then found that it is going to be complicated because the logic between the 2 functions get and update is different on how they handle and process the passed ids and logins. So i left it as is.

Please review when you can and let me know what you think.

Thanks,
Noura

Comment 10

[Max Kanat-Alexander]

Comment on attachment 305438 [details] [diff] [review]
v2 for function Bugzilla::WebService::User::update + pod

>+    my $editusers = $user->in_group('editusers');
>+
>+    # Reject access if there is no sense in continuing.
>+    $editusers

  You don't need those on separate lines, you don't use $editusers again.
>+    # obj_by_ids are only visible to the user if he can see 
>+    # the otheruser, for non visible otheruser throw an error

  No, if you're in editusers you can see everybody. So you also don't need the distinction between $obj_by_ids and @user_objects, above.

>+    # throw an error is user tries to update password or login for several users 

  No, you can update password for multiple users, just not login.

>+    if (scalar @user_objects > 1 && ( (grep {$_ =~ /name/ } keys %{$params->{update}}) || (grep {$_ =~ /password/ } keys %{$params->{update}}) ))

  Also, whoa, this line is way too long and does way too much in one line.

>+    else{
>+        foreach my $otheruser (@user_objects){
>+            $user->can_see_user($otheruser)

  Don't need to check that.

>+            if ($editusers) {

  Don't need to check that.

>+                    $otheruser->$method($params->{update}{$field});

  That needs to be {update}->{$field}. I'm surprised that even compiled when you tested it (did you test it)?

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

  You could just do:

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

  If you wanted.

>+                $changes{$otheruser->id}{$mapped_field}

  I doubt that will work, needs the ->, right?


>+    return { users_updates => \%changes };

  Just call it "changes" (that way we can keep it the same between all update functions).

>+different than their email. Also note that you can only do single
>+user update when passing name in the update hash.

  "Also note that you can only update one user at a time when changing the login name. (An error will be thrown if you try to update this field for multiple users at once.)"

>+=item disabled_text

  Make sure you fix this (and any other fields) to match what we checked in for User.get.

>+The hash may contain any of the following:
> [snip]

  You don't need to write out all the fields again. You can just refer people back to the "update" fields.

>+=item 51 (Bad Login Name)
>+
>+You passed an invalid login name in the "names" array.

  What error will you get if you pass a bad ID?

>+=item 304 (Authorization Required)
>+
>+Logged-in users are either not authorized to edit other users, or they 
>+can not see user's account.

  Just remove the second half, since we're not doing that.

>+=item 505 (Multiple Users Updates Not Allowed)

  505 is taken, so use another id now.

>+  [% ELSIF error == "multiple_users_update_not_allowed" %]
>+    [% title = "Multiple Users Updates Not Allowed" %]

  "Multiple User Update Denied"

>+    You can not change users password or login info when changing

  users'

Comment 11

[Noura Elhawary]

Attachment: v3 for WebService function User.update

Thanks for the review Max, I have made the changes you suggested, also i changed the param update to be updates instead so it will be consistent with ids and names.

regarding your questions.
1- throwing an error if the user passed invalid id, basically for user objects from ids we call the function new_from_list which ignores invalid ids without an error or a warning maybe we need to create a bug to fix that.

2- regarding the hash pointers example $updates->{field}{field} basically this works find and passes compilation and i have always been coding it that way, but i changed it as you suggested to $updates->{field}->{field}.

Noura

Comment 12

[Max Kanat-Alexander]

Comment on attachment 327555 [details] [diff] [review]
v3 for WebService function User.update

You should factor out the User.get code that does the parsing of the {name} and {id} fields into a separate function and re-use it for update().

Your mapping should be a constant in this package, not a variable in the subroutine.

I'm pretty sure that the returned changes value should be an array of hashes, not a hash of hashes. It should contain the original names and ids of the user and then the changes made.

Comment 13

[Julien Heyman]

Attachment: Implement the User.update webservice v1

Hi,

I got the patch of Noura and I modified it with last comments.

I refresh it with the trunk code.

Comment 14

[Max Kanat-Alexander]

Comment on attachment 524187 [details] [diff] [review]
Implement the User.update webservice v1

>+sub update {
>+
>+    my @user_objects;
>+    @user_objects = map { Bugzilla::User->check($_) } @{ $params->{names} }
>+                    if $params->{names};

  You can actually do this on the same line as the "my", you don't need two separate lines.

>+    my $obj_by_ids;
>+    $obj_by_ids = Bugzilla::User->new_from_list($params->{ids}) if $params->{ids};

  Same note there.

>+    # start filtering to remove duplicate user ids
>+    my %unique_users; 
>+    map { $unique_users{$_->id} = $_ } @user_objects;
>+    @user_objects = values %unique_users;

  That's okay (although you should be using foreach instead of map when you're not returning an array) but there's a better way to do it that doesn't affect the input order:

  push(@user_objects, @$obj_by_ids);
  my %seen;
  @user_objects = grep { $seen{$_->id}++ } @user_objects;

>+    use constant METHODS => {
>+        name => 'set_login',
>+        real_name => 'set_name',
>+        login_denied_text => 'set_disabledtext',
>+        email_enabled => 'set_disable_mail'
>+    };

  Instead of calling individual set_ functions, you should be calling set_all.

  Also, constants should be defined near the top of the file, around where other "use" statements are. (These actually happen at compile time and are global within this file.)

>+    # throw an error is user tries to update login for several users
>+    if (scalar @user_objects > 1 && 

  You should do "scalar(@user_objects) > 1" because that makes precedence much clearer.

>+        ((grep {$_ =~ /^name$/ } keys %{$params->{updates}}))) {

  There are too many parens there. Also, you don't need to use grep here. You can just do:

  exists $params->{updates}->{name}

>+    else{

  Nit: Space after "else" and before "{"

>+        foreach my $otheruser (@user_objects){
>+            foreach my $field (keys %{$params->{updates}}) {
>+                my $method = METHODS->{$field};
>+                $method ||= "set_" . $field;
>+                $otheruser->$method($params->{updates}->{$field});
>+            }
>+        }    

  There is where you should be using set_all instead.

>+        foreach my $otheruser (@user_objects){
>+            my $returned_changes = $otheruser->update();
>+            foreach my $changed_field (keys %{$returned_changes}){

  Just keys %$returned_changes is fine.

>+                my $mapped_field = MAPPED_RETURNS->{$changed_field} || $changed_field;
>+                $changes{$otheruser->id}->{$mapped_field} = $returned_changes->{$changed_field};

  These lines look pretty long. Are they shorter than 80 characters?

>+=head2 Update Accounts

  User Creation and Modification should be a =head1 section, and it should contain both create() and update(). Also, there should be a header for the function itself, =head2 update. 

>+=item C<ids> (array) B<Optional> - An array of integers, representing user ids.

  Don't put parameter descriptions on the same line as the parameters. (Fix the POD here much like we fixed it for Product.create.)

>+=item C<names> (array) B<Optional> - An array of login names (strings).

  We need to note that either ids or names is required. Don't mark them as Optional or Required.

Comment 15

[Max Kanat-Alexander]

Oh, I have the solution for the "updates" problem! Don't use "updates". Instead, we update the name by using "name" and we select which users to update using "names".

Comment 16

[Julien Heyman]

Attachment: Implement the User.update webservice v2

Hi,

New version.

Please review it when you can.

Julien.

Comment 17

[Max Kanat-Alexander]

Comment on attachment 526737 [details] [diff] [review]
Implement the User.update webservice v2

Review of attachment 526737 [details] [diff] [review]:

In general looks pretty good! :-) I may make a few fixes to the POD on checkin, but here are a few other comments:

::: Bugzilla/WebService/User.pm
@@ +308,5 @@
+    my %seen;
+    @user_objects = grep { !$seen{$_->id}++ } @user_objects;
+
+    my %values;
+

Nit: Need an extra space after each().

@@ +313,5 @@
+        my $mapped_field = MAPPED_FIELDS->{$key} || $key;
+        $values{ $mapped_field } = $value;
+    }
+    delete $values{names};
+        || ThrowCodeError('params_required', 

Add a comment about why you're deleting these. (This is the only actual r- reason--I'm not sure I could write the comment on checkin.)

@@ +320,5 @@
+
+    foreach my $otheruser (@user_objects){
+        $otheruser->set_all(\%values);
+        my $returned_changes = $otheruser->update();
+                       if $params->{names};

Since we have to translate these hashes twice, but with different translators, make a subroutine that translates a hash based on a provided mapping.

Comment 18

[Julien Heyman]

Hi. 

I'll give you a new patch the next week with fix.

Thanks.

Comment 19

[Julien Heyman]

Attachment: Implement the User.update webservice v3

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 20

[Max Kanat-Alexander]

Comment on attachment 540719 [details] [diff] [review]
Implement the User.update webservice v3

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

::: Bugzilla/WebService/User.pm
@@ +278,5 @@
> +    my $values = translate($params, MAPPED_FIELDS);
> +
> +    # We delete names and ids to keep only new values to set.
> +    delete $values->{names};
> +    delete $values->{ids};

Maybe we should just put this code into translate()? It seems like we do this a lot.

@@ +311,5 @@
> +                added   => $self->type('string', $change->[1]) 
> +            };
> +        }
> +
> +        push(@result, \%hash);

Could you make a generic function that will do this %hash{changes} conversion? We're doing it in three places now, and it's pretty much identical in every place.

You're welcome to do the generic stuff in a follow-up bug.

Comment 21

[Julien Heyman]

Hi,

I'm ok to do the generic stuff, but, may be it's easy to do it when patches are commited.

I talk about this bug, and 647980.

Comment 22

[Frédéric Buclin]

(In reply to comment #21)
> I'm ok to do the generic stuff, but, may be it's easy to do it when patches
> are commited.

We are currently only committing patches which will land in Bugzilla 4.2, to prepare Bugzilla 4.1.3. Once 4.1.3 is released, we will create a branch for 4.2, and reopen the trunk again for Bugzilla 5.0. That's why your patch in bug 647980 hasn't been committed yet, because it won't be taken for 4.2 (we are only accepting security fixes and bug fixes). So once the trunk is reopened, we will commit your patch. :)

Comment 23

[Frédéric Buclin]

a=LpSolit, assuming the patch still applies to trunk.

Comment 24

[Frédéric Buclin]

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

Comment 25

[Frédéric Buclin]

Added to relnotes for 4.4.