AO3-7219 Keep users with 'show the whole work by default' enabled in chapter by chapter mode when commenting

[Dame-Zoom-A-Lot]

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.

Note on tests:

I decided not to add tests since comments_controller_spec did not have any tests for the create endpoint, nor tests that would verify redirect_to_all_comments behaviors. works_controller_spec likewise only had tests for #navigate.

I'm assuming this was a conscious decision to avoid bogging down CI since there's a robust QA team.

I'm happy to put in some extra work to add net new tests for these endpoints if the testing gap was due to lack of resources, but that would increase the scope of this PR.

Issue

https://otwarchive.atlassian.net/browse/AO3-7219

Purpose

Fix the bug where users who had "show the whole work by default" enabled were getting redirected to the full work view after commenting.

This was happening because we were showing the whole work

if params[:view_full_work] || (logged_in? && current_user.preference.try(:view_full_works))

Since params[:view_full_work] is either "true" or "false" this always evaluates to true if it is set.
But even if params[:view_full_work] == "false", if the user had their preference set to view_full_works, we would have shown the full work view and not the chapter by chapter view.

There was also a routing error in the comments_controller where
if commentable.is_a?(Chapter) && (options[:view_full_work] || current_user.try(:preference).try(:view_full_works)) we set the chapter commentable to a work commentable, which also routed to the entire work view.

I added the same logic here that I added in the works controller

Testing Instructions

Reported bug

1. Log in
2. Have the "Show the whole work by default" preference enabled
3. Open a work in chapter by chapter mode
4. Leave a comment or reply to a comment on a chapter
5. Confirm users stays in chapter by chapter mode

Ensure other behavior did not change

1. Log in
2. Have the "Show the whole work by default" preference enabled
3. Open a work in entire work mode
4. Leave a comment or reply to a comment on a chapter
5. Confirm user stays in entire work mode, and that the new comment is tied to the last chapter

Testing Notes
While testing, I found that when the new comment would be in page 2+, the user does not get redirected to the comment section. This is an unrelated issue, and potentially a bug. To test

1. Log in
2. Open work in whatever mode
3. Leave a comment that would show in page 2 of the comments view
4. User is redirected to the work view upon submitting a comment, but the stays at the top of the page and does not get navigated to the comments box

How can the Archive's QA team verify that this is working as you intended?

If you have a Jira account with access, please update or comment on the issue
with any new or missing testing instructions instead.

You can remove this section if there are already full testing instructions in the Jira issue.

Credit

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

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.

[Dame-Zoom-A-Lot]

added some whitespace only rubocop auto corrects

[Dame-Zoom-A-Lot]

it would be nice to have a unified way to see when a user wanted to to view full work vs chaptered view. But all the different helper methods and endpoints had slightly different ways we would need to use to identify them.

[Bilka2]

The comment creation tests are in spec/controllers/comments/default_rails_actions_spec.rb, works show specs are in spec/controllers/works/default_rails_actions_spec.rb. I think we should definitely cover the changes here with automated tests because they're quite intricate between the many different cases (comment from work with or without the param, from chapter, and those with and without the preference). Though hopefully a lot of that is already covered by tests, whether it's rspec or cucumber

[Bilka2]

First thing I noticed here is that it should be params[:view_full_work], singular. So right now that second check isn't doing anything

However, from what I can tell, that second check doesn't need to do anything to fix the issue here: If we're in chapter by chapter mode and want to stay there, then the comments controller redirects to the chapter directly. So we should never end up in this controller with view_full_work set to anything other than "true" or nothing. Because the general idea is that view_full_work is only set to false when the user specifically declined being in that mode, which only happens inside comments_controller which already handles all that within redirect_to_all_comments

So I would remove the && params[:view_full_works] != "false" here

[Dame-Zoom-A-Lot]

thanks for the careful review. It's been a while, but I think not having that second check was causing some unexpected behaviors. I'm trying to get set back up so I can test.

[Bilka2]

Currently, this can leak a "view_full_work" param that is set to false into the URL it redirects to. That doesn't do anything, and it's ambiguous between the user having the preference disabled, coming from a chapter URL, or just view_full_work not being present. Since this controller handles the redirect itself, only dumping the user to the full work if they actually wanted it, I think we should set options[:view_full_work] to nil here if we're not redirecting to the work

[Bilka2]

That suggestion summarizes to the following decision tree to me:

if options[:view_full_work] it's "true" or true
  go to the work in full work mode (so including the param)
  The preference is irrelevant because either they manually chose full work mode or it was set to the bool true in your change above that checks the preference

if options[:view_full_work] is false
  go to the chapter, always
  because we already checked the preference in the only spot that can set the option to false (your change above)
  Don't include the param in the URL because we're going to a chapter, not a work

options[:view_full_work] can never be "false"

so if options[:view_full_work] is blank
  the user hasn't chosen anything
  Do the preference check and decide chapter/work based on that. Don't set view_full_work

[Bilka2]

The current decision tree here is

if options[:view_full_work] is "true" or true
  go to the work in full work mode

if options[:view_full_work] is "true" or true or blank
  if the preference is true
    go to the work
    view full work mode is set to options[:view_full_work] (so may be blank)
  
otherwise
  go to the chapter
  view full work mode may be blank or false

this is mostly the same as my suggested decision tree, the only difference is the view_full_work param possibly being set when redirecting to a chapter. But the current code took me ages to parse. Not sure if that is a me issue or not, so I'm leaving this analysis here and you can decide how you integrate my suggestion and whether you change how the checks here are done or not

[Dame-Zoom-A-Lot]

Agreed on not setting view_full_work unless it's true. I can edit.

I also had trouble following what we were doing with view_full_work, and honestly, I'm a bit concerned that this could have been the intended behavior because the comments_redirect test was specifically checking that users reverted back to full works view.

[Bilka2]

honestly, I'm a bit concerned that this could have been the intended behavior because the comments_redirect test was specifically checking that users reverted back to full works view.

That's a fair concern! I went digging into the history of that test, and it looks like AO3-4487 added those comments and solidified this behaviour and doesn't have any context on whether there was a discussion on whether it was wanted, it's a simple "fix test to match existing behaviour" issue. The initial PR that added this behaviour is #327 and the relevant Jira issues are AO3-1514, AO3-1515.

Neither of those explicitly defines the behaviour here, so I think we aren't undoing a specific past decision. Besides, when this Jira issue was discussed before getting created, it was a very short "Yup, definitely a bug." discussion so even if we considered this intended approximately 10 years ago, we no longer do now.

[Dame-Zoom-A-Lot]

@Bilka2 thanks for the careful review.

Agreed on both your points

1. Current logic is too convoluted
2. There aren't enough tests for what's honestly a pretty complicated flow

This PR's been open a little too long for me to be able to easily work off of it. I started a fresh PR here

Major change is that I'm ditching the view_full_works parameter and working off of the referer. Please see the Jira tickets for all routes I tested.