216008 - Time Tracking: default values cause change bug errors

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, defect)

Description

[Paul Gregg]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4) Gecko/20030624

When time tracking is enabled, adding comments to a bug errors out because
Bugzilla thinks that you are changing the time from "0.00" to "0.0".

Reproducible: Always

Steps to Reproduce:
1. Turn on time tracking by setting a group you are in, in the timetrackinggroup
field on editparams.cgi
2. Go to an existing bug
3. Notice time tracking is now visible (Orig. Est.="0.0" & Hours Left="0.0")
4. Add a comment to that bug, don't change anything else -->Commit
Actual Results:  
Error comes up, saying you are changing the Original time from 0.00 to 0.0

Workaround is to go back and change Orig. Est. & Hours Left to 0.00

Expected Results:  
time shouldn't be altered, as the user didn't change the time, and shouldn't
error out

I'm having trouble reproducing this using our local copy of Bugzilla using my
profile.

I witnessed one of our users having this problem, and the workaround of changing
the times to 0.00 worked.

Also, the Error mentioned "Remaining Time" when associated with "Hours Left",
and I noticed the user had initial problems associating the two, maybe they
should be the same name??

Running 2.17.4 on SunOS 5.8
Possibly related to Bug 215962.

Comment 1

[Dave Miller [:justdave]]

Jeff, any ideas?

Comment 2

[Paul Gregg]

I can reproduce consistently with the other user.
Let me know if you want me to provide the exact error message and parameters for
replication.

Comment 3

[Jeff Hedlund]

Hm, yes, those error messages should be consistent with the naming of the fields
on the edit bug page.

It would help if you had the exact error message and parameters.  

And am I correct in assuming that this is a bug that was in the system before
time tracking was turned on?  Was it a bug in the system before 2.17 was installed?

Comment 4

[Paul Gregg]

Upgraded last week from 2.16.1 to 2.17.4
Only enabled time tracking once 2.17.4 was working, so never had the problem before.

Parameters:
* Created a group called "TimeTrackers"
* Added "TimeTrackers" to the timetrackinggroup param.
* Added myself as member & turn bit on for other users
* Added "Bob" as member

I have no problems, with either Moz1.4 or IE6sp1 on XPsp1

Logging in as Bob, I hit it.
If I just enter in a comment and Commit, I get:
############################################
Not allowed  
    
You tried to change the estimated_time field from 0.00 to 0.0, but only the
owner or submitter of the bug, or a sufficiently empowered user, may change that
field.
############################################

But Bob is part of the TimeTrackers group, so not sure why he can't modify the
values, but more importantly is the fact that the form inserts the 0.0, yet when
processing, complains that its not the same as what the system thinks it is!!!


If I change Orig. Est. to 0.00, things still break:
############################################
Not allowed  
    
You tried to change the remaining_time field from 0.00 to 0.0, but only the
owner or submitter of the bug, or a sufficiently empowered user, may change that
field.
############################################

Changing Hours Left to 0.00, bug is submitted fine.
Interestingly, when reloaded the bug, it again comes up with 0.0, and makes life
hard again.

Wording issues:
1. "Orig. Est." becomes "estimated_time" in error page
2. "Hours Left" becomes "remaining_time" in error page

I would suggest changing "estimated_time" to "Original Estimate" &
"remaining_time" to "Hours Left".  The underscores hint at a variable name.

Comment 5

[Max Kanat-Alexander]

Does "Bob" have editbugs? -M

Comment 6

[Paul Gregg]

"Bob" doesn't have editbugs.
The workaround we used for a real user was to add editbugs (sorry, I forgot to
mention it here).  He also has canconfirm, but I think that’s unrelated.

I believe there's 3 issues here:
 1. Timetracking functionality shouldn't change its representation of what zero
hours is (which means equivalence tests don't work)

 2. A user in the Timetrackers group should then have the privileges to edit
Timetracking fields, or am I confused about how it is supposed to work.

 3. There should be consistency between the naming of fields within Timetracking
and the error messages.

Comment 7

[Jeff Hedlund]

(In reply to comment #6)
> "Bob" doesn't have editbugs.
> The workaround we used for a real user was to add editbugs (sorry, I forgot to
> mention it here).  He also has canconfirm, but I think that’s unrelated.

canconfirm is unrelated, but the user does need editbugs.

> I believe there's 3 issues here:
>  1. Timetracking functionality shouldn't change its representation of what zero
> hours is (which means equivalence tests don't work)

It needs to compare numerically.  It happens on non-zero time too ("1.00" to
"1.0").  I'll work on a patch if it is decided that non-editbugs users should be
allowed to change timetracking fields.  

>  2. A user in the Timetrackers group should then have the privileges to edit
> Timetracking fields, or am I confused about how it is supposed to work.

Yes, a user in the timetracking group should have privileges to edit the
timetracking fields.  Currently it is assumed that the people in the
timetracking group have editbugs capabilities.  

Should users that are not in editbugs be allowed to make timetracking changes?

>  3. There should be consistency between the naming of fields within Timetracking
> and the error messages.

I'm not getting the fieldnames in the error message, I'm getting the "Orig.
Est." and "Hours Left".  Perhaps something was broken in 2.17.4 with error handling?

Comment 8

[Paul Gregg]

(In reply to comment #7)
> It needs to compare numerically.  It happens on non-zero time too ("1.00" to
> "1.0").  I'll work on a patch if it is decided that non-editbugs users should
> be allowed to change timetracking fields.  

Well they effectively have the privilege now if they change 0.0 --> 0.00

> Yes, a user in the timetracking group should have privileges to edit the
> timetracking fields.  Currently it is assumed that the people in the
> timetracking group have editbugs capabilities.  
> 
> Should users that are not in editbugs be allowed to make timetracking changes?

I think so, because they are mutually exclusive options, so for one to depend on
another doesn't make sense IMHO.  We have a usecase here, obviously, because we
found the bug. The was because we have a user who is a Timetracker because they
have to sign off on hours worked, but is not involved in day to day running of
our Bugzilla.

> I'm not getting the fieldnames in the error message, I'm getting the "Orig.
> Est." and "Hours Left".  Perhaps something was broken in 2.17.4 with error
handling?

Cool, it must be fixed then !! (I can't confirm coz running 2.17.4 still)

Comment 9

[Christian Reis]

Attachment: kiko_v1: for TT fields, do numerical comparison.

Here's a fix for this issue: if you're in timetrackinggroup and you just want
to enter a comment this lets you do it. Note however that the correct fix for
this issue is to include a read-only timetracking form for people in TTgroup
and not in editbugs -- but I guess this applies to any field in this form,
really (we shouldn't offer editable boxes and select combos if the user can't
change them!).

Comment 10

[Christian Reis]

The problem is really caused by checking oldvalue and newvalue using "eq", which
isn't really correct for numerical values.

Comment 11

[Jouni Heikniemi]

Comment on attachment 154133 [details] [diff] [review]
kiko_v1: for TT fields, do numerical comparison.

Ok. Note that when constructing activity log entries from the bug snapshots
later on eq (or ne) operator is still used for all fields. However, since the
string representations pulled from the DB seem to be the same both pre and post
save, this doesn't make a difference.

This file needs a rewrite.

r=jouni

Comment 12

[Dave Miller [:justdave]]

This is a back-end only change, it's low risk, and it does the right thing. 
Let's put it in 2.18, too.

Comment 13

[Christian Reis]

Landed on trunk:

/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.210; previous revision: 1.209

and branch:

/cvsroot/mozilla/webtools/bugzilla/process_bug.cgi,v  <--  process_bug.cgi
new revision: 1.205.2.2; previous revision: 1.205.2.1

Thanks.