Last Comment Bug 550299 - User fields are left blank in buglists and whines when local user accounts are used (i.e. they have no @company.com suffix)
: User fields are left blank in buglists and whines when local user accounts ar...
Status: RESOLVED FIXED
:
Product: Bugzilla
Classification: Server Software
Component: Query/Bug List (show other bugs)
: 3.6.3
: All All
: P2 normal (vote)
: Bugzilla 4.0
Assigned To: Frédéric Buclin
: default-qa
Mentors:
: 581189 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-03-04 13:08 PST by Ryan Castro
Modified: 2011-12-26 06:56 PST (History)
4 users (show)
mkanat: approval+
mkanat: approval4.2+
LpSolit: approval4.0+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
patch, v1 (8.56 KB, patch)
2011-12-03 12:19 PST, Frédéric Buclin
mkanat: review+
Details | Diff | Review
patch for trunk, v1.1 (9.67 KB, patch)
2011-12-05 09:10 PST, Frédéric Buclin
no flags Details | Diff | Review
patch for 4.2, v1 (6.14 KB, patch)
2011-12-05 09:27 PST, Frédéric Buclin
no flags Details | Diff | Review
patch for 4.0.3, v1 (6.96 KB, patch)
2011-12-05 10:05 PST, Frédéric Buclin
mkanat: review+
Details | Diff | Review

Description Ryan Castro 2010-03-04 13:08:33 PST
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US) AppleWebKit/533.1 (KHTML, like Gecko) Chrome/5.0.335.1 Safari/533.1
Build Identifier: 3.4.5

When scheduling any kind of whine, the assignee field is always left blank in the HTML emails that are sent out.

Reproducible: Always

Steps to Reproduce:
1. Create a whine report
2. wait for whine to send
3. Check email
Actual Results:  
No assignee

Expected Results:  
The bugs Assignee listed.

This is a clone of bug 511216. It looks like the bug was fixed for 3.4.5, but whining appears to have changed considerably (looking at whine.pl, the query field text looks to have changed a lot recently) so it wouldn't surprise me if the bug was fixed, and reintroduced later in the development process.

----
Severity justification:
I have justified this as a Normal bug that needs fixing in that, an essential part of whining is finding out who needs to do what, and when. If the assignee isn't populated, you might as well just load your saved search yourself, because whining hasn't done much for you.

If this bug is confirmed and resolved, a patch for v 3.4.5 would be greatly appreciated.
Comment 1 Frédéric Buclin 2010-03-04 14:14:00 PST
Hum, I cannot reproduce the problem. Maybe you use custom templates, which would explain the problem you see.
Comment 2 Max Kanat-Alexander 2010-03-04 16:31:37 PST
I've seen the problem, but it was fixed in a 3.4.x release, so this is actually a dupe of a fixed bug.
Comment 3 Frédéric Buclin 2010-03-05 11:47:19 PST
(In reply to comment #2)
> I've seen the problem, but it was fixed in a 3.4.x release, so this is actually
> a dupe of a fixed bug.

Well, the bug was fixed in 3.4.5, see bug 511216. The reporter complained that he was still seeing the bug in 3.4.5, which would mean a regression rather than a dupe. So WFM is more appropriate than duplicate here.
Comment 4 Ryan Castro 2010-03-05 12:05:59 PST
As the original bug poster, i'd have to agree with Frédéric.

However, I double-checked my templates (no changes from release) and recompiled them from checksetup.pl, just to be sure. Doesn't seem to have made any difference.

I'll continue troubleshooting. Any recommendations for where I might look? The templates, whine.pl, and search.pm are all fresh from 345 release code.
Comment 5 Ryan Castro 2010-03-31 07:07:04 PDT
additional information:
If I run an Advanced bugzilla query (Simple. I just added a keyword and pulled back all the resolved bugs in our system) *as a user not logged in*,  the list page shows no assignee in the fields. If I view an individual bug, I can see the assignee.

This seems to be the same problem affecting whines. Again, I am on 3.4.5.

either this is a bug, or something is wrong in my system but checksetup and sanity check all check out fine, and I've never had a problem with upgrades.
Comment 6 Ryan Castro 2010-03-31 07:08:21 PDT
If I run the same query as a logged in user, everything works fine, and the assignee field is populated as expected.
Comment 7 Frédéric Buclin 2010-05-22 17:43:20 PDT
Running queries while being logged out are all displayed correctly. The assignee field is populated correctly. Such a huge problem would have been reported many times if the problem was real. Definitely looks like a local problem with your installation than a real bug in Bugzilla.
Comment 8 Ryan Castro 2010-07-27 15:12:51 PDT
Reopening this P1 Issue as I believe I have found the root cause:
Currently running Bugzilla 3.6.1

If a system is configured to use an e-mail suffix (emailsuffix param)
and email regexp  (emailregexp param) is configured as "^[^@]+", bugzilla users will have no email address configured for their login_name in profiles.

This creates a problem when Whine.pl calls Search.pm to create it's search criteria, sending "assigned_to" as a parameter.

Search.pm later merges assigned_to with Profiles, calling sql_string_until from db.pm. This functional trims out e-mail addresses, leaving users with only a username, which eventually populates the assignee field of a Whine.

Unfortunately, this function returns nothing when a login_name doesn't have an e-mail address as a login_id, which eventually leads to a blank assignee field.
Comment 9 Ryan Castro 2010-07-27 16:00:46 PDT
To fix this for our local installation, I'm proposing making the following change to bugzilla/Search.pm:

###Replace Line 151:
        if (!Bugzilla->user->id) {

###With:
if (!Bugzilla->user->id && length Bugzilla->params->{'emailsuffix'} == 0) {



I figured the best way to determine whether or not a user was going to be a local username was simply to see whether or not email suffix was being deployed.

This may or may not be the best solution. This is my first venture into BZ code.
Comment 10 Frédéric Buclin 2011-12-03 10:42:23 PST
OK, I can reproduce the issue with local user accounts.
Comment 11 Frédéric Buclin 2011-12-03 11:01:37 PST
*** Bug 581189 has been marked as a duplicate of this bug. ***
Comment 12 Frédéric Buclin 2011-12-03 12:19:13 PST
Created attachment 578855 [details] [diff] [review]
patch, v1

There were two problems:

- first, Bugzilla::Search::COLUMNS was always looking at Bugzilla->user instead of $self->_user when available to get the user object. This made whine.pl to always truncate the assignee email address.

- second, $dbh->sql_string_until() was broken everywhere except on PostgreSQL, because if the 3rd argument of SUBTR() or SUBSTRING() is < 1, then an empty string or NULL is returned. PostgreSQL was behaving correctly, because it had a check about what to do. As the SUBSTR(string, from, len) syntax is understood by all supported DB (MySQL, Pg, Oracle and SQLite), I used it for all DB.

I tested my patch on MySQL with buglists and whines, and this patch fixes the problem.
Comment 13 Max Kanat-Alexander 2011-12-05 04:14:24 PST
Comment on attachment 578855 [details] [diff] [review]
patch, v1

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

A few things to fix on checkin:

::: Bugzilla/Search.pm
@@ +530,3 @@
>      my $dbh = Bugzilla->dbh;
>      my $cache = Bugzilla->request_cache;
>      return $cache->{search_columns} if defined $cache->{search_columns};

Columns are cached per-request, but they could theoretically change per-user, even though they don't now. Probably the best thing to do is to make search_columns a cache per-user-id. (So, like $cache->{search_columns}->{$user->id}.)

@@ +2877,4 @@
>      
>      # If it doesn't match the regexps above, check to see if the old 
>      # SQL fragment matches the SQL of an existing column
> +    foreach my $key (%{ $self->COLUMNS() }) {

Nit: You could get rid of the () there now, if you wanted.
Comment 14 Max Kanat-Alexander 2011-12-05 04:15:24 PST
Oh, also, rename translate_old_column to _translate_old_column, while you're at it--it's not used outside of Search.pm anymore.
Comment 15 Max Kanat-Alexander 2011-12-05 04:16:14 PST
The code that's modified here is most likely very different on 4.0 than on 4.2.
Comment 16 Frédéric Buclin 2011-12-05 09:10:27 PST
Created attachment 579083 [details] [diff] [review]
patch for trunk, v1.1

Here is the patch with fixes on checkin, for trunk. As _translate_old_column() is now a private method, I removed it from the export list. I didn't rename it in 4.2 because translate_old_column() is still called from buglist.cgi.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm
modified Bugzilla/DB/Oracle.pm
modified Bugzilla/DB/Pg.pm
modified Bugzilla/DB/Sqlite.pm
Committed revision 8030.
Comment 17 Frédéric Buclin 2011-12-05 09:27:28 PST
Created attachment 579091 [details] [diff] [review]
patch for 4.2, v1

Here is the patch for 4.2, where I didn't touch translate_old_column() at all as it's still called from buglist.cgi.

Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.2/
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm
modified Bugzilla/DB/Oracle.pm
modified Bugzilla/DB/Pg.pm
modified Bugzilla/DB/Sqlite.pm
Committed revision 7973.
Comment 18 Frédéric Buclin 2011-12-05 10:05:28 PST
Created attachment 579110 [details] [diff] [review]
patch for 4.0.3, v1

Requesting review for this backport for 4.0.3. Tested, works.
Comment 19 Max Kanat-Alexander 2011-12-06 01:14:00 PST
Comment on attachment 579110 [details] [diff] [review]
patch for 4.0.3, v1

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

::: Bugzilla/DB.pm
@@ +368,5 @@
> +
> +    my $position = $self->sql_position($substring, $string);
> +    return "CASE WHEN $position != 0"
> +             . " THEN SUBSTR($string, 1, $position - 1)"
> +             . " ELSE $string END";

I do have one slight concern here. All other columns in Search.pm that are expressions have parens around them. This won't. Should we be putting parens somewhere?

I'm more concerned about these things on stable branches than I am on trunk/next branches.

::: Bugzilla/Search.pm
@@ +372,2 @@
>  
>      Bugzilla::Hook::process('buglist_columns', { columns => \%columns });

We'll want to relnote that this is now called once per-user instead of once per-request. We should probably also start passing $user in to the hook and document that, on all the branches. (We can do that in another bug if you want.)

@@ +372,5 @@
>  
>      Bugzilla::Hook::process('buglist_columns', { columns => \%columns });
>  
> +    $cache->{search_columns}->{$user->id} = \%columns;
> +    return $cache->{search_columns}->{$user->id};

You know, it also occurred to me that when we have $invocant as an object, we can cache in the object and don't need to use the request cache. But that's not critical and we can change that later if we want to.
Comment 20 Max Kanat-Alexander 2011-12-06 01:14:32 PST
Setting approval to ? and I'll let LpSolit set it to + if he's satisfied with the answers to my comments.
Comment 21 Frédéric Buclin 2011-12-06 03:45:35 PST
(In reply to Max Kanat-Alexander from comment #19)
> I do have one slight concern here. All other columns in Search.pm that are
> expressions have parens around them. This won't. Should we be putting parens
> somewhere?

Parens are not needed because the CASE WHEN ... END is a block in itself, so it cannot interact with other expressions. We would be forced to add parens if there was an AND or OR in this expression. Note that Pg.pm didn't have parens either. :)


> We should probably also start passing $user in to the hook and
> document that, on all the branches. (We can do that in another bug if you
> want.)

Yeah, a separate bug is a good idea. I will let you file it.


Thanks for the reviews!
Comment 22 Frédéric Buclin 2011-12-06 03:52:22 PST
Committing to: bzr+ssh://lpsolit%40gmail.com@bzr.mozilla.org/bugzilla/4.0/
modified Bugzilla/DB.pm
modified Bugzilla/Search.pm
modified Bugzilla/DB/Oracle.pm
modified Bugzilla/DB/Pg.pm
Committed revision 7665.
Comment 23 Frédéric Buclin 2011-12-26 06:56:51 PST
relnote added as part of bug 713345.

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