Page MenuHomePhabricator

[keyserver] add viewer acknowledgment fetcher
ClosedPublic

Authored by kamil on Dec 6 2022, 6:02 AM.
Tags
None
Referenced Files
F3489892: D5822.diff
Wed, Dec 18, 2:04 PM
Unknown Object (File)
Sun, Dec 15, 8:30 PM
Unknown Object (File)
Sun, Dec 15, 8:30 PM
Unknown Object (File)
Sun, Dec 15, 8:30 PM
Unknown Object (File)
Sun, Dec 15, 8:29 PM
Unknown Object (File)
Sun, Dec 15, 8:20 PM
Unknown Object (File)
Thu, Dec 5, 11:58 PM
Unknown Object (File)
Thu, Nov 28, 11:53 PM
Subscribers

Details

Summary

Function fetches viewer policies with confirmed value

Test Plan

Check if returns proper value for one/multiple policies

Diff Detail

Repository
rCOMM Comm
Branch
policy-stuff-5
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Dec 6 2022, 6:55 AM
tomek requested changes to this revision.Dec 6 2022, 9:23 AM
tomek added inline comments.
keyserver/src/fetchers/policy-acknowledgment-fetchers.js
13 ↗(On Diff #19175)

Do we have an index that will help us selecting it?

13–15 ↗(On Diff #19175)

Why do we call append multiple times? I think it would be better to just have a multiline string. The only thing that needs append is mergeOrConditions.

16 ↗(On Diff #19175)

Instead of using OR we can have IN condition. Usually IN is more readable and faster (might depend on the exact use case, db engine, etc.)

lib/types/policy-types.js
5–8 ↗(On Diff #19175)

We should use readonly where possible. Also the name is singular so it isn't a good idea to make a type an array.

This revision now requires changes to proceed.Dec 6 2022, 9:23 AM

improve query, make types read-only

keyserver/src/fetchers/policy-acknowledgment-fetchers.js
13 ↗(On Diff #19175)

we have a primary key consisting user and policy columns so I think it should be okay

16 ↗(On Diff #19175)

Good point, I missed that, thanks!

This revision is now accepted and ready to land.Dec 13 2022, 5:26 AM