From cff4c63713e794672532b10d614e4be01a1f4e48 Mon Sep 17 00:00:00 2001 From: Anton Khorev Date: Tue, 13 Aug 2024 13:57:31 +0300 Subject: [PATCH] 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 | 9 +- app/helpers/user_blocks_helper.rb | 2 +- app/models/user_block.rb | 24 ++-- ...70506_add_deactivates_at_to_user_blocks.rb | 5 + db/structure.sql | 4 +- .../user_blocks_controller_test.rb | 128 ++++++++++++++++++ test/factories/user_blocks.rb | 2 + test/integration/user_blocks_test.rb | 6 +- 8 files changed, 165 insertions(+), 15 deletions(-) create mode 100644 db/migrate/20240813070506_add_deactivates_at_to_user_blocks.rb diff --git a/app/controllers/user_blocks_controller.rb b/app/controllers/user_blocks_controller.rb index 962eff04c..73c6ccd69 100644 --- 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 3a55e1cde..e2770eb69 100644 --- 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 4fe50d921..9dd8d3138 100644 --- 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 index 000000000..a1b0fd7ec --- /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 e9d5efc3f..18884b058 100644 --- 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 e6782a64d..3d96199f3 100644 --- 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 6d5c02a03..82c2fbe6e 100644 --- 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 96dd879ee..13db86e28 100644 --- 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 -- 2.39.5