Edgewall Software
Modify

Opened 11 years ago

Closed 5 years ago

Last modified 16 months ago

#5658 closed defect (fixed)

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

Reported by: mlists at bigrideau.com Owned by: Ryan J Ollos
Priority: normal Milestone: 1.0.2
Component: roadmap Version: 0.10.4
Severity: major Keywords: milestone
Cc: th07@…, itamarost@…
Release Notes:

Ticket change history is updated when deleting milestones and when retargeting tickets to another milestone.

API Changes:

Added method move_tickets to the trac.ticket.model:Milestone class.

Description (last modified by Ryan J Ollos)

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.

Attachments (4)

Timeline.png (5.1 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 6 years ago.
t5658-r11413-1.patch (3.6 KB ) - added by Ryan J Ollos <ryan.j.ollos@…> 6 years ago.
Patch against r11413 of the trunk.
MilestoneDelete.png (11.4 KB ) - added by Ryan J Ollos 5 years ago.
TicketRetargeted.png (7.3 KB ) - added by Ryan J Ollos 5 years ago.

Download all attachments as: .zip

Change History (78)

comment:1 Changed 11 years ago by Christian Boos

Description: modified (diff)
Keywords: consider added
Milestone: 0.12

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).

comment:2 Changed 11 years ago by 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?

comment:3 in reply to:  2 ; Changed 11 years ago by 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).

comment:4 in reply to:  3 Changed 11 years ago by 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.

comment:5 Changed 11 years ago by 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.

comment:6 Changed 11 years ago by Christian Boos

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

comment:7 Changed 11 years ago by mape <th07@…>

Cc: th07@… added

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…

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:8 Changed 10 years ago by Christian Boos

Description: modified (diff)
Severity: normalmajor

#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).

comment:9 Changed 8 years ago by Remy Blank

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

comment:10 Changed 8 years ago by Itamar Ostricher

Cc: itamarost@… added

comment:11 Changed 6 years ago by Peter Suter

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

Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: Timeline.png added

Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Attachment: t5658-r11413-1.patch added

Patch against r11413 of the trunk.

comment:12 Changed 6 years ago by 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.

comment:13 Changed 6 years ago by 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?

comment:14 in reply to:  13 ; Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

Cc: ryan.j.ollos@… added

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?

comment:15 in reply to:  14 Changed 6 years ago by 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.

comment:16 in reply to:  14 ; Changed 6 years ago by 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).

comment:17 in reply to:  16 ; Changed 6 years ago by 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.

comment:18 in reply to:  17 ; Changed 6 years ago by 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.

comment:19 in reply to:  18 Changed 6 years ago by 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.

comment:20 Changed 6 years ago by 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.

comment:21 Changed 6 years ago by Christian Boos

Milestone: next-major-releases1.0.2

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 ;-)

comment:22 Changed 6 years ago by Ryan J Ollos <ryan.j.ollos@…>

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

comment:23 Changed 6 years ago by 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.

comment:24 Changed 5 years ago by Ryan J Ollos

Owner: changed from Christopher Lenz to Ryan J Ollos
Status: newassigned

comment:25 Changed 5 years ago by Ryan J Ollos

Cc: ryan.j.ollos@… removed
Keywords: milestone added; consider removed

comment:26 Changed 5 years ago by Ryan J Ollos

Resolution: fixed
Status: assignedclosed

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.

comment:27 Changed 5 years ago by 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?

comment:28 Changed 5 years ago by 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.

comment:29 Changed 5 years ago by 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.

comment:30 Changed 5 years ago by Ryan J Ollos

Resolution: fixed
Status: closedreopened

comment:31 in reply to:  27 ; Changed 5 years ago by 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.

comment:32 in reply to:  31 ; Changed 5 years ago by 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?

comment:33 Changed 5 years ago by 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.

comment:34 in reply to:  32 ; Changed 5 years ago by 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.

comment:35 in reply to:  34 Changed 5 years ago by 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.

comment:36 Changed 5 years ago by 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.

comment:37 Changed 5 years ago by 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 RegressionTestTicket5658TestMilestoneCloseRetargetTickets.

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.

Changed 5 years ago by Ryan J Ollos

Attachment: MilestoneDelete.png added

Changed 5 years ago by Ryan J Ollos

Attachment: TicketRetargeted.png added

comment:38 Changed 5 years ago by 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.

comment:39 Changed 5 years ago by 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].)

comment:40 in reply to:  39 ; Changed 5 years ago by 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.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:41 in reply to:  40 Changed 5 years ago by 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).

comment:42 Changed 5 years ago by 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.

comment:43 Changed 5 years ago by 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.

comment:44 Changed 5 years ago by Ryan J Ollos

Status: reopenedassigned

comment:45 in reply to:  28 Changed 5 years ago by 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].

comment:46 Changed 5 years ago by 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)))
    

comment:47 in reply to:  46 Changed 5 years ago by 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.

comment:48 in reply to:  38 Changed 5 years ago by 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.

comment:49 Changed 5 years ago by 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.
Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:50 Changed 5 years ago by Ryan J Ollos

Description: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

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.

comment:51 Changed 5 years ago by Ryan J Ollos

Release Notes: modified (diff)

comment:52 Changed 5 years ago by 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.

Last edited 5 years ago by Jun Omae (previous) (diff)

comment:53 Changed 5 years ago by 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

    diff --git a/trac/ticket/tests/functional.py b/trac/ticket/tests/functional.py
    index 98f4f21..30407b1 100755
    a b class TestMilestoneDelete(FunctionalTwillTestCaseSetup):  
    13161316            if tid is not None:
    13171317                tc.find(retarget_notice)
    13181318                self._tester.go_to_ticket(tid)
    1319                 tc.find('Changed[ \t\n]+<a.*seconds ago</a>[ \t\n]+by admin')
     1319                tc.find('Changed[ \t\n]+<a .*>\d+ seconds? ago</a>'
     1320                        '[ \t\n]+by admin')
    13201321                if retarget_to is not None:
    13211322                    tc.find('<a class="milestone" href="/milestone/%(name)s">'
    13221323                            '%(name)s</a>' % {'name': retarget_to})
    class TestMilestoneRename(FunctionalTwillTestCaseSetup):  
    13851386        tc.find("Your changes have been saved.")
    13861387        tc.find(r"<h1>Milestone %s</h1>" % new_name)
    13871388        self._tester.go_to_ticket(tid)
    1388         tc.find('Changed[ \t\n]+<a.*seconds ago</a>[ \t\n]+by admin')
     1389        tc.find('Changed[ \t\n]+<a .*>\d+ seconds? ago</a>[ \t\n]+by admin')
    13891390        tc.find('<a class="milestone" href="/milestone/%(name)s">'
    13901391                '%(name)s</a>' % {'name': new_name})
    13911392        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.

Last edited 5 years ago by Ryan J Ollos (previous) (diff)

comment:54 Changed 5 years ago by 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

    diff --git a/trac/ticket/tests/functional.py b/trac/ticket/tests/functional.py
    index 98f4f21..146fa2a 100755
    a b class TestMilestoneDelete(FunctionalTwillTestCaseSetup):  
    13031303            tc.submit(formname='deletemilestone')
    13041304            if retarget_to is not None:
    13051305                tc.formvalue('edit', 'target', retarget_to)
     1306            t = time.time()
     1307            time.sleep(t % 1.0)
     1308            t = time.time()
    13061309            tc.submit('delete', formname='edit')
    13071310
     1311            t = time.time() - t
     1312            if 0.0 < t < 1.0:
     1313                time.sleep(t % 1.0)
    13081314            tc.url(self._tester.url + '/roadmap')
    13091315            tc.find('The milestone "%s" has been deleted.' % name)
    13101316            tc.notfind('Milestone:.*%s' % name)

comment:55 Changed 5 years ago by 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.

comment:56 Changed 5 years ago by Ryan J Ollos

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

comment:57 Changed 5 years ago by t2y <tetsuya.morimoto@…>

Resolution: fixed
Status: closedreopened

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?

comment:58 Changed 5 years ago by Jun Omae

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

comment:59 Changed 5 years ago by Ryan J Ollos

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

comment:60 Changed 5 years ago by 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].

comment:61 Changed 5 years ago by 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?

comment:62 Changed 5 years ago by Jun Omae

That makes sense. Please go ahead ;)

comment:63 Changed 5 years ago by Ryan J Ollos

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

comment:64 Changed 5 years ago by 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.

Last edited 5 years ago by Jun Omae (previous) (diff)

comment:65 Changed 5 years ago by Ryan J Ollos

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

comment:66 Changed 5 years ago by Ryan J Ollos

API Changes: modified (diff)
Resolution: fixed
Status: reopenedclosed

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

comment:67 Changed 5 years ago by t2y <tetsuya.morimoto@…>

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

comment:68 Changed 5 years ago by 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.

comment:69 Changed 5 years ago by 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.

comment:70 Changed 5 years ago by Jun Omae

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

comment:71 Changed 4 years ago by Ryan J Ollos

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

comment:72 Changed 4 years ago by 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].

comment:73 Changed 16 months ago by 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?

comment:74 Changed 16 months ago by 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.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Ryan J Ollos.
The resolution will be deleted.
to The owner will be changed from Ryan J Ollos to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.