Page MenuHomePhabricator

[keyserver/lib/native] Create updates for new role creation
ClosedPublic

Authored by rohan on Jul 3 2023, 10:18 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 3:20 AM
Unknown Object (File)
Tue, Nov 26, 7:38 AM
Unknown Object (File)
Tue, Nov 26, 7:36 AM
Unknown Object (File)
Tue, Nov 26, 7:19 AM
Unknown Object (File)
Sat, Nov 23, 7:15 AM
Unknown Object (File)
Fri, Nov 22, 3:30 AM
Unknown Object (File)
Thu, Nov 21, 10:48 AM
Unknown Object (File)
Thu, Nov 7, 9:56 PM
Subscribers

Details

Summary

We want to generate updates for the thread when a new role is created and each member of that thread should recieve the update. The next diff will handle the client-side update of pulling the updated threadInfo from redux (a solution discussed in my 1:1 with Ashoat).

Depends on D8421

Test Plan

Verified that the threadInfo was successfully updated in redux after the updates were generated, and checked the thread store operations in the reducer and confirmed that the thread that needed to be replaced was the one that just had a new role created for it.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan edited the test plan for this revision. (Show Details)
rohan requested review of this revision.Jul 3 2023, 10:36 AM
keyserver/src/creators/role-creator.js
116 ↗(On Diff #28368)

Are you sure that the members list of fetchThreadInfos contains all pseudo-members that need the UPDATE_THREAD? If you have visibility into a threadInfo, you need to get this update, even if you're a member. You might need to use fetchServerThreadInfos instead of fetchThreadInfos here, but I'm not 100% sure

keyserver/src/creators/role-creator.js
116 ↗(On Diff #28368)

I checked this, and I wasn't sure why I would need ServerThreadInfo when writing the code because when I introspected into both the ServerThreadInfo and RawThreadInfo and checked the member IDs, they both seemed to be identical. I'm unsure if there's a case for when the containing member IDs would be different between the two?

In regards to if we fetch the ServerThreadInfo, we could either independently fetch the RawThreadInfo at the same time to include in the return, or use a provided helper method like rawThreadInfoFromServerThreadInfo, so it should be easy enough.

keyserver/src/creators/role-creator.js
116 ↗(On Diff #28368)

This is the condition I'm concerned about. Try creating a community, adding a member, and then creating a subchannel of the community without that member. You'll probably find that member in the ServerThreadInfo for that subchannel, but NOT the RawThreadInfo

keyserver/src/creators/role-creator.js
116 ↗(On Diff #28368)

Ah you're right, ok I'll update this

Use ServerThreadInfo instead of RawThreadInfo to generate updates for thread members, and use rawThreadInfosFromServerThreadInfos to get the necessary RawThreadInfo to return instead

ashoat added inline comments.
keyserver/src/creators/role-creator.js
131–135 ↗(On Diff #28421)

Can these lines be moved to the end of the function? They're not used until then

This revision is now accepted and ready to land.Jul 5 2023, 10:57 AM

Move lines to the end of the function

keyserver/src/creators/role-creator.js
153 ↗(On Diff #28430)

Are you sure it's safe to call createUpdates before dbQuery has completed? I worry that createUpdates will fetch from the roles table, and might fetch stale content

keyserver/src/creators/role-creator.js
153 ↗(On Diff #28430)

Oh you might be right - I haven't had any issues on my dev environment but it could behave differently on larger DBs.

I should probably await the dbQuery as well before I fetch the ServerThreadInfo (in my opinion), so I can

  1. dbQuery(...)
  2. fetchServerThreadInfos
  3. createUpdates
keyserver/src/creators/role-creator.js
153 ↗(On Diff #28430)

Yeah, that sounds right to me! Good catch

ashoat added inline comments.
keyserver/src/creators/role-creator.js
138 ↗(On Diff #28485)

This seems like more of an internal_error to me. It should be surprising for us if we can't fetch the community now, since we are able to fetch it on line 85. It's possible that the community is deleted between these two calls, but in that case it's not really an issue with invalid parameters

Use internal_error instead of invalid_parameters