733586 - Add cf_last_resolved column for metrics to use to determine when a bug was last closed

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[David Lawrence [:dkl]]

When at Red Hat we wrote an extension that added a new custom date field called 'cf_last_closed' that recorded that timestamp when the bug was last resolved. The show_bug.cgi page would show a readonly value similar to reported time and modified.
Using the bug_end_of_update hook, the extension would keep the value updated properly based on status change. Then anyone needing access to the database would be able to query that column to get the date. Otherwise if we abstracted the calculation of this value out using an extension it would only be available for query through the UI or a WebService call.

I have a working extension patch for review. Also if it lands, we can use it in the BMO extension instead of the last_closed_date method that is there now.

dkl

Comment 1

[David Lawrence [:dkl]]

Attachment: Patch to add cf_last_closed for use by metrics (v1)

Comment 2

[:glob ✱]

Comment on attachment 603496 [details] [diff] [review]
Patch to add cf_last_closed for use by metrics (v1)

Review of attachment 603496 [details] [diff] [review]:
-----------------------------------------------------------------

mcoate's request:

<mocates> is it correct that we have no "closed" date field for bugs?
<LpSolit> mcoates: correct
<mcoates> so there is no way to determine a metric on average time to close?
<LpSolit> define close. resolved, verified?
<mcoates> resolved

this is different from "closed" because a closed bug according to the bug_status table is RESOLVED, VERIFIED or CLOSED.

this extension and the field it adds should be called "last_resolved" not "last_closed", and only track when the bug was resolved, not when it was verified or closed.

::: extensions/LastClosed/Extension.pm
@@ +66,5 @@
> +}
> +
> +sub _cf_last_closed {
> +    my ($self) = @_;
> +    return format_time($self->{'cf_last_closed'});

don't call format_time if cf_last_close is undefined:

Use of uninitialized value $date in pattern match (m//) at Bugzilla/Util.pm line 421.  at Bugzilla/Util.pm line 421 \tBugzilla::Util::format_time(...) called at ./extensions/LastClosed/Extension.pm line 70  \tBugzilla::Extension::LastClosed::_cf_last_closed(...) called at ./extensions/LastClosed/Extension.pm line 96

@@ +87,5 @@
> +        @$args{qw(bug old_bug timestamp changes)};
> +    if ($changes->{'bug_status'}) {
> +        # If the bug has been closed then update the cf_last_closed
> +        # value to the current timestamp if cf_last_closed exists
> +        if (!$bug->status->is_open

you need to check for RESOLVED only, not is_open()

Comment 3

[David Lawrence [:dkl]]

Attachment: Patch to add cf_last_resolved for use by metrics (v2)

Thanks for the review and the clarification about RESOLVED. Here is a new patch which should address your suggested changes.

dkl

Comment 4

[David Lawrence [:dkl]]

Attachment: Patch to add cf_last_resolved for use by metrics (v3)

New patch that hides field completely from show_bug.cgi but still allows as a column of buglist.cgi

Also fixed _cf_last_resolved to return undef.

Note to self: Need to schedule outage when this is pushed live so will try to do so with other changes such as globs qa contact migration.

dkl

Comment 5

[:glob ✱]

Comment on attachment 603752 [details] [diff] [review]
Patch to add cf_last_resolved for use by metrics (v3)

Review of attachment 603752 [details] [diff] [review]:
-----------------------------------------------------------------

r=glob

review comments can be addressed at commit time.

::: extensions/LastResolved/Extension.pm
@@ +20,5 @@
> +our $VERSION = '0.01';
> +
> +BEGIN {
> +    *Bugzilla::Bug::cf_last_resolved = \&_cf_last_resolved;
> +}

bugzilla already knows now to return values for datatime custom fields, you don't need to monkeypatch this.

@@ +88,5 @@
> +    if ($changes->{'bug_status'}) {
> +        # If the bug has been resolved then update the cf_last_resolved
> +        # value to the current timestamp if cf_last_resolved exists
> +        if ($bug->bug_status eq 'RESOLVED'
> +            && Bugzilla::Field->new({ name => 'cf_last_resolved' }))

you don't need to check for the field; if this ext is installed it will always be there.

Comment 6

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #5)
> ::: extensions/LastResolved/Extension.pm
> @@ +20,5 @@
> > +our $VERSION = '0.01';
> > +
> > +BEGIN {
> > +    *Bugzilla::Bug::cf_last_resolved = \&_cf_last_resolved;
> > +}
> 
> bugzilla already knows now to return values for datatime custom fields, you
> don't need to monkeypatch this.

Originally I did when I was displaying the field on show_bug.cgi as I wanted to put it through format_time(). But now that we're not doing that, I can remove this.
 
dkl

Comment 7

[David Lawrence [:dkl]]

Attachment: Patch to add cf_last_resolved for use by metrics (v4)

New patch with suggested fixes. Moving r+ forward.

We will need to wait til a planned outage for this to be pushed to production as it modifies the DB schema and also requires a migration of values. We may have an outage coming soon to do some other tasks so we can roll it in with that one.

dkl

Comment 8

[:glob ✱]

Committing to: bzr+ssh://[e-mail]/bmo/4.0/
added extensions/LastResolved
added extensions/LastResolved/Config.pm
added extensions/LastResolved/Extension.pm
added extensions/LastResolved/template
added extensions/LastResolved/template/en
added extensions/LastResolved/template/en/default
added extensions/LastResolved/template/en/default/hook
added extensions/LastResolved/template/en/default/hook/global
added extensions/LastResolved/template/en/default/hook/global/field-descs-end.none.tmpl
Committed revision 8109.