Edgewall Software
Modify

Opened 17 years ago

Closed 9 years ago

Last modified 4 years ago

#4056 closed enhancement (fixed)

Notification Preferences

Reported by: jeff.wise@… Owned by: Peter Suter
Priority: normal Milestone: 1.1.3
Component: notification Version: 0.10
Severity: normal Keywords: notification email
Cc: Jun Omae Branch:
Release Notes:

Added a Notification preferences panel.

API Changes:

Added new extension point INotificationSubscriber.

Internal Changes:

Description

My team has been using Trac for 3 months now. Overall its been a great success. However, some team members complain that they receive lots of e-mail that doesn't matter to them. This eventually leads to their ignoring Trac e-mails. An example would be when they are placed on the CC list of a ticket and later someone changes one of the ticket properties. They aren't interested in ticket property changes.

I've searched, but I haven't found a mechanism to customize the notification preferences on a per user basis. There is global configuration in TracIni, but no means for a user to indicate the kind of notifications they are interested in receiving.

I would like for a user to indicate which notifications they would like to receive:

  • All (The current behaviour)
  • None
  • Ticket Creation
  • Addition of Comments/Replies
  • All Ticket properties
  • Owner changes
  • Milestone changes
  • etc.

Please consider adding this feature to Trac. Thanks.

Attachments (1)

SubscriptionRulesAsAnonymous.png (25.0 KB ) - added by Ryan J Ollos 9 years ago.

Download all attachments as: .zip

Change History (52)

comment:1 by sid, 17 years ago

Similar to, but not quite the same as #2073.

comment:2 by Christian Boos, 17 years ago

Milestone: 1.0

See also my note about user preferences settings.

Also, the level of details proposed here looks a bit to high for me, especially if in the future we have also notifications for wiki, milestone, changesets, etc.

So I'd propose a 3-levels setting, matching what we have in the timeline:

  • All changes
  • Only Status changes
  • None

comment:3 by Noah Kantrowitz (coderanger) <coderanger@…>, 17 years ago

If coupled with the TracObjects idea, maybe you could keep a list of (resource, field/facet) pairs to base notifications off.

comment:4 by sid, 17 years ago

If you are cc'ed on a ticket, you will probably want to know more than just status changes. You probably want to be involved in the discussion as well, and notified of new comments. But possibly care less about ticket property changes.

comment:5 by Alec Thomas, 17 years ago

Owner: changed from Jonas Borgström to Alec Thomas

comment:6 by sid, 17 years ago

#3269 was marked as a duplicate. It contained a patch for the 0.9 branch that hardcoded cc rules to only cc on comment or description changes.

comment:7 by Matthew Good, 17 years ago

#4166 and #3155 have been marked as duplicates of this ticket.

comment:8 by Emmanuel Blot, 17 years ago

Keywords: notification email added

comment:9 by Andy Schlaikjer <hazen@…>, 17 years ago

Cc: hazen@… added

comment:10 by sid, 17 years ago

#1640 was marked as duplicate of this ticket.

comment:11 by jeff.wise@…, 17 years ago

I'd really like to see ticket notification preferences look something like the following. Bullets are like checkboxes.

Notify me when:

  • I am the ticket reporter.
  • I am the ticket owner.
  • I am a ticket updater.
  • I am on the ticket CC list.

Notify me of:

  • New comments.
  • Status changes.
  • Resolution changes.
  • Property changes.
    • List all, including custom, properties here.

comment:12 by Emmanuel Blot, 17 years ago

I think we gonna face a potential issue here: if notification content are based on user preferences, this means that we'll need to emit one email per recipient - rather than one email for all recipients, as per today.

As notification are synchronous (i.e. the web request completes when all notifications have been sent to the SMTP server), how Trac will behave with numerous recipients ?

Does this mean we need an asynchronous service to actually send the notifications? This is something that should be thought about before starting any user-based content settings.

in reply to:  12 comment:13 by jeff.wise@…, 17 years ago

Replying to eblot:

I think we gonna face a potential issue here: if notification content are based on user preferences, this means that we'll need to emit one email per recipient - rather than one email for all recipients, as per today.

I don't understand the limitation here. Possible recipients are the reporter, owner, updater, and CC list. Use their preferences and the ticket activity to filter the list down to those who care. Then send a single e-mail to the filtered list. Unless the CC list or the set of updaters is incredibly long, it shouldn't take a significant amount of time to filter the recipient list.

comment:14 by hazen@…, 17 years ago

Cc: hazen@… removed

comment:15 by ThurnerRupert, 17 years ago

this feature would be also very useful for mixed user populations, like "web users" (maybe prefer notification via mail) and eclipse mylar users (get notified by mylar).

comment:16 by ThurnerRupert, 17 years ago

maybe prevent a notification to self like addressed in #2247 is also something to consider?

comment:17 by Christian Boos, 17 years ago

Component: ticket systemnotification
Milestone: 1.00.12

See also #2625 (preferred format)

in reply to:  12 comment:18 by Pedro Algarvio, aka, s0undt3ch <ufs@…>, 16 years ago

Replying to eblot:

I think we gonna face a potential issue here: if notification content are based on user preferences, this means that we'll need to emit one email per recipient - rather than one email for all recipients, as per today.

As notification are synchronous (i.e. the web request completes when all notifications have been sent to the SMTP server), how Trac will behave with numerous recipients ?

Does this mean we need an asynchronous service to actually send the notifications? This is something that should be thought about before starting any user-based content settings.

Something like the AnnouncerPlugin should be though of.

comment:19 by Carsten Fuchs <CarstenFuchs@…>, 14 years ago

I too have problems with users that get too many or too little email notifications.

A suggestion: Why discriminate between reporter, owner and updater at all? Instead, add a checkbox

  • Notify me when the ticket is changed (subscribe ticket)

to the "New Ticket" and "Modify Ticket" forms (keep the decision if this checkbox is checked by default as a per user setting), and a "Subscribe ticket (notify me when the ticket is changed)" link to the ticket view pages?

This would be a simple system that meets all requirements, analogous to how e.g. the phpBB forums notify users about replies.

While this replaces the current always_notify_[owner|reporter|updater] settings in trac.ini, the smtp_always_[cc|bcc] settings should be kept.

The only open question is how a user can automatically subscribe to new tickets (e.g. another per user checkbox setting: "Notify me when a new ticket is created").

comment:21 by Peter Suter, 10 years ago

The Extension API for subscriptions proposal adds a new DB table and a new extension point that allows plugins to implement different rules for subscribing to notifications.

A draft of the proposed changes can be found in log:psuter@advanced-notification-subscriptions (these depend on the proposed changes of comment:11:ticket:3517). These will have to be reviewed and discussed in some more detail. For example I listed some open questions in the relevant wiki pages.

A generic preferences dialog allows users to manage their subscriptions. (changeset:158e97e037b9/psuter, Screenshot)

comment:22 by Peter Suter, 10 years ago

Milestone: next-major-releases1.1.2
Owner: changed from Alec Thomas to Peter Suter
Status: newassigned

comment:23 by Jun Omae, 10 years ago

Cc: Jun Omae added

comment:24 by Jun Omae, 10 years ago

I'm reviewing your branch. Also, my proposed changes can be found in https://bitbucket.org/jun66j5/trac.advanced_notification/commits/branch/trunk.

  • subscription table: at least, we should add subscription (sid, authenticated) index for Subscription model.
    • Also, subscription (class) index if we would use Subscription.find_by_class.
  • prefs_notification.html
    • We should add i18n:msg for i18n/l10n
    • In the page, message is made from adverb and description of subscriber, e.g. always notify me of any ticket changes. It's difficult to translate such a message.

comment:25 by Peter Suter, 10 years ago

Excellent, thanks!

What does py:choose="a" do? (I only found it documented in combination with py:when.)

comment:26 by Jun Omae, 10 years ago

Oh, that's my bad. I just removed it in e8d8123.

comment:27 by Peter Suter, 10 years ago

I assume upgrades/db30.py would also need to be adjusted when adding a new index.

comment:28 by Jun Omae, 10 years ago

I updated https://bitbucket.org/jun66j5/trac.advanced_notification/commits/branch/trunk.

  • f37d03d subscription (sid, authenticated) index is added on upgrades/db30.py
  • edc936a changed end-of-line of db30.py to LF
  • c8b9105 imports only symbols in trac/notification.py for backward compatibility
  • f6f6e24 defined __all__ in api.py, compat.py and mail.py
  • 5467802 fixed a wrong import statement for BatchTicketNotifyEmail
  • 3982f62 added DefaultDomainEmailResolver component which is already in [notification] email_address_resolvers
  • ed06c0b fixed broken From header if smtp_from_name has unicode characters

Another issue: If adding first subscription rule, format column of created subscription would be empty. The issue isn't fixed until saving email format. I think email format should be stored in session.

sqlite> select * from subscription;
id          time              changetime        class                  sid         authenticated  distributor  format      priority    adverb
----------  ----------------  ----------------  ---------------------  ----------  -------------  -----------  ----------  ----------  ----------
1           1384869157624135  1384869157624135  TicketOwnerSubscriber  jun66j5     1              email                    1           always

comment:29 by Peter Suter, 10 years ago

  • 3982f62 added DefaultDomainEmailResolver component which is already in [notification] email_address_resolvers

I initially had a DefaultDomainEmailResolver and dropped it again in changeset:2c5d1c0c2abb/psuter (but forgot to remove it from email_address_resolvers). I'm not against adding it back if it's useful. I couldn't find a good reason to keep it, as it seems not quite to do the same thing as before (resolve vs. recognize) and default domain handling is needed in RecipientMatcher anyway to get the same behavior as before.

Another issue: If adding first subscription rule, format column of created subscription would be empty. The issue isn't fixed until saving email format. I think email format should be stored in session.

Yes, storing the format per subscription doesn't seem that useful and has some downsides. Will storing it in the session work for other distributors? (Or maybe only email really needs multiple formats?)

in reply to:  29 comment:30 by Jun Omae, 10 years ago

Replying to psuter:

  • 3982f62 added DefaultDomainEmailResolver component which is already in [notification] email_address_resolvers

I initially had a DefaultDomainEmailResolver and dropped it again in changeset:2c5d1c0c2abb/psuter (but forgot to remove it from email_address_resolvers). I'm not against adding it back if it's useful. I couldn't find a good reason to keep it, as it seems not quite to do the same thing as before (resolve vs. recognize) and default domain handling is needed in RecipientMatcher anyway to get the same behavior as before.

Oh, I missed that. I'll retry and remove DefaultDomainEmailResolver from default of email_address_resolvers.

Another issue: If adding first subscription rule, format column of created subscription would be empty. The issue isn't fixed until saving email format. I think email format should be stored in session.

Yes, storing the format per subscription doesn't seem that useful and has some downsides. Will storing it in the session work for other distributors? (Or maybe only email really needs multiple formats?)

Hmm, please ignore it. At least, empty format column doesn't lead notification.

If configuring subscription rules and the format even isn't matched, we should notify with the distributor and the other format. Otherwise, the user would neither receive any notification nor know the reason because the distributor and the format don't shown in subscription rules.

in reply to:  24 ; comment:31 by Peter Suter, 10 years ago

Replying to jomae:

  • subscription table: at least, we should add subscription (sid, authenticated) index for Subscription model.
    • Also, subscription (class) index if we would use Subscription.find_by_class.

Adding this index sounds like a good idea, so plugins can use find_by_class.

Do you think we should use a different name for the subscription table, to avoid a name clash with the Announcer plugin?

in reply to:  31 comment:32 by Jun Omae, 10 years ago

Replying to psuter:

Do you think we should use a different name for the subscription table, to avoid a name clash with the Announcer plugin?

Sounds good. What is the name good? notification table?

comment:33 by Ryan J Ollos, 10 years ago

Milestone: 1.1.21.1.3

comment:34 by Peter Suter, 9 years ago

Updated in log:psuter@advanced-notification-subscriptions.2. (Still depends on #3517, and I folded in all of Jun's improvements for easier reviewing. Thanks again!)

What is the name good? notification table?

The proposal now uses notify_subscription. I hope this is an acceptable compromise.

comment:35 by Peter Suter, 9 years ago

comment:36 by Peter Suter, 9 years ago

API Changes: modified (diff)
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

Committed to trunk in [13469].

comment:37 by Christian Boos, 9 years ago

Anything noteworthy on this topic to add to 1.1/TracUpgrade? Looking towards upgrading t.e.o at some point.

in reply to:  37 comment:38 by Peter Suter, 9 years ago

Replying to cboos:

Anything noteworthy on this topic to add to 1.1/TracUpgrade?

I'm not sure, there's not really anything that special so far. There's a new DB table and some new components, but nothing to upgrade manually I think, no new configuration options and the old ones should all still work the same as before.

comment:39 by Jun Omae, 9 years ago

Hints in prefs_notification.html say;

Example: The subscription rule <strong>"Notify: Any ticket changes"</strong> overrides …

However, no components which INotificationSubscriber is implemented have Any ticket changes as subscriber's description.

in reply to:  39 comment:40 by Peter Suter, 9 years ago

Replying to jomae:

Hints in prefs_notification.html say;

Example: The subscription rule <strong>"Notify: Any ticket changes"</strong> overrides …

However, no components which INotificationSubscriber is implemented have Any ticket changes as subscriber's description.

Right, it makes more sense with AllTicketSubscriber (Any ticket is modified) proposed in #11870.

I left it because the example still seems more helpful for understanding than the alternatives.

Also any static example could have that problem, depending on the configuration. Creating the example dynamically seems excessive though. Maybe it would be better to remove the example here and instead describe it on TracNotification?

Last edited 9 years ago by Peter Suter (previous) (diff)

comment:41 by Ryan J Ollos, 9 years ago

Minor defect in [13469#file13] fixed in [13600].

comment:42 by Jun Omae, 9 years ago

In [13673], fixed missing id attribute.

by Ryan J Ollos, 9 years ago

comment:43 by Ryan J Ollos, 9 years ago

Subscription rules select box is empty for anonymous user. Is that intended? If so, should we instead hide some or all of the Notifications preference panel for anonymous user?

in reply to:  43 comment:44 by Peter Suter, 9 years ago

Replying to rjollos:

Subscription rules select box is empty for anonymous user. Is that intended? If so, should we instead hide some or all of the Notifications preference panel for anonymous user?

It is intended that some subscriptions rules are not available to anonymous users. This includes all subscription rules available to authenticated users by default. I agree it would probably make sense to hide that UI if no rules are available to the current user.

comment:45 by Ryan J Ollos, 9 years ago

Thanks, I created ticket #11990.

comment:46 by Ryan J Ollos, 9 years ago

I posted a proposed fix to #11990 and I'm considering to commit it after adding tests. Could you take a look and let me know if it looks right to you? Thanks!

comment:47 by Ryan J Ollos, 9 years ago

log:rjollos.git:t4056_inotificationsubscriber_refactoring proposes a refactoring to simplify the implementation of INotificationSubscribers and ensure batchmodify events behave like ticket notification events unless explicitly overridden in the INotificationSubscriber. All tests pass after the change.

comment:48 by Ryan J Ollos, 9 years ago

comment:47 changes committed to trunk in [14328].

comment:49 by Ryan J Ollos, 4 years ago

The Subscription and Watch classes have a lot of classmethods and don't fit the typical class design pattern in Trac. Is there a design reason for that? Or was that code just carried over from TracAnnouncer?

The overhead in instantiating an object must be trivially small for our use-cases.

  • trac/ticket/notification.py

    diff --git a/trac/ticket/notification.py b/trac/ticket/notification.py
    index 81903a63a..fddda90ac 100644
    a b class NewTicketSubscriber(Component):  
    661661            return
    662662
    663663        klass = self.__class__.__name__
    664         for s in Subscription.find_by_class(self.env, klass):
     664        for s in Subscription(self.env).find_by_class(klass):
    665665            yield s.subscription_tuple()
    666666
    667667    def description(self):
    def _ticket_change_subscribers(subscriber, candidates):  
    773773        if sid:
    774774            sids.add((sid, auth))
    775775
    776     for s in Subscription.find_by_sids_and_class(subscriber.env, sids, klass):
     776    for s in Subscription(subscriber.env).find_by_sids_and_class(sids, klass):
    777777        yield s.subscription_tuple()

However, even if the latter is a better or more Trac-typical class design, it would be an API change to modify this now.

Last edited 4 years ago by Ryan J Ollos (previous) (diff)

in reply to:  49 ; comment:50 by Peter Suter, 4 years ago

The Subscription and Watch classes have a lot of classmethods and don't fit the typical class design pattern in Trac. Is there a design reason for that? Or was that code just carried over from TracAnnouncer?

Right, it was just carried over. (Watch was renamed from SubscriptionAttribute if I remember correctly.)

I don't know if this design (a mix of repository and static factory method design patterns?) was originally chosen for performance or other reasons.

Note that the classes do get instantiated for each found DB record, and each such object contains the fields of one DB record. Creating (and throwing away) an "empty" object first could be considered strange. Other Trac models actually do also use class methods for batch operations like *.select or Attachment.delete_all. Subscription and Watch are mostly used for batch operations. I think the only non-batch operation is add. (Which would arguably be nicer as a normal method and is a classmethod only for consistency I guess)

in reply to:  50 comment:51 by Ryan J Ollos, 4 years ago

Replying to Peter Suter:

Note that the classes do get instantiated for each found DB record, and each such object contains the fields of one DB record. Creating (and throwing away) an "empty" object first could be considered strange. Other Trac models actually do also use class methods for batch operations like *.select or Attachment.delete_all. Subscription and Watch are mostly used for batch operations. I think the only non-batch operation is add. (Which would arguably be nicer as a normal method and is a classmethod only for consistency I guess)

Yeah, that makes sense. Thanks!

Modify Ticket

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