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

RESOLVED FIXED in Bugzilla 2.20

Status

()

RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: karl, Assigned: karl)

Tracking

2.20
Bugzilla 2.20
Bug Flags:
approval +
approval2.20 +
blocking2.20.1 +

Details

(URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

13 years ago
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.
We should just throw an error stating that the search is used in a whine, and
ideally list which whines.
Version: unspecified → 2.20

Updated

13 years ago
Assignee: erik → karl
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 2

13 years ago
Created attachment 195808 [details] [diff] [review]
Patch v1

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

I don't know who to ask to review this.
Attachment #195808 - Flags: review?
(Assignee)

Comment 3

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

How about 2.20.1?
Flags: blocking2.20.1?

Comment 4

13 years ago
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.
Attachment #195808 - Flags: review? → review-

Updated

13 years ago
Target Milestone: --- → Bugzilla 2.20
(Assignee)

Comment 5

13 years ago
Created attachment 195912 [details] [diff] [review]
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.
Attachment #195808 - Attachment is obsolete: true
Attachment #195912 - Flags: review?

Updated

13 years ago
Flags: blocking2.20.1? → blocking2.20.1+

Comment 6

13 years ago
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.
Attachment #195912 - Flags: review? → review-
(Assignee)

Comment 7

13 years ago
Created attachment 197133 [details] [diff] [review]
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.
Attachment #195912 - Attachment is obsolete: true
Attachment #197133 - Flags: review?(LpSolit)

Comment 8

13 years ago
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.
Attachment #197133 - Flags: review?(LpSolit) → review+

Updated

13 years ago
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval2.20?
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+

Comment 9

13 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.