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.

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

✨ show average daily pageviews in charts list and references tab #4267

Merged
merged 2 commits into from
Dec 13, 2024
Merged
Changes from 1 commit
Commits
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
15 changes: 15 additions & 0 deletions 1733503871571-AddPageviewsUrlIndex.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { MigrationInterface, QueryRunner } from "typeorm"

export class AddPageviewsUrlIndex1733503871571 implements MigrationInterface {
public async up(queryRunner: QueryRunner): Promise<void> {
// add an index on url of the analtyics_pageviews table
// we already have one on (day, url) but we never join that way
await queryRunner.query(
`CREATE INDEX analytics_pageviews_url_index ON analytics_pageviews (url)`
)
}

public async down(queryRunner: QueryRunner): Promise<void> {
await queryRunner.query(`DROP INDEX analytics_pageviews_url_index`)
}
}
32 changes: 26 additions & 6 deletions adminSiteClient/ChartIndexPage.tsx
Original file line number Diff line number Diff line change
@@ -4,7 +4,7 @@ import { observable, computed, action, runInAction } from "mobx"

import { TextField } from "./Forms.js"
import { AdminLayout } from "./AdminLayout.js"
import { ChartList, ChartListItem } from "./ChartList.js"
import { ChartList, ChartListItem, SortConfig } from "./ChartList.js"
import { AdminAppContext, AdminAppContextType } from "./AdminAppContext.js"
import {
buildSearchWordsFromSearchString,
@@ -21,6 +21,8 @@ export class ChartIndexPage extends React.Component {
@observable searchInput?: string
@observable maxVisibleCharts = 50
@observable charts: ChartListItem[] = []
@observable sortBy: "pageviewsPerDay" | null = null
@observable sortConfig: SortConfig = null

@computed get searchWords(): SearchWord[] {
const { searchInput } = this
@@ -31,7 +33,8 @@ export class ChartIndexPage extends React.Component {
}

@computed get allChartsToShow(): ChartListItem[] {
const { searchWords, charts } = this
const { searchWords, charts, sortConfig } = this
let filtered = charts
if (searchWords.length > 0) {
const filterFn = filterFunctionForSearchWords(
searchWords,
@@ -48,10 +51,21 @@ export class ChartIndexPage extends React.Component {
...chart.tags.map((tag) => tag.name),
]
)
return charts.filter(filterFn)
} else {
return this.charts
filtered = charts.filter(filterFn)
}

// Apply sorting if needed
if (sortConfig?.field === "pageviewsPerDay") {
return [...filtered].sort((a, b) => {
const aValue = a.pageviewsPerDay || 0
const bValue = b.pageviewsPerDay || 0
return sortConfig.direction === "desc"
? bValue - aValue
: aValue - bValue
})
Copy link
Member

Choose a reason for hiding this comment

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

Probably better to use the existing sortNumeric from utils here.
Just take caution because it sorts in-place, but your current solution is, too :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice!

}

return filtered
}

@computed get chartsToShow(): ChartListItem[] {
@@ -67,8 +81,12 @@ export class ChartIndexPage extends React.Component {
this.maxVisibleCharts += 100
}

@action.bound onSort(sortConfig: SortConfig) {
this.sortConfig = sortConfig
}

render() {
const { chartsToShow, searchInput, numTotalCharts } = this
const { chartsToShow, searchInput, numTotalCharts, sortConfig } = this

const highlight = highlightFunctionForSearchWords(this.searchWords)

@@ -93,6 +111,8 @@ export class ChartIndexPage extends React.Component {
onDelete={action((c: ChartListItem) =>
this.charts.splice(this.charts.indexOf(c), 1)
)}
onSort={this.onSort}
sortConfig={sortConfig}
/>
{!searchInput && (
<button
31 changes: 30 additions & 1 deletion adminSiteClient/ChartList.tsx
Original file line number Diff line number Diff line change
@@ -38,13 +38,21 @@ export interface ChartListItem {
isInheritanceEnabled?: boolean

tags: DbChartTagJoin[]
pageviewsPerDay: number
}

export type SortConfig = {
field: "pageviewsPerDay"
direction: "asc" | "desc"
} | null

@observer
export class ChartList extends React.Component<{
charts: ChartListItem[]
searchHighlight?: (text: string) => string | React.ReactElement
onDelete?: (chart: ChartListItem) => void
onSort?: (sort: SortConfig) => void
sortConfig?: SortConfig
}> {
static contextType = AdminAppContext
context!: AdminAppContextType
@@ -109,9 +117,24 @@ export class ChartList extends React.Component<{
}

render() {
const { charts, searchHighlight } = this.props
const { charts, searchHighlight, sortConfig, onSort } = this.props
const { availableTags } = this

const getSortIndicator = () => {
if (!sortConfig || sortConfig.field !== "pageviewsPerDay") return ""
return sortConfig.direction === "desc" ? " ↓" : " ↑"
}

const handleSortClick = () => {
if (!sortConfig || sortConfig.field !== "pageviewsPerDay") {
onSort?.({ field: "pageviewsPerDay", direction: "desc" })
} else if (sortConfig.direction === "desc") {
onSort?.({ field: "pageviewsPerDay", direction: "asc" })
} else {
onSort?.(null)
}
}

// if the first chart has inheritance information, we assume all charts have it
const showInheritanceColumn =
charts[0]?.isInheritanceEnabled !== undefined
@@ -128,6 +151,12 @@ export class ChartList extends React.Component<{
<th>Tags</th>
<th>Published</th>
<th>Last Updated</th>
<th
style={{ cursor: "pointer" }}
onClick={handleSortClick}
>
views/day{getSortIndicator()}
</th>
<th></th>
<th></th>
</tr>
1 change: 1 addition & 0 deletions adminSiteClient/ChartRow.tsx
Original file line number Diff line number Diff line change
@@ -105,6 +105,7 @@ export class ChartRow extends React.Component<{
by={highlight(chart.lastEditedBy)}
/>
</td>
<td>{chart.pageviewsPerDay?.toLocaleString() ?? "0"}</td>
<td>
<Link
to={`/charts/${chart.id}/edit`}
12 changes: 12 additions & 0 deletions adminSiteClient/EditorReferencesTab.tsx
Original file line number Diff line number Diff line change
@@ -208,6 +208,18 @@ export class EditorReferencesTabForChart extends React.Component<{
<strong>Last 365 days:</strong>{" "}
{this.renderPageview(this.pageviews?.views_365d)}
</div>
<div>
<strong>
Average pageviews per day over the last year:
</strong>{" "}
{this.renderPageview(
this.pageviews?.views_365d
? Math.round(
this.pageviews?.views_365d / 36.5
) / 10
: undefined
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit arcane - I would prefer you use lodash's round(num, 1) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah this is much better, thanks!

)}
</div>
</div>
<small className="form-text text-muted">
Pageview numbers are inaccurate when the chart has been
4 changes: 4 additions & 0 deletions adminSiteServer/apiRouter.ts
Original file line number Diff line number Diff line change
@@ -715,6 +715,7 @@ getRouteWithROTransaction(apiRouter, "/charts.json", async (req, res, trx) => {
SELECT ${oldChartFieldList} FROM charts
JOIN chart_configs ON chart_configs.id = charts.configId
JOIN users lastEditedByUser ON lastEditedByUser.id = charts.lastEditedByUserId
LEFT JOIN analytics_pageviews on analytics_pageviews.url = CONCAT("https://ourworldindata.org/grapher/", chart_configs.slug)
LEFT JOIN users publishedByUser ON publishedByUser.id = charts.publishedByUserId
ORDER BY charts.lastEditedAt DESC LIMIT ?
`,
@@ -1558,6 +1559,7 @@ getRouteWithROTransaction(
JOIN chart_configs ON chart_configs.id = charts.configId
JOIN users lastEditedByUser ON lastEditedByUser.id = charts.lastEditedByUserId
LEFT JOIN users publishedByUser ON publishedByUser.id = charts.publishedByUserId
LEFT JOIN analytics_pageviews on analytics_pageviews.url = CONCAT("https://ourworldindata.org/grapher/", chart_configs.slug)
JOIN chart_dimensions cd ON cd.chartId = charts.id
WHERE cd.variableId = ?
GROUP BY charts.id
@@ -2044,6 +2046,7 @@ getRouteWithROTransaction(
JOIN variables AS v ON cd.variableId = v.id
JOIN users lastEditedByUser ON lastEditedByUser.id = charts.lastEditedByUserId
LEFT JOIN users publishedByUser ON publishedByUser.id = charts.publishedByUserId
LEFT JOIN analytics_pageviews on analytics_pageviews.url = CONCAT("https://ourworldindata.org/grapher/", chart_configs.slug)
WHERE v.datasetId = ?
GROUP BY charts.id
`,
@@ -2441,6 +2444,7 @@ getRouteWithROTransaction(
LEFT JOIN chart_tags ct ON ct.chartId=charts.id
JOIN users lastEditedByUser ON lastEditedByUser.id = charts.lastEditedByUserId
LEFT JOIN users publishedByUser ON publishedByUser.id = charts.publishedByUserId
LEFT JOIN analytics_pageviews on analytics_pageviews.url = CONCAT("https://ourworldindata.org/grapher/", chart_configs.slug)
WHERE ct.tagId ${tagId === UNCATEGORIZED_TAG_ID ? "IS NULL" : "= ?"}
GROUP BY charts.id
ORDER BY charts.updatedAt DESC
3 changes: 2 additions & 1 deletion db/model/Chart.ts
Original file line number Diff line number Diff line change
@@ -538,7 +538,8 @@ export const oldChartFieldList = `
lastEditedByUser.fullName AS lastEditedBy,
charts.publishedAt,
charts.publishedByUserId,
publishedByUser.fullName AS publishedBy
publishedByUser.fullName AS publishedBy,
round(views_365d / 365, 1) as pageviewsPerDay
`
// TODO: replace this with getBySlug and pick