This diff addresses ENG-9330.
Some complexity was necessary to allow us to fetch device lists for the new user prior to sending the message. More context on that complexity in this comment.
Depends on D13441
Details
Details
I tested all three cases of RobotextPlanForUser. I tested on both iOS and Android, with two clients talking to each other. I sent a friend request, accepted it, then removed the relationship and did the flow again.
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| lib/hooks/relationship-hooks.js | ||
|---|---|---|
| 89 ↗ | (On Diff #44476) | We should also include viewerID here so that other viewer's devices also receive this operation. |
| 108 ↗ | (On Diff #44476) | It would be more readable to name it as an action |
| 133 ↗ | (On Diff #44476) | We should also update getThickThreadRolePermissionsBlob to reflect the permissions from thin threads so that e.g. a user can't leave a PERSONAL thread. Created https://linear.app/comm/issue/ENG-9382/update-personal-and-private-threads-permissions to track because it also affects https://linear.app/comm/issue/ENG-9379/use-private-instead-of-genesis-private-on-registration. |
| 207–229 ↗ | (On Diff #44476) | We can consider merging these two blocks |
| 266–268 ↗ | (On Diff #44476) | Do we still want to return when only some users don't support thick threads? |
| 292–293 ↗ | (On Diff #44476) | This looks a bit fragile. Can we somehow encode it so that after breaking this assumption we don't have to remember about this place? |
| lib/hooks/relationship-hooks.js | ||
|---|---|---|
| 89 ↗ | (On Diff #44476) | Good catch! |
| 133 ↗ | (On Diff #44476) | Thanks for creating the task and handling it! |
| 266–268 ↗ | (On Diff #44476) | Yes – this is safe because on line 319, we make sure to only wait for users for whom identity returned a device list. Since the request returned a device list, we are confident that after the Redux action is reduced, the device lists should be populated in AuxUserStore I'll add a code comment clarifying this |
| 292–293 ↗ | (On Diff #44476) | I looked into doing this with types, but it's a bit messy. I'll put it up as a separate diff to get your perspective on whether it's worth it. For now, in this diff I'll add a comment to the keyserver code to encourage people to check this code if they're changing it |
| lib/hooks/relationship-hooks.js | ||
|---|---|---|
| 207–229 ↗ | (On Diff #44476) | I didn't do this... to be honest, I think the code is cleaner with the two blocks separated. That's why I factored out sendRobotextToThickThread |