Page MenuHomePhabricator
Differential D12307

Add identity platform details to aux user store
ClosedPublic
Actions

Authored by marcin on Tue, Jun 4, 1:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Jun 28, 6:49 PM
Unknown Object (File)
Fri, Jun 28, 11:57 AM
Unknown Object (File)
Thu, Jun 27, 5:18 AM
Unknown Object (File)
Wed, Jun 26, 5:46 AM
Unknown Object (File)
Sun, Jun 23, 9:33 PM
Unknown Object (File)
Sun, Jun 23, 8:22 PM
Unknown Object (File)
Sun, Jun 23, 2:51 PM
Unknown Object (File)
Sun, Jun 23, 1:02 AM
View All 43 Files
Subscribers
ashoat
tomek

Details

Reviewers
bartek
kamil
Commits
rCOMMca08c62896e9: Add identity platform details to aux user store
Summary

This differential add identity platform details to aux user store and reducer

Test Plan

Flow. running reducer jest test.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

marcin created this revision.Tue, Jun 4, 1:18 AM
Herald added subscribers: tomek, ashoat. · View Herald TranscriptTue, Jun 4, 1:18 AM
marcin added a child revision: D12308: Enable the client to fetch users platform details from identity RPC.Tue, Jun 4, 1:26 AM
Harbormaster completed remote builds in B29419: Diff 40920.Tue, Jun 4, 1:34 AM
marcin requested review of this revision.Tue, Jun 4, 1:34 AM
marcin added a parent revision: D12283: [grpc_clients] Accept PlatformMetadata when creating client.Tue, Jun 4, 7:13 AM
ashoat added inline comments.Tue, Jun 4, 10:52 AM
lib/flow-typed/npm/tcomb_v3.x.x.js
122 ↗(On Diff #40920)

I assume you tested and confirmed that tcomb can actually support this, yeah?

lib/types/identity-service-types.js
333–336 ↗(On Diff #40920)

Why isn't this read-only?

marcin added inline comments.Wed, Jun 5, 2:47 AM
lib/flow-typed/npm/tcomb_v3.x.x.js
122 ↗(On Diff #40920)

Further diffs in this stack use enums.of validator for array of numbers when returning identity response to JS. Additionally without this change flow complained when I used enums.of for array of numbers and after this change it was fixed. Do you think we need additional validation for this change?

lib/types/identity-service-types.js
333–336 ↗(On Diff #40920)

Clearly my oversight.

marcin updated this revision to Diff 40977.Wed, Jun 5, 2:59 AM

Make identity platform details type readonly

Harbormaster completed remote builds in B29447: Diff 40977.Wed, Jun 5, 3:15 AM
marcin planned changes to this revision.Wed, Jun 5, 5:35 AM

Those types are totally wrong. We need mapping from device id to platform details. Interestingly reducer worked as expected - it put map into the SQLite.

ashoat added inline comments.Wed, Jun 5, 6:04 AM
lib/flow-typed/npm/tcomb_v3.x.x.js
122 ↗(On Diff #40920)

If later diffs use this validator and it works, then I think that is sufficient for testing

marcin updated this revision to Diff 40984.Wed, Jun 5, 6:47 AM

Fix types. Use a map of [deviceID]: IdentityPlatformDetails

Harbormaster completed remote builds in B29454: Diff 40984.Wed, Jun 5, 7:03 AM
kamil accepted this revision.Thu, Jun 6, 1:23 AM
kamil added inline comments.
lib/types/aux-user-types.js
13 ↗(On Diff #40984)

worth noting that all keys in this map are values in the devices array in RawDeviceList above - wondering if there is a better way to type this structure

This revision is now accepted and ready to land.Thu, Jun 6, 1:23 AM
marcin added inline comments.Thu, Jun 6, 5:34 AM
lib/types/aux-user-types.js
13 ↗(On Diff #40984)

I talked to @bartek about this and he would rather leave this type as it is. His reasons are:

  1. platofrmDetails property would need to be optional anyway since we can have device in device list for which we don't have platformDetails.
  2. In most cases we will only need the device list so having to do a map() each time we fetch data from aux_users would be less convenient.
marcin updated this revision to Diff 41055.Thu, Jun 6, 8:22 AM

Rebase before landing

Harbormaster completed remote builds in B29490: Diff 41055.Thu, Jun 6, 8:38 AM
Closed by commit rCOMMca08c62896e9: Add identity platform details to aux user store (authored by marcin). · Explain WhyThu, Jun 6, 8:59 AM
This revision was automatically updated to reflect the committed changes.
marcin added a commit: rCOMMca08c62896e9: Add identity platform details to aux user store.

Revision Contents
Changeset List

PathSize
lib/
flow-typed/
npm/
4 lines
reducers/
1 line
35 lines
types/
10 lines
22 lines

Diff 41060

View Options

lib/flow-typed/npm/tcomb_v3.x.x.js

Loading...
View Options

lib/reducers/aux-user-reducer.js

Loading...
View Options

lib/reducers/aux-user-reducer.test.js

Loading...
View Options

lib/types/aux-user-types.js

Loading...
View Options

lib/types/identity-service-types.js

Loading...