Details

Reviewers

tomek
atul
• jacek
ashoat

Commits

rCOMMe4f88a80c756: [web] Add dispatching SET_DEVICE_ID action

Summary

I added a copmonent re-rendering on device ID change and setting a device ID whenever the device ID is null.
On logout/account deletion/cookie invalidation the reducer sets device ID to null, causing SET_DEVICE_ID action to get dispatched.
For now the device ID set on the login/create account screen is also used after login/account creation as I didn't find any reasons not to.
Persistance has not yet been added

Test Plan

Test using redux tools that the device ID is present and changes on logout, cookie invalidation and account deletion.

Diff Detail

Repository

rCOMM Comm

Branch

inka/redux_device_id

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

inka created this revision.Aug 19 2022, 2:49 AM

Herald added subscribers: • abosh, ashoat. · View Herald TranscriptAug 19 2022, 2:49 AM

inka added a parent revision: D4868: [web] Add device ID to redux.Aug 19 2022, 2:51 AM

Harbormaster completed remote builds in B11473: Diff 15784.Aug 19 2022, 2:59 AM

inka requested review of this revision.Aug 19 2022, 2:59 AM

inka added a child revision: D4887: [web] Add presisting device ID.Aug 19 2022, 3:23 AM

it is not yet possible to delete an account on web

It is possible. You can do that by opening account settings (cog icon in bottom left corner), then open danger zone and there's an option to do so.

web/redux/device-id-updater.js
2–12	In all the places we import `React` and use it instead of importing the effect. I think it might make sense to keep it consistent.
13	It might be a little safer to handle also `undefined`. We can do that by converting `deviceID` to boolean

This revision now requires changes to proceed.Aug 19 2022, 8:10 AM

Address the comments

It is possible. You can do that by opening account settings (cog icon in bottom left corner), then open danger zone and there's an option to do so.

Right, sorry, I missed that.

inka edited the test plan for this revision. (Show Details)Aug 22 2022, 12:00 AM

Harbormaster completed remote builds in B11487: Diff 15799.Aug 22 2022, 12:09 AM

tomek requested changes to this revision.Aug 22 2022, 5:33 AM

tomek added inline comments.

web/redux/device-id-updater.js
8–23 ↗	(On Diff #15799)	In all the places we split exporting and defining functions
20 ↗	(On Diff #15799)	We should always specify dependency array for effect hook. In this case skipping it wouldn't be a huge issue, but it is a good practice to never skip it.

This revision now requires changes to proceed.Aug 22 2022, 5:33 AM

Make code follow the project standars

Harbormaster completed remote builds in B11500: Diff 15816.Aug 22 2022, 7:01 AM

ashoat added inline comments.Aug 22 2022, 7:17 PM

web/redux/device-id-updater.js
13–19 ↗	(On Diff #15816)	If we end up adding more parameters into the dep list, or if `dispatch` changes, we could end up calling this twice in a row. It could be good to make sure we call this only when `!deviceID` becomes `true`. We could do this with two variables: const hasDeviceID = !!deviceID; const hadDeviceIDRef = React.useRef(hasDeviceID); React.useEffect(() => { if (hadDeviceIDRef.value && !hasDeviceID) { ... } hadDeviceIDRef.value = hasDeviceID; }, [hasDeviceID, dispatch]);