No ticket change data associated with re-targeting tickets to new milestone

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

When a milestone is closed and tickets are re-targeted to a new milestone, there are no entries in the ticket_change table associated with the re-targeting and no notifications sent. This can make it difficult to track down mistakes, which is what led to my investigation that resulted in this ticket.

Suppose the following events.

1. Milestone for a ticket X is changed from milestone A to milestoneB.
2. Ticket X is open and milestoneB is closed with open tickets re-targeted to milestone C.
3. Ticket X is changed to milestoneD.

The change history of ticket X will read:

• Milestone changed from milestoneA to milestoneB
• Milestone set to milestoneD

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

Patch against r11412 of the trunk.

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

Patch against r11412 of the trunk.

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

I accidentally included the patch for #10924 in the first upload, so t10925-r11412-2.patch​ is the one to look at.

The patch adds a comment to each re-targeted ticket, for example: Tickets associated with milestoneA retargeted to milestoneB. I'm not sure I see the value of the comment and could easily be talked into modifying the patch to remove the comment.

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

I gave this some more thought, and considered whether a user with MILESTONE_EDIT should be allowed to change the milestone for a ticket, or whether that operation should require TICKET_CHGPROP. The following lightly tested patch checks for TICKET_CHGPROP permission for each resource before allowing the milestone to be changed. I'm just hoping to get some feedback on which direction to go, and then will submit a final patch and look at adding a testcase or two.

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

Patch against r11412 of the trunk that required TICKET_CHGPROP permission to change the ticket's milestone.

[Peter Suter]

Looks like a duplicate of #5658. Don't let that discourage you though. :)

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

I'll leave the patches here for now, and hope to get some feedback. I'd post subsequent patches against #5658.

[Peter Suter]

Since this seems similar to batch modify, let's compare with that:

• There all changes are nested in one DB transaction​.
• And they all use the same timestamp​. (If I remember correctly, this helps bundling up the timeline events.)
• There are no TICKET_CHGPROP checks.
• I guess we could even send out a batch notification email​. (Although you could argue this is out of scope for your change.)

(Of course it doesn't automatically mean that batch modify does all this correctly!)

The comment included in the patch seems a bit redundant to me. But as comment:8:ticket:5658 notes it may be useful to mention the reason for retargeting (here: closed milestone; #4582: renamed milestone).

I guess internationalization support should be added for the comment and the warning.

[Remy Blank]

I agree with Peter that the behavior should be the same for batch modification and re-targeting on closing a milestone, should generate the same event in the timeline and the same notification.

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

I'll refresh the patch with these changes. I'd just note that although batch modify doesn't do a TICKET_CHGPROP check, it does require BATCH_MODIFY, which to me clearly says the user has been granted permission can modify all fields. On the other hand, I wouldn't expect the user to necessarily have the ability to modify any ticket fields given MILESTONE_EDIT permission.

[Peter Suter]

Maybe it should actually require TICKET_BATCH_MODIFY?

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

Sorry I didn't mention earlier, but thank you very much for the feedback Pete and Remy! I have a refreshed patch that batch updates the timeline and sends a batch email notification. I've deferred the permissions issue for now, until someone with more experience tells me that it is worthwhile to implement. I've attached the patch to #5658, so I guess the review should continue in that ticket.

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

Replying to psuter:

Maybe it should actually require TICKET_BATCH_MODIFY?

Yeah, that sort of makes sense to me, and would certainly keep the code simpler.