Page MenuHomePhabricator

[keyserver] add migration to create `policy_acknowledgments` table
ClosedPublic

Authored by kamil on Oct 6 2022, 3:20 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Nov 10, 7:44 PM
Unknown Object (File)
Mon, Oct 28, 7:59 AM
Unknown Object (File)
Mon, Oct 28, 7:59 AM
Unknown Object (File)
Mon, Oct 28, 7:59 AM
Unknown Object (File)
Mon, Oct 28, 7:59 AM
Unknown Object (File)
Mon, Oct 28, 7:59 AM
Unknown Object (File)
Mon, Oct 28, 7:58 AM
Unknown Object (File)
Mon, Oct 28, 7:56 AM

Details

Summary

Add the table to track user acknowledgments.

Table structure was described in ENG-1868.

Table using only primary key with two columns - (user, policy) and user is in first place on purpose.
A lot of queries will be performed on this table based on the user id. InnoDB by default will use BTREE as indexing algorithm which works fine for this approach (keep rows sorted based on user ID and should be decent).
I also did a test with added additional index only for user column a perform some queries on table with ~900 rows and ~400 users, measured times were almost equal on both approaches so I think there is no need for another index (might waster some resources) and primary key with pair (user, policy) should be sufficient.

Test Plan
  1. Restart the keyserver
  2. Check in metadata table if the database version was incremented.
  3. Check if policy_acknowledgments was created with proper structure, screenshot with table:

image.png (364×835 px, 40 KB)

Diff Detail

Repository
rCOMM Comm
Branch
track-acknowledgments
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil edited the test plan for this revision. (Show Details)
kamil added a reviewer: atul.
kamil published this revision for review.Oct 6 2022, 4:50 AM

Please address nit before landing, and please don't land without corresponding change to setup-db.js

keyserver/src/database/migration-config.js
42–49

Nit for multiline queries

This revision is now accepted and ready to land.Oct 6 2022, 6:12 AM
marcin requested changes to this revision.Oct 10 2022, 2:28 AM
marcin added a subscriber: tomek.
marcin added inline comments.
keyserver/src/database/migration-config.js
42

This SQL creates correct table but I have got a two nits.

  1. We need bigint for date since int has up to 10 digits and current unix time stamp (in milliseconds is 13 digit). But I think int would suffice for user and has 2 times less bytes.
  2. For policy table - are we sure 255 is enough and all characters will be UTF-8 valid?

Those are nits, but requesting changes since I want them answered before I accept this revision and pass it to @tomek

This revision now requires changes to proceed.Oct 10 2022, 2:28 AM
keyserver/src/database/migration-config.js
42

We need bigint for date since int has up to 10 digits and current unix time stamp (in milliseconds is 13 digit). But I think int would suffice for user and has 2 times less bytes.

Please see the users table... the id column is defined as id bigint(20) NOT NULL, so we should have the same here.

For policy table - are we sure 255 is enough and all characters will be UTF-8 valid?

I think so, this is just some string constant

marcin added a reviewer: tomek.
This revision now requires review to proceed.Oct 11 2022, 11:34 AM

thanks for responding to comments @ashoat!

tomek added inline comments.
keyserver/src/database/migration-config.js
46 ↗(On Diff #17505)

Why is the date not null? In https://linear.app/comm/issue/ENG-1868/create-new-table-to-track-whether-policies-have-been-acknowledged#comment-fbea223c it is said that

when one of the policies will be updated, eg. terms and privacy we will flip the confirmed field to 0 or date to NULL and wait for the user to accept

So it seems like date should be nullable

47 ↗(On Diff #17505)

It feels like we can consider adding an index over confirmed because we will probably query based on this

This revision is now accepted and ready to land.Oct 12 2022, 3:45 AM
keyserver/src/database/migration-config.js
46 ↗(On Diff #17505)

I guess the thinking is that;

  1. If the user has never accepted policies (before we run second migration described by @tomek here), then there is no row for them
  2. If the user has accepted policies, but we change them by running the script in D5310, then the date will have the time of the old acceptance, but confirmed will be 0
    • If we want, we could consider updating that diff to also set date to NULL, but I'm not sure how important that is
  3. After the user has accepted the new policy, date will be updated to the time of the latest accept

Open to changing this but I think that's the reason for the current design.

keyserver/src/database/migration-config.js
46 ↗(On Diff #17505)

Thanks for clarifying! That's exactly what I had in mind while implmenting

47 ↗(On Diff #17505)

For now, I think we will query only based on user and policy so it's probably better not to add another index to save some resources

rebase, update migration number