Ao3-7219 persist work preference after comment#5798
Ao3-7219 persist work preference after comment#5798Dame-Zoom-A-Lot wants to merge 8 commits intootwcode:masterfrom
Conversation
| anchor: "comment_#{comment.id}" | ||
| } | ||
| end | ||
| default_options = if comment.ultimate_parent.is_a?(Tag) |
There was a problem hiding this comment.
rubocop formatting changes
|
Setting to draft mode while I add tests to default_rails_actions_spec |
| return raise_not_found if @commentable.blank? | ||
|
|
||
| return unless @commentable.class == Comment | ||
| return unless @commentable.instance_of?(Comment) |
There was a problem hiding this comment.
rubocop fix
| 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] }) |
There was a problem hiding this comment.
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
| view_full_work = if is_coming_from_chapter_view | ||
| false | ||
| elsif is_not_coming_from_work_view | ||
| if params[:view_full_work] == "true" |
There was a problem hiding this comment.
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:
- If user is coming from chapter by chapter view --> stay in chapter by chapter view
- If user is coming from full work view -> stay in full work view
- 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
| 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 } |
There was a problem hiding this comment.
rubocop changes
| comment = Comment.last | ||
| expect(flash[:error]).to be_nil | ||
| expect(response).to redirect_to(chapter_path(comment.commentable, show_comments: true, view_full_work: false, anchor: "comment_#{comment.id}")) | ||
| expect(response).to redirect_to(chapter_path(comment.commentable, show_comments: true, anchor: "comment_#{comment.id}")) |
There was a problem hiding this comment.
We were translating params[:view_full_work] = nil to params[:view_full_work] = false, which would have overridden user preference.
I'm guessing that this is why we might have had that workaround that introduced the original bug, where we were setting view_full_work = params[:view_full_work == true ||
| end | ||
| end | ||
|
|
||
| context "when work is not restricted for a logged in user" do |
There was a problem hiding this comment.
I considered adding tests for delete as well, since that also calls redirect_to_all_comments under the hood.
But I want to be mindful of taking up too much CI resources. Since both #create and #delete are using redirect_to_all_comments to redirect to either chapter view, work view, etc, I'm assuming just testing in the create route should be enough coverage.
|
@Bilka2 this is ready for review again. Sorry about the churn |
1 participant
Pull Request Checklist
as the first thing in your pull request title (e.g.
AO3-1234 Fix thing)until they are reviewed and merged before creating new pull requests.
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 of this line in the
redirect_to_all_commentsfunctionWe also had bugs earlier in the code when setting
options[:view_full_work]In
show_commentsandadd_comment_reply,options[:view_full_work]always evaluates as truthy regardless of whether it's 'true', or 'false', since params get evaluated as string values.I simplified the logic by basing redirects on the previous page (referer) rather than the view_full_work param.
Based on manual test + looking at the code, I'm assuming that there's 3 ways to create / edit / delete a comment
/works/:id)/works/:id/chapters/:id)/comments/:id)I was concerned that by updating the logic so we no longer rely on
params[:view_full_work], I'd break the flow from when a user deletes a comment from the comment view.But I confirmed that we never set the
view_full_workparam when a user is navigated to to thecommentspath in both of the below cases:Threadon a comment box, which navigates to thecomments/:idpathcomments/:idpathWe also did not set the
view_full_workparam when a user switched from entire work view to chapter view.Since the
view_full_workparam is set unpredictably, and inconsistently, I believe it would be more maintainable if we consolidate all the logic to the redirect function and go off of where the user last was instead.Testing Instructions
Please see Jira ticket - most recent test instructions are here with screenshots
References
This replaces an original PR: #5524
@Bilka2 thank you for your careful review. I agreed with your comment that the routing logic in the original PR was too convoluted, and that the simplest solution is to take the user back to where they were to begin with.
I was going to work off of my old PR, but it was so old that just starting from a clean slate was easier.
Credit
What name and pronouns should we use to credit you in the Archive of Our Own's Release Notes?
Zooms (any)
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.