Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AO3-6598 Slow query in challenge_signups#update and challenge_signups#create #4720

Merged
merged 9 commits into from
Feb 17, 2024
Merged

AO3-6598 Slow query in challenge_signups#update and challenge_signups#create #4720

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
7ba62ed
AO3-6598 Don't perform any more checks if no tags
ceithir Jan 20, 2024
392f52b
Cop
ceithir Jan 20, 2024
b79e34f
Add non-canonical tag error test
ceithir Jan 21, 2024
9d1dd1f
Test for a non-error case
ceithir Jan 21, 2024
75b89d5
Actual test for feature
ceithir Jan 21, 2024
e7ac677
Cop
ceithir Jan 21, 2024
8b7abfa
Merge remote-tracking branch 'upstream/master' into AO3-6598
ceithir Feb 11, 2024
5fbab68
Fixing double call?
ceithir Feb 11, 2024
bef074c
Cop
ceithir Feb 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
52 changes: 34 additions & 18 deletions app/models/prompt.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,28 +116,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,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From how I understand it, if the challenge has no restriction at all, we don't check for canonicity, but if it has any sort of restriction, we make sure all tags are (at least) canonical? Is that intended behavior?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My knowledge of the challenge code is extremely rusty, but the Tag Options part of the Gift Exchange FAQ might help explain the intended/current behavior. The tl;dr is tags shouldn't need to be canonical if they're in one of the challenge's tag sets.

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
Expand Down
65 changes: 65 additions & 0 deletions spec/controllers/prompts_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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").at_least(:once)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure where the second call to this method happens actually, might be worth investigating.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, simply because the validations are called a first time through valid? then a second time on save.

Let's try to see if we can run them all only once without breaking everything.

Copy link
Contributor

@Bilka2 Bilka2 Feb 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was also looking at this. My in-progress comment:

The validation is called twice in prompts_controller#create, once by @challenge_signup.valid? and once by @prompt.save.

This happens because challenge_signup uses accepts_nested_attributes_for :offers, :prompts, :requests. accepts_nested_attributes_for means the associated record is autosaved and thus validated if the parent is validated: https://api.rubyonrails.org/v6.1.7.4/classes/ActiveRecord/AutosaveAssociation.html.

So, prompts_controller#create can use @prompt.save(validate: false) instead.

Your solution of combining it also looks okay, but I'm a bit hesitant to call save on the signup when it's the prompt that is getting created. Because this seems to be barely covered in tests, I think it may be better go for my more conservative solution.

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
Expand Down
Loading