Add deactivates_at date to user blocks

Block deactivation dates that take needs_view-block views into account were derived using updated_at. This was possible because inactive blocks couldn't be edited and their updated_at date wouldn't change. With editing of inactive blocks enabled deactivation date needs to be saved explicitly.

app/controllers/user_blocks_controller.rb

@@ -26,6 +26,7 @@ class UserBlocksController < ApplicationController
   def show
     if current_user && current_user == @user_block.user
       @user_block.needs_view = false
+      @user_block.deactivates_at = [@user_block.ends_at, Time.now.utc].max
       @user_block.save!
     end
   end
@@ -49,6 +50,7 @@ class UserBlocksController < ApplicationController
         :ends_at => now + @block_period.hours,
         :needs_view => params[:user_block][:needs_view]
       )
+      @user_block.deactivates_at = @user_block.ends_at unless @user_block.needs_view
 
       if @user_block.save
         flash[:notice] = t(".flash", :name => @user.display_name)
@@ -72,11 +74,16 @@ class UserBlocksController < ApplicationController
         @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?
           flash.now[:error] = t(".inactive_block_cannot_be_reactivated")
           render :action => "edit"
         else
-          @user_block.ends_at = @user_block.ends_at_was unless user_block_was_active
+          unless user_block_was_active
+            @user_block.ends_at = @user_block.ends_at_was
+            @user_block.deactivates_at = @user_block.deactivates_at_was
+            @user_block.deactivates_at = [@user_block.ends_at, @user_block.updated_at].max unless @user_block.deactivates_at # take updated_at into account before deactivates_at is backfilled
+          end
           if @user_block.save
             flash[:notice] = t(".success")
             redirect_to @user_block

app/helpers/user_blocks_helper.rb

@@ -20,7 +20,7 @@ module UserBlocksHelper
       # 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
       t("user_blocks.helper.time_past_html", :time => friendly_date_ago(last_time))
     end
   end

app/models/user_block.rb

@@ -12,6 +12,7 @@
 #  created_at     :datetime
 #  updated_at     :datetime
 #  reason_format  :enum             default("markdown"), not null
+#  deactivates_at :datetime
 #
 # Indexes
 #
@@ -28,6 +29,8 @@
 class UserBlock < ApplicationRecord
   validate :moderator_permissions
   validates :reason, :characters => true
+  validates :deactivates_at, :comparison => { :greater_than_or_equal_to => :ends_at }, :unless => -> { needs_view }
+  validates :deactivates_at, :absence => true, :if => -> { needs_view }
 
   belongs_to :user, :class_name => "User"
   belongs_to :creator, :class_name => "User"
@@ -67,6 +70,7 @@ class UserBlock < ApplicationRecord
   def revoke!(revoker)
     update(
       :ends_at => Time.now.utc,
+      :deactivates_at => Time.now.utc,
       :revoker_id => revoker.id,
       :needs_view => false
     )

db/migrate/20240813070506_add_deactivates_at_to_user_blocks.rb

@@ -0,0 +1,5 @@
+class AddDeactivatesAtToUserBlocks < ActiveRecord::Migration[7.1]
+  def change
+    add_column :user_blocks, :deactivates_at, :timestamp
+  end
+end

db/structure.sql

@@ -1488,7 +1488,8 @@ CREATE TABLE public.user_blocks (
     revoker_id bigint,
     created_at timestamp without time zone,
     updated_at timestamp without time zone,
-    reason_format public.format_enum DEFAULT 'markdown'::public.format_enum NOT NULL
+    reason_format public.format_enum DEFAULT 'markdown'::public.format_enum NOT NULL,
+    deactivates_at timestamp without time zone
 );
 
 
@@ -3578,6 +3579,7 @@ INSERT INTO "schema_migrations" (version) VALUES
 ('23'),
 ('22'),
 ('21'),
+('20240813070506'),
 ('20240618193051'),
 ('20240605134916'),
 ('20240405083825'),

test/controllers/user_blocks_controller_test.rb

@@ -619,6 +619,134 @@ class UserBlocksControllerTest < ActionDispatch::IntegrationTest
     assert_not_equal moderator_user, expired_block1.revoker
   end
 
+  ##
+  # test changes to end/deactivation dates
+  def test_dates_when_viewed_before_end
+    blocked_user = create(:user)
+    moderator_user = create(:moderator_user)
+
+    freeze_time do
+      session_for(moderator_user)
+      assert_difference "UserBlock.count", 1 do
+        post user_blocks_path(:display_name => blocked_user.display_name,
+                              :user_block_period => "48",
+                              :user_block => { :needs_view => true, :reason => "Testing deactivates_at" })
+      end
+      block = UserBlock.last
+      assert_equal Time.now.utc + 2.days, block.ends_at
+      assert_nil block.deactivates_at
+
+      travel 1.day
+      session_for(blocked_user)
+      get user_block_path(block)
+      block.reload
+      assert_equal Time.now.utc + 1.day, block.ends_at
+      assert_equal Time.now.utc + 1.day, block.deactivates_at
+    end
+  end
+
+  def test_dates_when_viewed_after_end
+    blocked_user = create(:user)
+    moderator_user = create(:moderator_user)
+
+    freeze_time do
+      session_for(moderator_user)
+      assert_difference "UserBlock.count", 1 do
+        post user_blocks_path(:display_name => blocked_user.display_name,
+                              :user_block_period => "24",
+                              :user_block => { :needs_view => true, :reason => "Testing deactivates_at" })
+      end
+      block = UserBlock.last
+      assert_equal Time.now.utc + 1.day, block.ends_at
+      assert_nil block.deactivates_at
+
+      travel 2.days
+      session_for(blocked_user)
+      get user_block_path(block)
+      block.reload
+      assert_equal Time.now.utc - 1.day, block.ends_at
+      assert_equal Time.now.utc, block.deactivates_at
+    end
+  end
+
+  def test_dates_when_edited_before_end
+    blocked_user = create(:user)
+    moderator_user = create(:moderator_user)
+
+    freeze_time do
+      session_for(moderator_user)
+      assert_difference "UserBlock.count", 1 do
+        post user_blocks_path(:display_name => blocked_user.display_name,
+                              :user_block_period => "48",
+                              :user_block => { :needs_view => false, :reason => "Testing deactivates_at" })
+      end
+      block = UserBlock.last
+      assert_equal Time.now.utc + 2.days, block.ends_at
+      assert_equal Time.now.utc + 2.days, block.deactivates_at
+
+      travel 1.day
+      put user_block_path(block,
+                          :user_block_period => "48",
+                          :user_block => { :needs_view => false, :reason => "Testing deactivates_at updated" })
+      block.reload
+      assert_equal Time.now.utc + 2.days, block.ends_at
+      assert_equal Time.now.utc + 2.days, block.deactivates_at
+    end
+  end
+
+  def test_dates_when_edited_after_end
+    blocked_user = create(:user)
+    moderator_user = create(:moderator_user)
+
+    freeze_time do
+      session_for(moderator_user)
+      assert_difference "UserBlock.count", 1 do
+        post user_blocks_path(:display_name => blocked_user.display_name,
+                              :user_block_period => "24",
+                              :user_block => { :needs_view => false, :reason => "Testing deactivates_at" })
+      end
+      block = UserBlock.last
+      assert_equal Time.now.utc + 1.day, block.ends_at
+      assert_equal Time.now.utc + 1.day, block.deactivates_at
+
+      travel 2.days
+      put user_block_path(block,
+                          :user_block_period => "0",
+                          :user_block => { :needs_view => false, :reason => "Testing deactivates_at updated" })
+      block.reload
+      assert_equal Time.now.utc - 1.day, block.ends_at
+      assert_equal Time.now.utc - 1.day, block.deactivates_at
+    end
+  end
+
+  ##
+  # test updates on legacy records without correctly initialized deactivates_at
+  def test_update_legacy_deactivates_at
+    blocked_user = create(:user)
+    moderator_user = create(:moderator_user)
+
+    freeze_time do
+      block = UserBlock.new :user => blocked_user,
+                            :creator => moderator_user,
+                            :reason => "because",
+                            :ends_at => Time.now.utc + 1.day,
+                            :needs_view => false
+
+      assert_difference "UserBlock.count", 1 do
+        block.save :validate => false
+      end
+
+      travel 2.days
+      session_for(moderator_user)
+      put user_block_path(block,
+                          :user_block_period => "0",
+                          :user_block => { :needs_view => false, :reason => "Testing legacy block update" })
+      block.reload
+      assert_equal Time.now.utc - 1.day, block.ends_at
+      assert_equal Time.now.utc - 1.day, block.deactivates_at
+    end
+  end
+
   ##
   # test the blocks_on action
   def test_blocks_on

test/factories/user_blocks.rb

@@ -2,12 +2,14 @@ FactoryBot.define do
   factory :user_block do
     sequence(:reason) { |n| "User Block #{n}" }
     ends_at { Time.now.utc + 1.day }
+    deactivates_at { ends_at }
 
     user
     creator :factory => :moderator_user
 
     trait :needs_view do
       needs_view { true }
+      deactivates_at { nil }
     end
 
     trait :expired do

test/integration/user_blocks_test.rb

@@ -15,7 +15,8 @@ class UserBlocksTest < ActionDispatch::IntegrationTest
       :user_id => blocked_user.id,
       :creator_id => create(:moderator_user).id,
       :reason => "testing",
-      :ends_at => Time.now.utc + 5.minutes
+      :ends_at => Time.now.utc + 5.minutes,
+      :deactivates_at => Time.now.utc + 5.minutes
     )
     get "/api/#{Settings.api_version}/user/details", :headers => basic_authorization_header(blocked_user.display_name, "test")
     assert_response :forbidden
@@ -29,7 +30,8 @@ class UserBlocksTest < ActionDispatch::IntegrationTest
       :user_id => blocked_user.id,
       :creator_id => moderator.id,
       :reason => "testing",
-      :ends_at => Time.now.utc + 5.minutes
+      :ends_at => Time.now.utc + 5.minutes,
+      :deactivates_at => Time.now.utc + 5.minutes
     )
     get "/api/#{Settings.api_version}/user/details", :headers => basic_authorization_header(blocked_user.display_name, "test")
     assert_response :forbidden