New Case Contacts Table: row actions menu#6834
New Case Contacts Table: row actions menu#6834cliftonmcintosh wants to merge 22 commits intorubyforgood:mainfrom
Conversation
36e1e71 to
7a11bdd
Compare
Co-Authored-By: claude-sonnet-4-6 <noreply@anthropic.com>
Each row in the new case contacts datatable now has a Bootstrap dropdown menu with Edit, Delete, and Set/Resolve Reminder actions. Items are shown or hidden based on per-row Pundit permissions already returned by the datatable JSON. Delete and Resolve Reminder use AJAX so the table reloads in place. Set Reminder opens a SweetAlert2 dialog for an optional note. Backend controllers now respond to JSON for destroy and followup resolve so jQuery does not follow the HTML redirect. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The delete action in the new case contacts table now shows a confirmation dialog before sending the DELETE request. Cancelling the dialog aborts the request. Tests updated to reflect async flow. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Edit and Delete are now rendered as disabled dropdown items when the current user lacks permission, rather than being omitted from the menu. Disabled items retain aria-disabled="true" for screen reader accessibility. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
b50076e to
e59e9ee
Compare
1600b95 to
92abe1e
Compare
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
92abe1e to
c4370f3
Compare
There was a problem hiding this comment.
Pull request overview
Implements a per-row ellipsis (⋮) actions dropdown for the new case contacts DataTable at /case_contacts/new_design, wiring the UI to Pundit permissions and followup state.
Changes:
- Extend the case contacts datatable JSON payload with per-row permissions, edit path, and requested-followup metadata.
- Add row actions UI (Edit/Delete/Set or Resolve Reminder) and AJAX handlers in the new-design table.
- Adjust controllers to respond with JSON-friendly
204 No Contentfor AJAX destroy/resolve, and expand test coverage (system/request/datatable/Jest).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/system/case_contacts/case_contacts_new_design_spec.rb | Adds system coverage for action menu behavior, followup toggling, and permission states. |
| spec/requests/case_contacts/case_contacts_new_design_spec.rb | Verifies datatable JSON includes permission/action metadata per row. |
| spec/datatables/case_contact_datatable_spec.rb | Covers new datatable fields (can_edit, can_destroy, edit_path, followup_id). |
| app/javascript/src/dashboard.js | Replaces placeholder ellipsis with Bootstrap dropdown and adds AJAX handlers for row actions. |
| app/javascript/src/case_contact.js | Exports fireSwalFollowupAlert for reuse by the dashboard action menu. |
| app/javascript/tests/dashboard.test.js | Adds Jest coverage for menu rendering and action click handlers (Swal + AJAX). |
| app/datatables/case_contact_datatable.rb | Adds current_user to compute per-row Pundit permissions and followup metadata. |
| app/controllers/case_contacts_controller.rb | Returns 204 No Content for JSON destroy requests to support AJAX. |
| app/controllers/case_contacts/followups_controller.rb | Returns 204 No Content for JSON resolve requests to support AJAX. |
| app/controllers/case_contacts/case_contacts_new_design_controller.rb | Passes current_user into the datatable initializer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| policy = CaseContactPolicy.new(current_user, case_contact) | ||
| requested_followup = case_contact.followups.find(&:requested?) | ||
|
|
There was a problem hiding this comment.
case_contact.followups.find(&:requested?) forces loading the full followups association for each case contact and then scans it in Ruby. With .includes(:followups) this can pull a lot of rows into memory even though only the requested followup (if any) is needed. Consider fetching only requested followups (e.g., via a scoped query keyed by the page of case_contact IDs) and then looking up the ID per row, to reduce memory and object allocations.
There was a problem hiding this comment.
has_followup was re-scanning the already-loaded followups collection with any?(&:requested?) when requested_followup was already computed on the line above. Simplified to requested_followup.present?.
Regarding the suggestion to scope the followups query to only load requested followups: the current includes(:followups) does load all followups per record, but in practice each case contact is unlikely to accumulate enough followups for this to be significant. I left this as-is for now but it's a reasonable future improvement if it becomes a concern.
| type: 'DELETE', | ||
| dataType: 'json', | ||
| headers: { 'X-CSRF-Token': csrfToken() }, | ||
| success: () => table.ajax.reload() | ||
| }) |
There was a problem hiding this comment.
The AJAX call declares dataType: 'json', but the controller responds with head :no_content (204) for JSON. With jQuery this can surface as a JSON parse error and skip success. Consider removing dataType (or setting it to 'text') or returning a JSON body. Also, table.ajax.reload() defaults to resetting pagination; if the intent is to reload “in place”, pass resetPaging = false so the user stays on the same page.
app/javascript/src/dashboard.js
Outdated
| const { value: text, isConfirmed } = await fireSwalFollowupAlert() | ||
| if (!isConfirmed) return | ||
| const params = text ? { note: text } : {} | ||
| $.post(`/case_contacts/${id}/followups`, params, () => table.ajax.reload()) |
There was a problem hiding this comment.
This POST does not include the CSRF token, while the other row actions explicitly set X-CSRF-Token. If @rails/ujs isn’t wiring jQuery’s $.post, this request can fail with an invalid authenticity token. Prefer using $.ajax (type POST) and set the same CSRF header, and keep pagination by reloading the DataTable without resetting paging.
| $.post(`/case_contacts/${id}/followups`, params, () => table.ajax.reload()) | |
| $.ajax({ | |
| url: `/case_contacts/${id}/followups`, | |
| type: 'POST', | |
| data: params, | |
| dataType: 'json', | |
| headers: { 'X-CSRF-Token': csrfToken() }, | |
| success: () => table.ajax.reload(null, false) | |
| }) |
There was a problem hiding this comment.
Removed dataType: 'json' from the Delete and Resolve Reminder $.ajax calls. I also replaced $.post with $.ajax for Set Reminder to make all three actions consistent. To compensate for losing the Accept: application/json header that dataType was providing implicitly, I added it explicitly to all three calls so Rails correctly routes to the format.json branch. I also added respond_to to followups#create, which was missing and would have caused a redirect to be followed instead of a 204 being returned.
Also changed all three table.ajax.reload() calls to table.ajax.reload(null, false) to keep the user on their current page after an action.
| type: 'PATCH', | ||
| dataType: 'json', | ||
| headers: { 'X-CSRF-Token': csrfToken() }, | ||
| success: () => table.ajax.reload() | ||
| }) |
There was a problem hiding this comment.
Same concern as delete: dataType: 'json' combined with a 204 No Content JSON response can trigger jQuery parse errors. Consider removing/adjusting dataType or returning a JSON payload. Also consider reloading without resetting paging to keep the user’s place in the table.
|
I may have introduced a lint issue while trying to resolve a merge conflict - feel free to overwrite that but keep the performance improvement please |
…esign-row-actions-menu
👍🏽 I also accidentally used github's sync fork on this branch instead of main. I'll assess what changed. I'll also take a look at the copilot review to see if things need adjusting. |
The missing comma after `has_followup` caused a Ruby syntax error, preventing the file from loading entirely. Also simplified `has_followup` to reuse the already-computed `requested_followup` value instead of scanning the collection a second time. Added a regression test asserting all action metadata keys are present in a single row. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jQuery parses the response body when dataType is 'json', causing a parse error on the 204 No Content response and skipping the success callback. Removing dataType lets jQuery accept the empty response and reload the table. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
$.post does not set the X-CSRF-Token header, which can cause Rails to reject the request with an invalid authenticity token error. Replaced $.post with $.ajax to match the pattern used by Delete and Resolve Reminder. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…actions table.ajax.reload() resets to page 1 by default. Passing (null, false) keeps the user on the current page after Delete, Set Reminder, and Resolve Reminder actions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…wups#create Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ntactDatatable Per-row policy checks call same_org? which loads casa_org through casa_case for every record. Added preload for :casa_org and :creator_casa_org since includes cannot resolve has_one :through when left_joins is already in the chain. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a missing comma in |
|
@compwron This is ready for review when you have time. Thanks! |
| .joins("INNER JOIN users creators ON creators.id = case_contacts.creator_id") | ||
| .left_joins(:casa_case) | ||
| .includes(:casa_case, :contact_types, :contact_topics, :followups, :creator, contact_topic_answers: :contact_topic) | ||
| .preload(:casa_org, :creator_casa_org) |
There was a problem hiding this comment.
To handle an N+1 warning
21:01:42 web.1 | N+1 queries detected:
21:01:42 web.1 | SELECT "casa_orgs".* FROM "casa_orgs" INNER JOIN "casa_cases" ON "casa_orgs"."id" = "casa_cases"."casa_org_id" WHERE "casa_cases"."id" = $1 LIMIT $2
21:01:42 web.1 | SELECT "casa_orgs".* FROM "casa_orgs" INNER JOIN "casa_cases" ON "casa_orgs"."id" = "casa_cases"."casa_org_id" WHERE "casa_cases"."id" = $1 LIMIT $2
21:01:42 web.1 | SELECT "casa_orgs".* FROM "casa_orgs" INNER JOIN "casa_cases" ON "casa_orgs"."id" = "casa_cases"."casa_org_id" WHERE "casa_cases"."id" = $1 LIMIT $2
21:01:42 web.1 | SELECT "casa_orgs".* FROM "casa_orgs" INNER JOIN "casa_cases" ON "casa_orgs"."id" = "casa_cases"."casa_org_id" WHERE "casa_cases"."id" = $1 LIMIT $2
21:01:42 web.1 | SELECT "casa_orgs".* FROM "casa_orgs" INNER JOIN "casa_cases" ON "casa_orgs"."id" = "casa_cases"."casa_org_id" WHERE "casa_cases"."id" = $1 LIMIT $2
21:01:42 web.1 | SELECT "casa_orgs".* FROM "casa_orgs" INNER JOIN "casa_cases" ON "casa_orgs"."id" = "casa_cases"."casa_org_id" WHERE "casa_cases"."id" = $1 LIMIT $2
21:01:42 web.1 | Call stack:
21:01:42 web.1 | app/policies/case_contact_policy.rb:57:in 'CaseContactPolicy#same_org?'
21:01:42 web.1 | app/policies/application_policy.rb:72:in 'ApplicationPolicy#is_supervisor_same_org?'
21:01:42 web.1 | app/policies/application_policy.rb:89:in 'ApplicationPolicy#admin_or_supervisor_same_org?'
21:01:42 web.1 | app/policies/case_contact_policy.rb:45:in 'CaseContactPolicy#creator_or_supervisor_or_admin?'
21:01:42 web.1 | app/policies/case_contact_policy.rb:11:in 'CaseContactPolicy#update?'
21:01:42 web.1 | app/datatables/case_contact_datatable.rb:49:in 'block in CaseContactDatatable#data'
21:01:42 web.1 | app/datatables/case_contact_datatable.rb:21:in 'Enumerable#map'
21:01:42 web.1 | app/datatables/case_contact_datatable.rb:21:in 'CaseContactDatatable#data'
21:01:42 web.1 | app/datatables/application_datatable.rb:15:in 'ApplicationDatatable#as_json'
21:01:42 web.1 | app/controllers/case_contacts/case_contacts_new_design_controller.rb:15:in 'CaseContacts::CaseContactsNewDesignController#datatable'| let!(:draft_contact) { create(:case_contact, casa_case: casa_case_for_volunteer, creator: volunteer, status: "started", occurred_at: 10.days.ago) } | ||
|
|
||
| before do | ||
| Capybara.reset_sessions! |
There was a problem hiding this comment.
I am curious to know why was this line needed?
There was a problem hiding this comment.
Without it, the test regularly failed in CI because the admin, who is signed in in the before block at the top of this file, stayed logged in. This made the test for the enable vs disable delete for volunteers invalid. The tests for "shows Delete as disabled for an active contact" for a volunteer would fail because the admin was logged in and admins can delete any case contact. The screenshots captured by CI showed that the admin was still logged in.
There was a problem hiding this comment.
Here's an example screenshot from a test run earlier. We should have the volunteer (User 3) logged in and accessing the menu for their own case contact. But up in the top right we can see that User 2 is logged in.
An aside: note that the menu is partially hidden by the bottom of the data table. I mention this and why I didn't try to fix it in the PR description.
There was a problem hiding this comment.
If there is a better way to make sure the volunteer is logged in, please let me know.
Also note that this failed regularly in CI but only intermittently when running the test locally. Not sure why.
There was a problem hiding this comment.
Ah, got it. That's one of the reasons I don't particularly like reusing objects with let and prefer to create the test setup for each test using only what they need (for reference: https://thoughtbot.com/blog/lets-not). I think adding this line is a bit of a code smell but I am not a maintainer of the repo.
Alternatives, if I may, feel free to ignore (haven't tried):
- move the before sign in blocks to contexts where the user is admin, and another context where the user is a volunteer
- sign in as user in a before and then create the specific user for each context, for example: https://github.com/hexdevs/casa/blob/main/spec/system/learning_hours/index_spec.rb#L9
There was a problem hiding this comment.
Thanks for the thoughts. I've made an update that doesn't rely on Capybara.reset_sessions!. It's a slightly different approach.
…min sign-in Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
What github issue is this PR for, if any?
Resolves #6500
What changed, and why?
Implements the ellipsis (⋮) action menu for each row in the new case contacts table at
/case_contacts/new_design. The menu contains three items: Edit, Delete, and Set Reminder (or Resolve Reminder if a pending followup already exists).app/datatables/case_contact_datatable.rbcurrent_userparameter to the initializer so the datatable can compute per-row permissionscan_edit,can_destroy,edit_path, andfollowup_idcan_editandcan_destroyare derived fromCaseContactPolicyso the UI reflects actual Pundit permissionsfollowup_idis the id of any open (requested) followup, used to toggle between Set Reminder and Resolve Reminderincludesto add:followupsand:creatorto avoid N+1 queriesapp/controllers/case_contacts/case_contacts_new_design_controller.rbcurrent_usertoCaseContactDatatable.newapp/controllers/case_contacts_controller.rbrespond_totodestroyso jQuery AJAX receives204 No Contentinstead of an HTML redirect that would be silently followedapp/controllers/case_contacts/followups_controller.rbrespond_totoresolvefor the same reasonapp/javascript/src/dashboard.jsedit_pathwithdata-turbo="false"; rendered as a disabled<span>whencan_editis falseDELETErequest via AJAX; rendered as a disabled<button>whencan_destroyis falsePATCHto the followup resolve endpoint; shown when an open followup exists<meta>tagaria-label,aria-haspopup, andaria-expandedattributes; the icon is markedaria-hidden="true"How is this tested? (please write rspec and jest tests!) 💖💪
Note: if you see a flake in your test build in github actions, please post in slack #casa "Flaky test: " :) 💪
Note: We love capybara tests! If you are writing both haml/js and ruby, please try to test your work with tests at every level including system tests like https://github.com/rubyforgood/casa/tree/main/spec/system
Datatable spec — updated
spec/datatables/case_contact_datatable_spec.rb:can_editis"true"for admin,"false"for a different volunteer's contactcan_destroyreflects policy (admin can destroy active contacts; volunteers cannot)edit_pathis the correct Rails edit path for the contactfollowup_idis empty when no requested followup existsfollowup_idcontains the followup id when a requested followup existsRequest specs — updated
spec/requests/case_contacts/case_contacts_new_design_spec.rb:can_edit,can_destroy,edit_path, andfollowup_idfor each rowcan_edit: "true"andcan_destroy: "true"can_edit: "true"for their own contact andcan_destroy: "false"for active contactscan_destroy: "true"for their own draft contactsJest — updated
app/javascript/__tests__/dashboard.test.js:Ellipsis column rendering:
aria-labelcontaining the contact datearia-hidden="true"can_editis"true"; rendered as a disabled<span>when"false"can_destroyis"true"; rendered as a disabled<button>witharia-disabled="true"when"false"followup_idis emptydata-followup-id) whenfollowup_idis presentClick handlers:
DELETEwith CSRF header when confirmed; does not send request when cancelled; reloads DataTable on success{ note }when confirmed with a note; does not POST when cancelled; reloads DataTable on successPATCHto/followups/:id/resolvewith CSRF header; reloads DataTable on successSystem specs — updated
spec/system/case_contacts/case_contacts_new_design_spec.rb:Action menu visibility:
Edit action:
Delete action:
Set Reminder action:
Resolve Reminder action:
Permission states:
Screen recordings
Edit case contact
edit.contact.mov
Delete case contact
delete.case.contact.mov
Delete disabled for volunteer
A volunteer can only delete a draft case contact. This screen recording shows the Delete disabled for non-drafts and the Delete working for drafts.
volunteer.can.only.delete.drafts.mov
Set reminder
set.reminder.mov
Resolve reminder
resolve.reminder.mov
AI Disclosure
This PR was implemented with the assistance of Claude Code (Anthropic, claude-sonnet-4-6). The AI was used to:
All generated code was reviewed and committed by the author.
Action menu clipped near bottom of table
When a row is near the bottom of the table, the dropdown action menu may be partially obscured by the table footer or page content below it.
The root cause seems to be
overflow-x: hiddenon the<body>element, which clipsposition: absoluteelements (including Bootstrap dropdowns) that extend beyond the body's paint area. I'm worried that fixing this globally risks unintended layout side effects elsewhere, so it is left as is.Menu is hidden by table footer
menu.hidden.by.footer.mov