449705 - Make buglist.cgi's LookupNamedQuery use Bugzilla::Search::Saved

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Max Kanat-Alexander]

This is a good first step for bug 449697--we'll just replace the direct SQL calls in LookupNamedQuery with calls to Bugzilla::Search::Saved.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

I've tested this except for Shared Searches.

Comment 2

[Frédéric Buclin]

Comment on attachment 332879 [details] [diff] [review]
v1

>Index: buglist.cgi

>+    my $query_user = $sharer_id || $user;

This is useless. new() already falls back to Bugzilla->user if the value you pass is undefined (or 0).


>+        || ($query->user->id != $user->id
>+            && !$user->in_group($query->shared_with_group)) )

in_group() takes a group name. So pass $query->shared_with_group->name if the group is defined. Changing the way in_group() works for this specific case doesn't make sense to me. (More below)


>+    if ($query->user->id != $user->id
>+        && !$user->in_group($query->shared_with_group)) 
>+    {
>             ThrowUserError("missing_query", {'queryname' => $name,
>                                              'sharer_id' => $sharer_id});
>-        }
>     }

You already did this check above.



>Index: Bugzilla/User.pm

> sub in_group {
>     my ($self, $group, $product_id) = @_;
>+    $group = $group->name if blessed $group;

This change is useless. This means you could pass either a group name or a group object. That's confusing (the POD doesn't even reflect this) and not useful for now (see above). Moreover, if there is no shared group above, in_group() will throw a warning because of "$_ eq $group", where $group is undefined. For now, leave in_group() as is.



>Index: Bugzilla/Search/Saved.pm

>+sub new {

>+    my $dbh = Bugzilla->dbh;

Nit: you don't need $dbh.


>+    if (ref $param) {
>+        $user = $param->{user} || Bugzilla->user;

Make clear in the POD that $param->{user} can be either an object or an ID.


>+        my $name = $param->{name};
>+        if (!defined $name) {
>+            ThrowCodeError('bad_arg',

If an empty name is passed, we should also catch it. Should be if (!$name) IMO.


>+        my $user_id = blessed $user ? $user->id : $user;

Please make sure $user_id is an integer (in case someone passed a string).


>+        $param = { condition => $condition, values => \@values };

The POD says you cannot pass a name to new(). Please fix the POD as well.

Otherwise looks good.

Comment 3

[Max Kanat-Alexander]

Attachment: v2

Okay, this patch is way better--it moves most of the important logic into check().

Comment 4

[Frédéric Buclin]

Comment on attachment 395166 [details] [diff] [review]
v2

Everything looks good and works fine as long as you don't hack the URL. The following two cases must be fixed, though:

1) Append &sharer_id=foo to one of your saved searches (or replace the sharer ID of a search shared with you by a string):

DBD::Pg::db selectrow_hashref failed: ERREUR:  syntaxe en entrée invalide pour l'entier : « foo » [for Statement "SELECT id,userid,name,query,query_type FROM namedqueries WHERE userid = ? AND name = ?"] at Bugzilla/Object.pm line 106

The reason is because PostgreSQL expects to get an integer for the user ID, not a string. So this parameter isn't validated, which is bad.


2) Use this URL: buglist.cgi?cmdtype=runnamed&namedcmd=foo (assuming you don't have a saved search named "foo"). The UI is broken. No Dusk skin anymore (it falls back to the default one), and half of the links in the header/footer are gone.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #4)
> 1) Append &sharer_id=foo to one of your saved searches (or replace the sharer
> ID of a search shared with you by a string):

  That's the case for all implementations of Bugzilla::Object->check that use an id. If we want to fix that, we should probably fix it in another bug. Bug I might fix it here, too.

> 2) Use this URL: buglist.cgi?cmdtype=runnamed&namedcmd=foo (assuming you don't
> have a saved search named "foo"). The UI is broken. No Dusk skin anymore (it
> falls back to the default one), and half of the links in the header/footer are
> gone.

  Yeah, I noticed that too. I have no idea why that happens. I'll look into it.

Comment 6

[Max Kanat-Alexander]

Attachment: v3

Okay, actually, you were right, it was indeed only a problem for Bugzilla::Search::Saved (the id problem). I've fixed both things you mentioned!

Comment 7

[Frédéric Buclin]

Comment on attachment 395527 [details] [diff] [review]
v3

Looks good, and the two broken testcases I reported are now fixed. r=LpSolit

Comment 8

[Max Kanat-Alexander]

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.404; previous revision: 1.403
done
Checking in Bugzilla/Object.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Object.pm,v  <--  Object.pm
new revision: 1.35; previous revision: 1.34
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.192; previous revision: 1.191
done
Checking in Bugzilla/Search/Saved.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Saved.pm,v  <--  Saved.pm
new revision: 1.9; previous revision: 1.8
done
Checking in template/en/default/global/code-error.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/code-error.html.tmpl,v  <--  code-error.html.tmpl
new revision: 1.115; previous revision: 1.114
done
Checking in template/en/default/global/per-bug-queries.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/per-bug-queries.html.tmpl,v  <--  per-bug-queries.html.tmpl
new revision: 1.14; previous revision: 1.13
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.285; previous revision: 1.284
done