fix(app): properly enable/disable robot update from file button #6483

mcous · 2020-09-03T18:55:21Z

Overview

This PR tackles a few things that were blocking each other in the Robot Settings page UI. Primarily, it ensures:

The robot autoupdate button displays the correct message and tooltip
- Including switching the button text from "Up to date" to "Unavailable" if update files haven't been downloaded
The update from file button actually gets enabled / disabled properly based on the update server health
- Including tooltip if disabled
The update from file button has been moved render in the Advanced Settings card before the toggles rather than after the toggles
- Moved with @pantslakz's agreement (see the bottom of this Slack thread: https://opentrons.slack.com/archives/C9GER57PC/p1599065069000300)
The "robot out of date" modal won't trigger anymore if the update server isn't healthy

Fixes #5429

Changelog

Removed all usage of <Card>'s disabled prop
- The prop is kinda buggy and also sets pointer-events: none
- This bad for us because not all controls in a given card use the same disable logic as each other (e.g. in Advanced Settings, the toggles should be disabled if the api server is unhealthy, but update from file should be driven by the health of the update server)
- This change picks away at Grayed out buttons should explain why they're grayed out #4391
Adds in some extra Card-child level disable logic to account for the parent component no longer caring about disable logic
Moved robot update disable logic / messaging to a shared selector in the app's buildroot module

Review requests

Note: using the local dev server is a good way to create a "bad update server" condition.

Regular update button is disabled with tooltip if update-server is down
Update from file button is disabled with tooltip if update-server is down
All other Wi-Fi, Advanced Settings and Robot Controls buttons are disabled if robot server is down
- A lot of these still don't have tooltips per Grayed out buttons should explain why they're grayed out #4391

Risk assessment

Hopefully pretty low, but since we're touching the update flow, a smoke test on a Buildroot robot update and a Balena robot migration couldn't hurt

Fixes #5429

app/src/buildroot/selectors.js

codecov · 2020-09-03T19:20:15Z

Codecov Report

❗ No coverage uploaded for pull request base (edge@d615d2d). Click here to learn what that means.
The diff coverage is 73.80%.

@@           Coverage Diff           @@
##             edge    #6483   +/-   ##
=======================================
  Coverage        ?   56.36%           
=======================================
  Files           ?     1058           
  Lines           ?    16368           
  Branches        ?        0           
=======================================
  Hits            ?     9226           
  Misses          ?     7142           
  Partials        ?        0

Impacted Files	Coverage Δ
...c/components/RobotSettings/AdvancedSettingsCard.js	`0.00% <0.00%> (ø)`
...pp/src/components/RobotSettings/InformationCard.js	`0.00% <0.00%> (ø)`
...tSettings/UpdateBuildroot/MigrationWarningModal.js	`0.00% <ø> (ø)`
...s/RobotSettings/UpdateBuildroot/ViewUpdateModal.js	`0.00% <0.00%> (ø)`
.../components/RobotSettings/UpdateBuildroot/index.js	`0.00% <0.00%> (ø)`
.../src/components/RobotSettings/UploadRobotUpdate.js	`0.00% <0.00%> (ø)`
app/src/pages/Robots/RobotSettings.js	`0.00% <0.00%> (ø)`
components/src/primitives/Btn.js	`100.00% <ø> (ø)`
components/src/styles/colors.js	`100.00% <ø> (ø)`
app/src/buildroot/selectors.js	`81.48% <100.00%> (ø)`
... and 1065 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d615d2d...9492dff. Read the comment docs.

mcous · 2020-09-10T21:14:23Z

Smoke tested...

✅ Regular upgrade via file
✅ Balena migration via file

...and everything went smoothly (read: as smoothly as expected when the lack of motor control hardware attached to a bare Pi will make the health endpoint 503 without config modification)

app/src/buildroot/selectors.js

mcous · 2020-09-10T21:15:55Z

app/src/buildroot/__tests__/selectors.test.js

  ]

  SPECS.forEach(spec => {
    const { name, selector, state, expected, setup } = spec
    const args = spec.args || []
-    if (typeof setup === 'function') setup()


Fun fact: mock setup wasn't working at all in these tests...

sanni-t

Tested on my robot. Works as expected.
Just don't know how to test the case in the comment but looks like the unit test works so 👍

sanni-t · 2020-09-11T21:34:55Z

app/src/buildroot/selectors.js

+const OTHER_ROBOT_UPDATING =
+  'Unable to update because the app is currently updating a different robot.'
+const NO_UPDATE_FILES =
+  'Unable to retrieve update for this robot. Ensure your computer is connected to the internet and try again later.'


When does this happen? (Not having update files)

Mainly this will happen in two cases:

The user's computer is not connected to the internet

It's not able to check the releases.json manifest from S3 for the robot updates

fix(app): cache robot releases manifest for offline usage #6494 makes this happen less often, so that it will only show up if the computer is not connected to the internet and it has never been connected long enough to download the latest update

The user has updated the latest version of the app but the robot update hasn't finished building / hasn't been uploaded to the release bucket

So in the 30 to 60 min window right when the app release is cut

shlokamin

Left a few questions, but tests are very thorough and code looks great

shlokamin · 2020-09-14T14:20:37Z

app/src/buildroot/__tests__/selectors.test.js

-        getViewableRobots.mockReturnValueOnce([
+        getViewableRobots.mockReturnValue([


how come you switched mockReturnValueOnce to mockReturnValue?

I think mockReturnValue is more accurate to how the selectors would actually behave; a selector (if it chose to) might call some selector it is dependent on multiple times with the same input, and you would expect the same output.

These tests weren't very well written (by me) with respect to mocks, so I've refactored to be more clear

shlokamin · 2020-09-14T14:36:46Z

app/src/buildroot/__tests__/selectors.test.js

+        getRobotByName.mockImplementation((state, name) => {
+          return { ...mockReachableRobot, name }
+        })


Why would we overwrite the name of the robot here? When I console.log the name that gets passed in to the mock, it's undefined.

Wouldn't we want to always just return the name of mockReachableRobot, which is different than other-robot-name? I.e.

getRobotByName.mockReturnValueOnce(mockReachableRobot)

Oops, test was bad. Good catch!

shlokamin · 2020-09-14T14:45:08Z

app/src/components/RobotSettings/UpdateFromFileControl.js

+const UPDATE_FROM_FILE_DESCRIPTION = (
+  <>
+    If your app is unable to auto-download robot updates, you can{' '}
+    <Link external href="https://www.opentrons.com/ot-app/">
+      download the robot update yourself
+    </Link>{' '}
+    and update your robot manually
+  </>
+)


Should descriptions like these go in i18n?

I think yes. Do you feel like I should put more than the TODO comment above given the app does not currently have an i18n system?

Ah missed the TODO comment. Do you know whether there are plans for the app to support i18n? If not, maybe @Opentrons/spddrs could consider taking this on as a longer term snacklog project we could pick at on Friday afternoons/during escalations or whatever. Or maybe we determine that it's just not worth it for the app to support i18n. Regardless, it's easy to keep kicking the can with this sort of stuff, and we should decide one way or another whether we'd like to commit to i18n or not.

There are no plans that I'm aware of, but I think it's a great thing to tackle. There are a lot of TODO... i18n comments in the app's codebase

shlokamin · 2020-09-14T19:25:30Z

app/src/buildroot/__tests__/selectors.test.js

+    expect(result).toBe('downgrade')
+  })
+
+  it('should get update type when robot is matches the update', () => {


This name seems wrong. It should be something like should return reinstall type when robot matches the update right?

The update type is one of "upgrade", "downgrade", "reinstall", or null, but sure, can tweak the description

ohhh, I misunderstood what "update type" means in the description, I thought it meant "the update type of type upgrade", but that isn't the case

shlokamin · 2020-09-14T19:26:34Z

app/src/buildroot/__tests__/selectors.test.js

+    expect(result).toBe('upgrade')
+  })
+
+  it('should get update type when robot is ahead of the update', () => {


I think this should say should get downgrade type or something right?

shlokamin

LGTM!

fix(app): properly enable/disable robot update from file button

011a35b

Fixes #5429

mcous requested review from a team, Kadee80, amitlissack and sanni-t and removed request for a team September 3, 2020 18:55

mcous added app Affects the `app` project robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). WIP fix PR fixes a bug labels Sep 3, 2020

fixup: add some missing changes

b421743

mcous commented Sep 3, 2020

View reviewed changes

app/src/buildroot/selectors.js Outdated Show resolved Hide resolved

mcous mentioned this pull request Sep 4, 2020

refactor(app, robot-server): build out pipette offset calibration views and wire up to session #6473

Merged

2 tasks

mcous added 2 commits September 8, 2020 11:51

Merge branch 'edge' into app_update-from-file-tooltip

544a8ff

fixup: revert some selector changes and tweak copy

65b3d19

mcous added ready for review and removed WIP labels Sep 8, 2020

mcous marked this pull request as ready for review September 8, 2020 16:30

mcous requested a review from a team as a code owner September 8, 2020 16:30

mcous mentioned this pull request Sep 8, 2020

fix(app): cache robot releases manifest for offline usage #6494

Merged

Merge branch 'edge' into app_update-from-file-tooltip

18c7fe7

mcous commented Sep 10, 2020

View reviewed changes

fixup: remove console.log

95ad791

sanni-t approved these changes Sep 11, 2020

View reviewed changes

shlokamin reviewed Sep 14, 2020

View reviewed changes

fixup: refactor buildroot selector tests for correctness and readability

3fed4cd

shlokamin reviewed Sep 14, 2020

View reviewed changes

fixup: tweak update type selector test names

9492dff

shlokamin approved these changes Sep 14, 2020

View reviewed changes

mcous merged commit a996cdc into edge Sep 14, 2020

mcous deleted the app_update-from-file-tooltip branch September 14, 2020 20:46

mcous mentioned this pull request Oct 4, 2021

bug: Manual update button is sometimes grayed out even when the robot is available to accept an update #8432

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(app): properly enable/disable robot update from file button #6483

fix(app): properly enable/disable robot update from file button #6483

mcous commented Sep 3, 2020 •

edited

Loading

codecov bot commented Sep 3, 2020 •

edited

Loading

mcous commented Sep 10, 2020

mcous Sep 10, 2020

shlokamin Sep 14, 2020

sanni-t left a comment

sanni-t Sep 11, 2020 •

edited

Loading

mcous Sep 13, 2020

shlokamin left a comment

shlokamin Sep 14, 2020

mcous Sep 14, 2020 •

edited

Loading

shlokamin Sep 14, 2020

mcous Sep 14, 2020

shlokamin Sep 14, 2020

mcous Sep 14, 2020

shlokamin Sep 14, 2020

mcous Sep 14, 2020

shlokamin Sep 14, 2020

mcous Sep 14, 2020

shlokamin Sep 14, 2020 •

edited

Loading

shlokamin Sep 14, 2020

shlokamin left a comment

		getViewableRobots.mockReturnValueOnce([
		getViewableRobots.mockReturnValue([

fix(app): properly enable/disable robot update from file button #6483

fix(app): properly enable/disable robot update from file button #6483

Conversation

mcous commented Sep 3, 2020 • edited Loading

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Sep 3, 2020 • edited Loading

Codecov Report

mcous commented Sep 10, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanni-t left a comment

Choose a reason for hiding this comment

sanni-t Sep 11, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlokamin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcous Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlokamin Sep 14, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shlokamin left a comment

Choose a reason for hiding this comment

mcous commented Sep 3, 2020 •

edited

Loading

codecov bot commented Sep 3, 2020 •

edited

Loading

sanni-t Sep 11, 2020 •

edited

Loading

mcous Sep 14, 2020 •

edited

Loading

shlokamin Sep 14, 2020 •

edited

Loading