308228 - Whine-related saved search can be forgotten through buglist.cgi

Status: RESOLVED FIXED

(Bugzilla :: Whining, defect)

Description

[A. Karl Kornel]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US) AppleWebKit/125.4 (KHTML, like Gecko, Safari) OmniWeb/v563.51
Build Identifier: 

Saved Searches that are associated with whines should not be forgotten.  This is already enforced in the 
Saved Searches prefs tab.  However, it is still possible for a user to execute a saved search and then 
click on the "Forget" link at the bottom of the results!

Reproducible: Always

Steps to Reproduce:
1. Create a new saved search
2. Go to Whining.  Create a new whine that references the saved search
3. Go to Prefs->Saved Searches.  Notice how the saved search from (1) can not be forgotten because it 
is being used in a whine.
4. Run the saved search from (1)
5. Click on the Forget Search link
Actual Results:  
The search was forgotten

Expected Results:  
The option to forget the search should be unavailable, because it is currently associated with a whine.

I am not completely sure what will happen when whine.pl gets to the whine and discovers that the 
query associated with it does not exist.

Comment 1

[Max Kanat-Alexander]

We should just throw an error stating that the search is used in a whine, and
ideally list which whines.

Comment 2

[A. Karl Kornel]

Attachment: Patch v1

This should fix the problem in the manner described in comment 1.

I don't know who to ask to review this.

Comment 3

[A. Karl Kornel]

> #qa-bugzilla 2005-09-12 18:30: <LpSolit_QA> it's not critical enough to go into 2.20

How about 2.20.1?

Comment 4

[Frédéric Buclin]

Comment on attachment 195808 [details] [diff] [review]
Patch v1

>+        # Do not forget the saved search if it is being used in a whine
>+        if ($dbh->do('SELECT COUNT(*) FROM whine_queries
>+                      INNER JOIN whine_events
>+                      ON whine_queries.eventid = whine_events.id
>+                      WHERE whine_events.owner_userid = ?
>+                      AND whine_queries.query_name = ?', undef,
>+                     Bugzilla->user->id, $qname)
>+            > 0)

First, you should use $dbh->selectrow_array() in order to return something.

Second, move this SQL query out of the IF ():

my $is_used_by_whines = $dbh->selectrow_array();
if ($is_used_by_whines) {}


>+            $vars->{'message'} = "buglist_query_in_use";
>+            $vars->{'url'} = "editwhines.cgi";
>+            $template->process("global/message.html.tmpl", $vars)
>+              || ThrowTemplateError($template->error());
>+            exit;

Use ThrowUserError() to return errors.

Comment 5

[A. Karl Kornel]

Attachment: Patch v2

(In reply to comment #4)
> <<<snip>>>
> 
> First, you should use $dbh->selectrow_array() in order to return something.
> 
> Second, move this SQL query out of the IF ():

Updated.  In fact, I'll go one better than that.  Instead of just getting a
count, I'm using 
selectcol_arrayref and searching for the subject lines of the whines, as per
comment 1.  The list is being returned as part of the error message.

> <<<snip>>>
> 
> Use ThrowUserError() to return errors.
> 

Updated.

Comment 6

[Frédéric Buclin]

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

>Index: buglist.cgi

>+        # Do not forget the saved search if it is being used in a whine
>+        my $whines_in_use = 
>+            $dbh->selectcol_arrayref('SELECT DISTINCT whine_events.subject
>+                                      FROM whine_events
>+                                      INNER JOIN whine_queries
>+                                      ON whine_queries.eventid = whine_events.id
>+                                      WHERE whine_events.owner_userid = ?
>+                                      AND whine_queries.query_name = ?', undef,
>+                                      Bugzilla->user->id, $qname);

Nit: You could use some indentation here (right aligning reserved words).


>+        ThrowUserError('saved_search_used_by_whines', 
>+                       { subjects    => $whines_in_use,
>+                         search_name => $qname          }
>+        ) unless (scalar @$whines_in_use == 0);

"unless (scalar() == 0)" is better rewritten as "if (scalar())". Moreover,
please enclose @$whines_in_use in parens and rewrite this in a more readable
way:

if (scalar(@$whines_in_use)) {
    ThrowUserError('saved_search_used_by_whines',
		   {subjects	=> join(', ', @$whines_in_use),
		    search_name => $qname});
}



>Index: template/en/default/global/user-error.html.tmpl

>+    by <A HREF="editwhines.cgi">Whining</A> events with the following subjects:

Nit: write HTML tags lowercase. And I would include the word 'events' in the
<a></a> tags.


>+    '[% subjects.shift FILTER html %]'
>+    [% FOREACH whine_subject = subjects %]
>+        , '[% whine_subject FILTER html %]'
>+    [% END %]

Now I understand why your patches are always huge. See how I did it using
join() in buglist.cgi.

Comment 7

[A. Karl Kornel]

Attachment: Patch v2.5

Modification of attachment 195912 [details] [diff] [review] with respect to comment 6.

> <<<snip>>>
> Nit: You could use some indentation here (right aligning reserved words).

Updated and rewrapped.

><<<snip>>>
> "unless (scalar() == 0)" is better rewritten as "if (scalar())". Moreover,
> please enclose @$whines_in_use in parens and rewrite this in a more readable
> way:
> <<<snip>>>

Updated (although this seems less readable to me).

><<<snip>>> 
> Nit: write HTML tags lowercase. And I would include the word 'events' in the
> <a></a> tags.

Updated.

><<<snip>>>
> 
> Now I understand why your patches are always huge. See how I did it using
> join() in buglist.cgi.
> 

That comment does not seem very appropriate for a bug comment.	Updated.

Comment 8

[Frédéric Buclin]

Comment on attachment 197133 [details] [diff] [review]
Patch v2.5

>Index: template/en/default/global/user-error.html.tmpl

>+  [% ELSIF error == "saved_search_used_by_whines" %]
>+    [% title = "Saved Search In Use" %]
>+    The saved search <em>[% search_name FILTER html %]</em> is being used 
>+    by <a href="editwhines.cgi">Whining events</a> with the following subjects:
>+    [% subjects FILTER html %]

Nit: a whitespace should be added before the list of subjects: [%+ subjects
FILTER html %]. I can fix that on checkin.


The patch works fine on both the tip and 2.20. r=LpSolit.

Comment 9

[Frédéric Buclin]

tip:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.311; previous revision: 1.310
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.128; previous revision: 1.127
done


2.20rc2:

Checking in buglist.cgi;
/cvsroot/mozilla/webtools/bugzilla/buglist.cgi,v  <--  buglist.cgi
new revision: 1.299.2.3; previous revision: 1.299.2.2
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.115.2.5; previous revision: 1.115.2.4
done