Closed Bug 550299 Opened 14 years ago Closed 13 years ago

User fields are left blank in buglists and whines when local user accounts are used (i.e. they have no @company.com suffix)

Categories

(Bugzilla :: Query/Bug List, defect, P2)

3.6.3
defect

Tracking

()

RESOLVED FIXED
Bugzilla 4.0

People

(Reporter: rcastro, Assigned: LpSolit)

References

Details

Attachments

(3 files, 1 obsolete file)

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.
Hum, I cannot reproduce the problem. Maybe you use custom templates, which would explain the problem you see.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → WORKSFORME
Version: unspecified → 3.4.5
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.
(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.
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.
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.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
If I run the same query as a logged in user, everything works fine, and the assignee field is populated as expected.
Summary: Assignee is left blank in Whines → Assignee is left blank in Whines, anonymous searches
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.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → WORKSFORME
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.
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
Summary: Assignee is left blank in Whines, anonymous searches → Assignee is left blank in Whines, anonymous searches when users are configured to use an Email Suffix with no fully qualified username
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.
Version: 3.4.5 → 3.6.1
Version: 3.6.1 → 3.6.3
Keywords: qawanted
Priority: P1 → --
OK, I can reproduce the issue with local user accounts.
Assignee: whining → query-and-buglist
Status: UNCONFIRMED → NEW
Component: Whining → Query/Bug List
Ever confirmed: true
Keywords: qawanted
Priority: -- → P2
Summary: Assignee is left blank in Whines, anonymous searches when users are configured to use an Email Suffix with no fully qualified username → User fields are left blank in buglists and whines when local user accounts are used (i.e. they have no @company.com suffix)
Target Milestone: --- → Bugzilla 4.0
Attached patch patch, v1 (obsolete) — Splinter Review
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.
Assignee: query-and-buglist → LpSolit
Status: NEW → ASSIGNED
Attachment #578855 - Flags: review?(mkanat)
Attachment #578855 - Flags: review?(glob)
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.
Attachment #578855 - Flags: review?(mkanat) → review+
Oh, also, rename translate_old_column to _translate_old_column, while you're at it--it's not used outside of Search.pm anymore.
The code that's modified here is most likely very different on 4.0 than on 4.2.
Flags: approval4.2+
Flags: approval+
Attachment #578855 - Flags: review?(glob)
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.
Attachment #578855 - Attachment is obsolete: true
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.
Requesting review for this backport for 4.0.3. Tested, works.
Attachment #579110 - Flags: review?(mkanat)
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.
Attachment #579110 - Flags: review?(mkanat) → review+
Setting approval to ? and I'll let LpSolit set it to + if he's satisfied with the answers to my comments.
Flags: approval4.0?
(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!
Flags: approval4.0? → approval4.0+
Keywords: relnote
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.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago13 years ago
Resolution: --- → FIXED
relnote added as part of bug 713345.
Keywords: relnote
You need to log in before you can comment on or make changes to this bug.