fix: sorting audit by progress regression on pagination #1886
Conversation
WalkthroughThe changes refactor how the "progress" of a compliance assessment is calculated and accessed throughout the backend codebase. The Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ComplianceAssessmentViewSet
participant ComplianceAssessment (Model)
participant Database
Client->>ComplianceAssessmentViewSet: Request list/order by progress
ComplianceAssessmentViewSet->>Database: Annotate queryset with progress fields
Database-->>ComplianceAssessmentViewSet: Return annotated queryset
ComplianceAssessmentViewSet->>Client: Return serialized data (progress via get_progress)
sequenceDiagram
participant InternalFunction
participant ComplianceAssessment (Model)
InternalFunction->>ComplianceAssessment: Call get_progress()
ComplianceAssessment-->>InternalFunction: Return progress percentage (int)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/core/views.py (1)
4039-4040: Note about static analysis warning.The static analysis tool flagged a potential undefined
RequirementAssessment.Result.NOT_ASSESSED, but this appears to be a false positive as the class is indeed available in this context.Consider making the import of
RequirementAssessmentexplicit rather than relying on star imports to address the static analysis warning.🧰 Tools
🪛 Ruff (0.8.2)
4039-4039:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/core/helpers.py(1 hunks)backend/core/models.py(2 hunks)backend/core/serializers.py(1 hunks)backend/core/views.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/core/serializers.py
- backend/core/models.py
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/core/helpers.py
930-930: ComplianceAssessment may be undefined, or defined from star imports
(F405)
backend/core/views.py
4039-4039: RequirementAssessment may be undefined, or defined from star imports
(F405)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
🔇 Additional comments (5)
backend/core/views.py (5)
11-19: Good addition of Django ORM utility imports.These imports (
Valuefromdjango.db.modelsandGreatest,Coalescefromdjango.db.models.functions) are necessary for the database-level progress calculations implemented in theComplianceAssessmentViewSet.get_queryset()method.
188-188: Good addition of return type hint.Adding the return type hint to
BaseModelViewSet.get_queryset()improves code clarity and helps with static type checking.
2444-2444: Correctly switched from property access to method call.This change from
audit.progresstoaudit.get_progress()aligns with the refactoring of the progress calculation approach in theComplianceAssessmentmodel, ensuring consistent access patterns throughout the codebase.
2648-2648: Improved string efficiency.Removed unnecessary f-string for a static string, which is a minor but worthwhile optimization.
4017-4052: Excellent implementation of database-level progress calculation.This new implementation properly addresses the issue of sorting by progress fields when paginating results. Key improvements:
- Progress calculation is now done at the database level instead of Python, which is much more efficient
- Annotations are conditionally applied only when needed for ordering
- The implementation carefully handles division by zero scenarios using
GreatestandCoalesce- Proper use of
distinct=Truein theCountannotations prevents duplicate countingThis change will significantly improve performance when sorting large datasets by progress-related fields.
🧰 Tools
🪛 Ruff (0.8.2)
4039-4039:
RequirementAssessmentmay be undefined, or defined from star imports(F405)
Summary by CodeRabbit
Bug Fixes
Refactor
Performance