AO3-6598 Slow query in challenge_signups#update and challenge_signups…

…#create (#4720)

* AO3-6598 Don't perform any more checks if no tags

https://otwarchive.atlassian.net/browse/AO3-6598

* Cop

* Add non-canonical tag error test

* Test for a non-error case

* Actual test for feature

* Cop

* Fixing double call?

* Cop

app/controllers/prompts_controller.rb

@@ -130,14 +130,12 @@ def create
       @prompt = @challenge_signup.requests.build(prompt_params)
     end
 
-    if !@challenge_signup.valid?
-      flash[:error] = ts("That prompt would make your overall sign-up invalid, sorry.")
-      redirect_to edit_collection_signup_path(@collection, @challenge_signup)
-    elsif @prompt.save
+    if @challenge_signup.save
       flash[:notice] = ts("Prompt was successfully added.")
       redirect_to collection_signup_path(@collection, @challenge_signup)
     else
-      render action: :new
+      flash[:error] = ts("That prompt would make your overall sign-up invalid, sorry.")
+      redirect_to edit_collection_signup_path(@collection, @challenge_signup)
     end
   end
 

app/models/prompt.rb

@@ -115,28 +115,44 @@ def correct_number_of_tags
   validate :allowed_tags
   def allowed_tags
     restriction = prompt_restriction
-    if restriction && tag_set
-      TagSet::TAG_TYPES.each do |tag_type|
-        # if we have a specified set of tags of this type, make sure that all the
-        # tags in the prompt are in the set.
-        if TagSet::TAG_TYPES_RESTRICTED_TO_FANDOM.include?(tag_type) && restriction.send("#{tag_type}_restrict_to_fandom")
-          # skip the check, these will be tested in restricted_tags below
-        elsif restriction.has_tags?(tag_type)
-          disallowed_taglist = tag_set.send("#{tag_type}_taglist") - restriction.tags(tag_type)
-          unless disallowed_taglist.empty?
-            errors.add(:base, ts("^These %{tag_label} tags in your %{prompt_type} are not allowed in this challenge: %{taglist}",
+
+    return unless restriction && tag_set
+
+    TagSet::TAG_TYPES.each do |tag_type|
+      # if we have a specified set of tags of this type, make sure that all the
+      # tags in the prompt are in the set.
+
+      # skip the check, these will be tested in restricted_tags below
+      next if TagSet::TAG_TYPES_RESTRICTED_TO_FANDOM.include?(tag_type) && restriction.send("#{tag_type}_restrict_to_fandom")
+
+      taglist = tag_set.send("#{tag_type}_taglist")
+      next if taglist.empty?
+
+      if restriction.has_tags?(tag_type)
+        disallowed_taglist = taglist - restriction.tags(tag_type)
+        unless disallowed_taglist.empty?
+          errors.add(
+            :base,
+            ts(
+              "^These %{tag_label} tags in your %{prompt_type} are not allowed in this challenge: %{taglist}",
               tag_label: tag_type_label_name(tag_type).downcase,
               prompt_type: self.class.name.downcase,
-              taglist: disallowed_taglist.collect(&:name).join(ArchiveConfig.DELIMITER_FOR_OUTPUT)))
-          end
-        else
-          noncanonical_taglist = tag_set.send("#{tag_type}_taglist").reject {|t| t.canonical}
-          unless noncanonical_taglist.empty?
-            errors.add(:base, ts("^These %{tag_label} tags in your %{prompt_type} are not canonical and cannot be used in this challenge: %{taglist}. To fix this, please ask your challenge moderator to set up a tag set for the challenge. New tags can be added to the tag set manually by the moderator or through open nominations.",
+              taglist: disallowed_taglist.collect(&:name).join(ArchiveConfig.DELIMITER_FOR_OUTPUT)
+            )
+          )
+        end
+      else
+        noncanonical_taglist = taglist.reject(&:canonical)
+        unless noncanonical_taglist.empty?
+          errors.add(
+            :base,
+            ts(
+              "^These %{tag_label} tags in your %{prompt_type} are not canonical and cannot be used in this challenge: %{taglist}. To fix this, please ask your challenge moderator to set up a tag set for the challenge. New tags can be added to the tag set manually by the moderator or through open nominations.",
               tag_label: tag_type_label_name(tag_type).downcase,
               prompt_type: self.class.name.downcase,
-              taglist: noncanonical_taglist.collect(&:name).join(ArchiveConfig.DELIMITER_FOR_OUTPUT)))
-          end
+              taglist: noncanonical_taglist.collect(&:name).join(ArchiveConfig.DELIMITER_FOR_OUTPUT)
+            )
+          )
         end
       end
     end

spec/controllers/prompts_controller_spec.rb

@@ -131,6 +131,71 @@
       it_redirects_to_simple("#{collection_signups_path(open_signup.collection)}/"\
                       "#{open_signup.collection.prompts.first.challenge_signup_id}/edit")
     end
+
+    context "prompt has tags" do
+      before do
+        # Delete existing prompt of current user
+        open_signup.offers.first.destroy
+      end
+      let!(:canonical_character) { create(:canonical_character, name: "Sakura Kinomoto") }
+
+      it "should accept canonical tags" do
+        post :create, params: {
+          collection_id: open_signup.collection.name,
+          prompt_type: "offer",
+          prompt: {
+            description: "This is a description.",
+            tag_set_attributes: {
+              character_tagnames: ["Sakura Kinomoto"]
+            }
+          }
+        }
+        it_redirects_to_with_notice(
+          collection_signup_path(open_signup.collection, open_signup),
+          "Prompt was successfully added."
+        )
+      end
+
+      it "should error if some tags aren't canonical" do
+        post :create, params: {
+          collection_id: open_signup.collection.name,
+          prompt_type: "offer",
+          prompt: {
+            description: "This is a description.",
+            tag_set_attributes: {
+              character_tagnames: ["Sakura Typomoto"]
+            }
+          }
+        }
+        it_redirects_to_with_error(
+          edit_collection_signup_path(open_signup.collection, open_signup),
+          "That prompt would make your overall sign-up invalid, sorry."
+        )
+        expect(assigns[:prompt].errors[:base]).to eq(
+          ["^These character tags in your offer are not canonical and cannot be used in this challenge: Sakura Typomoto. To fix this, please ask your challenge moderator to set up a tag set for the challenge. New tags can be added to the tag set manually by the moderator or through open nominations."]
+        )
+      end
+
+      it "shouldn't make needless checks on unused kind of tags" do
+        expect_any_instance_of(PromptRestriction).to receive(:has_tags?).with("character").once
+        expect_any_instance_of(PromptRestriction).not_to receive(:has_tags?).with("relationship")
+
+        post :create, params: {
+          collection_id: open_signup.collection.name,
+          prompt_type: "offer",
+          prompt: {
+            description: "This is a description.",
+            tag_set_attributes: {
+              character_tagnames: ["Sakura Kinomoto"]
+            }
+          }
+        }
+        it_redirects_to_with_notice(
+          collection_signup_path(open_signup.collection, open_signup),
+          "Prompt was successfully added."
+        )
+      end
+    end
   end
 
   describe "update" do