[openstreetmap/openstreetmap-website] Remove ability to upload/edit traces from blocked users (PR #4129)

[Andy Allan]

@gravitystorm requested changes on this pull request.



> @@ -61,7 +61,7 @@ Metrics/BlockNesting:
 # Offense count: 26
 # Configuration parameters: CountComments, CountAsOne.
 Metrics/ClassLength:
-  Max: 285
+  Max: 288

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.

>          can [:account, :go_public], User
 
+        can [:new, :create, :edit, :update, :destroy], Trace if user.blocks.active.empty?
+

I would rearrange these to keep the two Trace lines beside each other.

Also, we need to discuss putting the blocking logic into the ability file - it's not done for the existing blocking logic.

> @@ -326,6 +326,10 @@ def deny_access(_exception)
     end
   end
 
+  def deny_html_access_for_current_user(_exception)

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.

> @@ -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

You can use `action_name` to get the current action, in any controller. This might be easier than passing the exception around to get the same information.

> @@ -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?

It's slightly confusing logic here, since it's detecting if the action is 'new' or 'create' and then calling `render_blocked_from_writes` with `action=index`. 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!

I know the intention is to avoid too many translations, but there is hopefully a less convoluted way of achieving the same thing.

> @@ -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

Is the redirect here just to get the flash message to appear? If so, use `flash.now[:warning]` to add the message in the current rendering pass.

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

Message ID: <openstreetmap/openstreetmap-website/pull/4129/review/1580565627 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20230816/0336a574/attachment.htm>