From fe2698a011335ba874e3d4a983b3f69e3faefcf4 Mon Sep 17 00:00:00 2001 From: evazion Date: Wed, 14 Nov 2018 12:46:35 -0600 Subject: [PATCH] tag implications: convert descendant_names to array (#3987). --- app/models/tag_implication.rb | 14 ++--- ...dant_names_to_array_on_tag_implications.rb | 13 +++++ db/structure.sql | 5 +- test/unit/tag_implication_test.rb | 57 +++++++++---------- 4 files changed, 49 insertions(+), 40 deletions(-) create mode 100644 db/migrate/20181114180205_change_descendant_names_to_array_on_tag_implications.rb diff --git a/app/models/tag_implication.rb b/app/models/tag_implication.rb index 3a9dd506f..3f4c0f117 100644 --- a/app/models/tag_implication.rb +++ b/app/models/tag_implication.rb @@ -1,6 +1,8 @@ class TagImplication < TagRelationship extend Memoist + array_attribute :descendant_names + before_save :update_descendant_names after_save :update_descendant_names_for_parents after_destroy :update_descendant_names_for_parents @@ -22,7 +24,7 @@ class TagImplication < TagRelationship module ClassMethods # assumes names are normalized def with_descendants(names) - (names + active.where(antecedent_name: names).flat_map(&:descendant_names_array)).uniq + (names + active.where(antecedent_name: names).flat_map(&:descendant_names)).uniq end def automatic_tags_for(names) @@ -44,12 +46,8 @@ class TagImplication < TagRelationship end memoize :descendants - def descendant_names_array - descendant_names.split(/ /) - end - def update_descendant_names - self.descendant_names = descendants.join(" ") + self.descendant_names = descendants end def update_descendant_names! @@ -88,7 +86,7 @@ class TagImplication < TagRelationship def absence_of_transitive_relation # Find everything else the antecedent implies, not including the current implication. implications = TagImplication.active.where("antecedent_name = ? and consequent_name != ?", antecedent_name, consequent_name) - implied_tags = implications.flat_map(&:descendant_names_array) + implied_tags = implications.flat_map(&:descendant_names) if implied_tags.include?(consequent_name) self.errors[:base] << "#{antecedent_name} already implies #{consequent_name} through another implication" end @@ -170,7 +168,7 @@ class TagImplication < TagRelationship def update_posts Post.without_timeout do Post.raw_tag_match(antecedent_name).where("true /* TagImplication#update_posts */").find_each do |post| - fixed_tags = "#{post.tag_string} #{descendant_names}".strip + fixed_tags = "#{post.tag_string} #{descendant_names_string}".strip CurrentUser.scoped(creator, creator_ip_addr) do post.update_attributes( :tag_string => fixed_tags diff --git a/db/migrate/20181114180205_change_descendant_names_to_array_on_tag_implications.rb b/db/migrate/20181114180205_change_descendant_names_to_array_on_tag_implications.rb new file mode 100644 index 000000000..2719ac46c --- /dev/null +++ b/db/migrate/20181114180205_change_descendant_names_to_array_on_tag_implications.rb @@ -0,0 +1,13 @@ +class ChangeDescendantNamesToArrayOnTagImplications < ActiveRecord::Migration[5.2] + def up + TagImplication.without_timeout do + change_column :tag_implications, :descendant_names, "text[]", using: "string_to_array(descendant_names, ' ')::text[]", default: "{}" + end + end + + def down + TagImplication.without_timeout do + change_column :tag_implications, :descendant_names, "text", using: "array_to_string(descendant_names, ' ')", default: nil + end + end +end diff --git a/db/structure.sql b/db/structure.sql index 9227e739f..31a94f6d5 100644 --- a/db/structure.sql +++ b/db/structure.sql @@ -3012,7 +3012,7 @@ CREATE TABLE public.tag_implications ( id integer NOT NULL, antecedent_name character varying NOT NULL, consequent_name character varying NOT NULL, - descendant_names text NOT NULL, + descendant_names text[] DEFAULT '{}'::text[] NOT NULL, creator_id integer NOT NULL, creator_ip_addr inet NOT NULL, forum_topic_id integer, @@ -7572,6 +7572,7 @@ INSERT INTO "schema_migrations" (version) VALUES ('20180916002448'), ('20181108162204'), ('20181108205842'), -('20181113174914'); +('20181113174914'), +('20181114180205'); diff --git a/test/unit/tag_implication_test.rb b/test/unit/tag_implication_test.rb index b8ad95380..cdc243de9 100644 --- a/test/unit/tag_implication_test.rb +++ b/test/unit/tag_implication_test.rb @@ -46,7 +46,7 @@ class TagImplicationTest < ActiveSupport::TestCase ti2 = FactoryBot.build(:tag_implication, :antecedent_name => "b", :consequent_name => "c", :status => "pending") ti2.save ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "a", :consequent_name => "b") - assert_equal("b", ti1.descendant_names) + assert_equal(%w[b], ti1.descendant_names) end should "populate the creator information" do @@ -89,14 +89,11 @@ class TagImplicationTest < ActiveSupport::TestCase should "calculate all its descendants" do ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc") - assert_equal("ccc", ti1.descendant_names) - assert_equal(["ccc"], ti1.descendant_names_array) + assert_equal(%w[ccc], ti1.descendant_names) ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") - assert_equal("bbb ccc", ti2.descendant_names) - assert_equal(["bbb", "ccc"], ti2.descendant_names_array) + assert_equal(%w[bbb ccc], ti2.descendant_names) ti1.reload - assert_equal("ccc", ti1.descendant_names) - assert_equal(["ccc"], ti1.descendant_names_array) + assert_equal(%w[ccc], ti1.descendant_names) end should "update its descendants on save" do @@ -109,8 +106,8 @@ class TagImplicationTest < ActiveSupport::TestCase ) ti1.reload ti2.reload - assert_equal("bbb ddd", ti1.descendant_names) - assert_equal("ddd", ti2.descendant_names) + assert_equal(%w[bbb ddd], ti1.descendant_names) + assert_equal(%w[ddd], ti2.descendant_names) end should "update the descendants for all of its parents on destroy" do @@ -122,49 +119,49 @@ class TagImplicationTest < ActiveSupport::TestCase ti2.reload ti3.reload ti4.reload - assert_equal("bbb ccc ddd", ti1.descendant_names) - assert_equal("bbb ccc ddd", ti2.descendant_names) - assert_equal("ccc ddd", ti3.descendant_names) - assert_equal("ddd", ti4.descendant_names) + assert_equal(%w[bbb ccc ddd], ti1.descendant_names) + assert_equal(%w[bbb ccc ddd], ti2.descendant_names) + assert_equal(%w[ccc ddd], ti3.descendant_names) + assert_equal(%w[ddd], ti4.descendant_names) ti3.destroy ti1.reload ti2.reload ti4.reload - assert_equal("bbb", ti1.descendant_names) - assert_equal("bbb", ti2.descendant_names) - assert_equal("ddd", ti4.descendant_names) + assert_equal(%w[bbb], ti1.descendant_names) + assert_equal(%w[bbb], ti2.descendant_names) + assert_equal(%w[ddd], ti4.descendant_names) end should "update the descendants for all of its parents on create" do ti1 = FactoryBot.create(:tag_implication, :antecedent_name => "aaa", :consequent_name => "bbb") ti1.reload assert_equal("active", ti1.status) - assert_equal("bbb", ti1.descendant_names) + assert_equal(%w[bbb], ti1.descendant_names) ti2 = FactoryBot.create(:tag_implication, :antecedent_name => "bbb", :consequent_name => "ccc") ti1.reload ti2.reload assert_equal("active", ti1.status) assert_equal("active", ti2.status) - assert_equal("bbb ccc", ti1.descendant_names) - assert_equal("ccc", ti2.descendant_names) + assert_equal(%w[bbb ccc], ti1.descendant_names) + assert_equal(%w[ccc], ti2.descendant_names) ti3 = FactoryBot.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "ddd") ti1.reload ti2.reload ti3.reload - assert_equal("bbb ccc ddd", ti1.descendant_names) - assert_equal("ccc ddd", ti2.descendant_names) + assert_equal(%w[bbb ccc ddd], ti1.descendant_names) + assert_equal(%w[ccc ddd], ti2.descendant_names) ti4 = FactoryBot.create(:tag_implication, :antecedent_name => "ccc", :consequent_name => "eee") ti1.reload ti2.reload ti3.reload ti4.reload - assert_equal("bbb ccc ddd eee", ti1.descendant_names) - assert_equal("ccc ddd eee", ti2.descendant_names) - assert_equal("ddd", ti3.descendant_names) - assert_equal("eee", ti4.descendant_names) + assert_equal(%w[bbb ccc ddd eee], ti1.descendant_names) + assert_equal(%w[ccc ddd eee], ti2.descendant_names) + assert_equal(%w[ddd], ti3.descendant_names) + assert_equal(%w[eee], ti4.descendant_names) ti5 = FactoryBot.create(:tag_implication, :antecedent_name => "xxx", :consequent_name => "bbb") ti1.reload @@ -172,11 +169,11 @@ class TagImplicationTest < ActiveSupport::TestCase ti3.reload ti4.reload ti5.reload - assert_equal("bbb ccc ddd eee", ti1.descendant_names) - assert_equal("ccc ddd eee", ti2.descendant_names) - assert_equal("ddd", ti3.descendant_names) - assert_equal("eee", ti4.descendant_names) - assert_equal("bbb ccc ddd eee", ti5.descendant_names) + assert_equal(%w[bbb ccc ddd eee], ti1.descendant_names) + assert_equal(%w[ccc ddd eee], ti2.descendant_names) + assert_equal(%w[ddd], ti3.descendant_names) + assert_equal(%w[eee], ti4.descendant_names) + assert_equal(%w[bbb ccc ddd eee], ti5.descendant_names) end should "update any affected post upon save" do