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

RESOLVED FIXED in Bugzilla 2.18

Status

()

Bugzilla
Reporting/Charting
RESOLVED FIXED
12 years ago
12 years ago

People

(Reporter: Jeff Jensen, Assigned: Wurblzap)

Tracking

2.18.1
Bugzilla 2.18
Bug Flags:
approval +
blocking2.22 +
approval2.20 +
blocking2.20.1 +
blocking2.20 -
approval2.18 +
blocking2.18.5 +

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

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

Updated

12 years ago
Version: unspecified → 2.18.1
(Reporter)

Updated

12 years ago
Flags: blocking2.18.4?

Comment 1

12 years ago
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.
Severity: major → normal
Flags: blocking2.20+
Flags: blocking2.18.4?
Flags: blocking2.18.4+
OS: Linux → All
Hardware: PC → All
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

12 years ago
You're right -- this query should be
field0-0-0=resolution&type0-0-0=regexp&value0-0-0=. (including the trailing dot).
Status: RESOLVED → UNCONFIRMED
Resolution: WORKSFORME → ---
(Assignee)

Updated

12 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
(Assignee)

Comment 5

12 years ago
Created attachment 191780 [details] [diff] [review]
Patch
Assignee: gerv → wurblzap
Status: NEW → ASSIGNED
Attachment #191780 - Flags: review?(gerv)
(Assignee)

Comment 6

12 years ago
Tested on HEAD and 2.20rc1. Should work on the 2.18 branch as well, but it'll
need a backport.
Whiteboard: [patch awaiting review]
Target Milestone: --- → Bugzilla 2.18
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
(Assignee)

Comment 8

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

Comment 9

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

Comment 10

12 years ago
Created attachment 194423 [details] [diff] [review]
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.
Attachment #191780 - Attachment is obsolete: true
Attachment #194423 - Flags: review?
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
Attachment #194423 - Flags: review? → review+
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
(Assignee)

Comment 13

12 years ago
Created attachment 195175 [details] [diff] [review]
Patch 1.3

Addressed comment 11 and comment 12.
Attachment #194423 - Attachment is obsolete: true
Attachment #195175 - Flags: review?
(Assignee)

Comment 14

12 years ago
Created attachment 195177 [details] [diff] [review]
Patch 1.3 (2.18 branch)

Backport to 2.18.
The previous patch applies to 2.20 as well as HEAD.
Attachment #195177 - Flags: review?
(Assignee)

Comment 15

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

Comment 16

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

12 years ago
justdave, could you have a look at this patch too?
(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.
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?
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.
Flags: blocking2.20.1+
Flags: blocking2.20-
Flags: blocking2.20+
(Assignee)

Comment 21

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

12 years ago
gerv, could you have a look at these patches?
(Assignee)

Updated

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

Updated

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

Comment 23

12 years ago
Created attachment 200859 [details] [diff] [review]
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.
Attachment #195175 - Attachment is obsolete: true
Attachment #195177 - Attachment is obsolete: true
Attachment #200859 - Flags: review?
Flags: blocking2.22+
Attachment #200859 - Flags: review?(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. :)
Attachment #200859 - Flags: review?(wicked)
Attachment #200859 - Flags: review?
Attachment #200859 - Flags: review-
Whiteboard: [patch awaiting review]
(Assignee)

Comment 25

12 years ago
Created attachment 204214 [details] [diff] [review]
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.
Attachment #200859 - Attachment is obsolete: true
Attachment #204214 - Flags: review?(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/ ?
Attachment #204214 - Flags: review?(wicked) → review-
(Assignee)

Comment 27

12 years ago
Created attachment 205872 [details] [diff] [review]
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.
Attachment #204214 - Attachment is obsolete: true
Attachment #205872 - Flags: review?(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.
Attachment #205872 - Attachment description: Patch 1.6 → Patch 1.6 for tip
Attachment #205872 - Flags: review?(wicked) → review+

Updated

12 years ago
Flags: blocking2.18.4+ → blocking2.18.5+
(Assignee)

Comment 29

12 years ago
Created attachment 207185 [details] [diff] [review]
Patch 1.6 for the 2.18 branch
Attachment #207185 - Flags: review?(wicked)
(Assignee)

Comment 30

12 years ago
Created attachment 207186 [details] [diff] [review]
Patch 1.6 for the 2.20 branch
Attachment #207186 - Flags: review?(wicked)
Attachment #207185 - Flags: review?(wicked) → review+
Attachment #207186 - Flags: review?(wicked) → review+
Flags: approval?
Flags: approval2.20?
Flags: approval2.18?
Flags: approval?
Flags: approval2.20?
Flags: approval2.20+
Flags: approval2.18?
Flags: approval2.18+
Flags: approval+
(Assignee)

Comment 31

12 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.