Skip to content

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

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.

Sign up for GitHub

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

Closed
Dame-Zoom-A-Lot wants to merge 3 commits into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b1c8daa
A03-7219 explicitly handle view_full_work param as either text or option
Dame-Zoom-A-Lot Dec 27, 2025
f431157
AO3-7219 use single line if and fix option setting bug
Dame-Zoom-A-Lot Dec 28, 2025
b2cce49
AO3-7219 update cucumber test to match new desired behavior
Dame-Zoom-A-Lot Dec 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 12 additions & 3 deletions app/controllers/comments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,8 +55,10 @@ def check_blocked

def check_pseud_ownership
return unless params[:comment][:pseud_id]

Copy link
Copy Markdown
Contributor Author

@Dame-Zoom-A-Lot Dame-Zoom-A-Lot Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added some whitespace only rubocop auto corrects

pseud = Pseud.find(params[:comment][:pseud_id])
return if pseud && current_user && current_user.pseuds.include?(pseud)

flash[:error] = ts("You can't comment with that pseud.")
redirect_to root_path
end
Expand Down Expand Up @@ -180,14 +182,17 @@ def check_not_replying_to_spam
def check_permission_to_review
parent = find_parent
return if logged_in_as_admin? || current_user_owns?(parent)

flash[:error] = ts("Sorry, you don't have permission to see those unreviewed comments.")
redirect_to logged_in? ? root_path : new_user_session_path(return_to: request.fullpath)
end

def check_permission_to_access_single_unreviewed
return unless @comment.unreviewed?

parent = find_parent
return if logged_in_as_admin? || current_user_owns?(parent) || current_user_owns?(@comment)

flash[:error] = ts("Sorry, that comment is currently in moderation.")
redirect_to logged_in? ? root_path : new_user_session_path(return_to: request.fullpath)
end
Expand Down Expand Up @@ -384,7 +389,10 @@ def create
elsif @comment.unreviewed?
redirect_to_all_comments(@commentable)
else
redirect_to_comment(@comment, { view_full_work: (params[:view_full_work] == "true"), page: params[:page] })
# keep the user in chapter by chapter view if that was the view they were in when they commented
# An entire work view url would look like works/:id
user_chose_to_view_full_work = (current_user.try(:preference).try(:view_full_works) && request.referer&.match(/chapters/).nil?)
redirect_to_comment(@comment, { view_full_work: (params[:view_full_work] == "true" || user_chose_to_view_full_work), page: params[:page] })
end
end
end
Expand Down Expand Up @@ -689,8 +697,9 @@ def redirect_to_all_comments(commentable, options = {})
page: options[:page],
anchor: options[:anchor])
else
if commentable.is_a?(Chapter) && (options[:view_full_work] || current_user.try(:preference).try(:view_full_works))
commentable = commentable.work
if commentable.is_a?(Chapter)
user_chose_to_view_full_work = current_user.try(:preference).try(:view_full_works) && options[:view_full_work] != false
Copy link
Copy Markdown
Contributor Author

@Dame-Zoom-A-Lot Dame-Zoom-A-Lot Dec 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

commentable = commentable.work if options[:view_full_work] || user_chose_to_view_full_work
Comment on lines +700 to +702
Copy link
Copy Markdown
Contributor

@Bilka2 Bilka2 May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@Bilka2 Bilka2 May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor

@Bilka2 Bilka2 May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Dame-Zoom-A-Lot Dame-Zoom-A-Lot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor

@Bilka2 Bilka2 May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

end
redirect_to polymorphic_path(commentable,
options.slice(:show_comments,
Expand Down
2 changes: 1 addition & 1 deletion app/controllers/works_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,7 +191,7 @@ def show

# Users must explicitly okay viewing of entire work
if @work.chaptered?
if params[:view_full_work] || (logged_in? && current_user.preference.try(:view_full_works))
if params[:view_full_work] == "true" || (logged_in? && current_user.preference.try(:view_full_works) && params[:view_full_works] != "false")
Copy link
Copy Markdown
Contributor

@Bilka2 Bilka2 May 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Copy Markdown
Contributor Author

@Dame-Zoom-A-Lot Dame-Zoom-A-Lot May 5, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chapters = @work.chapters_in_order(
include_drafts: (logged_in_as_admin? ||
@work.user_is_owner_or_invited?(current_user))
Expand Down
3 changes: 1 addition & 2 deletions features/comments_and_kudos/comments_redirect.feature
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,8 +49,7 @@ Scenario: Posting top level comment on a chaptered work, with view full work in
And I post a comment "Woohoo"
Then I should see "Woohoo"
And I should see "Chapter 2" within "div#chapters"
# Once you've commented, it defaults back to your preference
And I should see "Chapter 1" within "div#chapters"
And I should not see "Chapter 1" within "div#chapters"

# REPLY COMMENTS

Expand Down
Loading