352403 - Create an object for saved searches, and have Bugzilla::User use it

Status: RESOLVED FIXED

(Bugzilla :: Query/Bug List, enhancement)

Description

[Max Kanat-Alexander]

I think the object will be called Bugzilla::SavedSearch, or Bugzilla::Search::Saved. I think the second one might be ideal, because then I could eventually actually make it a real subclass of Bugzilla::Search.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Here it is. Everything is working fine, EXCEPT the template code in places like useful-links, and I have no idea why.

Comment 2

[Max Kanat-Alexander]

Attachment: v2

Okay, this one works! :-)

Comment 3

[Max Kanat-Alexander]

This is a blocker because bug 319598 is a blocker.

Comment 4

[Frédéric Buclin]

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

>? Bugzilla/Search/Saved.pm

hum... Please include this module too.


>+    require Bugzilla::Search::Saved;
>+    $self->{queries} = Bugzilla::Search::Saved->new_from_list($query_ids);


The module is missing. I cannot test anything.

Comment 5

[Max Kanat-Alexander]

Attachment: v3

Okay, this fixes the bitrot, and it also has the actual Saved.pm module.

I tested it lightly and it's working.

Comment 6

[Frédéric Buclin]

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

>Index: userprefs.cgi

>-    my $sth_check_nl = $dbh->prepare('SELECT COUNT(*)
>-                                        FROM namedqueries_link_in_footer
>-                                       WHERE namedquery_id = ?
>-                                         AND user_id = ?');

>-                            $sth_check_nl->execute($q->{'id'}, $user_id);
>-                            my ($already_shown_in_footer) =
>-                                $sth_check_nl->fetchrow_array();
>-                            if (! $already_shown_in_footer) {
>-                                $sth_insert_nl->execute($q->{'id'}, $user_id);
>-                            }

>+                        if (!$q->link_in_footer) {
>+                            $sth_insert_nl->execute($q->id, $user_id);
>                         }

What you removed is about other users' "link in footer" while you added code for the current user's "link in footer".


>+            if ($group_map_entries > 0) {
>+                $sth_delete_ngm->execute($q->{'id'});
>+            }

Should be $q->id.



>Index: Bugzilla/Search/Saved.pm

>+sub used_in_whine {

>+    ($self->{used_in_whine}) = Bugzilla->dbh->selectrow_array(
>+        'SELECT 1 FROM whine_events INNER JOIN whine_queries
>+                       ON whine_events.id = whine_queries.eventid
>+          WHERE whine_events.owner_userid = ?', undef, $self->{userid});

The ID of the query is missing. Here, you get 1 if there is at least one query which is displayed in the footer. What we want is for this specific query.


In POD:

>+ my $query = new Bugzilla::Search::Saved(
>+     { user => Bugzilla->user, name => $query_name });

AFAIK, the new() method will only work when passing a query ID, not when passing a user object and a query name. Object.pm cannot guess what to do with them.


>+ my $report_params = $query->cgi;

I see no cgi() defined in this module. Maybe are you talking about $query->url?



>Index: template/en/default/account/prefs/saved-searches.html.tmpl

You forgot to replace usedinwhine with used_in_whine.



And you could also use bug_ids_only() in global/per-bug-queries.html.tmpl instead of the existing code.


I didn't test your patch yet so I may find other problems in a future review. But globally, this looks good.

Comment 7

[Max Kanat-Alexander]

(In reply to comment #6)
> >-                            $sth_check_nl->execute($q->{'id'}, $user_id);
> What you removed is about other users' "link in footer" while you added code
> for the current user's "link in footer".

  Eh, what? That code has nothing to do with other users. Note the $user_id.

  I'll fix everything else.

Comment 8

[Max Kanat-Alexander]

Attachment: v4

Okay, here we go! I've fixed all the things you pointed out.

Comment 9

[Frédéric Buclin]

(In reply to comment #7)
> (In reply to comment #6)
> > >-                            $sth_check_nl->execute($q->{'id'}, $user_id);
> > What you removed is about other users' "link in footer" while you added code
> > for the current user's "link in footer".
> 
>   Eh, what? That code has nothing to do with other users. Note the $user_id.


Read the code carefully:

>-                        my $user_id;
>-                        while ($user_id =
>-                               $sth_direct_group_members->fetchrow_array()) {
>-                            next if $user_id == $user->id;

$user_id comes from the SQL query, and this last line also shows you that $user_id and $user->id are two distinct things.

Comment 10

[Frédéric Buclin]

Max, could you update your patch per my previous comment?

Comment 11

[Max Kanat-Alexander]

Attachment: v5

Okay, you're right. I fixed it in a nice object-oriented way.

Note also that now regexp members are also considered to be "in" the group when we're automatically adding people (when we're a blesser). This makes sense for how most companies use regexp groups, that I've seen.

Comment 12

[Frédéric Buclin]

Comment on attachment 244365 [details] [diff] [review]
v5

>Index: userprefs.cgi

> sub DoSavedSearches {

    my $sth_check_ngm = $dbh->prepare('SELECT COUNT(*)
                                         FROM namedquery_group_map
                                        WHERE namedquery_id = ?');


This SQL query isn't used anymore. Please remove it.



>Index: Bugzilla/Search/Saved.pm

>+sub used_in_whine {

>+    ($self->{used_in_whine}) = Bugzilla->dbh->selectrow_array(
>+        'SELECT 1 FROM whine_events INNER JOIN whine_queries
>+                       ON whine_events.id = whine_queries.eventid
>+          WHERE whine_events.owner_userid = ? AND query_name = ?', undef, 
>+          $self->{userid}, $self->name);

Nit: you should append || 0 in case there is no row returned. Else you will set 'used_in_whine' to undef instead of 0.


>+    return $self->{used_in_whine} ? 1 : 0;

Nit: per my previous comment, | ? 1 : 0 | should now be useless.


>+sub shared_with_group {

>+    $self->{shared_with_group} = $group_id ? new Bugzilla::Group($group_id) 

You should either use or require Bugzilla::Group before calling new().



One more thing: in global/user-error.html.tmpl at line 1540, [% q.query FILTER html %] should now be [% q.url FILTER html %].


Your patch seems to work correctly. No problem found so far. This is a nice cleanup. r=LpSolit

Comment 13

[Frédéric Buclin]

Please fix all my previous comments on checkin.

Comment 14

[Max Kanat-Alexander]

Okay, I did all the checkin fixes.

Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.108; previous revision: 1.107
done
Checking in Bugzilla/Group.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Group.pm,v  <--  Group.pm
new revision: 1.19; previous revision: 1.18
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.139; previous revision: 1.138
done
RCS file: /cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Saved.pm,v
done
Checking in Bugzilla/Search/Saved.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Saved.pm,v  <--  Saved.pm
initial revision: 1.1
done
Checking in template/en/default/account/prefs/saved-searches.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/saved-searches.html.tmpl,v  <--  saved-searches.html.tmpl
new revision: 1.11; previous revision: 1.10
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.7; previous revision: 1.6
done
Checking in template/en/default/global/site-navigation.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/site-navigation.html.tmpl,v  <--  site-navigation.html.tmpl
new revision: 1.20; previous revision: 1.19
done
Checking in template/en/default/global/useful-links.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/global/useful-links.html.tmpl,v  <--  useful-links.html.tmpl
new revision: 1.52; previous revision: 1.51
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.tmplnew revision: 1.195; previous revision: 1.194
done