fix: add pagination to lead listing API - #670
Open
navashree9b11398 wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe lead listing API now returns paginated, newest-first results with configurable page sizes capped at 200. Existing organization and filter tests use the paginated response shape, and new tests cover pagination limits. ChangesLead listing pagination
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant LeadViewSet
participant LeadPagination
Client->>LeadViewSet: Request /api/v1/leads/
LeadViewSet->>LeadPagination: Paginate ordered lead queryset
LeadPagination-->>LeadViewSet: Return page results and metadata
LeadViewSet-->>Client: Return paginated response
Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
backend/leads/tests.py (1)
466-503: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd test for
page=2navigation and ordering verification.The pagination tests cover default page size,
page_sizeoverride, and max capping, but don't test actual page navigation or ordering. Issue#458mentionspage=2&page_size=100as a use case. Consider adding:
- A test requesting
?page=2and verifying it returns the remaining 10 leads (60 total, default page_size 50).- A test verifying
nextis non-null on page 1 andpreviousis null on page 1.- A test verifying results are ordered newest-first by
created_at.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/leads/tests.py` around lines 466 - 503, Extend LeadPaginationAPITests with coverage for page navigation and ordering: request page=2 with the default page size and assert the remaining 10 results, verify page 1 has a non-null next link and null previous link, and assert results are ordered newest-first by created_at. Reuse the existing self.leads fixtures and pagination response fields.backend/leads/views.py (1)
88-88: 🚀 Performance & Scalability | 🔵 Trivial
LeadSerializer.get_tagsissues a per-lead query (N+1).Even with pagination at 50 leads per page,
get_tagsrunsTag.objects.filter(tagged_leads__lead=obj)for each lead — 50 extra queries per page. Consider addingprefetch_relatedto the queryset to batch-fetch tags:from django.db.models import Prefetch qs = qs.prefetch_related( Prefetch('lead_tags__tag', queryset=Tag.objects.filter(organization=self.request.user.organization)) )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/leads/views.py` at line 88, Update the queryset construction before the final distinct/order_by return to prefetch the serializer’s lead tag relation using Prefetch, limiting the nested Tag queryset to self.request.user.organization. Reuse the existing lead_tags__tag relationship so LeadSerializer.get_tags can use prefetched data and avoid one query per lead.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/leads/tests.py`:
- Line 404: Rename the ambiguous comprehension variable `l` to `lead` in the
affected expressions throughout the tests, including the result-processing code
around the `emails` assignment and the corresponding lines at 411, 418, 428,
448, and 462, while preserving the existing behavior.
---
Nitpick comments:
In `@backend/leads/tests.py`:
- Around line 466-503: Extend LeadPaginationAPITests with coverage for page
navigation and ordering: request page=2 with the default page size and assert
the remaining 10 results, verify page 1 has a non-null next link and null
previous link, and assert results are ordered newest-first by created_at. Reuse
the existing self.leads fixtures and pagination response fields.
In `@backend/leads/views.py`:
- Line 88: Update the queryset construction before the final distinct/order_by
return to prefetch the serializer’s lead tag relation using Prefetch, limiting
the nested Tag queryset to self.request.user.organization. Reuse the existing
lead_tags__tag relationship so LeadSerializer.get_tags can use prefetched data
and avoid one query per lead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 13d28c7e-0f2a-4306-97d5-caa8fe2f25ad
📒 Files selected for processing (2)
backend/leads/tests.pybackend/leads/views.py
| resp = self._get(status='active') | ||
| self.assertEqual(resp.status_code, status.HTTP_200_OK) | ||
| emails = {l['email'] for l in resp.data} | ||
| emails = {l['email'] for l in resp.data['results']} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Ruff E741: ambiguous variable name l.
Ruff flags l as ambiguous (E741) on lines 404, 411, 418, 428, 448, and 462. Rename to lead for clarity and lint compliance.
🔧 Proposed fix
- emails = {l['email'] for l in resp.data['results']}
+ emails = {lead['email'] for lead in resp.data['results']}Also applies to: 411-411, 418-418, 428-428, 448-448, 462-462
🧰 Tools
🪛 Ruff (0.15.20)
[error] 404-404: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/leads/tests.py` at line 404, Rename the ambiguous comprehension
variable `l` to `lead` in the affected expressions throughout the tests,
including the result-processing code around the `emails` assignment and the
corresponding lines at 411, 418, 428, 448, and 462, while preserving the
existing behavior.
Source: Linters/SAST tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
🔗 Related Issue
Closes #458
📝 Summary of Changes
🏷️ Type of Change
🧪 Testing
Executed the lead test suite locally.
Command used: