<p></p>
<p><b>@gravitystorm</b> requested changes on this pull request.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4129#discussion_r1295871261">.rubocop_todo.yml</a>:</p>
<pre style='color:#555'>> @@ -61,7 +61,7 @@ Metrics/BlockNesting:
 # Offense count: 26
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 285
+  Max: 288
</pre>
<p dir="auto">I'm always happy to see a "foundation-PR" to be done first to refactor whatever is hitting the limit, rather than just increasing the limit! But that's not a blocker, just a comment.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4129#discussion_r1295877458">app/abilities/ability.rb</a>:</p>
<pre style='color:#555'>>          can [:account, :go_public], User
 
+        can [:new, :create, :edit, :update, :destroy], Trace if user.blocks.active.empty?
+
</pre>
<p dir="auto">I would rearrange these to keep the two Trace lines beside each other.</p>
<p dir="auto">Also, we need to discuss putting the blocking logic into the ability file - it's not done for the existing blocking logic.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4129#discussion_r1295882001">app/controllers/application_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -326,6 +326,10 @@ def deny_access(_exception)
     end
   end
 
+  def deny_html_access_for_current_user(_exception)
</pre>
<p dir="auto">Might be worth a comment here noting that it's designed for specific controllers to override this method. Most of the private methods in application_controller are never overridden.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4129#discussion_r1295887735">app/controllers/traces_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -262,6 +262,25 @@ def icon
 
   private
 
+  def deny_html_access_for_current_user(exception)
+    user_block = current_user.blocks.active.take
+    if exception.action == :new || exception.action == :create
</pre>
<p dir="auto">You can use <code class="notranslate">action_name</code> to get the current action, in any controller. This might be easier than passing the exception around to get the same information.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4129#discussion_r1295890406">app/controllers/traces_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -262,6 +262,25 @@ def icon
 
   private
 
+  def deny_html_access_for_current_user(exception)
+    user_block = current_user.blocks.active.take
+    if exception.action == :new || exception.action == :create
+      render_blocked_from_writes user_block, :index unless user_block.nil?
</pre>
<p dir="auto">It's slightly confusing logic here, since it's detecting if the action is 'new' or 'create' and then calling <code class="notranslate">render_blocked_from_writes</code> with <code class="notranslate">action=index</code>. Two things - it's clearly not the correct action, but also I wouldn't expect any writes to be blocked from an 'index' action in the first place!</p>
<p dir="auto">I know the intention is to avoid too many translations, but there is hopefully a less convoluted way of achieving the same thing.</p>

<hr>

<p>In <a href="https://github.com/openstreetmap/openstreetmap-website/pull/4129#discussion_r1295892523">app/controllers/traces_controller.rb</a>:</p>
<pre style='color:#555'>> @@ -262,6 +262,25 @@ def icon
 
   private
 
+  def deny_html_access_for_current_user(exception)
+    user_block = current_user.blocks.active.take
+    if exception.action == :new || exception.action == :create
+      render_blocked_from_writes user_block, :index unless user_block.nil?
+    else
+      render_blocked_from_writes user_block, :show unless user_block.nil?
+    end
+  end
+
+  def render_blocked_from_writes(user_block, action)
+    respond_to do |format|
+      format.html do
+        flash[:warning] = { :partial => "traces/blocked_flash", :locals => { :action => action, :block_link => user_block_path(user_block) } }
+        redirect_to :action => action, :display_name => current_user.display_name
</pre>
<p dir="auto">Is the redirect here just to get the flash message to appear? If so, use <code class="notranslate">flash.now[:warning]</code> to add the message in the current rendering pass.</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/4129#pullrequestreview-1580565627">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AAK2OLOJ2RQPZ7ZNLCCQ6RTXVTCXVANCNFSM6AAAAAA3ACSHSI">unsubscribe</a>.<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AAK2OLO5KED4RKV6RK2IE4LXVTCXVA5CNFSM6AAAAAA3ACSHSKWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTS6GWCHW.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/4129/review/1580565627</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/4129#pullrequestreview-1580565627",
"url": "https://github.com/openstreetmap/openstreetmap-website/pull/4129#pullrequestreview-1580565627",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>