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

aliases/implications: remove 'pending' state.

Browse Source 
Remove the pending status from tag aliases and implications.

Previously aliases would be created first in the pending state then
changed to active when the alias was later processed in a delayed job.
This meant that BURs weren't processed completely sequentially; first
all the aliases in a BUR would be created in one go, then later they
would be processed and set to active sequentially.

This was problematic in complex BURs that tried to reverse or swap
around aliases, since new pending aliases could be created before old
conflicting aliases were removed.
This commit is contained in:
evazion
2020-12-01 15:30:42 -06:00
parent 45d050d918
commit 8717c319ab
19 changed files with 70 additions and 114 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
test/functional/artists_controller_test.rb
View File

@@ -107,10 +107,13 @@ class ArtistsControllerTest < ActionDispatch::IntegrationTest
context "ban action" do
should "ban an artist" do
put_auth ban_artist_path(@artist.id), @admin
perform_enqueued_jobs do
put_auth ban_artist_path(@artist.id), @admin
end
assert_redirected_to(@artist)
assert_equal(true, @artist.reload.is_banned?)
assert_equal(true, TagImplication.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist"))
assert_equal(true, TagImplication.exists?(antecedent_name: @artist.name, consequent_name: "banned_artist", status: "active"))
end
should "not allow non-admins to ban artists" do

6
test/functional/bulk_update_requests_controller_test.rb
View File

@@ -156,12 +156,14 @@ class BulkUpdateRequestsControllerTest < ActionDispatch::IntegrationTest
create(:tag, name: "artist2a", category: Tag.categories.artist, post_count: 20)
@bulk_update_request = create(:bulk_update_request, script: "mass update artist1a -> artist1b\ncreate alias artist2a -> artist2b")
post_auth approve_bulk_update_request_path(@bulk_update_request), @builder
perform_enqueued_jobs do
post_auth approve_bulk_update_request_path(@bulk_update_request), @builder
end
assert_redirected_to(bulk_update_requests_path)
assert_equal("approved", @bulk_update_request.reload.status)
assert_equal(@builder, @bulk_update_request.approver)
assert_equal(true, TagAlias.where(antecedent_name: "artist2a", consequent_name: "artist2b").exists?)
assert_equal(true, TagAlias.exists?(antecedent_name: "artist2a", consequent_name: "artist2b", status: "active"))
end
end

1
test/functional/posts_controller_test.rb
View File

@@ -161,6 +161,7 @@ class PostsControllerTest < ActionDispatch::IntegrationTest
end
should "show a notice for a single tag search with a pending BUR" do
create(:tag, name: "foo")
create(:bulk_update_request, script: "create alias foo -> bar")
get_auth posts_path(tags: "foo"), @user
assert_select ".tag-change-notice"

4
test/functional/tag_aliases_controller_test.rb
View File

@@ -18,7 +18,7 @@ class TagAliasesControllerTest < ActionDispatch::IntegrationTest
@antecedent_wiki = create(:wiki_page, title: "touhou", body: "zun project")
@consequent_wiki = create(:wiki_page, title: "touhou_project")
@other_alias = create(:tag_alias, antecedent_name: "touhou", consequent_name: "touhou_project", creator: @user, status: "pending", forum_topic: @forum_topic, forum_post: @forum_post)
@other_alias = create(:tag_alias, antecedent_name: "touhou", consequent_name: "touhou_project", creator: @user, status: "deleted", forum_topic: @forum_topic, forum_post: @forum_post)
@unrelated_alias = create(:tag_alias)
end
@@ -30,7 +30,7 @@ class TagAliasesControllerTest < ActionDispatch::IntegrationTest
should respond_to_search({}).with { [@unrelated_alias, @other_alias, @tag_alias] }
should respond_to_search(antecedent_name: "aaa").with { @tag_alias }
should respond_to_search(consequent_name: "bbb").with { @tag_alias }
should respond_to_search(status: "pending").with { @other_alias }
should respond_to_search(status: "deleted").with { @other_alias }
context "using includes" do
should respond_to_search(antecedent_tag: {post_count: 1000}).with { @other_alias }

4
test/functional/tag_implications_controller_test.rb
View File

@@ -18,7 +18,7 @@ class TagImplicationsControllerTest < ActionDispatch::IntegrationTest
@antecedent_wiki = create(:wiki_page, title: "cannon", body: "made of fun")
@consequent_wiki = create(:wiki_page, title: "weapon")
@other_implication = create(:tag_implication, antecedent_name: "cannon", consequent_name: "weapon", creator: @user, status: "pending", forum_topic: @forum_topic, forum_post: @forum_post)
@other_implication = create(:tag_implication, antecedent_name: "cannon", consequent_name: "weapon", creator: @user, status: "deleted", forum_topic: @forum_topic, forum_post: @forum_post)
@unrelated_implication = create(:tag_implication)
end
@@ -30,7 +30,7 @@ class TagImplicationsControllerTest < ActionDispatch::IntegrationTest
should respond_to_search({}).with { [@unrelated_implication, @other_implication, @tag_implication] }
should respond_to_search(antecedent_name: "aaa").with { @tag_implication }
should respond_to_search(consequent_name: "bbb").with { @tag_implication }
should respond_to_search(status: "pending").with { @other_implication }
should respond_to_search(status: "deleted").with { @other_implication }
context "using includes" do
should respond_to_search(antecedent_tag: {post_count: 10}).with { @other_implication }

1
test/unit/artist_test.rb
View File

@@ -94,7 +94,6 @@ class ArtistTest < ActiveSupport::TestCase
end
should "create a new tag implication" do
perform_enqueued_jobs
assert_equal(1, TagImplication.where(:antecedent_name => "aaa", :consequent_name => "banned_artist").count)
assert_equal("aaa banned_artist", @post.reload.tag_string)
end

56
test/unit/tag_alias_test.rb
View File

@@ -26,7 +26,6 @@ class TagAliasTest < ActiveSupport::TestCase
should allow_value('active').for(:status)
should allow_value('deleted').for(:status)
should allow_value('pending').for(:status)
should allow_value('error: derp').for(:status)
should_not allow_value('ACTIVE').for(:status)
@@ -47,18 +46,17 @@ class TagAliasTest < ActiveSupport::TestCase
ta2 = FactoryBot.create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "retired")
ta3 = FactoryBot.create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "deleted")
ta4 = FactoryBot.create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "deleted")
ta5 = FactoryBot.create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
[ta1, ta2, ta3, ta4, ta5].each { |ta| assert(ta.valid?) }
[ta1, ta2, ta3, ta4].each { |ta| assert(ta.valid?) }
ta5.update(status: "active")
assert_includes(ta5.errors[:antecedent_name], "has already been taken")
ta4.update(status: "active")
assert_includes(ta4.errors[:antecedent_name], "has already been taken")
end
end
context "#reject!" do
should "not be blocked by validations" do
ta1 = create(:tag_alias, antecedent_name: "kitty", consequent_name: "kitten", status: "active")
ta2 = build(:tag_alias, antecedent_name: "cat", consequent_name: "kitty", status: "pending")
ta2 = build(:tag_alias, antecedent_name: "cat", consequent_name: "kitty", status: "active")
ta2.reject!
assert_equal("deleted", ta2.reload.status)
@@ -89,9 +87,8 @@ class TagAliasTest < ActiveSupport::TestCase
@ss3 = create(:saved_search, query: "123 ~... 456", user: CurrentUser.user)
@ss4 = create(:saved_search, query: "... 456", user: CurrentUser.user)
@ss5 = create(:saved_search, query: "123 ...", user: CurrentUser.user)
@ta = create(:tag_alias, antecedent_name: "...", consequent_name: "bbb")
@ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "...", consequent_name: "bbb", approver: @admin)
perform_enqueued_jobs
assert_equal("123 bbb 456", @ss1.reload.query)
@@ -111,9 +108,8 @@ class TagAliasTest < ActiveSupport::TestCase
@u5 = create(:user, blacklisted_tags: "111 ...")
@u6 = create(:user, blacklisted_tags: "111 222\n\n... 333\n")
@u7 = create(:user, blacklisted_tags: "111 ...\r\n222 333\n")
@ta = create(:tag_alias, antecedent_name: "...", consequent_name: "aaa")
@ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "...", consequent_name: "aaa", approver: @admin)
perform_enqueued_jobs
assert_equal("111 aaa 222", @u1.reload.blacklisted_tags)
@@ -130,8 +126,7 @@ class TagAliasTest < ActiveSupport::TestCase
post1 = FactoryBot.create(:post, :tag_string => "aaa bbb")
post2 = FactoryBot.create(:post, :tag_string => "ccc ddd")
ta = FactoryBot.create(:tag_alias, :antecedent_name => "aaa", :consequent_name => "ccc")
ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "aaa", consequent_name: "ccc", approver: @admin)
perform_enqueued_jobs
assert_equal("bbb ccc", post1.reload.tag_string)
@@ -151,11 +146,9 @@ class TagAliasTest < ActiveSupport::TestCase
context "when the tags have wikis" do
should "rename the old wiki if there is no conflict" do
@wiki = create(:wiki_page, title: "aaa")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
assert_equal("bbb", @wiki.reload.title)
end
@@ -163,11 +156,9 @@ class TagAliasTest < ActiveSupport::TestCase
should "merge existing wikis if there is a conflict" do
@wiki1 = create(:wiki_page, title: "aaa", other_names: "111 222", body: "first")
@wiki2 = create(:wiki_page, title: "bbb", other_names: "111 333", body: "second")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
assert_equal(true, @wiki1.reload.is_deleted)
assert_equal([], @wiki1.other_names)
@@ -181,11 +172,9 @@ class TagAliasTest < ActiveSupport::TestCase
should "ignore the old wiki if it has been deleted" do
@wiki1 = create(:wiki_page, title: "aaa", other_names: "111 222", body: "first", is_deleted: true)
@wiki2 = create(:wiki_page, title: "bbb", other_names: "111 333", body: "second")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
assert_equal(true, @wiki1.reload.is_deleted)
assert_equal(%w[111 222], @wiki1.other_names)
@@ -198,11 +187,9 @@ class TagAliasTest < ActiveSupport::TestCase
should "rewrite links in other wikis to use the new tag" do
@wiki = create(:wiki_page, body: "foo [[aaa]] bar")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb")
@ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
assert_equal("foo [[bbb]] bar", @wiki.reload.body)
end
@@ -211,11 +198,9 @@ class TagAliasTest < ActiveSupport::TestCase
context "when the tags have artist entries" do
should "rename the old artist entry if there is no conflict" do
@artist = create(:artist, name: "aaa")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
assert_equal("bbb", @artist.reload.name)
end
@@ -224,11 +209,9 @@ class TagAliasTest < ActiveSupport::TestCase
@tag = create(:tag, name: "aaa", category: Tag.categories.artist)
@artist1 = create(:artist, name: "aaa", group_name: "g_aaa", other_names: "111 222", url_string: "https://twitter.com/111\n-https://twitter.com/222")
@artist2 = create(:artist, name: "bbb", other_names: "111 333", url_string: "https://twitter.com/111\n-https://twitter.com/333\nhttps://twitter.com/444")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
assert_equal(true, @artist1.reload.is_deleted)
assert_equal([@artist2.name], @artist1.other_names)
@@ -244,11 +227,9 @@ class TagAliasTest < ActiveSupport::TestCase
should "ignore the old artist if it has been deleted" do
@artist1 = create(:artist, name: "aaa", group_name: "g_aaa", other_names: "111 222", url_string: "https://twitter.com/111\n-https://twitter.com/222", is_deleted: true)
@artist2 = create(:artist, name: "bbb", other_names: "111 333", url_string: "https://twitter.com/111\n-https://twitter.com/333\nhttps://twitter.com/444")
@ta = create(:tag_alias, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
@ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "aaa", consequent_name: "bbb", approver: @admin)
perform_enqueued_jobs
assert_equal("active", @ta.reload.status)
assert_equal(true, @artist1.reload.is_deleted)
assert_equal(%w[111 222], @artist1.other_names)
@@ -265,9 +246,8 @@ class TagAliasTest < ActiveSupport::TestCase
should "push the consequent's category to the antecedent if the antecedent is general" do
tag1 = create(:tag, name: "general", category: 0)
tag2 = create(:tag, name: "artist", category: 1)
ta = create(:tag_alias, antecedent_name: "general", consequent_name: "artist")
ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "general", consequent_name: "artist", approver: @admin)
perform_enqueued_jobs
assert_equal(1, tag1.reload.category)
@@ -277,9 +257,8 @@ class TagAliasTest < ActiveSupport::TestCase
should "push the antecedent's category to the consequent if the consequent is general" do
tag1 = create(:tag, name: "artist", category: 1)
tag2 = create(:tag, name: "general", category: 0)
ta = create(:tag_alias, antecedent_name: "artist", consequent_name: "general")
ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "artist", consequent_name: "general", approver: @admin)
perform_enqueued_jobs
assert_equal(1, tag1.reload.category)
@@ -289,9 +268,8 @@ class TagAliasTest < ActiveSupport::TestCase
should "not change either tag category when neither the antecedent or consequent are general" do
tag1 = create(:tag, name: "character", category: 4)
tag2 = create(:tag, name: "copyright", category: 3)
ta = create(:tag_alias, antecedent_name: "character", consequent_name: "copyright")
ta.approve!(@admin)
TagAlias.approve!(antecedent_name: "character", consequent_name: "copyright", approver: @admin)
perform_enqueued_jobs
assert_equal(4, tag1.reload.category)

18
test/unit/tag_implication_test.rb
View File

@@ -22,7 +22,6 @@ class TagImplicationTest < ActiveSupport::TestCase
should allow_value('active').for(:status)
should allow_value('deleted').for(:status)
should allow_value('pending').for(:status)
should allow_value('error: derp').for(:status)
should_not allow_value('ACTIVE').for(:status)
@@ -43,17 +42,16 @@ class TagImplicationTest < ActiveSupport::TestCase
ti2 = FactoryBot.create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", status: "retired")
ti3 = FactoryBot.create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", status: "deleted")
ti4 = FactoryBot.create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", status: "deleted")
ti5 = FactoryBot.create(:tag_implication, antecedent_name: "aaa", consequent_name: "bbb", status: "pending")
[ti1, ti2, ti3, ti4, ti5].each { |ti| assert(ti.valid?) }
[ti1, ti2, ti3, ti4].each { |ti| assert(ti.valid?) }
ti5.update(status: "active")
assert_includes(ti5.errors[:antecedent_name], "Implication already exists")
ti4.update(status: "active")
assert_includes(ti4.errors[:antecedent_name], "Implication already exists")
end
end
context "#reject!" do
should "not be blocked by alias validations" do
ti = create(:tag_implication, antecedent_name: "cat", consequent_name: "animal", status: "pending")
ti = create(:tag_implication, antecedent_name: "cat", consequent_name: "animal", status: "active")
ta = create(:tag_alias, antecedent_name: "cat", consequent_name: "kitty", status: "active")
ti.reject!
@@ -120,11 +118,9 @@ class TagImplicationTest < ActiveSupport::TestCase
should "update any affected post upon save" do
p1 = FactoryBot.create(:post, :tag_string => "aaa bbb ccc")
ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "xxx")
ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "yyy")
ti1.approve!(@admin)
ti2.approve!(@admin)
TagImplication.approve!(antecedent_name: "aaa", consequent_name: "xxx", approver: @admin)
TagImplication.approve!(antecedent_name: "aaa", consequent_name: "yyy", approver: @admin)
perform_enqueued_jobs
assert_equal("aaa bbb ccc xxx yyy", p1.reload.tag_string)
@@ -149,7 +145,7 @@ class TagImplicationTest < ActiveSupport::TestCase
should "not include inactive implications" do
create(:tag_implication, antecedent_name: "a", consequent_name: "b", status: "active")
create(:tag_implication, antecedent_name: "b", consequent_name: "c", status: "pending")
create(:tag_implication, antecedent_name: "b", consequent_name: "c", status: "deleted")
create(:tag_implication, antecedent_name: "c", consequent_name: "d", status: "active")
assert_equal(["b"], TagImplication.tags_implied_by("a").map(&:name))