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.

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-5502 Fix page title of the adult content warning for chapters #5012

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

slavalamp
Copy link
Contributor

@slavalamp slavalamp commented Jan 6, 2025

Pull Request Checklist

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
Copy link
Contributor Author

...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
Copy link
Contributor

Bilka2 commented Jan 12, 2025

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 and others added 7 commits January 12, 2025 22:24
* AO3-5578 ActiveStorage copy script performance fixes

* Flush $stdout to force message visibility
* AO3-5578 Avoid unnecessary DB writes when copying icons

* Rubocop :fistshake:
…5019)

* Test listing in s3

* Other tasks & fixes

* Experiment with delayed upload

* Fixes

* Upload after txn

* Rubocop things

* Fixes

* Avoid duplicate attachments

* Revert "Avoid duplicate attachments"

This reverts commit 476bd02.
@slavalamp slavalamp force-pushed the AO3-5502_adult_content_page_title branch from 271fa22 to a37d2e6 Compare January 12, 2025 21:25
@slavalamp
Copy link
Contributor Author

slavalamp commented Jan 12, 2025

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
Copy link
Contributor Author

slavalamp commented Jan 12, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants