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

feat: Notebook list sorting and filtering #19058

Merged
merged 19 commits into from
Dec 6, 2023
Merged
Show file tree
Hide file tree
Changes from 5 commits
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
Binary file modified frontend/__snapshots__/scenes-app-saved-insights--card-view.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
28 changes: 15 additions & 13 deletions frontend/src/lib/api.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1536,13 +1536,19 @@ const api = {
return await new ApiRequest().notebook(notebookId).update({ data })
},
async list(
contains?: NotebookNodeResource[],
createdBy?: UserBasicType['uuid'],
search?: string
): Promise<PaginatedResponse<NotebookType>> {
params: {
contains?: NotebookNodeResource[]
created_by?: UserBasicType['uuid']
search?: string
order?: string
offset?: number
limit?: number
} = {}
): Promise<CountedPaginatedResponse<NotebookType>> {
// TODO attrs could be a union of types like NotebookNodeRecordingAttributes
const apiRequest = new ApiRequest().notebooks()
let q = {}
const { contains, ...queryParams } = objectClean(params)

if (contains?.length) {
const containsString =
contains
Expand All @@ -1552,15 +1558,11 @@ const api = {
return `${target}${match}`
})
.join(',') || undefined
q = { ...q, contains: containsString, created_by: createdBy }
}
if (createdBy) {
q = { ...q, created_by: createdBy }
}
if (search) {
q = { ...q, search: search }

queryParams['contains'] = containsString
}
return await apiRequest.withQueryString(q).get()

return await apiRequest.withQueryString(queryParams).get()
},
async create(data?: Pick<NotebookType, 'content' | 'text_content' | 'title'>): Promise<NotebookType> {
return await new ApiRequest().notebooks().create({ data })
Expand Down
35 changes: 12 additions & 23 deletions frontend/src/lib/lemon-ui/LemonTable/columnUtils.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,11 @@ import { UserBasicType } from '~/types'
import { ProfilePicture } from '../ProfilePicture'
import { LemonTableColumn } from './types'

export function createdAtColumn<T extends { created_at?: string | Dayjs | null }>(): LemonTableColumn<T, 'created_at'> {
export function atColumn<T extends Record<string, any>>(key: keyof T, title: string): LemonTableColumn<T, typeof key> {
return {
title: 'Created',
dataIndex: 'created_at',
render: function RenderCreatedAt(created_at) {
title: title,
dataIndex: key,
render: function RenderAt(created_at) {
return created_at ? (
<div className="whitespace-nowrap text-right">
<TZLabel time={created_at} />
Expand All @@ -20,9 +20,16 @@ export function createdAtColumn<T extends { created_at?: string | Dayjs | null }
)
},
align: 'right',
sorter: (a, b) => dayjs(a.created_at || 0).diff(b.created_at || 0),
sorter: (a, b) => dayjs(a[key] || 0).diff(b[key] || 0),
}
}
export function createdAtColumn<T extends { created_at?: string | Dayjs | null }>(): LemonTableColumn<T, 'created_at'> {
return atColumn('created_at', 'Created') as LemonTableColumn<T, 'created_at'>
}

export function updatedAtColumn<T extends { updated_at?: string | Dayjs | null }>(): LemonTableColumn<T, 'updated_at'> {
return atColumn('updated_at', 'Updated') as LemonTableColumn<T, 'updated_at'>
}

export function createdByColumn<T extends { created_by?: UserBasicType | null }>(): LemonTableColumn<T, 'created_by'> {
return {
Expand All @@ -44,21 +51,3 @@ export function createdByColumn<T extends { created_by?: UserBasicType | null }>
),
}
}

export function updatedAtColumn<T extends { updated_at?: string | Dayjs | null }>(): LemonTableColumn<T, 'updated_at'> {
return {
title: 'Updated',
dataIndex: 'updated_at',
render: function RenderCreatedAt(updated_at) {
return updated_at ? (
<div className="whitespace-nowrap text-right">
<TZLabel time={updated_at} />
</div>
) : (
<span className="text-muted">—</span>
)
},
align: 'right',
sorter: (a, b) => dayjs(a.updated_at || 0).diff(b.updated_at || 0),
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ export const notebookSelectButtonLogic = kea<notebookSelectButtonLogicType>([
{
loadAllNotebooks: async (_, breakpoint) => {
await breakpoint(100)
const response = await api.notebooks.list(undefined, undefined, values.searchQuery ?? undefined)
const response = await api.notebooks.list({ search: values.searchQuery ?? undefined })
// TODO for simplicity we'll assume the results will fit into one page
return response.results
},
Expand All @@ -67,23 +67,23 @@ export const notebookSelectButtonLogic = kea<notebookSelectButtonLogicType>([
if (!props.resource) {
return []
}
const response = await api.notebooks.list(
props.resource && typeof props.resource !== 'boolean'
? [
{
type: props.resource.type,
attrs: {
id:
props.resource.type === NotebookNodeType.Query
? props.resource.attrs.query.shortId
: props.resource.attrs.id,
const response = await api.notebooks.list({
contains:
props.resource && typeof props.resource !== 'boolean'
? [
{
type: props.resource.type,
attrs: {
id:
props.resource.type === NotebookNodeType.Query
? props.resource.attrs.query.shortId
: props.resource.attrs.id,
},
},
},
]
: undefined,
undefined,
values.searchQuery ?? undefined
)
]
: undefined,
search: values.searchQuery ?? undefined,
})
// TODO for simplicity we'll assume the results will fit into one page
return response.results
},
Expand Down
20 changes: 14 additions & 6 deletions frontend/src/scenes/notebooks/NotebooksTable/NotebooksTable.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ import { IconDelete, IconEllipsis } from 'lib/lemon-ui/icons'
import { LemonBanner } from 'lib/lemon-ui/LemonBanner'
import { LemonMenu } from 'lib/lemon-ui/LemonMenu'
import { LemonTable, LemonTableColumn, LemonTableColumns } from 'lib/lemon-ui/LemonTable'
import { createdAtColumn, createdByColumn } from 'lib/lemon-ui/LemonTable/columnUtils'
import { atColumn, createdByColumn } from 'lib/lemon-ui/LemonTable/columnUtils'
import { Link } from 'lib/lemon-ui/Link'
import { useEffect } from 'react'
import { ContainsTypeFilters } from 'scenes/notebooks/NotebooksTable/ContainsTypeFilter'
Expand Down Expand Up @@ -39,8 +39,9 @@ function titleColumn(): LemonTableColumn<NotebookListItemType, 'title'> {
}

export function NotebooksTable(): JSX.Element {
const { notebooksAndTemplates, filters, notebooksLoading, notebookTemplates } = useValues(notebooksTableLogic)
const { loadNotebooks, setFilters } = useActions(notebooksTableLogic)
const { notebooksAndTemplates, filters, notebooksResponseLoading, notebookTemplates, sortValue, pagination } =
useValues(notebooksTableLogic)
const { loadNotebooks, setFilters, setSortValue } = useActions(notebooksTableLogic)
const { meFirstMembers } = useValues(membersLogic)
const { selectNotebook } = useActions(notebookPanelLogic)

Expand All @@ -50,11 +51,16 @@ export function NotebooksTable(): JSX.Element {

const columns: LemonTableColumns<NotebookListItemType> = [
titleColumn() as LemonTableColumn<NotebookListItemType, keyof NotebookListItemType | undefined>,

createdByColumn<NotebookListItemType>() as LemonTableColumn<
NotebookListItemType,
keyof NotebookListItemType | undefined
>,
createdAtColumn<NotebookListItemType>() as LemonTableColumn<
atColumn<NotebookListItemType>('created_at', 'Created') as LemonTableColumn<
Copy link
Contributor

@daibhin daibhin Dec 4, 2023

Choose a reason for hiding this comment

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

Any reason not to use the createdAtColumn helper anymore? Guessing it's to be consistent and avoid another import

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mostly because I figured eventually we could just ditch it altogether but no strong reason

NotebookListItemType,
keyof NotebookListItemType | undefined
>,
atColumn<NotebookListItemType>('last_modified_at', 'Last modified') as LemonTableColumn<
NotebookListItemType,
keyof NotebookListItemType | undefined
>,
Expand Down Expand Up @@ -135,14 +141,16 @@ export function NotebooksTable(): JSX.Element {
</div>
<LemonTable
data-attr="notebooks-table"
pagination={{ pageSize: 100 }}
pagination={pagination}
dataSource={notebooksAndTemplates}
rowKey="short_id"
columns={columns}
loading={notebooksLoading}
loading={notebooksResponseLoading}
defaultSorting={{ columnKey: '-created_at', order: 1 }}
emptyState={`No notebooks matching your filters!`}
nouns={['notebook', 'notebooks']}
sorting={sortValue}
onSort={(newSorting) => setSortValue(newSorting)}
/>
</div>
)
Expand Down
75 changes: 61 additions & 14 deletions frontend/src/scenes/notebooks/NotebooksTable/notebooksTableLogic.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { PaginationManual } from '@posthog/lemon-ui'
import { actions, connect, kea, listeners, path, reducers, selectors } from 'kea'
import { loaders } from 'kea-loaders'
import api from 'lib/api'
import api, { CountedPaginatedResponse } from 'lib/api'
import { Sorting } from 'lib/lemon-ui/LemonTable'
import { objectClean, objectsEqual } from 'lib/utils'

import { notebooksModel } from '~/models/notebooksModel'
Expand All @@ -21,11 +23,15 @@ export const DEFAULT_FILTERS: NotebooksListFilters = {
contains: [],
}

const RESULTS_PER_PAGE = 50

export const notebooksTableLogic = kea<notebooksTableLogicType>([
path(['scenes', 'notebooks', 'NotebooksTable', 'notebooksTableLogic']),
actions({
loadNotebooks: true,
setFilters: (filters: Partial<NotebooksListFilters>) => ({ filters }),
setSortValue: (sortValue: Sorting | null) => ({ sortValue }),
setPage: (page: number) => ({ page }),
}),
connect({
values: [notebooksModel, ['notebookTemplates']],
Expand All @@ -42,10 +48,24 @@ export const notebooksTableLogic = kea<notebooksTableLogicType>([
}),
},
],
sortValue: [
null as Sorting | null,
{
setSortValue: (_, { sortValue }) => sortValue,
},
],
page: [
1,
{
setPage: (_, { page }) => page,
setFilters: () => 1,
setSortValue: () => 1,
},
],
}),
loaders(({ values }) => ({
notebooks: [
[] as NotebookListItemType[],
notebooksResponse: [
null as CountedPaginatedResponse<NotebookListItemType> | null,
{
loadNotebooks: async (_, breakpoint) => {
// TODO: Support pagination
Expand All @@ -56,30 +76,57 @@ export const notebooksTableLogic = kea<notebooksTableLogicType>([
const createdByForQuery =
values.filters?.createdBy === DEFAULT_FILTERS.createdBy ? undefined : values.filters?.createdBy

const res = await api.notebooks.list(contains, createdByForQuery, values.filters?.search)
const res = await api.notebooks.list({
contains,
created_by: createdByForQuery,
search: values.filters?.search || undefined,
order: values.sortValue
? `${values.sortValue.order === -1 ? '-' : ''}${values.sortValue.columnKey}`
: '-last_modified_at',
limit: RESULTS_PER_PAGE,
offset: (values.page - 1) * RESULTS_PER_PAGE,
})

breakpoint()
return res.results
return res
},
},
],
})),
listeners(({ actions }) => ({
setFilters: () => {
actions.loadNotebooks()
},
deleteNotebookSuccess: () => {
// TODO at some point this will be slow enough it makes sense to patch the in-memory list but for simplicity...
actions.loadNotebooks()
},
setFilters: () => actions.loadNotebooks(),
setSortValue: () => actions.loadNotebooks(),
setPage: () => actions.loadNotebooks(),
deleteNotebookSuccess: () => actions.loadNotebooks(),
})),
selectors({
selectors(({ actions }) => ({
notebooksAndTemplates: [
(s) => [s.notebooks, s.notebookTemplates, s.filters],
(notebooks, notebookTemplates, filters): NotebookListItemType[] => {
const includeTemplates = objectsEqual(filters, DEFAULT_FILTERS)
return [...(includeTemplates ? (notebookTemplates as NotebookListItemType[]) : []), ...notebooks]
},
],
}),

notebooks: [
(s) => [s.notebooksResponse],
(notebooksResponse): NotebookListItemType[] => {
return notebooksResponse?.results || []
},
],

pagination: [
(s) => [s.page, s.notebooksResponse],
(page, notebooksResponse): PaginationManual => {
return {
controlled: true,
pageSize: RESULTS_PER_PAGE,
currentPage: page,
entryCount: notebooksResponse?.count ?? 0,
onBackward: () => actions.setPage(page - 1),
onForward: () => actions.setPage(page + 1),
}
},
],
})),
])
19 changes: 9 additions & 10 deletions posthog/api/notebook.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,27 +258,26 @@ def get_queryset(self) -> QuerySet:
def _filter_request(self, request: Request, queryset: QuerySet) -> QuerySet:
filters = request.GET.dict()

for key in filters:
for key, value in filters.items():
if key == "user":
queryset = queryset.filter(created_by=request.user)
elif key == "created_by":
queryset = queryset.filter(created_by__uuid=request.GET["created_by"])
queryset = queryset.filter(created_by__uuid=value)
elif key == "last_modified_by":
queryset = queryset.filter(last_modified_by__uuid=value)
elif key == "date_from":
queryset = queryset.filter(
last_modified_at__gt=relative_date_parse(request.GET["date_from"], self.team.timezone_info)
)
queryset = queryset.filter(last_modified_at__gt=relative_date_parse(value, self.team.timezone_info))
elif key == "date_to":
queryset = queryset.filter(
last_modified_at__lt=relative_date_parse(request.GET["date_to"], self.team.timezone_info)
)
queryset = queryset.filter(last_modified_at__lt=relative_date_parse(value, self.team.timezone_info))
elif key == "search":
queryset = queryset.filter(
# some notebooks have no text_content until next saved, so we need to check the title too
# TODO this can be removed once all/most notebooks have text_content
Q(title__search=request.GET["search"]) | Q(text_content__search=request.GET["search"])
Q(title__search=value)
| Q(text_content__search=value)
)
elif key == "contains":
contains = request.GET["contains"]
contains = value
match_pairs = contains.replace(",", " ").split(" ")
# content is a JSONB field that has an array of objects under the key "content"
# each of those (should) have a "type" field
Expand Down
Loading