<p></p>
<p><b>@gravitystorm</b> requested changes on this pull request.</p>
<p dir="auto">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.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1403132276">app/views/messages/_messages_table.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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 %>
</pre>
<p dir="auto">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.</p>
<p dir="auto">So I think either more refactoring is required (so that the inner_partial is useful) or this level of indirection can be removed.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1403134557">config/locales/en.yml</a>:</p>
<pre style='color:#555'>> @@ -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 🎉"
</pre>
<p dir="auto">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".</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1403135816">config/locales/en.yml</a>:</p>
<pre style='color:#555'>> @@ -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."
</pre>
<p dir="auto">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.</p>
<p dir="auto">I think "private" can simply be removed in these two translations.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1403142119">config/routes.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p dir="auto">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)</p>
<div class="highlight highlight-source-ruby" dir="auto"><pre class="notranslate"><span class="pl-en">scope</span> <span class="pl-s">"/user/:display_name"</span> <span class="pl-k">do</span>
<span class="pl-en">resources</span> <span class="pl-pds">:user_mutes</span><span class="pl-kos">,</span> <span class="pl-pds">:only</span> -> <span class="pl-kos">[</span><span class="pl-pds">:create</span><span class="pl-kos">,</span> <span class="pl-pds">:destroy</span><span class="pl-kos">]</span>
<span class="pl-k">end</span></pre></div>
<p dir="auto">We're (slowly) getting rid of all the "full handwritten" routes so I'm keen to avoid adding more.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1403143805">test/integration/user_muting_test.rb</a>:</p>
<pre style='color:#555'>> @@ -0,0 +1,31 @@
+require "test_helper"
+
+class UserMutingTest < ActionDispatch::IntegrationTest
</pre>
<p dir="auto">This should perhaps be a system test rather than an integration test, where we can use more natural things like "click_button" rather than <code class="notranslate">assert_select</code> followed by directly posting to controllers.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1403144819">test/integration/user_muting_test.rb</a>:</p>
<pre style='color:#555'>> @@ -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)
</pre>
<p dir="auto">I think just <code class="notranslate">create(:message, ....)</code> rather than switching sessions here</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1403146285">test/models/user_mute_test.rb</a>:</p>
<pre style='color:#555'>> @@ -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
</pre>
<p dir="auto"><code class="notranslate">create</code> rather than <code class="notranslate">build</code> followed immediately by <code class="notranslate">save</code>?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1403148115">app/views/application/_settings_menu.html.erb</a>:</p>
<pre style='color:#555'>> @@ -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'}" %>
</pre>
<p dir="auto">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.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#pullrequestreview-1746052823">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLKKDDQUGWV2SFKYK3LYF4MQDAVCNFSM6AAAAAA53SDOR2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTONBWGA2TEOBSGM">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLOBJQOCFBHXKECRMCLYF4MQDA5CNFSM6AAAAAA53SDOR2WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTICKTNO.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><openstreetmap/openstreetmap-website/pull/4284/review/1746052823</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/openstreetmap/openstreetmap-website/pull/4284#pullrequestreview-1746052823",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4284#pullrequestreview-1746052823",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>