AO3-5502 Fix page title of the adult content warning for chapters

[slavalamp]

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/browse/AO3-5502

Purpose

Fixes the page title on the adult content warning page for chapters of a work so that it's informative (has work title, chapter number, author and fandom) and consistent with the warning page for the whole work

Testing Instructions

See Jira ticket

References

none

Credit

slavalamp

[slavalamp]

...looks like I inserted an extra empty line by accident but it looks better that way, can it stay?

Rubocop is failing because of a line that I haven't changed, unrelated to this issue. (I think it could be satisfied by turning the negated if condition into an unless. Also it could return after access_denied and else wouldn't be needed and that whole block after it would have one less indentation level...)

[Bilka2]

Thank you, the code looks good! Could you add a test in features/other_a/page_title.feature that checks for the browser page title?

(I think it could be satisfied by turning the negated if condition into an unless. Also it could return after access_denied and else wouldn't be needed and that whole block after it would have one less indentation level...)

What about going even further and turning it into this?

access_denied and return unless @chapters.include?(@chapter)

[slavalamp]

Hopefully the test is good, I'm new to this and seeing what looks like plain English sentences but is supposed to be automated was a bit intimidating

What about going even further and turning it into this?

access_denied and return unless @chapters.include?(@chapter)

This doesn't work, I think it's because access_denied always returns false.

I wasn't sure if it was okay for me to touch those lines because they aren't directly related to the issue, but this sounds like an approval, so I commited my idea, let me know if I need to undo/change that then

---

Oops I forgot to put the AO3-5502 in my commit messages and tried to fix that and something scary happened. All these weird other commits used to be a "merged master into here" commit. I have no idea if there's a good way to fix this

[slavalamp]

The wiki says I should ignore all Rubocop errors not related to lines I've changed but doesn't say what to do when Reviewdog seems to be complaining about the whole file... I'll stop here for now, waiting for advice before I proceed

[Bilka2]

This doesn't work, I think it's because access_denied always returns false.

You're right! There's one spot in the code that does this anyway, which mislead me.

I wasn't sure if it was okay for me to touch those lines because they aren't directly related to the issue, but this sounds like an approval, so I commited my idea, let me know if I need to undo/change that then

Yep, you're good! We try to keep changes to unrelated lines low because it makes reviewing harder, merge conflicts are more likely and so on. That goes especially for rubocop because fixing all the violations can become a bit of an endless endavour. But that's just a guideline, so it's okay to have exceptions sometimes, like here.

I'd stop at this state in regards to rubocop and just tolerate the remaining errors instead of going down the rest of the rabbit hole.

Regarding the commits, I think this may be a result of merge commit that was then rebased. There's likely some git magic to undo this, but the current state should be fine too. For the future, we prefer merge commits (or the occasional commit with a missing issue number) over rebasing/force pushing. Because we squash on merge those messups become pretty much invisible in the final git history anyway.

[Bilka2]

I'm approving this because the test and code works as-is. Thank you for contributing 🎉

However, I left some extra comments on the test since you said you're new to the tests. You don't have to address these if you'd rather not, they are nitpicks and not important in the grand scheme of things!

[slavalamp]

I don't mind the nitpicks, I'd rather do this now so that someone else doesn't have to think about the same stuff again in the future

I've figured out I can configure my code editor to insert the final newline automatically so that shouldn't be an issue with my PRs anymore :)

Glad to know the commit mess is okay, I'll try not to have that happen again