Project

General

Profile

Actions

Feature #36162

open

Add notification reason to the email instead of the default static email footer

Added by Marius BĂLTEANU over 2 years ago. Updated about 1 month ago.

Status:
New
Priority:
Normal
Category:
Email notifications
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:

Description

The default email footer setting is:

You have received this notification because you have either subscribed to it, or are involved in it.
To change your notification preferences, please click here: http://hostname/my/account

This setting can be changed from Settings -> Notifications -> Email footer.

This approach have several issues:
  • the text can be removed by an admin
  • the text is not translatable
  • the text is too generic and the user cannot understand why he received that notification

My proposal is to replace this text with the real reason of why the user received the notification. In this phase, I propose the following reasons with the according translations:

Reason Message Details
INVOLVED You have received this notification because you are involved in. user is either author, assignee or previous assignee
MENTIONED You have received this notification because you have been mentioned in. user in mentioned in that event
SUBSCRIBED You have received this notification because you have subscribed to it. user is subscribed to that event
WATCHER You have received this notification because you are watching it. user is a watcher
ADMIN You have received this security notification because you are an administrator. user is an admin and he received a security notification

In the future, we can add even a more granular reason for the involved reason: author, assignee or previous assignee.

Also, I propose to always display the text "To change your notification preferences, please click here: %{link to my account}."

Below is an example from a security notification with both messages (first message is the new one; second message is the old one).


Files

sn.png (109 KB) sn.png Marius BĂLTEANU, 2021-11-08 23:06
bugzilla-sample.png (21.6 KB) bugzilla-sample.png pasquale [:dedalus], 2021-11-09 08:34
0001-Add-notification-reason.patch (35.5 KB) 0001-Add-notification-reason.patch Marius BĂLTEANU, 2024-01-17 05:56
all_reasons.png (166 KB) all_reasons.png Marius BĂLTEANU, 2024-01-22 12:26

Related issues

Related to Redmine - Feature #13919: Mention user on issues and wiki pages using @user with autocompleteClosedMarius BĂLTEANU

Actions
Related to Redmine - Feature #38492: Provide some ways to find the issues where an user Is mentioned New

Actions
Actions #1

Updated by Marius BĂLTEANU over 2 years ago

  • Description updated (diff)
Actions #2

Updated by Marius BĂLTEANU over 2 years ago

  • Related to Feature #13919: Mention user on issues and wiki pages using @user with autocomplete added
Actions #3

Updated by Bernhard Rohloff over 2 years ago

To me, this sounds like a good improvement of user experience.
It also helps to set the personal notification settings appropriately.
+1 from my side.

Actions #4

Updated by pasquale [:dedalus] over 2 years ago

+1 for me.
I suggest a more coincise way (like bugzilla): if anyone has more reasons to receive an email, we can summarize with a bullet list as shown in attached screenshot:

Actions #5

Updated by C S over 2 years ago

+1 because it is a really good idea!

Actions #6

Updated by Bernhard Rohloff over 2 years ago

pasquale [:dedalus] wrote:

... we can summarize with a bullet list as shown in attached screenshot:

Nice input. I like it.

Actions #7

Updated by Marius BĂLTEANU almost 2 years ago

  • Target version changed from Candidate for next major release to 5.1.0
Actions #8

Updated by Miodrag Milic almost 2 years ago

Why not adding MENTIONED reason now?

Actions #9

Updated by Go MAEDA 11 months ago

  • Related to Feature #38492: Provide some ways to find the issues where an user Is mentioned added
Actions #10

Updated by Go MAEDA 6 months ago

  • Target version changed from 5.1.0 to 6.0.0
Actions #11

Updated by Marius BĂLTEANU 2 months ago

I've added the patch that I would like to commit in the following days.

The patch introduces a new class that keeps the UserNotificationReasons and two methods that:
  • return the reason priority as integer
  • reorder an array of users based on the notifications reason.

In mailer.rb, instead of uniquely append users, we add all the users (included duplicates), reorder them based on priority reason and then remove duplicates. In the end, user still receives only one notification, but the one with the highest reason priority. So, if a user is subscribed to all events from a project and also is assigned to issue A, when issue A is updated, he received the "INVOLVED" and not the "SUBSCRIBED" reason.

Also:
  • the text reason from email is translatable now, so each user will receive the message based on his language
  • the reason is available also as email header
Actions #12

Updated by Marius BĂLTEANU 2 months ago

I'm not sure about two things:

1. Add all reasons to the email as Pasquale suggested in #note-4.
2. Add a migration that removes from the database the default value for emails_footer if this value was not changed (except the generic link to /my/account).

Any feedback is really appreciated, most of the work is done so we can include this in the next major release.

Actions #13

Updated by Marius BĂLTEANU 2 months ago

Miodrag Milic wrote in #note-8:

Why not adding MENTIONED reason now?

The patch already includes this reason.

Actions #14

Updated by Marius BĂLTEANU 2 months ago

  • Description updated (diff)
Actions #15

Updated by Go MAEDA 2 months ago

How about showing the notification reason in the header instead of the footer? It would be useful to know the reason before you start reading the email to decide if you need to read the entire email.

For example, on some projects I only want to read emails that are assigned to me and don't want to read emails that someone added me as a watcher.

Actions #16

Updated by Marius BĂLTEANU 2 months ago

Thanks for your feedback!

Can you give me an example of such type of mail with reason in the header?

Actions #17

Updated by Holger Just 2 months ago

Some random remarks regarding the patch in #note-11:

  • I'm not a huge fan of storing transient information on a model instance with an attr_accessor (here the notification_reason). This makes it rather hard to reason about what is actual (stored) model data and what is transient data. As this is only used to transport data between "unrelated" code areas, this also causes spooky magic at a distance. If possible, we should send the data directly. For example, we could define a new value class (or decorator or wrapper) which contains a reference to the user and create objects of those when collecting the notified users. We could then pass these new objects to the mailer, rather than just plain users.
    • This could well be the UserNotificationReason class (possibly with a slightly different name then)
    • Maybe this isn't even needed as it appears that we can always (?) detect the notification reason from the method called in the Mailer.deliver_* methods alone. As each called method called to collect notified users by the deliver_* methods returns users for only one notification reason, we could localize this knowledge in the Mailer.deliver_* methods
    • If this is not enough, we could also add new helper methods to the Issue, WikiPage, News, ... classes which wrap the output of the existing methods into the new wrapper class.
  • The UserNotificationReason class should not live in app/models, as it is not a model. It should be in lib/redmine instead.
  • You are using map instead of each multiple times (e.g. in the app/models/issue.rb patch). As map is generally used for its returned value rather than the side-effects of the block, this is confusing.
  • There could be multiple reasons why a user receives an email, such as (1) the mail is high priority and (2) the user is the author and (3) the user is a watcher. We should be able to express these multiple concurrent reasons in the mail headers and the rendered bodies.

What do you think?

Actions #18

Updated by Marius BĂLTEANU about 2 months ago

Holger Just wrote in #note-17:

Some random remarks regarding the patch in #note-11:

Thank you for taking your time to reviewing the patch! I had concerns about the chosen solution, but I started to work at this for so long time that I just wanted to have a first working version.

  • I'm not a huge fan of storing transient information on a model instance with an attr_accessor (here the notification_reason). This makes it rather hard to reason about what is actual (stored) model data and what is transient data. As this is only used to transport data between "unrelated" code areas, this also causes spooky magic at a distance. If possible, we should send the data directly. For example, we could define a new value class (or decorator or wrapper) which contains a reference to the user and create objects of those when collecting the notified users. We could then pass these new objects to the mailer, rather than just plain users.
    • This could well be the UserNotificationReason class (possibly with a slightly different name then)

Something like this?

UserNotificationRecipient as a value object class with two attributes:
  • user: stores the user
  • reason stores the reason

All notified_users methods return UserNotificationRecipient objects instead of User. If we choose this option, we can move more logic related to notifications from User class to UserNotificationRecipient class.

Then, in the Mailer class we can use some kind of service to build the notification recipients by iterating through all the UserNotificationRecipient objects in order to concatenate all the reasons or reorder based on priority reason and remove duplicates.

  • Maybe this isn't even needed as it appears that we can always (?) detect the notification reason from the method called in the Mailer.deliver_* methods alone. As each called method called to collect notified users by the deliver_* methods returns users for only one notification reason, we could localize this knowledge in the Mailer.deliver_* methods

I think that it's not quite easy because some notified_users methods return a mix of reasons. For example, issue.notified_users can return involved users (author, assignee, previous_assignee), users subscripted to project events (project.notified_users) and users notified about high priority issues. If project.notified_users can be extracted, I think the "involved" and "high priority" require more changes. If we want to do this, we can extract each reason in its own method and concatenate all of them in deliver_issue_ methods.

  • If this is not enough, we could also add new helper methods to the Issue, WikiPage, News, ... classes which wrap the output of the existing methods into the new wrapper class.
  • The UserNotificationReason class should not live in app/models, as it is not a model. It should be in lib/redmine instead.

I'll take this into consideration.

  • You are using map instead of each multiple times (e.g. in the app/models/issue.rb patch). As map is generally used for its returned value rather than the side-effects of the block, this is confusing.

I'll review this!

  • There could be multiple reasons why a user receives an email, such as (1) the mail is high priority and (2) the user is the author and (3) the user is a watcher. We should be able to express these multiple concurrent reasons in the mail headers and the rendered bodies.

Just to be double check, you're in favour of displaying all the reasons as Pasquale suggested in #note-4, something like that:

What do you think?

To summarise this, I'm fully open to rework this path and I'm waiting for your feedback before starting again to work on this.

Actions #19

Updated by Dennis Buehring about 1 month ago

Hi,

i wanted to add my two cents to this, because we just had to switch from a redmine_mentions plugin to the builtin functionality, because both where showing the list of users on top of each other.
The plugin sent extra notifications that were easy to spot and treat differently in my mailbox (and all my colleagues), the builtin mentions just sends a regular notification email... everybody just assumed the mentions did not work anymore...

Mentions are a special case, which should be treated differently. I want to inform someone, regardless of his default mail notification options..
If this depends on their settings, why would anyone use this feature ?
If these notifications get lost between the other hundreds of notifications, why would anyone use this feature ?

The plugins did it right, why reinvent the wheel ( as a square ) after ignoring it for several years ?? :)
This feature is shit in its current form, and one should at least be able to disable it to use the plugins again ;)

Actions

Also available in: Atom PDF