Page MenuHomePhabricator

[keyserver] Delete contained chats when container chat is deleted
ClosedPublic

Authored by patryk on Jul 7 2023, 2:39 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Nov 23, 9:56 AM
Unknown Object (File)
Tue, Nov 19, 6:26 AM
Unknown Object (File)
Tue, Nov 19, 6:26 AM
Unknown Object (File)
Tue, Nov 19, 6:26 AM
Unknown Object (File)
Tue, Nov 19, 6:25 AM
Unknown Object (File)
Tue, Nov 19, 6:19 AM
Unknown Object (File)
Mon, Nov 18, 8:18 AM
Unknown Object (File)
Oct 18 2024, 9:01 AM
Subscribers

Details

Summary
Test Plan
  1. Run web app
  2. Create subchannel with some threads (sidebars)
  3. Check if threads are searchable
  4. Delete subchannel and check if threads are still searchable

Diff Detail

Repository
rCOMM Comm
Branch
arcpatch-D8436
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

ashoat requested changes to this revision.Jul 7 2023, 11:46 AM

Great work! Mostly nits inline

keyserver/src/deleters/thread-deleters.js
60 ↗(On Diff #28465)

Let's delete this line

65 ↗(On Diff #28465)

serverThreadInfo is an array. We typically try to use plurals for arrays. The English word "information" is technically both singular and plural, but to disambiguate in the code we generally try to use the suffix "Infos"

93 ↗(On Diff #28465)

The convention in the codebase is to omit semicolons at the end of SQL statements unless using multipleStatements: true

It's best to try to match conventions as much as possible, rather than changing things to match your preference. Please try to identify conventions on your own, and try to match the style in the codebase as much as possible!

98 ↗(On Diff #28465)

Nit: convention in the code is to use capital letter for the second letter of ID

keyserver/src/fetchers/thread-fetchers.js
312 ↗(On Diff #28465)

Nit

313 ↗(On Diff #28465)

I don't think we need this condition here, as it doesn't appear to be used in your use case. It's best to keep things as simple as possible, and avoid "overengineering" for potential future use cases. If we need this in the future, it should be really easy to reintroduce it

332 ↗(On Diff #28465)

I don't think this is necessary. We typically let DB errors surface directly... they're helpful to see in the keyserver log because of this line. Meanwhile, this line makes sure that we don't surface anything weird to the API caller

This revision now requires changes to proceed.Jul 7 2023, 11:46 AM
This revision is now accepted and ready to land.Jul 10 2023, 7:29 AM