When a particular "Saved" search is also used for a whine you get two entries for it in "User Preferences" -> "Saved Searches" the first entry contains a link for "Forget" the second notes: "Remove from whining first" with whinning being a link to "Whinning" (as in the footer). the second note is the only one which should appear.

This is a dupe of bug 311002. Leaving this one open as it may be due to some local customization on b.m.o.

I can't reproduce this on b.m.o either. Got a screenshot?

I wont modify Bug resolution, but I still see this problem, (was not there prior to BMO upgrade)

ok, replicated. It's a Bugzilla bug.

The problem happens when you have the same query used in more than one whine event. The query in Bugzilla::User that grabs the list of the user's queries isn't restrictive enough on the joins, and winds up with a row for each whine event that uses it.

This seems to do the trick

*** Bug 311002 has been marked as a duplicate of this bug. ***

Comment on attachment 201244 [details] [diff] [review] Patch v1 (2.20 and tip) r=joel if you've tested it.

Comment on attachment 201244 [details] [diff] [review] Patch v1 (2.20 and tip) You have to use $dbh->sql_group_by()

This is my frist patch. I hope did everything correct. Thanks for the help thus far.

Comment on attachment 205065 [details] [diff] [review] Database independant fix. >+ $dbh->sql_group_by('uppername', 'name', 'query', 'linkinfooter') . sql_group_by() only takes two arguments. I certainly want something like: $dbh->sql_group_by('uppername', 'name, query, linkinfooter') but I didn't check if the list was correct. ;)

While testing the previous fix, I discoved a test case that did not return the correct results. Therefore I rewrote the entire SQL query to fix the broken test case and the problem mentioned in this bug.

Comment on attachment 205234 [details] [diff] [review] Patch 313571 v2 >+ my $sth = $dbh->prepare(q{SELECT name, query, linkinfooter, query_type, >+ IF (name IN (SELECT wq.query_name >+ FROM whine_events we, whine_queries wq >+ WHERE we.owner_userid = ? >+ AND we.id = wq.eventid),1,0), IF() is MySQL specific, see bug 253360. And subselects require MySQL 4.1, a version higher than our requirements.

Here is another go. This is a bit different than the other attempts. They say the third time is the charm! :D

This patch is in the correct diff -u format. Sorry. Newbie mistake! :)

judevries: Could you explain why justdave's patch is one line, and yours is much larger?

mkanat, The reason for the longer patch is because I found a test case were justdave's patch did not show the correct results. Under certain circumstances the list of saved searches in the User Preferences was not showing the correct list. For example, sometimes the list would show saved searches as not being used in whines, but in reality they were. I duplicated this on a couple difference instances of Bugzilla at Novell. I can not duplicate this on b.m.o, because justdave's patch has not been applied. Does that make sense? judevries = cardinal = Justin De Vries

(In reply to comment #16) > judevries: Could you explain why justdave's patch is one line, and yours is > much larger? Probably because justdave's patch is wrong. Using "CASE WHEN whine_queries.id" together with GROUP BY doesn't look right to me. Probably should we use COUNT() instead of CASE WHEN, but even in this case, getting the correct in a single SQL query doesn't look obvious (at least, all my attempts failed). cardinal, about your patch, you don't need uppername nor namedqueries for $usedinwhineref as you are only interested to know if the query is used or not.

(In reply to comment #18) > (In reply to comment #16) > > judevries: Could you explain why justdave's patch is one line, and yours is > > much larger? > > Probably because justdave's patch is wrong. Using "CASE WHEN whine_queries.id" > together with GROUP BY doesn't look right to me. Probably should we use COUNT() > instead of CASE WHEN, but even in this case, getting the correct in a single > SQL query doesn't look obvious (at least, all my attempts failed). > > > cardinal, about your patch, you don't need uppername nor namedqueries for > $usedinwhineref as you are only interested to know if the query is used or not. > LpSolit, I agree with you about not needing "uppername", but I disagree with not needing the namedqueries. You need to query this table to get the saved searches for the current user. May be I am miss underderstanding what you are asking for. Did you mean to say "name" instead of "namedqueries"?

(In reply to comment #19) > searches for the current user. May be I am miss underderstanding what you are > asking for. Did you mean to say "name" instead of "namedqueries"? No, I mean that query names are in whine_queries and the user ID is in whine_events. So you know all queries used by the user for whining using these two tables only.

Removed some unneeded columns and tables from $usedinwhineref

Comment on attachment 206322 [details] [diff] [review] Patch 313571 v3.2 >+ my $usedinwhineref = $dbh->selectcol_arrayref(q{ >+ SELECT DISTINCT query_name >+ FROM whine_events we, whine_queries wq >+ WHERE we.owner_userid = ? >+ AND we.id = wq.eventid}, undef, $self->{id}); >+ Please use INNER JOIN instead of commas. >+ my $sth = $dbh->prepare(q{SELECT name, query, linkinfooter, query_type, >+ 0, UPPER(name) AS uppername >+ FROM namedqueries n >+ WHERE n.userid = ? >+ ORDER BY uppername}); > $sth->execute($self->{id}); Several comments here: 1) prepare, execute, fetch can be merged together using selectall_arrayref(). Moreover, in order to get an array of hashes, please use {'Slice' => {}}: my @queries = $dbh->selectcol_arrayref(q{foo}, {'Slice' => {}}, $self->{id}). 2) As you use only one table here, there is no need to write n.userid. userid alone is fine, as so you don't need the alias "n" anymore. 3) Remove UPPER(name) AS uppername and write ORDER BY UPPER(name) instead. This will clean up the SQL query a bit a avoid to get uppername in the hash. 4) Remove '0' as well, as we know it's zero. We will add it later when checking whine queries. >+ my @usedinwhine = @{$usedinwhineref}; Useless. You can write @$usedinwhineref where required later (and use var_like_this). > my @queries; > while (my $row = $sth->fetch) { >+ my %qhash = ( >+ name => $row->[0], >+ query => $row->[1], >+ linkinfooter => $row->[2], >+ query_type => $row->[3], >+ usedinwhine => $row->[4], >+ ); All this can go away thanks to 'Slice'. Your patch looks good, but I think these cleanups are welcome.

Here is my fourth revison. I think this takes care of most of the previous questions. Happy Reviewing.

This is a well test version of my previous patch. If there are questions please let me know.

Comment on attachment 206500 [details] [diff] [review] Patch 313571 v4.1 >+ my $used_in_whine_ref = $dbh->selectcol_arrayref(q{ >+ SELECT DISTINCT query_name >+ FROM whine_events we >+ INNER JOIN whine_queries wq >+ ON we.owner_userid = ? >+ AND we.id = wq.eventid}, undef, $self->{id}); Nit: please move "we.owner_userid = ?" in the WHERE statement. Only keep the join condition in the ON part (that's what the docs say too): FROM whine_events we INNER JOIN whine_queries wq ON we.id = wq.eventid WHERE we.owner_userid = ? >+ foreach my $name (@$used_in_whine_ref) { >+ foreach my $queries_hash (@$queries_ref) { >+ if ($queries_hash->{name} eq $name) { >+ $queries_hash->{usedinwhine} = 1; >+ } >+ } > } Nit: as soon as you set 'usedinwhine', you can move out of the innermost loop as you know that query names are unique. So you could add "last;" right after "$queries_hash->{usedinwhine} = 1;" Please either fix these two nits in an updated patch or fix them on checkin. r=LpSolit

The 'query_type' field doesn't exist on 2.20. This patch needs to be backported.

This patch contains reviewers suggestion agains the TIP

Backport of Patch 313571 v4.2 TIP to BUGZILLA-2_20-BRANCH

Comment on attachment 206620 [details] [diff] [review] Patch 313571 v4.2 TIP yes, looks good. r=LpSolit

Comment on attachment 206622 [details] [diff] [review] Patch 313571 v1.0 2.20 r=LpSolit

tip: Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.100; previous revision: 1.99 done 2.20: Checking in Bugzilla/User.pm; /cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v <-- User.pm new revision: 1.61.2.15; previous revision: 1.61.2.14 done