Migrate Wordpress post rendering #3135
Conversation
WalkthroughThe recent updates primarily involve refining the post retrieval and enrichment processes across various components of a web application. By eliminating outdated WordPress API dependencies, enhancing database interactions, and updating data models to better reflect enriched post information, the changes aim to streamline content management and presentation. This overhaul not only simplifies the codebase by removing redundant checks and outdated references but also improves the efficiency and accuracy of data handling within the application. Changes
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 (
|
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite. Current dependencies on/for this PR:
This stack of pull requests is managed by Graphite. |
mlbrgl
force-pushed
the
migrate-wp-post-rendering
branch
from
9afea05 to
a063282
Compare
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (11)
- adminSiteServer/mockSiteRouter.tsx (2 hunks)
- baker/SiteBaker.tsx (3 hunks)
- baker/algolia/indexToAlgolia.tsx (5 hunks)
- baker/pageOverrides.test.ts (5 hunks)
- baker/pageOverrides.tsx (4 hunks)
- baker/siteRenderers.tsx (9 hunks)
- baker/sitemap.ts (3 hunks)
- db/model/Post.ts (3 hunks)
- db/wpdb.ts (9 hunks)
- packages/@ourworldindata/types/src/domainTypes/Posts.ts (1 hunks)
- packages/@ourworldindata/types/src/wordpressTypes/WordpressTypes.ts (2 hunks)
Additional comments: 20
packages/@ourworldindata/types/src/domainTypes/Posts.ts (1)
- 29-34: > 📝 NOTE
This review was outside the diff hunks, and no overlapping diff hunk was found. Original lines [11-11]
The removal of the
pathproperty from theFullPostinterface and the addition of anidproperty of type number changes the data model. Ensure all usages ofFullPostthroughout the codebase are updated to reflect this change.packages/@ourworldindata/types/src/wordpressTypes/WordpressTypes.ts (2)
- 1-1: The addition of an import statement for
PostRowRawis necessary for the updatedFilterFnPostRestApitype. Ensure thatPostRowRawis correctly defined and imported from the expected location.- 56-56: The modification of
FilterFnPostRestApifrom(post: PostRestApi) => booleanto(post: PostRowRaw) => booleanaligns the type with the new data model. Verify that all usages ofFilterFnPostRestApihave been updated to passPostRowRawobjects instead ofPostRestApi.Verification successful
The search results confirm that
FilterFnPostRestApihas been updated in its definition to usePostRowRawand is being used accordingly within the codebase, particularly indb/model/Post.tswhere it's applied as a filter function for posts. This aligns with the review comment's request to verify that all usages ofFilterFnPostRestApihave been updated to passPostRowRawobjects instead ofPostRestApi. Therefore, the modification and its application across the codebase have been successfully verified.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of `FilterFnPostRestApi` to ensure it's now used with `PostRowRaw` objects. ast-grep --lang typescript --pattern $'type FilterFnPostRestApi = ($_: PostRowRaw) => boolean' # Expecting results confirming the updated usage.Length of output: 232
Script:
#!/bin/bash # Search for actual usages of `FilterFnPostRestApi` across the codebase to ensure it's being used with `PostRowRaw` objects. rg "FilterFnPostRestApi" --type ts # This command aims to find references to `FilterFnPostRestApi` in TypeScript files, which will help in verifying if the usages have been updated to work with `PostRowRaw` objects.Length of output: 507
baker/pageOverrides.test.ts (2)
- 11-30: The replacement of
FullPostwithPostRowEnrichedand the addition of several new properties in themockCreatePostfunction align the test suite with the updated data model. Ensure that these new properties (status,isListed,published_at, etc.) are correctly utilized in tests and reflect the actual data model changes.- 76-86: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [36-84]
The replacement of
getPostBySlugLogToSlackNoThrowwithgetPostEnrichedBySlugin test cases and the addition of various test scenarios to reflect the updated logic and data model are appropriate. Verify that the newgetPostEnrichedBySlugfunction is implemented correctly and that all test cases accurately test the intended functionality.Verification successful
The attempt to verify the implementation of
getPostEnrichedBySlugusingast-grepdid not yield any results. This could indicate that the function might not be present in the expected form or there was an issue with the search parameters. To ensure a thorough verification, a different approach to search for the function's implementation is necessary.
The search for the
getPostEnrichedBySlugfunction usingast-grepdid not return any results, but the subsequentrgsearch successfully identified multiple references togetPostEnrichedBySlugacross various files, including its import inpageOverrides.tsxandGrapherBaker.tsx, as well as its definition indb/model/Post.ts. This confirms the presence and usage of thegetPostEnrichedBySlugfunction within the codebase, aligning with the context provided in the review comment regarding its implementation and use in test cases.Given this information, it can be concluded that the replacement of
getPostBySlugLogToSlackNoThrowwithgetPostEnrichedBySlugin test cases and the addition of various test scenarios to reflect the updated logic and data model are indeed appropriate and have been verified to be correctly implemented in the codebase.Scripts Executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the implementation of `getPostEnrichedBySlug`. ast-grep --lang typescript --pattern $'export async function getPostEnrichedBySlug($_: string) { $$$ }' # Expecting to find the function definition and its logic.Length of output: 106
Script:
#!/bin/bash # Attempting a broader search for the function `getPostEnrichedBySlug` without specifying it must be exported or async, to catch any form of its declaration. ast-grep --lang typescript --pattern $'function getPostEnrichedBySlug($_: string) { $$$ }'Length of output: 93
Script:
#!/bin/bash # Additionally, using ripgrep to search for any mention of `getPostEnrichedBySlug` in TypeScript files, in case the function is declared in a way that ast-grep's pattern does not capture. rg "getPostEnrichedBySlug" -ttsLength of output: 1292
baker/pageOverrides.tsx (1)
- 1-16: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [3-57]
The modifications in
pageOverrides.tsx, including changes to imports and adjustments to thegetLandingOnlyIfParentandgetPageOverridesfunctions, are necessary to align with the new data model. Ensure that the removal ofgetPostBySlugLogToSlackNoThrowand the adjustments made to parameter types and logic do not introduce regressions and that all references to these functions are updated accordingly.baker/sitemap.ts (1)
- 60-66: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [15-93]
The removal of the
wpdbdependency and the update of thelastmodfield in themakeSitemapfunction to useupdated_atinstead ofmodified_gmtare aligned with the transition to using the grapher DB for content rendering. Ensure that thegetPostsfunction correctly retrieves and filters posts from the grapher DB and that thelastmodfield accurately reflects the most recent update time of the posts.db/wpdb.ts (4)
- 38-38: The import for
PostRowEnrichedandTopicfrom@ourworldindata/typesreplaces removed imports, aligning with the new data source strategy.- 47-47: The addition of
getPostsandfilterListedPostsfunction imports from./model/Post.jsis crucial for fetching and filtering posts from the grapher DB, supporting the PR's objective to transition data sources.- 375-382: Modification of the
getPostIdAndTypeBySlugfunction to query from thepoststable instead of using the Wordpress API or GraphQL endpoint is a significant change that aligns with the PR's objective of transitioning to the grapher DB.- 647-673: The
getFullPostfunction has been significantly modified to adapt to the newPostRowEnrichedtype. This change is crucial for integrating the new data source into the post rendering process. Ensure that all fields used fromPostRowEnrichedmatch the expected structure and types.baker/siteRenderers.tsx (5)
- 67-67: The addition of
getFullPostto the import list from../db/wpdb.jsis necessary for fetching enriched post data from the new data source, supporting the PR's objective.- 89-93: The replacement of
getPostBySlugwithgetPostEnrichedBySlugandgetFullPostfor enriching and retrieving post data is a key change that aligns with the PR's objective of utilizing the grapher DB for content rendering.- 200-203: The
renderPageBySlugfunction now usesgetPostEnrichedBySlugandgetFullPostto fetch and enrich post data, which is a significant change that aligns with the PR's objective of transitioning to the grapher DB for post rendering.- 207-210: Similar to
renderPageBySlug, therenderPreviewfunction's adaptation to usegetPostEnrichedByIdandgetFullPostfor fetching and enriching post data supports the PR's objective of utilizing the grapher DB.- 241-241: The
renderPostfunction's modification to usegetPageOverrideswith the post slug instead of the post object is a subtle but important change that could impact how page overrides are applied. Ensure that this change is tested thoroughly to avoid unintended behavior in post rendering.baker/SiteBaker.tsx (4)
- 74-74: The import statement for
getPostsfrom "../db/model/Post.js" has been added. This change aligns with the PR's objective to transition data sources from the Wordpress API to the grapher DB.- 389-389: The addition of
db.getConnection()before fetching posts withgetPosts()in theremoveDeletedPostsmethod is crucial for establishing a database connection. This ensures that the data retrieval is performed on an active connection, which is a good practice for database interactions.- 391-391: Replacing
wpdb.getPosts()withgetPosts()in theremoveDeletedPostsmethod is consistent with the PR's goal of transitioning away from the Wordpress API towards using the grapher DB. This change is necessary for fetching posts directly from the new data source.- 385-394: > 📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [74-420]
The modifications in
SiteBaker.tsx, including the addition of a new import and changes within theremoveDeletedPostsmethod, are well-aligned with the PR's objectives to shift the data source for rendering Wordpress posts. These changes are correctly implemented and should not introduce any issues based on the provided context. It's important to ensure that all other parts of the system that interact with these modified areas are also updated to accommodate the new data source.
|
@danyx23 rebasing onto the lastest DB access updates. Will reopen once I'm done. |
|
@mlbrgl thank you! |
mlbrgl
force-pushed
the
migrate-wp-post-rendering
branch
from
da807f5 to
1eb8e4f
Compare
|
@danyx23 ok done, good to go! Re-baked with (in addition to the partial bake done on push): |
mlbrgl
force-pushed
the
migrate-wp-post-rendering
branch
from
1eb8e4f to
4d891f9
Compare
|
Hey @mlbrgl, this looks good already. I checked it manually for a few pages and found that for example https://ourworldindata.org/books looks different. Is this expected at this point? Should I investigate these differences more? I think it would be useful to rebase on top of current master so that we can use the screenshot tool with the new improments that Ike build that depend on the new events. I think |
|
@danyx23 thanks for the review! Yes this is to be expected at this point, so no need to spend time on investigating further. I also revised my thinking and I am now pursing a different approach as you know from our last conversation: baking from rendered API response (vs baking from unprocessed source HTML here) So I'll retract this PR for now. Thanks again for spending time on this regardless! 🙏 |
mlbrgl
force-pushed
the
migrate-wp-post-rendering
branch
from
4d891f9 to
b70c6f4
Compare
This PR swaps the data source for the rendering of Wordpress posts from the Wordpress API to the synced grapher DB.
This change aims to be as surgical as possible, but still touches a lot of rendering code, including the
sitemap, algolia, blog index, country profiles, post previews and post rendering.
The next steps will be centered around the render functions in
formatWordpressPost(), e.g.renderProminentLinks(), which relied on PHP code that is not part of the rendering pipeline anymore (e.g.prominent-link.php)Note: I kept
getFullPostandgetPostRowEnrichedseparate but we could imagine merging them. I didn't want to propagate getFullPost edge cases to the parts of the codebase already relying on getPostRowEnriched. If this overall approach seems promising, we might decide to spend more time on refactoring for clarity, although the opportunity cost needs to be carefully weighed given the expected temporary and frozen nature of this part of the codebase.Summary by CodeRabbit