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(cohorts): enable cohort pagination #27422

Merged
merged 22 commits into from
Jan 15, 2025
Merged
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
f47772c
saving work, didn't include pagination for the sake of proving someth…
dmarticus Jan 8, 2025
248a183
Merge branch 'master' into fix/cohort-pagination
dmarticus Jan 8, 2025
0e106eb
UGH
dmarticus Jan 8, 2025
3882b9a
this should do it
dmarticus Jan 9, 2025
72f1d97
Update UI snapshots for `chromium` (1)
github-actions[bot] Jan 9, 2025
919fd75
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 14, 2025
7b1c250
tests and whatnot
dmarticus Jan 14, 2025
861664e
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 14, 2025
965f748
fix merge
dmarticus Jan 14, 2025
7fbb34b
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 14, 2025
39346d4
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 14, 2025
44c7631
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 14, 2025
0e86baf
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 15, 2025
d4eed59
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 15, 2025
74d6ba9
Update query snapshots
github-actions[bot] Jan 15, 2025
421b6d4
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 15, 2025
f23b8b0
Update UI snapshots for `chromium` (1)
github-actions[bot] Jan 15, 2025
cc3673a
Update UI snapshots for `chromium` (2)
github-actions[bot] Jan 15, 2025
2f3a96e
getting the all users cohort filters working
dmarticus Jan 15, 2025
ddc5834
Merge branch 'feat/cohort-pagination' of github.com:PostHog/posthog i…
dmarticus Jan 15, 2025
b395c43
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 15, 2025
3b93bd8
Merge branch 'master' into feat/cohort-pagination
dmarticus Jan 15, 2025
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
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
2 changes: 1 addition & 1 deletion frontend/src/layout/navigation-3000/sidebars/cohorts.ts
Original file line number Diff line number Diff line change
@@ -94,7 +94,7 @@ export const cohortsSidebarLogic = kea<cohortsSidebarLogicType>([
if (searchTerm) {
return fuse.search(searchTerm).map((result) => [result.item, result.matches as FuseSearchMatch[]])
}
return cohorts.map((cohort) => [cohort, null])
return cohorts.results.map((cohort) => [cohort, null])
},
],
})),
13 changes: 9 additions & 4 deletions frontend/src/lib/api.ts
Original file line number Diff line number Diff line change
@@ -1510,16 +1510,21 @@ const api = {
async duplicate(cohortId: CohortType['id']): Promise<CohortType> {
return await new ApiRequest().cohortsDuplicate(cohortId).get()
},
async list(): Promise<PaginatedResponse<CohortType>> {
// TODO: Remove hard limit and paginate cohorts
return await new ApiRequest().cohorts().withQueryString('limit=600').get()
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we used to cap the limit at 600, which was causing a problem with returning the cohort name here: https://posthog.slack.com/archives/C07Q2U4BH4L/p1734442912694879

},
determineDeleteEndpoint(): string {
return new ApiRequest().cohorts().assembleEndpointUrl()
},
determineListUrl(cohortId: number | 'new', params: PersonListParams): string {
return `/api/cohort/${cohortId}/persons?${toParams(params)}`
},
async listPaginated(
params: {
limit?: number
offset?: number
search?: string
} = {}
): Promise<CountedPaginatedResponse<CohortType>> {
return await new ApiRequest().cohorts().withQueryString(toParams(params)).get()
},
},

dashboards: {
Original file line number Diff line number Diff line change
@@ -48,7 +48,7 @@ export function InsightLegendRow({ rowIndex, item }: InsightLegendRowProps): JSX
const formattedBreakdownValue = formatBreakdownLabel(
item.breakdown_value,
breakdownFilter,
cohorts,
cohorts.results,
formatPropertyValueForDisplay
)

Original file line number Diff line number Diff line change
@@ -68,7 +68,8 @@ export const taxonomicPropertyFilterLogic = kea<taxonomicPropertyFilterLogicType
],
selectedCohortName: [
(s) => [s.filter, cohortsModel.selectors.cohorts],
(filter, cohorts) => (filter?.type === 'cohort' ? cohorts.find((c) => c.id === filter?.value)?.name : null),
(filter, cohorts) =>
filter?.type === 'cohort' ? cohorts.results.find((c) => c.id === filter?.value)?.name : null,
],
activeTaxonomicGroup: [
(s) => [s.filter, s.taxonomicGroups],
Original file line number Diff line number Diff line change
@@ -247,7 +247,8 @@ export const infiniteListLogic = kea<infiniteListLogicType>([

if (group?.logic && group?.value) {
let items = group.logic.selectors[group.value]?.(state)
if (group?.value === 'featureFlags' && items.results) {
// Handle paginated responses for cohorts, which return a CountedPaginatedResponse
if (items?.results) {
items = items.results
}
// TRICKY: Feature flags don't support dynamic behavioral cohorts,
Original file line number Diff line number Diff line change
@@ -355,7 +355,7 @@ export const taxonomicFilterLogic = kea<taxonomicFilterLogicType>([
name: 'Cohorts',
searchPlaceholder: 'cohorts',
type: TaxonomicFilterGroupType.Cohorts,
logic: cohortsModel,
endpoint: combineUrl(`api/projects/${projectId}/cohorts/`).url,
value: 'cohorts',
getName: (cohort: CohortType) => cohort.name || `Cohort ${cohort.id}`,
getValue: (cohort: CohortType) => cohort.id,
214 changes: 214 additions & 0 deletions frontend/src/models/cohortsModel.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,214 @@
import { router } from 'kea-router'
import { expectLogic } from 'kea-test-utils'
import api from 'lib/api'
import { urls } from 'scenes/urls'

import { useMocks } from '~/mocks/jest'
import { initKeaTests } from '~/test/init'
import { CohortType, FilterLogicalOperator } from '~/types'

import { cohortsModel } from './cohortsModel'

const MOCK_COHORTS = {
count: 2,
results: [
{
id: 1,
name: 'Cohort one',
count: 1,
groups: [],
filters: {
properties: {
type: 'AND',
values: [],
},
},
is_calculating: false,
is_static: false,
created_at: '2023-08-01T00:00:00Z',
},
{
id: 2,
name: 'Cohort two',
count: 2,
groups: [],
filters: {
properties: {
type: 'AND',
values: [],
},
},
is_calculating: true,
is_static: false,
created_at: '2023-08-02T00:00:00Z',
},
],
}

describe('cohortsModel', () => {
let logic: ReturnType<typeof cohortsModel.build>

beforeEach(() => {
useMocks({
get: {
'/api/projects/:team/cohorts/': MOCK_COHORTS,
},
delete: {
'/api/projects/:team/cohorts/:id/': { success: true },
},
patch: {
'/api/projects/:team/cohorts/:id/': (req) => {
const data = req.body as Record<string, any>
return { ...MOCK_COHORTS.results[0], ...data }
},
},
})
initKeaTests()
logic = cohortsModel()
logic.mount()
})

describe('core assumptions', () => {
it('loads cohorts on mount', async () => {
await expectLogic(logic).toDispatchActions(['loadCohorts', 'loadCohortsSuccess'])
expect(logic.values.cohorts.results).toHaveLength(2)
expect(logic.values.cohortsWithAllUsers).toHaveLength(3) // includes "All Users"
})

it('sets polling timeout for calculating cohorts when on cohorts page', async () => {
// Set the current location to the cohorts page
router.actions.push(urls.cohorts())

await expectLogic(logic).toDispatchActions(['loadCohorts', 'loadCohortsSuccess'])
expect(logic.values.pollTimeout).not.toBeNull()
})

it('does not set polling timeout when not on cohorts page', async () => {
// Set the current location to a different page
router.actions.push(urls.dashboards())

// Mock API to return cohorts with no calculating ones
useMocks({
get: {
'/api/projects/:team/cohorts/': {
...MOCK_COHORTS,
results: MOCK_COHORTS.results.map((c) => ({ ...c, is_calculating: false })),
},
},
})

await expectLogic(logic).toDispatchActions(['loadCohorts', 'loadCohortsSuccess'])
expect(logic.values.pollTimeout).toBeNull()
})
})

describe('cohort filters', () => {
it('can set and update filters', async () => {
// Navigate to cohorts page first
router.actions.push(urls.cohorts())

// Wait for initial load
await expectLogic(logic).toDispatchActions(['loadCohortsSuccess'])

// Test search filter
await expectLogic(logic, () => {
logic.actions.setCohortFilters({ search: 'test' })
})
.toDispatchActions(['setCohortFilters', 'loadCohorts', 'loadCohortsSuccess'])
.toMatchValues({
cohortFilters: expect.objectContaining({ search: 'test' }),
})

// Test pagination
await expectLogic(logic, () => {
logic.actions.setCohortFilters({ page: 2 })
})
.toDispatchActions(['setCohortFilters', 'loadCohorts', 'loadCohortsSuccess'])
.toMatchValues({
cohortFilters: expect.objectContaining({ page: 2 }),
})
})
})

describe('cohort operations', () => {
it('can update a cohort', async () => {
// Wait for initial load
await expectLogic(logic).toDispatchActions(['loadCohortsSuccess'])

const updatedCohort: CohortType = {
id: 1,
name: 'Updated name',
count: 1,
groups: [],
filters: {
properties: {
type: FilterLogicalOperator.And,
values: [],
},
},
is_calculating: false,
is_static: false,
}

await expectLogic(logic, () => {
logic.actions.updateCohort(updatedCohort)
}).toMatchValues({
cohorts: expect.objectContaining({
results: expect.arrayContaining([
expect.objectContaining({
id: 1,
name: 'Updated name',
}),
]),
}),
})
})

it('can delete a cohort', async () => {
// Wait for initial load
await expectLogic(logic).toDispatchActions(['loadCohortsSuccess'])

jest.spyOn(api.cohorts, 'determineDeleteEndpoint').mockImplementation(() => 'cohorts')

await expectLogic(logic, () => {
logic.actions.deleteCohort({ id: 1 })
})
.toDispatchActions(['deleteCohort'])
.toMatchValues({
cohorts: expect.objectContaining({
results: expect.not.arrayContaining([
expect.objectContaining({
id: 1,
}),
]),
}),
})
})
})

describe('selectors', () => {
it('correctly calculates pagination values', async () => {
// Wait for initial load
await expectLogic(logic).toDispatchActions(['loadCohortsSuccess'])

await expectLogic(logic).toMatchValues({
pagination: expect.objectContaining({
currentPage: 1,
pageSize: 100,
entryCount: 2,
}),
})
})

it('correctly maps cohorts by id', async () => {
await expectLogic(logic)
.toDispatchActions(['loadCohortsSuccess'])
.toMatchValues({
cohortsById: expect.objectContaining({
1: expect.objectContaining({ id: 1, name: 'Cohort one' }),
2: expect.objectContaining({ id: 2, name: 'Cohort two' }),
}),
})
})
})
})
Loading
Loading