Edgewall Software

Opened 17 years ago

Closed 10 years ago

Last modified 7 years ago

#5658 closed defect (fixed)

Closing milestone and retargeting tickets to another does not show in change history — at Version 51

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@… Branch:
Release Notes:

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

API Changes:
Internal Changes:

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.

Change History (55)

comment:1 by Christian Boos, 17 years ago

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 by anonymous, 17 years ago

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?

in reply to:  2 ; comment:3 by Christian Boos, 17 years ago

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

in reply to:  3 comment:4 by Emmanuel Blot, 17 years ago

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 by mlists at bigrideau.com, 17 years ago

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 by Christian Boos, 17 years ago

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

comment:7 by mape <th07@…>, 17 years ago

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 11 years ago by Ryan J Ollos (previous) (diff)

comment:8 by Christian Boos, 16 years ago

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 by Remy Blank, 14 years ago

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

comment:10 by Itamar Ostricher, 13 years ago

Cc: itamarost@… added

comment:11 by Peter Suter, 11 years ago

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

by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Attachment: Timeline.png added

by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

Attachment: t5658-r11413-1.patch added

Patch against r11413 of the trunk.

comment:12 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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 by Remy Blank, 11 years ago

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?

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

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?

in reply to:  14 comment:15 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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.

in reply to:  14 ; comment:16 by Remy Blank, 11 years ago

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

in reply to:  16 ; comment:17 by Christian Boos, 11 years ago

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.

in reply to:  17 ; comment:18 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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.

in reply to:  18 comment:19 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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 by Christian Boos, 11 years ago

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 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

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

comment:23 by Ryan J Ollos <ryan.j.ollos@…>, 11 years ago

#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 by Ryan J Ollos, 11 years ago

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

comment:25 by Ryan J Ollos, 11 years ago

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

comment:26 by Ryan J Ollos, 11 years ago

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 by Ryan J Ollos, 11 years ago

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 by Jun Omae, 11 years ago

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 by Ryan J Ollos, 11 years ago

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 by Ryan J Ollos, 11 years ago

Resolution: fixed
Status: closedreopened

in reply to:  27 ; comment:31 by Jun Omae, 11 years ago

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.

in reply to:  31 ; comment:32 by Ryan J Ollos, 11 years ago

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 by Ryan J Ollos, 11 years ago

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.

in reply to:  32 ; comment:34 by Jun Omae, 11 years ago

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.

in reply to:  34 comment:35 by Ryan J Ollos, 11 years ago

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 by Ryan J Ollos, 11 years ago

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 by Ryan J Ollos, 11 years ago

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.

by Ryan J Ollos, 11 years ago

Attachment: MilestoneDelete.png added

by Ryan J Ollos, 11 years ago

Attachment: TicketRetargeted.png added

comment:38 by Ryan J Ollos, 11 years ago

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 by Peter Suter, 11 years ago

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

in reply to:  39 ; comment:40 by Christian Boos, 11 years ago

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 10 years ago by Ryan J Ollos (previous) (diff)

in reply to:  40 comment:41 by Peter Suter, 11 years ago

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 by Ryan J Ollos, 11 years ago

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 by Jun Omae, 11 years ago

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 by Ryan J Ollos, 11 years ago

Status: reopenedassigned

in reply to:  28 comment:45 by Ryan J Ollos, 11 years ago

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 by Ryan J Ollos, 10 years ago

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

in reply to:  46 comment:47 by Ryan J Ollos, 10 years ago

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.

in reply to:  38 comment:48 by Ryan J Ollos, 10 years ago

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 by Ryan J Ollos, 10 years ago

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 10 years ago by Ryan J Ollos (previous) (diff)

comment:50 by Ryan J Ollos, 10 years ago

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 by Ryan J Ollos, 10 years ago

Release Notes: modified (diff)
Note: See TracTickets for help on using tickets.