329405 - [Time-Tracking] Add a start date to Time tracking

Status: NEW

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[oliver]

User-Agent:       Mozilla/5.0 (compatible; Konqueror/3.4; Linux) KHTML/3.4.0 (like Gecko)
Build Identifier: 

Actually we got a deadline in the time tracking but no
start date.
The bug 235059 wish to generate a gantt so it s also needed.
Add a new field in a first time seem me simple.

Reproducible: Always

Comment 1

[oliver]

Attachment: add startline v0.1

this patch add a field "startline"

Im not shure it s the better name
I have made few test it s working

Comment 2

[oliver]

Comment on attachment 229537 [details] [diff] [review]
add startline v0.1

incomplete and only work with 2.22

Comment 3

[oliver]

Attachment: startline for cvs version

Comment 4

[Frédéric Buclin]

Max, this patch looks like to implement a new 'date' field. This could probably be managed by the custom field feature. I'm not a fan to hardcode a new field. WONTFIX?

Comment 5

[oliver]

"add" is ok
but I have:
  '' is not a legal date. Please use the format 'YYYY-MM-DD'.

after a search

waiting to know if wont fix or not

Comment 6

[Frédéric Buclin]

do not request approval unless a reviewer granted review.

Comment 7

[oliver]

"add" is ok
but I have:
  '' is not a legal date. Please use the format 'YYYY-MM-DD'.

after a search

waiting to know if wont fix or not

Comment 8

[Frédéric Buclin]

For the record:

<justdave> The question was whether it'd qualify for a schema change just for it or if it needed to be a custom field
<justdave> Right now I'm tending toward going ahead and putting it in.
<justdave> we already have precident for the timetracking stuff being native objects
<justdave> might want to run it past myk quickly though because he's been working on the custom fields stuff.
<justdave> if he doesn't have a stong opinion that it should wait for custom fields I'm fine with it
<justdave> (fine with adding it as a native column that is)

Comment 9

[Max Kanat-Alexander]

It's an additional time-tracking field, so perhaps we could add it, even though we *are* going to have Custom Fields. (Basically, yeah, what justdave said.)

Comment 10

[Frédéric Buclin]

<myk> LpSolit: it should go in, not wait for custom fields

Comment 11

[Dave Miller [:justdave]]

(In reply to comment #10)
> <myk> LpSolit: it should go in, not wait for custom fields

And with that unambiguous lack of objection from myk, consider it approved.

Comment 12

[oliver]

Attachment: add startline v0.3

Comment 13

[oliver]

1. I have missed the colchange

Index: colchange.cgi
===================================================================
RCS file: /cvsroot/mozilla/webtools/bugzilla/colchange.cgi,v
retrieving revision 1.57
diff -u -p -r1.57 colchange.cgi
--- colchange.cgi       17 Jul 2006 19:57:35 -0000      1.57
+++ colchange.cgi       18 Jul 2006 17:54:55 -0000
@@ -73,7 +73,7 @@ if (Bugzilla::Keyword::keyword_count())

 if (UserInGroup(Bugzilla->params->{"timetrackinggroup"})) {
     push(@masterlist, ("estimated_time", "remaining_time", "actual_time",
-                       "percentage_complete", "deadline"));
+                       "percentage_complete", "startline", "deadline"));
 }

 push(@masterlist, ("short_desc", "short_short_desc"));

2. on the result of a search I still have start line after deadline. It s not nice. But I don't know how to have startdate first

3. do we have to add a constraint to verify that the deadline is after the startline?

Comment 14

[Frédéric Buclin]

(In reply to comment #13)
> 1. I have missed the colchange

Attach a new patch with colchange.cgi included.


> 3. do we have to add a constraint to verify that the deadline is after the
> startline?

Would be fine, but not mandatory (this could be done in a separate bug).

Comment 15

[oliver]

okay for 2 (column order startline first deadline next) it s the order in the cookie that gave the order
with the colchange.cgi patch in comment 13 

Comment 16

[Frédéric Buclin]

Comment on attachment 229684 [details] [diff] [review]
add startline v0.3

> DefineColumn("deadline"  ...
>+DefineColumn("startline" ...

Maybe it's just me, but I would really prefer to see the code relative to startline being before the one for deadline, just because the "start" comes before the "end". ;) This remark applies everywhere in this patch.



>Index: checksetup.pl

>+AddFDef("startline", "Startline", 1);

This code is no longer in checksetup.pl.


>+$dbh->bz_add_column("bugs", "startline", {TYPE => 'DATETIME'});

Nor this one.


>Index: importxml.pl

>+        my $date = format_time( $bug_fields{'startline'}, "%Y-%m-%d" )
>+          || undef;
>+        push( @values, $date );
>+        push( @query,  "startline" );

Could you refactor this code a bit? Maybe a loop over 'startline', 'deadline' would be better.


>Index: post_bug.cgi

>+if ( UserInGroup(Bugzilla->params->{"timetrackinggroup"})
>+     && $cgi->param('startline') )
>+{
>+    validate_date($cgi->param('startline'))
>+      || ThrowUserError('illegal_date', {date => $cgi->param('startline'),
>+                                         format => 'YYYY-MM-DD'});
>+    $startline = $cgi->param('startline');
>+    trick_taint($startline);
>+}

Same comment here and in several other places.


This was only a quick review, for the reasons above and because the patch doesn't apply cleanly anymore on the trunk (there has been many large checkins recently).

Comment 17

[Max Kanat-Alexander]

Also, "startline" is funny, but doesn't mean anything in English. Make it start_date or something instead.