Details

Reviewers

atul
marcin
ashoat
tomek

Commits

rCOMM328b5c5d0af4: [keyserver] add logic to update acknowledgment table

Summary

The function should be called after detecting login from a proper source.

Functionality:

a. if there is no record with the given user and policy it means that this is a new one - so It adds a new row
b. if there is a record with a given user and policy but confirmed is set to 0 it means that policy was updated, so it updated confirmed and date
c. in any other case do nothing

Test Plan

Check if described in Summary logic works for:

new policy
updated policy
policy accepted by user previously

Diff Detail

Repository

rCOMM Comm

Branch

track-acknowledgments

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

kamil created this revision.Oct 6 2022, 4:33 AM

kamil held this revision as a draft.

Herald added subscribers: • abosh, atul, ashoat. · View Herald TranscriptOct 6 2022, 4:33 AM

kamil edited the summary of this revision. (Show Details)Oct 6 2022, 4:40 AM

kamil edited the test plan for this revision. (Show Details)

kamil added a reviewer: atul.

Harbormaster completed remote builds in B12651: Diff 17382.Oct 6 2022, 4:40 AM

kamil added a parent revision: D5310: [keyserver] add script to force users to acknowledge updated policy.Oct 6 2022, 4:40 AM

kamil added a child revision: D5312: [keyserver] update acknowledgment table in login was performed from proper source.Oct 6 2022, 4:48 AM

kamil published this revision for review.Oct 6 2022, 4:51 AM

tomek edited reviewers, added: marcin; removed: tomek.Oct 6 2022, 5:31 AM

Clean up spacing / indentation in queries please!

keyserver/src/updaters/user-acknowledgment-updater.js
1	Newline after `// @flow`
20	I think this would be cleaner with `policies.map`. What do you think?
23–41	We can just use a single query with the `ON DUPLICATE KEY` construction

This revision now requires changes to proceed.Oct 6 2022, 6:17 AM

keyserver/src/updaters/user-acknowledgment-updater.js
13	We could `ORDER BY` policy here and sort policies table. Then we could just iterate over either policies table or userPolicies and use the same index to find corresponding entry in another table (no need to call `find` later). However I am not sure how much optimization is needed for this script or how much optimization would this change introduce. Therefore I am not going to block revision on this change, but please answer it before landing.

use ON DUPLICATE KEY, use map, add newline

kamil added inline comments.Oct 12 2022, 3:17 AM

keyserver/src/updaters/user-acknowledgment-updater.js
13	We could `ORDER BY` policy here and sort policies table. Then we could just iterate over either policies table or userPolicies and use the same index to find corresponding entry in another table (no need to call `find` later). However I am not sure how much optimization is needed for this script or how much optimization would this change introduce. Therefore I am not going to block revision on this change, but please answer it before landing. Thanks for the suggestion, I don't think we will gain a lot from this solution, I expect a lot of users (which means a lot of entries in `policy_acknowledgments`) and a relatively small number of policies, so I believe using `find` on a small array will be still faster than calling `ORDER BY` on the table with a lot of entries (the table do not have a separate index for `policy` and because it is using BTREE will be fast for sorting only by tuple `(user, policy)` and leftmost prefix).

Harbormaster completed remote builds in B12747: Diff 17509.Oct 12 2022, 3:20 AM

tomek requested changes to this revision.Oct 12 2022, 5:40 AM

tomek added inline comments.

keyserver/src/updaters/user-acknowledgment-updater.js
22–24 ↗	(On Diff #17509)	When `!confirmed` we're returning so maybe we can avoid selecting it in the first place? Also, we're interested only with policies from `policies` array - it also can be used in a select where condition.
27–32 ↗	(On Diff #17509)	We don't expect having too many policies, so it's fine as it is, but there are some optimization which could be done if we had more of them: For every policy we have a separate query - it is expensive and we could use an insert with multiple values https://www.mariadbtutorial.com/mariadb-basics/mariadb-insert-multiple-rows/ We're computing some things on JS side, but it feels like this whole operation can be made using a single query which selects and inserts... but that query would be a lot more complicated. Additional benefit would be that the query would be a single operation on the db, so we would avoid a scenario in which content of the table is updated between select and insert.

This revision now requires changes to proceed.Oct 12 2022, 5:40 AM

ashoat requested changes to this revision.Oct 12 2022, 8:18 AM

ashoat added inline comments.

keyserver/src/updaters/user-acknowledgment-updater.js
8 ↗	(On Diff #17509)	Something I just realized here... The idea of having separate kinds of policies in the table is meant to reflect policies appearing in different places in the app. The idea would be that we might have Terms when you log in, and maybe a separate set of Terms for some other feature. For that reason I think we should require the caller here to specify a specific `policy`. This will also simplify the code. Sorry I didn't call this out sooner!
27 ↗	(On Diff #17509)	Can you apply the nits from D5310 here too?

rebase, force caller to specify policy type

kamil added a parent revision: D5663: [keyserver] add migration to populate `policy_acknowledgments` table.Nov 17 2022, 3:12 AM

remove semicolon

kamil edited the summary of this revision. (Show Details)Nov 17 2022, 3:15 AM

kamil edited the test plan for this revision. (Show Details)

Harbormaster completed remote builds in B13511: Diff 18520.Nov 17 2022, 3:24 AM

kamil removed a parent revision: D5310: [keyserver] add script to force users to acknowledge updated policy.Nov 17 2022, 3:25 AM

Harbormaster completed remote builds in B13512: Diff 18521.Nov 17 2022, 3:27 AM

tomek accepted this revision.Nov 17 2022, 4:46 AM

tomek added inline comments.

keyserver/src/updaters/user-acknowledgment-updater.js
8 ↗	(On Diff #18521)	We can be more precise with the name. In most of the places `viewer` updates some info about `user`: they might be the same person, but doesn't have to. To avoid confusion with other places, we can change function's name to indicate that it's viewer who gets updated. Other option is to change this method to allow updating any user, but that brings the question about permissions: who can update whom... I don't think it is a good idea to focus on this now.