119635 - Templatise duplicates.cgi

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect, P1)

Description

[Gervase Markham [:gerv]]

I should have this ready in the next few days. I hope.

Gerv

Comment 1

[Gervase Markham [:gerv]]

Attachment: Patch v.1

Here it is. I've given it a fair spanking - it now has multiple sortable
columns (Myk; you may want to check out how I did this for buglist.cgi - there
are some nifty tricks I discovered) and a bevy of changeable options. This also
resolves all known bugs in duplicates.cgi.

Gerv

Comment 2

[Andreas Franke (gone)]

Comment on attachment 65326 [details] [diff] [review]
Patch v.1

When I apply the patch to 1.14 of duplicates.cgi and then upgrade to the tip, I
get the following software error:

Insecure dependency in glob while running with -T switch at
/home/bugzilla/apache/htdocs/duplicates.cgi line 88. 

Line 88 reads:
if (<data/duplicates/dupes$today*>) {

Maybe this hasn't been tested with tainting yet?

Marking needs-work on that assumption.

Comment 3

[Andreas Franke (gone)]

Comment on attachment 65326 [details] [diff] [review]
Patch v.1

Oops. The current duplicates.cgi already has this problem. Seems to be a
different bug.

Comment 4

[Bradley Baetz (:bbaetz)]

Comment on attachment 65326 [details] [diff] [review]
Patch v.1

Theres a trivial conflict although its easy to work arround.

You've removed the original copyright date. Don't do that.

You should use the global template stuff via |use vars|.

Why not allow the user to limit between a group of products (ie make the select
a MULTIPLE)?

The conditions you add wrt closed/verified/invalid should probably be part of
the query.

Actually, why are we reading these dupe stats at all, instead of querying the
db directly? Then we could use LIMIT eventually (that requires 3.23). We could
also let the db do the sorting. Is this just done for caching? I'm dubious that
asking the db is quicker than iterating through an on-disk hash or every single
entry each time. We could do the changedbefore stuff via the db, but it would
just be a single hash lookup on the items we want to show anyway.

You can't mention the bugzilla helper in the template - thats not a generic
option.

The maxrows <input> size should be at least 4, or lengthof(maxrows)+1, or
something - with my font in mozilla the 100 is partly obscured.

Comment 5

[Bradley Baetz (:bbaetz)]

> You can't mention the bugzilla helper in the template - thats not a generic
> option.

IE you sould be using the param instead.

Comment 6

[Gervase Markham [:gerv]]

Removing keywords per reviewer comments,marking assigned.

Comment 7

[Gervase Markham [:gerv]]

kiko, if you are going to post to Bugzilla from my machine, please log in as
yourself :-)

Gerv

Comment 8

[Gervase Markham [:gerv]]

Attachment: Patch v.2

All problems fixed. But:

> You can't mention the bugzilla helper in the template - thats not a generic
> option. [ Use a param. ]

Given that the default Bugzilla banner says "mozilla.org" in big letters, this
really isn't an issue. The default templates are b.m.o's ones - that's just the
way it is. The param is going away (as it has in the latest patch - thanks for
reminding me.

Gerv

Comment 9

[Bradley Baetz (:bbaetz)]

Yes, but theres a bug to change that, IIRC.

If bmo wants to mention the bugzilla helper, then they need their own customised
template.

I'll look at the patch this evening, after the mozilla tree closure.

Comment 10

[Gervase Markham [:gerv]]

The Bugzilla Helper will eventually be a template for enter_bug.cgi, and so will
be mentionable everywhere. In the mean time, we should use the "don't break
b.m.o" rule of Bugzilla development. :-)

Gerv

Comment 11

[Bradley Baetz (:bbaetz)]

Comment on attachment 70412 [details] [diff] [review]
Patch v.2

>Index: duplicates.cgi
>===================================================================
>RCS file: /cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v
>retrieving revision 1.15
>diff -u -r1.15 duplicates.cgi
>--- duplicates.cgi	20 Jan 2002 01:44:38 -0000	1.15
>+++ duplicates.cgi	20 Feb 2002 00:15:05 -0000
>+
>+# Limit to a single product if requested             
>+$generic_query .= ("product = " . SqlQuote($product) . "AND ") if $product;

You're missing spaces before product and AND, so the generated sql is then
invalid.

>+ 
>+my @bugs;
>+my @bug_ids; 
>+my $loop = 0;
>+
>+foreach my $id (keys(%count)) {
>+    # Maximum row count is dealt with in the template.

This doesn't stop this, since its how the current code is, but you didn't
comment on my suggestion I made in my other comment.

>+ 
>+[% DEFAULT title = "Most Frequently Reported Bugs" %]
>+[% IF product %]
>+  [% title = "Most Frequently Reported Bugs for $product" %]
>+[% END %]   

Why not do this as if/else? Its cleaner that way

>+        <select name="product" size="5" multiple>
>+          <option value="">All Products</option>
>+          [% FOREACH p = products %]
>+            <option name="[% p %]"
>+            [% " selected" IF product == p %]>[% p %]</option>
>+          [% END %]
>+        </select>

If you search on all products, the AllProducts link isn't selected. the if
needs to be:

(NOT PRODUCT AND p = products.first) OR product == p

I think. Its a select mutiple, so the first option isn't selected by default.

>+      <td>Open bugs only:</td>
>+      <td>
>+        <input type="checkbox" name="openonly" value="1" 
>+          [% "checked" IF openonly %] />      
>+      </td>

<label> ? ;)

As an aside, the stuff at the end here is something I would support putting
into a strings file.

Comment 12

[Gervase Markham [:gerv]]

Attachment: Patch v.3

> This doesn't stop this, since its how the current code is, but you didn't
> comment on my suggestion I made in my other comment.

You mean about using the DB? The data in the hashes is historical; it's not in
the DB (at least, not in a usable form.)

> [AllProducts]

Given that nothing selected is the same as AllProducts, I just removed
AllProducts.

Ready for re-review.

Gerv

Comment 13

[Bradley Baetz (:bbaetz)]

Comment on attachment 71200 [details] [diff] [review]
Patch v.3

You can have a table keyed by date.

Anyway, you didn't test the product stuff, since you only added one space, not
two, so it still gives an sql error

Comment 14

[Gervase Markham [:gerv]]

Attachment: Patch v.4

> You can have a table keyed by date.

I'm not rewriting the entire duplicates system in this bug :-)

SQL finally fixed.

Gerv

Comment 15

[Bradley Baetz (:bbaetz)]

Comment on attachment 71246 [details] [diff] [review]
Patch v.4

But you rerewritten most of it... ;) What I actually meant was: "Is there a bug
on doing that?"

r=bbaetz

Comment 16

[Gervase Markham [:gerv]]

Not that I know of. The current system works fine for the moment :-)

Gerv

Comment 17

[Ville Skyttä]

Attachment: Patch v.5

(X)HTML fixes, obsoletes Patch v.4 (I don't have rights to obsolete..)

Comment 18

[Bradley Baetz (:bbaetz)]

A read-through of the interdiff between patch 4 and patch 5 seems ok. Is the
& stuff accepted on all browsers? I know its techinically correct, but will
some old browser die on it, or something?

Comment 19

[Gervase Markham [:gerv]]

Comment on attachment 74600 [details] [diff] [review]
Patch v.5

Having chatted to Ville, he's going to do the XHTML stuff as a later patch if
it's still thought necessary.

Gerv

Comment 20

[Gervase Markham [:gerv]]

Patch v.4 has r=bbaetz here; looking for second review.

Gerv

Comment 21

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 71246 [details] [diff] [review]
Patch v.4

Index: duplicates.cgi

>+my $sortby = formvalue("sortby", "dup_count");

"dup_count" is the default sort field, but duplicates.tmpl only recognizes
"count".  If this field is being renamed "count", "dup_count" should be
translated to accommodate old bookmarks.

In testing, the new version generates different results than the old version.
Some bugs on the old list are missing from the new list, and vice-versa.
This needs to be investigated, but I'm not sure how without landfill.

Comment 22

[Gervase Markham [:gerv]]

Myk - can you be more specific? Where did you test the patch - on b.m.o? Can you
give examples of bugs not appearing or appearing when they didn't?

Remember that the current CGI does a "sort entire list" whenever you sort; if
you have "sort visible list" selected, you'll get different results.

Gerv

Comment 23

[Myk Melez [:myk] [@mykmelez]]

I tested the patch against a several-weeks-old copy of the Bugzilla database
running on a local workstation.  I fixed the dup_count problem in the new
version of duplicates.cgi by changing the default for that field to "count",
then I ran both the old and the new version.  I then clicked the "Dupe Count"
header in the new version to reverse the sort order (which is least to most
dupes by default, another bug), and compared the results.  "entire list" was
selected in the parameters.

Both lists started with bug 55583 (89 duplicates).  The second bug in the old
version was 22274 with 80 duplicates, which doesn't appear in the new list at
all.  After that are four bugs that appear in both lists (two of which, with the
same number of duplicates, appear in reverse order, which may or may not be a
bug).  Then there are bugs 991, 77675, 114377, 74313, 5821, 47617, and 20185 in
the new list, none of which appear in the old list.

Comment 24

[Bradley Baetz (:bbaetz)]

Comment on attachment 74600 [details] [diff] [review]
Patch v.5

It looks like your'e shoing VERIFIED bugs only if they were FIXED, rather than
the other way arround:

>+# Don't add CLOSED, and don't add VERIFIED unless they are INVALID or 
>+# WONTFIX. We want to see VERIFIED INVALID and WONTFIX because common 
>+# "bugs" which aren't bugs end up in this state.
>+my $generic_query = "
>+  SELECT component, bug_severity, op_sys, target_milestone,
>+         short_desc, bug_status, resolution
>+  FROM bugs 
>+  WHERE (bug_status != 'CLOSED') 
>+  AND   ((bug_status = 'VERIFIED' AND resolution NOT IN ('INVALID', 'WONTFIX')) 
>+         OR (bug_status != 'VERIFIED'))
>+  AND ";
>+

shouldn't that be resolution IN ('INVALID', 'WONTFIX') - ie lose the NOT ?

I havne't tested this, though

Comment 25

[Gervase Markham [:gerv]]

Attachment: Patch v.5

Excellent catch :-) You are correct; I suspect that this is indeed the problem.


Myk - could you possibly retest?

Gerv

Comment 26

[Myk Melez [:myk] [@mykmelez]]

>shouldn't that be resolution IN ('INVALID', 'WONTFIX') - ie lose the NOT ?

With this fix the duplicate tables seem to match, although it's hard to verify
this since bugs with the same number of duplicates are displayed in a different
order in the new version of duplicates.cgi than they are displayed in the old
version.

Even with the fix, however, clicking the "Bug List" button gives a different set
of bugs for the new vs. the old version.

Comment 27

[Gervase Markham [:gerv]]

Again, more details would be really helpful. I can't see immediately what the
problem might be - unless it's a permissions thing (the old list was everything,
and relied on buglist.cgi to do the permissions checking.) But that doesn't make
much sense either.

Gerv

Comment 28

[Bradley Baetz (:bbaetz)]

Ordering will be variable, I suspect. Unless the old order sorted by a secondary
key, or something, don't worry about it.

When this stuff is brought into the db, then we can add ORDER by clauses to work
correctly.

Comment 29

[Bradley Baetz (:bbaetz)]

myk: which bugs are present on the bug list in the orignal but not the new (and
the other way arround)?

Comment 30

[Myk Melez [:myk] [@mykmelez]]

With the bug in comment 24 fixed and debug=1 applied to the bug list query, the
following pages are generated:

http://melez.com/bugzilla/duplicates-old.html
http://melez.com/bugzilla/duplicates.html

http://melez.com/bugzilla/buglist-old.html
http://melez.com/bugzilla/buglist.html

Comment 31

[Gervase Markham [:gerv]]

Well, those are obviously two different sets of bugs - the old version is sorted
by dup_count, and the new one isn't (I'm not sure what it is sorted by.)

Myk said:
"Even with the fix, however, clicking the "Bug List" button gives a different
set of bugs for the new vs. the old version."

This would only be a bug if the two lists on duplicates.cgi were initially the
same. As they aren't in your test data, it's not surprising that you get
different buglists.

I think it's far more sensible to check this in and diagnose the problem (if,
indeed, there is one) that way.

Gerv

Comment 32

[Myk Melez [:myk] [@mykmelez]]

>Well, those are obviously two different sets of bugs - the old version is sorted
>by dup_count, and the new one isn't (I'm not sure what it is sorted by.)

Oops, I forgot to sort by dup_count in the new list.  Fixed.  Go back and take
another look.

Comment 33

[Bradley Baetz (:bbaetz)]

myk: The original code is long - bug 1582, for example, isn't in the table, but
is in the list.

This is because current duplicates.cgi sorts the list, then does the table by
picking the first n suitable entries (ie non closed/verified fixed/etc) from the
sorted list. It then generates the bug list by considering the first n entries
in @sortedcount, but there is no check that such an entry was suitable. The fix
would be to just remove such entries from sorted count, and this patch does that
as a side effect. (since only valid entries are pushed onto the list anyway)

I guess that goes to show how often pepole get buglists from the duplicates count.

Comment 34

[Bradley Baetz (:bbaetz)]

s/long/wrong/, rather - ie the current code is correct.

Comment 35

[Gervase Markham [:gerv]]

bbaetz: good catch :-) So can I have review for this patch, now? :-)

Gerv

Comment 36

[Bradley Baetz (:bbaetz)]

Comment on attachment 77496 [details] [diff] [review]
Patch v.5

r=bbaetz

Comment 37

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 77496 [details] [diff] [review]
Patch v.5

This patch still has the default sort problem.	The "sortby" parameter is now
correctly set to "dup_count" in the script if not otherwise specified by the
user, but the bugs recordset has a "count" column name and the template expects
the value of the sortby parameter to be "count".  These should all be either
"count" or "dup_count", and if the former (which does sound better) then
"dup_count" should be translated to "count" for compatibility with old
bookmarks.

Comment 38

[Gervase Markham [:gerv]]

Attachment: Patch v.6

Fixed the sort order/field name issues Myk raised; needs rubber-stamp from
bbaetz, and another look from Myk :-)

Gerv

Comment 39

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 78606 [details] [diff] [review]
Patch v.6

ok, works. r=myk

Comment 40

[Bradley Baetz (:bbaetz)]

Comment on attachment 78606 [details] [diff] [review]
Patch v.6

r=bbaetz. My test installation isn't big enough to pick up these differences.

Comment 41

[Gervase Markham [:gerv]]

Fixed.

Checking in duplicates.cgi;
/cvsroot/mozilla/webtools/bugzilla/duplicates.cgi,v  <--  duplicates.cgi
new revision: 1.16; previous revision: 1.15
done
RCS file:
/cvsroot/mozilla/webtools/bugzilla/template/default/report/duplicates.html.tmpl,v
done
Checking in template/default/report/duplicates.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/default/report/duplicates.html.tmpl,v
 <--  duplicates.html.tmpl
initial revision: 1.1
done

Gerv