313571 - Duplicate "Saved Search" entries in Preferences when Saved Search is also used for whine

Status: VERIFIED FIXED

(Bugzilla :: User Accounts, defect)

Description

[Justin Wood (:Callek)]

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.

Comment 1

[Frédéric Buclin]

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

Comment 2

[Dave Miller [:justdave]]

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

Comment 3

[Justin Wood (:Callek)]

Attachment: Screenshot of problem

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

Comment 4

[Dave Miller [:justdave]]

ok, replicated.  It's a Bugzilla bug.

Comment 5

[Dave Miller [:justdave]]

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.

Comment 6

[Dave Miller [:justdave]]

Attachment: Patch v1 (2.20 and tip)

This seems to do the trick

Comment 7

[Marc Schumann [:Wurblzap]]

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

Comment 8

[Joel Peshkin]

Comment on attachment 201244 [details] [diff] [review]
Patch v1 (2.20 and tip)

r=joel if you've tested it.

Comment 9

[Frédéric Buclin]

Comment on attachment 201244 [details] [diff] [review]
Patch v1 (2.20 and tip)

You have to use $dbh->sql_group_by()

Comment 10

[Justin C. De Vries]

Attachment: Database independant fix.

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

Comment 11

[Frédéric Buclin]

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. ;)

Comment 12

[Justin C. De Vries]

Attachment: Patch 313571 v2

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 13

[Frédéric Buclin]

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.

Comment 14

[Justin C. De Vries]

Attachment: Patch 313571 v3

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

Comment 15

[Justin C. De Vries]

Attachment: Patch 313571 v3.1

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

Comment 16

[Max Kanat-Alexander]

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

Comment 17

[Justin C. De Vries]

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

Comment 18

[Frédéric Buclin]

(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.

Comment 19

[Justin C. De Vries]

(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"? 

Comment 20

[Frédéric Buclin]

(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.

Comment 21

[Justin C. De Vries]

Attachment: Patch 313571 v3.2

Removed some unneeded columns and tables from $usedinwhineref

Comment 22

[Frédéric Buclin]

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.

Comment 23

[Justin C. De Vries]

Attachment: 313571 v4

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

Comment 24

[Justin C. De Vries]

Attachment: Patch 313571 v4.1

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

Comment 25

[Frédéric Buclin]

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

Comment 26

[Frédéric Buclin]

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

Comment 27

[Justin C. De Vries]

Attachment: Patch 313571 v4.2 TIP

This patch contains reviewers suggestion agains the TIP

Comment 28

[Justin C. De Vries]

Attachment: Patch 313571 v1.0 2.20

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

Comment 29

[Frédéric Buclin]

Comment on attachment 206620 [details] [diff] [review]
Patch 313571 v4.2 TIP

yes, looks good. r=LpSolit

Comment 30

[Frédéric Buclin]

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

r=LpSolit

Comment 31

[Frédéric Buclin]

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