jaculabilis/danbooru
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

Merge pull request #3980 from evazion/fix-3979

Browse Source 
Fix #3979 - Pools: store post_ids as array instead of string
This commit is contained in:
Albert Yi
2018-11-08 14:24:08 -08:00
committed by GitHub
parent 8515bf43b4 fb7d83c54f
commit 7c86d3a2e0
15 changed files with 108 additions and 116 deletions
Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/pools_controller.rb
View File

@@ -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

2
app/logical/post_query_builder.rb
View File

@@ -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?

105
app/models/pool.rb
View File

@@ -84,7 +84,7 @@ class Pool < ApplicationRecord
when "created_at"
q = q.order("pools.created_at desc")
when "post_count"
q = q.order("pools.post_count desc").default_order
q = q.order(Arel.sql("cardinality(post_ids) desc")).default_order
else
q = q.apply_default_order(params)
end
@@ -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.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.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,6 @@ class Pool < ApplicationRecord
end
normalize_post_ids
clear_post_id_array
self.post_count = post_id_array.size
end
def synchronize!
@@ -270,50 +262,29 @@ class Pool < ApplicationRecord
save if will_save_change_to_post_ids?
end
def post_count
post_ids.size
end
def first_post?(post_id)
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,14 +299,8 @@ 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]
super + [:creator_name, :post_count]
end
def update_category_pseudo_tags_for_posts_async
@@ -345,7 +310,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 +343,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

2
app/models/pool_archive.rb
View File

@@ -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),

2
app/views/pool_orders/edit.html.erb
View File

@@ -7,7 +7,7 @@
<ul id="sortable">
<% @pool.posts(:limit => 1_000).each do |post| %>
<li class="ui-state-default" id="pool[post_id_array]_<%= post.id %>">
<li class="ui-state-default" id="pool[post_ids]_<%= post.id %>">
<%= PostPresenter.preview(post).presence || "Hidden: Post ##{post.id}" %>
</li>
<% end %>

2
app/views/pools/edit.html.erb
View File

@@ -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 %>

2
app/views/pools/new.html.erb
View File

@@ -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" %>

6
app/views/posts/partials/show/_pool_list_item.html.erb
View File

@@ -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("&laquo;".html_safe, post_path(pool.first_post_id, pool_id: pool.id), class: "first", title: "to page 1") %>
<%= link_to("&laquo;".html_safe, post_path(pool.post_ids.first, pool_id: pool.id), class: "first", title: "to page 1") %>
<% else -%>
<span class="first">&laquo;</span>
<% end -%>
@@ -25,8 +25,8 @@
<% end -%>
<% end -%>
<% if post.id != pool.post_id_array.last -%>
<%= link_to("&raquo;".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("&raquo;".html_safe, post_path(pool.post_ids.last, pool_id: pool.id), class: "last", title: "to page #{pool.post_count}") -%>
<% else -%>
<span class="last">&raquo;</span>
<% end -%>

17
db/migrate/20181108162204_change_post_ids_to_array_on_pools.rb Normal file
View File

@@ -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

10
db/migrate/20181108205842_drop_post_count_from_pools.rb Normal file
View File

@@ -0,0 +1,10 @@
class DropPostCountFromPools < ActiveRecord::Migration[5.2]
def up
remove_column :pools, :post_count
end
def down
add_column :pools, :post_count, :integer, default: 0, null: false
Pool.update_all("post_count = cardinality(post_ids)")
end
end

7
db/structure.sql
View File

@@ -2563,8 +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_count integer DEFAULT 0 NOT NULL,
post_ids integer[] DEFAULT '{}'::integer[] NOT NULL,
is_deleted boolean DEFAULT false NOT NULL,
created_at timestamp without time zone,
updated_at timestamp without time zone,
@@ -7578,6 +7577,8 @@ INSERT INTO "schema_migrations" (version) VALUES
('20180816230604'),
('20180912185624'),
('20180913184128'),
('20180916002448');
('20180916002448'),
('20181108162204'),
('20181108205842');

8
test/functional/pool_elements_controller_test.rb
View File

@@ -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

10
test/functional/pools_controller_test.rb
View File

@@ -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

35
test/unit/pool_test.rb
View File

@@ -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

14
test/unit/post_test.rb
View File

@@ -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