Skip to content

Conversation

@nmajor25
Copy link
Contributor

@nmajor25 nmajor25 commented Oct 22, 2025

Shortcut Story ID: [sc-64560]

Summary by CodeRabbit

  • Bug Fixes

    • Improved robustness of patient bulk-edit when state information is missing or structured differently, reducing errors when changing states in bulk.
  • Tests

    • Strengthened end-to-end checks for bulk-edit flows to ensure state, owner, due date/time, and duration display expected values during the workflow.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a bug where the bulk edit state button displays a blank label when all selected items share the same state. The issue was caused by passing stateId directly from the model instead of extracting the id property from the nested state object.

Key Changes:

  • Added get utility import from underscore
  • Updated state ID extraction to use get(this.model.get('state'), 'id') instead of this.model.get('stateId')

@coderabbitai
Copy link

coderabbitai bot commented Oct 22, 2025

Walkthrough

Replaced direct stateId property access with a safe getter reading the id from a nested state object in two bulk-edit view components; added test assertions in Cypress integration tests to verify initial and modal UI values for bulk-edit flows and multiple-item edits.

Changes

Cohort / File(s) Change Summary
Bulk-edit views
src/js/views/patients/shared/bulk-edit/bulk-edit_views.js
Use get(this.model.get('state'), 'id') (underscore get imported) in BulkEditActionsBodyView and BulkEditFlowsBodyView to derive stateId from a nested state object instead of direct property access.
Integration tests (Cypress)
test/integration/patients/worklist/bulk-edit.js
Added runtime assertions validating modal UI fields (state, owner, due date/time, duration) for flow and multi-item bulk-edit flows before and after step transitions.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

  • Bulk edit request batching #1538 — Touches the same bulk-edit view components (BulkEditActionsBodyView, BulkEditFlowsBodyView) and may overlap with state and UI handling changes.

Suggested reviewers

  • paulfalgout

Poem

🐰 I hop through code, a careful little critter,
I fetch the id where nested values flitter,
With gentle getters and tests that peek,
The bulk-edit modal now plays hide-and-seek,
Hooray — no surprises, just a safer glitter! ✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The PR title "Fix bug: bulk edit states button label is blank when all selected states are the same" is specific and directly relates to the changeset. The modifications in bulk-edit_views.js involve replacing direct stateId access with a safer getter pattern (using underscore's get method on nested state objects), which would address rendering issues like blank labels caused by missing or improperly structured state data. The test additions further validate that state values are correctly rendered in the bulk edit UI. The title is clear and sufficiently specific to communicate the main bug being fixed without being vague or overly broad.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch bulk-state-btn

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 3ba3924 and 80db12c.

📒 Files selected for processing (2)
  • src/js/views/patients/shared/bulk-edit/bulk-edit_views.js (3 hunks)
  • test/integration/patients/worklist/bulk-edit.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/js/views/patients/shared/bulk-edit/bulk-edit_views.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.js

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.js: Filenames stay kebab-case; exported classes/views use PascalCase
Import order: 1) polyfills/3rd-party, 2) SCSS/CSS modules, 3) shared utilities/base, 4) entities/service modules, 5) apps/controllers, 6) views/components, 7) templates (*.hbs) and final SCSS overrides
Use dayjs for dates/times instead of handcrafted formatting
Keep modules under ~300 lines; extract helpers and prefer guard clauses for clarity
Route network requests through the entities service and reuse existing utilities before adding dependencies
Keep feature flags easy to remove; isolate conditional logic and avoid leaking flag checks

Files:

  • test/integration/patients/worklist/bulk-edit.js
**/*.{hbs,js}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/*.{hbs,js}: Follow FormatJS key schema: view.directory.fileViews.viewName.stringContext (suffix HTML for HTML strings)
Access i18n strings via @intl helpers in templates and intl utilities in JS

Files:

  • test/integration/patients/worklist/bulk-edit.js
🧬 Code graph analysis (1)
test/integration/patients/worklist/bulk-edit.js (1)
test/integration/patients/patient/patient-flow.js (1)
  • tomorrow (24-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: test-build
🔇 Additional comments (2)
test/integration/patients/worklist/bulk-edit.js (2)

519-527: Good addition: Tests single-item bulk edit display.

These assertions verify that when a single flow is selected (after a bulk edit), the modal correctly displays the actual state ("Done") and owner ("Nurse") values rather than placeholder text or blank labels. This directly addresses the bug described in the PR and the testing gap mentioned in your PR comment.


894-912: Excellent coverage for single-action bulk edit.

These assertions comprehensively verify that all bulk-editable fields (state, owner, due date, due time, duration) display their actual values when a single action is selected post-bulk-edit. The use of formatDate(tomorrow, 'LONG') at line 904 is consistent with similar assertions elsewhere in the test (line 98). This provides strong coverage for the bug fix scenario across all action fields.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@cypress
Copy link

cypress bot commented Oct 22, 2025

RoundingWell Care Ops Frontend    Run #8229

Run Properties:  status check passed Passed #8229  •  git commit 80db12c6cf: Fix bug: bulk edit states button label shows blank when all selected actions/flo...
Project RoundingWell Care Ops Frontend
Branch Review bulk-state-btn
Run status status check passed Passed #8229
Run duration 02m 08s
Commit git commit 80db12c6cf: Fix bug: bulk edit states button label shows blank when all selected actions/flo...
Committer Nick Major
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 0
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 334
View all changes introduced in this branch ↗︎

@coveralls
Copy link

coveralls commented Oct 22, 2025

Pull Request Test Coverage Report for Build d439b1e2-a32f-4569-b625-092fce74ac6b

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 99.963%

Totals Coverage Status
Change from base Build 6a7c2166-02ed-4d3f-a9e4-883a513c4925: 0.0%
Covered Lines: 6325
Relevant Lines: 6325

💛 - Coveralls

Copy link
Member

@paulfalgout paulfalgout left a comment

Choose a reason for hiding this comment

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

Feels like there should be a test for this or no?

@nmajor25
Copy link
Contributor Author

nmajor25 commented Oct 23, 2025

Wasn't sure about testing these. Only the Multiple Owners/States/Dates/etc. versions of the buttons are tested. Not the versions where the individual state, owner, date, is shown.

So we should probably test all of the other ones too. Not just the state button.

Which could be a pain... but I guess we could just add an extra action/flow to the list in the test, select just that one, and test the buttons.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Easier than I originally thought. To verify the button labels are shown correctly when all states, owners, due dates, etc. are the same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants