Page MenuHomePhabricator

[keyserver] add script to force users to acknowledge updated policy
ClosedPublic

Authored by kamil on Oct 6 2022, 4:21 AM.
Tags
None
Referenced Files
F3497672: D5310.diff
Thu, Dec 19, 7:44 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: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

Details

Summary

The script is for the case where already present policy was updated and we need to force user to acknowledge it again.

Test Plan
  1. Run yarn script dist/scripts/force-policy-acknowledgment.js
  2. Check if in all policy_acknowledgments rows with given policy and confirmation date lower than the date when policy was published confirmed column was flipped to 0.

Diff Detail

Repository
rCOMM Comm
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.
keyserver/src/scripts/force-policy-acknowledgment.js
9 ↗(On Diff #17381)

This is needed for case when there is a gap between publishing policy and running script, if within this time range eg. a new user will signs in, we don't want to force him to do it again.
If this is too minor case and there is no possibility this can be removed.

kamil published this revision for review.Oct 6 2022, 4:50 AM
ashoat requested changes to this revision.Oct 6 2022, 6:15 AM

Looks good, but same three comments as in D5309

This revision now requires changes to proceed.Oct 6 2022, 6:15 AM
marcin requested changes to this revision.Oct 10 2022, 2:41 AM

As in D5309 - shouldn't we iterate?

keyserver/src/scripts/force-policy-acknowledgment.js
11 ↗(On Diff #17381)

I assume that as in D5309 this one is also intended to be one-time script so I am not going to pursue my idea of iterating over policies array. Nevertheless defining const policy = policyTypes.tosAndPrivacyPolicy (specific policy) and using it in function called forcePolicyAcknowledgment (suggests policies in general) doesn't seem right to me. We should either call this function forceTosAndPrivacyPolicyAcknowledgement or parameterize it with policy.

kamil edited the summary of this revision. (Show Details)

fix formating, add new line, remove const

keyserver/src/scripts/force-policy-acknowledgment.js
11 ↗(On Diff #17381)

I assume that as in D5309 this one is also intended to be one-time script so I am not going to pursue my idea of iterating over policies array. Nevertheless defining `

It's not a one-time script, it will be executed every time a policy will be updated and we will need another confirmation from users (I don't think all policies will be updated at once that's why there is no iterating).

const policy = policyTypes.tosAndPrivacyPolicy` (specific policy) and using it in function called forcePolicyAcknowledgment (suggests policies in general) doesn't seem right to me. We should either call this function forceTosAndPrivacyPolicyAcknowledgement or parameterize it with policy.

Looking through other scripts in keyserver I think this is the way of parametrization, by setting a const and then running a script.

Please address nits before landing

keyserver/src/scripts/force-policy-acknowledgment.js
13–15

Encourage you to match existing conventions as much as possible. Whenever you're writing something new, try to look for prior examples in the codebase and compare

Three nits here:

  1. Removed the semicolon, which we only use with multipleStatements: true
  2. Removed the indentation for SET and WHERE
  3. No line the codebase should exceed an 80 character width. Your third line currently exceeds this

Looks good, should be good to land once feedback is addressed

marcin added a reviewer: tomek.
marcin added inline comments.
keyserver/src/scripts/force-policy-acknowledgment.js
11 ↗(On Diff #17381)

Looking through other scripts in keyserver I think this is the way of parametrization, by setting a const and then running a script.

In such a case I should not block this revision on refactor.

This revision is now accepted and ready to land.Oct 14 2022, 9:12 AM
kamil added inline comments.
keyserver/src/scripts/force-policy-acknowledgment.js
13–15

Thanks! Will keep this in mind, my bad for not search trough the codebase deep enough to find patterns we should use.