Search Criteria Headers on Buglist Duplicate Unnecessarily

RESOLVED FIXED in Bugzilla 3.6

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mockodin, Assigned: LpSolit)

Tracking

({regression})

3.5.2
Bugzilla 3.6
regression
Dependency tree / graph
Bug Flags:
approval +
blocking3.6 +

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

9 years ago
To Reproduce:
1. View a bug with multiple dependencies
2. Click Tree link: (Show dependency tree  / graph)
3. Click 'view as bug list'

Example:
https://landfill.bugzilla.org/bugzilla-tip/buglist.cgi?bug_id=6753,5550

This occurs with other search fields as well but this is the simplest way to replicate.
(Assignee)

Comment 1

9 years ago
debug=1 shows that the criteria appears twice in the SQL query. No reason to.
(Reporter)

Comment 2

9 years ago
Note that the URL denotes the field only once and that it lists twice in the header with the same bug_ids. Regardless of the sql, how does it makes sense to display the same data it twice?

Happens even with only one bug_id is included:
https://landfill.bugzilla.org/bugzilla-tip/buglist.cgi?bug_id=2667

A single bug_id still displaying the header twice.
(Reporter)

Comment 3

9 years ago
Doesn't occur on BMO (version 3.4.4+)
(Assignee)

Comment 4

9 years ago
(In reply to comment #2)
> Regardless of the sql, how does it makes sense to
> display the same data it twice?

It doesn't, I agree. I'm only saying the SQL query contains the bug ID twice. But only with HEAD code. 3.4.4+ is indeed fine.
Keywords: regression
Target Milestone: --- → Bugzilla 3.6
(Reporter)

Comment 5

9 years ago
MKanat/LpSolit

Would it makes sense to cycle through the array and combine keys and their values to correct this? I don't think we're really displaying the search criteria in groups right? By that I mean

WHERE (group 1 blah blah blah)
      OR (group 2 blah blah something else)

So long as we are not do this we could consolidate headers as well:
/buglist.cgi?bug_id=666,100000
    * Issue ID: 666, 100000
    * Issue ID: 666, 100000
becomes
    * Issue ID: 666, 100000

Cases where a header may repeat with separate values, such as boolean searches
the values could be combined so instead of:
 Status: CLOSED
 Status: RESOLVED
 Severity: blocker

you would get:
 Status: CLOSED, RESOLVED
 Severity: blocker

Comment 6

9 years ago
(In reply to comment #5)
> Would it makes sense to cycle through the array and combine keys and their
> values to correct this?

  No. As LpSolit pointed out, the criteria actually appear twice in the SQL, so there's something else in Search.pm that needs to be addressed.
(Assignee)

Comment 7

9 years ago
Regression due to bug 524669, and actually the code in Search.pm code is now broken when using the 'exclude' bit for bug IDs in query.cgi.

Search.pm already has special code for bugs.bug_id at line 293, which also correctly handles the 'exclude' bit:

    if ($params->param('bug_id')) {
        my $type = "anyexact";
        if ($params->param('bugidtype') && $params->param('bugidtype') eq 'exclude') {
            $type = "nowords";
        }
        push(@specialchart, ["bug_id", $type, join(',', $params->param('bug_id'))]);
    }

but bug 524669 re-adds bugs.bug_id to the list due to its

    foreach my $field ($params->param()) { ... }

loop, and which doesn't correctly take the 'exclude' bit into account. As a consequence, buglist.cgi always returns zero bugs in this case.
Depends on: 524669
Flags: blocking3.6+

Comment 8

9 years ago
Oh, okay. Should be easy enough to fix by excluding bug_id from the first block.
(Assignee)

Comment 9

9 years ago
Created attachment 422406 [details] [diff] [review]
patch, v1

Added backward-compatibility code in buglist.cgi so that existing saved searches still work. Thanks to the renaming, the special code in Search.pm is no longer needed (one less hack).
Assignee: query-and-buglist → LpSolit
Status: NEW → ASSIGNED
Attachment #422406 - Flags: review?(mkanat)
(Assignee)

Comment 10

9 years ago
Created attachment 422408 [details] [diff] [review]
patch, v1.1

Oh, the special case in CGI.pm can actually go away with my renaming. Another hack removed! :)
Attachment #422406 - Attachment is obsolete: true
Attachment #422408 - Flags: review?(mkanat)
Attachment #422406 - Flags: review?(mkanat)

Comment 11

9 years ago
Comment on attachment 422408 [details] [diff] [review]
patch, v1.1

>Index: buglist.cgi
> if (!$params->param('query_format')) {
>     $params->param('query_format', 'advanced');
>-    $buffer = $params->query_string;
>+    $rebuild_buffer = 1;

  Instead of doing rebuild_buffer, I think it would be better to just put the backwards-compat code above this point, no?

>+# Another backward compatibility hack: bugidtype is now bug_id_type.
>+if ($params->param('bugidtype')) {
>+    my $value = $params->param('bugidtype') eq 'exclude' ? 'nowords' : 'anyexact';
>+    $params->param('bug_id_type', $value);
>+    $params->delete('bugidtype');
>+    $rebuild_buffer = 1;
>+}

  Oh, but this probably needs to happen inside Search.pm, because of other places that use it (like graphical and tabular reports). We can still delete the CGI.pm fix, though, because we don't have to worry about canonicalizing old URLs, I think, only new ones.



  All in all, I have to say that I love this patch--it's always nice when you can fix a bug mostly by *removing* code! :-D
Attachment #422408 - Flags: review?(mkanat) → review-
(Assignee)

Comment 12

9 years ago
Created attachment 424494 [details] [diff] [review]
patch, v2

Now this works with reports too.
Attachment #422408 - Attachment is obsolete: true
Attachment #424494 - Flags: review?(mkanat)

Comment 13

9 years ago
Comment on attachment 424494 [details] [diff] [review]
patch, v2

Okay, sure, this looks good. :-)
Attachment #424494 - Flags: review?(mkanat) → review+

Updated

9 years ago
Flags: approval+
(Assignee)

Comment 14

9 years ago
Checking in query.cgi;
/cvsroot/mozilla/webtools/bugzilla/query.cgi,v  <--  query.cgi
new revision: 1.190; previous revision: 1.189
done
Checking in Bugzilla/CGI.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/CGI.pm,v  <--  CGI.pm
new revision: 1.53; previous revision: 1.52
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.181; previous revision: 1.180
done
Checking in Bugzilla/Search/Quicksearch.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search/Quicksearch.pm,v  <--  Quicksearch.pm
new revision: 1.29; previous revision: 1.28
done
Checking in template/en/default/search/form.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/search/form.html.tmpl,v  <--  form.html.tmpl
new revision: 1.67; previous revision: 1.66
done
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 550148
You need to log in before you can comment on or make changes to this bug.