[openstreetmap/openstreetmap-website] Add UserMute to control private message visibility (PR #4284)

[Gregory Igelmund]

@grekko commented on this pull request.



> @@ -47,7 +47,7 @@ def create
       render :action => "new"
     elsif @message.save
       flash[:notice] = t ".message_sent"
-      UserMailer.message_notification(@message).deliver_later
+      @message.notify_recipient_via_mail

> I think it should be the controller that decides what is the list of things that should be done.

I think that is still the case. The controller still makes the call, but now there is this extra logic which is imo a detail that the controller does not need to bother about.

I understand though that currently all direct calls to the `UserMailer` are made either from within the context of a Controller or Job and my change deviates from that pattern.

To me it is important that the logic wether a E-Mail Notification should be sent is kept in a single place and is not spread across the application code.

What do you think about this version?

```diff
diff --git a/app/controllers/messages_controller.rb b/app/controllers/messages_controller.rb
index 21d08e7af..2ca86fc02 100644
--- a/app/controllers/messages_controller.rb
+++ b/app/controllers/messages_controller.rb
@@ -47,7 +47,7 @@ class MessagesController < ApplicationController
       render :action => "new"
     elsif @message.save
       flash[:notice] = t ".message_sent"
-      @message.notify_recipient_via_mail
+      UserMailer.message_notification(@message).deliver_later if @message.notify_recipient?
       redirect_to :action => :inbox
     else
       @title = t "messages.new.title"
diff --git a/app/models/message.rb b/app/models/message.rb
index 1be482cfb..fdeb89c4b 100644
--- a/app/models/message.rb
+++ b/app/models/message.rb
@@ -74,10 +74,8 @@ class Message < ApplicationRecord
     md5.hexdigest
   end
 
-  def notify_recipient_via_mail
-    return false if muted?
-
-    UserMailer.message_notification(self).deliver_later
+  def notify_recipient?
+    !muted?
   end
 
   def unmute
diff --git a/script/deliver-message b/script/deliver-message
index a382bffa1..496773597 100755
--- a/script/deliver-message
+++ b/script/deliver-message
@@ -33,6 +33,6 @@ mail = Mail.new($stdin.read
 message = Message.from_mail(mail, from, to)
 message.save!
 
-message.notify_recipient_via_mail
+UserMailer.message_notification(message).deliver_later if message.notify_recipient?
 
 exit 0
```

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1373619275
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/4284/review/1700339750 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20231026/1f98015f/attachment-0001.htm>