Add automatic row timestamps for Accounts and Groups tables #1674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 9577490268Details
💛 - Coveralls |
hectorgomezv
force-pushed
the
add-accounts-table-timestamps
branch
from
d75e2af
to
fc56d7c
Compare
Pull Request Test Coverage Report for Build 9577639417Details
💛 - Coveralls |
hectorgomezv
force-pushed
the
add-accounts-table-timestamps
branch
from
fc56d7c
to
a22412f
Compare
Pull Request Test Coverage Report for Build 9582299562Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9583178940Details
💛 - Coveralls |
Pull Request Test Coverage Report for Build 9583280310Details
💛 - Coveralls |
iamacook
reviewed
Jun 19, 2024
|
|
||
| await sql.end(); | ||
| it('should add and update row timestamps', async () => { |
There was a problem hiding this comment.
Nit: let's add another test that then ensures the created_at doesn't change but updated_at does after as the after callback combines all queries in one transaction.
There was a problem hiding this comment.
I've added a test in b1a9b0f
src/config/entities/configuration.ts
Outdated
| @@ -175,7 +175,7 @@ export default () => ({ | |||
| delegatesV2: process.env.FF_DELEGATES_V2?.toLowerCase() === 'true', | |||
| counterfactualBalances: | |||
| process.env.FF_COUNTERFACTUAL_BALANCES?.toLowerCase() === 'true', | |||
| accounts: process.env.FF_ACCOUNTS?.toLowerCase() === 'true', | |||
| accounts: true, | |||
There was a problem hiding this comment.
Was this an accidental commit?
There was a problem hiding this comment.
Yes 😓 I've fixed it in b1a9b0f
|
|
||
| export type Row = z.infer<typeof RowSchema>; | ||
|
|
||
| export const RowSchema = z.object({ |
There was a problem hiding this comment.
Nit: as we will be extending this for all tables, I would add some comments here as I've seen issues when using merge, e.g. outling that id is the primary key and caution should be taken when updating.
There was a problem hiding this comment.
I've added a comment in b1a9b0f
Please let me know if you want to rephrase it or add something else!
| @@ -51,7 +56,8 @@ export class PostgresDatabaseMigrator { | |||
| } | |||
|
|
|||
| /** | |||
| * @private migrates up to/allows for querying before/after migration to test it. | |||
| * Migrates up to/allows for querying before/after migration to test it. | |||
| * Uses generics to allow the caller to specify the return type of the before/after functions. | |||
Co-authored-by: Aaron Cook <aaron@safe.global>
Co-authored-by: Aaron Cook <aaron@safe.global>
Co-authored-by: Aaron Cook <aaron@safe.global>
Pull Request Test Coverage Report for Build 9584039461Details
💛 - Coveralls |
iamacook
approved these changes
Jun 19, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds
createdAtandupdatedAttimestamps toAccountsandGroupstables.createdAtis set to the current timestamp when a new row is created and it's not meant to be modified.updatedAtis set to the current timestamp when a new row is created and the idea is to modify it automatically using a trigger.The goal is to enhance these database tables' maintainability and auditing while improving debugging.
Changes
Rowas an entity meant to be extended by any persistence entity (id,created_at,updated_atare enforced).createdAtandupdatedAttimestamps toAccountsandGroupstables.updatedAton each row modification.PostgresDatabaseMigrator.testto make it generic, as the caller may need to specify the types returned to access their properties in the tests.