AO3-5447 Give 404 instead of 500 for works pages for nonexistent users #3345
Conversation
AO3-5447 Test mergers in collections as long as I'm here
* It thought 'the works page' meant /users/the/works and not just /works * Typo in admin tests had us checking a nonexistent user's works page AO3-5447 Fix tests that were accidentally checking the works pages for nonexistent users AO3-5447 Update the regex for user work paths * Thanks zz9pzza
* Less FactoryGirl.create * Less posted: true * Less repetition on the includes AO3-5447 Older tests shouldn't say should
| end | ||
| end | ||
|
|
||
| context "with a valid owner user" do |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [29/25]
| it "raises an error" do | ||
| params = { user_id: "nonexistent_user", pseud_id: "nonexistent_pseud" } | ||
| expect{ get :index, params: params }.to raise_error( | ||
| ActiveRecord::RecordNotFound, "Couldn't find user named 'nonexistent_user'") |
There was a problem hiding this comment.
Layout/MultilineMethodCallBraceLayout: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
| context "with an invalid pseud" do | ||
| it "raises an error" do | ||
| params = { user_id: "nonexistent_user", pseud_id: "nonexistent_pseud" } | ||
| expect{ get :index, params: params }.to raise_error( |
There was a problem hiding this comment.
Layout/SpaceBeforeBlockBraces: Space missing to the left of {.
| it "raises an error" do | ||
| params = { user_id: "nonexistent_user" } | ||
| expect{ get :index, params: params }.to raise_error( | ||
| ActiveRecord::RecordNotFound, "Couldn't find user named 'nonexistent_user'") |
There was a problem hiding this comment.
Layout/MultilineMethodCallBraceLayout: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
| context "with an invalid owner user" do | ||
| it "raises an error" do | ||
| params = { user_id: "nonexistent_user" } | ||
| expect{ get :index, params: params }.to raise_error( |
There was a problem hiding this comment.
Layout/SpaceBeforeBlockBraces: Space missing to the left of {.
| it "raises an error" do | ||
| params = { tag_id: "nonexistent_tag" } | ||
| expect{ get :index, params: params }.to raise_error( | ||
| ActiveRecord::RecordNotFound, "Couldn't find tag named 'nonexistent_tag'") |
There was a problem hiding this comment.
Layout/MultilineMethodCallBraceLayout: Closing method call brace must be on the line after the last argument when opening brace is on a separate line from the first argument.
| context "with an invalid owner tag" do | ||
| it "raises an error" do | ||
| params = { tag_id: "nonexistent_tag" } | ||
| expect{ get :index, params: params }.to raise_error( |
There was a problem hiding this comment.
Layout/SpaceBeforeBlockBraces: Space missing to the left of {.
| end | ||
| end | ||
| end | ||
|
|
There was a problem hiding this comment.
Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end.
| update_and_refresh_indexes('work') | ||
| get :index | ||
| expect(assigns(:works)).not_to include(work2) | ||
| end | ||
| end | ||
|
|
||
| context "with an owner tag" do | ||
| context "with a valid owner tag" do |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [53/25]
app/controllers/works_controller.rb
Outdated
| @@ -797,7 +797,9 @@ def mark_as_read | |||
| def load_owner | |||
| if params[:user_id].present? | |||
| @user = User.find_by(login: params[:user_id]) | |||
| if params[:pseud_id].present? | |||
| if @user.blank? | |||
| raise ActiveRecord::RecordNotFound, "Couldn't find user named '#{params[:user_id]}'" | |||
There was a problem hiding this comment.
Instead of manually raising the exception, we could use find_by!.
| get :index, params: { tag_id: @fandom.name } | ||
| expect(assigns(:works).items).to include(@work) | ||
| expect(assigns(:works).items).not_to include(@work2) | ||
| end | ||
|
|
||
| end | ||
|
|
||
| context "when tag is a synonym" do | ||
| before do | ||
| @fandom_synonym = create(:fandom, merger: @fandom) |
There was a problem hiding this comment.
We should use let to avoid leaking state between tests in instance variables. You don't need to change all the existing instance variables though.
| update_and_refresh_indexes('work') | ||
| get :index | ||
| expect(assigns(:works)).not_to include(work2) | ||
| end | ||
| end | ||
|
|
||
| context "with an owner tag" do | ||
| context "with a valid owner tag" do |
There was a problem hiding this comment.
Metrics/BlockLength: Block has too many lines. [49/25]
|
This needs updating |
|
I took a stab at the merge conflicts. 🤞 I didn't make a mess. I hate past me. |
redsummernight
added
Reviewed: Ready to Merge
and removed
Coder Has Actioned Review
labels
Issue
https://otwarchive.atlassian.net/browse/AO3-5447
Purpose
ActiveRecord::RecordNotFound) if you try to access the works page for a user that doesn't exist, regardless of whether you're aiming for the user's works page or a pseud'sload_ownerpossibilities, since that's the method I was editingTesting
Refer to Jira