Page MenuHomePhabricator

[lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`
ClosedPublic

Authored by atul on Jun 1 2024, 9:21 PM.
Tags
None
Referenced Files
F3353089: D12265.diff
Sat, Nov 23, 8:06 AM
Unknown Object (File)
Thu, Nov 14, 5:08 PM
Unknown Object (File)
Thu, Nov 14, 3:12 PM
Unknown Object (File)
Wed, Nov 13, 12:04 PM
Unknown Object (File)
Wed, Nov 13, 11:52 AM
Unknown Object (File)
Wed, Nov 13, 7:01 AM
Unknown Object (File)
Wed, Nov 13, 5:52 AM
Unknown Object (File)
Wed, Nov 13, 2:45 AM
Subscribers
None

Details

Summary

We're no longer using member permissions anywhere on client (or anywhere ThreadInfo is used which includes eg send.js), so it can be removed.

NOTE: Marked this as DRAFT because I think we can do better than the invariant in memberHasAdminRole, but I think it would require some flow refactoring which I want to defer for now until I get rest of migration stack up.

Depends on D12259

Test Plan

flow

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/types/minimally-encoded-thread-permissions-types.js
134–141 ↗(On Diff #40822)

Rather than deriving this from MemberInfo which is then derived from LegacyMemberInfo, I think it requires less jumping between files to just have it all inline like this. Open to different approach if it would be preferred.

atul requested review of this revision.Jun 1 2024, 9:38 PM
atul planned changes to this revision.Jun 1 2024, 10:07 PM

Agree we can do better than the invariant. It seems like memberHasAdminPowers should not be called with RelativeMemberInfo anymore, since permissions is missing there. Are there current callsites that pass RelativeMemberInfo into memberHasAdminPowers?

lib/types/minimally-encoded-thread-permissions-types.js
134–141 ↗(On Diff #40822)

I don't have a strong opinion

lib/permissions/minimally-encoded-thread-permissions-validators.js
17–26 ↗(On Diff #40822)

NOTE TO SELF: Make sure that changes to validators in this stack (or higher level validators which consume them) do not break anything for older clients, etc.

atul requested review of this revision.Jun 11 2024, 1:05 PM
atul retitled this revision from [DRAFT][lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions` to [lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`.

Agree we can do better than the invariant.

invariant gets removed in D12292

Please make sure to consider your "note to self" above before landing:

NOTE TO SELF: Make sure that changes to validators in this stack (or higher level validators which consume them) do not break anything for older clients, etc.

lib/types/minimally-encoded-thread-permissions-types.js
134 ↗(On Diff #40822)

Now that we no longer have a type spread, we can remove the $ReadOnly wrapper

This revision is now accepted and ready to land.Jun 12 2024, 8:49 AM

rebase BEFORE addressing feedback and landing

address feedback (remove $ReadOnly wrapper)

remove relativeMemberInfoValidator export

NOTE TO SELF: Make sure that changes to validators in this stack (or higher level validators which consume them) do not break anything for older clients, etc.

There is one use of relativeMemberInfoValidator and it's in threadInfoValidator.

threadInfoValidator is used in

  • thread-utils.test.js: threadInfoFromRawThreadInfo should return correctly formed ThreadInfo from RawThreadInfo test where things remain working as expected.
  • webNavInfoValidator

webNavInfoValidator is only consumed by initialReduxStateValidator which validates ServerWebInitialReduxStateResponse.

webNavInfoValidator is of type TInterface<WebNavInfo> and threadInfoValidator is consumed within to validate pendingThread:

...
pendingThread: t.maybe(threadInfoValidator),
...

My understanding is the WebNavInfo.pendingThread is a client concept and can only exist for the client representation of WebNavInfo and can't be included in response from keyserver since they aren't persisted or stored anywhere.

To double check this, looked at getInitialReduxStateResponder which calls navInfoFromURL which takes in urlInfo and backupInfo and returns WebNavInfo. Nowhere in navInfoFromURL is a pendingThread field included:

8c85fe.png (1×2 px, 331 KB)

As a result, I think this diff is safe to land. Please let me know if there's something I'm missing/misunderstanding and I will revert.