Closing milestone and retargeting tickets to another does not show in change history

[mlists at bigrideau.com]

When I close a milestone and select "retarget open tickets" the tickets are successfully retargeted, however the change in milestone is not reflected in the ticket change history.

[Christian Boos]

Right, this would be a nice thing to have. However, the design is not trivial if we don't want to duplicate the change metadata everywhere.

Related to #1890 (change metadata) and #525 (batch modifications).

[anonymous]

Pardon my ignorance, but given that a milestone change on its own is reflected in the ticket history could this same api not be called when a ticket is retargeted?

[Christian Boos]

As I said, it's not trivial if we don't want to duplicate the metadata. Conversely, it is trivial if we decide to duplicate the change information everywhere…

The real problem is that doing a big bunch of inserts (one entry recording the change metadata, either directly or indirectly a la #1890) will likely expose the milestone retarget operation to locking issues (see #3446).

[Emmanuel Blot]

Replying to cboos:

The real problem is that doing a big bunch of inserts (one entry recording the change metadata, either directly or indirectly a la #1890) will likely expose the milestone retarget operation to locking issues (see #3446).

Would it be possible, as a workaround, to acquire and release a DB lock for each updated ticket? Retargetting ticket is an operation that occurs only a few times so a slow execution is not a big issue IMHO. OTOH, managing metadata may not be implemented in the next months… so a temp. workaround may be a useful solution.

[mlists at bigrideau.com]

Has my vote. You are correct that at least in our usage model this would happen at most once per week so speed is not of the essence.

[Christian Boos]

#5897 mentions the same problem but for milestone renames instead of retarget after close.

[mape <th07@…>]

This kind of silent changes really confuses up my ​TimeVisualizerPlugin, which draws burndown graphs from ticket, ticket_custom and especially from ticket_change. If history is not properly written, the graph gets messed and can't be reliably used to trace projects (scrum burndown graph).

I'm quite new to trac. Is there any other cases when ticket_change gets outdated? Quicly I could name these:

• retarget open tickets when closing milestone #5658
• rename milestone #5897
• my guess: using webadmin to change component or milestone names

It would feasible to minimize db altering with SQL and use APIs whenever possible. And then APIs should respect history (as proposed in ​TracObjectModelProposal. I strongly support idea of having unique (and constant) id for objects as proposed in ​DataModel, which would fix renaming issues.

I have now a bad feeling that it is too easy mess up my graph with trac (or 3rd components). Maybe the original idea of having ticket listener and writing own log would have been better - even it has many other drawbacks. Of course ticket listener itself would not be enough - I would need also milestone and component listeners…

[Christian Boos]

#7615 was closed as duplicate.

One of the issue here is how to store the change efficiently for all the tickets (see related discussion on this topic in #1890). Same problematic applies to #525 as well.

Also I think that adding a fixed comment like "milestone was renamed" or "retargeted after closing milestone", depending on the situation would be nice. In the future, this might even contain a link to the milestone version which triggered the change (for now milestone data is unversioned).

[Remy Blank]

See #4582 for the same issue when renaming a milestone.

[Peter Suter]

#10925 was closed as a duplicate, and includes some patches.

[Ryan J Ollos <ryan.j.ollos@…>]

Patch against r11413 of the trunk.

[Ryan J Ollos <ryan.j.ollos@…>]

The attached t5658-r11413-1.patch​ incorporates Pete's feedback from comment:5:ticket:10925.

• The timeline is batch updated:

• A batch email notification is sent.
• I found the wording of the batch email notification to be a bit confusing: milestone to milestone2, thinking this meant the old value was milestone and the new value was milestone2. So, I changed this to read milestone changed to milestone2.
• I've deferred the permission checks described in comment:2:ticket:10925, but would gladly incorporate them into a new patch if others believe they are worthwhile.

[Remy Blank]

I like the idea of requiring TICKET_BATCH_MODIFY for this.

Another idea, instead of hard-coding the comment associated with the change, how about providing an edit box for the user to specify the comment?

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to rblank:

I like the idea of requiring TICKET_BATCH_MODIFY for this.

Thinking forward, would we also require TICKET_BATCH_MODIFY for editing the Milestone when implementing #4582 and presumably calling Ticket.save_changes here?

Another idea, instead of hard-coding the comment associated with the change, how about providing an edit box for the user to specify the comment?

Should we provide a default comment like the one we have now?

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

Replying to rblank:

I like the idea of requiring TICKET_BATCH_MODIFY for this.

Thinking forward, would we also require TICKET_BATCH_MODIFY for editing the Milestone when implementing #4582 and presumably calling Ticket.save_changes here?

On closer look, I think this doesn't matter since TICKET_ADMIN is required for accessing the webadmin panels, and TICKET_ADMIN grants TICKET_BATCH_MODIFY.

[Remy Blank]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

Should we provide a default comment like the one we have now?

I wouldn't, but I don't have a strong opinion on that. Let's hear what others think (also about the edit box, that was just an idea I threw in for discussion).

[Christian Boos]

Replying to rblank:

Replying to Ryan J Ollos <ryan.j.ollos@…>:

Should we provide a default comment like the one we have now?

I wouldn't, but I don't have a strong opinion on that. Let's hear what others think (also about the edit box, that was just an idea I threw in for discussion).

I think the idea of an edit box is nice, but if we don't give a default message, it has good chances to stay empty… That wouldn't be that bad if the message wasn't very informative, but as here we can do better ("retargeted after close" vs. "milestone renamed") I think it's a good thing to provide it.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to cboos:

I think the idea of an edit box is nice, but if we don't give a default message, it has good chances to stay empty…

That was my line of thinking as well. I'll refresh the patch tomorrow.

[Ryan J Ollos <ryan.j.ollos@…>]

Replying to Ryan J Ollos <ryan.j.ollos@…>:

That was my line of thinking as well. I'll refresh the patch tomorrow.

Didn't reach my goal of returning to this "tomorrow", but I'll resume work on it shortly.

[Ryan J Ollos <ryan.j.ollos@…>]

​e82651ef builds on t5658-r11413-1.patch​ to add a comment edit box. I couldn't see a way to implement the functional tests without adding a name attribute to several elements.

​b8fa8b95 attempts to eliminate some redundant code by utilizing the BatchModify class in a way that can be reused in #4582 and elsewhere. I'll await feedback on this patch, but when the patch is finalized I propose to also add the retarget functionality to the milestone admin page. Once the initial patch is finalized, it will be trivial to replicate the functionality on that page.

[Christian Boos]

Looks good! Sorry for the late review, as soon as the 1.x.1 releases are out, we'll be able to get more stuff in ;-)

[Ryan J Ollos <ryan.j.ollos@…>]

No problem. Once this patch gets applied I'll move on to #4582.

[Ryan J Ollos <ryan.j.ollos@…>]

#7163 may be an example of the defect that this ticket intends to resolve. The milestone for #7163 is currently next-minor-0.12.x, but the last ticket change entry shown in the ticket comments is Milestone set to 0.11.1.

[Ryan J Ollos]

Committed to 1.0-stable in [11832]. I had separated out the fix for the change history and the addition of the retarget comment in my local git branch, but I guess I messed up the commit history before I pushed the change.

[Ryan J Ollos]

There was an issue that I meant to ask about before committing. I'm calling a private method _save_ticket_changes of the BatchModifyModule class, which is not a good practice. In the patch I had added a public function save_ticket_changes, but I wasn't sure that is the best way to deal with this. I could add a save_ticket_changes method, but it would just directly call _save_ticket_changes. Any thoughts on what should be added to the public interface?

[Jun Omae]

r11832 is…

• I don't think that we should use id="comment" for comment field cause ticket.css has #comment { width: 100% } and label[for=comment] { float: right }. Some wiki macros may add the stylesheet.
• class="trac-resizable" for the text field? The class works for only textarea.

[Ryan J Ollos]

Thanks, I'll fix those issues. I should have pushed the patch up for review first. I'm also questioning whether it is good to utilize BatchModifyModule._save_ticket_changes at all.

[Jun Omae]

Replying to rjollos:

In the patch I had added a public function save_ticket_changes, but I wasn't sure that is the best way to deal with this. I could add a save_ticket_changes method, but it would just directly call _save_ticket_changes. Any thoughts on what should be added to the public interface?

Sounds good to be the public interface. However, I think it should be Ticket.save_changes.

[Ryan J Ollos]

Replying to jomae:

Sounds good to be the public interface. However, I think it should be Ticket.save_changes.

Are you suggesting a static method for the Ticket class? I think the signature would need to be something like save_changes(ids, new_values, authname, comment, action). Would Ticket.batch_modify be better?

I'm also considering that it would be better to have a Milestone.move_tickets method rather than MilestoneModule._retarget_tickets. Milestone.move_tickets could be reused in Milestone.delete​. Maybe move is not a good verb though, is retarget better?

[Ryan J Ollos]

I guess that you are suggesting that I just directly call Ticket.save_changes. What I had in mind is a function or method that takes a list of ticket ids, calls Ticket.save_changes and sends a batch notification email.

[Jun Omae]

Replying to rjollos:

Are you suggesting a static method for the Ticket class? I think the signature would need to be something like save_changes(ids, new_values, authname, comment, action). Would Ticket.batch_modify be better?

Ideally, it'd be made independent from req. However, TicketSystem(self.env).action_controllers requires an instance of Request.

At least, _save_ticket_changes depends on batch-modify form in query view. I don't think the method should depend on the form and retrieve values from req.args. The values should be pass as arguments.

My POC, [e10010da/jomae.git], made it independent from the form, added optional when argument and removed the email notification from it.

I'm also considering that it would be better to have a Milestone.move_tickets method rather than MilestoneModule._retarget_tickets. Milestone.move_tickets could be reused in Milestone.delete​. Maybe move is not a good verb though, is retarget better?

IMO, Milestone.move_tickets sounds good.

[Ryan J Ollos]

Replying to jomae:

At least, _save_ticket_changes depends on batch-modify form in query view. I don't think the method should depend on the form and retrieve values from req.args. The values should be pass as arguments.

I like your approach. I'll test it out today and prepare the other changes related to this ticket.

[Ryan J Ollos]

Sorry for the delay on this one. I haven't forgotten about it, and plan to work on it within the next few days.

[Ryan J Ollos]

The changes I've made so far can be found in repos:rjollos.git:t5658. I wanted to post the changes early on, as well as some additional thoughts, to elicit feedback.

I like the changes to BatchModifyModule that Jun showed in comment:34 and I think they'll be useful in general, but I've ended up taking a different approach, and I think I misled myself early by thinking I needed to utilize BatchModifyModule. The approach is to extract the "move tickets" code from Milestone.delete​ to a method move_tickets, and eventually replace the call to MilestoneModule._retarget_tickets with Milestone.move_tickets. It seems simpler, and avoids having a dependency on BatchModifyModule in roadmap.py or model.py.

Regarding [11832#file2], I've found that for Twill's submit command, if there is only one submit button on the form, then it's not necessary to specify the name of the submit button. I've also found that Twill will find a submit element by its id attribute. I've since removed the name attributes that were added in [11832#file2] since they only result in more noise in req.args, e.g. req.args['delete'] = 'Delete milestone'. The id attributes could have been added, but in this case there is only one submit button per form.

The two functional tests I've added are named TestMilestoneDelete and TestMilestoneDeleteRetargetTickets rather than RegressionTestTicket5658a, RegressionTestTicket5658b, … For fundamental behavior like these tests cover, it seems better to make them easily located in the file. I was having trouble determining if we had any functional test coverage for the milestone module. I'm considering renaming RegressionTestTicket5658 → TestMilestoneCloseRetargetTickets.

My next steps are:

• MilestoneModule._do_delete in roadmap.py should send a batch notification when tickets are retargeted.
• The _retarget_tickets method can be removed from MilestoneModule and Milestone.move_tickets can be called in it's place. After move_tickets is called, a batch notification email should be sent.
• It seems best to just included a fix for #4582 here as well.
• The same issues, including notification emails and notices, should be considered for milestones closed, deleted and renamed from the Milestone Admin panel.

[Ryan J Ollos]

The Milestone edit and Milestone delete pages have a retarget checkbox and corresponding target select. For the case of Milestone edit, you can choose to close the milestone and leave open tickets associated with the milestone. For the case of Milestone delete, it doesn't make much sense to leave tickets associated with a non-existent milestone, and the behavior is that, if the user unselects the retarget checkbox or leaves the default of None, the tickets will be retargeted to the None milestone. If retarget is unchecked but an entry other than None is shown in the disabled target select, the tickets will still be retargeted to the None milestone.

All of the behavior I've described so far seems correct and good to me.

Now that a comment is being added to retargeted tickets, we'll have the somewhat odd situation that the user may have unselected the retarget checkbox, but all of the associated tickets have been moved to the None milestone and have a "retargeted" comment.

My conclusion is, we probably don't need the retarget checkbox since the tickets will always be retargeted somewhere. Rather, we could just continue to have None as the default, remove the checkbox, and and it will be clear to the user that the tickets will always be retargeted somewhere.

On a somewhat related note, for both Milestone edit and Milestone delete, I propose to only show the retarget option if, in the case of Milestone edit there are open tickets, and in the case of Milestone delete if there are any associated tickets. I think this will be helpful to the user, since we won't be proposing actions that won't be taken on any tickets. This change probably deserves its own ticket, so I'll probably wait until this ticket is closed out.

[Peter Suter]

Removing the retarget checkbox (and the entire option if there are no tickets to retarget) makes sense to me.

Any idea why there was a # don't translate comment here​? (Added in [10185].)

[Christian Boos]

Replying to psuter:

Any idea why there was a # don't translate comment here​? (Added in [10185].)

This string is not part of the localized user interface, rather a description for the change which is going to be stored in the ticket's comment, and we avoid to store localized content in the database. English is the "neutral" choice here. Note that when possible we even avoid English! (e.g. a comment for a wiki rename says "PageA → PageB"). We could imagine having this translated for the situation in which a Trac site is only ever used by speakers of the same language, but we don't currently have a setting for expressing this.

[Peter Suter]

Replying to cboos:

we avoid to store localized content in the database

Ah, right of course. In other words, just because the current user localizes his UI to German doesn't mean other users want to see a German ticket comment.

I guess that also goes for the default of the user editable field (comment:17).

[Ryan J Ollos]

Thanks, I had meant to ask about the don't translate, and I should ask before changing!

I removed the translation from both comment strings in [b5d6b8a1/rjollos.git], and removed the checkbox from the Milestone delete page in [66c34bbf/rjollos.git]. I'll proceed now with the remainder of the changes.

[Jun Omae]

The comment in [b5d6b8a1/rjollos.git], <!-- Don't translate ticket comment (comment:40:ticket:5658) -->, I would like to be <!--! ... --> as Genshi template comment. The latter is not rendered in output.

[Ryan J Ollos]

Replying to jomae:

r11832 is…

• I don't think that we should use id="comment" for comment field cause ticket.css has #comment { width: 100% } and label[for=comment] { float: right }. Some wiki macros may add the stylesheet.
• class="trac-resizable" for the text field? The class works for only textarea.

The fixes for those two issues have been committed in [12022:12023].

[Ryan J Ollos]

Starting back at this one with a few questions so far.

1. In the following code, is there any reason to have self.env.log inside the transaction, or is it better to keep it outside the transaction?

   with self.env.db_transaction as db:
       self.env.log.info("Deleting milestone %s", self.name)
       db("DELETE FROM milestone WHERE name=%s", (self.name,))
   

2. I've been trying to figure out what to do with code blocks like this, since they are repeated in so many places. Maybe we could make a decorator that handles the try / except, log and add_warning. I haven't tried yet, and I haven't written many decorators, so I thought throw out the idea before taking a try at it.

   tn = BatchTicketNotifyEmail(self.env)
   try:
       tn.notify(tkt_ids, new_values, comment, action, req.authname)
   except Exception, e:
       self.log.error("Failure sending notification on ticket batch"
                      "change: %s", exception_to_unicode(e))
       add_warning(req, tag_("The changes have been saved, but an "
                             "error occurred while sending "
                             "notifications: %(message)s",
                             message=to_unicode(e)))
   

[Ryan J Ollos]

Replying to rjollos:

Maybe we could make a decorator that handles the try / except, log and add_warning. I haven't tried yet, and I haven't written many decorators, so I thought throw out the idea before taking a try at it.

Hmmm … that's not what I wanted to say. I was thinking of context manager, or something like that, but I don't have it straight in my mind how it would work. Than again, maybe I just need to wrap the code in a function.

[Ryan J Ollos]

Replying to rjollos:

On a somewhat related note, for both Milestone edit and Milestone delete, I propose to only show the retarget option if, in the case of Milestone edit there are open tickets, and in the case of Milestone delete if there are any associated tickets. I think this will be helpful to the user, since we won't be proposing actions that won't be taken on any tickets. This change probably deserves its own ticket, so I'll probably wait until this ticket is closed out.

Ticket #11336 was created for this issue.

[Ryan J Ollos]

Latest proposed changes to resolve #4582 and #5658 can be found in log:rjollos.git:t5658.2. I'll list some decisions that I've made along the way; let me know if you think any of these things should be handled differently.

• In order to send the batch modification ticket notification, a list of retargeted ticket ids is needed. So that Milestone.delete does not have to return a list of retargeted ticket ids, I moved the retarget operation out of delete. Returning a list of retargeted ticket ids is not something I'd expect the Milestone model to be doing, and it feels like it would be cluttering the API and mixing functionality into the model that doesn't belong there. It is now handled in the roadmap module, by calling Milestone.move_tickets. I've deprecated the retarget_to parameter of Milestone.delete. Please review and let me know if it makes sense to deprecate and eventually remove this parameter.
• Notifications are sent when tickets are retargeted in response to a milestone being closed or deleted, but not when a milestone is renamed from the milestone edit page, the milestone admin page or through trac-admin.
• When tickets are retargeted to the None milestone on milestone close, the milestone is set to null. This is consistent with how the milestone is set when retargeting tickets on milestone delete. See discussion in #11018.
• When deleting a milestone from the Milestone Admin page, the ticket comment will be "Ticket retargeted after milestone deleted". The ticket is retargeted to the "None" milestone, because the user isn't presented with a choice. This will be dealt with in #3754. The same situation exists for milestones deleted through trac-admin, which will be dealt with in #11343.

[Ryan J Ollos]

Committed to 1.0-stable in [12186:12195]. Merged [11832,12022:12023,12186:12195] to trunk in [12196].

The commit message for [12189] is wrong, and should be:

1.0.2dev: Add notice that tickets have been retargeted when a milestone is deleted.

[Jun Omae]

I got the following failure on TestMilestoneDelete.

======================================================================
FAIL: Delete a milestone and verify that tickets are retargeted
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/jun66j5/src/trac.git/trac/ticket/tests/functional.py", line 1343, in runTest
    delete_milestone(name, tid=tid)
  File "/home/jun66j5/src/trac.git/trac/ticket/tests/functional.py", line 1319, in delete_milestone
    tc.find('Changed[ \t\n]+<a.*seconds ago</a>[ \t\n]+by admin')
  File "/home/jun66j5/src/trac.git/trac/tests/functional/better_twill.py", line 217, in better_find
    raise twill.errors.TwillAssertionError(*args)
TwillAssertionError: ("no match to 'Changed[ \t\n]+<a.*seconds ago</a>[ \t\n]+by admin'", '/home/jun66j5/src/trac.git/testenv/trac/log/TestMilestoneDelete.html', '/home/jun66j5/src/trac.git/testenv/trac/log/TestMilestoneDelete.html')

----------------------------------------------------------------------
Ran 166 tests in 468.197s

FAILED (failures=1)
make: *** [functional-test] Error 1

The test failed on 1 second ago in testenv/trac/log/TestMilestoneDelete.html.

        Changed <a class="timeline" href="/timeline?from=2013-10-28T11%3A59%3A44%2B09%3A00&amp;precision=second" title="See timeline at Oct 28, 2013 11:59:44 AM">1 second ago</a> by admin

I think that TestMilestoneRename has the same issue.

[Ryan J Ollos]

I guess the tests just run slow on my system since I've run them repeatedly and never encountered this error. How about the following fix?

trac/ticket/tests/functional.py

@@ -1316 +1316 @@
             if tid is not None:
                 tc.find(retarget_notice)
                 self._tester.go_to_ticket(tid)
-                tc.find('Changed[ \t\n]+<a.*seconds ago</a>[ \t\n]+by admin')
+                tc.find('Changed[ \t\n]+<a .*>\d+ seconds? ago</a>'
+                        '[ \t\n]+by admin')
                 if retarget_to is not None:
                     tc.find('<a class="milestone" href="/milestone/%(name)s">'
                             '%(name)s</a>' % {'name': retarget_to})
@@ -1385 +1386 @@
         tc.find("Your changes have been saved.")
         tc.find(r"<h1>Milestone %s</h1>" % new_name)
         self._tester.go_to_ticket(tid)
-        tc.find('Changed[ \t\n]+<a.*seconds ago</a>[ \t\n]+by admin')
+        tc.find('Changed[ \t\n]+<a .*>\d+ seconds? ago</a>[ \t\n]+by admin')
         tc.find('<a class="milestone" href="/milestone/%(name)s">'
                 '%(name)s</a>' % {'name': new_name})
         tc.find('<strong class="trac-field-milestone">Milestone</strong>'

I've tested the regex in the Python shell but can't confirm that it fixes the issue since the tests run too slowly for me.

[Jun Omae]

Ok. The tests pass. Thanks!

The issue is rare on my system, also. I temporarily add the following then the issue happens at almost 100% on slow machine.

trac/ticket/tests/functional.py

@@ -1303 +1303 @@
             tc.submit(formname='deletemilestone')
             if retarget_to is not None:
                 tc.formvalue('edit', 'target', retarget_to)
+            t = time.time()
+            time.sleep(t % 1.0)
+            t = time.time()
             tc.submit('delete', formname='edit')
 
+            t = time.time() - t
+            if 0.0 < t < 1.0:
+                time.sleep(t % 1.0)
             tc.url(self._tester.url + '/roadmap')
             tc.find('The milestone "%s" has been deleted.' % name)
             tc.notfind('Milestone:.*%s' % name)

[Ryan J Ollos]

Oh, I see. So the text most likely reads Changed 0 seconds ago when the tests pass on my system; I had been thinking it read Changed 2 seconds ago when I assumed that the tests were running too slowly on my system.

[Ryan J Ollos]

Fixed on 1.0-stable in [12218] and merged to trunk in [12219].

[t2y <tetsuya.morimoto@…>]

I'm using r12218 in the 1.0-stable branch. All the tickets (including closed tickets) have changed to the retargeting milestone when I used this feature. I think that the closed tickets shouldn't change to. Is this a bug?

[Jun Omae]

Good catch! That's definitely a bug. Thanks.

[Ryan J Ollos]

Proposed fix can be found in log:rjollos.git:t5658.3.

[Jun Omae]

Verified t5658.3 branch. It works well.

But I think we should add unit tests for Milestone.move_tickets and creating changelog of tickets on renaming and deleting milestone, see [22d1e60e/jomae.git].

[Ryan J Ollos]

While studying your tests I saw two possible improvements for move_tickets:

• The comment argument could default to an empty string, comment=''.
• Check that new_milestone exists.

These would not necessarily affect our use case for move_tickets, but would make the public method more robust for other users (such as plugins). Should I go ahead and implement those changes?

[Jun Omae]

That makes sense. Please go ahead ;)

[Ryan J Ollos]

Additional changes have been added to log:rjollos.git:t5658.3.

[Jun Omae]

[a5a0fa15/rjollos.git]: I don't think we should change test_delete_milestone_retarget_tickets because it tests Milestone.delete with retarget_to and the parameter is still available. Also, we should add another test for retarget feature if needed.

[Ryan J Ollos]

I'll revert [a5a0fa15/rjollos.git] before committing the changes.

[Ryan J Ollos]

Changes committed to 1.0-stable in [12263:12265] and merged to trunk in [12266].

[t2y <tetsuya.morimoto@…>]

I confirmed the closed tickets don't retargeted with [12265] in my environment. Thank you!

[Jun Omae]

I missed that the following failures on Windows caused by milliseconds resolution time. The resolution of Windows timer is 10 milliseconds to 16 milliseconds, ​http://msdn.microsoft.com/en-us/library/windows/desktop/ms725496%28v=vs.85%29.aspx.

...
======================================================================
ERROR: test_rename_milestone_retarget_tickets (trac.ticket.tests.model.MilestoneTestCase)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "trac\ticket\tests\model.py", line 1058, in test_rename_milestone_retarget_tickets
    milestone.update()
  File "trac\ticket\model.py", line 1063, in update
    self.move_tickets(self.name, author, "Milestone renamed")
  File "trac\ticket\model.py", line 1117, in move_tickets
    ticket.save_changes(author, comment, now)
  File "trac\ticket\model.py", line 358, in save_changes
    """, (self.id, when_ts, author, cnum, comment))
  File "trac\db\util.py", line 121, in execute
    cursor.execute(query, params)
  File "trac\db\util.py", line 54, in execute
    r = self.cursor.execute(sql_escape_percent(sql), args)
  File "trac\db\sqlite_backend.py", line 78, in execute
    result = PyFormatCursor.execute(self, *args)
  File "trac\db\sqlite_backend.py", line 56, in execute
    args or [])
  File "trac\db\sqlite_backend.py", line 48, in _rollback_on_error
    return function(self, *args, **kwargs)
IntegrityError: columns ticket, time, field are not unique

----------------------------------------------------------------------
Ran 1382 tests in 104.015s

FAILED (errors=4)

Proposed fix can be found in [591602fe/jomae.git] using when argument of Ticket.insert and save_changes methods.

[Ryan J Ollos]

It looks like #11301 is discussing a fix for the same issue encountered in a different test case. Your fix looks okay, it appears that we just need to be careful in the future not to call _update_ticket in succession for the same ticket when writing additional tests.

[Jun Omae]

Thanks for your comment. The changes have been committed in [12272] and merged into trunk in [12273].

[Ryan J Ollos]

Documentation string fixed and parameters scheduled for removal in [13059], merged in [13060].

[Ryan J Ollos]

In [13622]: Milestone.move_tickets() is used when deleting milestone from the admin page. The deprecated author parameter of Milestone.delete() is no longer used. Merged to trunk in [13623].

[Jun Omae]

I noticed that batch ticket notification is not sent when deleting milestone from admin panel on Trac 1.0.15. Is that intentional? Do we have plan to send the notification?

[Ryan J Ollos]

There's still some work to do to make Milestone admin equivalent to the Milestone module. The batch ticket notification could be added in #3754.