feat: implement lead status sorting#633
Open
kreeti22 wants to merge 1 commit into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughThe backend LeadViewSet now supports ordering via DRF's OrderingFilter, whitelisting fields including a new annotated ChangesStatus Sorting Feature
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant LeadsPage
participant LeadViewSet
User->>LeadsPage: click sort-status-btn
LeadsPage->>LeadsPage: toggle statusSortDir, show spinner
LeadsPage->>LeadViewSet: fetchLeadsWithFilters(activeFilters, ordering)
LeadViewSet-->>LeadsPage: sorted leads (status annotation)
LeadsPage->>LeadsPage: renderLeadRows(allLeads)
Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
🧹 Nitpick comments (1)
backend/leads/views.py (1)
86-88: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAnnotate
statusonly when ordering by statusThe annotation is applied unconditionally on every list request, adding a JOIN to the
campaignleadtable even when the client isn't sorting by status. Consider applying it only whenordering=statusis requested to avoid unnecessary JOINs on the hot path.♻️ Suggested conditional annotation
# Annotate the queryset with the status from the related campaignlead - qs = qs.annotate(status=F('campaignlead__status')) + ordering = self.request.query_params.get('ordering', '') + if 'status' in ordering: + qs = qs.annotate(status=F('campaignlead__status'))🤖 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` around lines 86 - 88, The queryset in the leads list view is always annotated with campaignlead status, which forces an unnecessary join on every request. Update the list/query construction in the relevant view method that uses qs.annotate(status=F('campaignlead__status')) so the annotation is applied only when the requested ordering is by status (for example, when ordering=status is present), and leave the queryset unmodified for other orderings.
🤖 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 `@frontend/leads.html`:
- Around line 906-912: The fetchLeadsWithFilters error path in the sorting flow
only logs to the console, leaving the loading spinner in place and giving no
recovery behavior. Update the catch block around renderLeadRows so it restores
the table to a usable state on failure—either by re-rendering the previous leads
already held in allLeads or by clearing/replacing the spinner with an
error/empty state—while keeping the existing console.error for debugging.
- Around line 367-369: The Status table header in the leads view is only
mouse-clickable, so make the sortable `<th>` accessible by adding keyboard focus
and semantics in the status header markup and wiring keyboard activation in the
existing sort handler area. Move the shared sort behavior into a
`toggleStatusSort` function, then call it from both the current click handler
and a new keydown handler that responds to Enter and Space so keyboard and
screen reader users can trigger sorting.
---
Nitpick comments:
In `@backend/leads/views.py`:
- Around line 86-88: The queryset in the leads list view is always annotated
with campaignlead status, which forces an unnecessary join on every request.
Update the list/query construction in the relevant view method that uses
qs.annotate(status=F('campaignlead__status')) so the annotation is applied only
when the requested ordering is by status (for example, when ordering=status is
present), and leave the queryset unmodified for other orderings.
🪄 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: 836073bb-35fb-4be1-8f57-cc2a3232209d
📒 Files selected for processing (3)
.gitignorebackend/leads/views.pyfrontend/leads.html
Comment on lines
+367
to
+369
| <th id="sort-status-btn" style="cursor: pointer;" class="user-select-none text-primary" title="Sort by Status"> | ||
| Status <i class="bi bi-arrow-down-up ms-1" style="font-size: 0.8rem;"></i> | ||
| </th> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Status header is not keyboard accessible
The clickable <th> has no tabindex, role, or keyboard event handler. Keyboard and screen reader users cannot activate the sort.
♿ Proposed accessibility fix
- <th id="sort-status-btn" style="cursor: pointer;" class="user-select-none text-primary" title="Sort by Status">
+ <th id="sort-status-btn" style="cursor: pointer;" class="user-select-none text-primary" role="button" tabindex="0" aria-label="Sort by Status" title="Sort by Status">And add a keyboard handler alongside the click handler (around line 890):
document.getElementById('sort-status-btn').addEventListener('click', async () => {
+ await toggleStatusSort();
+ });
+ document.getElementById('sort-status-btn').addEventListener('keydown', async (e) => {
+ if (e.key === 'Enter' || e.key === ' ') {
+ e.preventDefault();
+ await toggleStatusSort();
+ }
});Extract the sort logic into a shared toggleStatusSort function to avoid duplication.
🤖 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 `@frontend/leads.html` around lines 367 - 369, The Status table header in the
leads view is only mouse-clickable, so make the sortable `<th>` accessible by
adding keyboard focus and semantics in the status header markup and wiring
keyboard activation in the existing sort handler area. Move the shared sort
behavior into a `toggleStatusSort` function, then call it from both the current
click handler and a new keydown handler that responds to Enter and Space so
keyboard and screen reader users can trigger sorting.
Comment on lines
+906
to
+912
| try { | ||
| const leads = await fetchLeadsWithFilters(params); | ||
| allLeads = leads; | ||
| renderLeadRows(leads); | ||
| } catch (e) { | ||
| console.error('Error sorting leads by status:', e); | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Error handling leaves spinner stuck
On fetch failure, only console.error is called. The spinner HTML injected at line 904 remains in the table body indefinitely with no user feedback or recovery path.
🛡️ Proposed fix to restore previous data on error
} catch (e) {
console.error('Error sorting leads by status:', e);
+ renderLeadRows(allLeads);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try { | |
| const leads = await fetchLeadsWithFilters(params); | |
| allLeads = leads; | |
| renderLeadRows(leads); | |
| } catch (e) { | |
| console.error('Error sorting leads by status:', e); | |
| } | |
| try { | |
| const leads = await fetchLeadsWithFilters(params); | |
| allLeads = leads; | |
| renderLeadRows(leads); | |
| } catch (e) { | |
| console.error('Error sorting leads by status:', e); | |
| renderLeadRows(allLeads); | |
| } |
🤖 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 `@frontend/leads.html` around lines 906 - 912, The fetchLeadsWithFilters error
path in the sorting flow only logs to the console, leaving the loading spinner
in place and giving no recovery behavior. Update the catch block around
renderLeadRows so it restores the table to a usable state on failure—either by
re-rendering the previous leads already held in allLeads or by
clearing/replacing the spinner with an error/empty state—while keeping the
existing console.error for debugging.
Author
|
@Kuldeeep18, The implementation for lead status sorting is ready and passing all checks. Looking forward to your feedback for the final merge! |
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 # issue #557
📝 Summary of Changes
Added a status sorting toggle to the lead table to improve workflow efficiency
🏷️ Type of Change
🧪 Testing
Steps to test:
1.
2.
3.
📸 Screenshots (if applicable)
✅ Checklist
Summary by CodeRabbit
New Features
Bug Fixes