New Case Contacts Table: expandable case contact rows#6823
New Case Contacts Table: expandable case contact rows#6823cliftonmcintosh wants to merge 7 commits intorubyforgood:mainfrom
Conversation
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>
|
Please fix lint errors :) |
There was a problem hiding this comment.
Pull request overview
Adds expand/collapse behavior to the new Case Contacts DataTable so users can view per-row topic answers and notes inline, while also improving how topics are rendered in the Topics column.
Changes:
- Extend the case contacts datatable JSON payload to include
contact_topic_answersandnotes, and preload associations to avoid N+1 queries. - Add DataTables child-row expand/collapse behavior in
dashboard.js, plus topic “pill” rendering with overflow handling. - Add SCSS for chevron rotation and expanded-row content styling; add request/system/Jest coverage for the new behavior.
Reviewed changes
Copilot reviewed 6 out of 6 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 | System coverage for expanding/collapsing and displaying answers/notes |
| spec/requests/case_contacts/case_contacts_new_design_spec.rb | Request coverage for new JSON fields (contact_topic_answers, notes, contact_topics array) |
| app/javascript/src/dashboard.js | UI behavior for toggling child rows and rendering topic pills |
| app/javascript/tests/dashboard.test.js | Jest updates for chevron markup + topic pill rendering rules |
| app/datatables/case_contact_datatable.rb | Add expanded-content fields to JSON + includes for preloading |
| app/assets/stylesheets/pages/case_contacts.scss | Styling for chevron rotation + expanded content layout |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
app/javascript/src/dashboard.js
Outdated
| orderable: false, | ||
| searchable: false, | ||
| render: () => '<i class="fa-solid fa-chevron-down"></i>' | ||
| render: () => '<i class="fa-solid fa-chevron-down expand-toggle" style="cursor: pointer;"></i>' |
There was a problem hiding this comment.
The chevron toggle is rendered as a plain and only responds to mouse clicks. This is not keyboard-accessible and doesn’t expose state to assistive tech. Consider rendering a (or adding role="button" + tabindex="0") and toggling aria-expanded (and handling Enter/Space) when the row is expanded/collapsed.
| render: () => '<i class="fa-solid fa-chevron-down expand-toggle" style="cursor: pointer;"></i>' | |
| render: () => '<button type="button" class="expand-toggle" aria-expanded="false" aria-label="Expand row details" style="cursor: pointer; background: none; border: 0; padding: 0;"><i class="fa-solid fa-chevron-down" aria-hidden="true"></i></button>' |
app/javascript/src/dashboard.js
Outdated
| orderable: false, | ||
| searchable: false, | ||
| render: () => '<i class="fa-solid fa-chevron-down"></i>' | ||
| render: () => '<i class="fa-solid fa-chevron-down expand-toggle" style="cursor: pointer;"></i>' |
There was a problem hiding this comment.
The chevron icon includes an inline cursor style even though .expand-toggle has a dedicated stylesheet block. Moving cursor styling to CSS (e.g., .expand-toggle { cursor: pointer; }) will keep presentation concerns out of JS and make future test expectations less brittle.
| render: () => '<i class="fa-solid fa-chevron-down expand-toggle" style="cursor: pointer;"></i>' | |
| render: () => '<i class="fa-solid fa-chevron-down expand-toggle"></i>' |
| $('table#case_contacts tbody').on('click', '.expand-toggle', function () { | ||
| const tr = $(this).closest('tr') | ||
| const row = table.row(tr) | ||
|
|
||
| if (row.child.isShown()) { | ||
| row.child.hide() | ||
| $(this).removeClass('expanded') | ||
| } else { | ||
| row.child(buildExpandedContent(row.data())).show() | ||
| $(this).addClass('expanded') | ||
| } |
There was a problem hiding this comment.
This click handler toggles the expanded visual state but doesn’t update any accessibility attributes (e.g., aria-expanded) or support keyboard activation. If the toggle becomes a button (or role=button), update the handler to keep aria-expanded in sync and respond to Enter/Space.
| .reject { |a| a.value.blank? } | ||
| .map { |a| {question: a.contact_topic&.question, value: a.value} }, |
There was a problem hiding this comment.
contact_topic_answers uses safe navigation for a.contact_topic, which can result in { question: nil/blank } entries being returned. That will render an empty heading in the expanded row. Consider filtering out answers with missing/blank questions (or providing a fallback label) so the expanded UI always has a meaningful topic heading.
| .reject { |a| a.value.blank? } | |
| .map { |a| {question: a.contact_topic&.question, value: a.value} }, | |
| .map { |a| {question: a.contact_topic&.question, value: a.value} } | |
| .reject { |a| a[:value].blank? || a[:question].blank? }, |
There was a problem hiding this comment.
I investigated this and, from what I can tell, the scenario where question could be blank is unreachable in practice, for two reasons:
-
There is a database-level foreign key constraint on
contact_topic_answers.contact_topic_id— deleting aContactTopicwhile answers still reference it raises aPG::ForeignKeyViolation. I confirmed this by attempting to write a spec that creates an answer, then destroys its topic, and the spec failed with that error. -
The
ContactTopicAnswermodel validates thatcontact_topicis present whenevervalueis present. Combined with our existing.reject { |a| a.value.blank? }, every answer that reaches the.mapis guaranteed to have a non-nilcontact_topic.
The &. safe navigation operator on a.contact_topic&.question is good defensive code and I've left it in place, but adding a second .reject to guard against a state the database constraints prevent would add noise without real protection. I've left the code as-is.
@compwron thanks for the review. It looks to me like these are test errors in code unrelated to the changes in this PR. Please let me know if you see anything here that you think might have been impacted by the changes in this PR. Errors from npm lint npm run test
shell: /usr/bin/bash -e {0}
> casa@0.1.0 test
> jest
PASS app/javascript/__tests__/dashboard.test.js
PASS app/javascript/__tests__/type_checker.test.js
FAIL app/javascript/__tests__/validated_form.test.js
● NonDrivingContactMediumWarning › warningHighlightUI › when passed a truthy value, it makes the parent container for the checkboxes yellow and draws a yellow border around the miles driven input
expect(received).toBe(expected) // Object.is equality
Expected: "2px solid #ffc107"
Received: "2px solid rgb(255, 193, 7)"
152 |
153 | expect(checkboxContainer.css('background-color')).toBe('rgb(255, 248, 225)')
> 154 | expect(milesDrivenInput.css('border')).toBe('2px solid #ffc107')
| ^
155 | done()
156 | } catch (error) {
157 | done(error)
at Document.toBe (app/javascript/__tests__/validated_form.test.js:154:50)
at mightThrow (node_modules/jquery/dist/jquery.js:3489:29)
at process (node_modules/jquery/dist/jquery.js:3557:12)
at Timeout.task [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:579:19)
● NonDrivingContactMediumWarning › warningHighlightUI › when passed a falsy value, it removes the error highlighting
expect(received).toBe(expected) // Object.is equality
Expected: "2px solid #ffc107"
Received: "2px solid rgb(255, 193, 7)"
169 |
170 | expect(checkboxContainer.css('background-color')).toBe('rgb(255, 248, 225)')
> 171 | expect(milesDrivenInput.css('border')).toBe('2px solid #ffc107')
| ^
172 |
173 | component.warningHighlightUI()
174 |
at Document.toBe (app/javascript/__tests__/validated_form.test.js:171:50)
at mightThrow (node_modules/jquery/dist/jquery.js:3489:29)
at process (node_modules/jquery/dist/jquery.js:3557:12)
at Timeout.task [as _onTimeout] (node_modules/jsdom/lib/jsdom/browser/Window.js:579:19)
PASS app/javascript/__tests__/add_to_calendar_button.test.js
PASS app/javascript/__tests__/read_more.test.js
PASS app/javascript/__tests__/case_emancipations.test.js
PASS app/javascript/__tests__/require_communication_preference.test.js
PASS app/javascript/__tests__/password_confirmation.test.js
PASS app/javascript/__tests__/case_button_states.test.js
PASS app/javascript/__tests__/case_contact.test.js
FAIL app/javascript/__tests__/two_minute_warning_session_timeout.test.js
● session_timeout_poller › timer runs every second
TypeError: Cannot redefine property: location
at Object.defineProperty (<anonymous>)
14 | mockReload = jest.fn()
15 | global.alert = mockAlert
> 16 | Object.defineProperty(window, 'location', {
| ^
17 | writable: true,
18 | value: { reload: mockReload }
19 | })
at Object.defineProperty (app/javascript/__tests__/two_minute_warning_session_timeout.test.js:16:12)
PASS app/javascript/__tests__/time_zone.test.js
PASS app/javascript/__tests__/casa_case.test.js
PASS app/javascript/__tests__/notifier.test.js (7.43 s)
Test Suites: 2 failed, 12 passed, 14 total
Tests: 3 failed, 175 passed, 178 total
Snapshots: 0 total
Time: 8.067 s
Ran all test suites.
Error: Process completed with exit code 1.My assessment of the Jest failures in CIWith the assistance of Claude Code, from what I can tell, neither of the jest failures is related to the changes in this PR. Both are pre-existing test compatibility issues with newer versions of Node and jsdom. The production code in both cases appears to be correct.
The tests assert
The test uses I've created an issue and opened a PR for fixing this. I am happy to cherry-pick those commits into this PR if that is preferred. Please let me know. |
…panded Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
While running the app locally I noticed two N+1 warnings in the Rails log. I think both of them predate the changes in this PR, but I want to flag them for visibility. expand for log snippet18:09:23 web.1 | N+1 queries detected:
18:09:23 web.1 | SELECT "casa_cases"."id", "casa_cases"."case_number", "casa_cases"."created_at", "casa_cases"."updated_at", "casa_cases"."casa_org_id", "casa_cases"."birth_month_year_youth", "casa_cases"."active", "casa_cases"."court_report_submitted_at", "casa_cases"."court_report_status", "casa_cases"."slug", "casa_cases"."date_in_care" FROM "casa_cases" WHERE "casa_cases"."id" = $1 LIMIT $2
18:09:23 web.1 | SELECT "casa_cases"."id", "casa_cases"."case_number", "casa_cases"."created_at", "casa_cases"."updated_at", "casa_cases"."casa_org_id", "casa_cases"."birth_month_year_youth", "casa_cases"."active", "casa_cases"."court_report_submitted_at", "casa_cases"."court_report_status", "casa_cases"."slug", "casa_cases"."date_in_care" FROM "casa_cases" WHERE "casa_cases"."id" = $1 LIMIT $2
18:09:23 web.1 | SELECT "casa_cases"."id", "casa_cases"."case_number", "casa_cases"."created_at", "casa_cases"."updated_at", "casa_cases"."casa_org_id", "casa_cases"."birth_month_year_youth", "casa_cases"."active", "casa_cases"."court_report_submitted_at", "casa_cases"."court_report_status", "casa_cases"."slug", "casa_cases"."date_in_care" FROM "casa_cases" WHERE "casa_cases"."id" = $1 LIMIT $2
18:09:23 web.1 | SELECT "casa_cases"."id", "casa_cases"."case_number", "casa_cases"."created_at", "casa_cases"."updated_at", "casa_cases"."casa_org_id", "casa_cases"."birth_month_year_youth", "casa_cases"."active", "casa_cases"."court_report_submitted_at", "casa_cases"."court_report_status", "casa_cases"."slug", "casa_cases"."date_in_care" FROM "casa_cases" WHERE "casa_cases"."id" = $1 LIMIT $2
18:09:23 web.1 | SELECT "casa_cases"."id", "casa_cases"."case_number", "casa_cases"."created_at", "casa_cases"."updated_at", "casa_cases"."casa_org_id", "casa_cases"."birth_month_year_youth", "casa_cases"."active", "casa_cases"."court_report_submitted_at", "casa_cases"."court_report_status", "casa_cases"."slug", "casa_cases"."date_in_care" FROM "casa_cases" WHERE "casa_cases"."id" = $1 LIMIT $2
18:09:23 web.1 | SELECT "casa_cases"."id", "casa_cases"."case_number", "casa_cases"."created_at", "casa_cases"."updated_at", "casa_cases"."casa_org_id", "casa_cases"."birth_month_year_youth", "casa_cases"."active", "casa_cases"."court_report_submitted_at", "casa_cases"."court_report_status", "casa_cases"."slug", "casa_cases"."date_in_care" FROM "casa_cases" WHERE "casa_cases"."id" = $1 LIMIT $2
18:09:23 web.1 | SELECT "casa_cases"."id", "casa_cases"."case_number", "casa_cases"."created_at", "casa_cases"."updated_at", "casa_cases"."casa_org_id", "casa_cases"."birth_month_year_youth", "casa_cases"."active", "casa_cases"."court_report_submitted_at", "casa_cases"."court_report_status", "casa_cases"."slug", "casa_cases"."date_in_care" FROM "casa_cases" WHERE "casa_cases"."id" = $1 LIMIT $2
18:09:23 web.1 | Call stack:
18:09:23 web.1 | app/datatables/case_contact_datatable.rb:20:in 'block in CaseContactDatatable#data'
18:09:23 web.1 | app/datatables/case_contact_datatable.rb:14:in 'Enumerable#map'
18:09:23 web.1 | app/datatables/case_contact_datatable.rb:14:in 'CaseContactDatatable#data'
18:09:23 web.1 | app/datatables/application_datatable.rb:15:in 'ApplicationDatatable#as_json'
18:09:23 web.1 | app/controllers/case_contacts/case_contacts_new_design_controller.rb:15:in 'CaseContacts::CaseContactsNewDesignController#datatable'
18:09:23 web.1 |
18:09:23 web.1 | N+1 queries detected:
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | SELECT 1 AS one FROM "followups" WHERE "followups"."case_contact_id" = $1 AND "followups"."status" = $2 LIMIT $3
18:09:23 web.1 | Call stack:
18:09:23 web.1 | app/datatables/case_contact_datatable.rb:38:in 'block in CaseContactDatatable#data'
18:09:23 web.1 | app/datatables/case_contact_datatable.rb:14:in 'Enumerable#map'
18:09:23 web.1 | app/datatables/case_contact_datatable.rb:14:in 'CaseContactDatatable#data'
18:09:23 web.1 | app/datatables/application_datatable.rb:15:in 'ApplicationDatatable#as_json'
18:09:23 web.1 | app/controllers/case_contacts/case_contacts_new_design_controller.rb:15:in 'CaseContacts::CaseContactsNewDesignController#datatable'N+1 #1
Fix: add N+1 #2
Fix: replace with I am happy to fix both in this PR if that is preferred, or leave them for a separate issue. |
What github issue is this PR for, if any?
Resolves #6498
What changed, and why?
Implements expand/collapse functionality for rows in the new case contacts table.
app/datatables/case_contact_datatable.rbcontact_topic_answersandnotesto the JSON response so the browser has the data it needs to render expanded contentcontact_topic_answers: :contact_topicto theincludesinraw_recordsto avoid N+1 queriesapp/javascript/src/dashboard.jsexpand-toggleCSS class and a pointer cursorrow.child()) to show/hide expanded content on chevron clickbuildExpandedContent()to build the expanded row HTML from the row's JSON data, matching the structure of the Figma design (topic heading followed by answer paragraph)app/assets/stylesheets/pages/case_contacts.scssexpand-togglestyles with a 0.3s CSS rotation animation for the chevron (matching the existing sidebar pattern)expanded-contentandexpanded-topicstyles to match the Figma layoutThe DataTables Child Rows API is used rather than Bootstrap collapse or manual DOM insertion because DataTables replaces the entire
<tbody>via AJAX on initialization. When this happens, any server-rendered or manually inserted rows are destroyed.row.child()inserts rows that DataTables manages itself, so they persist correctly.Topics column pill styling. While working on this issue I noticed the Topics column was rendering as a raw pipe-delimited string. I did not see an issue that covers the Topics column, so I updated the render for each topic to be a
badge-pillchip (matching the existing Draft badge style) with a+N Moreoverflow indicator when there are more than two topics. I am happy to pull this into a separate issue and PR if that is preferred.How is this tested?
Request specs — added to
spec/requests/case_contacts/case_contacts_new_design_spec.rb:contact_topic_answersis present and is an array in the datatable JSON responsenotesis included in the responsecontact_topicsis returned as an array of strings (not a pipe-delimited string)System specs — added
spec/system/case_contacts/case_contacts_new_design_spec.rb:Jest — updated
app/javascript/__tests__/dashboard.test.js:expand-toggleclassbadge-pillchip+N Morebadge for overflow topicsScreenshots
Screen recording updated after commit fb31759.
Screen recording of collapsible sections
Screen.Recording.2026-04-05.at.07.40.38.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.