https://www.redmine.org/https://www.redmine.org/favicon.ico?16793021292021-11-08T22:10:02ZRedmineRedmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1045612021-11-08T22:10:02ZMarius BĂLTEANU
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/104561/diff?detail_id=84945">diff</a>)</li></ul> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1045642021-11-08T22:15:52ZMarius BĂLTEANU
<ul><li><strong>Related to</strong> <i><a class="issue tracker-2 status-5 priority-4 priority-default closed" href="/issues/13919">Feature #13919</a>: Mention user on issues and wiki pages using @user with autocomplete</i> added</li></ul> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1045862021-11-09T06:48:44ZBernhard Rohloff
<ul></ul><p>To me, this sounds like a good improvement of user experience.<br />It also helps to set the personal notification settings appropriately. <br />+1 from my side.</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1045872021-11-09T07:35:31Zpasquale [:dedalus]
<ul><li><strong>File</strong> <a href="/attachments/28375">bugzilla-sample.png</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/28375/bugzilla-sample.png">bugzilla-sample.png</a> added</li></ul><p>+1 for me.<br />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:</p>
<p><img src="https://www.redmine.org/attachments/download/28375/bugzilla-sample.png" style="width:500px;" alt="" /></p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1045882021-11-09T07:38:50ZC S
<ul></ul><p>+1 because it is a really good idea!</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1045902021-11-09T10:08:59ZBernhard Rohloff
<ul></ul><p>pasquale [:dedalus] wrote:</p>
<blockquote>
<p>... we can summarize with a bullet list as shown in attached screenshot:</p>
</blockquote>
<p>Nice input. I like it.</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1062132022-03-29T05:03:35ZMarius BĂLTEANU
<ul><li><strong>Target version</strong> changed from <i>Candidate for next major release</i> to <i>5.1.0</i></li></ul> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1065102022-04-26T13:20:44ZMiodrag Milic
<ul></ul><p>Why not adding MENTIONED reason now?</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1099072023-05-04T00:10:26ZGo MAEDA
<ul><li><strong>Related to</strong> <i><a class="issue tracker-2 status-1 priority-4 priority-default" href="/issues/38492">Feature #38492</a>: Provide some ways to find the issues where an user Is mentioned </i> added</li></ul> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1108702023-10-02T21:57:02ZGo MAEDA
<ul><li><strong>Target version</strong> changed from <i>5.1.0</i> to <i>6.0.0</i></li></ul> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1123112024-01-17T05:13:22ZMarius BĂLTEANU
<ul><li><strong>File</strong> <a href="/attachments/31813">0001-Add-notification-reason.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/31813/0001-Add-notification-reason.patch">0001-Add-notification-reason.patch</a> added</li><li><strong>Description</strong> updated (<a title="View differences" href="/journals/112311/diff?detail_id=91007">diff</a>)</li></ul><p>I've added the patch that I would like to commit in the following days.</p>
The patch introduces a new class that keeps the <code>UserNotificationReasons</code> and two methods that:
<ul>
<li>return the reason priority as integer</li>
<li>reorder an array of users based on the notifications reason.</li>
</ul>
<p>In <code>mailer.rb</code>, 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.</p>
Also:
<ul>
<li>the text reason from email is translatable now, so each user will receive the message based on his language</li>
<li>the reason is available also as email header</li>
</ul> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1123122024-01-17T05:19:07ZMarius BĂLTEANU
<ul></ul><p>I'm not sure about two things:</p>
<p>1. Add all reasons to the email as Pasquale suggested in <a href="#note-4">#note-4</a>.<br />2. Add a migration that removes from the database the default value for <code>emails_footer</code> if this value was not changed (except the generic link to /my/account).</p>
<p>Any feedback is really appreciated, most of the work is done so we can include this in the next major release.</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1123132024-01-17T05:19:36ZMarius BĂLTEANU
<ul></ul><p>Miodrag Milic wrote in <a href="#note-8">#note-8</a>:</p>
<blockquote>
<p>Why not adding MENTIONED reason now?</p>
</blockquote>
<p>The patch already includes this reason.</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1123152024-01-17T06:52:28ZMarius BĂLTEANU
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/journals/112315/diff?detail_id=91008">diff</a>)</li></ul> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1123222024-01-18T00:59:40ZGo MAEDA
<ul></ul><p>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.</p>
<p>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.</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1123232024-01-18T01:33:27ZMarius BĂLTEANU
<ul></ul><p>Thanks for your feedback!</p>
<p>Can you give me an example of such type of mail with reason in the header?</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1123312024-01-18T14:24:56ZHolger Just
<ul></ul><p>Some random remarks regarding the patch in <a href="#note-11">#note-11</a>:</p>
<ul>
<li>I'm not a huge fan of storing transient information on a model instance with an <code>attr_accessor</code> (here the <code>notification_reason</code>). 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.
<ul>
<li>This could well be the <code>UserNotificationReason</code> class (possibly with a slightly different name then)</li>
<li>Maybe this isn't even needed as it appears that we can always (?) detect the notification reason from the method called in the <code>Mailer.deliver_*</code> methods alone. As each called method called to collect notified users by the <code>deliver_*</code> methods returns users for only one notification reason, we could localize this knowledge in the <code>Mailer.deliver_*</code> methods</li>
<li>If this is not enough, we could also add new helper methods to the <code>Issue</code>, <code>WikiPage</code>, <code>News</code>, ... classes which wrap the output of the existing methods into the new wrapper class.</li>
</ul>
</li>
<li>The <code>UserNotificationReason</code> class should not live in app/models, as it is not a model. It should be in <code>lib/redmine</code> instead.</li>
<li>You are using <code>map</code> instead of <code>each</code> multiple times (e.g. in the <code>app/models/issue.rb</code> patch). As <code>map</code> is generally used for its returned value rather than the side-effects of the block, this is confusing.</li>
<li>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.</li>
</ul>
<p>What do you think?</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1124032024-01-22T12:12:10ZMarius BĂLTEANU
<ul><li><strong>File</strong> <a href="/attachments/31837">all_reasons.png</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/31837/all_reasons.png">all_reasons.png</a> added</li></ul><p>Holger Just wrote in <a href="#note-17">#note-17</a>:</p>
<blockquote>
<p>Some random remarks regarding the patch in <a href="#note-11">#note-11</a>:</p>
</blockquote>
<p>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.</p>
<blockquote>
<ul>
<li>I'm not a huge fan of storing transient information on a model instance with an <code>attr_accessor</code> (here the <code>notification_reason</code>). 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.
<ul>
<li>This could well be the <code>UserNotificationReason</code> class (possibly with a slightly different name then)</li>
</ul></li>
</ul>
</blockquote>
<p>Something like this?</p>
<code>UserNotificationRecipient</code> as a value object class with two attributes:
<ul>
<li><code>user</code>: stores the user</li>
<li><code>reason</code> stores the reason</li>
</ul>
<p>All <code>notified_users</code> methods return <code>UserNotificationRecipient</code> objects instead of <code>User</code>. If we choose this option, we can move more logic related to notifications from <code>User</code> class to <code>UserNotificationRecipient</code> class.</p>
<p>Then, in the <code>Mailer</code> class we can use some kind of service to build the notification recipients by iterating through all the <code>UserNotificationRecipient</code> objects in order to concatenate all the reasons or reorder based on priority reason and remove duplicates.</p>
<blockquote>
<ul>
<li>Maybe this isn't even needed as it appears that we can always (?) detect the notification reason from the method called in the <code>Mailer.deliver_*</code> methods alone. As each called method called to collect notified users by the <code>deliver_*</code> methods returns users for only one notification reason, we could localize this knowledge in the <code>Mailer.deliver_*</code> methods</li>
</ul>
</blockquote>
<p>I think that it's not quite easy because some <code>notified_users</code> methods return a mix of reasons. For example, <code>issue.notified_users</code> can return involved users (author, assignee, previous_assignee), users subscripted to project events (<code>project.notified_users</code>) and users notified about high priority issues. If <code>project.notified_users</code> 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 <code>deliver_issue_</code> methods.</p>
<blockquote>
<ul>
<li>If this is not enough, we could also add new helper methods to the <code>Issue</code>, <code>WikiPage</code>, <code>News</code>, ... classes which wrap the output of the existing methods into the new wrapper class.</li>
</ul>
<ul>
<li>The <code>UserNotificationReason</code> class should not live in app/models, as it is not a model. It should be in <code>lib/redmine</code> instead.</li>
</ul>
</blockquote>
<p>I'll take this into consideration.</p>
<blockquote>
<ul>
<li>You are using <code>map</code> instead of <code>each</code> multiple times (e.g. in the <code>app/models/issue.rb</code> patch). As <code>map</code> is generally used for its returned value rather than the side-effects of the block, this is confusing.</li>
</ul>
</blockquote>
<p>I'll review this!</p>
<blockquote>
<ul>
<li>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.</li>
</ul>
</blockquote>
<p>Just to be double check, you're in favour of displaying all the reasons as Pasquale suggested in <a href="#note-4">#note-4</a>, something like that:</p>
<p><img src="https://www.redmine.org/attachments/download/31837/all_reasons.png" style="border: 1px solid gray;width: 80%;" alt="" /></p>
<blockquote>
<p>What do you think?</p>
</blockquote>
<p>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.</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1129392024-02-16T10:06:22ZDennis Buehring
<ul></ul><p>Hi,</p>
<p>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.<br />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...</p>
<p>Mentions are a special case, which should be treated differently. I want to inform someone, regardless of his default mail notification options.. <br />If this depends on their settings, why would anyone use this feature ? <br />If these notifications get lost between the other hundreds of notifications, why would anyone use this feature ?</p>
<p>The plugins did it right, why reinvent the wheel ( as a square ) after ignoring it for several years ?? :)<br />This feature is shit in its current form, and one should at least be able to disable it to use the plugins again ;)</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1132292024-03-19T23:18:23ZMarius BĂLTEANU
<ul><li><strong>File</strong> <a href="/attachments/32092">0001-WIP.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/32092/0001-WIP.patch">0001-WIP.patch</a> added</li></ul><p><a class="user active" href="https://www.redmine.org/users/2784">Holger Just</a>, I made a patch (WIP - only for demo purposes) with an alternative implementation using Service Objects. I know that we are not currently using this kind of pattern, but I think it will be useful to start using it in order to cleanup a little bit the models. Also, the patch addresses only the "design" issue caused by <code>attr_accessor</code>.</p>
The patch contains the following:
<ul>
<li>The <code>UserNotificationRecipient</code> that stores the <code>User</code> and the <code>reason</code></li>
<li>A <code>BuildService</code> that have static methods for each <code>Mailer.deliver_*</code> method and that return all the recipients</li>
<li>A <code>Builder::Base</code> that is extended by builders for each object, for example <code>Builder::Issue</code> or <code>Builder::Journal</code></li>
<li>All <code>Builder::*</code> methods return <code>UserNotificationRecipient</code> objects.</li>
</ul>
<p>In theory, if we move forward with this implementation, we can get rid of all the methods related to notifications from models. Beside this new pattern, another disadvantage is the change that could be quite big.</p>
<p>What do you think? Also, if you have in mind another solution that require less changes, please let me know (if you can show your ideea with a quick patch, it will be great).</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1132582024-03-25T06:13:45ZJens Krämerjk@jkraemer.net
<ul></ul><p>I'd like to add a general +1 for moving things out of the ActiveRecord models :)</p>
<p>Few remarks about the patch:</p>
<p>- I believe you could replace all uses of <code>protected</code> with <code>private</code> and everything should still work?<br />- I'd like to suggest moving the <code>UserNotificationRecipient</code> 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 <code>app/models</code>. How about <code>NotificationRecipients::NotifiedUser</code>?<br />- the static methods in <code>BuildService</code> might be moved up one level into the `NotificationRecipients` module</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1132682024-03-25T21:08:57ZMarius BĂLTEANU
<ul></ul><p>Jens, thanks for your feedback, I tried to incorporate your changes in the attached patch (<a class="attachment" href="https://www.redmine.org/attachments/32115">0001-WIP_v2.patch</a>), but I'm not sure about last point because <code>NotificationRecipients ::BuildService</code> was already in the namespace. Please correct me if I'm wrong:</p>
<ul>
<li>notification_recipients -> <code>NotificationRecipients</code>
<ul>
<li>notified_user.rb -> <code>NotificationRecipients::NotifiedUser</code></li>
<li>build_service.rb -> <code>NotificationRecipients::BuildService</code></li>
<li>builder
<ul>
<li>base.rb -> <code>NotificationRecipients::Builder::Base</code></li>
<li>issue.rb -> <code>NotificationRecipients::Builder::Issue</code></li>
<li>journal.rb -> <code>NotificationRecipients::Builder::Journal</code></li>
</ul></li>
</ul></li>
</ul>
<p>Once we agree on the structure, I will continue working on the patch.</p> Redmine - Feature #36162: Add notification reason to the email instead of the default static email footerhttps://www.redmine.org/issues/36162?journal_id=1132692024-03-25T21:10:19ZMarius BĂLTEANU
<ul><li><strong>File</strong> <a href="/attachments/32115">0001-WIP_v2.patch</a> <a class="icon-only icon-download" title="Download" href="/attachments/download/32115/0001-WIP_v2.patch">0001-WIP_v2.patch</a> added</li></ul>