Change updated_at attribute when action/flow model is updated via web socket
#1352
Conversation
| @@ -76,6 +76,7 @@ const LayoutView = View.extend({ | |||
| modelEvents: { | |||
| 'change:_state': 'showOwner', | |||
| 'change:_owner': 'showFlow', | |||
| 'change:updated_at': 'showTimestamps', | |||
There was a problem hiding this comment.
This doesn't do anything. But the updated timestamp in the flow sidebar is updating correctly on model change.
So I'm not sure what's going on here yet.
There was a problem hiding this comment.
I'm noticing other weird things with the flow sidebar, in terms of updating on a web socket model change.
Name and owner update correctly. But details and state do not.
I'm going to investigate/address those in this story here.
There was a problem hiding this comment.
For the first comment above, the change:updated_at model event is indeed working. I just had the test setup incorrectly. The flow sidebar needs to be open before the web socket notification comes in. I updated the test to do that.
I still plan on investigating the second comment in a separate story.
src/js/base/model.js
Outdated
| payload.attributes.updated_at = dayjs.utc().format(); | ||
| } else { | ||
| payload.attributes = { updated_at: dayjs.utc().format() }; | ||
| } |
There was a problem hiding this comment.
Probably a better way to do this. But just attaching attributes.updated_at to the payload data before its passed to the individual entity services.
There was a problem hiding this comment.
payload.attributes = extend(payload.attributes, { updated_at: day...
There was a problem hiding this comment.
That would be much cleaner 👍
WalkthroughThe pull request introduces enhancements to the Backbone model and related views. A new timestamp property Changes
Possibly related PRs
Suggested reviewers
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 (
|
RoundingWell Care Ops Frontend
|
||||||||||||||||||||||||||||
| Project |
RoundingWell Care Ops Frontend
|
| Branch Review |
ws-updated-at
|
| Run status |
|
| Run duration | 02m 48s |
| 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 ↗︎ | |
Pull Request Test Coverage Report for Build e1cef074-1d57-494e-a1a8-cc157d870451Details
💛 - Coveralls |
src/js/base/model.js
Outdated
| payload.attributes.updated_at = dayjs.utc().format(); | ||
| } else { | ||
| payload.attributes = { updated_at: dayjs.utc().format() }; | ||
| } |
There was a problem hiding this comment.
payload.attributes = extend(payload.attributes, { updated_at: day...
nmajor25
force-pushed
the
ws-updated-at
branch
2 times, most recently
from
1a115de to
5e52dfc
Compare
![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 (8)
src/js/base/model.js (1)
88-89: Consider creating a new payload object for better immutability.The implementation looks good and follows the suggested approach from previous comments. However, consider creating a new payload object instead of modifying the input parameter directly:
- payload.attributes = extend(payload.attributes || {}, { updated_at: dayjs.utc().format() }); + const updatedPayload = { + ...payload, + attributes: extend(payload.attributes || {}, { updated_at: dayjs.utc().format() }) + };This would prevent potential side effects if the payload object is used elsewhere in the codebase.
src/js/views/patients/sidebar/flow/flow-sidebar_views.js (2)
Line range hint
26-33: Add model event bindings for timestamp changesThe
TimestampsViewshould listen for changes to the timestamps to ensure the UI stays in sync with the model. Consider adding:const TimestampsView = View.extend({ className: 'sidebar__footer flex', + modelEvents: { + 'change:updated_at': 'render', + 'change:created_at': 'render' + }, template: hbs` <div class="sidebar__footer-left"><h4 class="sidebar__label">{{ @intl.patients.sidebar.flow.flowSidebarViews.timestampsView.createdAt }}</h4><div>{{formatDateTime created_at "AT_TIME"}}</div></div> <div><h4 class="sidebar__label">{{ @intl.patients.sidebar.flow.flowSidebarViews.timestampsView.updatedAt }}</h4><div>{{formatDateTime updated_at "AT_TIME"}}</div></div> `, });
79-79: Optimize view lifecycle managementWhile the model event binding is correct, creating a new
TimestampsViewinstance on every timestamp update is inefficient. Consider reusing the existing view instance by letting it handle its own updates through model events as suggested in the previous comment.showTimestamps() { - this.showChildView('timestamps', new TimestampsView({ model: this.model })); + if (!this.getChildView('timestamps')) { + this.showChildView('timestamps', new TimestampsView({ model: this.model })); + } }src/js/views/patients/sidebar/action/action-sidebar-activity-views.js (1)
267-269: LGTM! Consider adding error handling for invalid dates.The addition of the
modelEventsproperty to automatically re-render onupdated_atchanges is a clean implementation that follows Backbone's best practices. This ensures the UI stays in sync with the model when updates occur via web socket.Consider adding date validation in the template to handle cases where
updated_atmight be invalid:template: hbs` <div class="sidebar__footer-left"><h4 class="sidebar__label">{{ @intl.patients.sidebar.action.activityViews.createdAt }}</h4><div>{{formatDateTime createdAt "AT_TIME"}}</div></div> - <div><h4 class="sidebar__label">{{ @intl.patients.sidebar.action.activityViews.updatedAt }}</h4><div>{{formatDateTime updated_at "AT_TIME"}}</div></div> + <div><h4 class="sidebar__label">{{ @intl.patients.sidebar.action.activityViews.updatedAt }}</h4><div>{{#if updated_at}}{{formatDateTime updated_at "AT_TIME"}}{{else}}-{{/if}}</div></div> `,test/integration/patients/patient/patient-flow.js (4)
195-201: Use fixed timestamps to prevent flaky testsUsing
dayjs.utc().format()can lead to flaky tests due to timing differences. Consider using a fixed test timestamp to ensure consistent test results.Apply this diff to modify the timestamp to use the test timestamp helper:
- .should('contain', formatDate(dayjs.utc().format(), 'AT_TIME')); + .should('contain', formatDate(testTs(), 'AT_TIME'));
2409-2412: Unnecessary click action on '[data-header-region]'The click action on
[data-header-region]may be unnecessary if it does not contribute to the test's purpose. Removing it can improve test performance.Apply this diff to remove the unnecessary click:
- cy - .get('[data-header-region]') - .click('top');
2431-2437: Use fixed timestamps to prevent flaky testsSimilar to the previous comment, using
dayjs.utc().format()can cause intermittent test failures. Consider using a fixed test timestamp.Apply this diff:
- .should('contain', formatDate(dayjs.utc().format(), 'AT_TIME')); + .should('contain', formatDate(testTs(), 'AT_TIME'));
2479-2483: Use fixed timestamps to prevent flaky testsUsing dynamic timestamps can result in flaky tests. It's better to use a fixed timestamp or mock the system time to ensure consistent results.
Apply this diff:
- cy - .get('.patient-flow__list') - .find('.table-list__item .patient__action-ts') - .should('contain', formatDate(dayjs.utc().format(), 'TIME_OR_DAY')); + cy + .get('.patient-flow__list') + .find('.table-list__item .patient__action-ts') + .should('contain', formatDate(testTs(), 'TIME_OR_DAY'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/js/base/model.js(2 hunks)src/js/views/patients/sidebar/action/action-sidebar-activity-views.js(1 hunks)src/js/views/patients/sidebar/flow/flow-sidebar_views.js(1 hunks)test/integration/patients/patient/patient-flow.js(9 hunks)
🔇 Additional comments (6)
src/js/base/model.js (2)
3-3: LGTM! Good choice of date library.
The addition of dayjs is appropriate for handling UTC timestamps. It's a lightweight and modern alternative to moment.js.
88-89: Verify the timestamp handling in related views.
The addition of updated_at to the payload affects the UI components. Let's verify the implementation in related files:
✅ Verification successful
Based on the search results, I can now provide a final response as there is sufficient evidence to verify the timestamp handling in related views.
The timestamp handling is properly implemented across views
The updated_at timestamp is consistently used across the application with proper formatting:
- Program views format timestamps using
formatDateTimehelper:- Program sidebars show detailed time with
"AT_TIME"format - Program lists show relative time with
"TIME_OR_DAY"format
- Program sidebars show detailed time with
- Patient views handle timestamps similarly:
- Patient action/flow sidebars display detailed time
- Patient worklist and dashboard views show relative time
- Form views display update times in activity logs
The implementation is consistent and the timestamp changes will be properly reflected in all UI components.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the handling of updated_at attribute in related views
# Check for views listening to updated_at changes
rg -A 5 "updated_at|handleMessage" "src/js/views/"
# Check for potential test coverage
rg -A 5 "updated_at.*test" "test/"
Length of output: 54506
src/js/views/patients/sidebar/flow/flow-sidebar_views.js (1)
78-80: Verify complete model synchronization via websockets
Based on the past review comments, there are inconsistencies in how different attributes update via websockets. While name, owner, and now updated_at update correctly, details and state do not. This suggests potential issues with the websocket message handling.
Let's verify the model attribute synchronization:
test/integration/patients/patient/patient-flow.js (3)
2-2: Importing 'dayjs' library is appropriate
The addition of the dayjs import ensures proper date handling in the test cases.
125-125: Adding 'updated_at' attribute to 'testFlowAction'
Including updated_at: testTsSubtract(1) in testFlowAction ensures that the action's last updated timestamp is correctly set for testing.
2397-2397: Including 'routeFlowActivity()' in test setup
Adding .routeFlowActivity() ensures that flow activity is properly intercepted during tests, which is necessary for accurate simulation of application behavior.
src/js/base/model.js
Outdated
| @@ -84,6 +85,8 @@ export default Backbone.Model.extend(extend({ | |||
| return isFunction(messages[category]) ? messages[category] : this[messages[category]]; | |||
| }, | |||
| handleMessage({ category, payload }) { | |||
| payload.attributes = extend(payload.attributes || {}, { updated_at: dayjs.utc().format() }); | |||
There was a problem hiding this comment.
nitpick but I think extend({}, payload.attributes, updated_at is probably better since it'll remove the logic branch and it doesn't mutate the payload.attributes
There was a problem hiding this comment.
Agreed, I updated it to that 👍
…ia socket notification
nmajor25
force-pushed
the
ws-updated-at
branch
from
5e52dfc to
7df44f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
test/integration/patients/patient/patient-flow.js (2)
2409-2412: Consider adding negative test cases for UI interactions.The test covers basic UI interactions like clicking and closing, but consider adding test cases for:
- Multiple rapid clicks
- Interaction during socket updates
- Edge cases with null/undefined timestamps
cy .get('[data-header-region]') .click('top'); +// Add test for rapid clicks +cy + .get('[data-header-region]') + .click('top') + .click('top') + .click('top'); + +// Add test for interaction during socket update +cy.sendWs({ + category: 'NameChanged', + resource: { + type: 'flows', + id: testSocketFlow.id, + }, + payload: { + attributes: { + name: 'New Flow Name', + updated_at: null + }, + }, +});Also applies to: 2438-2442
2479-2482: Enhance timestamp format verification in list view.The test verifies the timestamp format in the list view, but it would be beneficial to add assertions for different time scenarios (same day, previous day, etc.).
cy .get('.patient-flow__list') .find('.table-list__item .patient__action-ts') .should('contain', formatDate(dayjs.utc().format(), 'TIME_OR_DAY')); + +// Add test for same day timestamp +const sameDayTime = dayjs.utc().subtract(1, 'hour'); +cy.clock(sameDayTime.valueOf()); +cy + .get('.patient-flow__list') + .find('.table-list__item .patient__action-ts') + .should('contain', formatDate(sameDayTime.format(), 'TIME_OR_DAY')); + +// Add test for previous day timestamp +const previousDayTime = dayjs.utc().subtract(1, 'day'); +cy.clock(previousDayTime.valueOf()); +cy + .get('.patient-flow__list') + .find('.table-list__item .patient__action-ts') + .should('contain', formatDate(previousDayTime.format(), 'TIME_OR_DAY'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
src/js/base/model.js(2 hunks)src/js/views/patients/sidebar/action/action-sidebar-activity-views.js(1 hunks)src/js/views/patients/sidebar/flow/flow-sidebar_views.js(1 hunks)test/integration/patients/patient/patient-flow.js(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/js/base/model.js
- src/js/views/patients/sidebar/action/action-sidebar-activity-views.js
- src/js/views/patients/sidebar/flow/flow-sidebar_views.js
🔇 Additional comments (4)
test/integration/patients/patient/patient-flow.js (4)
125-125: LGTM: Test data setup includes timestamp.
The addition of updated_at timestamp in the test action data properly simulates the real-world scenario where actions have timestamps.
2356-2356: LGTM: Socket test data includes timestamps.
The test data for socket notifications properly includes updated_at timestamps for both flow and action objects.
Also applies to: 2371-2371
2397-2397: LGTM: Flow activity routing is properly set up.
The test correctly sets up routing for flow activity which is essential for timestamp updates.
195-200: Verify timestamp format consistency.
The test verifies the timestamp display in the sidebar footer using formatDate with the 'AT_TIME' format. However, it would be good to ensure this format matches the requirements across all timestamp displays.
✅ Verification successful
Based on the search results, I can provide the final response as we have comprehensive information about timestamp format usage across the codebase. The search reveals two main timestamp formats being used consistently:
Timestamp format usage is consistent across the codebase
The codebase consistently uses two distinct timestamp formats for different contexts:
AT_TIME: Used in sidebars and detailed views to show precise timestampsTIME_OR_DAY: Used in list views and summary displays for relative time representation
The test's usage of formatDate with AT_TIME format in the sidebar footer aligns perfectly with the established pattern across similar components, as evidenced by multiple sidebar implementations using the same format (e.g., in action-sidebar, program-sidebar, flow-sidebar).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent timestamp format usage across the codebase
# Search for formatDate usage with AT_TIME format
rg "formatDate.*AT_TIME" -A 2
# Search for other timestamp format patterns
rg "formatDate.*TIME" -A 2
Length of output: 33135
Shortcut Story ID: [sc-56745]
When we receive a web socket notification of any kind for a flow or action, its
updated_atattribute will now be updated to the current date-time.This change is applied on the base model level. But will only be put to use on the patient flow page since that's the only place we currently subscribe to web socket notifications.
On the patient flow page, that is displayed in these UI locations:
Summary by CodeRabbit
Release Notes
New Features
updated_at) for better tracking.Bug Fixes
Documentation