Page MenuHomePhabricator

[lib] Add `skipMemberAdminRoleCheck` argument to `threadIsWithBlockedUserOnly`
ClosedPublic

Authored by atul on May 21 2024, 8:27 AM.
Tags
None
Referenced Files
Unknown Object (File)
Fri, Oct 18, 11:58 PM
Unknown Object (File)
Fri, Oct 18, 8:15 AM
Unknown Object (File)
Fri, Oct 18, 1:49 AM
Unknown Object (File)
Thu, Oct 17, 1:12 PM
Unknown Object (File)
Thu, Oct 17, 12:40 PM
Unknown Object (File)
Thu, Oct 17, 12:35 PM
Unknown Object (File)
Sat, Oct 12, 4:49 AM
Unknown Object (File)
Fri, Oct 11, 4:36 PM
Subscribers
None

Details

Summary

We want this specifically for useThreadsWithPermission where we're doing member admin role check the "updated way." We handle the early return condition previously handled by threadOrParentThreadHasAdminRole using community root role check.

Considered a few other options which included

  • turning threadIsWithBlockedUserOnly into hook (would be tedious because we'd need to "hook-ify" a couple other functions and then have variants which accept collection of items vs. single item because hooks can't be called in loop, etc.)
  • creating duplicate/higher order functions similar to threadFrozenDueToBlock/threadFrozenDueToViewerBlock but the call stack would get more complex than needed imo

Depends on D12150

Test Plan

Will add log statements to make sure behavior is as expected. Specifically with native client connected to prod store since after MariaDB update my local DB is sparse.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

atul added inline comments.
lib/shared/thread-utils.js
1188–1189 ↗(On Diff #40484)

We could create some type of options argument that's an object, but I think that this is fine if there are just two of these booleans and the callsites are fairly limited?

At least with VSCode, WebStorm, etc there are inlay hints that make things more clear eg:

00b489.png (348×818 px, 53 KB)

But can go ahead and add options obj if preferred.

atul published this revision for review.May 21 2024, 9:03 AM

i'm probably not a good reviewer of this code

ashoat requested changes to this revision.May 24 2024, 3:03 AM
ashoat added inline comments.
lib/shared/thread-utils.js
1167 ↗(On Diff #40484)

Can we avoid exposing this outside of this file?

  1. Rename this to innerThreadFrozenDueToBlock
  2. Introduce new threadFrozenDueToBlock that just calls innerThreadFrozenDueToBlock with false
1188–1189 ↗(On Diff #40484)

Yeah, can you please do this? Personally I think we should "bag of params" when we have so many params and several of them are booleans

This revision now requires changes to proceed.May 24 2024, 3:03 AM

rebase before addressing feedback

partially address feedback

  • new threadFrozenDueToBlock that calls innerThreadFrozenDueToBlock with skipMemberAdminRoleCheck set to false for callsites outside of file
  • create some sort of options object for threadIsWithBlockedUserOnly

"create some sort of options object for threadIsWithBlockedUserOnly"

ashoat added inline comments.
lib/shared/thread-utils.js
881–882 ↗(On Diff #40797)

Read-only

This revision is now accepted and ready to land.May 31 2024, 4:05 AM
lib/shared/thread-utils.js
881–882 ↗(On Diff #40797)

Oops, cut/paste from function argument below and missed adding +. Will update diff.

make entries of ThreadIsWithBlockedUserOnlyOptions read-only

This revision was landed with ongoing or failed builds.May 31 2024, 11:44 AM
This revision was automatically updated to reflect the committed changes.