Edgewall Software
Modify

Opened 10 years ago

Closed 10 years ago

Last modified 6 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
Priority: normal Milestone: 0.8
Component: ticket system Version: 0.7.1
Severity: normal Keywords:
Cc: toni.brkic@…
Release Notes:
API 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 10 years ago.
patch

Download all attachments as: .zip

Change History (12)

comment:1 Changed 10 years ago by cmlenz

  • Milestone changed from 0.7.2 to 0.8

0.7.2 is cancelled

comment:2 Changed 10 years ago by cmlenz

  • Component changed from general to ticket system

comment:3 Changed 10 years ago by tbrkic

  • Summary changed from Ticket's cc list modification should send mail to old cc list to [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

Changed 10 years ago by tbrkic

patch

comment:4 Changed 10 years ago by jonas

  • Status changed from new to assigned

comment:5 Changed 10 years ago by jonas

  • Resolution set to fixed
  • Status changed from assigned to closed

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

comment:6 Changed 10 years ago by jonas

Just a test…

comment:7 Changed 10 years ago by jonas

Another test…

comment:8 Changed 10 years ago by tonib

  • Resolution fixed deleted
  • Status changed from closed to reopened

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 Changed 10 years ago by jonas

  • 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 Changed 10 years ago by 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.

comment:11 Changed 10 years ago by jonas

  • Resolution set to fixed
  • Status changed from reopened to closed

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.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from jonas 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.