344072 - Require Time Tracking for certain products

Status: NEW

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[A. Karl Kornel]

Right now people can enter time tracking information, if they're in the time tracking group and only if they want to.  How about, on a product-by-product basis, forcing people to enter time tracking information if they're in the time tracking group?

Of course, this could get out of hand rather quickly, so I'd limit the requirement to times when a comment is entered.  That way, quick actions like adding someone to the CC list (which doesn't need a comment) wouldn't force people to record the 30 seconds they spent adding someone to the CC list!

Comment 1

[Max Kanat-Alexander]

I'd be against requiring time tracking on any Bugzilla installation just to add a comment. I don't think it would be helpful to anybody.

Could you describe a situation where this would be useful to an organization using Bugzilla?

Comment 2

[A. Karl Kornel]

(In reply to comment #1)
> Could you describe a situation where this would be useful to an organization
> using Bugzilla?

This is actually a patch that was requested by, and is in, our organization.  We now provide software support to certain customers, and the support contracts limit support by # of hours (among other things).  We require our engineers to enter time tracking information so we can determine how many hours of support have been used up.  Customers aren't in the time tracking group, so the change doesn't affect them.

Comment 3

[A. Karl Kornel]

Attachment: Version 1

Not that I'm trying to railroad this through, but I've got a patch ready so I might as well post it!  I'm still open to comments, and this functionality is turned off by default.

This patch makes changes in several areas.  It begins with `checksetup.pl` and `DB/`Schema.pm`, to add a new column to the products table.  There is also a new parameter, manifested in `Config/BugChange.pm` and `admin/params/bugchange.html.tmpl`, which either makes time tracking the default for new products or does not.

Most of the changes are in the products area.  `Product.pm` is now aware of the new DB field (so it will get the field value when Product objects are created), and `editproducts.cgi` is changed to check the value when creating/changing products.  Four products templates were also changed to include this option: `admin/products/create.html.tmpl`, `admin/products/updated.html.tmpl`, `admin/products/edit-common.html.tmpl`, and `admin/products/confirm-delete.html.tmpl`.

Next up is bug display and processing.  `bug/edit.html.tmpl` has several notable changes in it. Under the label "Hours Worked" is some additional HTML.  "(required)" is displayed if the currently-selected product requires time tracking; this is displayed/hidden using CSS, and its state is changed using JavaScript.  A JavaScript array is created, specifying if each product in the products list requires time tracking.  This JavaScript array is generated from a Perl array, which is created inside `Bug.pm`.  The JavaScript array isn't generated, and the "(required)" tag is kept hidden, if the user accessing the page isn't in the timetracking group.

`process_bug.cgi` has been modified to check if timr-tracking is required when a change is made.  If time tracking information is required, but not present when a submission is made, an error is thrown (error text is in `global/user-error.html.tmpl`).

Comment 4

[Dave Miller [:justdave]]

I had a nice big comment written up on this the other night and lost it. :(

But anyway, I just talked with karl about this on IRC just now, and my basic view is that I think this is setting a dangerous precedent because it could lead to bunches of columns getting added to the products table for willy-nilly things.  I'd prefer this to be implemented as a plugin if possible, where we figure out what hooks would be needed and add them as appropriate if missing.

However, I'm not against the idea of checking this in in its current form (or something similar to it), if the plugin route proves too difficult or time consuming.

We'd discussed on IRC the other day that what we really need is some sort of rule-based processing that admins can set up on bug changes that could be used for this sort of thing.  But that's going to be a lot of work to implement, and certainly not worth stalling this for it.

Comment 5

[Frédéric Buclin]

Making DB schema changes which will be eventually reverted later doesn't sound like a good idea to me.

Comment 6

[Frédéric Buclin]

Comment on attachment 228735 [details] [diff] [review]
Version 1

karl, I prefer to be honest with you and say that I'm not a fan of reviewing this patch for the reasons justdave mentioned above + the discussion we had on IRC. Keeping this patch in my review queue would make this review never happen. I prefer you to ask another reviewer. Sorry.

Or implement this as part of the workflow bug, i.e. to go from A to B, you have to match this list of restrictions/conditions. Maybe a better approach, but much harder to do, probably.

Comment 7

[A. Karl Kornel]

(In reply to comment #6)
> Or implement this as part of the workflow bug, i.e. to go from A to B, you have
> to match this list of restrictions/conditions. Maybe a better approach, but
> much harder to do, probably.

In that case, you'd have to help me convince mkanat to make custom status & workflow even bigger.

(In reply to comment #4)
> But anyway, I just talked with karl about this on IRC just now, and my basic
> view is that I think this is setting a dangerous precedent because it could
> lead to bunches of columns getting added to the products table for willy-nilly
> things.  I'd prefer this to be implemented as a plugin if possible, where we
> figure out what hooks would be needed and add them as appropriate if missing.

I'd agree to that (changing this patch to be the first contrib code/template plugin) on 2 conditions:

1) Someone else would need to write up & land the appropriate template & code hooks, since I don't know anything at all about code hooks.
2) Someone would still need to review this patch, when it is rewritten into plugin form.

Comment 8

[Max Kanat-Alexander]

(In reply to comment #7)
> In that case, you'd have to help me convince mkanat to make custom status &
> workflow even bigger.

  I don't need any convincing. Just don't do two things in one bug.

  And it doesn't have to be part of workflow, anyway--it can be a part of general field preferences, which we haven't implemented yet.

Comment 9

[Frédéric Buclin]

This bug is retargetted to Bugzilla 3.2 for one of the following reasons:

- it has no assignee (except the default one)
- we don't expect someone to fix it in the next two weeks (i.e. before we freeze the trunk to prepare Bugzilla 3.0 RC1)
- it's not a blocker

If you are working on this bug and you think you will be able to submit a patch in the next two weeks, retarget this bug to 3.0.

If this bug is something you would like to see implemented in 3.0 but you are not a developer or you don't think you will be able to fix this bug yourself in the next two weeks, please *do not* retarget this bug.

If you think this bug should absolutely be fixed before we release 3.0, either ask on IRC or use the "blocking3.0 flag".

Comment 10

[Teemu Mannermaa (:wicked)]

Comment on attachment 228735 [details] [diff] [review]
Version 1

What a surprise, this patch has bitrotted. Only slightly, though.

Hunks 2 and 5 of changes in template/en/default/bug/edit.html.tmpl don't apply cleanly anymore. Also, UserInGroup used in that file is no more. It's user.in_group since bug 283582.

>Index: checksetup.pl
>===================================================================
>+# Bug 344072 - 2006-07-10 - [e-mail]
>+$dbh->bz_add_column('products', 'requiretime', 
>+                    {TYPE => 'INT1', NOTNULL => 1, DEFAULT => '0'});

Shouldn't this be a BOOLEAN? Also, this section has been moved to Bugzilla/Install/DB.pm in update_table_definitions subroutine.

>Index: Bugzilla/DB/Schema.pm
>===================================================================
>+            requiretime       => {TYPE => 'INT1', NOTNULL => 1,
>+                                  DEFAULT => '0'},

Same BOOLEAN remark here.

>Index: template/en/default/admin/params/bugchange.html.tmpl
>===================================================================
>+  requiretime => "If this option is on, all new products will require " _
>+                 "time tracking on by default.  This option does not affect " _

Change "on by" to "for [% terms.bug %] comments by".

>Index: editproducts.cgi
>===================================================================
>@@ -200,6 +200,7 @@
>+    my $requiretime = $cgi->param('requiretime') =~ /^(1)$/ ? $1 : 0;
>@@ -745,6 +747,7 @@
>+    my $requiretime         = $cgi->param('requiretime') =~ /^(1)$/ ? $1 : 0;

Theses lines cause "Use of uninitialized value in pattern match (m//) at" errors to popup if requiretime checkbox is not selected.

>Index: template/en/default/admin/products/confirm-delete.html.tmpl
>===================================================================
>+    <td>Require time tracking for [% terms.bugs %] in this product?</td>

Better wording would be "Require time tracking for [% terms.bug %] comments:".

>Index: template/en/default/admin/products/create.html.tmpl
>===================================================================
>+    [% PROCESS "admin/products/edit-common.html.tmpl" 
>+      create = 1

Instead of creating new parameter for this template, add the wanted default for requiretime to the product variable in the DEFAULT block few lines above this line and let edit-common do same thing for both update and create calls.

>Index: template/en/default/admin/products/edit-common.html.tmpl
>===================================================================
>@@ -111,3 +113,15 @@
>+    Require time tracking for [% terms.bugs %] in this product:

Better wording would be to change "..for [% terms.bugs %] in this product:" to "..for [% terms.bug %] comments:".

>Index: template/en/default/admin/products/updated.html.tmpl
>===================================================================
>@@ -79,6 +79,16 @@
>+  require time tracking for [% terms.bugs %] in this product.

Change "..for [% terms.bugs %] in this product." to "..for [% terms.bug %] comments."

>Index: Bugzilla/Bug.pm
>===================================================================
>+       'time_required' => \@time_required,

Instead of using raw SQL for this, use products objects in the prod_obj array.

>Index: process_bug.cgi
>===================================================================
>+        trick_taint($product);

No need to trick taint this just for the Bugzilla::Product constructor. Besides, this uses same variable for both product name and object which is confusing.

>+        if ($product->require_time &&
>+            (!$wk_time || ($wk_time == 0)) &&
>+            defined($cgi->param('comment')) &&
>+            ($cgi->param('comment') !~ /^\s*$/)

Not sure if we need to check for comment because at this point we know there's a comment if work time is set.

>+            ThrowUserError('time_tracking_required') 
>+                if $user->in_group(Bugzilla->params->{'timetrackinggroup'});

This if is useless as we are already inside if block that only runs when user is in timetrackinggroup.

>Index: template/en/default/bug/edit.html.tmpl
>===================================================================
>+    time_required[[% i %]] = [% bug.choices.time_required.shift FILTER js %];/*
>+    [%- p FILTER html %] */

Nit: Add space before and after of the comment start (/*) markup.

>@@ -434,6 +458,9 @@
>+            <b>(required)</b>

No need to use b tag here.

>@@ -695,7 +729,9 @@
>+     [% IF onchange %]onchange="[% onchange FILTER html %]"[% END %]

You need to blank onchange var at the end of this block or the onchange is copied to all select blocks that come after product select. See input BLOCK for an example.

Nit: No blank before onchange in the generated HTML.

>Index: template/en/default/global/user-error.html.tmpl
>===================================================================
>+    You must enter the number of hours worked when changing or 
>+    commenting on this [% terms.bug %].

Drop "changing or" because looks like only commenting honors require_time setting.

Comment 11

[Max Kanat-Alexander]

Bugzilla 3.2 is now frozen. Only enhancements blocking 3.2 or specifically approved for 3.2 may be checked in to the 3.2 branch. If you would like to nominate your enhancement for Bugzilla 3.2, set the "blocking3.2" flag to "?". Then, either the target milestone will be changed back, or the blocking3.2 flag will be granted, if we will accept this enhancement for Bugzilla 3.2.

This particular bug has not been touched in over eight months, and thus is being retargeted to "---" instead of "Bugzilla 4.0". If you believe this is a mistake, feel free to retarget it to Bugzilla 4.0.