AO3-6626: stop tags from being added with Chinese or Japanese commas

[kwerey]

Hi! First time looking at the AO3 codebase - I wanted to learn a bit about Ruby and I'm a happy AO3 user so I set up an AO3 dev env and rummaged around Jira a little. Here is a very simple change for an existing ticket, please let me know if I'm missing anything.

Pull Request Checklist

• Have you read "How to write the perfect pull request"?
• Have you read the contributing guidelines?
• Have you added tests for any changed functionality?
• Have you added the Jira issue number
  as the first thing in your pull request title (e.g. AO3-1234 Fix thing)
• Do you have fewer than 5 pull requests already open? If not, please wait
  until they are reviewed and merged before creating new pull requests.

Issue

(https://otwarchive.atlassian.net/jira/software/c/projects/AO3/issues/AO3-6626)

Purpose

This pull request adds two additional characters to the regex of forbidden characters

Testing Instructions

The Archive's QA team can verify that this is working as intended by following the test steps set out in the Jira ticket, copy/pasted here:

Log in as a tag wrangler (e.g., testy)

Create a new tag with a Chinese ( ， ) or Japanese (、) comma

    Browse > Tags > New Tag

    In the Name field, enter a tag name that contains one of the commas 

        Both Tag With 、Japanese Comma and Tag With ，Chinese Comma have been created on staging

    Select a Category, e.g., Additional Tag

    Check the Canonical checkbox

    Press Create Tag

Try to use the tag in an autocomplete field

    Post > New Work

    Fill in required fields

    In the appropriate tag field, enter the name of the tag you created in Step 2, or choose it from the autocomplete

    Press Post

I have tested this on a local Docker environment. I did not add a unit test because there's already an appropriate-looking unit test for the forbidden character filter, which currently passes:

  it "should not be valid with disallowed characters" do
    tag = Tag.new
    tag.name = "bad<tag"
    expect(tag.save).to be_falsey
    expect(tag.errors[:name].join).to match(/restricted characters/)
  end

Which currently passes:

Run options: include {:locations=>{"./spec/models/tag_spec.rb"=>[171]}}

Tag
  should not be valid with disallowed characters

Finished in 5.17 seconds (files took 5.43 seconds to load)
1 example, 0 failures

References

No other relevant issues/pull requests/mailing list discussions.

Credit

What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?

username: kwerey
pronouns: they/them

I do not currently have a public Jira account.

[kwerey]

I'm happy to fix the styleguide comment, but I'm not really clear what it's asking me to change - Github Actions here seems to give different output than I get from running it locally with: docker-compose run --rm test bundle exec rubocop app/models/tag.rb (where I get 304 offenses detected but no entry for the line I've changed).

I'll not push another commits so as to avoid running these other fairly long build jobs but very happy to tweak stuff on request. Thanks!

[Bilka2]

Hey, thank you for contributing to this repository! Usually someone from the OTW says hello to new contributors, but I figured I'd already comment on the Rubocop warnings. (I'm an outside contributor, not part of the OTW.)

When I run the command you pasted, it reports 304 offenses, with 2 of them on line 171 which was changed by this PR:

app/models/tag.rb:171:5: C: [Correctable] Layout/ArgumentAlignment: Align the arguments of a method call if they span more than one line.
    with:    /\A[^,，、*<>^{}=`\\%]+\z/, ...
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
app/models/tag.rb:171:5: C: [Correctable] Layout/HashAlignment: Align the keys of a hash literal if they span more than one line.
    with:    /\A[^,，、*<>^{}=`\\%]+\z/,
    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Only the Layout/HashAlignment offense is directly caused by the changes here, due to the extra spaces added in front of the regular expression.

However, this PR is a good opportunity is fix both the offenses above as well as this one:

app/models/tag.rb:170:3: C: [Correctable] Rails/Validation: Prefer the new style validations validates :column, format: value over validates_format_of.
  validates_format_of :name,
  ^^^^^^^^^^^^^^^^^^^

Applying these suggestions would result in:

  validates :name,
            format: { with: /\A[^,，、*<>^{}=`\\%]+\z/,
                      message: "^Tag name '%{value}' cannot include the following restricted characters: , &#94; * < > { } = ` ， 、 \\ %" }

I think it would be good to make these changes.

[kwerey]

Thank you for useful guidance! I've made those corrections and adjusted some the other validation statements in this block to match the format that it recommends.

Edit: I fixed the Rubocop issue, but two different Cucumber tests failed on two different commits, one about unmarking comments as spam, one about navigating between works. I ran both the specific failing tests a few times locally and didn't see the same failure. Is there something I can do to sort that out? IIRC there's a "Rerun only failed tests" option in Github Actions but I think the only way I can retrigger tests is by pushing a new commit, and it seems a little wasteful to retrigger pipelines again without changing anything else or understanding the failure mode here.

[sarken]

Hi, kwerey!

Thank you so much for this pull request!

I've updated the Jira issue status to In Review so no one will mistakenly create a duplicate pull request. If you'd like the ability to comment on, assign, and transition issues in the future, you're welcome to create a Jira account! You can just reply here with the account name and we'll set up the permissions for you. (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 in the release notes or includes it in parentheses, e.g. "Nickname (PREFERRED NAME).")

Regarding the test failures, they do sound unrelated, so I've restarted them. (We try to document known failures like that on our wiki, but these sound like new ones we'll need to keep an eye on.) Usually the causes of these is test failures, where the tests run a bit too quickly in GitHub Actions and things haven't been saved, reindexed, or had their caches expired before the next step runs. In any case, there's no need to push extra commits -- someone will come along and rerun the failures in GitHub Actions.

Thanks again for contributing! If you have any questions, you can contact us at otw-coders@transformativeworks.org.

[Bilka2]

The rubocop fixes look good, thank you!

Regarding the test, it currently only tests for one of the restricted characters (<). It would be great to change it to test an array of the restricted characters, making and saving one tag per character. That way the newly added characters are tested and we will also notice if something is accidentally removed from the regex.

[kwerey]

That makes sense! I've taken a stab at that and also tried to tidy up some of the Rubocop pings in this file - I see that Rubocop does not like just sticking an array of Forbidden Tags in the spec_helper file like we've done for forbidden emails. Figuring out some Ruby best practices is one of the things I wanna learn so very happy to go learn where constants are meant to be declared. but I won't get to that until later today.

[Bilka2]

Since the bad tags are only needed in one test, they can defined directly in the test instead of in the spec_helper file. The skin spec does it that way:

otwarchive/spec/models/skin_spec.rb

Lines 170 to 185 in 942f4a3

	[
	"all",
	"screen",
	"handheld",
	"speech",
	"print",
	"braille",
	"embossed",
	"projection",
	"tty",
	"tv",
	"only screen and (max-width: 42em)",
	"only screen and (max-width: 62em)",
	"(prefers-color-scheme: dark)",
	"(prefers-color-scheme: light)"
	].each do |media_query|

While you're at it, please also add a tag with a backslash (\) to the array.

[Bilka2]

Looks good, thank you!