236779 - Add UI for changing "linkinfooter" flag for saved searches

Status: RESOLVED FIXED

(Bugzilla :: User Accounts, defect, P2)

Description

[Gervase Markham [:gerv]]

We need to add linkinfooter UI to the new Saved Searches prefs panel.

Gerv

Comment 1

[Gervase Markham [:gerv]]

And perhaps allow people to edit the names - bug 156662.

Gerv

Comment 2

[Dave Miller [:justdave]]

Gerv, did you have a plan for this, or want someone else to take it?

Comment 3

[Gervase Markham [:gerv]]

Anyone who has time could do it, I guess. It's not hard - just add an extra
column of checkboxes to the UI, get the data to populate them in
DoSavedSearches(), and write SaveSavedSearches() to write the results to the
database. I'd forget name editing at this stage - there are issues there, and
it's not a blocker.

One catch: you may also have to mess with Bugzilla->user or somesuch to make
sure the footer on the results page shows the right thing.

Gerv

Comment 4

[Joel Peshkin]

Attachment: The patch

Comment 5

[Gervase Markham [:gerv]]

Comment on attachment 147859 [details] [diff] [review]
The patch

>-# No SaveSavedSearches() because this panel has no changeable fields (yet).
>+sub SaveSavedSearches() {
>+    my $cgi = Bugzilla->cgi;
>+    my $dbh = Bugzilla->dbh;
>+    my @queries = @{Bugzilla->user->queries};
>+    my $sth = $dbh->prepare("UPDATE namedqueries SET linkinfooter = ?
>+                          WHERE userid = ?
>+                          AND name = ?");

AIUI, you shouldn't use a placeholder for userid because it doesn't change.

>+    foreach my $q (@queries) {
>+        if ($q->{linkinfooter} 
>+            && !defined($cgi->param("linkinfooter_" . $q->{name}))) {

This logic is hard to read immediately - why not simplify, and just UPDATE
whatever the current state is?

Nit: I believe most Bugzilla code quotes hash references in single quotes.

>+            $sth->execute(0, $userid, $q->{name});
>+        } elsif (!$q->{linkinfooter} 
>+            && defined($cgi->param("linkinfooter_" . $q->{name}))) {
>+            $sth->execute(1, $userid, $q->{name});
>+        }
>+    }
>+    Bugzilla->user->flush_queries_cache;

Nit: we tend to put blank lines after closing quotes, unless they are followed
by another closing quote.

>Index: template/en/default/account/prefs/saved-searches.html.tmpl
>===================================================================
> <blockquote>
>+  Show query in page footer
>   <table cellpadding="3">  
>+    <tr>
>+      <th>
>+        |
>+      </th>
>+    </tr>

Ooh, ick. Now that it needs it, can we convert this table to have proper
headings?

>     [% FOREACH q = queries %]
>       <tr>
>+        <td>
>+          <input type="checkbox" 
>+                 name="linkinfooter_[% q.name FILTER html %]"

Are you certain this (and the corresponding Perl) works whatever nasty
characters you put in the query name?

Gerv

Comment 6

[Joel Peshkin]

It does work with eascaped characters like "%" in the name.

I'll de-nit this and submit a revised patch.

Comment 7

[Gervase Markham [:gerv]]

Great :-) Thanks for doing this, by the way. Feel free to take the bug.

Gerv

Comment 8

[Dave Miller [:justdave]]

(In reply to comment #5)
> (From update of attachment 147859 [details] [diff] [review])
> >+    my $sth = $dbh->prepare("UPDATE namedqueries SET linkinfooter = ?
> >+                          WHERE userid = ?
> >+                          AND name = ?");
> 
> AIUI, you shouldn't use a placeholder for userid because it doesn't change.

But it does change between invocations, when different users connect, and using
a placeholder for it allows MySQL 4's server-side query caching to work.  (not
that it's a terribly intensive query anyway)

Comment 9

[Joel Peshkin]

Attachment: Patch v2

Changed format to use a table and cleaned up SaveSavedSearches code.

Comment 10

[J. Paul Reed [:preed]]

Comment on attachment 147949 [details] [diff] [review]
Patch v2

>+            defined($cgi->param("linkinfooter_$q->{name}")) ? 1 : 0;
>+            $sth->execute($linkinfooter, $userid, $q->{name});

Aestheticallly, this should be $q->{'name'}.

Other than that, r=preed.

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 147949 [details] [diff] [review]
Patch v2

r=gerv too. Sorry for the delay.

Gerv

Comment 12

[Joel Peshkin]

Checking in userprefs.cgi;
/cvsroot/mozilla/webtools/bugzilla/userprefs.cgi,v  <--  userprefs.cgi
new revision: 1.58; previous revision: 1.57
done
Checking in template/en/default/account/prefs/prefs.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/account/prefs/prefs.html.tmpl,v
 <--  prefs.html.tmpl
new revision: 1.15; previous revision: 1.14
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.2; previous revision: 1.1
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.25; previous revision: 1.24
done