-
Notifications
You must be signed in to change notification settings - Fork 510
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.
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-6823 Validation for invalid tags in tag sets #4945
base: master
Are you sure you want to change the base?
AO3-6823 Validation for invalid tags in tag sets #4945
Conversation
Hi, chanthakoun2002! Thank you so much for this pull request. Someone will be along to review it soon. In the meantime, I've assigned the Jira issue to you and set its status to In Review, so no one will mistakenly create a duplicate pull request. I've also given your account permission to comment on, assign, and transition issues in the future. (I noticed your Jira name is different from the name you'd like credited in the release notes, so I wanted to mention that it makes things a bit easier for us on the organizational side if the Full Name on your Jira account either closely matches the name you'd like us to credit or includes it in parentheses, e.g. "Nickname (CREDIT NAME).") Thanks again for contributing! If you have any questions, you can contact us at [email protected]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for taking a look at this tag set code! I suggested a different approach for the validation and cleanup of invalid tags. Furthermore, this PR is missing an automated test for the issue and the interface text change to inform tag set moderators about the restrictions for the tag formats. Please add these two things in addition to changing the validation strategy.
@@ -83,6 +92,12 @@ def assign_tags | |||
old_tags = self.with_type(type.classify) | |||
tags_to_add += (new_tags - old_tags) | |||
tags_to_remove += (old_tags - new_tags) | |||
if TagSet.invalid_tag?(tag.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of redoing the validation manually, it would be better to check and use the validation errors on the record (with errors) after create was called in tagnames_to_list
. So the check for the errors on the tags should be right before the add_to_set
call in this method.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Furthermore, this if self.instance_variable_get("@#{type}_tagnames")
code path is only taken when using tags for prompts/gift exchanges, which cannot be non-canonical. The problematic code for manually adding code to owned tag sets goes via unless self.instance_variable_get("@#{type}_tagnames_to_add").blank?
.
@@ -250,17 +271,21 @@ def remove_from_autocomplete | |||
|
|||
def add_tags_to_autocomplete(tags_to_add) | |||
tags_to_add.each do |tag| | |||
next if TagSet.invalid_tag?(tag.name) # skips invalid tags in autocomplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Due to your changes above, the code never gets to adding tags to the tag set if any tag is invalid - so this extra check is not necessary.
REDIS_AUTOCOMPLETE.zrem("autocomplete_tagset_all_#{self.id}", value) | ||
REDIS_AUTOCOMPLETE.zrem("autocomplete_tagset_#{tag.type.downcase}_#{self.id}", value) | ||
# cleanup of invalid tags for autocomplete | ||
def remove_invalid_tags_from_autocomplete |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This method can't just be repurposed like this, it's used for removing tags from the autocomplete when removing them from the tag set.
Instead, to do the cleanup suggested in the Jira issue, you'd have to write a rake task that goes through all tag sets and removes the invalid tags from the tag set's autocomplete. This is optional according to the Jira issue, so feel free to leave it out.
If you do want to create the cleanup task, there is even more to be done. The current code adds the invalid tags to the set_taggings
of the tag set, with a nil tag on the set_tagging. So the cleanup code should remove those set_taggings as well.
Issue
This PR addresses AO3-6823.
Purpose
This pull request:
Adds validation for invalid tags to ensure that:
Tags are less than 100 characters.
Tags do not contain restricted characters: , ^ * < > { } = \ %.
Ensures that invalid tags do not appear in the autocomplete feature.
Removes existing invalid tags from the Redis autocomplete cache.
Testing Instructions
Try to add tags with more than 100 characters.
adding restricted characters i.e ^ * < > { } = \ % to tags
Verify that Tags with these invalid characteristics trigger an error message.
Valid tags should be successfully added.
It should be successful if invalid tags are not visible in autocomplete.
Existing invalid tags should have been removed from Redis.
Credit
GitHub: chanthakoun2002
pronouns: He/Him
If you have a Jira account, please include the same name in the "Full name"
field on your Jira profile, so we can assign you the issues you're working on.
Please note that if you do not fill in this section, we will use your GitHub account name and
they/them pronouns.