[openstreetmap/openstreetmap-website] Add deactivates_at date to user blocks (PR #5069)

[Tom Hughes]

@tomhughes commented on this pull request.



>        else
-        render :action => "edit"
+        user_block_was_active = @user_block.active?
+        @user_block.reason = params[:user_block][:reason]
+        @user_block.needs_view = params[:user_block][:needs_view]
+        @user_block.ends_at = Time.now.utc + @block_period.hours
+        @user_block.deactivates_at = (@user_block.ends_at unless @user_block.needs_view)

I don't think this needs parentheses does it?

>        else
-        render :action => "edit"
+        user_block_was_active = @user_block.active?
+        @user_block.reason = params[:user_block][:reason]
+        @user_block.needs_view = params[:user_block][:needs_view]
+        @user_block.ends_at = Time.now.utc + @block_period.hours
+        @user_block.deactivates_at = (@user_block.ends_at unless @user_block.needs_view)
+        if !user_block_was_active && !@user_block.active?
+          @user_block.ends_at = @user_block.ends_at_was
+          @user_block.deactivates_at = @user_block.deactivates_at_was
+        end
+        @user_block.deactivates_at = [@user_block.ends_at, @user_block.updated_at].max if !@user_block.deactivates_at && !@user_block.needs_view
+
+        if @user_block.save
+          flash[:notice] = t(".success")
+          redirect_to(@user_block)

Does this needs parentheses?

>        else
-        render :action => "edit"
+        user_block_was_active = @user_block.active?
+        @user_block.reason = params[:user_block][:reason]
+        @user_block.needs_view = params[:user_block][:needs_view]
+        @user_block.ends_at = Time.now.utc + @block_period.hours
+        @user_block.deactivates_at = (@user_block.ends_at unless @user_block.needs_view)
+        if !user_block_was_active && !@user_block.active?
+          @user_block.ends_at = @user_block.ends_at_was
+          @user_block.deactivates_at = @user_block.deactivates_at_was
+        end
+        @user_block.deactivates_at = [@user_block.ends_at, @user_block.updated_at].max if !@user_block.deactivates_at && !@user_block.needs_view

I don't think this needs both conditions does it? The only way `deactivates_at` can be unset is it `needs_view` is false?

The whole logic here is really hard to grasp and reason about to be honest and I wonder if there's a better way to do it or if we should even be allowing all the cases this is allowing - should we be allowed an expired block to be reactivated?

> @@ -20,7 +20,7 @@ def block_status(block)
       # the max of the last update time or the ends_at time is when this block finished
       # either because the user viewed the block (updated_at) or it expired or was
       # revoked (ends_at)
-      last_time = [block.ends_at, block.updated_at].max
+      last_time = block.deactivates_at || [block.ends_at, block.updated_at].max

Do we need the second half of this? It's just for old blocks because we're not backfilling `deactivates_at` for them?

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

Message ID: <openstreetmap/openstreetmap-website/pull/5069/review/2236228362 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240813/fade3a9f/attachment.htm>