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)

r/a=mkanat

5 files changed, 35 insertions, 52 deletions

diff --git a/Bugzilla/DB.pm b/Bugzilla/DB.pm
index ccd86d781..6043aee00 100644
--- a/Bugzilla/DB.pm
+++ b/Bugzilla/DB.pm
@@ -397,8 +397,11 @@ sub sql_string_concat {
 
 sub sql_string_until {
     my ($self, $string, $substring) = @_;
-    return "SUBSTRING($string FROM 1 FOR " .
-                      $self->sql_position($substring, $string) . " - 1)";
+
+    my $position = $self->sql_position($substring, $string);
+    return "CASE WHEN $position != 0"
+             . " THEN SUBSTR($string, 1, $position - 1)"
+             . " ELSE $string END";
 }
 
 sub sql_in {
diff --git a/Bugzilla/DB/Oracle.pm b/Bugzilla/DB/Oracle.pm
index ceee1eb50..157eeca30 100644
--- a/Bugzilla/DB/Oracle.pm
+++ b/Bugzilla/DB/Oracle.pm
@@ -160,13 +160,6 @@ sub sql_string_concat {
     return 'CONCAT(' . join(', ', @params) . ')';
 }
 
-sub sql_string_until {
-    my ($self, $string, $substring) = @_;
-    return "SUBSTR($string, 1, " 
-           . $self->sql_position($substring, $string)
-           . " - 1)";
-}
-
 sub sql_to_days {
     my ($self, $date) = @_;
 
diff --git a/Bugzilla/DB/Pg.pm b/Bugzilla/DB/Pg.pm
index 386a67709..e59a638a4 100644
--- a/Bugzilla/DB/Pg.pm
+++ b/Bugzilla/DB/Pg.pm
@@ -192,18 +192,6 @@ sub sql_string_concat {
     return '(CAST(' . join(' AS text) || CAST(', @params) . ' AS text))';
 }
 
-sub sql_string_until {
-    my ($self, $string, $substring) = @_;
-
-    # PostgreSQL does not permit a negative substring length; therefore we
-    # use CASE to only perform the SUBSTRING operation when $substring can
-    # be found withing $string.
-    my $position = $self->sql_position($substring, $string);
-    return "CASE WHEN $position != 0"
-             . " THEN SUBSTRING($string FROM 1 FOR $position - 1)"
-             . " ELSE $string END";
-}
-
 # Tell us whether or not a particular sequence exists in the DB.
 sub bz_sequence_exists {
     my ($self, $seq_name) = @_;
diff --git a/Bugzilla/DB/Sqlite.pm b/Bugzilla/DB/Sqlite.pm
index fb6aaba97..e13fd18e1 100644
--- a/Bugzilla/DB/Sqlite.pm
+++ b/Bugzilla/DB/Sqlite.pm
@@ -237,12 +237,6 @@ sub sql_date_math {
     return "DATETIME($date, '$operator' || $interval || ' $units')";
 }
 
-sub sql_string_until {
-    my ($self, $string, $substring) = @_;
-    my $position = $self->sql_position($substring, $string);
-    return "SUBSTR($string, 1, $position - 1)"
-}
-
 ###############
 # bz_ methods #
 ###############
diff --git a/Bugzilla/Search.pm b/Bugzilla/Search.pm
index e6682fcc4..e0bb2355d 100644
--- a/Bugzilla/Search.pm
+++ b/Bugzilla/Search.pm
@@ -38,7 +38,6 @@ use base qw(Exporter);
 @Bugzilla::Search::EXPORT = qw(
     IsValidQueryType
     split_order_term
-    translate_old_column
 );
 
 use Bugzilla::Error;
@@ -55,6 +54,7 @@ use Bugzilla::Keyword;
 use Data::Dumper;
 use Date::Format;
 use Date::Parse;
+use Scalar::Util qw(blessed);
 use List::MoreUtils qw(all part uniq);
 use POSIX qw(INT_MAX);
 use Storable qw(dclone);
@@ -509,9 +509,14 @@ use constant COLUMN_JOINS => {
 # and we don't want it to happen at compile time, so we have it as a
 # subroutine.
 sub COLUMNS {
+    my $invocant = shift;
+    my $user = blessed($invocant) ? $invocant->_user : Bugzilla->user;
     my $dbh = Bugzilla->dbh;
     my $cache = Bugzilla->request_cache;
-    return $cache->{search_columns} if defined $cache->{search_columns};
+
+    if (defined $cache->{search_columns}->{$user->id}) {
+        return $cache->{search_columns}->{$user->id};
+    }
 
     # These are columns that don't exist in fielddefs, but are valid buglist
     # columns. (Also see near the bottom of this function for the definition
@@ -549,10 +554,10 @@ sub COLUMNS {
     );
 
     # Backward-compatibility for old field names. Goes new_name => old_name.
-    # These are here and not in translate_old_column because the rest of the
+    # These are here and not in _translate_old_column because the rest of the
     # code actually still uses the old names, while the fielddefs table uses
-    # the new names (which is not the case for the fields handled by 
-    # translate_old_column).
+    # the new names (which is not the case for the fields handled by
+    # _translate_old_column).
     my %old_names = (
         creation_ts => 'opendate',
         delta_ts    => 'changeddate',
@@ -567,10 +572,7 @@ sub COLUMNS {
 
     foreach my $col (@email_fields) {
         my $sql = "map_${col}.login_name";
-        # XXX This needs to be generated inside an accessor instead,
-        #     probably, because it should use $self->_user to determine
-        #     this, not Bugzilla->user.
-        if (!Bugzilla->user->id) {
+        if (!$user->id) {
              $sql = $dbh->sql_string_until($sql, $dbh->quote('@'));
         }
         $special_sql{$col} = $sql;
@@ -610,7 +612,10 @@ sub COLUMNS {
 }
 
 sub REPORT_COLUMNS {
-    my $columns = dclone(COLUMNS);
+    my $invocant = shift;
+    my $user = blessed($invocant) ? $invocant->_user : Bugzilla->user;
+
+    my $columns = dclone(blessed($invocant) ? $invocant->COLUMNS : COLUMNS);
     # There's no reason to support reporting on unique fields.
     # Also, some other fields don't make very good reporting axises,
     # or simply don't work with the current reporting system.
@@ -625,7 +630,7 @@ sub REPORT_COLUMNS {
 
     # If you're not a time-tracker, you can't use time-tracking
     # columns.
-    if (!Bugzilla->user->is_timetracker) {
+    if (!$user->is_timetracker) {
         push(@no_report_columns, TIMETRACKING_FIELDS);
     }
 
@@ -856,7 +861,7 @@ sub _sql_select {
         my $alias = $column;
         # Aliases cannot contain dots in them. We convert them to underscores.
         $alias =~ s/\./_/g;
-        my $sql = COLUMNS->{$column}->{name} . " AS $alias";
+        my $sql = $self->COLUMNS->{$column}->{name} . " AS $alias";
         push(@sql_fields, $sql);
     }
     return @sql_fields;
@@ -885,13 +890,13 @@ sub _validate_order_column {
 
     # Translate old column names
     my ($field, $direction) = split_order_term($order_item);
-    $field = translate_old_column($field);
+    $field = $self->_translate_old_column($field);
 
     # Only accept valid columns
-    return if (!exists COLUMNS->{$field});
+    return if (!exists $self->COLUMNS->{$field});
 
     # Relevance column can be used only with one or more fulltext searches
-    return if ($field eq 'relevance' && !COLUMNS->{$field}->{name});
+    return if ($field eq 'relevance' && !$self->COLUMNS->{$field}->{name});
 
     $direction = " $direction" if $direction;
     return "$field$direction";
@@ -1227,7 +1232,7 @@ sub _sql_group_by {
     my @extra_group_by;
     foreach my $column ($self->_select_columns) {
         next if $self->_skip_group_by->{$column};
-        my $sql = COLUMNS->{$column}->{name};
+        my $sql = $self->COLUMNS->{$column}->{name};
         push(@extra_group_by, $sql);
     }
 
@@ -2336,11 +2341,11 @@ sub _content_matches {
     #
     # We build the relevance SQL by modifying the COLUMNS list directly,
     # which is kind of a hack but works.
-    my $current = COLUMNS->{'relevance'}->{name};
+    my $current = $self->COLUMNS->{'relevance'}->{name};
     $current = $current ? "$current + " : '';
     # For NOT searches, we just add 0 to the relevance.
     my $select_term = $operator =~ /not/ ? 0 : "($current$rterm1 + $rterm2)";
-    COLUMNS->{'relevance'}->{name} = $select_term;
+    $self->COLUMNS->{'relevance'}->{name} = $select_term;
 }
 
 sub _long_descs_count {
@@ -2390,13 +2395,13 @@ sub _work_time_changedbefore_after {
 sub _work_time {
     my ($self, $args) = @_;
     $self->_add_extra_column('actual_time');
-    $args->{full_field} = COLUMNS->{actual_time}->{name};
+    $args->{full_field} = $self->COLUMNS->{actual_time}->{name};
 }
 
 sub _percentage_complete {
     my ($self, $args) = @_;
     
-    $args->{full_field} = COLUMNS->{percentage_complete}->{name};
+    $args->{full_field} = $self->COLUMNS->{percentage_complete}->{name};
 
     # We need actual_time in _select_columns, otherwise we can't use
     # it in the expression for searching percentage_complete.
@@ -2844,8 +2849,8 @@ sub split_order_term {
 
 # Used to translate old SQL fragments from buglist.cgi's "order" argument
 # into our modern field IDs.
-sub translate_old_column {
-    my ($column) = @_;
+sub _translate_old_column {
+    my ($self, $column) = @_;
     # All old SQL fragments have a period in them somewhere.
     return $column if $column !~ /\./;
 
@@ -2859,9 +2864,9 @@ sub translate_old_column {
     
     # 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 (%{ COLUMNS() }) {
-        next unless exists COLUMNS->{$key}->{name};
-        return $key if COLUMNS->{$key}->{name} eq $column;
+    foreach my $key (%{ $self->COLUMNS }) {
+        next unless exists $self->COLUMNS->{$key}->{name};
+        return $key if $self->COLUMNS->{$key}->{name} eq $column;
     }
 
     return $column;