[keyserver] Call canToggleMessagePin in toggleMessagePinForThread
ClosedPublic
Actions

Authored by rohan on Oct 31 2023, 7:42 AM.

Details

Reviewers

ashoat
atul
ginsu

Commits

rCOMM94abca6395fb: [keyserver] Call canToggleMessagePin in toggleMessagePinForThread

Summary

This is some feedback from D9638:

Still, it might be a good idea to add this to toggleMessagePinForThread. Besides the benefits of consistently checking the same condition in as many places as possible, you also get a bonus for moving the fetchServerThreadInfos call earlier. I think that lets you replace the isInvalidPinSource check there with canToggleMessagePin, and drop the checkThreadPermission entirely. (You'd need to update the types on canToggleMessagePin I think.)

While the fact that we won't benefit from the additional check is true (I confirmed this as well), we do get to benefit from a few of the things that @ashoat mentioned.

Resolves https://linear.app/comm/issue/ENG-5628/call-cantogglemessagepin-in-togglemessagepinforthread

Depends on D9638

Test Plan

Made sure that pinning / unpinning messages still worked on the server.

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Oct 31 2023, 7:42 AM

Herald added subscribers: will, tomek. · View Herald TranscriptOct 31 2023, 7:42 AM

rohan added inline comments.Oct 31 2023, 7:47 AM

lib/utils/toggle-pin-utils.js
13–18 ↗	(On Diff #32547)	Wasn't really sure the best way to type this. I thought this read a little weird: function canToggleMessagePin( messageInfo: ComposableMessageInfo \| RobotextMessageInfo \| RawMessageInfo, threadInfo: RawThreadInfo \| ThreadInfo, ): boolean { ... } So I thought having a distinction between the expected message types when called from the client vs. the server would be useful? I don't really have a strong opinion on either way to do this. To note: all of the callsites for the client will provide a `messageInfo: ComposableMessageInfo \| RobotextMessageInfo` (I think this stems from ChatMessageInfoItem ) while the callsite from the server will just provide a `RawMessageInfo`.

rohan edited the summary of this revision. (Show Details)Oct 31 2023, 7:49 AM

Harbormaster completed remote builds in B23604: Diff 32547.Oct 31 2023, 8:06 AM

rohan requested review of this revision.Oct 31 2023, 8:06 AM

ashoat accepted this revision.Nov 1 2023, 7:28 AM

ashoat added inline comments.

lib/utils/toggle-pin-utils.js
13–18 ↗	(On Diff #32547)	I'm okay with this. You could also do `RawMessageInfo \| MessageInfo`... might be more simple, and while it's true that not all `MessageInfo` will be passed in, it doesn't hurt to make the function more "general"