If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

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

VERIFIED FIXED in Bugzilla 2.20

Status

()

Bugzilla
User Accounts
--
major
VERIFIED FIXED
12 years ago
11 years ago

People

(Reporter: Callek, Assigned: Justin C. De Vries)

Tracking

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

Details

Attachments

(4 attachments, 7 obsolete attachments)

(Reporter)

Description

12 years ago
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.
No longer blocks: 313429
(Reporter)

Updated

12 years ago
Blocks: 313429

Comment 1

12 years ago
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?
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → WORKSFORME
(Reporter)

Comment 3

12 years ago
Created attachment 201240 [details]
Screenshot of problem

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.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
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.
Assignee: justdave → user-accounts
Status: REOPENED → NEW
Component: Bugzilla: Other b.m.o Issues → User Accounts
OS: Windows XP → All
Product: mozilla.org → Bugzilla
QA Contact: myk → default-qa
Hardware: PC → All
Target Milestone: --- → Bugzilla 2.20
Version: other → 2.20
Created attachment 201244 [details] [diff] [review]
Patch v1 (2.20 and tip)

This seems to do the trick
Assignee: user-accounts → justdave
Status: NEW → ASSIGNED
Attachment #201244 - Flags: review?(bugreport)
*** Bug 311002 has been marked as a duplicate of this bug. ***

Comment 8

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

r=joel if you've tested it.
Attachment #201244 - Flags: review?(bugreport) → review+

Comment 9

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

You have to use $dbh->sql_group_by()
Attachment #201244 - Flags: review-
Flags: blocking2.22?
Flags: blocking2.20.1?
Flags: blocking2.22?
Flags: blocking2.22+
Flags: blocking2.20.1?
Flags: blocking2.20.1+
(Assignee)

Comment 10

12 years ago
Created attachment 205065 [details] [diff] [review]
Database independant fix.

This is my frist patch. I hope did everything correct. Thanks for the help thus far.
Attachment #205065 - Flags: review?(mkanat)

Comment 11

12 years ago
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. ;)
Attachment #205065 - Flags: review?(mkanat) → review-
(Assignee)

Comment 12

12 years ago
Created attachment 205234 [details] [diff] [review]
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.
Attachment #205065 - Attachment is obsolete: true
Attachment #205234 - Flags: review?(mkanat)

Comment 13

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

Comment 14

12 years ago
Created attachment 205409 [details] [diff] [review]
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
Attachment #205234 - Attachment is obsolete: true
Attachment #205409 - Flags: review?(mkanat)
(Assignee)

Comment 15

12 years ago
Created attachment 205412 [details] [diff] [review]
Patch 313571 v3.1

This patch is in the correct diff -u format. Sorry. Newbie mistake! :)
Attachment #205409 - Attachment is obsolete: true
Attachment #205412 - Flags: review?(mkanat)
Attachment #205409 - Flags: review?(mkanat)

Comment 16

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

Updated

12 years ago
Assignee: justdave → judevries
Status: ASSIGNED → NEW
(Assignee)

Comment 17

12 years ago
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
Status: NEW → ASSIGNED

Comment 18

12 years ago
(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.
(Assignee)

Comment 19

12 years ago
(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

12 years ago
(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.
(Assignee)

Comment 21

12 years ago
Created attachment 206322 [details] [diff] [review]
Patch 313571 v3.2

Removed some unneeded columns and tables from $usedinwhineref
Attachment #205412 - Attachment is obsolete: true
Attachment #206322 - Flags: review?(mkanat)
Attachment #205412 - Flags: review?(mkanat)
(Assignee)

Updated

12 years ago
Attachment #206322 - Flags: review?(LpSolit)

Comment 22

12 years ago
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.
Attachment #206322 - Flags: review?(mkanat)
Attachment #206322 - Flags: review?(LpSolit)
Attachment #206322 - Flags: review-
(Assignee)

Comment 23

12 years ago
Created attachment 206442 [details] [diff] [review]
313571 v4

Here is my fourth revison. I think this takes care of most of the previous questions. Happy Reviewing.
Attachment #206322 - Attachment is obsolete: true
Attachment #206442 - Flags: review?(LpSolit)
(Assignee)

Updated

12 years ago
Attachment #206442 - Flags: review?(LpSolit)
(Assignee)

Comment 24

12 years ago
Created attachment 206500 [details] [diff] [review]
Patch 313571 v4.1

This is a well test version of my previous patch. If there are questions please let me know.
Attachment #206442 - Attachment is obsolete: true
Attachment #206500 - Flags: review?(LpSolit)

Comment 25

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

Comment 26

12 years ago
The 'query_type' field doesn't exist on 2.20. This patch needs to be backported.
(Assignee)

Comment 27

12 years ago
Created attachment 206620 [details] [diff] [review]
Patch 313571 v4.2 TIP

This patch contains reviewers suggestion agains the TIP
Attachment #206500 - Attachment is obsolete: true
Attachment #206620 - Flags: review?(LpSolit)
(Assignee)

Comment 28

12 years ago
Created attachment 206622 [details] [diff] [review]
Patch 313571 v1.0 2.20

Backport of Patch 313571 v4.2 TIP to BUGZILLA-2_20-BRANCH
Attachment #206622 - Flags: review?(LpSolit)

Comment 29

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

yes, looks good. r=LpSolit
Attachment #206620 - Flags: review?(LpSolit) → review+

Comment 30

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

r=LpSolit
Attachment #206622 - Flags: review?(LpSolit) → review+

Updated

12 years ago
Flags: approval?
Flags: approval2.20?
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval+

Comment 31

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED

Updated

11 years ago
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.