Support SharingUpdated web socket notification on patient flow page
#1351
Conversation
WalkthroughThe changes in this pull request modify the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| @@ -58,7 +58,7 @@ function getSharingOpts(sharing) { | |||
| } | |||
|
|
|||
| const FormSharingView = View.extend({ | |||
| className: 'sidebar__dialog', | |||
| className: 'sidebar__dialog u-margin--t-24', | |||
There was a problem hiding this comment.
So that the element has the correct top margin when being shown/hidden on the fly via a web socket notification.
RoundingWell Care Ops Frontend
|
||||||||||||||||||||||||||||
| Project |
RoundingWell Care Ops Frontend
|
| Branch Review |
ws-sharing
|
| Run status |
|
| Run duration | 02m 55s |
| Commit |
|
| Committer | Nick Major |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
1
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
312
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/js/views/patients/sidebar/action/action-sidebar-forms_views.js (1)
105-107: LGTM: Improved margin handlingGood refactor moving the margin responsibility to the child view (FormSharingView) instead of handling it conditionally in the parent template. This makes the spacing logic more maintainable and centralized.
Consider documenting this layout dependency in a comment to make it clear that FormSharingView handles its own spacing:
<div class="flex"> + {/* Spacing handled by FormSharingView's margin classes */} <div class="flex-grow" data-form-sharing-region></div> </div>src/js/entities-service/entities/actions.js (1)
42-44: Consider adding JSDoc comments for the new message handler.To improve maintainability and make the websocket notification handling more discoverable, consider adding documentation that describes:
- The purpose of the
SharingUpdatedmessage- The expected format of the message payload
- Example usage in the context of websocket notifications
Here's a suggested documentation format:
+ /** + * Handles the 'sharing:updated' websocket notification + * @param {Object} options + * @param {string} options.sharing - The new sharing status + * @param {string} options.outreach - The new outreach status + * @param {Object} [options.attributes] - Additional attributes to update + */ SharingUpdated({ sharing, outreach, attributes = {} }) { this.set({ sharing, outreach, ...attributes }); },test/integration/patients/patient/patient-flow.js (2)
250-290: Consider adding negative test cases for sharing statesWhile the current test cases cover the positive flow of sharing state transitions (disabled → waiting → responded), consider adding test cases for:
- Invalid state transitions
- Error scenarios
- Edge cases like cancellation of sharing
2453-2474: Enhance icon verification in sharing testsThe test verifies icon changes from file-lines to share-from-square, but consider:
- Adding assertions for icon accessibility attributes
- Verifying icon color/styling classes
- Testing icon behavior when sharing is disabled
Example enhancement:
cy .get('.patient-flow__list') .find('.table-list__item .patient__action-icon') - .find('.fa-share-from-square'); + .find('.fa-share-from-square') + .should('have.attr', 'aria-label', 'Sharing Enabled') + .should('have.class', 'is-success');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
src/js/entities-service/entities/actions.js(1 hunks)src/js/views/patients/sidebar/action/action-sidebar-forms_views.js(2 hunks)test/integration/patients/patient/patient-flow.js(6 hunks)
🔇 Additional comments (7)
src/js/views/patients/sidebar/action/action-sidebar-forms_views.js (1)
61-61: LGTM: Consistent class naming convention
The addition of the top margin utility class follows the project's naming conventions and improves the visual spacing.
src/js/entities-service/entities/actions.js (2)
42-44: Verify permission handling for sharing updates.
While the implementation looks correct, it's important to ensure that sharing updates respect the model's permission system.
Let's verify the permission checks:
#!/bin/bash
# Description: Verify permission handling for sharing updates
# Check for permission checks in sharing-related code
rg -B 5 -A 5 "sharing.*canEdit|canEdit.*sharing" --type js
# Look for websocket authorization checks
ast-grep --pattern 'socket.on("sharing:updated", function($params) {
$$$
can($$$)
$$$
})'42-44: LGTM! The message handler follows established patterns.
The new SharingUpdated message handler is well-implemented and consistent with other handlers in the class. It properly updates both sharing and outreach attributes while supporting additional attributes through the spread operator.
Let's verify the integration points with this handler:
✅ Verification successful
The SharingUpdated message handler is correctly implemented and well-integrated
The verification confirms that the handler is properly integrated with the codebase:
- The
hasSharing()andhasOutreach()methods are used extensively across multiple views for UI rendering ACTION_SHARINGandACTION_OUTREACHconstants are properly imported and used throughout the codebase- The sharing/outreach states are used in various contexts including form sharing, UI icons, and state management
- Integration tests verify the correct handling of sharing states ('pending', 'canceled', etc.)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the integration of sharing and outreach functionality
# Check for consistent usage of sharing/outreach constants
rg "ACTION_SHARING\." --type js
rg "ACTION_OUTREACH\." --type js
# Check for related websocket handling
ast-grep --pattern 'socket.on("sharing:updated", $_)'
# Verify the model's sharing-related method implementations
ast-grep --pattern 'hasOutreach() {
$$$
}'
ast-grep --pattern 'hasSharing() {
$$$
}'
Length of output: 364
Script:
#!/bin/bash
# Let's try a different approach to verify the integration
# Check for sharing/outreach related methods and their usage
rg "hasSharing|hasOutreach" --type js -A 2 -B 2
# Look for sharing/outreach attribute usage
rg "\.sharing|\.outreach" --type js -A 2 -B 2
# Check for websocket event handlers related to sharing
rg "sharing:updated" --type js -A 2 -B 2
# Look for imports of ACTION_SHARING and ACTION_OUTREACH
rg "import.*ACTION_(SHARING|OUTREACH)" --type js
Length of output: 13416
test/integration/patients/patient/patient-flow.js (4)
108-116: LGTM: Workspace relationship addition
The addition of workspace relationships to the test patient data is well-structured and aligns with the PR objectives for supporting sharing functionality.
122-123: LGTM: Form and sharing attributes
The addition of outreach and sharing attributes set to 'disabled' along with the form relationship provides good test coverage for the initial state of actions.
Also applies to: 129-130
155-159: LGTM: Patient routing setup
The patient routing setup is properly implemented with appropriate test data handling.
2347-2348: LGTM: Socket test initial state
The socket test setup correctly initializes the action with disabled sharing state and form relationship.
Also applies to: 2354-2354
Pull Request Test Coverage Report for Build d558e510-661e-4a3b-8848-8ccb361d24ceDetails
💛 - Coveralls |
There was a problem hiding this comment.
I think this is a simple implementation, unless I'm missing something.
In the action sidebar tests, I did:
- Start out action as having
sharing: 'disabled'. i.e. sharing section is hidden in UI. - Socket notification comes in that sets
sharing: 'enabled'. Verify UI elements are now shown. - Socket notification comes in tha sets
sharing: 'responded'. Verify UI elements are updated.
In the action list, I did:
- Start out action as having
outreach: 'disabled'. i.e. icon for the action list item is the default version. - Socket notification comes in that sets
outreach: 'patient'. Verify icon for action list item changes. (I'm not sure there's anything else that changes based on thatoutreachattribute).
There was a problem hiding this comment.
The notification from the BE contains data for the recipient: https://github.com/RoundingWell/care-ops-backend/blob/develop/tests/Notifications/Data/Messages/sharing-updated-message-structure.json.
Can we assume the recipient will always be the patient the action/flow belongs to? And therefore we don't need to do anything on the FE with that recipient data?
There was a problem hiding this comment.
I don't think FE needs to know the recipient. FE needs to know all of the data such that if the user hard refreshed they'd see the same thing.
| @@ -39,6 +39,9 @@ const _Model = BaseModel.extend({ | |||
| DetailsChanged({ details, attributes = {} }) { | |||
| this.set({ details, ...attributes }); | |||
| }, | |||
| SharingUpdated({ sharing, outreach, attributes = {} }) { | |||
There was a problem hiding this comment.
so a bunch of these are wrong.. the only thing doing any work here is attributes
There was a problem hiding this comment.
These have been updated 👍
| @@ -102,7 +102,7 @@ const FormLayoutView = View.extend({ | |||
| {{#if hasForm}}<h4 class="sidebar__label u-margin--t-8">{{ @intl.patients.sidebar.action.actionSidebarFormsViews.formLayoutView.formLabel }}</h4>{{/if}} | |||
| <div class="flex-grow" data-form-region></div> | |||
| </div> | |||
| <div class="flex{{#if hasSharing}} u-margin--t-24{{/if}}"> | |||
| <div class="flex"> | |||
There was a problem hiding this comment.
is the templateContext still in use?
There was a problem hiding this comment.
The hasSharing value can be removed. But hasForm is still in use in that templateContext().
There was a problem hiding this comment.
I don't think FE needs to know the recipient. FE needs to know all of the data such that if the user hard refreshed they'd see the same thing.
And update the tests to replicate what actual data structure will look like in prod
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
test/integration/patients/patient/patient-flow.js (1)
2473-2496: Consider adding comments to explain icon meaningsWhile the test correctly verifies icon changes from
fa-file-linestofa-share-from-square, it would be helpful to add comments explaining what each icon represents in terms of the sharing state.cy .get('.patient-flow__list') .find('.table-list__item .patient__action-icon') + // Verify default form icon when sharing is disabled .find('.fa-file-lines'); cy.sendWs({ category: 'SharingUpdated', resource: { type: 'patient-actions', id: testSocketAction.id, }, payload: { attributes: { sharing: 'pending', outreach: 'patient', }, }, }); cy .get('.patient-flow__list') .find('.table-list__item .patient__action-icon') + // Verify sharing icon when sharing is pending .find('.fa-share-from-square');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/js/entities-service/entities/actions.js(1 hunks)src/js/entities-service/entities/flows.js(1 hunks)src/js/views/patients/sidebar/action/action-sidebar-forms_views.js(2 hunks)test/integration/patients/patient/patient-flow.js(13 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/js/entities-service/entities/actions.js
- src/js/views/patients/sidebar/action/action-sidebar-forms_views.js
🔇 Additional comments (5)
src/js/entities-service/entities/flows.js (2)
25-26: LGTM! Clean refactoring of the NameChanged handler.
The simplified message handler that only accepts attributes improves code maintainability and follows a consistent pattern across the codebase.
28-29: LGTM! Clean refactoring of the DetailsChanged handler.
The simplified message handler that only accepts attributes improves code maintainability and follows a consistent pattern across the codebase.
test/integration/patients/patient/patient-flow.js (3)
108-116: LGTM: Good addition of workspace relationship
The addition of the workspace relationship to the test patient makes the test data more realistic and complete.
122-123: LGTM: Proper initialization of sharing-related attributes
The addition of outreach and sharing attributes with 'disabled' default state, along with the form relationship, provides the necessary setup for testing sharing functionality.
Also applies to: 129-130, 2359-2360, 2366-2366
259-302: LGTM: Comprehensive sharing state UI tests
The test cases effectively verify the sharing functionality through different states:
- Initial empty state
- Enabled state with "Waiting for Response"
- Responded state with "Response Saved"
The assertions properly verify the UI elements for each state.
| }, | ||
| NameChanged({ name, attributes = {} }) { | ||
| this.set({ name, ...attributes }); | ||
| NameChanged({ attributes }) { | ||
| this.set(attributes); | ||
| }, | ||
| DetailsChanged({ details, attributes = {} }) { | ||
| this.set({ details, ...attributes }); | ||
| DetailsChanged({ attributes }) { | ||
| this.set(attributes); | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
Implement SharingUpdated Message Handler
The SharingUpdated handler is not implemented in the messages object. Please add the SharingUpdated handler to support websocket notifications.
🔗 Analysis chain
Missing SharingUpdated message handler implementation.
The PR objectives mention supporting SharingUpdated websocket notifications, but I don't see the handler implemented in the messages object. Please add the SharingUpdated handler to support this functionality.
Let's verify if this handler exists elsewhere:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any existing SharingUpdated handler implementations
rg -l "SharingUpdated"
# Search for websocket notification handlers pattern
ast-grep --pattern 'messages: {
$$$
SharingUpdated($$$) {
$$$
}
$$$
}'
Length of output: 323
Shortcut Story ID: [sc-56333]
Summary by CodeRabbit
New Features
Improvements
attributesparameter, simplifying state updates.Tests