300473 - "All Closed" for components missing bug_status= in series.query

Status: RESOLVED FIXED

(Bugzilla :: Reporting/Charting, defect)

Description

[Jeff Jensen]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.8) Gecko/20050511 Firefox/1.0.4

For the automatically added "All Closed" series on components in the series
table, the query field does not get a
"bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED".

So "All Closed" is always the total bug count for that component.

Note that the "All Open" ones do get the "bug_status=" entries for them.
And there is no "All Closed" created for the "-All-" grouping component, so
nothing to check there (but would like an enchancement to add that one
automatically too!).


Reproducible: Always

Steps to Reproduce:
1.  Add a component.
2.  Inspect the series.query field for that component and "All Closed".
3.  It does not have "bug_status=" query parameters.

Actual Results:  
It does not have "bug_status=" query parameters.


Expected Results:  
It should have have "bug_status=" query parameters.

Comment 1

[Max Kanat-Alexander]

I think we could possibly at least fix this for new installations, and it would
be a simple fix.

If it's not a simple fix, or if there's nobody who can get to this before we
release 2.20, ping me on it and I may reconsider its blocking status.

Comment 2

[Marc Schumann [:Wurblzap]]

While it doesn't get bug_status=RESOLVED&bug_status=VERIFIED&bug_status=CLOSED,
it does get field0-0-0=resolution&type0-0-0=notequals&value0-0-0=---, which has
essentially the same effect.

Comment 3

[Jeff Jensen]

What I found was whatever were the original query values in that field caused
the  wrong "All Closed" counts in the series_data table.  When I changed it to
the string in my original description, the numbers were correct in the
series_data table.

"essentially the same effect" - So my question is does that query string really
work to generate correct numbers?  It did not for me.

Comment 4

[Marc Schumann [:Wurblzap]]

You're right -- this query should be
field0-0-0=resolution&type0-0-0=regexp&value0-0-0=. (including the trailing dot).

Comment 5

[Marc Schumann [:Wurblzap]]

Attachment: Patch

Comment 6

[Marc Schumann [:Wurblzap]]

Tested on HEAD and 2.20rc1. Should work on the 2.18 branch as well, but it'll
need a backport.

Comment 7

[Gervase Markham [:gerv]]

Rats. Of course, the database doesn't store "---" - that's just the UI
representation. We _keep_ having problems with that.

I'm sure this patch is good, but I may not have a chance to test it before going
on holiday tomorrow.

Gerv

Comment 8

[Marc Schumann [:Wurblzap]]

Comment on attachment 191780 [details] [diff] [review]
Patch

Perhaps somebody else can review this one instead of Gerv then, and decide
afterwards whether to ask him for a second-review?

Comment 9

[Vlad Dascalu]

Comment on attachment 191780 [details] [diff] [review]
Patch

This is non commitable due to:

+    print "Repairing broken series...\n" unless $silent;

We want the cronjob script of running checksetup.pl every x minutes/hours or so
in silent mode to email the admin about this.

DB upgrades, one-time changes or other modifications should be emailed to the
admin, even when running in silent mode. As far as I understand it (and I think
justdave confirmed it once), $silent is used to suppress repeate-able
checksetup.pl information from being emailed by the cronjobs that run
checksetup.pl regularly.

Brainstorming optional (and probably unuseful) nits:

Nit: there is no "done" message, although that's probably overkill.

Nit: 

 # 2004-04-12 - Keep regexp-based group permissions up-to-date - Bug 240325

(the following comment has the timestamp added as well - I haven't checked if
this is standard practice in checksetup.pl; adding the date for 300473 would be
again probably overkill, and a little bit difficult since it implies guessing
the commit date in advance :) )

Comment 10

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.2

Not so silent any more.
Added "done" message.

New: nukes corrupt series data.

I think adding the timestamp to a code comment is redundant to data in bonsai
and the bug.

Comment 11

[Gervase Markham [:gerv]]

Comment on attachment 194423 [details] [diff] [review]
Patch 1.2

r=gerv, with one correction: although adding dates is redundant with CVS data,
it's both traditional and useful in checksetup.pl, because of the nature of the
file. So please add a "change header" like the other ones.

Gerv

Comment 12

[Gervase Markham [:gerv]]

Oh, and unless there's a good reason not to, please place the block at the
bottom of the dated blocks (i.e. so they are in chronological order). Currently,
that would be at around line 4060.

Gerv

Comment 13

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3

Addressed comment 11 and comment 12.

Comment 14

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.3 (2.18 branch)

Backport to 2.18.
The previous patch applies to 2.20 as well as HEAD.

Comment 15

[Marc Schumann [:Wurblzap]]

Please apply the HEAD patch with fuzz 1 (needed since bug 301743).

Comment 16

[Frédéric Buclin]

gerv, have you time to review these patches and backports today? You already
reviewed the original one and we would like to start QA stuff as soon as this
one lands.

Comment 17

[Frédéric Buclin]

justdave, could you have a look at this patch too?

Comment 18

[Dave Miller [:justdave]]

(In reply to comment #7)
> Rats. Of course, the database doesn't store "---" - that's just the UI
> representation. We _keep_ having problems with that.

Only in boolean charts apparently...  the actual resolution field on the form
deals with --- just fine.  I suspect we're not consistent with decoding it on
the back-end.

Comment 19

[Dave Miller [:justdave]]

I'm not sure I like the idea of blindly nuking the old series data without
warning the admin and giving them a chance to opt out.  Although the data is
corrupted, it might actually be useful for something, and the admin should make
that decision.  We should of course, nag the admin until it's corrected.

Another thought...  the particular corruption is that it's recording all bugs
instead of just closed bugs, right?  Isn't closed bugs = all bugs - open bugs? 
Could we not correct the data automatically by grabbing the series data from the
open bugs query and subtracting it off?

Comment 20

[Dave Miller [:justdave]]

As much as I hate to push this off, it *is* part of charting, and charting is
well-known to be a bit on the buggy side still, so this is no big regression or
anything, and we did come up with a way to fix it retroactively, unfortunately
it's not easy to do.  But, we need to release.

Comment 21

[Marc Schumann [:Wurblzap]]

(In reply to comment #19)
> I'm not sure I like the idea of blindly nuking the old series data without
> warning the admin and giving them a chance to opt out.  Although the data is
> corrupted, it might actually be useful for something, and the admin should make
> that decision.  We should of course, nag the admin until it's corrected.

If we choose to go this way, we must keep in mind that we need to keep the query
corrupt. I don't think we want to end up with a series having partially broken
and partially correct data.
In other words, we must keep series query and series data in sync (repair them
at the same time, not one after the other).

> Another thought...  the particular corruption is that it's recording all bugs
> instead of just closed bugs, right?  Isn't closed bugs = all bugs - open bugs? 
> Could we not correct the data automatically by grabbing the series data from the
> open bugs query and subtracting it off?

This should be possible. It's not trivial, though, because the name of the
open-bugs-series depends on the localized template it's been created with. (You
may say "argh" now.)
So, to identify the open-bugs-series, we must parse the series' queries 8)

Comment 22

[Frédéric Buclin]

gerv, could you have a look at these patches?

Comment 23

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.4

Addressing comment 19. Nuking single data entries only, if at all, and only if the single data entry cannot be corrected. This should never happen unless there is a way data may be collected for one series and not for another.

Comment 24

[Teemu Mannermaa (:wicked)]

Comment on attachment 200859 [details] [diff] [review]
Patch 1.4

>Index: checksetup.pl
>===================================================================
>+# Repair broken automatically generated series queries for non-open bugs.

Nit: Please use term closed instead of non-open for clarity. Same comment applies to other places where this term is used.

>+my $broken_nonopen_series =
>+    $dbh->selectall_arrayref("SELECT series_id, query FROM series
>+                               WHERE query LIKE \"$broken_series_indicator%\"");

Developer's Guide seems to say that double quotes shouldn't be used for string literals. Probably because they are not cross DB compatible? So please use single quotes instead.

>+    # The corresponding series for open bugs look like this. This depends on
>+    # the set of bug states representing open bugs not to have changed since
>+    # series creation.
>+    my $open_bugs_query_base = 
>+        join("&", map { "bug_status=" . url_quote($_) } OpenStates());

Some of my All Open queries have product selection before status selection. They don't seem to have associated All Closed query so I assume they are from some older incarnation of automatic series query generation code? Am I correct or should they be handled somehow?

Has OpenStates() always included UNCONFIRMED status? And has it's position changed from beginning to end? Those older All Open queries seem to have UNCONFIRMED status as first whereas those newer with All Closed has it in the end. Can we safely ignore this discrepancy?

This will obviously break (like the comment says) if the user has added an open status but I think that's unavoidable.

>+            if ($found_open_series_id) {

Nit: Maybe output a warning message if some series can't be fixed because no open query was found for it? This way admin can possibly change them manually or with some special script.

>+                    $sth_select_open_data->execute($found_open_series_id, $date);
>+                    my ($openbugs_value) =
>+                        $sth_select_open_data->fetchrow_array();

These separate execute/fetchrow_array calls can be combined into one selectrow_array() call.

>+                        $sth_fix_broken_nonopen_data->execute($broken_value - $openbugs_value,

Nit: Please fix this line to be within 80 cols.

>+                    else {
>+                        $sth_delete_broken_nonopen_data->execute($broken_series_id,
>+                                                                 $date);
>+                    }

I'd like to see a warning message when this deletion happens.

>+print "Ready.\n";
>+exit;

Eek. Do execute rest of the checksetup.pl too. So remove these debug lines. :)

Comment 25

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.5

Thanks for the review!

(In reply to comment #24)
> Nit: Please use term closed instead of non-open for clarity. Same comment
> applies to other places where this term is used.

Hmmm, I'd rather not. We're looking at more than just closed bugs, we're looking at resolved and verified ones, too. To be consistent, I have editcomponents.cgi say nonopen now as well.
If you want, we can use some other term, but there is currently nothing coming to my mind.

> Developer's Guide seems to say that double quotes shouldn't be used for string
> literals. Probably because they are not cross DB compatible? So please use
> single quotes instead.

Fixed.

> Some of my All Open queries have product selection before status selection.
> They don't seem to have associated All Closed query so I assume they are from
> some older incarnation of automatic series query generation code? Am I correct
> or should they be handled somehow?

I don't know... Are these series maybe manually created?
As the series for non-open bugs is the one that is broken, I'm assuming there is nothing to do here for us.

> Has OpenStates() always included UNCONFIRMED status? And has it's position
> changed from beginning to end? Those older All Open queries seem to have
> UNCONFIRMED status as first whereas those newer with All Closed has it in the
> end. Can we safely ignore this discrepancy?

No, I don't think we can ignore this discrepancy. Good catch! Bug 225687 changed the order. The repairing code now looks for both variants.

> Nit: Maybe output a warning message if some series can't be fixed because no
> open query was found for it? This way admin can possibly change them manually
> or with some special script.

Ok.
Additional change: series query repairs now only happen if an open-bugs query was found.

> These separate execute/fetchrow_array calls can be combined into one
> selectrow_array() call.

No, I want to keep the prepare() part of selectrow_array() out of the enclosing loop.

> Nit: Please fix this line to be within 80 cols.

Done.

> I'd like to see a warning message when this deletion happens.

Ok, you'll get a warning now.

> Eek. Do execute rest of the checksetup.pl too. So remove these debug lines. :)

Ugh, sorry.

Comment 26

[Teemu Mannermaa (:wicked)]

Comment on attachment 204214 [details] [diff] [review]
Patch 1.5

Note: This doesn't apply to either 2.20 or 2.18 branches so they need a backport after tip has an accepted patch.

>Index: checksetup.pl
>===================================================================

>+    my $sth_openbugs_series =
>+        $dbh->prepare("SELECT series_id FROM series
>+                         WHERE query IN (?, ?)");

Nit: Please, line up WHERE like in other statements. :)

>+                $sth_select_broken_nonopen_data->execute($broken_series_id);
>+                foreach ($sth_select_broken_nonopen_data->fetchrow_arrayref()) {
>+                    my ($date, $broken_value) = @$_;

For some reason, only first series data value is fixed. Rest are simply ignored. Also, maybe you should consider moving these two execute/fetch calls into one selectrow_arrayref call and then just process the array in the loop? Note that you don't have to move the prepare statement. Just pass the prepared statement handle as the first parameter and it will just work.

>+                    $sth_select_open_data->execute($found_open_series_id,
>+                                                   $date);
>+                    my ($openbugs_value) =
>+                        $sth_select_open_data->fetchrow_array();

Nit: You really can combine these into one selectrow_array() call because same note about prepare applies here too. I think this would simplify code readability nicely.

>+                    if ($openbugs_value) {

This check considers value zero to be a failure. This means the warning text is printed and series data removed as unrepairable if open count is zero. :(

>+                        print " $broken_series_id...";

Shouldn't this indicate the date that was fixed? Doesn't this now just input long row of same series id's?

>+                        $sth_fix_broken_nonopen_data->execute
>+                            ($broken_value - $openbugs_value,
>+                             $broken_series_id, $date);

What if there are more open bugs than total bugs? Hmm, I guess that shouldn't happen.

>+                              "$broken_series_id, the irreparable data\n" .

Spell as "irrepairable", note the second i. Hmm, that word still sounds strange.

>+                              "entry for date $date was encountered and is " .
>+                              "being deleted.\n" .

Hmm, maybe s/is being/will be/ ?

>+                              "Continuing repairs...";

>+                      "automatically because there is no series to be found\n" .
>+                      "which has collected open bug counts. You'll probably\n" .

Rather "..because no series that collected open bug counts was found. You'll.." ?

>+                      "want to delete or repair it manually.\n" .

Maybe, s/it/collected data for series $broken_series_id/ ?

Comment 27

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.6 for tip

Thank you for your review.

(In reply to comment #26)
> Nit: Please, line up WHERE like in other statements. :)

Oops...

> >+                $sth_select_broken_nonopen_data->execute($broken_series_id);
> >+                foreach ($sth_select_broken_nonopen_data->fetchrow_arrayref()) {
> >+                    my ($date, $broken_value) = @$_;
> 
> For some reason, only first series data value is fixed. Rest are simply
> ignored. Also, maybe you should consider moving these two execute/fetch calls

Yeah; it's a while loop now.

> into one selectrow_arrayref call and then just process the array in the loop?
> Note that you don't have to move the prepare statement. Just pass the prepared
> statement handle as the first parameter and it will just work.

I didn't want to fetch all at once because we may be looking at quite a lot of data. I'm fetching them one-by-one instead.

> >+                    $sth_select_open_data->execute($found_open_series_id,
> >+                                                   $date);
> >+                    my ($openbugs_value) =
> >+                        $sth_select_open_data->fetchrow_array();
> 
> Nit: You really can combine these into one selectrow_array() call because same
> note about prepare applies here too. I think this would simplify code
> readability nicely.

Done.

> >+                    if ($openbugs_value) {
> 
> This check considers value zero to be a failure. This means the warning text 

Added defined().

> >+                        print " $broken_series_id...";
> 
> Shouldn't this indicate the date that was fixed? Doesn't this now just input
> long row of same series id's?

Moved it up so that the ID is printed only once. I didn't want to print a confirmation for each conversion because there may be some many of them.

> What if there are more open bugs than total bugs? Hmm, I guess that shouldn't
> happen.

Agreed.

> >+                              "$broken_series_id, the irreparable data\n" .
> 
> Spell as "irrepairable", note the second i. Hmm, that word still sounds
> strange.

Irreparable seems to be the correct spelling (see http://www.bartleby.com/61/97/I0239700.html).

> >+                              "entry for date $date was encountered and is "
> >+                              "being deleted.\n" .
> 
> Hmm, maybe s/is being/will be/ ?

My thinking here is that at the time the text is being read, it is deleted :)

> >+                      "automatically because there is no series to be found\n" .
> >+                      "which has collected open bug counts. You'll probably\n" .
> 
> Rather "..because no series that collected open bug counts was found. You'll.."
> ?
> 
> >+                      "want to delete or repair it manually.\n" .
> 
> Maybe, s/it/collected data for series $broken_series_id/ ?

Both done.

Comment 28

[Teemu Mannermaa (:wicked)]

Comment on attachment 205872 [details] [diff] [review]
Patch 1.6 for tip

Looks ok and seems to work in my tests. Needs a backport for 2.20 and 2.18.

Comment 29

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.6 for the 2.18 branch

Comment 30

[Marc Schumann [:Wurblzap]]

Attachment: Patch 1.6 for the 2.20 branch

Comment 31

[Marc Schumann [:Wurblzap]]

Tip:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.462; previous revision: 1.461
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.66; previous revision: 1.65
done
Checking in template/en/default/admin/components/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/components/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.4; previous revision: 1.3
done

2.20 branch:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.412.2.20; previous revision: 1.412.2.19
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <-- editcomponents.cgi
new revision: 1.54.4.1; previous revision: 1.54
done
Checking in template/en/default/admin/components/create.html.tmpl;
/cvsroot/mozilla/webtools/bugzilla/template/en/default/admin/components/create.html.tmpl,v  <--  create.html.tmpl
new revision: 1.3.2.1; previous revision: 1.3
done

2.18 branch:
Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.289.2.34; previous revision: 1.289.2.33
done
Checking in editcomponents.cgi;
/cvsroot/mozilla/webtools/bugzilla/editcomponents.cgi,v  <--  editcomponents.cgi
new revision: 1.41.2.3; previous revision: 1.41.2.2
done