Convert users and variables types to knex #3145
Merged
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
This was referenced Jan 29, 2024
|
Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
|
Important Auto Review SkippedDraft detected. Please check the settings in the CodeRabbit UI or the To trigger a single review, invoke the Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (invoked as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
danyx23
force-pushed
the
db-types-migrate-variables-and-users
branch
2 times, most recently
from
fdb0dbc to
8a389c2
Compare
|
This PR has had no activity within the last two weeks. It is considered stale and will be closed in 3 days if no further activity is detected. |
mlbrgl
added a commit
that referenced
this pull request
Feb 19, 2024
- pass knex as a parameter to downstream functions
- wrap top API calls in a transaction
- replace knex.raw() with knexRaw()
- this PR doesn't add [{readOnly: true}](https://www.notion.so/owid/Database-access-in-Grapher-d9db343c2bfb4ae0b14b3dec72f686c6?pvs=4#fdb6c225d4134e47a78fe3436a19f72c).
@danyx23 [as you know](#3145), these refactors tend to start with a small surface area but end up requiring extensive surgery across the codebase.
This is partly why I hadn't dived into it as part of this project, and continued using the `db.knexInstance().raw()` paradigm in downstream functions. (Another reason had to do with the value of doing this refactor on partly obsolete code).
So in case this ends up needing more work and blocks the delivery of the core wordpress sunsetting project, I'll revert it to draft for later.
How does that sound?
Context: #3166 (comment)
danyx23
force-pushed
the
db-types-migrate-variables-and-users
branch
2 times, most recently
from
c72f4cb to
35e1788
Compare
sophiamersmann
approved these changes
Feb 27, 2024
db/model/Dataset.ts
Outdated
| `SELECT t.id, t.name FROM dataset_tags dt JOIN tags t ON t.id=dt.tagId WHERE dt.datasetId=?`, | ||
| db.knexInstance(), // TODO: pass down the knex instance instead of using the global one |
There was a problem hiding this comment.
Did you miss this or is this a todo for later?
| users: await db | ||
| .knexInstance() | ||
| .select( | ||
| "id" satisfies keyof DbPlainUser, |
There was a problem hiding this comment.
I didn't know about satifies. Neat!
Also make sure we pass down knex in the related places and use transaction scopes where appropriate.
danyx23
force-pushed
the
db-types-migrate-variables-and-users
branch
from
d1f180f to
66b77fa
Compare
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.
Adds knex based DB access code for tables users and variables