Script to initially populate policy_acknowledgments table assuming that all current users have already accepted the given policy.
IGNORE clause is for situations when the script will be executed twice to don't change history.
Details
Details
- run yarn script dist/scripts/populate-policies-acknowledgment.js
- script should add rows to policy_acknowledgments for all users for the given policy with current time and confirmed set to 1. (if the row with given user and policy do not exist).
- running twice should not changing anything
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Branch
- track-acknowledgments
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
| keyserver/src/scripts/populate-policies-acknowledgment.js | ||
|---|---|---|
| 1 | Nit: extra newline after // @flow to match the rest of the codebase | |
| 13–17 | Can you fix the spacing issues? Make it look like other queries in the codebase. In general, each line should not be indented more than one indentation past the previous line | |
| 15 | Personally I would just use policyTypes.tosAndPrivacyPolicy here | |
Comment Actions
Script to initially populate policy_acknowledgments table assuming that all current users have already accepted the given policy.
It is not clear for me why do we assume that all users accepted policy when it is inserted into table for the first time but since you wrote it in the diff summary then I assume it is explicitly required in corresponding Linear issue so I am not going to dig into that.
Separately I am of an opinion that it is better to implement this function in a way that it iterates over all policy types to execute appropriate SQL query.
Requesting changes to get it implemented or see explanation why approach above is better.
Comment Actions
It is not clear for me why do we assume that all users accepted policy when it is inserted into table for the first time but since you wrote it in the diff summary then I assume it is explicitly required in corresponding Linear issue so I am not going to dig into that.
To create an account, you have to accept the policies... so everybody with an account has already accepted them. The goal of the Linear task is to support changing the policies and then requiring everybody to re-accept the new policies.
Separately I am of an opinion that it is better to implement this function in a way that it iterates over all policy types to execute appropriate SQL query.
Requesting changes to get it implemented or see explanation why approach above is better.
Wouldn't hurt, but keep in mind that this is a simple one-time script, so code quality / thoroughness is not so important here.
| keyserver/src/scripts/populate-policies-acknowledgment.js | ||
|---|---|---|
| 14 | On one hand we intend this script to be one-time script but on the other we prevent it from terminating if we happen to run it twice. Wouldn't it be ok to actually let it terminate in case it is executed twice or at least provide some information for person executing this script that it was already executed? | |
| keyserver/src/scripts/populate-policies-acknowledgment.js | ||
|---|---|---|
| 14 | I'm confused... what happens if it's run twice? What prevents it from terminating? | |
| keyserver/src/scripts/populate-policies-acknowledgment.js | ||
|---|---|---|
| 14 |
What I believe @marcin's concerned about is the fact of using INSERT IGNORE instead of plain INSERT. The script is not intended to run twice but I tried to make it save even if someone will do it. | |
Comment Actions
Running this script manually seems like an unnecessary task - we can use our migrations to do it for us.
So I would propose the following ordering of operations:
- Introduce a migration that creates the table - done in previous diffs
- Introduce code that inserts values into the table when user signs up
- Add a migration that inserts values for all the registered users - this diff
- Change the logic to insert / update when user actually accepts (during login from login screen) instead of during the registration - later in the stack
This approach feels safer because:
- We don't rely on running manual action
- It is not possible for someone to deny the condition and be marked as accepted
The 2nd point is important, because when we run this script manually, we need to chose when to run it. If we do that before inserting into / updating the policy table after logout is in place, we could have users that accepted the terms but weren't marked as so (it isn't a huge issue but it's better to avoid it - they would need to accept again when we start requiring it). If we do it after we start marking terms as accepted after login from the login screen, we could have another issue: what if, before we run this script, a registered user decided to not login due to the terms? The user would get marked as accepted, when we run this script, because the account was created.
So depending on how important the issues sound to us, we can decide to use more complicated approach with migration, or keep the manual one. So it's probably up to @ashoat. (we can also decide to release the logic and the migration at once, which will be the simplest approach).
Comment Actions
The script is not intended to run twice but I tried to make it save even if someone will do it.
The second thing which in my opinion makes IGNORE is a case where those changes will be deployed and between deployment and running a script will be a time gap, and if new user will sign in within this gap there will be an entry in table and script will fail. I know it's not so possible with the current amount of users but why not make it easier for someone responsible for running it.
This sounds reasonable to me.