feat(app): add wifi disconnect #5028

iansolano · 2020-02-19T20:13:10Z

overview

Refactors select network functionality on robot settings screen. Handles disconnect, connect, and joining of wifi networks.

changelog

review requests

api/src/opentrons/server/endpoints/networking.py

codecov · 2020-02-26T20:02:38Z

Codecov Report

Merging #5028 into edge will increase coverage by 0.24%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             edge    #5028      +/-   ##
==========================================
+ Coverage   68.86%   69.10%   +0.24%     
==========================================
  Files        1082     1090       +8     
  Lines       36045    36158     +113     
==========================================
+ Hits        24822    24988     +166     
+ Misses      11223    11170      -53

Impacted Files	Coverage Δ
...omponents/RobotSettings/SelectNetwork/constants.js	`100.00% <0.00%> (ø)`
app/src/networking/epic/disconnectEpic.js	`100.00% <0.00%> (ø)`
...Settings/SelectNetwork/SelectNetworkModal/index.js	`100.00% <0.00%> (ø)`
...SelectNetworkModal/ConnectDisconnectModal/utils.js	`33.33% <0.00%> (ø)`
...obotSettings/SelectNetwork/SelectSsid/constants.js	`100.00% <0.00%> (ø)`
...SelectNetworkModal/ConnectDisconnectModal/index.js	`50.00% <0.00%> (ø)`
...Settings/SelectNetwork/SelectNetworkModal/utils.js	`16.66% <0.00%> (ø)`
...rc/components/RobotSettings/SelectNetwork/hooks.js	`100.00% <0.00%> (ø)`
...rc/components/RobotSettings/SelectNetwork/utils.js	`90.00% <0.00%> (ø)`
components/src/forms/Select.js	`93.33% <0.00%> (+6.66%)`	⬆️
... and 3 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 3713002...5c9424e. Read the comment docs.

mcous

Batch of comments. Haven't gotten to the big components yet, so more incoming tomorrow

app/src/networking/index.js

app/src/networking/epic/wifiListEpic.js

app/src/networking/constants.js

app/src/networking/actions.js

app/src/components/RobotSettings/ConnectionCard.js

...rc/components/RobotSettings/SelectNetwork/SelectNetworkModal/ConnectDisconnectModal/index.js

...rc/components/RobotSettings/SelectNetwork/SelectNetworkModal/ConnectDisconnectModal/utils.js

app/src/components/RobotSettings/SelectNetwork/index.js

mcous

Lot of comments in this one, but the only two that are important to tackle in this PR before merging are:

"Join other" and "Disconnect" need to be in groups to get group separators
"Disconnect" should not be an option if the robot isn't on Wi-Fi

The rest of my comments are important, but addressing them before we switch to the better actions / state will result in unnecessary churn, so please resist the temptation

mcous · 2020-02-27T22:06:14Z

app/src/components/RobotSettings/SelectNetwork/SelectSsid/index.js

 }

+const SELECT_ACTIONS_OPTIONS = [
+ { value: JOIN_OTHER_VALUE },


The { options: [{value: JOIN_OTHER_VALUE}] } was intentional and created an option group rather than a simple option. Removing the group wrapper causes the list to render incorrectly, without the separator

This build:

Production:

mcous · 2020-02-27T22:07:43Z

app/src/components/RobotSettings/SelectNetwork/SelectSsid/index.js

+ if (showWifiDisconnect) {
+ return list
+ .map(({ ssid }) => ({ value: ssid }))
+ .concat(SELECT_ACTIONS_OPTIONS)


Per screens, "disconnect" should be in its own group at the beginning of the list. We don't have to address in this PR, but adding this comment because it relates to the option group comment above

(Ignore incorrect background color + various other wrong stuff in screens)

mcous · 2020-02-27T22:13:54Z

app/src/components/RobotSettings/SelectNetwork/SelectNetworkModal/index.js

+ failure,
+ modalOpen,
+ ssid,
+ previousSsid,


previousSsid seems entirely redundant with the presence of networkingType or am I missing something?

yeah, it can go away once we fix up the handler, which will be made even easier by splitting up the onChange work

mcous · 2020-02-27T22:19:28Z

app/src/components/RobotSettings/SelectNetwork/hooks.js

+ const [networkingType, setNetworkingType] = useState<NetworkingType | null>(
+ null
+ )
+ const [securityType, setSecurityType] = useState<WifiSecurityType | null>(


This is only a concern when joining an unknown network; I don't think it needs to be up here

mcous · 2020-02-27T22:19:48Z

app/src/components/RobotSettings/SelectNetwork/hooks.js

+ const [securityType, setSecurityType] = useState<WifiSecurityType | null>(
+ null
+ )
+ const [modalOpen, setModalOpen] = useState<boolean>(false)


This seems redundant when networkingType is a thing

app/src/pages/Robots/index.js

app/src/components/RobotSettings/SelectNetwork/index.js

app/src/networking/epic/__tests__/wifiListEpic.test.js

mcous · 2020-02-27T22:52:58Z

app/src/components/RobotSettings/SelectNetwork/index.js

- getActiveSsid(): ?string {
- const activeNetwork = find(this.props.list, 'active')
- return activeNetwork && activeNetwork.ssid
+ setSsid(currentSsid)


The fact that one handler sets all these different things is a red flag. At most I think we need two pieces of state:

Current action: connect, disconnect, join other, null (close modal)

Network to join (if action is connect)

Disconnect needs to hang on to the active network so it can display proper copy after it's no longer marked "active" in state, but that should be local to whatever DisconnectModal we write

add wifi disconnect feature to robot settings screen closes #4849

mcous · 2020-03-25T14:06:19Z

Closing in favor of #5234 and #5280. Those PRs were both created from this branch, which is now safe to delete

iansolano added WIP DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available labels Feb 19, 2020

iansolano requested review from a team as code owners February 19, 2020 20:13

amitlissack reviewed Feb 19, 2020

View reviewed changes

api/src/opentrons/server/endpoints/networking.py Outdated Show resolved Hide resolved

iansolano force-pushed the app_add-wifi-disconnect branch 2 times, most recently from 395ef20 to 7997770 Compare February 21, 2020 21:36

mcous mentioned this pull request Feb 24, 2020

refactor(app): add wifi configure actions and epics to networking module #5074

Merged

4 tasks

iansolano force-pushed the app_add-wifi-disconnect branch 2 times, most recently from 2e43469 to 14ac643 Compare February 26, 2020 21:52

mcous reviewed Feb 26, 2020

View reviewed changes

iansolano added the robot-svcs Falls under the purview of the Robot Services squad (formerly CPX, Core Platform Experience). label Feb 27, 2020

iansolano commented Feb 27, 2020

View reviewed changes

app/src/components/RobotSettings/SelectNetwork/index.js Outdated Show resolved Hide resolved

iansolano force-pushed the app_add-wifi-disconnect branch from 2442de2 to 27ee539 Compare February 27, 2020 21:55

iansolano changed the title ~~[WIP] feat(app): add wifi disconnect~~ feat(app): add wifi disconnect Feb 27, 2020

iansolano added DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available and removed DO NOT MERGE Indicates a PR should not be merged, even if there's a shiny green merge button available WIP labels Feb 27, 2020

mcous reviewed Feb 27, 2020

View reviewed changes

sanni-t and others added 10 commits March 2, 2020 13:10

Disconnect working

cf9c280

add eth0 connectivity check. add networking unit test

8bbb815

remove eth connection check

29c6407

string required

6f0a5ad

feat(app): add wifi disconnect to robot settings

30e4fa6

add wifi disconnect feature to robot settings screen closes #4849

more tests, refactors, and networking setup

ea980dd

continuing refactors

27eebae

wiring up loading state and success/failure handlers

d21b1cb

adding error handling to disconnect

e9abeee

refactors and adding tests

51af6e0

iansolano added 18 commits March 2, 2020 13:34

refactors and adding tests

e73440e

fixing types and adding tests

2c3f995

adding types

2c2199b

cleanup

55f4a85

adding line

8c3a947

add utils and types

c98eb53

more component cleanup

283dc7d

renaming component

b60f3dd

updating tests based on rebase

720af19

moved formtable and add back in key

806c8cc

addressing PR feedback and refactors

456b075

adding feature flag

744443f

add API version check for feature

a833771

refactoring select styles

01af6fb

refactoring dropdown layout

05c2c40

refactoring based on PR comments

68796e6

starting tests fixups

6480e27

fixing ConnectionCard test

a9cf07d

iansolano force-pushed the app_add-wifi-disconnect branch from 96a2d09 to a9cf07d Compare March 2, 2020 18:41

fixing css linter

5c9424e

mcous mentioned this pull request Mar 16, 2020

refactor(app): add client state for Wi-Fi disconnect #5234

Merged

mcous mentioned this pull request Mar 24, 2020

refactor(app): Rewrite Wi-Fi connection flows to use new HAPI state #5280

Merged

mcous closed this Mar 25, 2020

mcous deleted the app_add-wifi-disconnect branch May 6, 2020 23:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(app): add wifi disconnect #5028

feat(app): add wifi disconnect #5028

iansolano commented Feb 19, 2020 •

edited

Loading

codecov bot commented Feb 26, 2020 •

edited

Loading

mcous left a comment

mcous left a comment

mcous Feb 27, 2020

mcous Feb 27, 2020

mcous Feb 27, 2020

iansolano Feb 28, 2020

mcous Feb 27, 2020

mcous Feb 27, 2020

mcous Feb 27, 2020

iansolano Feb 28, 2020

mcous commented Mar 25, 2020

feat(app): add wifi disconnect #5028

feat(app): add wifi disconnect #5028

Conversation

iansolano commented Feb 19, 2020 • edited Loading

overview

changelog

review requests

codecov bot commented Feb 26, 2020 • edited Loading

Codecov Report

mcous left a comment

Choose a reason for hiding this comment

mcous left a 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

Choose a reason for hiding this comment

mcous commented Mar 25, 2020

iansolano commented Feb 19, 2020 •

edited

Loading

codecov bot commented Feb 26, 2020 •

edited

Loading