Skip to content

Ao3-7219 persist work preference after comment #5798

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

Open
Dame-Zoom-A-Lot wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Ao3-7219 persist work preference after comment #5798

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
0200152
base comment redirect on referer not param
Dame-Zoom-A-Lot May 9, 2026
461f4a6
clean up redirect logic and consider comments flow
Dame-Zoom-A-Lot May 10, 2026
d2c09d1
clean up unnecessary use of variables
Dame-Zoom-A-Lot May 10, 2026
e82263c
apply rubocop linting
Dame-Zoom-A-Lot May 10, 2026
04c1e40
follow view full work param when origin is not clear
Dame-Zoom-A-Lot May 10, 2026
c237094
apply rubocop for changed row
Dame-Zoom-A-Lot May 10, 2026
100bc19
remove change in line so I don't trigger rubocop i18n
Dame-Zoom-A-Lot May 10, 2026
90d41a9
work path should go to work not chapter
Dame-Zoom-A-Lot May 10, 2026
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
85 changes: 52 additions & 33 deletions app/controllers/comments_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,8 +93,10 @@

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

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 @@ -218,14 +220,17 @@
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 @@ -337,7 +342,7 @@
def index
return raise_not_found if @commentable.blank?

return unless @commentable.class == Comment
return unless @commentable.instance_of?(Comment)
Copy link
Copy Markdown
Contributor Author

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

Choose a reason for hiding this comment

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

rubocop fix


# we link to the parent object at the top
@commentable = @commentable.ultimate_parent
Expand Down Expand Up @@ -436,7 +441,7 @@
elsif @comment.unreviewed?
redirect_to_all_comments(@commentable)
else
redirect_to_comment(@comment, { view_full_work: (params[:view_full_work] == "true"), page: params[:page] })
redirect_to_comment(@comment, { view_full_work: (params[:view_full_work] && params[:view_full_work] == "true"), page: params[:page] })
Copy link
Copy Markdown
Contributor Author

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

Choose a reason for hiding this comment

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

former syntax was setting view_full_work = false when params[:view_full_work] was not set, and we should have been navigating the user based on their preference

This only sets view_full_work if params[:view_full_work] was defined to begin with

end
end
end
Expand All @@ -452,10 +457,11 @@
def update
updated_comment_params = comment_params.merge(edited_at: Time.current)
if @comment.update(updated_comment_params)
flash[:comment_notice] = ts('Comment was successfully updated.')
flash[:comment_notice] = ts("Comment was successfully updated.")

Check warning on line 460 in app/controllers/comments_controller.rb

View workflow job for this annotation

GitHub Actions / Rubocop 

[rubocop] reported by reviewdog 🐶
Prefer Rails built-in `t` helper over `ts` and move the text into the yml file. `ts` is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards

Raw Output:
app/controllers/comments_controller.rb:460:32: C: I18n/DeprecatedHelper: Prefer Rails built-in `t` helper over `ts` and move the text into the yml file. `ts` is not actually translatable. For more information, refer to https://github.com/otwcode/otwarchive/wiki/Internationalization-(i18n)-Standards
respond_to do |format|
format.html do
redirect_to comment_path(@comment) and return if @comment.unreviewed?

redirect_to_comment(@comment)
end
format.js # updating the comment in place
Expand Down Expand Up @@ -487,7 +493,7 @@
redirect_to_comment(parent_comment)
else
flash[:comment_notice] = ts("Comment deleted.")
redirect_to_all_comments(parent, {show_comments: true})
redirect_to_all_comments(parent, { show_comments: true })
end
end

Expand Down Expand Up @@ -612,10 +618,10 @@
format.html do
# if non-ajax it could mean sudden javascript failure OR being redirected from login
# so we're being extra-nice and preserving any intention to comment along with the show comments option
options = {show_comments: true}
options = { show_comments: true }
options[:add_comment_reply_id] = params[:add_comment_reply_id] if params[:add_comment_reply_id]
options[:view_full_work] = params[:view_full_work] if params[:view_full_work]
options[:page] = params[:page]
options[:view_full_work] = params[:view_full_work] if params[:view_full_work]
redirect_to_all_comments(@commentable, options)
end

Expand All @@ -640,11 +646,11 @@
@comment = Comment.new
respond_to do |format|
format.html do
options = {show_comments: true}
options = { show_comments: true }
options[:controller] = @commentable.class.to_s.underscore.pluralize
options[:anchor] = "comment_#{params[:id]}"
options[:page] = params[:page]
options[:view_full_work] = params[:view_full_work]
options[:view_full_work] = params[:view_full_work] if params[:view_full_work]
if @thread_view
options[:id] = @thread_root
options[:add_comment_reply_id] = params[:id]
Expand Down Expand Up @@ -706,44 +712,57 @@
# if necessary to display it
def redirect_to_comment(comment, options = {})
if comment.depth > ArchiveConfig.COMMENT_THREAD_MAX_DEPTH
if comment.ultimate_parent.is_a?(Tag)
default_options = {
controller: :comments,
action: :show,
id: comment.commentable.id,
tag_id: comment.ultimate_parent.to_param,
anchor: "comment_#{comment.id}"
}
else
default_options = {
controller: comment.commentable.class.to_s.underscore.pluralize,
action: :show,
id: (comment.commentable.is_a?(Tag) ? comment.commentable.to_param : comment.commentable.id),
anchor: "comment_#{comment.id}"
}
end
default_options = if comment.ultimate_parent.is_a?(Tag)
Copy link
Copy Markdown
Contributor Author

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

Choose a reason for hiding this comment

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

rubocop formatting changes

{
controller: :comments,
action: :show,
id: comment.commentable.id,
tag_id: comment.ultimate_parent.to_param,
anchor: "comment_#{comment.id}"
}
else
{
controller: comment.commentable.class.to_s.underscore.pluralize,
action: :show,
id: (comment.commentable.is_a?(Tag) ? comment.commentable.to_param : comment.commentable.id),
anchor: "comment_#{comment.id}"
}
end
# display the comment's direct parent (and its associated thread)
redirect_to(url_for(default_options.merge(options)))
else
# need to redirect to the specific chapter; redirect_to_all will then retrieve full work view if applicable
redirect_to_all_comments(comment.parent, options.merge({show_comments: true, anchor: "comment_#{comment.id}"}))
redirect_to_all_comments(comment.parent, options.merge({ show_comments: true, anchor: "comment_#{comment.id}" }))
end
end

def redirect_to_all_comments(commentable, options = {})
default_options = {anchor: "comments"}
default_options = { anchor: "comments" }
options = default_options.merge(options)

if commentable.is_a?(Tag)
redirect_to comments_path(tag_id: commentable.to_param,
add_comment_reply_id: options[:add_comment_reply_id],
delete_comment_id: options[:delete_comment_id],
page: options[:page],
anchor: options[:anchor])
add_comment_reply_id: options[:add_comment_reply_id],
delete_comment_id: options[:delete_comment_id],
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
end
is_coming_from_chapter_view = request.referer&.match(/chapters/).present?
is_not_coming_from_work_view = request.referer&.match(/works/).nil?
view_full_work = if is_coming_from_chapter_view
false
elsif is_not_coming_from_work_view
if params[:view_full_work] == "true"

Check warning on line 755 in app/controllers/comments_controller.rb

View workflow job for this annotation

GitHub Actions / Rubocop 

[rubocop] reported by reviewdog 🐶
Convert `if-elsif` to `case-when`.

Raw Output:
app/controllers/comments_controller.rb:755:26: C: Style/CaseLikeIf: Convert `if-elsif` to `case-when`.
Copy link
Copy Markdown
Contributor Author

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

Choose a reason for hiding this comment

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

We may want to set this as a helper, because I think this is how we'd actually want the logic to be for full work vs chapter view:

  1. If user is coming from chapter by chapter view --> stay in chapter by chapter view
  2. If user is coming from full work view -> stay in full work view
  3. If user is coming from a third location, such as the comment page, if we have a param set, then follow param, otherwise, stick to the user's preference

true
elsif params[:view_full_work] == "false"
false
else
current_user.try(:preference).try(:view_full_works).present?
end
else
true
end
commentable = commentable.work if commentable.is_a?(Chapter) && view_full_work
redirect_to polymorphic_path(commentable,
options.slice(:show_comments,
:add_comment_reply_id,
Expand Down
6 changes: 2 additions & 4 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 Expand Up @@ -115,5 +114,4 @@ Scenario: Posting top level comment on a middle chapter, while in temporary view
When I reply to a comment with "Supercalifragelistic"
Then I should see "Supercalifragelistic"
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"
112 changes: 102 additions & 10 deletions spec/controllers/comments/comments_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,56 @@
end

describe "GET #cancel_comment_delete" do
it "redirects to the comment on the commentable without an error" do
get :cancel_comment_delete, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(chapter_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
context "when cancelling from chapter by chapter view" do
before do
request.env["HTTP_REFERER"] = "/works/1/chapters/1"
end

it "redirects to the comment on the chapter view without an error" do
get :cancel_comment_delete, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(chapter_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
end
end

context "when cancelling from the work view" do
before do
request.env["HTTP_REFERER"] = "/works/1"
end

it "redirects to the comment on the work view without an error" do
get :cancel_comment_delete, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(work_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
end
end

context "when cancelling from comments view" do
before do
request.env["HTTP_REFERER"] = "/comments/1"
end

context "when user set preference to full work view" do
before do
known_user = create(:user)
known_user.preference.update!(view_full_works: true)
fake_login_known_user(known_user)
end

it "redirects to the comment on the work view without an error" do
get :cancel_comment_delete, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(work_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
end
end

context "when user set preference to chapter by chapter view" do
it "redirects to the comment on the chapter view without an error" do
get :cancel_comment_delete, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(chapter_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
end
end
end
end

Expand All @@ -91,10 +137,54 @@
before { fake_login_known_user(comment.pseud.user) }

context "when the format is html" do
it "redirects to the comment on the commentable without an error" do
get :cancel_comment_edit, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(chapter_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
context "when user is navigating from chapter view" do
before do
request.env["HTTP_REFERER"] = "/works/1/chapters/1"
end

it "redirects to the comment on the chapter view without an error" do
get :cancel_comment_edit, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(chapter_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
end
end

context "when user is navigating from work view" do
before do
request.env["HTTP_REFERER"] = "/works/1"
end

it "redirects to the comment on the full work view without error" do
get :cancel_comment_edit, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(work_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
end
end

context "when user is navigating from comment view" do
before do
request.env["HTTP_REFERER"] = "/comments/1"
end

context "when user preference is full work view" do
before do
comment.pseud.user.preference.update!(view_full_works: true)
end

it "redirects to the comment on the full work view without error" do
get :cancel_comment_edit, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(work_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
end
end

context "when user preference is not full work view" do
it "redirects to the comment on the chapter view without error" do
get :cancel_comment_edit, params: { id: comment.id }
expect(flash[:error]).to be_nil
expect(response).to redirect_to(chapter_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}"))
end
end
end
end

Expand Down Expand Up @@ -298,7 +388,8 @@
delete :destroy, params: { id: comment.id }
expect(flash[:comment_notice]).to eq "Comment deleted."
it_redirects_to_simple(work_path(work, show_comments: true, anchor: :comments))
expect { comment.reload }.to raise_exception(ActiveRecord::RecordNotFound)
expect { comment.reload }
Copy link
Copy Markdown
Contributor Author

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

Choose a reason for hiding this comment

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

rubocop changes

.to raise_exception(ActiveRecord::RecordNotFound)
end

it "GET #add_comment_reply redirects to the work with an error" do
Expand Down Expand Up @@ -382,7 +473,8 @@
delete :destroy, params: { id: comment.id }
expect(flash[:comment_notice]).to eq "Comment deleted."
it_redirects_to_simple(work_path(work, show_comments: true, anchor: :comments))
expect { comment.reload }.to raise_exception(ActiveRecord::RecordNotFound)
expect { comment.reload }
.to raise_exception(ActiveRecord::RecordNotFound)
end
end
end
Expand Down
Loading
Loading