Details

Reviewers

ashoat
tomek

Commits

rCOMM1aad44e6fb68: [web] Introduce `web` migration to strip member permissions

Summary

Effectively the same as D12789, but for web.

Depends on D12789

Test Plan

Before (thin, with member permissions):

Screenshot 2024-10-08 at 4.24.03 PM.png (2×2 px, 581 KB)

Before (thick, with member permissions):

Screenshot 2024-10-08 at 4.24.24 PM.png (2×2 px, 598 KB)

After (thin, WITHOUT member permissions):

Screenshot 2024-10-08 at 4.25.16 PM.png (2×2 px, 648 KB)

After (thick, still WITH member permissions):

Screenshot 2024-10-08 at 4.25.38 PM.png (2×2 px, 639 KB)

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Jul 17 2024, 4:08 PM

Harbormaster completed remote builds in B30449: Diff 42430.Jul 17 2024, 4:25 PM

atul requested review of this revision.Jul 17 2024, 4:25 PM

ashoat accepted this revision.Jul 18 2024, 12:14 PM

ashoat added inline comments.

web/redux/persist.js
648–650 ↗	(On Diff #42430)	Same feedback here as in D12789

This revision is now accepted and ready to land.Jul 18 2024, 12:14 PM

rebase

Harbormaster completed remote builds in B30489: Diff 42491.Jul 18 2024, 1:26 PM

rebase

address feedback (any-cast stripMemberPermissionsFromRawThreadInfos instead of $FlowIgnore)

Harbormaster completed remote builds in B31990: Diff 44787.Oct 1 2024, 11:01 AM

Harbormaster completed remote builds in B31991: Diff 44788.Oct 1 2024, 11:12 AM

workaround that works for now (DRAFT)

cut extraneous changes

Harbormaster completed remote builds in B32003: Diff 44807.Oct 1 2024, 3:24 PM

Harbormaster completed remote builds in B32004: Diff 44808.

update migration to work with new migration system

Okay this migration now appears to work.

Would be great if others could test in their environments as well to make sure nothing breaks. Basically just need to patch this in and increment storeVersion to 84 and the migration should run. Can use https://phab.comm.dev/D13561 as reference.

Screenshot 2024-10-01 at 6.41.59 PM.png (460×1 px, 83 KB)

Harbormaster completed remote builds in B32005: Diff 44809.Oct 1 2024, 3:45 PM

Will make some changes before landing eg

Go from FlowFixMe to something like FlowIgnore
Remove logs related skipping ThickThreads

(Will be available noon onwards tomorrow to work on this)

rebase after type changes

This revision is now accepted and ready to land.Tue, Oct 8, 12:04 PM

remove console log and change flowfixme to flowignore

Harbormaster completed remote builds in B32104: Diff 44969.Tue, Oct 8, 12:22 PM

ashoat added inline comments.Tue, Oct 8, 12:22 PM

lib/utils/member-info-utils.js
48 ↗	(On Diff #44809)	Do we really need this log?
46 ↗	(On Diff #44970)	Can we avoid this if we update `ThreadStoreWithMemberPermissions` to include thick threads?
web/redux/persist.js
656 ↗	(On Diff #44970)	Can you cast the function to `MigrationFunction<WebNavInfo, AppState>` to match the other ones, and catch type errors like `ops: []` below?
663 ↗	(On Diff #44970)	Shouldn't this be `{}`?