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

Open
wants to merge 12 commits into
base: master
Choose a base branch
from

Conversation

dmarticus
Copy link
Contributor

@dmarticus dmarticus commented Jan 9, 2025

Problem

We stopped showing cohort names for cohorts past the 600th index because we couldn't fetch them without cohorts logic: https://posthog.slack.com/archives/C07Q2U4BH4L/p1734442912694879

Fix

Paginate cohorts, now we can return all of them if needed

Demo

https://www.loom.com/share/ec8c6d73952c4042849cfdab23a9dc69

@dmarticus dmarticus marked this pull request as draft January 9, 2025 23:07
Copy link
Contributor

github-actions bot commented Jan 9, 2025

Size Change: +7 B (0%)

Total Size: 1.13 MB

ℹ️ View Unchanged
Filename Size Change
frontend/dist/toolbar.js 1.13 MB +7 B (0%)

compressed-size-action

@posthog-bot
Copy link
Contributor

📸 UI snapshots have been updated

2 snapshot changes in total. 0 added, 2 modified, 0 deleted:

  • chromium: 0 added, 2 modified, 0 deleted (diff for shard 1)
  • webkit: 0 added, 0 modified, 0 deleted

Triggered by this commit.

👉 Review this PR's diff of snapshots.

@@ -142,7 +142,7 @@ export function Cohorts(): JSX.Element {
productKey={ProductKey.COHORTS}
thingName="cohort"
description="Use cohorts to group people together, such as users who used your app in the last week, or people who viewed the signup page but didn’t convert."
isEmpty={cohorts?.length == 0 && !cohortsLoading}
isEmpty={cohorts.count == 0 && !cohortsLoading && !searchTerm}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to show the "create new cohort" option if the cohort list has filters applied

Comment on lines +166 to +167
pagination={pagination}
dataSource={cohorts.results}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

the pagination and data are now controlled exclusively by the logic, and not some mix of react state management and the logic. And now pagination actually works (it didn't beforehand 😓 )

Comment on lines +311 to +313
search_query = self.request.query_params.get("search", None)
if search_query:
queryset = queryset.filter(name__icontains=search_query)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

added search functionality to this endpoint using the built-in django ORM search.

@dmarticus dmarticus marked this pull request as ready for review January 14, 2025 01:16
@@ -1506,16 +1506,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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants