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.

diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb
index 962eff04ce06306cb97906d9d4a0edf2c126e4bc..73c6ccd69e1b73da6003217449d4dd0f71c7528d 100644 (file)
--- a/app/controllers/user_blocks_controller.rb
+++ b/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

diff --git a/app/helpers/user_blocks_helper.rb b/app/helpers/user_blocks_helper.rb
index 3a55e1cdedfcc12d9f3d02ca1135ed7f8f339feb..e2770eb69ea5bf655866b04499708eadd70a575e 100644 (file)
--- a/app/helpers/user_blocks_helper.rb
+++ b/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

diff --git a/app/models/user_block.rb b/app/models/user_block.rb
index 4fe50d921219320f04426eb82394d6e2a7d0cffc..9dd8d3138cefb11c887255371f01ec7b9bc2e939 100644 (file)
--- a/app/models/user_block.rb
+++ b/app/models/user_block.rb
@@ -2,16 +2,17 @@
 #
 # Table name: user_blocks
 #
-#  id            :integer          not null, primary key
-#  user_id       :bigint(8)        not null
-#  creator_id    :bigint(8)        not null
-#  reason        :text             not null
-#  ends_at       :datetime         not null
-#  needs_view    :boolean          default(FALSE), not null
-#  revoker_id    :bigint(8)
-#  created_at    :datetime
-#  updated_at    :datetime
-#  reason_format :enum             default("markdown"), not null
+#  id             :integer          not null, primary key
+#  user_id        :bigint(8)        not null
+#  creator_id     :bigint(8)        not null
+#  reason         :text             not null
+#  ends_at        :datetime         not null
+#  needs_view     :boolean          default(FALSE), not null
+#  revoker_id     :bigint(8)
+#  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
     )

diff --git a/db/migrate/20240813070506_add_deactivates_at_to_user_blocks.rb b/db/migrate/20240813070506_add_deactivates_at_to_user_blocks.rb
new file mode 100644 (file)
index 0000000..a1b0fd7
--- /dev/null
+++ b/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

diff --git a/db/structure.sql b/db/structure.sql
index e9d5efc3f9b8d71b40e965226bd83caab39d49c5..18884b058a95e51fb35feeb4dc28ff3c9b5940e9 100644 (file)
--- a/db/structure.sql
+++ b/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'),

diff --git a/test/controllers/user_blocks_controller_test.rb b/test/controllers/user_blocks_controller_test.rb
index e6782a64d0d3910393c70b026f1350b738211e3e..3d96199f397f56e705eb6ca058ae6f3d4fad5c20 100644 (file)
--- a/test/controllers/user_blocks_controller_test.rb
+++ b/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

diff --git a/test/factories/user_blocks.rb b/test/factories/user_blocks.rb
index 6d5c02a03614d64a00d54016f33598a56709ac52..82c2fbe6e8f694b87578e1dbbe2bdb9f8726c667 100644 (file)
--- a/test/factories/user_blocks.rb
+++ b/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

diff --git a/test/integration/user_blocks_test.rb b/test/integration/user_blocks_test.rb
index 96dd879ee08273309e9cba75cffad8ec131e5551..13db86e28134ecf899e19d21a147dfac5fc90d8c 100644 (file)
--- a/test/integration/user_blocks_test.rb
+++ b/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