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, Jun 21, 5:28 AM
Unknown Object (File)
Fri, Jun 21, 5:28 AM
Unknown Object (File)
Fri, Jun 21, 5:28 AM
Unknown Object (File)
Thu, Jun 20, 4:47 AM
Unknown Object (File)
Sun, Jun 16, 5:42 PM
Unknown Object (File)
Sun, Jun 16, 3:09 PM
Unknown Object (File)
Wed, Jun 12, 12:03 PM
Unknown Object (File)
Wed, Jun 12, 11:25 AM
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
Lint Not Applicable
Unit
Tests Not Applicable

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.Fri, May 31, 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.Fri, May 31, 11:44 AM
This revision was automatically updated to reflect the committed changes.