Edgewall Software
Modify

Opened 11 years ago

Closed 9 years ago

Last modified 8 years ago

#8360 closed enhancement (fixed)

[PATCH] Use updater's name and email in ticket notifications

Reported by: gm.outside+trac@… Owned by: Remy Blank
Priority: high Milestone: 1.0
Component: notification Version: 0.11-stable
Severity: normal Keywords: updater notification from consider patch
Cc: Thijs Triemstra, mpotter@… Branch:
Release Notes:

notification: Allow using the author of a change as the From: header of the notification e-mail.

API Changes:
Internal Changes:

Description

This patch adds a new boolean config option named 'smtp_from_smart'. When this option is set to true notifications use updater's name and email for their 'From:' message header. We find it very useful since this patch helps to easily see who was modifying a ticket in a list of messages. Also, it's now possible to reply to the person directly.

The patch was created by (GalaxyMaster) from Openwall Project

This work was sponsored by CivicActions LLC

Attachments (2)

trac-0.11.4-gm-smtp_from_smart.diff (3.4 KB ) - added by gm.outside+trac@… 11 years ago.
8360-smtp-from-author-r10410.patch (7.7 KB ) - added by Remy Blank 10 years ago.
Use the change author as a sender if [notifiation] smtp_from_author is true.

Download all attachments as: .zip

Change History (18)

by gm.outside+trac@…, 11 years ago

comment:1 by Christian Boos, 11 years ago

Keywords: consider added
Milestone: 0.13

comment:2 by Thijs Triemstra, 10 years ago

Cc: Thijs Triemstra added
Keywords: patch added

I'm not such a fan of the smtp_from_smart name, but I like the feature, even though I wonder if it's not possible already. If not, let's raise the priority of this ticket.

comment:3 by Thijs Triemstra, 10 years ago

Here's an updated patch that re-uses the stmp_from_name and applies the updater's name when it's passed the value __updater__.

  • trac/ticket/notification.py

     
    8585        changes_body = ''
    8686        self.reporter = ''
    8787        self.owner = ''
     88        self.updater = ''
    8889        changes_descr = ''
    8990        change_data = {}
    9091        link = self.env.abs_href.ticket(ticket.id)
     
    329330            for updater, in cursor:
    330331                break
    331332
     333        self.updater = updater
    332334        if not notify_updater:
    333335            filter_out = True
    334336            if notify_reporter and (updater == self.reporter):
  • trac/notification.py

     
    276276        self._init_pref_encoding()
    277277        domains = self.env.config.get('notification', 'ignore_domains', '')
    278278        self._ignore_domains = [x.strip() for x in domains.lower().split(',')]
    279         # Get the email addresses of all known users
     279        # Get the names and email addresses of all known users
    280280        self.email_map = {}
     281        self.name_map = {}
    281282        for username, name, email in self.env.get_known_users(self.db):
     283            if name:
     284                self.name_map[username] = name
    282285            if email:
    283286                self.email_map[username] = email
    284287               
     
    404407            reactivate(t)
    405408        projname = self.env.project_name
    406409        public_cc = self.config.getbool('notification', 'use_public_cc')
     410
     411        if self.from_name == '__updater__' and self.updater:
     412            name = self.updater
     413            if self.updater in self.name_map:
     414                name = self.name_map[self.updater]
     415            self.from_name = name
     416
     417            if self.updater in self.email_map:
     418                self.from_email = self.email_map[self.updater]
     419
    407420        headers = {}
    408421        headers['X-Mailer'] = 'Trac %s, by Edgewall Software' % __version__
    409422        headers['X-Trac-Version'] =  __version__

I'll add a new patch with documentation if it gets accepted.

Last edited 10 years ago by Thijs Triemstra (previous) (diff)

comment:4 by Thijs Triemstra, 10 years ago

The patch on #6512 seems to do almost the same, except it uses the username instead of the name specified in the preferences panel.

comment:5 by Remy Blank, 10 years ago

Milestone: next-major-0.1X0.13
Owner: changed from Emmanuel Blot to Remy Blank
Priority: normalhigh

Putting on my todo list for review. The patch in #6512 looks simpler at first sight, but I'll review both properly.

in reply to:  3 comment:6 by anonymous, 10 years ago

Replying to thijstriemstra:

Here's an updated patch that re-uses the stmp_from_name and applies the updater's name when it's passed the value __updater__.

I think you miss one feature I introduced in the original patch — it also uses {{smtp_from_name}}, but it appends it to updater's name, e.g. if {{smtp_from_name}} was configured to be '[via Trac]' and updater is "Happy Joe", then the resulting From in my patch will be '"Happy Joe [via Trac]" <happy.joe@…'.

The rationale for this is that people are not happy to see an automated messages which they didn't write, but with the addition of, say, '[via Trac]' it's clear that the message was generated on their behalf. Also, it's much easier to filter/search for these messages.

by Remy Blank, 10 years ago

Use the change author as a sender if [notifiation] smtp_from_author is true.

comment:7 by Remy Blank, 10 years ago

8360-smtp-from-author-r10410.patch is a slightly more complete version of the patch in comment:3, with the following changes:

  • It uses a separate boolean option [notifiation] smtp_from_author to enable the functionality, so that smtp_from and smtp_from_name can still be set independently. The latter two are used if the author of a change hasn't configured an e-mail address.
  • It adds a test case for the new behavior.

I didn't keep the suggested "misuse" of smtp_from_name as a name suffix. Sure, the author of a change didn't write the notification e-mail, but she did make the change, so sending in her name feels acceptable. And filtering is better done with other headers (e.g. X-Trac-Project).

Feedback and testing welcome.

comment:8 by Remy Blank, 10 years ago

#6512 was closed as a duplicate. The patch above uses the idea from #6512 to use the reporter in the From: header for new tickets, and the change author for changes.

comment:9 by Remy Blank, 10 years ago

An improved patch that also handles anonymous users having defined a name and an e-mail address was applied in [10421].

comment:10 by Remy Blank, 10 years ago

Release Notes: modified (diff)
Resolution: fixed
Status: newclosed

Documentation updated in 0.13/TracNotification@2.

comment:11 by mpotter@…, 9 years ago

Cc: mpotter@… added

Maybe I've missed something in my reading of this ticket, but I think I see a few issues with the current solution:

  1. With the "improvement" mentioned in 9, it would be possible for a malefactor to:
    • Attached to a Trac system which allows anonymous users to add or comment to tickets and uses this feature.
    • Use the Preferences to set an email address to someone else's email address and name, the fall-guy.
    • Enter a inappropriate comment and/or new ticket.
    • The Trac system would then send inappropriate emails that would appear to have come from the fall-guy.
    Suggestion: This feature should only use email addresses from authenticated users.
  1. Many SMTP servers are configured in such a way that they will refuse any message with a From: field not being the same as the authenticated user (i.e. smtp_from != smtp_user). If one has such an SMTP server, one cannot utilize this new feature.

    Suggestion: Additionally allow, which could be used instead:
    • Setting the Reply-To: field to the change author.
    • Adding the author to the from-name. e.g. User Ford makes a change to a ticket in the HHGTrac system from Megadodo Publications. This results in emails with a from line something like:
      • From: Ford via HHGTrac <trac@megadodo.com>

in reply to:  11 comment:12 by Mark Potter <mpotter@…>, 9 years ago

Resolution: fixed
Status: closedreopened

Since I commented on this ticket with issues I feel should be addressed or at least acknowledge by a Trac authority, I am reopening this ticket. If the powers that be do not think these issues need further examination, feel free to just re-close. Just wanted to make sure that someone of authority thought about these issues and that they weren't just overlooked in the noise.

in reply to:  11 ; comment:13 by Remy Blank, 9 years ago

Replying to mpotter@…:

  1. With the "improvement" mentioned in 9, it would be possible for a malefactor to:

Yes, that is correct. Given that it's trivial to send emails impersonating someone else, I don't think this is actually a problem. And the feature should probably not be enabled for externally-facing Trac instances that allow anonymous posting. It's disabled by default.

Suggestion: Additionally allow, which could be used instead:

  • Setting the Reply-To: field to the change author.
  • Adding the author to the from-name. e.g. User Ford makes a change to a ticket in the HHGTrac system from Megadodo Publications. This results in emails with a from line something like:
    • From: Ford via HHGTrac <trac@megadodo.com>

Those sound like good ideas. Unfortunately, our notification subsystem is in a relatively sorry state, and adding the features would make the situation even worse. We were hoping for the integration of the th:AnnouncerPlugin to give a new breath to that part, allowing to more easily customize notifications and how they are sent, but that effort seems to have stalled.

I'm closing this again, as the initial feature request was implemented. Could you please file a new ticket with your suggestions? Thanks.

comment:14 by Remy Blank, 9 years ago

Resolution: fixed
Status: reopenedclosed

Actually closing.

in reply to:  11 comment:15 by Mark Potter <mpotter@…>, 9 years ago

Replying to rblank:

Could you please file a new ticket with your suggestions? Thanks.

Filed new ticket #10579 to cover the first issue (do not use "anonymous" email addresses).

Filed new ticket #10578 to cover the first bullet of the second issue (changing the ReplyTo: field). Left off the second bullet (changing the From: name) for negative comments on this had been made before which I can agree with, but some may disagree. Conversations can be take there.

in reply to:  13 comment:16 by Steffen Hoffmann, 8 years ago

Replying to rblank:

Replying to mpotter@…:

Suggestion: Additionally allow, which could be used instead:

  • Setting the Reply-To: field to the change author.
  • Adding the author to the from-name. e.g. User Ford makes a change to a ticket in the HHGTrac system from Megadodo Publications. This results in emails with a from line something like:
    • From: Ford via HHGTrac <trac@megadodo.com>

Those sound like good ideas. Unfortunately, our notification subsystem is in a relatively sorry state, and adding the features would make the situation even worse. We were hoping for the integration of the th:AnnouncerPlugin to give a new breath to that part, allowing to more easily customize notifications and how they are sent, but that effort seems to have stalled.

Right, I've been one of the supporters of the proposal. Only I've got distracted by other Trac plugin projects for a while. Please note, that th:AnnouncerPlugin development as just continued, and I foresee that we'll follow-up on the original proposal in due time as well.

I'll think about using the preliminary state from TracNotification regarding this feature and preparing a more competing feature for AnnouncerPlugin along the lines of #10578. Please consider to follow-up on th:#10613 with comments.

Modify Ticket

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