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

[Andy Allan]

@gravitystorm commented on this pull request.

I'm sorry I haven't had the opportunity to do a full review - this is my initial thoughts from reading the code, without trying out the interfaces. Happy to discuss any of these points.

> @@ -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'm not a big fan of this change. My personal preference is for models to be unaware of things like sending notifications - I think it should be the controller that decides what is the list of things that should be done.

We don't have a proper notifier class, so controllers currently call the Mailer (since our only notification channel is, currently, via email). I'm mostly ambivalent about where the logic check goes for "should this notification be sent" - ideally it would be in the notifier, perhaps for now it can live in the Mailer (although I can accept the controller).

> @@ -47,13 +47,14 @@ def initialize(user)
         can [:show], :dashboard
         can [:new, :create, :edit, :update, :comment, :subscribe, :unsubscribe], DiaryEntry
         can [:make_friend, :remove_friend], Friendship
-        can [:new, :create, :reply, :show, :inbox, :outbox, :mark, :destroy], Message
+        can [:new, :create, :reply, :show, :inbox, :outbox, :muted, :mark, :unmute, :destroy], Message

I've been trying to refactor us away from having long lists of non-crud methods on controllers. See http://jeromedalbert.com/how-dhh-organizes-his-rails-controllers/ for background on this idea.

I know that Messages isn't the best example currently, so I'd be happy with you following the same path. If you have ideas on better controllers for all these different actions (e.g. InboxController::index) then I'm interested to hear more.

> @@ -107,6 +107,13 @@ def outbox
     @title = t ".title"
   end
 
+  # Display the list of muted messages received by the user.
+  def muted
+    @title = t ".title"
+
+    redirect_to inbox_messages_path if current_user.muted_messages.none?

I think this should just show a "no muted messages" rather than a redirect.

> +# Indexes
+#
+#  index_user_mutes_on_owner_id_and_subject_id  (owner_id,subject_id) UNIQUE
+#
+# Foreign Keys
+#
+#  fk_rails_...  (owner_id => users.id)
+#  fk_rails_...  (subject_id => users.id)
+#
+class UserMute < ApplicationRecord
+  belongs_to :owner, :class_name => "User"
+  belongs_to :subject, :class_name => "User"
+
+  validates :subject, :uniqueness => { :scope => :owner_id, :message => :is_already_muted }
+
+  def self.for_message?(message)

This might be one level of indirection too far - the message object isn't being used for anything here, other than a sort of container for the two users. It feels like a method that takes the two users (as named parameters) could work better?

> @@ -35,6 +35,8 @@ class Application < Rails::Application
     # Enable locale fallbacks for I18n (makes lookups for any locale fall back to
     # the I18n.default_locale when a translation cannot be found).
     config.i18n.fallbacks = true
+    # Enables custom error message formats
+    config.active_model.i18n_customize_full_message = true

Nice, this looks useful for other things too!

> +    create(:user_mute, :owner => user, :subject => muted_user)
+
+    message = build(:message, :sender => muted_user, :recipient => user)
+    message.save
+
+    assert_predicate message, :muted?
+  end
+
+  def test_messages_by_admins_or_moderators_are_never_muted
+    user = create(:user)
+
+    [create(:administrator_user), create(:moderator_user)].each do |admin_or_moderator|
+      create(:user_mute, :owner => user, :subject => admin_or_moderator)
+
+      message = build(:message, :sender => admin_or_moderator, :recipient => user)
+      message.save

Is `build` followed by `save` the same as `create`?

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

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