#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 )
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 , 17 years ago
Description: | modified (diff) |
---|---|
Keywords: | consider added |
Milestone: | → 0.12 |
follow-up: 3 comment:2 by , 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?
follow-up: 4 comment:3 by , 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).
comment:4 by , 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 , 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 , 17 years ago
#5897 mentions the same problem but for milestone renames instead of retarget after close.
comment:7 by , 17 years ago
Cc: | 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…
comment:8 by , 16 years ago
Description: | modified (diff) |
---|---|
Severity: | normal → major |
#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:10 by , 13 years ago
Cc: | added |
---|
by , 11 years ago
Attachment: | Timeline.png added |
---|
comment:12 by , 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.
follow-up: 14 comment:13 by , 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?
follow-ups: 15 16 comment:14 by , 11 years ago
Cc: | 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 by , 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 theMilestone
when implementing #4582 and presumably callingTicket.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
.
follow-up: 17 comment:16 by , 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).
follow-up: 18 comment:17 by , 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.
follow-up: 19 comment:18 by , 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.
comment:19 by , 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 , 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 , 11 years ago
Milestone: | next-major-releases → 1.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:23 by , 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 , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:25 by , 11 years ago
Cc: | removed |
---|---|
Keywords: | milestone added; consider removed |
comment:26 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
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.
follow-up: 31 comment:27 by , 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?
follow-up: 45 comment:28 by , 11 years ago
r11832 is…
- I don't think that we should use
id="comment"
for comment field causeticket.css
has#comment { width: 100% }
andlabel[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 , 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 , 11 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
follow-up: 32 comment:31 by , 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 asave_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
.
follow-up: 34 comment:32 by , 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 , 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.
follow-up: 35 comment:34 by , 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 likesave_changes(ids, new_values, authname, comment, action)
. WouldTicket.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 thanMilestoneModule._retarget_tickets
.Milestone.move_tickets
could be reused in Milestone.delete. Maybemove
is not a good verb though, isretarget
better?
IMO, Milestone.move_tickets
sounds good.
comment:35 by , 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 fromreq.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 , 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 , 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 RegressionTestTicket5658
→ TestMilestoneCloseRetargetTickets
.
My next steps are:
MilestoneModule._do_delete
inroadmap.py
should send a batch notification when tickets are retargeted.- The
_retarget_tickets
method can be removed fromMilestoneModule
andMilestone.move_tickets
can be called in it's place. Aftermove_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 , 11 years ago
Attachment: | MilestoneDelete.png added |
---|
by , 11 years ago
Attachment: | TicketRetargeted.png added |
---|
follow-up: 48 comment:38 by , 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.
follow-up: 40 comment:39 by , 11 years ago
follow-up: 41 comment:40 by , 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.
comment:41 by , 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 , 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 , 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 , 11 years ago
Status: | reopened → assigned |
---|
comment:45 by , 11 years ago
Replying to jomae:
r11832 is…
- I don't think that we should use
id="comment"
for comment field causeticket.css
has#comment { width: 100% }
andlabel[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].
follow-up: 47 comment:46 by , 10 years ago
Starting back at this one with a few questions so far.
- 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,))
- 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
andadd_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 by , 10 years ago
Replying to rjollos:
Maybe we could make a decorator that handles the
try / except,
log
andadd_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 by , 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 , 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 ofdelete
. Returning a list of retargeted ticket ids is not something I'd expect theMilestone
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 theroadmap
module, by callingMilestone.move_tickets
. I've deprecated theretarget_to
parameter ofMilestone.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.
comment:50 by , 10 years ago
Description: | modified (diff) |
---|---|
Release Notes: | modified (diff) |
Resolution: | → fixed |
Status: | assigned → closed |
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 , 10 years ago
Release Notes: | modified (diff) |
---|
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).