Page MenuHomePhabricator

[lib] add identity register action
ClosedPublic

Authored by varun on Dec 22 2023, 7:54 AM.
Tags
None
Referenced Files
F3149432: D10454.diff
Mon, Nov 4, 1:56 PM
Unknown Object (File)
Mon, Oct 21, 11:39 PM
Unknown Object (File)
Thu, Oct 17, 11:14 PM
Unknown Object (File)
Thu, Oct 17, 11:14 PM
Unknown Object (File)
Thu, Oct 17, 11:14 PM
Unknown Object (File)
Thu, Oct 17, 11:14 PM
Unknown Object (File)
Thu, Oct 17, 11:13 PM
Unknown Object (File)
Thu, Oct 17, 11:13 PM
Subscribers
None

Details

Summary

since this action is native-specific, i've created a new interface for native-only client methods.

Depends on D10440

Test Plan

successfully dispatched the action on native and registered a new account with staging identity service

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

varun requested review of this revision.Dec 22 2023, 8:14 AM

i'll re-request review after i put up the diff with the actual useIdentityRegister callsite in a sec

ashoat requested changes to this revision.Dec 23 2023, 8:35 AM

Where is the diff stack here? You're using types that are defined in neither the diff stack nor on master...

lib/actions/user-actions.js
27 ↗(On Diff #34991)

Where is this type defined?

246 ↗(On Diff #34991)
  1. Why is this returned a string if it's just going to get parsed immediately?
  2. Where is registerUser defined?
lib/actions/user-actions.js
27 ↗(On Diff #34991)

it's defined below in lib/types/identity-service-types.js

246 ↗(On Diff #34991)
  1. it's because we return a string in CommRustModule due to Rust limitations
  2. it's defined in CommRustModule
This comment has been deleted.
lib/types/identity-service-types.js
31 ↗(On Diff #34991)

This needs to extend IdentityServiceClient

lib/actions/user-actions.js
259 ↗(On Diff #34991)

Where does the rest of CurrentUserInfo come from if not from identity?

lib/actions/user-actions.js
259 ↗(On Diff #34991)

Related discussion: https://linear.app/comm/issue/ENG-4210/handle-avatars-in-currentuserinfo#comment-6bcc3e2a
It looks like we at least want the username to be returned here

lib/actions/user-actions.js
259 ↗(On Diff #34991)

the client provides the username when it registers. for the time being we'll have to continue to get settings / avatar from the keyserver

lib/actions/user-actions.js
259 ↗(On Diff #34991)

ah i understand your question better now. yes, we should return username here as well so that the reducers can use it

lib/actions/user-actions.js
259 ↗(On Diff #34991)

Can you also rename userId to userID? This is the way we usually name variables in our JS codebase

lib/types/identity-service-types.js
25 ↗(On Diff #35581)

talked to @tomek about this and we agreed that making this method optional on both platforms and providing it only on native is the best option. it forces the developer to think about whether this method is actually available before using it

This revision is now accepted and ready to land.Jan 13 2024, 6:26 PM
This revision was automatically updated to reflect the committed changes.