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

[Andy Allan]

@gravitystorm requested changes on this pull request.

In general this looks good to me, just a few more notes on things I would like to see before it's ready to go.

> @@ -0,0 +1,13 @@
+<table class="table table-sm align-middle">
+  <thead>
+    <tr>
+      <th><%= t ".from" %></th>
+      <th><%= t ".subject" %></th>
+      <th><%= t ".date" %></th>
+      <th class="d-flex justify-content-end"><%= t ".actions" %></th>
+    </tr>
+  </thead>
+  <tbody>
+    <%= render :partial => inner_partial, :collection => messages %>

There is a level of indirection here with isn't used, since "inner_partial" is always equal to "message_summary". I guess what might have happened is that you started looking at the two different contents for this table - inbox and outbox being slightly different - but you haven't actually refactored the outbox, perhaps because the headings are different too.

So I think either more refactoring is required (so that the inner_partial is useful) or this level of indirection can be removed.

> @@ -2906,6 +2927,30 @@ en:
       showing_page: "Page %{page}"
       next: "Next »"
       previous: "« Previous"
+  user_mutes:
+    index:
+      title: "Muted Users"
+      my_muted_users: "My muted users"
+      you_have_muted_no_users: "You have not muted any users 🎉"

No celebration emoji here please! I'm no big fan of emojis (they are an i18n nightmare), but this one should be used when something good happens to you, or you've taken a successful action, so it doesn't really fit the context of "nothing bad happened yet".

> @@ -2906,6 +2927,30 @@ en:
       showing_page: "Page %{page}"
       next: "Next »"
       previous: "« Previous"
+  user_mutes:
+    index:
+      title: "Muted Users"
+      my_muted_users: "My muted users"
+      you_have_muted_no_users: "You have not muted any users 🎉"
+      you_have_muted_n_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."

We don't use the term "private messages" anywhere else in the UI, only "messages". So this could be confusing if someone is trying to work out if "private messages" are different from regular messages.

I think "private" can simply be removed in these two translations.

> @@ -289,6 +292,11 @@
   get "/message/new/:display_name" => "messages#new", :as => "new_message"
   get "/message/read/:message_id", :to => redirect(:path => "/messages/%{message_id}")
 
+  # muting users
+  post "/user/:display_name/mute" => "user_mutes#create", :as => :mute_user
+  delete "/user/:display_name/mute" => "user_mutes#destroy", :as => :unmute_user

If these can be added as resourceful routes then that would be better. Or at lest, specific actions on a resource or scoped resource. Something like (untested)

```ruby
scope "/user/:display_name" do
  resources :user_mutes, :only -> [:create, :destroy]
end
```

We're (slowly) getting rid of all the "full handwritten" routes so I'm keen to avoid adding more.

> @@ -0,0 +1,31 @@
+require "test_helper"
+
+class UserMutingTest < ActionDispatch::IntegrationTest

This should perhaps be a system test rather than an integration test, where we can use more natural things like "click_button" rather than `assert_select` followed by directly posting to controllers.

> @@ -0,0 +1,31 @@
+require "test_helper"
+
+class UserMutingTest < ActionDispatch::IntegrationTest
+  def test_muted_messages_are_moved_into_separate_inbox
+    user = create(:user)
+    other_user = create(:user)
+    user.mutes.create(:subject => other_user)
+
+    session_for(user)
+    get inbox_messages_path
+    assert_select "a[href='#{muted_messages_path}']", false
+
+    session_for(other_user)

I think just `create(:message, ....)` rather than switching sessions here

> @@ -0,0 +1,26 @@
+require "test_helper"
+
+class UserMuteTest < ActiveSupport::TestCase
+  def test_messages_by_muted_users_are_muted
+    user = create(:user)
+    muted_user = create(:user)
+    create(:user_mute, :owner => user, :subject => muted_user)
+
+    message = build(:message, :sender => muted_user, :recipient => user)
+    message.save

`create` rather than `build` followed immediately by `save`?

> @@ -14,5 +14,8 @@
     <li class="nav-item">
       <%= link_to t(".oauth2_authorizations"), oauth_authorized_applications_path, :class => "nav-link #{'active' if controller_name == 'oauth2_authorized_applications'}" %>
     </li>
+    <li class="nav-item">
+      <%= link_to t(".muted_users"), user_mutes_path, :class => "nav-link #{'active' if controller_name == 'user_mutes'}" %>

We hide the "Muted messages" tab when there's no muted messages. Should we also hide the "Muted users" tab when you haven't muted anyone yet? There's not much value for this tab until you do (and the muting happens elsewhere), and most people are (hopefully) never going to need it.

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

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