Page MenuHomePhabricator

[keyserver] Handle unpinning deleted messages
ClosedPublic

Authored by tomek on Fri, Apr 4, 8:06 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Apr 19, 11:33 AM
Unknown Object (File)
Thu, Apr 17, 12:01 PM
Unknown Object (File)
Tue, Apr 15, 1:40 PM
Unknown Object (File)
Tue, Apr 15, 1:54 AM
Unknown Object (File)
Mon, Apr 14, 1:46 PM
Unknown Object (File)
Mon, Apr 14, 4:39 AM
Unknown Object (File)
Fri, Apr 11, 3:24 PM
Unknown Object (File)
Fri, Apr 11, 3:42 AM
Subscribers
None

Details

Summary

This diff handles a case when a pinned message becomes deleted.

There are a couple of possible solutions, from which the simplest one would be to hide pins on the client - a one line change. The problem is that there's one place that doesn't respect the client logic - a banner with pin count. The count is a value stored on a keyserver in a thread table. So in order for everything to work correctly, we need a more complicated (and also more proper) solution where messages indeed get unpinned.

In this solution we simply call an already existing logic - toggling a pin. There are two modifications that we need: we don't want a message to be generated, and we shouldn't check permissions - because of that we're introducing a new behavior to the existing logic.

https://linear.app/comm/issue/ENG-10495/handle-unpinning-a-deleted-message

Depends on D14548

Test Plan

Pinned two messages, deleted one of them and checked that:

  1. A deleted message doesn't have a pin icon
  2. A banner shows one as a pin count
  3. A screen where pinned messages are shown, shows one message

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested review of this revision.Fri, Apr 4, 8:26 AM
ashoat added inline comments.
keyserver/src/updaters/thread-updaters.js
1027–1039 ↗(On Diff #47662)

We only need to fetch the RawThreadInfo for this check, right? I think we can wrap the whole thing in an async IIFE that gets short-circuited if behavior !== 'normal'

This revision is now accepted and ready to land.Fri, Apr 4, 9:00 AM

Optimize the code a bit

keyserver/src/updaters/thread-updaters.js
1027–1039 ↗(On Diff #47662)

We only need to fetch the RawThreadInfo for this check, right?

No, we're using fetchServerThreadInfosResult on line 1083 that is used for both behaviors. But we can extract a bit of logic into an if.