Page MenuHomePhabricator

[lib] implement `KeyserverStore` spec
ClosedPublic

Authored by kamil on Jan 22 2024, 2:41 AM.
Tags
None
Referenced Files
F3281684: D10755.id35910.diff
Sat, Nov 16, 9:52 AM
Unknown Object (File)
Wed, Nov 6, 6:56 PM
Unknown Object (File)
Oct 15 2024, 10:47 AM
Unknown Object (File)
Oct 15 2024, 10:47 AM
Unknown Object (File)
Oct 15 2024, 10:47 AM
Unknown Object (File)
Oct 15 2024, 10:47 AM
Unknown Object (File)
Oct 15 2024, 10:46 AM
Unknown Object (File)
Sep 28 2024, 3:26 PM

Details

Summary

Add code with required ops.

Depends on D10754

Test Plan

Flow, functionality tested later in the stack.

Diff Detail

Repository
rCOMM Comm
Branch
publish-ks-sql-2
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Jan 22 2024, 6:32 AM
lib/ops/keyserver-store-ops.js
20–21 ↗(On Diff #35910)

Typically we have a remove action that removes a subset, with a payload that looks like this: +payload: { +ids: $ReadOnlyArray<string> },. Can we do the same here?
This will be useful for example for delete account action, that returns an array of keyserver ids to be removed.

lib/ops/keyserver-store-ops.js
79 ↗(On Diff #35910)

Can we take the whole store here? It seems odd that we call this "process store operations", but only process a part of the store.

Couple nits

lib/ops/keyserver-store-ops.js
16 ↗(On Diff #35910)

This payload and the next are not read-only

53–54 ↗(On Diff #35910)

This input obj should be read-only

  • make fields read-only
  • use array in remove operation
kamil added inline comments.
lib/ops/keyserver-store-ops.js
20–21 ↗(On Diff #35910)

I was introducing +payload: { +ids: $ReadOnlyArray<string> }, in other stores because it was a use case for this. In this reducer, we will either remove only one keyserver or remove all (we have a separate operation for it).

But since it was also requested by @marcin in D10775 I can refactor this.

79 ↗(On Diff #35910)

We are doing the same for ReportStore and UserStore, so I think it's fine. If you insist I can update this but I would prefer to do it in a separate diff or D10773 (I know it's a bad convention but if I change this here I will have to update all diffs in the stack, so making this change in the separate diff or the end of the stack will save lot of my time)

This revision is now accepted and ready to land.Jan 29 2024, 1:43 AM
This revision was automatically updated to reflect the committed changes.