Edgewall Software
Modify

Opened 20 years ago

Closed 19 years ago

Last modified 15 years ago

#578 closed defect (fixed)

[Merge]Ticket's cc list modification should send mail to old cc list

Reported by: anonymous Owned by: Jonas Borgström
Priority: normal Milestone: 0.8
Component: ticket system Version: 0.7.1
Severity: normal Keywords:
Cc: toni.brkic@… Branch:
Release Notes:
API Changes:
Internal Changes:

Description

If a ticket change includes a modification of the ticket's cc list, then the mail corresponding to the change goes only to the new addresses on the cc list. The mail should also go to the addresses on the old cc list, so that the people concerned know that they won't be getting any more mails.

Attachments (1)

patch_578.diff (1.6 KB ) - added by tbrkic 19 years ago.
patch

Download all attachments as: .zip

Change History (12)

comment:1 by Christopher Lenz, 20 years ago

Milestone: 0.7.20.8

0.7.2 is cancelled

comment:2 by Christopher Lenz, 20 years ago

Component: generalticket system

comment:3 by tbrkic, 19 years ago

Summary: Ticket's cc list modification should send mail to old cc list[Merge]Ticket's cc list modification should send mail to old cc list

I have supplied a patch. The patch makes changes go to the merg of the old an new cc list.

I also added so that owner is notified as suggested in comment from #395. Only though if always_notify_reporter is set to true.

Best regards,

Toni

by tbrkic, 19 years ago

Attachment: patch_578.diff added

patch

comment:4 by Jonas Borgström, 19 years ago

Status: newassigned

comment:5 by Jonas Borgström, 19 years ago

Resolution: fixed
Status: assignedclosed

Applied a modified version of the patch in [1009]. Thanks Toni!

comment:6 by Jonas Borgström, 19 years ago

Just a test…

comment:7 by Jonas Borgström, 19 years ago

Another test…

comment:8 by tonib, 19 years ago

Resolution: fixed
Status: closedreopened

There is a bug in the patch. The prev_cc must be set to empty list in notify. If not doing that and using modpython you might send emails to more ppl than it should be sent. The below should be added:

def notify(self, ticket, newticket=1, modtime=0):  
    self.prev_cc = []  

The code is written with the syntax of [1009]

And two more question on the changes between the patch and the changeset.

If I understand correctly you dont call get_email_addresses on emails that are on the cc list. Is this intended behaviour? Doesnt this mean that trac will send email to cc addresses even if they dont contain any @ sign.

In the patch an email is also sent to the owner of the ticket. Why has this been removed from the changeset?

comment:9 by Jonas Borgström, 19 years ago

Cc: toni.brkic@… added

self.prev_cc is already set to [] in the constructor isn't that enough?

We don't use get_email_addresses becuase it can't parse multiple addresses if they are separated by spaces. It only works if they are comma-separated. I guess we could ignore not fully qualified email addresses (without domain name) but it might be handy if the email server is configured to deliver unqualified addresses to the local domain.

This ticket isn't about always sending notifications to the owner. It's not even determined if that's the right thing to do. A separate ticket should be created about that issue.

Toni, I hope you don't mind me adding you to the cc list :)

comment:10 by tonib <toni.brkic at switchcore.com>, 19 years ago

When I now look at the code It seems that it should be enough that it is set in the constructor.

But we had weird problems at our site, were ppl got email for tickets that they werent supposed to have. Our problems dissappeared when I set the email list to the empty list in notify. But that was with my patch and It might have been something else that was acting strange. Maybe the weather or our grumpy server had a bad day. But you are right it should work without setting the email to the empty list in notify. Sorry for that.

I think that the parsing should be consistent. Either you allow not fully qualified names for all email addresses or none. Now you use get_email_addresses for the reporter and acc which means you dont allow not fully qualified names for those fields.

For me it was obvious that also the owner should have an email. This might not be the case, but anyway in #395 there is a comment about it so I am not opening a new ticket.

comment:11 by Jonas Borgström, 19 years ago

Resolution: fixed
Status: reopenedclosed

About the empty list. The problem was that your patch (that was never actually merged) contained a bug. The list was set empty directly in the class block and not in the constructor or any functions, that's a big no no (All instances will share the same list).

I added a simple filter in [1039] that ignores unqualified addresses. I think that change is good enough for 0.8, we can always add a better solution later…

I'm closing this ticket now.

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jonas Borgström.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jonas Borgström 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.