514970 - Clean up duplicates.cgi and make it use Bug objects

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, enhancement, P1)

Description

[Max Kanat-Alexander]

There are a few places where duplicates.cgi should be using Bug objects instead of using Bugzilla::Search. This would also help with the speed of the page, because right now it calls bug_link and grabs data for every bug displayed, when we could just already grab that data using new_from_list.

Also, things like maxrows should be implemented in the code, not in the templates.

Comment 1

[Max Kanat-Alexander]

Attachment: v1

Okay, here we go. Enormous cleanup. This also fixes a bug that I introduced when moving duplicates into the DB, in Bugzilla::Install::Filesystem.

Comment 2

[Max Kanat-Alexander]

The next step after this is probably just to move the duplicates.cgi functionality into buglist.cgi and have duplicates.cgi just call buglist with the right arguments.

Comment 3

[Frédéric Buclin]

Comment on attachment 398994 [details] [diff] [review]
v1

>Index: duplicates.cgi

>+    exclude_status     => ['CLOSED', 'VERIFIED'],
>+    exclude_resolution => ['INVALID', 'WONTFIX'],

This logic is wrong. You don't want to exclude INVALID nor WONTFIX, and you don't want to exclude VERIFIED if the resolution is one of them.


>+sub formvalue {
>+    my ($name, $default) = (@_);

$default is unused.


>+sub sort_duplicates {

>+    if ($sort_by =~ /id/) {
>+        return $a->{'bug'}->$sort_by <=> $b->{'bug'}->$sort_by;
>+    }

If I pass $sort_by = 'fid', this crashes.


>+    return $a->{'bug'}->$sort_by cmp $b->{'bug'}->$sort_by;

If I pass $sort_by = 'foo', this crashes.


>+    @bugs = @{ Bugzilla::Bug->new_from_list(\@limit_to_ids) };

Are we sure Bug->new_from_list() is working as expected?


>+# Enforce the mostfreqthreshold parameter and the "buglist" cgi param.

What's the 'buglist' parameter??


>+if (!@bugs) {
>+    @bugs = @{ Bugzilla::Bug->new_from_list([keys %total_dups]) };
>+    @bugs = @{ $user->visible_bugs(\@bugs) };
>+}

I suspect this IF condition is wrong. I'm free to pass the buglist I want, unrelated to dupes, and you would ignore %total_dups because @bugs wouldn't be empty. You only want bugs from @bugs which belong to %total_dups, AFAIK.


>+    next if grep($_ eq $bug->bug_status, @$exclude_status);
>+    next if grep($_ eq $bug->resolution, @$exclude_resolution);

Both checks are wrong, see my first comment.


>+    # Note: maximum row count is dealt with in the template.

That's incorrect. You now do it here.


>+my %vars = (
>+    products => Bugzilla->user->get_selectable_products,

You don't need this key. You can already get the list using [% user.get_selectable_products %].



>Index: template/en/default/reports/duplicates-table.html.tmpl

>+[% base_args_string = base_args.join('&') FILTER html %]

Hum, you already escaped all values, so they will be escaped twice. You should do join("&amp;") with no filter, instead (I think).


>-              [% IF changedsince %]&amp;changedsince=[% changedsince FILTER html %][% END %]
>-              [% FOREACH p = query_products %]&amp;product=[% p FILTER html %][% END %]

Why aren't they in the URL anymore?



>Index: template/en/default/reports/duplicates.html.tmpl

>-        <input size="4" name="maxrows" value="[% maxrows %]">
>+        <input size="4" name="maxrows" id="maxrows" 
>+               value="[% maxrows FILTER html %]">

I'm pretty sure that filterexceptions.pl doesn't like this filtering change.


>-        <input size="4" name="changedsince" value="[% changedsince %]"> days
>+        <input size="4" name="changedsince" id="changedsince"
>+               value="[% changedsince FILTER html %]"> days

Same here.


I didn't test your patch yet, as the logic in duplicates.cgi is broken.

Comment 5

[Max Kanat-Alexander]

(In reply to comment #3)
> >+    @bugs = @{ Bugzilla::Bug->new_from_list(\@limit_to_ids) };
> 
> Are we sure Bug->new_from_list() is working as expected?

  Yeah, it's fine. It just doesn't support aliases yet.

> >+if (!@bugs) {
> >+    @bugs = @{ Bugzilla::Bug->new_from_list([keys %total_dups]) };
> >+    @bugs = @{ $user->visible_bugs(\@bugs) };
> >+}
> 
> I suspect this IF condition is wrong. I'm free to pass the buglist I want,
> unrelated to dupes, and you would ignore %total_dups because @bugs wouldn't be
> empty. You only want bugs from @bugs which belong to %total_dups, AFAIK

  Yeah, it does limit to %total_dups. That's what the [keys %total_dups] is there for.


  Everything else is getting fixed and I'll be posting a new patch soon. :-)

Comment 6

[Max Kanat-Alexander]

Attachment: v2

Here's the new patch.

Comment 7

[Frédéric Buclin]

Comment on attachment 414949 [details] [diff] [review]
v2

>Index: duplicates.cgi

>+sub sort_duplicates {
>+    if ($sort_by =~ /id/) {

Nit: to be more robust, this should be: $sort_by =~ /^(bug_)?id$/, to accept both 'id' and 'bug_id'. You cannot pass component_id nor product_id, and they would be the only other valid bug attributes having 'id' in them.


>+if (!grep(lc($_) eq lc($sortby), qw(count delta id))) {
>+    Bugzilla::Field->check($sortby);
>+}

This doesn't work. There are tons of fields which are not valid bug attributes (try passing sortby=commenter for instance), and they make Bugzilla crash. Maybe you should hardcode the list of valid sortkeys.


>+my @limit_to_ids = (split(/[:,]/, formvalue("bug_id") || ''));
>+my @bugs;
>+if ($sortvisible) {
>+    @bugs = @{ Bugzilla::Bug->new_from_list(\@limit_to_ids) };
>+    @bugs = @{ $user->visible_bugs(\@bugs) };
>+}

Move @limit_to_ids into the IF block as it isn't used elsewhere. Also, as it contains unvalidated data, this would make it clearer that this array shouldn't be used outside this IF block (for future reviews).


>+    if ($sortvisible and @bugs and !grep($_->id == $id, @bugs)) {

Nit: no need to write |and @bugs|. if ($sortvisible && !grep(...)) is enough as @bugs is always defined when $sortvisible is true.


>-if (scalar %total_dups) {
>+if (!@bugs) {

I reiterate what I said in my previous comment. If $sortkey = 1 and I pass a bug ID which isn't a duplicate (by hacking the URL, or using an old bookmarked URL), @bugs will contain the bug I passed, and so you won't enter this IF block. This means that you ignore %total_dups entirely here rather than restricting the list to it. More below...


>+        # ...unless it has a resolution in @except_resolution.
>+        next if !grep($bug->resolution, @except_resolution);

Must be !grep($_ eq $bug->resolution, @except_resolution).


>+    if (scalar @query_products) {
>+        next if !grep($_ eq $bug->product, @query_products);
>+    }

This part is very slow and is the main reason for my r-. $bug->product will query the DB for each bug to get the product name. You should rather convert @query_products to a list of product IDs (which is trivial to do as you previously called Bugzilla::Product::check_product() and so you just have to collect ->id for each of the products) and grep for |$_ == $bug->product_id|. This is much faster.


>+    # Note: maximum row count is dealt with later.
>+    push (@result_bugs, { bug => $bug,
>+                          count => $total_dups{$bug->id},
>+                          delta => $since_dups{$bug->id} || 0 });

As said above, as you previously ignored %total_dups when @bugs is not empty, you should exclude $bug if the count is 0 here. Else the list could contain bugs which aren't dupes, which is not desired.


>+my $final_bug = scalar(@bugs) > $maxrows ? ($maxrows-1) : $#bugs;
>+@bugs = @bugs[0..$final_bug];

To make this more readable, simply write:

 @bugs = @bugs[0..$maxrows-1] if (scalar(@bugs) > $maxrows);



>Index: template/en/default/reports/duplicates-table.html.tmpl

>+  [% NEXT IF NOT ${param}.defined %]
>+  [% FOREACH value = ${param} %]

Nit: for both, you can simply write param, which is lighter (no need for ${...}).


>+  [% bug_ids_string = bug_ids.join(',') FILTER url_quote %]

This should be bug_ids.nsort.join(',') to not break the web browser history (bug_id=1,2 is not the same as bug_id=2,1 but from a historical point of view, it's the same list).


>+            >[% column.description %]</a>

Missing FILTER none (else 008filter.t fails).


>+          <td class="bug_id">

Should be class="id" to match the column name (see <th>).


This is a nice cleanup, and still something I would like to take for 3.6.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> >+if (!grep(lc($_) eq lc($sortby), qw(count delta id))) {
> >+    Bugzilla::Field->check($sortby);
> >+}
> 
> This doesn't work. There are tons of fields which are not valid bug attributes
> (try passing sortby=commenter for instance), and they make Bugzilla crash.

  That's OK, because we only care about the actual fields that are displayed in the UI. The fact that a few other fields will also happen to work is now just an added bonus.


  Everything else is getting fixed! Thanks for all the detailed review comments, they were really helpful.

Comment 9

[Max Kanat-Alexander]

Attachment: v3

Okay, here's the new patch! I also realized that we were exposing the non-existence of products, so I fixed that by modifying Bugzilla::Product->check and using it.

Comment 10

[Max Kanat-Alexander]

Attachment: v4

I discovered that the code that was checking for duplicate loops actually didn't always work--sometimes you hit loops that don't involve the original bug id. So this version of this patch fixes it.

Comment 11

[Frédéric Buclin]

Comment on attachment 424599 [details] [diff] [review]
v4

Looks good so far, but when trying to restrict the list to some product, I get:

Can't use string ("TestProduct") as a HASH ref while "strict refs" in use at Bugzilla/Product.pm line 918.
 at Bugzilla/Product.pm line 918
	Bugzilla::Product::check('Bugzilla::Product', 'TestProduct') called at /var/www/html/bugzilla/duplicates.cgi line 153

Comment 12

[Max Kanat-Alexander]

Attachment: v5

Ah, easy enough to fix.

Comment 13

[Frédéric Buclin]

Comment on attachment 424614 [details] [diff] [review]
v5

>Index: template/en/default/reports/duplicates.html.tmpl

> [%# INTERFACE:
>+  # product: array of strings. The set of products we check for dups.

'product' actually contains an array of product objects.


>+    Most Frequently Reported [% terms.Bugs %] for 
>+    [%+ product.join(', ') FILTER html %]

This no longer works, per my previous comment.


>+          [% FOREACH p = user.get_selectable_products %]
>             <option name="[% p.name FILTER html %]"
>+            [% ' selected="selected"' IF product.contains(p.name) %]

Same problem here. This makes the selection in the product list to no longer be persistent. I didn't find any other issue.

Comment 14

[Max Kanat-Alexander]

Attachment: v6

Okay, I fixed it by just passing in the names--it's too difficult to do "map"-like things inside of TT.

Comment 15

[Frédéric Buclin]

Comment on attachment 424620 [details] [diff] [review]
v6

Looks good, and no other problem found. r=LpSolit

Comment 16

[Max Kanat-Alexander]

Committing to: bzr+ssh://[e-mail]/bugzilla/trunk/                                                                                                   
modified duplicates.cgi
modified Bugzilla/Object.pm
modified Bugzilla/Product.pm
modified Bugzilla/User.pm
modified Bugzilla/WebService/Bug.pm
modified skins/standard/duplicates.css
modified template/en/default/filterexceptions.pl
modified template/en/default/global/user-error.html.tmpl                                                                                                                         
modified template/en/default/reports/duplicates-simple.html.tmpl
modified template/en/default/reports/duplicates-table.html.tmpl
modified template/en/default/reports/duplicates.html.tmpl
Committed revision 6952.