Closed rohanpurohit closed 3 years ago
@Sudhir-dev Item number 3 is necessary but seems like it might be complicated because it probably interacts with routing. We'll need to have documentation before proceeding with this
Observations from testing:
Perhaps we should pause work on this PR until after we merge #340, because that PR uses AlarmLimitsRequestStandby as the only place to store unsaved changes for AlarmLimitsRequest - so it will interact with the bug I describe in item 3, and it might even fix that behavior if the "Cancel" button confirmation dialog triggers a dispatch which changes the values of AlarmLimitsRequestStandby to be overwritten with the values of AlarmLimitsRequest.
Observations from testing:
- When I first start the frontend and am in standby mode, the alarm limits are initialized as SpO2 = [90 - 100], HR = [60 - 100]. If I change the alarm limits and press the "Discard Changes" button, the confirmation dialog which pops up asks if I want to keep SpO2 at [90 - 100] and HR at [60 - 100]. If I press "Confirm", it does what it says - good.
- Now I think the presence of "Discard Changes" and absence of the "Apply Changes" button in standby mode is confusing. When I see a button in the lower-right corner, I'm primed to think it means "Apply Changes" - this is the pattern set everywhere else in the frontend. I found myself instinctively trying to click it to save changes, without reading the button, Let's just hide the "Cancel"/"Discard Changes" button in standby mode, to make button placement in the frontend more consistent with assumptions we want to encourage the user to make. This also would make the Alarm Limits screen more consistent with the Quickstart screen.
I concur, I will disable it.
- When I start the frontend, start ventilation without changing any alarm limits, change the alarm limits from the alarm limits page, save the changes (e.g. to SpO2 = [90 - 99], HR = [60 - 99]), navigate to the dashboard, the dashboard reflects the new AlarmLimitsRequest values. But if I then navigate back to the alarm limits page, it shows the alarm limits as the values from before saving changes (namely it shows them as SpO2 = [90 - 100] and HR = [60 - 100]). Initially the "Cancel" and "Submit" buttons are disabled, even though the values shown on the screen are different from the actual values of AlarmLimitsRequest. Once I start adjusting the alarm limits, then the enabled/disabled state behaviors of the "Cancel" and "Submit" buttons become correct with respect to the actual values of the AlarmLimitsRequest (so e.g. if I use the alarm limits buttons to change the SpO2 and HR upper limits to 99 again, then the Cancel & Submit buttons are disabled; but once I change them back to 100, then the Cancel & Submit buttons become enabled)
Yep, I found this bug too, and was working on fixing that!
Perhaps we should pause work on this PR until after we merge #340, because that PR uses AlarmLimitsRequestStandby as the only place to store unsaved changes for AlarmLimitsRequest - so it will interact with the bug I describe in item 3, and it might even fix that behavior if the "Cancel" button confirmation dialog triggers a dispatch which changes the values of AlarmLimitsRequestStandby to be overwritten with the values of AlarmLimitsRequest.
Yes, I agree, meanwhile I'll try to locally get this PR working with those changes.
- When I start the frontend, start ventilation without changing any alarm limits, change the alarm limits from the alarm limits page, save the changes (e.g. to SpO2 = [90 - 99], HR = [60 - 99]), navigate to the dashboard, the dashboard reflects the new AlarmLimitsRequest values. But if I then navigate back to the alarm limits page, it shows the alarm limits as the values from before saving changes (namely it shows them as SpO2 = [90 - 100] and HR = [60 - 100]). Initially the "Cancel" and "Submit" buttons are disabled, even though the values shown on the screen are different from the actual values of AlarmLimitsRequest. Once I start adjusting the alarm limits, then the enabled/disabled state behaviors of the "Cancel" and "Submit" buttons become correct with respect to the actual values of the AlarmLimitsRequest (so e.g. if I use the alarm limits buttons to change the SpO2 and HR upper limits to 99 again, then the Cancel & Submit buttons are disabled; but once I change them back to 100, then the Cancel & Submit buttons become enabled)
This seems to be have been fixed after light testing on top of your store changes.
I tested triggering of confirmation dialog to discard unsaved changes when we attempt to go to the dashboard from the alarm limits page, and it works well. However, when we attempt to go to the settings page from the alarm limits page, this confirmation does not get triggered (but it should).
I realized another test procedure we should apply to this PR: go to the alarm limits page, change some limits, then open the events log and adjust alarm limits from there (the HR and SpO2 alarms need to be active for this to be possible), and see what the behavior of the Cancel and Submit buttons is like. With some light testing in this procedure, it already seems to work (even though we have local state controlling the enable/disable states rather than a selector), but I don't understand how it works.
I tested triggering of confirmation dialog to discard unsaved changes when we attempt to go to the dashboard from the alarm limits page, and it works well. However, when we attempt to go to the settings page from the alarm limits page, this confirmation does not get triggered (but it should).
thanks for testing. I will try to add that!
I realized another test procedure we should apply to this PR: go to the alarm limits page, change some limits, then open the events log and adjust alarm limits from there (the HR and SpO2 alarms need to be active for this to be possible), and see what the behavior of the Cancel and Submit buttons is like. With some light testing in this procedure, it already seems to work (even though we have local state controlling the enable/disable states rather than a selector), but I don't understand how it works.
https://github.com/pez-globo/pufferfish-software/pull/338/files#diff-a3a6688cdb1d790fc9bf6d27c0b6ff6b075351c0f1216481f61b5cb1f84cfc2eR365 it's because of this where this useEffect will check if there are any changes in Standby and current and disable/enable buttons accordingly
@ethanjli about opening confirmation dialog on clicking settings page, I think we'll have to add that in Navigation.tsx which might complicate things, one thing to note here is even after going to the settings page, pressing the dashboard button will still shows the confirmation dialog which I think is fair enough, only other thing there is pressing Home
in the sidebar menu will go straight to dashboard, but even then set alarms page will have the unsaved changes? so if it's ok with you I can maybe create a new issue for this and address it in a future PR?
it's because of this where this useEffect will check if there are any changes in Standby and current and disable/enable buttons accordingly
Oh! That useEffect almost looks like a selector already - unless I've missed something else in the code (which is likely), changing to a getAlarmLimitsUnsavedChanges
selector (which could maybe return a list of the unsaved changes, so that they're also usable with the unsaved changes confirmation dialogs) should simplify the code.
so if it's ok with you I can maybe create a new issue for this and address it in a future PR?
Yes, let's track it in a separate issue for a future PR
Here's my test procedure - the code generally passes these tests, except for a weird edge case:
I haven't looked at the code yet, but the way I expect the confirmation dialog to work is that it queries a selector to identify the list of all (relevant) differences between AlarmLimitsRequestStandby
and AlarmLimitsRequest
and shows it. I would expect this to be robust against the bugs identified in test cases 7 and 8 above. Since the code is behaving differently, there must be something else going on. I'll do a review of the code now.
- Making an unsaved change on the alarm limits screen, and changing a different alarm limits range from the Active Alarms modal: Go to the alarm limits screen (SpO2 alarm limits are [90 - 100] and HR limits are [60 - 120]), and make an unsaved change to set SpO2 alarm limits to [90 - 95]. Open the Active Alarms modal where HR is too high alarm is active. Open the HR Alarms modal, and set HR alarm limits to [60 - 110]. When I press Submit and Confirm, and then I close the Active Alarms modal, now the alarm limits screen shows HR alarm limits as [60 - 100], SpO2 is still shown as [90 - 95], and the Cancel/Submit buttons are still enabled ✅. But when I press the "Submit" button, it doesn't show the unsaved change to SpO2 alarm limits - the description is blank ❌. When I press the "Confirm" button, it does save the unsaved SpO2 alarm limit as [90 - 95].
- Making an unsaved change on the alarm limits screen, overwriting it from the Active Alarms modal, and making another unsaved change on the alarm limits screen: Go to the alarm limits screen (SpO2 alarm limits are [90 - 95] and HR limits are [60 - 110]), and make an unsaved change to set SpO2 alarm limits to [90 - 100]. Open the Active Alarms modal where the SpO2 and HR too high alarms are active. Open the HR Alarms modal, and set SpO2 alarm limits to [90 - 100]. When I press Submit and Confirm, and then I close the Active Alarms modal, now the alarm limits screen shows SpO2 alarm limits as [90 - 100], and the Cancel/Submit buttons are disabled ✅. Now change SpO2 alarm limits to [90 - 95]. When I press the "Submit" button, it doesn't show the unsaved change to SpO2 alarm limits - the description is blank ❌. When I press the "Confirm" button, it does save the unsaved SpO2 alarm limit as [90 - 95].
I haven't looked at the code yet, but the way I expect the confirmation dialog to work is that it queries a selector to identify the list of all (relevant) differences between
AlarmLimitsRequestStandby
andAlarmLimitsRequest
and shows it. I would expect this to be robust against the bugs identified in test cases 7 and 8 above. Since the code is behaving differently, there must be something else going on. I'll do a review of the code now.
@ethanjli This should fix it fb3d99797065a6b7f5417f80befa7324120071af
about the confirmation dialog quering a selector, I think what we have currently is best, since we need to see which of the two Spo2
or HR
is changed and pass that dynamically with the stateKey, might be complicated if we try to use selector there
For simplicity, I would suggest that the selector should just return a list of keys whose respective values are different between alarmLimitsRequest
and alarmLimitsRequestDraft
. Then you would just use that as the input for the .map
operation, which can check the keys against AlarmConfiguration
. This way the somewhat more complicated logic is all inside a selector (and there are several places we would reuse it), and the rendering stuff is in the component.
Hi @ethanjli if it's possible can you review the changes to the selectors? so what I'm doing now is taking your suggestion created a RangeSelector that gives out alarmLimits/current or draft as Record<string, Range> which I can re-use in AlarmsPage
and anywhere else, then taking those I compare it in the other selector which then returns a string[]
(maybe this one can be improved) and finally checking if the returned string[] is empty or not and that would be the unsaved changes. Also I'm not sure about the naming should it be getAlarmLimitsRequestUnsavedKeys
similarly for all, in some cases then it's too long.
thanks!
@ethanjli made the suggested changes and renamed a few things, looks good on testing on my end.
https://github.com/pez-globo/pufferfish-software/pull/338#discussion_r633041093 is the only remaining unresolved discussion, but I don't consider it a blocker, so I'm approving this PR for merging.
This PR:
apply changes
toSubmit
Submit
andCancel
button if there is no change in limits and vice versa.alignItems
ofGrid
must be used oncontainer
. , Following component has two or more children with the same key attribute: "indicator-[object Object]". This may cause glitches and misbehavior in rendering process. Component:dashboard
button set alarms page shows a confirmation dialog