<p></p>
<p><b>@tomhughes</b> requested changes on this pull request.</p>
<p dir="auto">In addition to the specific comments I've made, a few general observations/questions...</p>
<p dir="auto">Firstly I think we should have a "mute user" button on the detail view (messages#show) for messages to make it more obvious that the option is available when a problem arises? Probably we should also have report user (or ideally report message) but that's not really in scope for this project.</p>
<p dir="auto">Secondly should we have a mute/unmute button on the main message view - currently messages can only be unmuted from the index page and there's no way to mute a message at all if the user is not muted when it is received.</p><hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1367751760">app/controllers/messages_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -127,6 +134,23 @@ def mark
render :action => "no_such_message", :status => :not_found
end
+ # Moves message into Inbox by unsetting the muted-flag
+ def unmute
+ message = Message.where(:recipient => current_user).find(params[:message_id])
</pre>
<p dir="auto">This never actually changes the state of the message, so nothing happens when you click the <code class="notranslate">Unmute</code> button for a message in the user interface.</p>
<p dir="auto">The condition on the recipient is not strictly necessary as message IDs are globally unique, and we prefer <code class="notranslate">find_by</code> now though for some reason rubocop hasn't picked that up.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1367752489">app/controllers/user_mutes_controller.rb</a>:</p>
<pre style='color:#555'>> + @title = t ".title"
+ end
+
+ def create
+ user_mute = UserMute.new(
+ :owner => current_user,
+ :subject => @user
+ )
+
+ if user_mute.save
+ flash[:notice] = t(".notice", :name => user_mute.subject.display_name)
+ else
+ flash[:error] = t(".error")
+ end
+
+ redirect_back :fallback_location => user_mutes_path(current_user)
</pre>
<p dir="auto">It seems <code class="notranslate">redirect_back_or_to</code> is now preferred over <code class="notranslate">redirect_back</code> (that might be new in rails 7.1) though both work for now.</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1367753338">app/views/user_mutes/index.html.erb</a>:</p>
<pre style='color:#555'>> + <thead>
+ <tr>
+ <th><%= t ".muted_user" %></th>
+ <th class="d-flex justify-content-end"><%= t ".actions" %></th>
+ </tr>
+ </thead>
+ <tbody>
+ <% @muted_users.each do |user| %>
+ <tr>
+ <td>
+ <%= user_thumbnail_tiny user %>
+ <%= link_to user.display_name, user_path(user) %>
+ </td>
+ <td class="text-nowrap text-end">
+ <%= link_to t(".unmute"), unmute_user_path(user), :method => :delete, :class => "btn btn-sm btn-primary" %>
+ <%= link_to t(".send_message"), new_message_path(user), :class => "btn btn-sm btn-secondary" %>
</pre>
<p dir="auto">An option to send a message to a muted user seems a bit strange when by definition any reply will wind up in the muted messages and won't be notified?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1367753599">config/locales/en.yml</a>:</p>
<pre style='color:#555'>> @@ -1724,9 +1729,16 @@ en:
wrong_user: "You are logged in as `%{user}' but the message you have asked to read was not sent by or to that user. Please login as the correct user in order to read it."
sent_message_summary:
destroy_button: "Delete"
+ heading:
+ my_inbox: "My Inbox"
+ my_outbox: "My Outbox"
+ muted_messages: "Muted messages"
</pre>
<p dir="auto">Should the capitalisation be consistent here given these are three labels that sit adjacent to each other?</p>
<hr>
<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4284#discussion_r1367753695">config/locales/en.yml</a>:</p>
<pre style='color:#555'>> @@ -2900,6 +2915,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"
</pre>
<p dir="auto">Why are we capitalising User/Users here?</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-1691281876">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLPKJW2E5FFPGX2L2RTYAPY2FAVCNFSM6AAAAAA53SDOR2VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMYTMOJRGI4DCOBXGY">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLLB2EIUIYSLVN4ZIITYAPY2FA5CNFSM6AAAAAA53SDOR2WGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTTEZ3U5I.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/1691281876</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-1691281876",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4284#pullrequestreview-1691281876",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>