Page MenuHomePhabricator
Differential D12265

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

Authored by atul on Sat, Jun 1, 9:21 PM.
Tags
None
Referenced Files
F2017775: 8c85fe.png
Sat, Jun 15, 4:24 PM
Unknown Object (File)
Thu, Jun 13, 3:33 PM
Unknown Object (File)
Wed, Jun 12, 1:11 PM
Unknown Object (File)
Tue, Jun 11, 9:25 AM
Unknown Object (File)
Fri, Jun 7, 10:43 PM
Unknown Object (File)
Thu, Jun 6, 12:55 PM
Unknown Object (File)
Tue, Jun 4, 10:45 AM
Unknown Object (File)
Sat, Jun 1, 9:54 PM
View All 10 Files
Subscribers
None

Details

Reviewers
ashoat
ginsu
tomek
varun
Commits
rCOMM56ba0de13be0: [lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`
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

atul created this revision.Sat, Jun 1, 9:21 PM
atul added inline comments.Sat, Jun 1, 9:32 PM
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.

Harbormaster completed remote builds in B29342: Diff 40822.Sat, Jun 1, 9:38 PM
atul requested review of this revision.Sat, Jun 1, 9:38 PM
atul added a child revision: D12266: [lib] Narrow `threadInfo` argument of `threadFrozenDueToViewerBlock`.Sat, Jun 1, 10:00 PM
atul planned changes to this revision.Sat, Jun 1, 10:07 PM
ashoat added a comment.Sun, Jun 2, 1:58 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

atul added a child revision: D12287: [lib] Introduce `useThreadFrozenDueToViewersBlock`.Mon, Jun 3, 12:40 PM
atul mentioned this in D12292: [lib] Narrow types in `memberHasAdminPowers` call stack to remove `invariant`.Mon, Jun 3, 1:43 PM
atul added inline comments.Mon, Jun 3, 2:24 PM
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.Tue, Jun 11, 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

ashoat accepted this revision.Wed, Jun 12, 8:49 AM

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.Wed, Jun 12, 8:49 AM
atul updated this revision to Diff 41258.Wed, Jun 12, 3:25 PM

rebase BEFORE addressing feedback and landing

Harbormaster completed remote builds in B29635: Diff 41258.Wed, Jun 12, 3:42 PM
atul updated this revision to Diff 41347.Sat, Jun 15, 2:26 PM

address feedback (remove $ReadOnly wrapper)

Harbormaster completed remote builds in B29694: Diff 41347.Sat, Jun 15, 2:43 PM
atul updated this revision to Diff 41348.Sat, Jun 15, 3:13 PM

remove relativeMemberInfoValidator export

Harbormaster completed remote builds in B29695: Diff 41348.Sat, Jun 15, 3:30 PM
atul added a comment.Sat, Jun 15, 4:24 PM

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.

Closed by commit rCOMM56ba0de13be0: [lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions` (authored by atul). · Explain WhySat, Jun 15, 6:17 PM
This revision was automatically updated to reflect the committed changes.
atul added a commit: rCOMM56ba0de13be0: [lib] Remove `permissions` field from `ThreadInfo.members[memberID].permissions`.
atul mentioned this in rCOMM509ef589a60d: [lib] Narrow types in `memberHasAdminPowers` call stack to remove `invariant`.Sun, Jun 16, 5:50 PM

Revision Contents
Changeset List

Diff 41349

View Options

lib/permissions/minimally-encoded-thread-permissions-validators.js

Loading...
View Options

lib/selectors/user-selectors.js

Loading...
View Options

lib/shared/thread-utils.js

Loading...
View Options

lib/types/minimally-encoded-thread-permissions-types.js

Loading...