271913 - Don't force the user to comment when adding Hours Worked

Status: RESOLVED FIXED

(Bugzilla :: Creating/Changing Bugs, enhancement)

Description

[Shane H. W. Travis]

In the current implementation, users are forced to enter a comment when 
changing the "Hours Worked" field, even if they change nothing else. 

This is not in keeping with the Bugzilla design decisions to date that allow 
individual site-admins control over which changes require comments and which do 
not. I have already been asked to change this locally, so I'll append my patch.

Comment 1

[Shane H. W. Travis]

Attachment: Code patch for 2.18 and tip

Comment 2

[Shane H. W. Travis]

Comment on attachment 167146 [details] [diff] [review]
Code patch for 2.18 and tip

Joel, since you reviewed the initial E|A|R patches, I'm requesting your review
on this one too. Thanks,

Comment 3

[Joel Peshkin]

I tried hacking this in a long time ago and found that there are a lot of places
in the code that depend on having a non-blank comment otherwise they don't build
a longdescs record.  Did you test this out?

Comment 4

[Shane H. W. Travis]

Just to clarify, are you talking about non-blank comments being needed *for 
timetracking*, or for other things? (And if there is no comment, why would you 
NEED a longdesc entry? Am I missing/forgetting something?)

As for testing... after coding this, I installed it on a 2.18rc3 branch (our 
test system) and confirmed that:
1) With timetracking (TT) on, and commentontimetracking (CoTT) on, a user
     received an error when entering a time with no comment.
2) With TT on and CoTT off, a user could enter a time with no comment,
3) With TT off and CoTT on, behaviour was otherwise unaffected.

I tested this only for entering times/comments in an existing bug. Once the 
comments were entered (or not entered), I did not regression-check other parts 
of the code to see if times-with-no-comments would affect them, as it never 
occurred to me that it might. Can you remember anywhere specific that you 
recall seeing an effect like this?

Comment 5

[Joel Peshkin]

The time is actually recorded as a field in the longdescs table.  If no row is
added to that table, then the time is not recorded.  In order to have this
feature work properly, you will have to create a row in longdescs with a blank
comment but with a non-zero time and all the code will have to be happy with
that row.  This was not previously the case.

Comment 6

[Shane H. W. Travis]

(In reply to comment #5)
> The time is actually recorded as a field in the longdescs table.  If no row is
> added to that table, then the time is not recorded.

I am just upgrading from 2.16, and had not caught that this was stored in the 
longdescs table. I only went looking for it in bugs_activity, which is where 
everything else about 'what has changed on this bug' is stored. After this 
patch, the time is recorded just fine... in the bugs_activity table. You're 
right in that it won't get stored in longdescs without a comment to go along 
with it.

This shatters one of the fundamental rules of database design -- namely, that 
you don't store the exact same information in multiple places. I know that BZ 
isn't perfect in following DB conventions, but I thought that the current 
coding practices were to 'do things better'. I am boggled by this design 
decision; I am similarly boggled that such a basic flaw survived through the 
review process for a new enhancement.

(FTR: I *did* do the testing I indicated, and had a problem bringing the form 
up after this patch was applied and Step 2 had been performed, but my test 
system often has issues like that because it's overworked and underpowered, so 
I chalked it up to that. I looked in the DB and it all seemed to apply 
properly, so I left it at that. I realize now that the form is NEVER going to 
come up with this patch applied.)

Joel, you're right that this patch is not going to fix what I wanted fixed, 
because there are *way* more problems under the surface than I had realized. So 
(to me) the question now becomes; should this be fixed so that the database 
retains some semblance of Normal Form? 

According to a recent post from Dave in netscape.mozilla.public.webtools 
(http://www.google.ca/groups?
hl=en&lr=&c2coff=1&selm=mailman.1101099061.6050.mozilla-webtools%40mozilla.org) 
the bug_when fields are guaranteed always to be the same for longdescs and 
bugs_activity, so there shouldn't be any reason why the code can't just take 
the hours_worked from the bugs_activity table, and join it with longdescs based 
on bug_when, rather than storing it directly in the longdescs table. 

Thoughts?

Comment 7

[Joel Peshkin]

Well, the longdescs table should win because the activity table is considered to
be a "human-readable" log rather than one that can be used to, for example, roll
back the state of a bug to an earlier point in time.  There have been some
fierce debates among the team about this.  For example, what happens to activity
entries when products/components/users are subsequently renamed??  

Personally, I think that the activity log needs a major overhaul.

Comment 8

[Shane H. W. Travis]

I still stand by what I said in comment #0 -- that the lack of a 
commentontimetracking parameter is inconsistent with other things in Bugzilla. 
Having learned more, however, I realize now that this is not something that can 
be fixed easily, if at all. Since the timetracking is stored in the longdescs 
field, it's not really possible to enter a value *without* entering a comment. 
IMHO, this is a poor place to store timetracking... but I'd rather have it 
there than not at all.

I am resolving this bug as WONTFIX not because I think it's a bad idea, but 
because I don't think that it is fixable at this time, with this schema -- at 
least, not without a lot of underlying work on the code to be able to deal with 
rows in longdesc that have nothing in their 'thetext' field. If that work is 
ever done, or if time tracking ever gets moved to another table (as it probably 
should be, since not every longdesc entry has a timetracking entry), then this 
can be re-opened and re-assessed... but for now, IMHO, it's too much work for 
too little payoff.

Comment 9

[Shane H. W. Travis]

*** Bug 280974 has been marked as a duplicate of this bug. ***

Comment 10

[Stephen Lee]

Rather than changing the schema, couldn't this work like RESOLVED DUPLICATE if 
there is no comment?

i.e. rather than complaining that the user didn't supply a comment, simply add 
some machine-generated comment text something like

*** 15 hours worked ***

Comment 11

[Shane H. W. Travis]

(In reply to comment #10)
> i.e. rather than complaining that the user didn't supply a comment, 
> simply add some machine-generated comment text something like
> *** 15 hours worked ***

That would work fine as a local customization if everyone in your site was in a 
group that had access to timetracking. (In fact, since that holds true for me, 
I was considering this very solution for my own site just the other day.)

As a solution for the trunk, however, I do not think that it would be 
acceptable, as it would put the time information into the comments field, thus 
negating the whole purpose of hiding the information from users not in a 
timetracking-priviliged group.

Comment 12

[Steve Wardell]

This proposed changed has benefits for those who have access to time but for
those who wish to not receive emails on "comments" that are solely time changes.
This change would allow the addition of a user configuration option to not
receive emails for time worked changes only. Is there another route that that
level of gradularity might be achieve?

Comment 13

[Shane H. W. Travis]

Comment on attachment 167146 [details] [diff] [review]
Code patch for 2.18 and tip

Marking my own patch obsolete, mostly since it doesn't work.

Comment 14

[Shane H. W. Travis]

(In reply to comment #12)
> This proposed changed...

*which* proposed change? Sorry, I'm not following you.

As to your thoughts on adding an email parameter not to receive mail for time-
tracking changes only... I think that's a fine idea, *when* time tracking gets 
its own table, and you really can add a time change independent of any 
comments. Right now, any time tracking must also be accompanied by a comment, 
so the 'receive mail on comments' stuff takes precedence, and always will.

If/when a new bug is opened and resolved that moves work_time out of longdescs, 
all those things (and this) can happen, all as part of their own bugs. If that 
happens, this one could be reopened and marked as being BLOCKED by that one... 

Comment 15

[Stephen Lee]

(In reply to comment #14)
> If/when a new bug is opened and resolved that moves work_time out of
> longdescs, [...], this one could be reopened and marked as being BLOCKED
> by that one... 

I've entered a bug 281354 which could be a candidate for this.

Comment 16

[Shane H. W. Travis]

I have been reminded that WONTFIX isn't for things that can't be fixed now, but 
for things that won't be fixed *ever*. This is a valid complaint, and putting 
in any form of RESOLVED stops people from finding when looking for duplicates.

This could/should be fixed, eventually, and it may actually be possible once we 
re-organize the schema. As such, I'll REOPEN the bug.

Comment 17

[Frédéric Buclin]

I agree with travis, we should have a param for that. travis, I retargetted this
bug to 2.20 as I don't think we will change the schema in 2.18. ;)

Comment 18

[Shane H. W. Travis]

I'm actually going to 'undefine' it and mark it as Future, I'd be very 
surprised if someone could get a change of this magnitude necessary to make 
this possible designed, agreed-upon, coded, reviewed, and approved before 
freeze takes place in 5 weeks.

I'm also taking my name off it; I'm not working on it any more, so if someone 
has the drive to push on with it, go for it.

Comment 19

[Frédéric Buclin]

*** Bug 194028 has been marked as a duplicate of this bug. ***

Comment 21

[Filipe Brandenburger]

Attachment: Patch against Bugzilla 3.0.4

I have created a patch to address this issue. The patch creates the
"commentonworkedtime" variable that will permit this behaviour to be
configurable. The variable's default is set to "True" (which is the current
behaviour), but after adding the code and even after running checksetup.pl ir
continued to be false in my setup, maybe there's the need to add some code to
update_params() to set it to true by default. Anyway, feel free to change the
implementation if you feel it should be different than what I'm suggesting.

Thanks,
Filipe

Comment 22

[Rune Forberg]

I found that after installing this patch values entered in the 'Add' box in team planning are not registered in bugzilla, it just disables the warning. 

Instead i removed this added code in the if statement from the patch in process_bug.cgi:

[...] Bugzilla->params->{'commentonworkedtime'} && [...]

and disabled the warning and inserted an auto generated comment instead, we use this in out team so we don't have to write comments every time we add worked hours for a bug. 

#DISABLE WARNING
#ThrowUserError('comment_required');
#Add autogenerated comment
$cgi->param('comment', "Hours worked: $wk_time (autogenerated comment)");




 


if (Bugzilla->params->{'commentonworkedtime'} &&

Comment 23

[Rune Forberg]

NOTE: that last sentence in the previous comment should not be there. sorry..

Comment 25

[Kent Rogers [:mrbball]]

I don't see the need for another parameter.  The only time comments can
currently be configured to be required or not are on status/resolution
changes, but no other field changess.  Thus, not adding
'commentontimetracking' parameter does not seem inconsistent, and adding
one seems unnecessary and undesirable IMO.

In bug 555975, I recommended simply getting rid of the 'comment_required'
error in Bug::add_comment() if the work_time is non-zero and the comment
text is empty.  Are there still places in the code that depend on having
a non-blank comment?  It doesn't seem so.  I just found dozens of bugs in
my installation that have had blank comments for years, without issues.

There is the odd case where if a user is not in the timetracking group,
a blank comment with hours worked shows up with empty text (i.e. without
the canned "Additional hours worked: XXX").  That obviously looks awkward,
but that comment could be treated in the same way that private comments
are treated for non-insiders.  That is, it isn't displayed it at all.

Feedback?

Comment 26

[Kent Rogers [:mrbball]]

Attachment: Eliminate error if no comment included when adding Hours Worked

This patch simply gets rid of throwing an error if no comment is added
when Hours Worked are updated (no parameter).  I don't see any reason
why empty text would be a problem in this case.  The only situation I
saw that needed to be considered was if Hours Worked was the only thing
that changed, but the user was not a member of the timetracking group.
I think the comment should simply be skipped (as if it were private and
the user was not an insider) in that case.

Please let me know what you think.

Comment 27

[Max Kanat-Alexander]

Comment on attachment 438782 [details] [diff] [review]
Eliminate error if no comment included when adding Hours Worked

>+    if ($comment eq '' && !($params->{type} || $params->{work_time} != 0)) {

  You don't need to make that change, and also shouldn't, because work_time can be undef.

  Otherwise, this looks reasonable to me.

Comment 28

[Kent Rogers [:mrbball]]

(In reply to comment #27)
> (From update of attachment 438782 [details] [diff] [review])
> >+    if ($comment eq '' && !($params->{type} || $params->{work_time} != 0)) {
> 
>   You don't need to make that change, and also shouldn't, because work_time can
> be undef.
> 
You're right, work_time could be undef and I should have checked for
that.  But, if you do not also do a numerical comparison here, it is
possible to get an empty comment by setting Hours Worked to '0.0' or
'-0', or something similar.

Comment 29

[Max Kanat-Alexander]

(In reply to comment #28)
> You're right, work_time could be undef and I should have checked for
> that.  But, if you do not also do a numerical comparison here, it is
> possible to get an empty comment by setting Hours Worked to '0.0' or
> '-0', or something similar.

  Oh, in that case we should be checking abs(work_time or 0) or something like that. Can I get a new patch that does that?

Comment 30

[Kent Rogers [:mrbball]]

Attachment: v2

It doesn't look like there is a need to check for existence of
$params->{work_time} before using abs().  And if there were, it is handled
by _check_work_time() and it seems like _check_work_time() should be called
in add_comment(), even if $params->{work_time} isn't defined.  I'd say the
same for the calls to _check_comment_type() and _check_commentprivacy() too.
I don't see the need to check for existence of the parameter before calling
those functions.

Comment 31

[Max Kanat-Alexander]

Comment on attachment 441852 [details] [diff] [review]
v2

On Perl 5.12, I suspect that your call to abs() with an undefined variable will throw a warning. (Don't check it by using the Bugzilla UI--check it by either using the WebService or direct code.)

Comment 32

[Kent Rogers [:mrbball]]

Attachment: v3

Ok.  I think simply calling _check_work_time(), regardless of whether
$params->{work_time} is defined or not, is the solution.  If not
defined, the following line in _check_time() will set it to 0:

    $time = trim($time) || 0;

Comment 33

[Max Kanat-Alexander]

Comment on attachment 444079 [details] [diff] [review]
v3

Yeah, good idea! :-) Looks good. :-)

Comment 34

[Frédéric Buclin]

Comment on attachment 444079 [details] [diff] [review]
v3

>=== modified file 'template/en/default/bug/comments.html.tmpl'

>+  [% RETURN IF timetracking_only AND !user.in_group(Param('timetrackinggroup')) %]

Replace !user.in_group(Param('timetrackinggroup')) by !user.is_timetracker on checkin, or attach another patch with this change.

Comment 35

[Max Kanat-Alexander]

Thanks for the patch, Kent! You're fast becoming one of our top 3.8 contributors! :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified template/en/default/bug/comments.html.tmpl
Committed revision 7159.

Comment 36

[Max Kanat-Alexander]

Oh, and the checkin fix. :-)

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified template/en/default/bug/comments.html.tmpl
Committed revision 7160.

Comment 37

[Frédéric Buclin]

Comment on attachment 444079 [details] [diff] [review]
v3

This patch completely breaks comments. They are no longer displayed.

Comment 38

[Frédéric Buclin]

I backed out the patch, see bug 565794.

Comment 39

[Kent Rogers [:mrbball]]

Attachment: v4

My apologies.

I have no idea what happened.  Looking at it now, the changes to
comments.html.tmpl don't make sense to me at all.  I spent some time
trying to figure out what happened, but couldn't come up with anything.
There have not been any major changes in that file since I created the
patch, and my local installed version (3.4.6) is not that different.
I'm at a loss.

The idea was that if a comment ONLY contained hours worked (and no text),
it should be completely skipped for non-timetrackers.  A user that is not
logged in is obviously not in the timetracking group, and this version of
the patch correctly skips timetracking-only comments in that case.  I've
tested it in that case and also tested it when a user is logged in but
not in the timetracking group.

Comment 40

[Frédéric Buclin]

(In reply to comment #39)
> There have not been any major changes in that file since I created the
> patch, and my local installed version (3.4.6) is not that different.
> I'm at a loss.

Hum, you have to created your patch against Bugzilla 3.7, not 3.4.6. One major change is that comment are now objects.

Comment 41

[Kent Rogers [:mrbball]]

Yes I created the patch against 3.7, but the structure of the template
is not that different from 3.4.6.

I suppose it's possible having multiple trees going somehow caused
confusion, but even in the context of 3.4.6, the two lines I added to
comments.html.tmpl don't make sense (and don't work).

I'm sure there's an explanation for what happened, but I'm not going to
spend any more time trying to figure it out.  I'm sure it boils down to
just too many things going at once ....

Comment 42

[Max Kanat-Alexander]

Comment on attachment 445320 [details] [diff] [review]
v4

Tested, and it works just fine, both for users in and out of the timetrackinggroup. :-) Thanks!

Comment 43

[Max Kanat-Alexander]

Committing to: bzr+ssh://bzr.mozilla.org/bugzilla/trunk/
modified Bugzilla/Bug.pm
modified template/en/default/bug/comments.html.tmpl
Committed revision 7198.

Comment 44

[Max Kanat-Alexander]

Added to the release notes in bug 604256.