Redmine

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.

+1 because it is a really good idea!

pasquale [:dedalus] wrote:

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

Nice input. I like it.

Why not adding MENTIONED reason now?

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.

Miodrag Milic wrote in #note-8:

Why not adding MENTIONED reason now?

The patch already includes this reason.

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.

Thanks for your feedback!

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

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?

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

I'd like to add a general +1 for moving things out of the ActiveRecord models :)

Few remarks about the patch:

- I believe you could replace all uses of protected with private and everything should still work?
- I'd like to suggest moving the UserNotificationRecipient inside the namespace of the service. It's a class that most certainly won't be used outside of this context so really does not have to live in app/models. How about NotificationRecipients::NotifiedUser?
- the static methods in BuildService might be moved up one level into the `NotificationRecipients` module

Jens, thanks for your feedback, I tried to incorporate your changes in the attached patch (0001-WIP_v2.patch), but I'm not sure about last point because NotificationRecipients ::BuildService was already in the namespace. Please correct me if I'm wrong:

• notification_recipients -> NotificationRecipients
  • notified_user.rb -> NotificationRecipients::NotifiedUser
  • build_service.rb -> NotificationRecipients::BuildService
  • builder
    • base.rb -> NotificationRecipients::Builder::Base
    • issue.rb -> NotificationRecipients::Builder::Issue
    • journal.rb -> NotificationRecipients::Builder::Journal

Once we agree on the structure, I will continue working on the patch.

Thanks Marius for your efforts! Sorry for the late reply, I'll try to check your patch and provide feedback soon.

For now, as a further extension, as requested / mentioned by Patrick Donnelly on IRC, we may want to include the notification reason(s) in a mail header too to allow easy filtering.

Holger Just wrote in #note-24:

For now, as a further extension, as requested / mentioned by Patrick Donnelly on IRC, we may want to include the notification reason(s) in a mail header too to allow easy filtering.

It's already taken care in the patch, it just need to be adapted based on the decision to show only one reason (high highest priority) or all reasons.

Jenk, Holger, did you have the change to take a look on the last proposal?

cc Holger Just and Jens Krämer per #36162#note-26

Hi,

this feature is exactly what we are trying to achive. We are on v5.1.2, and I see that target version is 6.0.0: are there any way to make it work on our version?

Thanks!