338793 - Remove DBID_to_name() from globals.pl

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement)

Description

[Frédéric Buclin]

DBID_to_name() can probably be replaced by Bugzilla::User->new($user_id)->name. We just have to make sure we don't break the code when we passed an unknown or invalid user ID.

Comment 1

[Max Kanat-Alexander]

(In reply to comment #0)
> DBID_to_name() can probably be replaced by Bugzilla::User->new($user_id)->name.
> We just have to make sure we don't break the code when we passed an unknown or
> invalid user ID.

  Shouldn't that be "new Bugzilla::User($user_id)->login" instead of ->name?

Comment 2

[Frédéric Buclin]

(In reply to comment #1)
>   Shouldn't that be "new Bugzilla::User($user_id)->login" instead of ->name?

err... ->login instead of ->name, yes. But
"new Bugzilla::User($user_id)->login" won't work, you have to write
"Bugzilla::User->new($user_id)->login".

Comment 3

[Frédéric Buclin]

Attachment: patch, v1

The patch assumes the one about get_product_* to be checked in first (else you have a conflict in globals.pl). Note that I use a user cache almost everywhere.

Comment 4

[Frédéric Buclin]

Hum, I have to admit that my patch doesn't make sure the user exists, i.e. $user->login may fail if the user account has been deleted. On the other hand, we should fine a more efficient way to write:

$foo = $user ? $user->login : '';

Comment 5

[Frédéric Buclin]

Attachment: patch, v2

I fixed all the places where a deleted user account could be a problem. The ones that are left untouched have already been validated.

Comment 6

[Vlad Dascalu]

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

+    # still valid before looking for there login name. Else we would crash.

"there" is incorrect in this context.

+    # Avoid extraneaous SQL queries.

Google says: "Did you mean: extraneous"

Comment 7

[Max Kanat-Alexander]

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

>Index: process_bug.cgi
> # from buglist.cgi, or just the one bug when called from
> # show_bug.cgi).
> #
>+# We are going to handle several user objects, maybe several times
>+# the same ones. It worths a cache.
>+my %users;

  "Maybe several times the same ones" doesn't make a lot of sense.

  Also: "It's worth"

  Also, premature optimization is the root of all evil.

  Instead of all this either/or stuff, have you considered making a login_name exported function from Bugzilla::User that just does DBID_to_name without the fancy cache, and returns the same thing that DBID_to_name did?

  We'll eventually be replacing most of those calls with User obects anyhow, but right now it's silly to create a User object just to get the login name out of it, and look at all the code complexity it creates!

Comment 8

[Frédéric Buclin]

Attachment: patch, v3

Comment 9

[Vlad Dascalu]

Comment on attachment 226164 [details] [diff] [review]
patch, v3

I'd feel better with the function rejecting values like "01" or "0001" (numbers with leading zeros), as the previous version of the function did, but it seems this is not critical and it works because we're not inserting into DB IDs received from user input.

+Returns the login name of the user account for the given user ID. If no
+valid user ID is given or has no entry in the profiles table, we return
+an emtpy string.

"emtpy" => "empty"
"or has" => "or the user has"

(fixable on checkin)

Comment 10

[Frédéric Buclin]

Checking in editproducts.cgi;
/cvsroot/mozilla/webtools/bugzilla/editproducts.cgi,v  <--  editproducts.cgi
new revision: 1.123; previous revision: 1.122
done
Checking in globals.pl;
/cvsroot/mozilla/webtools/bugzilla/globals.pl,v  <--  globals.pl
new revision: 1.371; previous revision: 1.370
done
Checking in process_bug.cgi;
/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.327; previous revision: 1.326
done
Checking in Bugzilla/BugMail.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/BugMail.pm,v  <--  BugMail.pm
new revision: 1.79; previous revision: 1.78
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.112; previous revision: 1.111
done
Checking in contrib/bug_email.pl;
/cvsroot/mozilla/webtools/bugzilla/contrib/bug_email.pl,v  <--  bug_email.pl
new revision: 1.41; previous revision: 1.40
done