diff --git a/app/controllers/pools_controller.rb b/app/controllers/pools_controller.rb
index c018b9ad8..610c22c7e 100644
--- a/app/controllers/pools_controller.rb
+++ b/app/controllers/pools_controller.rb
@@ -95,6 +95,6 @@ class PoolsController < ApplicationController
def pool_params
permitted_params = %i[name description category is_active post_ids]
- params.require(:pool).permit(*permitted_params, post_id_array: [])
+ params.require(:pool).permit(*permitted_params, post_ids: [])
end
end
diff --git a/app/logical/post_query_builder.rb b/app/logical/post_query_builder.rb
index fa3f78bd9..02219ac87 100644
--- a/app/logical/post_query_builder.rb
+++ b/app/logical/post_query_builder.rb
@@ -406,7 +406,7 @@ class PostQueryBuilder
if q[:ordpool].present?
pool_id = q[:ordpool].to_i
- relation = relation.order(Arel.sql("position(' '||posts.id||' ' in ' '||(select post_ids from pools where id = #{pool_id})||' ')"))
+ relation = relation.find_ordered(Pool.find(pool_id).post_ids)
end
if q[:favgroups_neg].present?
diff --git a/app/models/pool.rb b/app/models/pool.rb
index 8bab916b8..8d7a562bd 100644
--- a/app/models/pool.rb
+++ b/app/models/pool.rb
@@ -111,14 +111,6 @@ class Pool < ApplicationRecord
normalize_name(name).mb_chars.downcase
end
- def self.normalize_post_ids(post_ids, unique)
- hoge = post_ids.scan(/\d+/)
- if unique
- hoge = hoge.uniq
- end
- hoge.join(" ")
- end
-
def self.find_by_name(name)
if name =~ /^\d+$/
where("pools.id = ?", name.to_i).first
@@ -158,7 +150,19 @@ class Pool < ApplicationRecord
end
def normalize_post_ids
- self.post_ids = self.class.normalize_post_ids(post_ids, is_collection?)
+ self.post_ids = post_ids.uniq if is_collection?
+ end
+
+ # allow assigning a string to post_ids so it can be assigned from the text
+ # field in the pool edit form (PUT /pools/1?post_ids=1+2+3).
+ def post_ids=(value)
+ if value.respond_to?(:to_str)
+ super value.to_str.scan(/\d+/).map(&:to_i)
+ elsif value.respond_to?(:to_a)
+ super value.to_a
+ else
+ raise ArgumentError, "post_ids must be a String or an Array"
+ end
end
def revert_to!(version)
@@ -166,18 +170,18 @@ class Pool < ApplicationRecord
raise RevertError.new("You cannot revert to a previous version of another pool.")
end
- self.post_ids = version.post_ids.join(" ")
+ self.post_ids = version.post_ids
self.name = version.name
self.description = version.description
synchronize!
end
def contains?(post_id)
- post_ids =~ /(?:\A| )#{post_id}(?:\Z| )/
+ post_ids.include?(post_id)
end
def page_number(post_id)
- post_id_array.find_index(post_id).to_i + 1
+ post_ids.find_index(post_id).to_i + 1
end
def deletable_by?(user)
@@ -203,9 +207,8 @@ class Pool < ApplicationRecord
return if is_deleted?
with_lock do
- update_attributes(:post_ids => add_number_to_string(post.id, post_ids), :post_count => post_count + 1)
+ update(post_ids: post_ids + [post.id], post_count: post_count + 1)
post.add_pool!(self, true)
- clear_post_id_array
end
end
@@ -215,24 +218,15 @@ class Pool < ApplicationRecord
with_lock do
reload
- update_attributes(:post_ids => remove_number_from_string(post.id, post_ids), :post_count => post_count - 1)
+ update(post_ids: post_ids - [post.id], post_count: post_count - 1)
post.remove_pool!(self)
- clear_post_id_array
end
end
- def add_number_to_string(number, string)
- "#{string} #{number}"
- end
-
- def remove_number_from_string(number, string)
- string.gsub(/(?:\A| )#{number}(?:\Z| )/, " ")
- end
-
def posts(options = {})
offset = options[:offset] || 0
limit = options[:limit] || Danbooru.config.posts_per_page
- slice = post_id_array.slice(offset, limit)
+ slice = post_ids.slice(offset, limit)
if slice && slice.any?
slice.map do |id|
begin
@@ -247,8 +241,8 @@ class Pool < ApplicationRecord
end
def synchronize
- added = post_id_array - post_id_array_was
- removed = post_id_array_was - post_id_array
+ added = post_ids - post_ids_was
+ removed = post_ids_was - post_ids
added.each do |post_id|
post = Post.find(post_id)
@@ -261,8 +255,7 @@ class Pool < ApplicationRecord
end
normalize_post_ids
- clear_post_id_array
- self.post_count = post_id_array.size
+ self.post_count = post_ids.size
end
def synchronize!
@@ -274,46 +267,21 @@ class Pool < ApplicationRecord
page_number(post_id) == 1
end
- def first_post_id
- post_id_array[0]
- end
-
- def post_id_array
- @post_id_array ||= post_ids.scan(/\d+/).map(&:to_i)
- end
-
- def post_id_array=(array)
- self.post_ids = array.join(" ")
- clear_post_id_array
- self
- end
-
- def post_id_array_was
- old_post_ids = post_ids_before_last_save || post_ids_was
- @post_id_array_was ||= old_post_ids.to_s.scan(/\d+/).map(&:to_i)
- end
-
- def clear_post_id_array
- @post_id_array = nil
- @post_id_array_was = nil
- self
- end
-
# XXX finds wrong post when the pool contains multiple copies of the same post (#2042).
def previous_post_id(post_id)
- n = post_id_array.index(post_id) - 1
+ n = post_ids.index(post_id) - 1
return nil if n < 0
- post_id_array[n]
+ post_ids[n]
end
def next_post_id(post_id)
- n = post_id_array.index(post_id) + 1
- return nil if n >= post_id_array.size
- post_id_array[n]
+ n = post_ids.index(post_id) + 1
+ return nil if n >= post_ids.size
+ post_ids[n]
end
def cover_post_id
- post_ids[/^(\d+)/, 1]
+ post_ids.first
end
def create_version(updater: CurrentUser.user, updater_ip_addr: CurrentUser.ip_addr)
@@ -328,12 +296,6 @@ class Pool < ApplicationRecord
(post_count / CurrentUser.user.per_page.to_f).ceil
end
- def reload(options = {})
- super
- clear_post_id_array
- self
- end
-
def method_attributes
super + [:creator_name]
end
@@ -345,7 +307,7 @@ class Pool < ApplicationRecord
end
def update_category_pseudo_tags_for_posts
- Post.where("id in (?)", post_id_array).find_each do |post|
+ Post.where(id: post_ids).find_each do |post|
post.reload
post.set_pool_category_pseudo_tags
Post.where(:id => post.id).update_all(:pool_string => post.pool_string)
@@ -378,7 +340,7 @@ class Pool < ApplicationRecord
end
def updater_can_remove_posts
- removed = post_id_array_was - post_id_array
+ removed = post_ids_was - post_ids
if removed.any? && !CurrentUser.user.can_remove_from_pools?
errors[:base] << "You cannot removes posts from pools within the first week of sign up"
end
diff --git a/app/models/pool_archive.rb b/app/models/pool_archive.rb
index 463e932a7..f91ef9be4 100644
--- a/app/models/pool_archive.rb
+++ b/app/models/pool_archive.rb
@@ -50,7 +50,7 @@ class PoolArchive < ApplicationRecord
json = {
pool_id: pool.id,
- post_ids: pool.post_ids.scan(/\d+/).map(&:to_i),
+ post_ids: pool.post_ids,
updater_id: updater.id,
updater_ip_addr: updater_ip_addr.to_s,
created_at: pool.created_at.try(:iso8601),
diff --git a/app/views/pool_orders/edit.html.erb b/app/views/pool_orders/edit.html.erb
index b3a53a9d1..336d83780 100644
--- a/app/views/pool_orders/edit.html.erb
+++ b/app/views/pool_orders/edit.html.erb
@@ -7,7 +7,7 @@
<% @pool.posts(:limit => 1_000).each do |post| %>
- -
+
-
<%= PostPresenter.preview(post).presence || "Hidden: Post ##{post.id}" %>
<% end %>
diff --git a/app/views/pools/edit.html.erb b/app/views/pools/edit.html.erb
index 934419365..460a0ba5c 100644
--- a/app/views/pools/edit.html.erb
+++ b/app/views/pools/edit.html.erb
@@ -8,7 +8,7 @@
<%= f.input :name, :as => :string, :input_html => { :value => @pool.pretty_name } %>
<%= dtext_field "pool", "description" %>
<%= dtext_preview_button "pool", "description" %>
- <%= f.input :post_ids, :label => "Posts" %>
+ <%= f.input :post_ids, as: :text, label: "Posts", input_html: { value: @pool.post_ids.join(" ") } %>
<%= f.input :category, :collection => ["series", "collection"], :include_blank => false %>
<%= f.input :is_active %>
<%= f.button :submit %>
diff --git a/app/views/pools/new.html.erb b/app/views/pools/new.html.erb
index 01c8c8411..d3d28221f 100644
--- a/app/views/pools/new.html.erb
+++ b/app/views/pools/new.html.erb
@@ -7,7 +7,7 @@
<%= simple_form_for(@pool) do |f| %>
<%= f.input :name, :as => :string, :required => true %>
<%= dtext_field "pool", "description" %>
- <%= f.input :post_ids, :label => "Posts" %>
+ <%= f.input :post_ids, as: :text, label: "Posts", input_html: { value: @pool.post_ids.join(" ") } %>
<%= f.input :category, :collection => ["series", "collection"], :include_blank => true, :selected => "", :required => true %>
<%= f.input :is_active %>
<%= f.button :submit, "Submit" %>
diff --git a/app/views/posts/partials/show/_pool_list_item.html.erb b/app/views/posts/partials/show/_pool_list_item.html.erb
index ee27041c9..63d0723a3 100644
--- a/app/views/posts/partials/show/_pool_list_item.html.erb
+++ b/app/views/posts/partials/show/_pool_list_item.html.erb
@@ -1,6 +1,6 @@
<%= content_tag :li, id: "nav-link-for-pool-#{pool.id}", class: "pool-category-#{pool.category} pool-selected-#{selected}" do -%>
<% if !pool.first_post?(post.id) -%>
- <%= link_to("«".html_safe, post_path(pool.first_post_id, pool_id: pool.id), class: "first", title: "to page 1") %>
+ <%= link_to("«".html_safe, post_path(pool.post_ids.first, pool_id: pool.id), class: "first", title: "to page 1") %>
<% else -%>
«
<% end -%>
@@ -25,8 +25,8 @@
<% end -%>
<% end -%>
- <% if post.id != pool.post_id_array.last -%>
- <%= link_to("»".html_safe, post_path(pool.post_id_array.last, pool_id: pool.id), class: "last", title: "to page #{pool.post_count}") -%>
+ <% if post.id != pool.post_ids.last -%>
+ <%= link_to("»".html_safe, post_path(pool.post_ids.last, pool_id: pool.id), class: "last", title: "to page #{pool.post_count}") -%>
<% else -%>
»
<% end -%>
diff --git a/db/migrate/20181108162204_change_post_ids_to_array_on_pools.rb b/db/migrate/20181108162204_change_post_ids_to_array_on_pools.rb
new file mode 100644
index 000000000..735e622c2
--- /dev/null
+++ b/db/migrate/20181108162204_change_post_ids_to_array_on_pools.rb
@@ -0,0 +1,17 @@
+class ChangePostIdsToArrayOnPools < ActiveRecord::Migration[5.2]
+ def up
+ Pool.without_timeout do
+ change_column_default :pools, :post_ids, nil
+ change_column :pools, :post_ids, "integer[]", using: "string_to_array(post_ids, ' ')::integer[]"
+ change_column_default :pools, :post_ids, "{}"
+ end
+ end
+
+ def down
+ Pool.without_timeout do
+ change_column_default :pools, :post_ids, nil
+ change_column :pools, :post_ids, :text, using: "array_to_string(post_ids, ' ')"
+ change_column_default :pools, :post_ids, ""
+ end
+ end
+end
diff --git a/db/structure.sql b/db/structure.sql
index dbe979da3..caf555201 100644
--- a/db/structure.sql
+++ b/db/structure.sql
@@ -2563,7 +2563,7 @@ CREATE TABLE public.pools (
creator_id integer NOT NULL,
description text,
is_active boolean DEFAULT true NOT NULL,
- post_ids text DEFAULT ''::text NOT NULL,
+ post_ids integer[] DEFAULT '{}'::integer[] NOT NULL,
post_count integer DEFAULT 0 NOT NULL,
is_deleted boolean DEFAULT false NOT NULL,
created_at timestamp without time zone,
@@ -7578,6 +7578,7 @@ INSERT INTO "schema_migrations" (version) VALUES
('20180816230604'),
('20180912185624'),
('20180913184128'),
-('20180916002448');
+('20180916002448'),
+('20181108162204');
diff --git a/test/functional/pool_elements_controller_test.rb b/test/functional/pool_elements_controller_test.rb
index 99627d6a4..33306b581 100644
--- a/test/functional/pool_elements_controller_test.rb
+++ b/test/functional/pool_elements_controller_test.rb
@@ -21,14 +21,14 @@ class PoolElementsControllerTest < ActionDispatch::IntegrationTest
should "add a post to a pool" do
post_auth pool_element_path, @user, params: {:pool_id => @pool.id, :post_id => @post.id, :format => "json"}
@pool.reload
- assert_equal([@post.id], @pool.post_id_array)
+ assert_equal([@post.id], @pool.post_ids)
end
should "add a post to a pool once and only once" do
as_user { @pool.add!(@post) }
post_auth pool_element_path, @user, params: {:pool_id => @pool.id, :post_id => @post.id, :format => "json"}
@pool.reload
- assert_equal([@post.id], @pool.post_id_array)
+ assert_equal([@post.id], @pool.post_ids)
end
end
@@ -40,7 +40,7 @@ class PoolElementsControllerTest < ActionDispatch::IntegrationTest
should "remove a post from a pool" do
delete_auth pool_element_path, @user, params: {:pool_id => @pool.id, :post_id => @post.id, :format => "json"}
@pool.reload
- assert_equal([], @pool.post_id_array)
+ assert_equal([], @pool.post_ids)
end
should "do nothing if the post is not a member of the pool" do
@@ -50,7 +50,7 @@ class PoolElementsControllerTest < ActionDispatch::IntegrationTest
end
delete_auth pool_element_path, @user, params: {:pool_id => @pool.id, :post_id => @post.id, :format => "json"}
@pool.reload
- assert_equal([], @pool.post_id_array)
+ assert_equal([], @pool.post_ids)
end
end
end
diff --git a/test/functional/pools_controller_test.rb b/test/functional/pools_controller_test.rb
index 8dd42b181..a336b1959 100644
--- a/test/functional/pools_controller_test.rb
+++ b/test/functional/pools_controller_test.rb
@@ -71,9 +71,9 @@ class PoolsControllerTest < ActionDispatch::IntegrationTest
context "update action" do
should "update a pool" do
- put_auth pool_path(@pool), @user, params: { pool: { name: "xyz", post_ids: @post.id.to_s }}
+ put_auth pool_path(@pool), @user, params: { pool: { name: "xyz", post_ids: [@post.id] }}
assert_equal("xyz", @pool.reload.name)
- assert_equal(@post.id.to_s, @pool.post_ids)
+ assert_equal([@post.id], @pool.post_ids)
end
should "not allow updating unpermitted attributes" do
@@ -110,10 +110,10 @@ class PoolsControllerTest < ActionDispatch::IntegrationTest
setup do
as_user do
@post_2 = create(:post)
- @pool = create(:pool, :post_ids => "#{@post.id}")
+ @pool = create(:pool, post_ids: [@post.id])
end
CurrentUser.scoped(@user, "1.2.3.4") do
- @pool.update(:post_ids => "#{@post.id} #{@post_2.id}")
+ @pool.update(post_ids: [@post.id, @post_2.id])
end
end
@@ -123,7 +123,7 @@ class PoolsControllerTest < ActionDispatch::IntegrationTest
assert_equal([@post.id], version.post_ids)
put_auth revert_pool_path(@pool), @mod, params: {:version_id => version.id}
@pool.reload
- assert_equal([@post.id], @pool.post_id_array)
+ assert_equal([@post.id], @pool.post_ids)
end
should "not allow reverting to a previous version of another pool" do
diff --git a/test/unit/pool_test.rb b/test/unit/pool_test.rb
index 8cb1d0ab2..f2dc09813 100644
--- a/test/unit/pool_test.rb
+++ b/test/unit/pool_test.rb
@@ -55,7 +55,7 @@ class PoolTest < ActiveSupport::TestCase
context "Creating a pool" do
setup do
@posts = FactoryBot.create_list(:post, 5)
- @pool = FactoryBot.create(:pool, post_ids: @posts.map(&:id).join(" "))
+ @pool = FactoryBot.create(:pool, post_ids: @posts.map(&:id))
end
should "initialize the post count" do
@@ -104,7 +104,7 @@ class PoolTest < ActiveSupport::TestCase
end
should "update its post_ids" do
- assert_equal([@p1.id], @pool.post_id_array)
+ assert_equal([@p1.id], @pool.post_ids)
end
should "update any old posts that were removed" do
@@ -132,7 +132,7 @@ class PoolTest < ActiveSupport::TestCase
context "by #attributes=" do
setup do
- @pool.attributes = {post_ids: [@p1, @p2].map(&:id).join(" ")}
+ @pool.attributes = {post_ids: [@p1.id, @p2.id]}
@pool.synchronize
@pool.save
end
@@ -143,7 +143,7 @@ class PoolTest < ActiveSupport::TestCase
end
should "add the post to the pool" do
- assert_equal("#{@p1.id}", @pool.post_ids)
+ assert_equal([@p1.id], @pool.post_ids)
end
should "add the pool to the post" do
@@ -160,7 +160,7 @@ class PoolTest < ActiveSupport::TestCase
end
should "not double add the post to the pool" do
- assert_equal("#{@p1.id}", @pool.post_ids)
+ assert_equal([@p1.id], @pool.post_ids)
end
should "not double add the pool to the post" do
@@ -178,7 +178,7 @@ class PoolTest < ActiveSupport::TestCase
CurrentUser.user = FactoryBot.create(:builder_user)
@pool.update_attribute(:is_deleted, true)
- @pool.post_ids = "#{@pool.post_ids} #{@p2.id}"
+ @pool.post_ids += [@p2.id]
@pool.synchronize!
@pool.save
@pool.reload
@@ -186,7 +186,7 @@ class PoolTest < ActiveSupport::TestCase
end
should "add the post to the pool" do
- assert_equal("#{@p1.id} #{@p2.id}", @pool.post_ids)
+ assert_equal([@p1.id, @p2.id], @pool.post_ids)
end
should "add the pool to the post" do
@@ -210,7 +210,7 @@ class PoolTest < ActiveSupport::TestCase
end
should "remove the post from the pool" do
- assert_equal("", @pool.post_ids)
+ assert_equal([], @pool.post_ids)
end
should "remove the pool from the post" do
@@ -228,7 +228,7 @@ class PoolTest < ActiveSupport::TestCase
end
should "not affect the pool" do
- assert_equal("#{@p1.id}", @pool.post_ids)
+ assert_equal([@p1.id], @pool.post_ids)
end
should "not affect the post" do
@@ -275,7 +275,7 @@ class PoolTest < ActiveSupport::TestCase
user2 = Timecop.travel(1.month.ago) {FactoryBot.create(:user)}
CurrentUser.scoped(user2, "127.0.0.2") do
- @pool.post_ids = "#{@p1.id}"
+ @pool.post_ids = [@p1.id]
@pool.save
end
@@ -285,7 +285,7 @@ class PoolTest < ActiveSupport::TestCase
assert_equal("127.0.0.2", @pool.versions.last.updater_ip_addr.to_s)
CurrentUser.scoped(user2, "127.0.0.3") do
- @pool.post_ids = "#{@p1.id} #{@p2.id}"
+ @pool.post_ids = [@p1.id, @p2.id]
@pool.save
end
@@ -304,9 +304,8 @@ class PoolTest < ActiveSupport::TestCase
end
should "know what its post ids were previously" do
- @pool.post_ids = "#{@p1.id}"
- assert_equal("", @pool.post_ids_was)
- assert_equal([], @pool.post_id_array_was)
+ @pool.post_ids = [@p1.id]
+ assert_equal([], @pool.post_ids_was)
end
should "normalize its name" do
@@ -318,8 +317,8 @@ class PoolTest < ActiveSupport::TestCase
end
should "normalize its post ids" do
- @pool.update_attributes(:post_ids => " 1 2 ")
- assert_equal("1 2", @pool.post_ids)
+ @pool.update(category: "collection", post_ids: [1, 2, 2, 3, 1])
+ assert_equal([1, 2, 3], @pool.post_ids)
end
context "when validating names" do
@@ -343,14 +342,14 @@ class PoolTest < ActiveSupport::TestCase
context "that is synchronized" do
setup do
@pool.reload
- @pool.post_ids = "#{@p2.id}"
+ @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)
+ assert_equal([@p2.id], @pool.post_ids)
end
should "update the posts" do
diff --git a/test/unit/post_test.rb b/test/unit/post_test.rb
index a0b0bd942..21b8e55ab 100644
--- a/test/unit/post_test.rb
+++ b/test/unit/post_test.rb
@@ -115,11 +115,11 @@ class PostTest < ActiveSupport::TestCase
end
should "remove the post from all pools" do
- assert_equal("", @pool.post_ids)
+ assert_equal([], @pool.post_ids)
end
should "remove the post from deleted pools" do
- assert_equal("", @deleted_pool.post_ids)
+ assert_equal([], @deleted_pool.post_ids)
end
should "destroy the record" do
@@ -840,7 +840,7 @@ class PostTest < ActiveSupport::TestCase
should "add the post to the pool" do
@post.reload
@pool.reload
- assert_equal("#{@post.id}", @pool.post_ids)
+ assert_equal([@post.id], @pool.post_ids)
assert_equal("pool:#{@pool.id} pool:series", @post.pool_string)
end
end
@@ -857,7 +857,7 @@ class PostTest < ActiveSupport::TestCase
should "remove the post from the pool" do
@post.reload
@pool.reload
- assert_equal("", @pool.post_ids)
+ assert_equal([], @pool.post_ids)
assert_equal("", @post.pool_string)
end
end
@@ -871,7 +871,7 @@ class PostTest < ActiveSupport::TestCase
should "add the post to the pool" do
@post.reload
@pool.reload
- assert_equal("#{@post.id}", @pool.post_ids)
+ assert_equal([@post.id], @pool.post_ids)
assert_equal("pool:#{@pool.id} pool:series", @post.pool_string)
end
end
@@ -886,7 +886,7 @@ class PostTest < ActiveSupport::TestCase
should "add the post to the pool" do
@post.reload
@pool.reload
- assert_equal("#{@post.id}", @pool.post_ids)
+ assert_equal([@post.id], @pool.post_ids)
assert_equal("pool:#{@pool.id} pool:series", @post.pool_string)
end
end
@@ -897,7 +897,7 @@ class PostTest < ActiveSupport::TestCase
@pool = Pool.find_by_name("abc")
@post.reload
assert_not_nil(@pool)
- assert_equal("#{@post.id}", @pool.post_ids)
+ assert_equal([@post.id], @pool.post_ids)
assert_equal("pool:#{@pool.id} pool:series", @post.pool_string)
end
end