🚧 remove typeorm annotations from GDocs classes #3299

danyx23 · 2024-03-06T12:06:50Z

This PR converts GDocBase and all related classes (including links, images etc) to be fetched via knex. For GDocs, the classes are kept for business logic but the database loading/saving is done via knex and objects.

The main functionality for loading and storing Gdocs has moved to GDocFactory as plain functions.

danyx23 · 2024-03-06T12:07:04Z

🚧 remove typeorm annotations from GDocs classes #3299 : 2 dependent PRs (#3358 , #3372 ) 👈
🐝 remove obsolete code #3298
🔨 migrate charts from typeorm to knex #3293
🔨 clean up posts access via knex #3292
✨ add transaction types and api route helpers #3271
🔨 migrate PostLink to knex #3259
💄 reorder parameters of knexRaw and knexRawFirst to have knex in first pos #3258
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @danyx23 and the rest of your teammates on Graphite

ikesau

Phew! That took a while. Mostly nitpicks to be honest. Imagine there are a couple "🙂" at the end of basically every comment 😅

Will QA tomorrow 🙂🙂

ikesau · 2024-03-19T22:25:44Z

packages/@ourworldindata/types/src/gdocTypes/Gdoc.ts

@@ -93,6 +95,27 @@ export interface OwidGdocMinimalPostInterface {
    "featured-image"?: string // used in prominent links and research & writing block
 }

+export type OwidGdocIndexItem = Pick<


prettier 🤮

ikesau · 2024-03-19T22:29:24Z

adminSiteClient/GdocsStore.tsx

+        const indexItem = extractGdocIndexItem(gdoc)
+        const gdocToUpdateIndex = this.gdocs.findIndex((g) => g.id === gdoc.id)
+        if (gdocToUpdateIndex >= 0) this.gdocs[gdocToUpdateIndex] = indexItem


This is definitely more correct, but is it ever actually used? Whenever you load the index page it refetches all gdocs anyway.

Yeah probably not but it just seems like the right thing to do - maybe we change the behaviour in the future and then it's nice if this part already does what you would expect.

ikesau · 2024-03-19T22:30:38Z

adminSiteClient/gdocsDeploy.ts

@@ -66,6 +67,7 @@ export const checkIsLightningUpdate = (
        published: false, // requires an update of the blog roll
        publishedAt: false, // could require an update of the blog roll
        slug: false, // requires updating any articles that link to it
+        markdown: true,


Do you mind putting this above all the falses ? Keeps it easier to scan I think.

ikesau · 2024-03-19T22:36:44Z

adminSiteServer/apiRouter.ts

-            return b.updatedAt.getTime() - a.updatedAt.getTime()
-        })
-    )
+getRouteWithROTransaction(apiRouter, "/gdocs", async (req, res, trx) => {


It doesn't bother me, but adding async here is unnecessary, right?

Jup, I mechanically replaced some things and the occasionally a superfluous async came in

ikesau · 2024-03-19T23:06:30Z

db/model/Gdoc/GdocFactory.ts

+    knex: KnexReadonlyTransaction,
+    id: string,
+    gdoc: OwidGdoc,
+    markdownContentSource: OwidEnrichedGdocBlock[]


Not a biggie, just floating it for your consideration, but my rule of thumb is once functions get more than 3 parameters (i.e. it becomes complex enough that the name alone probably doesn't fully describe what it does), I strongly consider changing them to a object so that the arguments get descriptive names in usage.

When first reading this code I had no idea what the flattened sources could be for. Jumping to the definition is easy, so whatever, but also sometimes there are arguments that are similar to one another and bugs can happen if you mistakenly put them in the wrong order.

function updateGdocContentOnly({ knex: trx, id, gdoc, markdownContentSource: gdoc.enrichedBlockSources.flat() })

It also comes in handy when you've got a series of boolean parameters where you might want to rely on defaults like in getGdocBaseObjectById below.

Yeah valid point - when looking at it now though I realized that we might as well just extract it inside the function and get rid of the last parameter.

ikesau · 2024-03-20T01:20:20Z

db/model/Gdoc/GdocPost.ts


-        if (!gdoc) {
+        if (!gdoc || !("details" in gdoc.content)) {


Maybe update the error message here too?

ikesau · 2024-03-20T01:20:51Z

db/model/Gdoc/GdocPost.ts

+    static async getListedGdocPosts(
+        knex: KnexReadonlyTransaction
+    ): Promise<GdocPost[]> {
+        return getAndLoadListedGdocPosts(knex)


Could we just remove this and call the function directly?

ikesau · 2024-03-20T01:23:04Z

db/model/Image.ts

+    filename: string
+): Promise<Image | undefined> {
+    const image = await knex
+        .table("images")


I think we should use a variable here if we're using one for all our other tables 🙂

ikesau · 2024-03-20T01:24:26Z

db/model/Link.ts

+        select posts_gdocs_links.*, posts_gdocs.slug as sourceSlug
+        from posts_gdocs_links


Probably not worth it to try given the lack of code formatter for SQL in TS, but should we try to maintain a consistent style for our SQL? Uppercase keywords, indentation?

There actually is one that works for selections - see the message in our slack in the #developers channel

ikesau · 2024-03-20T01:26:52Z

packages/@ourworldindata/types/src/dbTypes/PostsGdocs.ts

@@ -61,5 +63,6 @@ export function serializePostsGdocsRow(row: DbEnrichedPostGdoc): DbRawPostGdoc {
        ...row,
        content: serializePostGdocContent(row.content),
        breadcrumbs: serializePostsGdocsBreadcrumbs(row.breadcrumbs),
+        published: row.published ? 1 : 0,


jsyk you can also do Number(row.published) 🙂

Again, the ternary is probably more explicit but I just like sharing little JS special recipes

ikesau · 2024-03-22T18:44:56Z

Haven't found any remaining issues in QA on the tip of this 🙂

danyx23 · 2024-03-26T14:04:43Z

Merge activity

Mar 26, 10:04 AM EDT: @danyx23 started a stack merge that includes this pull request via Graphite.
Mar 26, 10:17 AM EDT: Graphite rebased this pull request as part of a merge.
Mar 26, 10:18 AM EDT: @danyx23 merged this pull request with Graphite.

Also remove the `GdocXImage` and `GdocXTag` classes and switches them to knex. Also introduces OwidGdocIndexItem which is a more lightweight representation of a Gdoc that is used in the overview page of the admin.

danyx23 mentioned this pull request Mar 6, 2024

🐝 remove obsolete code #3298

Merged

github-actions bot assigned danyx23 Mar 6, 2024

danyx23 force-pushed the db-remove-dead-code-round-one branch from 5d00c2b to b93390a Compare March 6, 2024 12:42

danyx23 force-pushed the db-migrate-gdocs branch from 47f72ea to b63272c Compare March 6, 2024 12:42

danyx23 force-pushed the db-remove-dead-code-round-one branch from b93390a to a02c68d Compare March 16, 2024 00:06

danyx23 force-pushed the db-migrate-gdocs branch from 9253472 to e33773a Compare March 16, 2024 00:10

danyx23 marked this pull request as ready for review March 16, 2024 00:10

danyx23 force-pushed the db-remove-dead-code-round-one branch from a02c68d to 2124f46 Compare March 16, 2024 12:36

danyx23 force-pushed the db-migrate-gdocs branch from e33773a to 5dedb67 Compare March 16, 2024 12:37

This was referenced Mar 16, 2024

🔨 set up eslint rule to error on dangling promises + fix 245 violations #3358

Merged

🔨 switch almost all remaining typeorm util function holdouts to knex #3359

Merged

danyx23 force-pushed the db-remove-dead-code-round-one branch from 2124f46 to 146ea9e Compare March 16, 2024 13:46

danyx23 force-pushed the db-migrate-gdocs branch 2 times, most recently from 43f3e8f to f7d9953 Compare March 16, 2024 17:18

danyx23 mentioned this pull request Mar 16, 2024

✨ unify transaction creation and close db connection in same call for scripts #3360

Merged

danyx23 force-pushed the db-remove-dead-code-round-one branch from b37a219 to 6358087 Compare March 18, 2024 18:59

danyx23 force-pushed the db-migrate-gdocs branch from c5a2b89 to 0ca18ae Compare March 18, 2024 18:59

danyx23 requested a review from ikesau March 18, 2024 19:00

danyx23 force-pushed the db-migrate-gdocs branch from 0ca18ae to 23dccc3 Compare March 19, 2024 16:02

ikesau requested changes Mar 20, 2024

View reviewed changes

danyx23 force-pushed the db-remove-dead-code-round-one branch from 6358087 to 2b680e3 Compare March 20, 2024 10:05

danyx23 force-pushed the db-migrate-gdocs branch from 23dccc3 to 713d246 Compare March 20, 2024 10:05

This was referenced Mar 20, 2024

🐛 Fix bug in SQL query #3372

Merged

🔨 images need a RW transaction in case they need to update the db 😢 - bubble this up #3385

Merged

danyx23 mentioned this pull request Mar 21, 2024

Fix remaining refactor issues #3391

Merged

danyx23 force-pushed the db-remove-dead-code-round-one branch from 2b680e3 to 887bc31 Compare March 21, 2024 22:22

danyx23 force-pushed the db-migrate-gdocs branch from bf7385a to 8c6f6ac Compare March 21, 2024 22:22

danyx23 force-pushed the db-remove-dead-code-round-one branch from 887bc31 to 1e899ed Compare March 23, 2024 15:34

danyx23 force-pushed the db-migrate-gdocs branch from 8c6f6ac to 575b97e Compare March 23, 2024 15:34

danyx23 mentioned this pull request Mar 23, 2024

🔨 refactor suggested chart revision tool to use knex #3396

Merged

danyx23 force-pushed the db-migrate-gdocs branch from 575b97e to 86ce847 Compare March 25, 2024 11:26

danyx23 force-pushed the db-remove-dead-code-round-one branch from 1e899ed to 1a011bc Compare March 26, 2024 11:44

danyx23 force-pushed the db-migrate-gdocs branch 2 times, most recently from eb9cbd7 to 3a05c68 Compare March 26, 2024 13:08

danyx23 force-pushed the db-remove-dead-code-round-one branch from 1a011bc to 805d56f Compare March 26, 2024 14:14

Base automatically changed from db-remove-dead-code-round-one to master March 26, 2024 14:16

🚧 remove typeorm annotations from GDocs classes

5e0e462

Also remove the `GdocXImage` and `GdocXTag` classes and switches them to knex. Also introduces OwidGdocIndexItem which is a more lightweight representation of a Gdoc that is used in the overview page of the admin.

danyx23 force-pushed the db-migrate-gdocs branch from 3a05c68 to 5e0e462 Compare March 26, 2024 14:16

danyx23 merged commit 395c86a into master Mar 26, 2024
15 of 20 checks passed

danyx23 deleted the db-migrate-gdocs branch March 26, 2024 14:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

🚧 remove typeorm annotations from GDocs classes #3299

🚧 remove typeorm annotations from GDocs classes #3299

danyx23 commented Mar 6, 2024 •

edited

Loading

danyx23 commented Mar 6, 2024 •

edited

Loading

ikesau left a comment

ikesau Mar 19, 2024

ikesau Mar 19, 2024

danyx23 Mar 20, 2024

ikesau Mar 19, 2024

ikesau Mar 19, 2024

danyx23 Mar 20, 2024

ikesau Mar 19, 2024

danyx23 Mar 20, 2024

ikesau Mar 20, 2024

ikesau Mar 20, 2024

ikesau Mar 20, 2024

ikesau Mar 20, 2024

danyx23 Mar 20, 2024

ikesau Mar 20, 2024

ikesau commented Mar 22, 2024

danyx23 commented Mar 26, 2024 •

edited

Loading

		select posts_gdocs_links.*, posts_gdocs.slug as sourceSlug
		from posts_gdocs_links

🚧 remove typeorm annotations from GDocs classes #3299

🚧 remove typeorm annotations from GDocs classes #3299

Conversation

danyx23 commented Mar 6, 2024 • edited Loading

danyx23 commented Mar 6, 2024 • edited Loading

ikesau left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikesau commented Mar 22, 2024

danyx23 commented Mar 26, 2024 • edited Loading

Merge activity

danyx23 commented Mar 6, 2024 •

edited

Loading

danyx23 commented Mar 6, 2024 •

edited

Loading

danyx23 commented Mar 26, 2024 •

edited

Loading