349349 - Use ->create from Bugzilla::Object instead of insert_new_user for Bugzilla::User

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, enhancement)

Description

[Max Kanat-Alexander]

Now that Bugzilla::User is a Bugzilla::Object, and we don't depend on insert_new_user returning a password, we can make it into an implementation of Bugzilla::Object->create, which is much cleaner.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. Here are some things to know about the patch:

1) I've tested it lightly.
2) I have a plan to fix the XXX comment in Bugzilla::Auth::Verify, but it's
   going to take enough code that it will require it's own bug.
3) We call validate_password directly in token.cgi because we have to make
   sure the passwords match, and there's no easy way to do that in create().
4) create() does not generate a random password if you don't specify a
   password, unlike insert_new_user. We don't need that functionality
   anymore, and it makes the calling code much simpler this way. We simply
   always require a password argument.
5) There is no...number 5. :-)

Comment 2

[Frédéric Buclin]

Comment on attachment 234626 [details] [diff] [review]
v1

>Index: token.cgi

> sub confirm_create_account {

>-    (defined $cgi->param('passwd1') && defined $cgi->param('passwd2'))
>-      || ThrowUserError('new_password_missing');
>-    validate_password($cgi->param('passwd1'), $cgi->param('passwd2'));

>+    validate_password(scalar $cgi->param('passwd1'), 
>+                      scalar $cgi->param('passwd2'));

I would like that you keep the first check, i.e. if both values are defined, before calling validate_password().



>Index: Bugzilla/User.pm

>+    login_name    => \&check_login_name,

check_login_name() is confusing as I read it as "make sure this login name currently exists in the DB", which is exactly the opposite of what you want. I would prefer something like "check_login_availability".


>+        cryptpassword  - B<Required> The password for the new user.
>+            Even though the name says "crypt", you should just specify
>+            a plain-text password.

Nit: I find this really confusing, as discussed on IRC.


The last nit is optional and doesn't need to be fixed, but I would like to see the first 2 to be fixed on checkin, if possible. Your patch is working fine; r=LpSolit

Comment 3

[Max Kanat-Alexander]

  Okay, I fixed all the nits except the last one (it would have been too complicated to fix on checkin). I'll attach what actually got checked in.

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.538; previous revision: 1.537
done
Checking in createaccount.cgi;
/cvsroot/mozilla/webtools/bugzilla/createaccount.cgi,v  <--  createaccount.cgi
new revision: 1.53; previous revision: 1.52
done
Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.134; previous revision: 1.133
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.46; previous revision: 1.45
done
Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.4; previous revision: 1.3
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.128; previous revision: 1.127
done
Checking in Bugzilla/Auth/Verify.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Auth/Verify.pm,v  <--  Verify.pm
new revision: 1.5; previous revision: 1.4
done
Checking in Bugzilla/DB/Schema.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Schema.pm,v  <--  Schema.pm
new revision: 1.65; previous revision: 1.64
done
Checking in Bugzilla/Install/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Install/DB.pm,v  <--  DB.pm
new revision: 1.11; previous revision: 1.10
done
Checking in contrib/syncLDAP.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/syncLDAP.pl,v  <--  syncLDAP.pl
new revision: 1.9; previous revision: 1.8
done

Comment 4

[Max Kanat-Alexander]

Attachment: Checked-In Version

Here's what I checked in.

I also forgot to remove account/exists.html.tmpl in the previous checkin, so I did it just now:

Removing template/en/default/account/exists.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/exists.html.tmpl,v  <--  exists.html.tmpl
new revision: delete; previous revision: 1.8
done