refactor(app): Remove ssid parameter from OnDeviceRouteParams #11930

koji · 2022-12-27T19:36:58Z

Overview

This PR removes :ssid from routers and passes ssid as a prop and move a couple of components from pages to organisms.
ConnectViaWifi renders components conditionally.

changeState: null => DisplayWifiList
changeState: !null && showSelectAuthenticationType: true => SelectAuthenticationType
changeState: !null && showSelectAuthenticationType: false => SetWifiCred
After tapping Connect button in SetWifiCred, ConnectViaWifi checks requestState

requestState.status pending => ConnectingNetwork
requestState.status success => SucceededToConnect
requestState.status failure => FailedToConnect

This PR also fixes UI diff between current edge and mid-fi design.

Changelog

remove :ssid from routers
remove 2 routes (set-wifi-cred and connected-network-info ) from OnDeviceDisplayApp
Update Wifi Connection screens to align with the mid-fi design
ConnectResult component changes into FailedToConnect component

Review requests

make -C app-shell push-ot3 host=dev_kit_ip_address and wifi auto connection process works as expected

Risk assessment

low

…parameter from router This PR removes :ssid parameter from route paths

codecov · 2022-12-27T19:41:09Z

Codecov Report

Merging #11930 (ee7098f) into edge (da72cfb) will increase coverage by 0.34%.
The diff coverage is 84.55%.

@@            Coverage Diff             @@
##             edge   #11930      +/-   ##
==========================================
+ Coverage   73.72%   74.06%   +0.34%     
==========================================
  Files        1439     2195     +756     
  Lines       47598    60734   +13136     
  Branches     2987     6464    +3477     
==========================================
+ Hits        35091    44985    +9894     
- Misses      12052    14221    +2169     
- Partials      455     1528    +1073

Flag	Coverage Δ
app	`72.48% <84.55%> (+26.14%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
app/src/App/OnDeviceDisplayApp.tsx	`44.44% <ø> (ø)`
...p/src/organisms/SetupNetwork/ConnectingNetwork.tsx	`100.00% <ø> (ø)`
...rganisms/SetupNetwork/SelectAuthenticationType.tsx	`61.90% <61.90%> (ø)`
app/src/pages/OnDeviceDisplay/ConnectViaWifi.tsx	`82.69% <82.69%> (ø)`
app/src/organisms/SetupNetwork/SetWifiCred.tsx	`83.33% <83.33%> (ø)`
.../src/organisms/SetupNetwork/SucceededToConnect.tsx	`91.66% <91.66%> (ø)`
app/src/organisms/SetupNetwork/DisplayWifiList.tsx	`100.00% <100.00%> (ø)`
app/src/organisms/SetupNetwork/FailedToConnect.tsx	`100.00% <100.00%> (ø)`
app/src/organisms/SetupNetwork/SearchNetwork.tsx	`100.00% <100.00%> (ø)`
...ons_hardware/hardware_control/move_group_runner.py	`82.77% <0.00%> (-5.90%)`	⬇️
... and 771 more

brenthagen · 2023-01-06T18:50:11Z

app/src/organisms/SetupNetwork/FailedToConnect.tsx

+ return (
+ <>
+ {isShowSetWifiCred ? (
+ <SetWifiCred ssid={ssid} />


SetWifiCred is the parent of this component - nesting an instance of the parent here potentially creates multiple instances of SetWifiCred all the way down:

Probably SelectWifiNetwork should manage switching back to SetWifiCred instead

brenthagen · 2023-01-06T18:50:57Z

app/src/pages/OnDeviceDisplay/SelectWifiNetwork.tsx

+ const [isShowSetWifiCred, setIsShowSetWifiCred] = React.useState<boolean>(
+ false
+ )
+ const [selectedSsid, setSelectedSsid] = React.useState<string>('')


is it possible to use a single local state value, string | null, here?

brenthagen · 2023-01-06T19:18:02Z

app/src/organisms/SetupNetwork/FailedToConnect.tsx

+ </SecondaryButton>
+ <PrimaryButton
+ flex="1"
+ onClick={() => history.push('/network-setup/wifi')}


this push now points to the page the app is on, so doesn't make any change.

same issue with history.push('/network-setup/wifi') in SetWifiCred and ConnectedNetworkInfo components

Merge branch 'edge' into refactor-remove-ssid-parameter-from-routes

…github.com/Opentrons/opentrons into refactor-remove-ssid-parameter-from-routes

brenthagen

a few comments to consider - overall a big improvement in page/component structure 👍

brenthagen · 2023-01-27T21:54:36Z

app/src/pages/OnDeviceDisplay/ConnectViaWifi.tsx

+ React.useEffect(() => {
+ dispatch(Networking.fetchWifiList(robotName))
+ }, [dispatch, robotName])


with the interval below, is this useEffect necessary?

brenthagen · 2023-01-27T22:22:46Z

app/src/organisms/SetupNetwork/SelectAuthenticationType.tsx

+ setChangeState,
+}: SelectAuthenticationTypeProps): JSX.Element {
+ const { t } = useTranslation(['device_settings', 'shared'])
+ const [isWpaTapped, setIsWpaTapped] = React.useState<boolean>(true)


this local state is duplicating selectedAuthType from the parent - should pass that in as a prop instead and delete this useState

brenthagen · 2023-01-27T22:33:20Z

app/src/pages/OnDeviceDisplay/ConnectViaWifi.tsx

+ React.useEffect(() => {
+ // a user selects none as authType
+ if (selectedSsid !== '' && selectedAuthType === 'none') {
+ const network = list.find((nw: WifiNetwork) => nw.ssid === selectedSsid)


i think this useEffect is causing issues on the SelectAuthenticationType screen - if i select "None" the connect request is made immediately, and if it fails, it fails silently. i don't see the failure until clicking "Next". instead, it seems more in line with what i expect to make the request on clicking "Next".

Related, if i select "None" then switch back to "WPA2 Personal" and click "Next", I get the failure screen without being prompted to enter a password. this is also unexpected.

instead of fixing now, we could make a bug ticket in order to get this large PR merged.

Will make a ticket for this

brenthagen · 2023-01-27T22:44:03Z

app/src/organisms/SetupNetwork/SucceededToConnect.tsx

+ React.useEffect(() => {
+ dispatch(fetchStatus(robotName))
+ dispatch(fetchWifiList(robotName))
+ }, [robotName, dispatch])


similar question, is the useEffect necessary with the intervals above? (and can those two be a single useInterval?

brenthagen · 2023-01-27T22:45:24Z

app/src/organisms/SetupNetwork/__tests__/FailedToConnect.test.tsx

+// import { DisplayWifiList } from '../DisplayWifiList'
+// import { SetWifiCred } from '../SetWifiCred'


some leftover comments

brenthagen · 2023-01-27T22:45:32Z

app/src/organisms/SetupNetwork/__tests__/FailedToConnect.test.tsx

+// const mockDisplayWifiList = DisplayWifiList as jest.MockedFunction<
+// typeof DisplayWifiList
+// >
+// const mockSetWifiCred = SetWifiCred as jest.MockedFunction<typeof SetWifiCred>


some leftover comments

brenthagen · 2023-01-27T22:45:51Z

app/src/organisms/SetupNetwork/__tests__/FailedToConnect.test.tsx

+ // mockDisplayWifiList.mockReturnValue(<div>Mock DisplayList</div>)
+ // mockSetWifiCred.mockReturnValue(<div>Mock SetWifiCred</div>)


some leftover comments

brenthagen · 2023-01-27T22:50:50Z

app/src/pages/OnDeviceDisplay/ConnectViaWifi.tsx

+ setPassword('')
+ }
+
+ const renderScreen = (): JSX.Element | null => {


it's just a matter of code style/preference, but these nested conditionals are tough to reason about (and could cause bugs). some options here:

use a switch statement

use useReducer

lift the nested conditions to a single flat level, like:

} else if (changeState.ssid != null && currentRequestState === null) {

instead of

} else if (changeState.ssid != null) { if (currentRequestState === null) {

koji · 2023-01-30T20:54:21Z

address the last comment
test this branch with a dev kit

* edge: (116 commits) feat(system-server): add sqlite database and barebones HTTP server (#12085) feat(ot3): add enableOT3FirmwareUpdates feature flag to gate firmware update functionality. (#12102) feat(app): add bare bones hardware section to protocol details (#12099) feat(app): Support failed calibrations in the calibration wizard (#12092) refactor(docs): clean up Versioning page (#12084) refactor(robot-server): Make run and protocol limits configurable at launch (#12094) feat(app): add robotServerVersion to display the current robot software version (#12096) fix(hardware): do not track tip motor positions (#12093) feat(engine): allow calibrateGripper command to save calibration data (#12046) feat(app, api-client, react-api-client): delete POC TLC calibration data from overflow menu (#12075) fix(api): actually update OT3 instrument calibration offset in cache instrument (#12089) fix(robot-server): correct the data returned from instruments endpoint (#12067) feat(api): add thermocycler plate lift to hardware controller (#12068) fix(app): reference moduleId from result not params (#12077) feat(app): create ODD protocol setup page (#12071) refactor(api): Deprecate presses and increment args when using PAPI pick_up_tip (#12079) fix(ot3): handle multiple responses for a tip action request (#12083) refactor(api): touch tip implementation for PAPIv2 engine core (#12053) refactor(app): Remove ssid parameter from OnDeviceRouteParams (#11930) fix(api): fix broken test in the api hardware controller (#12080) ...

koji added 2 commits December 27, 2022 14:28

refactor(app): refactoring wifi connection components to remove ssid …

08c7ec9

…parameter from router This PR removes :ssid parameter from route paths

fix SelectWifiNetwork test error

214daf9

koji added 2 commits December 27, 2022 14:43

remove connected-network-info from routes

3079890

fix test cases for connection errors

98253d0

koji requested review from shlokamin and brenthagen December 27, 2022 21:44

koji marked this pull request as ready for review December 27, 2022 22:25

koji requested a review from a team as a code owner December 27, 2022 22:25

Update FailedToConnect.tsx

8f73c60

koji mentioned this pull request Dec 30, 2022

feat(app): add screens for wifi manual connection and update wifi auto connection screen #11917

Closed

brenthagen reviewed Jan 6, 2023

View reviewed changes

koji added 9 commits January 9, 2023 16:52

add index

1c88d33

remove SetWifiCred from FailedToConnect

0373f17

Update FailedToConnect.tsx

e18a445

sync with edge to use app-shell-odd

2a3e402

Merge branch 'edge' into refactor-remove-ssid-parameter-from-routes

fix test errors

5cad119

fix test errors

a18b54a

temp

e927617

Merge branch 'refactor-remove-ssid-parameter-from-routes' of https://…

3b87c3e

…github.com/Opentrons/opentrons into refactor-remove-ssid-parameter-from-routes

Update connect via wifi components structure

d270f93

koji changed the title ~~refactor(app): Remove ssid parameter from routes~~ refactor(app): Remove ssid parameter from OnDeviceRouteParams Jan 12, 2023

koji marked this pull request as draft January 12, 2023 16:52

koji added 6 commits January 13, 2023 17:26

add ConnectViaWifi and DisplayWifiList then remove SeleectWifiNetwork

1cc7352

remove unused lines

eda87c5

fix test errors

e789775

Merge branch 'edge' into refactor-remove-ssid-parameter-from-routes

2e2cde7

Wifi Connection success case

658294a

fix header title position

b3ffc6b

koji added 12 commits January 16, 2023 12:49

add a missing prop

d7e8e64

modify rendering condition

bcd3c39

fix back button issues and update components structure

5ab0886

remove unnecessary margin

0150221

fix tests

40c8e07

remove unused lines

0eebd51

add tests for DisplayWifiList

f4b4599

update tests

c28f209

Merge branch 'edge' into refactor-remove-ssid-parameter-from-routes

d9d51c1

fix lint-js and check-js errors

8d9c747

fix ConnectViaWifi test

e2df991

add comment and clean up the test

14aefc4

koji marked this pull request as ready for review January 19, 2023 18:50

koji mentioned this pull request Jan 19, 2023

feat(odd): add manual connection for a hidden network #12033

Merged

solve the conflicts

6827cb0

brenthagen approved these changes Jan 27, 2023

View reviewed changes

koji added 3 commits January 30, 2023 13:48

Merge branch 'edge' into refactor-remove-ssid-parameter-from-routes

b71c58b

address comments

f1c3231

fix check-js error

27e7d88

koji added 3 commits January 30, 2023 16:10

remove nested if statemente

ec4c82a

fix test-error

9f25352

fix lint-js error

ee7098f

koji merged commit 4774d49 into edge Jan 31, 2023

koji deleted the refactor-remove-ssid-parameter-from-routes branch June 17, 2023 10:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(app): Remove ssid parameter from OnDeviceRouteParams #11930

refactor(app): Remove ssid parameter from OnDeviceRouteParams #11930

koji commented Dec 27, 2022 •

edited

Loading

codecov bot commented Dec 27, 2022 •

edited

Loading

brenthagen Jan 6, 2023

brenthagen Jan 6, 2023

brenthagen Jan 6, 2023

brenthagen Jan 6, 2023

brenthagen left a comment

brenthagen Jan 27, 2023

brenthagen Jan 27, 2023

brenthagen Jan 27, 2023

koji Jan 30, 2023

brenthagen Jan 27, 2023

brenthagen Jan 27, 2023

brenthagen Jan 27, 2023

brenthagen Jan 27, 2023

brenthagen Jan 27, 2023

koji commented Jan 30, 2023 •

edited

Loading

		// import { DisplayWifiList } from '../DisplayWifiList'
		// import { SetWifiCred } from '../SetWifiCred'

		// mockDisplayWifiList.mockReturnValue(<div>Mock DisplayList</div>)
		// mockSetWifiCred.mockReturnValue(<div>Mock SetWifiCred</div>)

refactor(app): Remove ssid parameter from OnDeviceRouteParams #11930

refactor(app): Remove ssid parameter from OnDeviceRouteParams #11930

Conversation

koji commented Dec 27, 2022 • edited Loading

Overview

Changelog

Review requests

Risk assessment

codecov bot commented Dec 27, 2022 • edited Loading

Codecov Report

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

brenthagen 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

Choose a reason for hiding this comment

koji commented Jan 30, 2023 • edited Loading

koji commented Dec 27, 2022 •

edited

Loading

codecov bot commented Dec 27, 2022 •

edited

Loading

koji commented Jan 30, 2023 •

edited

Loading