refactor(app): Drive device reset settings fully from client#17921
Conversation
Just for readability.
…slideout. This keeps all the implicit onDeviceDisplay special-casing contained to one file.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## chore_release-8.4.0 #17921 +/- ##
=======================================================
- Coverage 25.82% 25.43% -0.39%
=======================================================
Files 3005 2936 -69
Lines 227433 225275 -2158
Branches 18912 19272 +360
=======================================================
- Hits 58724 57309 -1415
+ Misses 168696 167953 -743
Partials 13 13
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
| // handleClearData assumes options are loaded. | ||
| !areServerOptionsLoaded |
There was a problem hiding this comment.
Disabling this button if the server options haven't been loaded yet is important to prevent this:
- The slideout slides out
- We try fetching the available options from the server, but it's slow
- The user selects a bunch of checkboxes and hits the submit button
- The request for available options from the server still hasn't returned, so
serverOptionsis still[], so it looks tobuildResetRequest()as if all of the options are too new for the server to understand, so it filters them all out. The robot restarts, but we haven't actually reset anything.
Is there a safer, more foolproof way to deal with this?
Failing that, is there an easy way to throw in a loading indicator of some sort if this button is disabled because the server options haven't been loaded yet?
There was a problem hiding this comment.
Unfortunately, no, I don't think there is an easy way of doing this given how this network request is tied to a selector in the manner that it is. I think the actual solution would be to modern the way we get this data, by placing it in a bone fide hook that wraps react-query, which calls for the settings.
Barring that, I think we could "fake" a loading state by initializing the settings to something like 'INITIALIZING', and explicitly check for the presence of that string, showing loading state as needed.
| // If the server is older than this app, we might send it options that it doesn't | ||
| // understand, which it could choke on. Filter to send only the options that the | ||
| // server advertises. | ||
| const serverResetOptionIds = serverResetOptions.map(o => o.id) | ||
| requestToReturn = Object.fromEntries( | ||
| Object.entries(requestToReturn).filter(([k, _v]) => | ||
| serverResetOptionIds.includes(k) | ||
| ) | ||
| ) |
There was a problem hiding this comment.
We were accounting for this before by not even showing the options that were missing from the server's advertisement. Now all of the options are always shown, so we need to filter them here.
mjhuff
left a comment
There was a problem hiding this comment.
Looks great, thanks! Nice and clean. If you're keen on adding explict loading state in this PR, we could do so, but I don't think it's necessary for merging.
TamarZanzouri
left a comment
There was a problem hiding this comment.
YOU DID IT!!!! nice job Max!
…8244) ## Overview Fixes RQA-4161. In PR #17921, I apparently wrote a stupid bug where I just outright forgot to send `runsHistory: true` when that option was selected in the UI. ## Test Plan and Hands on Testing * [x] Push it to a Flex and test manually. * [x] Manually review the reset code (both ODD and desktop app) for other similar bugs [This exact case *was* already unit tested.](https://github.com/Opentrons/opentrons/blob/00b83c9c5102041e5a22fb615f1fd660ca44c796/app/src/organisms/ODD/RobotSettingsDashboard/__tests__/DeviceReset.test.tsx#L72) However, there appears to be some kind of bug in the test causing that assertion to always pass, no matter how wrong the value is. ~~I suspect this has to do with a misconfigured mock somewhere, but I haven't figured it out yet.~~ See [comment below](#18244 (comment)). ## Review requests Any ideas for making the `buildResetRequest()` code inherently safer? See [comment below](#18244 (comment)) for the test bug. Does this fix make sense? ## Risk assessment The fix is low-risk. I need therapy for the test bug, though.
Overview
This refactors the desktop app and ODD components for doing a device reset.
Goes towards EXEC-1281.
Background
Historically, all of the reset options were server-defined. The client would fetch the available options from the server (via
GET /settings/reset/options) and display them as-is, with server-defined text.More recently, that model has been stretched beyond its limits:
onDeviceDisplay, to be special: they should be hidden from the UI and selected implicitly depending on the user's selection of other options./settings/reset. (See e.g. this discussion, though I sometimes have second thoughts about this.)So we have been in a weird in-between where we were getting the dynamic list of options from the server, but still making strong assumptions about what that list would contain. This PR rewrites it to be fully client-decided.
Changelog
This is intended to not have any user-facing change.
The one "accidental" change that I'm aware of is that if the client is newer than the server and has an extra reset option, it will now be displayed, and filtered out of the HTTP request. Formerly, it would not even be displayed.
Test Plan and Hands on Testing
Review requests
Specific things: see review comments below.
Also general things: React and TypeScript idioms, state structuring, and so on.
Risk assessment
Medium. This code is used for last-ditch troubleshooting and system recovery, so it needs to always work.
Possible failures include: