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

[Tom Hughes]

@tomhughes commented on this pull request.

Just a few initial comments, not a detailed review.

> +#  updated_at   :datetime         not null
+#
+# Indexes
+#
+#  index_user_mutes_on_appointee_id                 (appointee_id)
+#  index_user_mutes_on_creator_id                   (creator_id)
+#  index_user_mutes_on_creator_id_and_appointee_id  (creator_id,appointee_id) UNIQUE
+#
+# Foreign Keys
+#
+#  fk_rails_...  (appointee_id => users.id)
+#  fk_rails_...  (creator_id => users.id)
+#
+class UserMute < ApplicationRecord
+  belongs_to :creator, :class_name => "User"
+  belongs_to :appointee, :class_name => "User"

I think `appointee` is a pretty odd term to use for the muted user.

I mean I could argue for `mutor` and `mutee` for the creator and person being muted but that might be taking english a bit far.

Maybe `subject` for the person being muted?

On the other side `creator` isn't terrible but maybe `owner` is better? Not sure either really captures the concept well though.

> +  belongs_to :appointee, :class_name => "User"
+
+  validates :appointee, :uniqueness => { :scope => :creator_id }
+
+  def self.for_message?(message)
+    sender = message.sender
+
+    !sender.administrator? &&
+      !sender.moderator? &&
+      active_for?(
+        :current_user => message.recipient,
+        :other_user => sender
+      )
+  end
+
+  def self.active_for?(current_user:, other_user:)

Why use different names for the users in the argument list here? It's the same concept as the member names so why not use the same names?

Of course once you do that the question becomes is this method even needed when it's just forwarding the arguments to `exists?`?

> @@ -0,0 +1,15 @@
+class CreateUserMutes < ActiveRecord::Migration[7.0]
+  def change
+    create_table :user_mutes do |t|
+      t.references :creator, :null => false

I think we can add `:index => false` here as we have a separate index on creator+appointee for uniqueness so also having one on creator is unnecessary.

> @@ -2900,6 +2912,23 @@ en:
       showing_page: "Page %{page}"
       next: "Next »"
       previous: "« Previous"
+  user_mutes:
+    index:
+      you_have_muted_n_users:
+        zero: "You haven't muted any Users 🎉"
+        one: "You have muted %{count} User"
+        other: "You have muted %{count} Users"
+      user_mute_explainer: "Private messages of muted users are moved into a separate inbox and you won't receive email notifications."
+      user_mute_admins_and_moderators: "You can mute Admins and Moderators but their private messages cannot."

That change is of course equivalent to saying "you can mute them but it has no effect" so should we just not allow them to be muted?

I guess if their status changes later then the mute would suddenly activate but then that raises more questions...

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

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