Closed
Bug 308228
Opened 20 years ago
Closed 20 years ago
Whine-related saved search can be forgotten through buglist.cgi
Categories
(Bugzilla :: Whining, defect)
Tracking
()
RESOLVED
FIXED
Bugzilla 2.20
People
(Reporter: karl, Assigned: karl)
References
()
Details
Attachments
(1 file, 2 obsolete files)
3.08 KB,
patch
|
LpSolit
:
review+
|
Details | Diff | Splinter Review |
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•20 years ago
|
||
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•20 years ago
|
Assignee: erik → karl
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee | ||
Comment 2•20 years ago
|
||
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•20 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•20 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•20 years ago
|
Target Milestone: --- → Bugzilla 2.20
Assignee | ||
Comment 5•20 years ago
|
||
(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•20 years ago
|
Flags: blocking2.20.1? → blocking2.20.1+
![]() |
||
Comment 6•20 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•20 years ago
|
||
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•20 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•20 years ago
|
Status: NEW → ASSIGNED
Flags: approval?
Flags: approval2.20?
Updated•20 years ago
|
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+
![]() |
||
Comment 9•20 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
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•