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

[anonymous]

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.

[Christopher Lenz]

0.7.2 is cancelled

[tbrkic]

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

[tbrkic]

patch

[Jonas Borgström]

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

[Jonas Borgström]

Just a test…

[Jonas Borgström]

Another test…

[tonib]

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?

[Jonas Borgström]

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

[tonib <toni.brkic at switchcore.com>]

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.

[Jonas Borgström]

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.