feat(app): allow app update pop-up notifications to be disabled by mcous · Pull Request #6715 · Opentrons/opentrons

mcous · 2020-10-09T14:51:11Z

overview

This PR adds the ability for a user to disable app update pop-up notifications either via the pop-up or via a toggle in More > App > App Software Settings.

Closes #6684

~~This is a draft PR blocked by #6708. Will require rebase.~~

changelog

feat(app): allow app update pop-up notifications to be disabled

review requests

The acceptance criteria are in #6684, so take a look at that. As usual, manually editing version in app-shell/package.json to a previous version is the best way to test this with make -C app-shell dev

App notification pops up if app is "outdated"
Clicking on "Turn off update notifications" shows you a warning
Clicking "View App Software Settings" takes you to the App settings page
If you close and re-open the app, the notification does not pop up
If you navigate to the App settings and toggle the setting back on, the notification pops up the next time you open the app
- Note: The notification will also pop up as soon as you toggle it back on the first time you make the switch
If you open the app release notes outside of the notification, there's no "Turn off..." link
- Click "View Available Update" in More > App
- Click "Downgrade" (or whatever it says based on your fake dev version) on a Robot settings page and click "View App Update" on the warning modal that tells you to update your app before doing any robot updates

risk assessment

Medium risk. Mitigating factors:

We can / should smoke test actual app updates before merging / releasing
This PR doesn't mess with the actual plumbing of app updates at all, just the UI that hooks into it
Unit tests in this and previous PRs ensure the plumbing is still hooked into properly
I think this PR has very high test coverage, including of the UI and the user flows through the UI
- Most, if not all of it was TDD'd

sfoster1

Tested what's in the PR mesage, LGTM

Closes #6684

mcous · 2020-10-09T18:15:33Z

Reviewed with product and UX and requested changes have been made:

Widen UpdateAppModal to match other alert modals and give the buttons more breathing room
Switch heading of notifications toggle section to Title Case
Update copy and links in downgrade app section

This PR is good to review for final approval

mcous

Some stuff to know

mcous · 2020-10-09T14:56:56Z

components/__utils__/mountWithStore.js

-  return { wrapper, store }
+  // force a re-render by returning a new state to recalculate selectors
+  // and sending a blank set of new props to the wrapper
+  const refresh = maybeNextState => {


We do this all the time, so I figured I would add this into our mountWithStore helper. If we want to re-render a connected component to test how the UI reacts to state changes, we run into two problems:

Obviously, we have to tell enzyme to rerender the component

We have to convince any useSelector's to recalculate

This refresh helper does this by:

Calling the awkward wrapper.setProps({}) for us

Telling the getState mock to return a new object, which will tell useSelector to recalculate (after the react-redux upgrade included in refactor(app): trigger UpdateAppModal based on state.alerts #6708)

someSelector.mockReturnValue('foo') const { wrapper, refresh } = mountWithStore(...) expect(wrapper.exists(SomeComponent)).toBe(false) someSelector.mockReturnValue('bar') refresh() expect(wrapper.exists(SomeComponent)).toBe(true)

mcous · 2020-10-09T18:24:01Z

components/src/styles/typography.js

 export const FONT_SIZE_CAPTION = '0.625rem'
 export const FONT_SIZE_TINY = '0.325rem'
 export const FONT_SIZE_MICRO = '0.2rem'
+export const FONT_SIZE_INHERIT = 'inherit'


I added this just to be consistent, but I think of all the primitives stuff, these non-scale constants are the most tedious. I'd like to explore some or all of the following options:

Move common CSS constants to deduplicated vars rather than having a bunch of different ones depending on the property

e.g. we only need one const for auto, one fore inherit, etc.

Figure out if we can incorporate csstype into our style props

Might remove the need for most of our explicitly defined constants

Incorporating csstype sounds like a great idea. Maintaining our own explicitly defined constants has never felt sound or odor-less. It also makes for massive imports at the top of component files.

mcous · 2020-10-09T18:24:15Z

components/src/styles/colors.js

 export const C_BLUE = '#006fff'

 // colors by usage
-export const COLOR_DISABLED = '#9c9c9c'


Was not used anywhere I could find

mcous · 2020-10-09T18:25:01Z

components/src/modals/BaseModal.js

  backgroundColor: Styles.C_WHITE,
  position: Styles.POSITION_RELATIVE,
  overflowY: Styles.OVERFLOW_AUTO,
-  maxWidth: Styles.SIZE_6,


Realized the maxWidth was inappropriate to apply on the BaseModal. BaseModal will now stretch to 100% width of relative parent by default

codecov · 2020-10-09T18:43:45Z

Codecov Report

❗ No coverage uploaded for pull request base (hotfix_3.21.1@9432111). Click here to learn what that means.
The diff coverage is n/a.

@@               Coverage Diff                @@
##             hotfix_3.21.1    #6715   +/-   ##
================================================
  Coverage                 ?   80.23%           
================================================
  Files                    ?      240           
  Lines                    ?    20813           
  Branches                 ?        0           
================================================
  Hits                     ?    16700           
  Misses                   ?     4113           
  Partials                 ?        0

Impacted Files	Coverage Δ
opentrons/legacy_api/__init__.py	`100.00% <0.00%> (ø)`
...ver/robot_server/service/legacy/models/pipettes.py	`100.00% <0.00%> (ø)`
...rver/robot_server/service/legacy/routers/motors.py	`100.00% <0.00%> (ø)`
opentrons/protocols/geometry/deck.py	`92.44% <0.00%> (ø)`
opentrons/data_storage/schema_changes.py	`100.00% <0.00%> (ø)`
opentrons/hardware_control/robot_calibration.py	`100.00% <0.00%> (ø)`
...te-server/otupdate/buildroot/ssh_key_management.py	`78.40% <0.00%> (ø)`
..._server/robot/calibration/pipette_offset/models.py	`100.00% <0.00%> (ø)`
...server/robot_server/robot/calibration/constants.py	`96.77% <0.00%> (ø)`
robot-server/robot_server/service/system/models.py	`100.00% <0.00%> (ø)`
... and 230 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 9432111...8049edd. Read the comment docs.

Kadee80

🚫 🎶

mcous · 2020-10-09T20:07:39Z

Finished smoke testing with a "production" app and checked off the boxes in the test plan. Found one issue where "Turn off app update notifications" was showing up in the modal when it shouldn't have, and added a unit test + fix to the PR accordingly

mcous added feature Ticket is a feature request / PR introduces a feature WIP blocked Ticket or PR is blocked by other work ux review required robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). labels Oct 9, 2020

sfoster1 approved these changes Oct 9, 2020

View reviewed changes

mcous mentioned this pull request Oct 9, 2020

refactor(app): trigger UpdateAppModal based on state.alerts #6708

Merged

4 tasks

Base automatically changed from app_update-available-alert to hotfix_3.21.1 October 9, 2020 18:09

feat(app): allow app update pop-up notifications to be disabled

c7e8e66

Closes #6684

mcous force-pushed the app_disable-app-update-pop-up branch from 6715d41 to c7e8e66 Compare October 9, 2020 18:13

mcous added ready for review and removed WIP blocked Ticket or PR is blocked by other work ux review required labels Oct 9, 2020

mcous commented Oct 9, 2020

View reviewed changes

mcous marked this pull request as ready for review October 9, 2020 18:25

mcous requested a review from a team as a code owner October 9, 2020 18:25

mcous requested review from shlokamin and removed request for a team October 9, 2020 18:25

Kadee80 approved these changes Oct 9, 2020

View reviewed changes

fixup: fixes from smoke testing

8049edd

mcous merged commit 7982d5f into hotfix_3.21.1 Oct 9, 2020

mcous deleted the app_disable-app-update-pop-up branch October 9, 2020 21:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app): allow app update pop-up notifications to be disabled#6715

feat(app): allow app update pop-up notifications to be disabled#6715
mcous merged 2 commits intohotfix_3.21.1from
app_disable-app-update-pop-up

mcous commented Oct 9, 2020 •

edited

Loading

Uh oh!

sfoster1 left a comment

Uh oh!

mcous commented Oct 9, 2020 •

edited

Loading

Uh oh!

mcous left a comment

Uh oh!

mcous Oct 9, 2020 •

edited

Loading

Uh oh!

mcous Oct 9, 2020 •

edited

Loading

Uh oh!

b-cooper Oct 9, 2020

Uh oh!

mcous Oct 9, 2020

Uh oh!

mcous Oct 9, 2020

Uh oh!

codecov bot commented Oct 9, 2020 •

edited

Loading

Uh oh!

Kadee80 left a comment

Uh oh!

mcous commented Oct 9, 2020 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

mcous commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

overview

changelog

review requests

risk assessment

Uh oh!

sfoster1 left a comment

Choose a reason for hiding this comment

Uh oh!

mcous commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mcous left a comment

Choose a reason for hiding this comment

Uh oh!

mcous Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mcous Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

b-cooper Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

mcous Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

mcous Oct 9, 2020

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Kadee80 left a comment

Choose a reason for hiding this comment

Uh oh!

mcous commented Oct 9, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mcous commented Oct 9, 2020 •

edited

Loading

mcous commented Oct 9, 2020 •

edited

Loading

mcous Oct 9, 2020 •

edited

Loading

mcous Oct 9, 2020 •

edited

Loading

codecov bot commented Oct 9, 2020 •

edited

Loading

mcous commented Oct 9, 2020 •

edited

Loading