Skip to content

#6563 improve case contact workflow for supervisor and admin #6679

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
UtkarshDixit-97 wants to merge 6 commits into
base: main
Choose a base branch
from
Open

#6563 improve case contact workflow for supervisor and admin #6679

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f43ffed
work done for issue number - #6563
UtkarshDixit-97 Jan 27, 2026
8c6442a
debugger removed
UtkarshDixit-97 Jan 27, 2026
4178a8f
linting error resolved
UtkarshDixit-97 Jan 27, 2026
7066812
failing tests were corrected to align with new feature
UtkarshDixit-97 Jan 27, 2026
9201608
if supervisor adds a case details, he should see view/edit button not…
UtkarshDixit-97 Jan 27, 2026
fd1d775
Update app/views/case_contacts/_case_contact.html.erb
compwron Jan 28, 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
13 changes: 12 additions & 1 deletion app/controllers/case_contacts_controller.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ class CaseContactsController < ApplicationController
before_action :set_case_contact, only: %i[edit destroy]
before_action :set_contact_types, only: %i[new edit create]
before_action :require_organization!
after_action :verify_authorized, except: %i[leave]
before_action :set_volunteer, only: %i[impersonate_and_edit]
after_action :verify_authorized, except: %i[leave impersonate_and_edit]

def index
load_case_contacts
Expand Down Expand Up @@ -61,6 +62,12 @@ def leave
redirect_back_to_referer(fallback_location: case_contacts_path)
end

def impersonate_and_edit
impersonate_user(@volunteer)

redirect_to casa_case_path(params[:casa_case_id])
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

Redirecting to casa_case_path(params[:casa_case_id]) after impersonation can fail because volunteers are only authorized to view cases they are actively assigned to (see CasaCasePolicy#show?). If the case contact was authored by a volunteer who is no longer actively assigned, the impersonated session will be blocked from the case details page, defeating the workflow.

Consider redirecting directly to the case contact edit flow (passing the case contact id) rather than the case details page, or otherwise ensuring the impersonated volunteer can access the target case.

Suggested change
redirect_to casa_case_path(params[:casa_case_id])
redirect_to case_contact_form_path(:details, case_contact_id: params[:id])

Copilot uses AI. Check for mistakes.
end
Comment on lines +65 to +69
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

New behavior is introduced via impersonate_and_edit, but there’s no request/system spec covering it (especially authorization and the post-impersonation redirect behavior). The existing suite already has request coverage for CaseContactsController actions (e.g., spec/requests/case_contacts_spec.rb).

Add request specs for this endpoint for (1) authorized supervisor/admin, (2) unauthorized volunteer, and (3) cross-org volunteer id, so regressions in impersonation security/redirect behavior are caught.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@compwron compwron Jan 28, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Comment on lines +65 to +69
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

impersonate_and_edit performs an impersonation without any Pundit authorization (and verify_authorized explicitly skips this action). This allows any authenticated user who can hit this endpoint to impersonate arbitrary volunteers by id, bypassing VolunteerPolicy#impersonate?.

Authorize the target user (e.g., authorize @volunteer, :impersonate?) and remove the action from the verify_authorized exception list so authorization is enforced consistently.

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@compwron compwron Jan 28, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback


private

def update_or_create_additional_expense(all_ae_params, cc)
Expand Down Expand Up @@ -98,4 +105,8 @@ def build_draft_case_ids(params, casa_cases)

[]
end

def set_volunteer
@volunteer = Volunteer.find(params[:creator_id])
end
Comment on lines +109 to +111
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

set_volunteer uses Volunteer.find(params[:creator_id]) with no scoping to the current organization. Combined with the missing authorization on impersonate_and_edit, this enables cross-organization impersonation by guessing ids.

Even with authorization added, it’s safer to scope the lookup (e.g., via policy_scope(Volunteer) or current_organization.volunteers) and to handle non-volunteer creators gracefully (since CaseContact#creator can be a Supervisor/CasaAdmin).

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@compwron compwron Jan 28, 2026

Choose a reason for hiding this comment

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

@copilot open a new pull request to apply changes based on this feedback

Copy link
Collaborator

@kcdragon kcdragon Jan 28, 2026

Choose a reason for hiding this comment

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

This is my first time seeing Copilot reviews so maybe I'm misunderstanding something. Why would we open a new PR instead of modifying this PR? I don't think we'd want to merge it without this change since its a security issue.

Copy link
Collaborator

@compwron compwron Jan 28, 2026

Choose a reason for hiding this comment

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

I would merge the new PR into this PR before merging this PR into main
This also allows multiple concurrent changes
Feel free to do the changes by hand instead if you prefer

end
10 changes: 8 additions & 2 deletions app/views/case_contacts/_case_contact.html.erb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,8 +38,14 @@
<% if Pundit.policy(current_user, contact).update? %>
<%= render "case_contacts/followup", contact: contact, followup: contact.requested_followup %>
<div class="mr-2">
<%= link_to edit_case_contact_path(contact), class: "text-danger", data: { turbo: false } do %>
<i class="lni lni-pencil-alt"></i> Edit
<% if (current_user.casa_admin? || current_user.supervisor?) && contact.creator != current_user %>
<%= link_to case_contacts_impersonate_and_edit_path(creator_id: contact.creator.id, casa_case_id: @casa_case), class: "text-danger", data: { turbo: false } do %>
<i class="lni lni-pencil-alt"></i> View/Edit
<% end %>
<% else %>
<%= link_to edit_case_contact_path(contact), class: "text-danger", data: { turbo: false } do %>
<i class="lni lni-pencil-alt"></i> Edit
<% end %>
<% end %>
</div>
<% end %>
Expand Down
1 change: 1 addition & 0 deletions config/routes.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
# Feature flag for new case contact table design
get "case_contacts/new_design", to: "case_contacts/case_contacts_new_design#index"
post "case_contacts/new_design/datatable", to: "case_contacts/case_contacts_new_design#datatable", as: "datatable_case_contacts_new_design"
get "case_contacts/impersonate_and_edit", to: "case_contacts#impersonate_and_edit"
resources :case_contacts, except: %i[create update show], concerns: %i[with_datatable] do
member do
post :restore
Expand Down
5 changes: 2 additions & 3 deletions spec/views/case_contacts/case_contact.html.erb_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,17 +73,16 @@
enable_pundit(view, admin)
allow(view).to receive(:current_user).and_return(admin)
end

context "occurred_at is before the last day of the month in the quarter that the case contact was created" do
let(:case_contact) { build_stubbed(:case_contact, creator: volunteer) }
let(:case_contact2) { build_stubbed(:case_contact, deleted_at: Time.current, creator: volunteer) }

it "shows edit button" do
it "shows view/edit button" do
assign :case_contact, case_contact
assign :casa_cases, [case_contact.casa_case]

render(partial: "case_contacts/case_contact", locals: {contact: case_contact})
expect(rendered).to have_link(nil, href: "/case_contacts/#{case_contact.id}/edit")
expect(rendered).to have_link(nil, href: "/case_contacts/impersonate_and_edit?creator_id=#{case_contact.creator.id}")
end
Comment on lines +80 to 86
Copy link

Copilot AI Jan 28, 2026

Choose a reason for hiding this comment

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

This spec asserts the impersonation URL without casa_case_id, but the partial builds the link using casa_case_id: @casa_case when it’s rendered from the case details page (the primary usage). As written, the spec passes even if @casa_case is accidentally missing, which would later cause the controller redirect to fail at runtime.

Assign @casa_case in the spec and assert the full generated href (preferably using the route helper), and also assert the visible link text (e.g., "View/Edit") to avoid matching an unrelated link.

Copilot uses AI. Check for mistakes.

it "shows make reminder button" do
Expand Down
Loading