[#57664] ActiveRecord::RecordNotFound in ActivitiesController#index #16688
Conversation
There was a problem hiding this comment.
Thanks for the fix. I have a few remarks.
The PR description is lacking explanations, making it harder to review. It could make it even harder for any future developer stumbling upon it. Same for the commit message, it's not helpful. It should describe what you fixed and why. The screenshot is not helpful as well: I thought it was the error happening before the fix. Just a label saying "After fix" would have helped.
If I understand correctly, the user is fetched in #determine_author. It looks up only active users. If the user is inactive (locked for instance), then the method will raise an ActiveRecord::RecordNotFound error. The index action is intended to catch it, but it can't work outside of this method. determine_author being called in a before action, that's why it's not caught.
You fixed it by using rescue_from helper which works in action AND in action hooks, fixing the issue.
Here is a proposal for the commit message:
Fix error when viewing inactive user activity
`#determine_author` can raise `ActiveRecord::RecordNotFound` if user does not
exist or is inactive. Use `rescue_from` to catch it and send appropriate
response.
Fixes https://community.openproject.org/wp/57664
Fixes https://appsignal.com/openproject-gmbh/sites/66b224a4d30d867bed8a1772/exceptions/incidents/461
Can you also modify your test to test against an inactive user instead of a non-existing user id?
Also this is not a critical bug and it's not a regression either, so it should target current dev branch.
| describe "with not existent user_id" do | ||
| it "renders 404" do | ||
| get "index", params: { project_id: project.id, user_id: 123123123 } | ||
| expect(response).to have_http_status(:not_found) | ||
| expect(response).to render_template "common/error" | ||
| end |
There was a problem hiding this comment.
Can you add a test with an inactive user instead? It would better reflect what happened in production.
|
@cbliard Thanks for your review! |
ba1ash
force-pushed
the
bug/57664-activerecordrecordnotfound-in-activitiescontrollerindex
branch
from
007fe56
to
dba9f10
Compare
ba1ash
force-pushed
the
bug/57664-activerecordrecordnotfound-in-activitiescontrollerindex
branch
from
dba9f10
to
5690f52
Compare
`#determine_author` can raise `ActiveRecord::RecordNotFound` if user does not exist or is inactive. Use `rescue_from` to catch it and send appropriate response. Fixes https://community.openproject.org/wp/57664 Fixes https://appsignal.com/openproject-gmbh/sites/66b224a4d30d867bed8a1772/exceptions/incidents/461 Co-authored-by: Christophe Bliard <[email protected]>
ba1ash
force-pushed
the
bug/57664-activerecordrecordnotfound-in-activitiescontrollerindex
branch
from
5690f52
to
e7a482d
Compare
ba1ash
deleted the
bug/57664-activerecordrecordnotfound-in-activitiescontrollerindex
branch
Ticket
https://community.openproject.org/work_packages/57664
What are you trying to accomplish?
Fix error when viewing inactive user activity.
#determine_authorcan raiseActiveRecord::RecordNotFoundif user does not exist or is inactive. Userescue_fromto catch it and send appropriate response.Screenshots
What approach did you choose and why?
Merge checklist