From 7d503f088e732e4da27447d40990707eae112158 Mon Sep 17 00:00:00 2001 From: evazion Date: Thu, 7 Oct 2021 05:55:43 -0500 Subject: [PATCH] posts: stop using pool_string attribute. Stop using the pool_string attribute on posts: * Stop updating it when adding or removing posts from pools. * Stop returning pool_string in the /posts.json API. * Stop including the `data-pools` attribute on thumbnails. The pool_string attribute was used in the past to facilitate pool:X searches. Posts had a hidden pool_string attribute that contained a list of every pool the post belonged to. These pools were treated like fake hidden tags on the post and a search for `pool:X` was treated like a tag search. The pool_string has no longer been used for this purpose for a long time now, and was only maintained for API compatibility purposes. Getting rid of it eliminates a bunch of legacy cruft relating to adding and removing posts from pools. If you need to see which pools a post belongs to, do this: * https://danbooru.donmai.us/pools.json?search[post_ids_include_any]=318550 The `data-pools` attribute on thumbnails was used by some people to add custom borders to pooled posts with custom CSS. This will no longer work. This was already broken because it included things like collection pools and deleted pools, which you probably didn't want. Use a userscript to add this attribute back to thumbnails if you need it. --- app/components/post_preview_component.rb | 1 - app/controllers/pools_controller.rb | 5 +- app/models/pool.rb | 28 +---------- app/models/post.rb | 41 +++------------- test/unit/pool_test.rb | 60 +----------------------- test/unit/post_test.rb | 37 +-------------- test/unit/post_version_test.rb | 2 +- 7 files changed, 13 insertions(+), 161 deletions(-) diff --git a/app/components/post_preview_component.rb b/app/components/post_preview_component.rb index 80ea2205e..8a272613a 100644 --- a/app/components/post_preview_component.rb +++ b/app/components/post_preview_component.rb @@ -69,7 +69,6 @@ class PostPreviewComponent < ApplicationComponent "data-id" => post.id, "data-has-sound" => post.has_tag?("sound"), "data-tags" => post.tag_string, - "data-pools" => post.pool_string, "data-approver-id" => post.approver_id, "data-rating" => post.rating, "data-large-width" => post.large_image_width, diff --git a/app/controllers/pools_controller.rb b/app/controllers/pools_controller.rb index c6ed65d98..d307424ba 100644 --- a/app/controllers/pools_controller.rb +++ b/app/controllers/pools_controller.rb @@ -45,11 +45,8 @@ class PoolsController < ApplicationController end def update - # need to do this in order for synchronize! to work correctly @pool = authorize Pool.find(params[:id]) - @pool.attributes = permitted_attributes(@pool) - @pool.synchronize - @pool.save + @pool.update(permitted_attributes(@pool)) unless @pool.errors.any? flash[:notice] = "Pool updated" end diff --git a/app/models/pool.rb b/app/models/pool.rb index 01995915c..f8da79b7c 100644 --- a/app/models/pool.rb +++ b/app/models/pool.rb @@ -10,7 +10,6 @@ class Pool < ApplicationRecord validate :updater_can_edit_deleted before_validation :normalize_post_ids before_validation :normalize_name - after_create :synchronize! after_save :create_version deletable @@ -130,7 +129,7 @@ class Pool < ApplicationRecord self.post_ids = version.post_ids self.name = version.name self.description = version.description - synchronize! + save! end def contains?(post_id) @@ -161,7 +160,6 @@ class Pool < ApplicationRecord with_lock do update(post_ids: post_ids + [post.id]) - post.add_pool!(self, true) end end @@ -171,7 +169,6 @@ class Pool < ApplicationRecord with_lock do reload update(post_ids: post_ids - [post.id]) - post.remove_pool!(self) end end @@ -181,29 +178,6 @@ class Pool < ApplicationRecord Post.joins("JOIN (#{pool_posts.to_sql}) pool_posts ON pool_posts.post_id = posts.id").order("pool_posts.pool_index ASC") end - def synchronize - post_ids_before = post_ids_before_last_save || post_ids_was - added = post_ids - post_ids_before - removed = post_ids_before - post_ids - - added.each do |post_id| - post = Post.find(post_id) - post.add_pool!(self, true) - end - - removed.each do |post_id| - post = Post.find(post_id) - post.remove_pool!(self) - end - - normalize_post_ids - end - - def synchronize! - synchronize - save if will_save_change_to_post_ids? - end - def post_count post_ids.size end diff --git a/app/models/post.rb b/app/models/post.rb index 562019170..ed4315a7e 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -10,6 +10,8 @@ class Post < ApplicationRecord NOTE_COPY_TAGS = %w[translated partially_translated check_translation translation_request reverse_translation annotated partially_annotated check_annotation annotation_request] + self.ignored_columns = [:pool_string] + deletable before_validation :merge_old_changes @@ -561,23 +563,23 @@ class Post < ApplicationRecord case tag when /^-pool:(\d+)$/i pool = Pool.find_by_id($1.to_i) - remove_pool!(pool) if pool + pool&.remove!(self) when /^-pool:(.+)$/i pool = Pool.find_by_name($1) - remove_pool!(pool) if pool + pool&.remove!(self) when /^pool:(\d+)$/i pool = Pool.find_by_id($1.to_i) - add_pool!(pool) if pool + pool&.add!(self) when /^pool:(.+)$/i pool = Pool.find_by_name($1) - add_pool!(pool) if pool + pool&.add!(self) when /^newpool:(.+)$/i pool = Pool.find_by_name($1) - add_pool!(pool) if pool + pool&.add!(self) when /^fav:(.+)$/i add_favorite(CurrentUser.user) @@ -777,35 +779,6 @@ class Post < ApplicationRecord pools.undeleted.present? end - def belongs_to_pool?(pool) - pool_string =~ /(?:\A| )pool:#{pool.id}(?:\Z| )/ - end - - def belongs_to_pool_with_id?(pool_id) - pool_string =~ /(?:\A| )pool:#{pool_id}(?:\Z| )/ - end - - def add_pool!(pool, force = false) - return if belongs_to_pool?(pool) - return if pool.is_deleted? && !force - - with_lock do - self.pool_string = "#{pool_string} pool:#{pool.id}".strip - update_column(:pool_string, pool_string) unless new_record? - pool.add!(self) - end - end - - def remove_pool!(pool) - return unless belongs_to_pool?(pool) - - with_lock do - self.pool_string = pool_string.gsub(/(?:\A| )pool:#{pool.id}(?:\Z| )/, " ").strip - update_column(:pool_string, pool_string) unless new_record? - pool.remove!(self) - end - end - def remove_from_all_pools pools.find_each do |pool| pool.remove!(self) diff --git a/test/unit/pool_test.rb b/test/unit/pool_test.rb index 6bf9492d6..f13335583 100644 --- a/test/unit/pool_test.rb +++ b/test/unit/pool_test.rb @@ -68,13 +68,6 @@ class PoolTest < ActiveSupport::TestCase should "initialize the post count" do assert_equal(@posts.size, @pool.post_count) end - - should "synchronize the posts with the pool" do - assert_equal(@posts.map(&:id), @pool.post_ids) - - @posts.each(&:reload) - assert_equal(["pool:#{@pool.id}"] * @posts.size, @posts.map(&:pool_string)) - end end context "Reverting a pool" do @@ -94,19 +87,15 @@ class PoolTest < ActiveSupport::TestCase assert_equal([], @pool.versions[0].post_ids) assert_equal([@p1.id], @pool.versions[1].post_ids) assert_equal([@p1.id, @p2.id], @pool.versions[2].post_ids) - assert_equal([@p1.id, @p2.id], @pool.post_ids) + assert_equal([@p1.id, @p2.id], @pool.reload.post_ids) end should "update its post_ids" do @pool.revert_to!(@pool.versions[1]) - assert_equal([@p1.id], @pool.reload.post_ids) - assert_equal("pool:#{@pool.id}", @p1.reload.pool_string) - #assert_equal("", @p2.reload.pool_string) + assert_equal([@p1.id], @pool.post_ids) @pool.revert_to!(@pool.versions[0]) assert_equal([], @pool.reload.post_ids) - assert_equal("", @p1.reload.pool_string) - #assert_equal("", @p2.reload.pool_string) end end @@ -125,7 +114,6 @@ class PoolTest < ActiveSupport::TestCase context "by #attributes=" do setup do @pool.attributes = {post_ids: [@p1.id, @p2.id]} - @pool.synchronize @pool.save end @@ -138,10 +126,6 @@ class PoolTest < ActiveSupport::TestCase assert_equal([@p1.id], @pool.post_ids) end - should "add the pool to the post" do - assert_equal("pool:#{@pool.id}", @p1.pool_string) - end - should "increment the post count" do assert_equal(1, @pool.post_count) end @@ -155,10 +139,6 @@ class PoolTest < ActiveSupport::TestCase assert_equal([@p1.id], @pool.post_ids) end - should "not double add the pool to the post" do - assert_equal("pool:#{@pool.id}", @p1.pool_string) - end - should "not double increment the post count" do assert_equal(1, @pool.post_count) end @@ -171,7 +151,6 @@ class PoolTest < ActiveSupport::TestCase @pool.update_attribute(:is_deleted, true) @pool.post_ids += [@p2.id] - @pool.synchronize! @pool.save @pool.reload @p2.reload @@ -181,10 +160,6 @@ class PoolTest < ActiveSupport::TestCase assert_equal([@p1.id, @p2.id], @pool.post_ids) end - should "add the pool to the post" do - assert_equal("pool:#{@pool.id}", @p2.pool_string) - end - should "increment the post count" do assert_equal(2, @pool.post_count) end @@ -205,10 +180,6 @@ class PoolTest < ActiveSupport::TestCase assert_equal([], @pool.post_ids) end - should "remove the pool from the post" do - assert_equal("", @p1.pool_string) - end - should "update the post count" do assert_equal(0, @pool.post_count) end @@ -223,10 +194,6 @@ class PoolTest < ActiveSupport::TestCase assert_equal([@p1.id], @pool.post_ids) end - should "not affect the post" do - assert_equal("pool:#{@pool.id}", @p1.pool_string) - end - should "not affect the post count" do assert_equal(1, @pool.post_count) end @@ -289,29 +256,6 @@ class PoolTest < ActiveSupport::TestCase @pool.add!(@p3) end - context "that is synchronized" do - setup do - @pool.reload - @pool.post_ids = [@p2.id] - @pool.synchronize! - end - - should "update the pool" do - @pool.reload - assert_equal(1, @pool.post_count) - assert_equal([@p2.id], @pool.post_ids) - end - - should "update the posts" do - @p1.reload - @p2.reload - @p3.reload - assert_equal("", @p1.pool_string) - assert_equal("pool:#{@pool.id}", @p2.pool_string) - assert_equal("", @p3.pool_string) - end - end - should "find the neighbors for the first post" do assert_nil(@pool.previous_post_id(@p1.id)) assert_equal(@p2.id, @pool.next_post_id(@p1.id)) diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb index c218168b2..46f8fdc28 100644 --- a/test/unit/post_test.rb +++ b/test/unit/post_test.rb @@ -751,7 +751,6 @@ class PostTest < ActiveSupport::TestCase @post.reload @pool.reload assert_equal([@post.id], @pool.post_ids) - assert_equal("pool:#{@pool.id}", @post.pool_string) end end @@ -759,7 +758,7 @@ class PostTest < ActiveSupport::TestCase setup do @pool = FactoryBot.create(:pool) @post = FactoryBot.create(:post, :tag_string => "aaa") - @post.add_pool!(@pool) + @pool.add!(@post) @post.tag_string = "aaa -pool:#{@pool.id}" @post.save end @@ -768,7 +767,6 @@ class PostTest < ActiveSupport::TestCase @post.reload @pool.reload assert_equal([], @pool.post_ids) - assert_equal("", @post.pool_string) end end @@ -782,7 +780,6 @@ class PostTest < ActiveSupport::TestCase @post.reload @pool.reload assert_equal([@post.id], @pool.post_ids) - assert_equal("pool:#{@pool.id}", @post.pool_string) end end @@ -797,7 +794,6 @@ class PostTest < ActiveSupport::TestCase @post.reload @pool.reload assert_equal([@post.id], @pool.post_ids) - assert_equal("pool:#{@pool.id}", @post.pool_string) end end @@ -808,7 +804,6 @@ class PostTest < ActiveSupport::TestCase @post.reload assert_not_nil(@pool) assert_equal([@post.id], @pool.post_ids) - assert_equal("pool:#{@pool.id}", @post.pool_string) end end @@ -1661,36 +1656,6 @@ class PostTest < ActiveSupport::TestCase setup do SqsService.any_instance.stubs(:send_message) end - - context "Removing a post from a pool" do - should "update the post's pool string" do - post = FactoryBot.create(:post) - pool = FactoryBot.create(:pool) - post.add_pool!(pool) - post.remove_pool!(pool) - post.reload - assert_equal("", post.pool_string) - post.remove_pool!(pool) - post.reload - assert_equal("", post.pool_string) - end - end - - context "Adding a post to a pool" do - should "update the post's pool string" do - post = FactoryBot.create(:post) - pool = FactoryBot.create(:pool) - post.add_pool!(pool) - post.reload - assert_equal("pool:#{pool.id}", post.pool_string) - post.add_pool!(pool) - post.reload - assert_equal("pool:#{pool.id}", post.pool_string) - post.remove_pool!(pool) - post.reload - assert_equal("", post.pool_string) - end - end end context "Uploading:" do diff --git a/test/unit/post_version_test.rb b/test/unit/post_version_test.rb index fa30ee0c5..d89664753 100644 --- a/test/unit/post_version_test.rb +++ b/test/unit/post_version_test.rb @@ -73,7 +73,7 @@ class PostVersionTest < ActiveSupport::TestCase should "create a version" do assert_equal("tagme", @post.tag_string) - assert_equal("pool:#{@pool.id}", @post.pool_string) + assert_equal([@pool.id], @post.pools.pluck(:id)) assert_equal(1, @post.versions.size) assert_equal("tagme", @post.versions.last.tags)