332598 - Move ValidatePassword() and DBNameToIdAndCheck() from globals.pl into User.pm

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Frédéric Buclin]

DBNameToIdAndCheck() should be dropped in favor of a second argument to the User::login_to_id() function. For instance: login_to_id($login_name, 1) to throw an error if the login name doesn't exist, and login_to_id($login_name, 0) and login_to_id($login_name) to return 0 if it doesn't exist. That's better than a separate function, IMO.

Comment 1

[Max Kanat-Alexander]

(In reply to comment #0)
> login_to_id($login_name, 1)

  That's not very readable, though, unless you're very familiar with Bugzilla code.

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
> > login_to_id($login_name, 1)
> 
>   That's not very readable, though, unless you're very familiar with Bugzilla
> code.

Yes, I am. :-D

More seriously, I was thinking about adding a new constant WARN = 1, so that we could write:

login_to_id($login_name, WARN)

Do you prefer this solution?

Comment 3

[Max Kanat-Alexander]

(In reply to comment #2)
> More seriously, I was thinking about adding a new constant WARN = 1, so that we
> could write:
> 
> login_to_id($login_name, WARN)
> 
> Do you prefer this solution?

  Yeah. Except make the constant called THROW_ERROR, because that's really what it does.

Comment 4

[Frédéric Buclin]

Attachment: patch, v1

I cannot rename ValidatePassword() as check_password() because this name is already used by Auth/Verify/DB.pm and ./runtests.pl complains that I try to redefine it. So I renamed it as validate_password().

Comment 5

[Max Kanat-Alexander]

Comment on attachment 217779 [details] [diff] [review]
patch, v1

  Looks fine, except:

>+sub validate_password {
>+    my ($password, $matchpassword) = @_;
>+
>+    if (length($password) < 3) {
>+        ThrowUserError('password_too_short');
>+    } elsif (length($password) > 16) {

  That 3 and that 16 really need to be constants.

> Takes a login name of a Bugzilla user and changes that into a numeric
> ID for that user. This ID can then be passed to Bugzilla::User::new to
> create a new user.
> 
>-If no valid user exists with that login name, then the function will return 0.
>+If no valid user exists with that login name, then the function
>+either returns 0 or throws an error if $throw_error is true.

  Instead:

If no valid user exists with that login name, then the function
returns 0. However, if $throw_error is set, the function will
throw a UserError instead of returning.

Comment 6

[Frédéric Buclin]

Attachment: patch, v2

Now use constants for the min and max password lengths.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 220922 [details] [diff] [review]
patch, v2

Great, looks good.

Comment 8

[Frédéric Buclin]

Checking in editusers.cgi;
/cvsroot/mozilla/webtools/bugzilla/editusers.cgi,v  <--  editusers.cgi
new revision: 1.119; previous revision: 1.118
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.364; previous revision: 1.363
done
Checking in post_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/post_bug.cgi,v  <--  post_bug.cgi
new revision: 1.144; previous revision: 1.143
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.316; previous revision: 1.315
done
Checking in token.cgi;
/cvsroot/mozilla/webtools/bugzilla/token.cgi,v  <--  token.cgi
new revision: 1.40; previous revision: 1.39
done
Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.96; previous revision: 1.95
done
Checking in votes.cgi;
/cvsroot/mozilla/webtools/bugzilla/votes.cgi,v  <--  votes.cgi
new revision: 1.37; previous revision: 1.36
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.71; previous revision: 1.70
done
Checking in Bugzilla/Constants.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Constants.pm,v  <--  Constants.pm
new revision: 1.37; previous revision: 1.36
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.127; previous revision: 1.126
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.107; previous revision: 1.106
done
Checking in template/en/default/global/user-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/user-error.html.tmpl,v  <--  user-error.html.tmpl
new revision: 1.166; previous revision: 1.165
done