Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use static navigation #3126

Merged
merged 6 commits into from
Jan 25, 2024
Merged

Use static navigation #3126

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
d151203
export live headerMenu.json
mlbrgl Jan 19, 2024
8a35723
(nav) remove excerpt and kpi keys
mlbrgl Jan 19, 2024
82bebf4
✨(nav) use static navigation
mlbrgl Jan 23, 2024
3fd221f
Remove unused code related to categories
mlbrgl Jan 23, 2024
1ea97dc
🤖 style: prettify code
mlbrgl Jan 23, 2024
5a2393a
🐝 remove unused import
danyx23 Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions baker/siteRenderers.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ import { CountryProfileSpec } from "../site/countryProfileProjects.js"
import { formatPost } from "./formatWordpressPost.js"
import {
getBlogIndex,
getEntriesByCategory,
getLatestPostRevision,
getPostBySlug,
isPostCitable,
Expand Down Expand Up @@ -92,6 +91,7 @@ import { postsTable } from "../db/model/Post.js"
import { GdocPost } from "../db/model/Gdoc/GdocPost.js"
import { logErrorAndMaybeSendToBugsnag } from "../serverUtils/errorLog.js"
import { GdocFactory } from "../db/model/Gdoc/GdocFactory.js"
import { SiteNavigationStatic } from "../site/SiteNavigation.js"

export const renderToHtmlPage = (element: any) =>
`<!doctype html>${ReactDOMServer.renderToStaticMarkup(element)}`
Expand Down Expand Up @@ -204,8 +204,7 @@ export const renderPreview = async (postId: number): Promise<string> => {
}

export const renderMenuJson = async () => {
const categories = await getEntriesByCategory()
return JSON.stringify({ categories: categories })
return JSON.stringify(SiteNavigationStatic)
}

export const renderPost = async (
Expand Down
136 changes: 10 additions & 126 deletions db/wpdb.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,11 @@ import { registerExitHandler } from "./cleanup.js"
import {
RelatedChart,
CategoryWithEntries,
EntryNode,
FullPost,
WP_PostType,
DocumentNode,
PostReference,
JsonError,
CategoryNode,
FilterFnPostRestApi,
PostRestApi,
TopicId,
Expand All @@ -49,6 +47,7 @@ import {
import { TOPICS_CONTENT_GRAPH } from "../settings/clientSettings.js"
import { GdocPost } from "./model/Gdoc/GdocPost.js"
import { Link } from "./model/Link.js"
import { SiteNavigationStatic } from "../site/SiteNavigation.js"

let _knexInstance: Knex

Expand Down Expand Up @@ -307,133 +306,19 @@ export const getDocumentsInfo = async (
}
}

const getEntryNode = ({
slug,
title,
excerpt,
kpi,
}: EntryNode): {
slug: string
title: string
excerpt: string
kpi: string
} => ({
slug,
title: decodeHTML(title),
excerpt: excerpt === null ? "" : decodeHTML(excerpt),
kpi,
})

const isEntryInSubcategories = (entry: EntryNode, subcategories: any): any => {
return subcategories.some((subcategory: any) => {
return subcategory.pages.nodes.some(
(node: EntryNode) => entry.slug === node.slug
)
})
}

// Retrieve a list of categories and their associated entries
let cachedEntries: CategoryWithEntries[] = []
export const getEntriesByCategory = async (): Promise<
CategoryWithEntries[]
> => {
if (!isWordpressAPIEnabled) return []
if (cachedEntries.length) return cachedEntries

const first = 100
// The filtering of cached entries below makes the $first argument
// less accurate, as it does not represent the exact number of entries
// returned per subcategories but rather their maximum number of entries.
const orderby = "TERM_ORDER"

// hack: using the description field ("01", "02", etc.) to order the top
// categories as TERM_ORDER doesn't seem to work as expected anymore.
const query = `
query getEntriesByCategory($first: Int, $orderby: TermObjectsConnectionOrderbyEnum!) {
categories(first: $first, where: {termTaxonomId: ${ENTRIES_CATEGORY_ID}, orderby: $orderby}) {
nodes {
name
children(first: $first, where: {orderby: DESCRIPTION}) {
nodes {
...categoryWithEntries
children(first: $first, where: {orderby: $orderby}) {
nodes {
...categoryWithEntries
}
}
}
}
}
}
}

fragment categoryWithEntries on Category {
name
slug
pages(first: $first, where: {orderby: {field: MENU_ORDER, order: ASC}}) {
nodes {
slug
title
excerpt
kpi
}
}
}
`
const categories = await graphqlQuery(query, { first, orderby })

cachedEntries = categories.data.categories.nodes[0].children.nodes.map(
({ name, slug, pages, children }: CategoryNode) => ({
name: decodeHTML(name),
slug,
entries: pages.nodes
.filter(
(node: EntryNode) =>
/* As entries are sometimes listed at all levels of the category hierarchy
(e.g. "Entries" > "Demographic Change" > "Life and Death" for "Child and
Infant Mortality"), it is necessary to filter out duplicates, by giving precedent to
the deepest level. In other words, if an entry is present in category 1 and category
1.1, it will only show in category 1.1.

N.B. Pre wp-graphql 0.6.0, entries would be returned at all levels of the category
hierarchy, no matter what categories were effectively selected. 0.6.0 fixes that
(cf. https://github.com/wp-graphql/wp-graphql/issues/1100). Even though this behaviour
has been fixed, we still have potential duplicates, from the multiple hierarchical
selection as noted above. The only difference is the nature of the duplicate, which can
now be considered more intentional as it is coming from the data / CMS.
Ultimately, this discrepency in the data should be addressed to make the system
less permissive. */
!isEntryInSubcategories(node, children.nodes)
)
.map((node: EntryNode) => getEntryNode(node)),
subcategories: children.nodes
.filter(
(subcategory: CategoryNode) =>
subcategory.pages.nodes.length !== 0
)
.map(({ name, slug, pages }: CategoryNode) => ({
name: decodeHTML(name),
slug,
entries: pages.nodes.map((node: EntryNode) =>
getEntryNode(node)
),
})),
})
)

return cachedEntries
}

export const isPostCitable = async (post: FullPost): Promise<boolean> => {
const entries = await getEntriesByCategory()
const entries = SiteNavigationStatic.categories
return entries.some((category) => {
return (
category.entries.some((entry) => entry.slug === post.slug) ||
category.subcategories.some((subcategory: CategoryWithEntries) => {
return subcategory.entries.some(
(subCategoryEntry) => subCategoryEntry.slug === post.slug
)
})
(category.subcategories ?? []).some(
(subcategory: CategoryWithEntries) => {
return subcategory.entries.some(
(subCategoryEntry) =>
subCategoryEntry.slug === post.slug
)
}
)
Comment on lines +310 to +321
Copy link

@coderabbitai coderabbitai bot Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic within isPostCitable assumes that the subcategories property always exists, which may not be the case according to the changes in ContentGraph.ts. Consider adding a nullish coalescing operator or default value for subcategories.

- (category.subcategories ?? []).some(
+ (category.subcategories || []).some(
Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation.

Suggested change
const entries = SiteNavigationStatic.categories
return entries.some((category) => {
return (
category.entries.some((entry) => entry.slug === post.slug) ||
category.subcategories.some((subcategory: CategoryWithEntries) => {
return subcategory.entries.some(
(subCategoryEntry) => subCategoryEntry.slug === post.slug
)
})
(category.subcategories ?? []).some(
(subcategory: CategoryWithEntries) => {
return subcategory.entries.some(
(subCategoryEntry) =>
subCategoryEntry.slug === post.slug
)
}
)
const entries = SiteNavigationStatic.categories
return entries.some((category) => {
return (
category.entries.some((entry) => entry.slug === post.slug) ||
(category.subcategories || []).some(
(subcategory: CategoryWithEntries) => {
return subcategory.entries.some(
(subCategoryEntry) =>
subCategoryEntry.slug === post.slug
)
}
)

The isPostCitable function uses a deeply nested structure with multiple some and every calls. This could be refactored for better readability and potentially performance by flattening the structure or using a different approach to check for the post's citability.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai a nullish coalescing operator has already been added in 82bebf4

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mlbrgl Thank you for the update. I've verified the changes in commit 82bebf4c1d05b7aee20cead971f37cd50f207633, and the nullish coalescing operator has indeed been added to the isPostCitable function. Your implementation now correctly handles potential null or undefined values for subcategories.

🎉

🐇✨

Analysis chain
Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check the changes made in the specified commit for the `isPostCitable` function
git show 82bebf4c1d05b7aee20cead971f37cd50f207633 -- db/wpdb.ts

Length of output: 5661

If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!

)
})
}
Expand Down Expand Up @@ -1005,7 +890,6 @@ export const getTables = async (): Promise<Map<string, TablepressTable>> => {

export const flushCache = (): void => {
cachedAuthorship = undefined
cachedEntries = []
cachedFeaturedImages = undefined
getBlogIndex.cache.clear?.()
cachedTables = undefined
Expand Down
5 changes: 2 additions & 3 deletions packages/@ourworldindata/types/src/domainTypes/ContentGraph.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,15 @@ import { TopicId } from "./Various.js"
export interface EntryMeta {
slug: string
title: string
excerpt: string
kpi: string
}

export interface CategoryWithEntries {
name: string
slug: string
entries: EntryMeta[]
subcategories: CategoryWithEntries[]
subcategories?: CategoryWithEntries[]
}

export enum GraphDocumentType {
Topic = "topic",
Article = "article",
Comment on lines 3 to 17
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tip

Codebase Verification

The search for .excerpt yielded multiple results across various files in the codebase, indicating that the excerpt property is indeed used in several places. However, there were no results for .kpi, suggesting that this property might not be used or is less prevalent.

Given the results, it is clear that the removal of the excerpt property from the EntryMeta interface could potentially impact the functionality in the files where .excerpt is referenced. This needs to be addressed to ensure that the removal does not cause any unintended side effects.

The review comment about verifying the impact of removing excerpt and kpi from EntryMeta is therefore substantiated by the search results for .excerpt. The absence of .kpi in the search results does not necessarily mean it is not used, as it could be referenced in a way that was not captured by the search pattern.

Based on the findings, the appropriate response is to flag the potential issues with the removal of the excerpt property.

Analysis chain

📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [1-11]

Verify that the removal of excerpt and kpi from EntryMeta does not negatively impact other parts of the codebase.

Scripts Executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usages of EntryMeta properties that have been removed.
ast-grep --lang typescript --pattern $'EntryMeta { $$$ .excerpt $$$ }'
ast-grep --lang typescript --pattern $'EntryMeta { $$$ .kpi $$$ }'

Length of output: 1041

Script:

#!/bin/bash
# Correct the pattern syntax and search for usages of EntryMeta properties that have been removed.
ast-grep --lang typescript --pattern $'interface EntryMeta { $$$ }' --exec 'rg "\.excerpt" {}'
ast-grep --lang typescript --pattern $'interface EntryMeta { $$$ }' --exec 'rg "\.kpi" {}'

Length of output: 575

Script:

#!/bin/bash
# Find files that contain the EntryMeta interface definition.
entry_meta_files=$(ast-grep --lang typescript --pattern $'interface EntryMeta { $$$ }' | rg -o '[^ ]+$')

# Search for usages of the removed properties within those files.
for file in $entry_meta_files; do
  rg "\.excerpt" $file
  rg "\.kpi" $file
done

Length of output: 1109

Script:

#!/bin/bash
# Directly search the entire codebase for references to the removed properties.
rg "\.excerpt"
rg "\.kpi"

Length of output: 1611

Expand Down
Loading
Loading