In the bug_update.php file, the note type is not getting set or sent to the bugnote_add function. So even if a comment is made the reports will likely not be correct because the note type is defaulted to a normal bugnote rather than a time tracking note.

In addition, when the status is changed to closed or resolved the bug_close and bug_resolve functions from the bug_api call bugnote_add. Neither function assigns the note_type or checks for time tracking information. If the text is empty, it skips the add note.

I would think the check for empty text and time tracking information should happen in the bugnote_add function so that it doesn't have to be duplicated in five different places.

There is also a note in the code regarding whether or not time tracking hours should be allowed with a blank comment. My opinion is that this should be configurable. This is a fairly simple fix and I would be happy to submit a patch for it.

Anyone else have a preference as to whether this should be fixed in bugnote_add or if each place that calls bugnote_add should perform the checks first?

In bug_update.php,

Replace:
if ( ( !is_blank( $f_bugnote_text ) ) && ( false == $t_bug_note_set ) ) {

With:
if ( ( !is_blank( $f_bugnote_text ) || !is_blank( $f_time_tracking ) ) && ( false == $t_bug_note_set ) ) {

Posted to mantis dev list on Friday 10-19-07:

The bug was resolved with a fix to bug_update.php. There are three
additional places where this bug exists. The bug_resolve, bug_close,
and bug_reopen functions in the bug_api.php file call bugnote_add
directly, check for empty note text but do not check for time tracking.
So, if there is no note on a bug_resolve, bug_close, or bug_reopen but
there is time tracking, the time tracking gets dropped.

The bug_assign function also calls bugnote_add but as far as I can tell
it is not possible to add time tracking where bug_assign would be called
anyway.

Posted to mantis dev list on Friday 10-19-07

Giallu,

Sorry I missed your comment on IRC, I was in a meeting. I would be
willing to provide a patch for this issue. I am interested in a
consensus on how this should be solved. I can duplicate the fix that
was made to bug_update.php ( minus the page specific change ) in each of
the functions, or I can move all of the validation for bugnote_add into
the bugnote_add function and remove the checks everywhere else. My
preference would be to put it in the bugnote_add function but I will
patch according to the Mantis team wishes.

Daryn

Posted by vboctor to dev list on Friday 10-19-07

Hi Daryn,

I don't have the code handy now, but I agree with you that moving such
logic to bugnote_add() is a better approach. The logic should be as
follows:

bugnote_add()

• if note is empty and time is empty, return.
• if time is not empty, set note type to time tracking, otherwise, use
  notetype passed in.
• process as usual.

This way all places that call bugnote_add() can call it without having
to check whether it will end up being a productive call or whether it
will end up being a normal note or a time tracking note.

Again, this is all based on memory, so my response may not be
accurate, but it should convey the approach that I have in mind.
Basically, I would love to reduce the logic in the script pages and
have the APIs provide all such business logic. This also provides
benefit to webservice API that are built directly on top of the core
API, otherwise, we will have to replicate the logic there as well.

Thanks a lot for offering to provide a patch, I am looking foward to review it.

I assume that you should have the access to reopen a resolved issue.
If so, then please re-open the issue and capture this discussion in
there. Then attach your patch when done so that we can review and
commit. I personally prefer patches as a zip file with both the diff
as well as the modified/added files.

Posted by Gianluca to dev list on Saturday 10-20-07.

I agree on the general concept that the logic in pages should be moved
to the underlying API as much as possible.

In this case, I think the main problem is that bugnote_add() should be
used to add, you know... a bugnote (and nothing else).

You should also have a look at a past discussion about time tracking
and bugnotes "types" in general:

http://thread.gmane.org/gmane.comp.bug-tracking.mantis.devel/1312

where I made a proposal that could help (despite I'm sure if you could
analyze the overall design issue, a even better solution could be more
obvious)

thanks for your help

Gianluca

Discussion between daryn and giallu on IRC Monday 10-22-07

[09:41] <daryn> i was looking at the discussion as you suggested
[09:41] <daryn> I noticed that the api has not been changed to place the time tracking param at the end
[09:42] <giallu> no. nothing come out after the discussion there. but moving that at the end was not really a good solution IMHO
[09:43] <daryn> is time tracking just being release in 1.1.0?
[09:43] <giallu> yeah, I think it was not in 1.0
[09:44] <daryn> hm...seems like a big change to move that out so close to release
[09:45] <giallu> well... it entered the repo quite some time ago...
[09:45] <daryn> right. i'm just hesitant to try to fix that when we're almost to rc3 but i can also see that it should be fixed before releasing it.
[09:46] <giallu> yeah. I think it's better if we can fix it ( or try to)
[09:46] <giallu> it's somewhat broken anyway
[09:52] <giallu> I think we should resume some discussion about the feature
[09:53] <giallu> If you have some time to study the code, I think you can come up with something better than what we have now.
[09:54] <daryn> ok. I'll take a look ... see if I come up with anything.

Apply to SVN Revision 4704.
patch -p0 <bug7953.patch from mantis root directory.

Changes to fix Bug 7953:

1. Moved validation checks from pages into bugnote_add function
2. Added configuration option to allow time tracking without note text. Defaulted ON.
3. Moved calls to bugnote_add in bug_resolve, bug_close, and bug_reopen functions before
   bug_set calls in order to prevent inconsistency where time tracking without note text is OFF, user
   submits hours without a note, an error is triggered but bug status is changed anyway.
4. Added error in bugnote_add.php if bugnote_add returns false. Previous versions continued as though
   a note was added when in fact it was not.

Daryn, looks good. Following are some comments:

bugnote_add() -- should send email, can add a flag that controls whether it should be sent or not. This may be useful to inhibit the bugnote addition email if it is part of a bug update or bug reslove. This change makes it easier for SOAP API and other API to reuse the sending of emails (i.e. avoid it being forgotton).

config_defaults_inc - fix indentation, use tabs.
#allow -> # allow

bug_close()/bug_resolve() - fix indentation.

bugnote_add() should this method be using config_get_global() to get table names?

bugnote_add(): adjust indentation, you seem to be using spaces rather than tabs.

Daryn, please also make sure that any change of the interface / semantics of bugnote_add() or whatever else modified core APIs should be modified in the usage by api/soap/*.

Moved checks for adding a bugnote into the bugnote add function.
Added configuration option for allowing adding time tracking
without bugnote text. This defaults to ON. Moved email on adding a bugnote into the bugnote add function with a parameter to specify
whether an email should be sent or not. Defaults to send email. Updated calls to bugnote_add to prevent sending an email where one is already sent. Fixed indentation.