Improve dashboard metrics list #1967
Conversation
Summary by CodeRabbit
WalkthroughThis change updates the backend and frontend to ensure the Project Health Metrics dashboard only displays active projects and removes duplicate projects by enforcing a secondary ordering by project name. The backend adds filtering for active projects and secondary ordering logic, while the frontend adapts its GraphQL queries to reflect these changes. Tests are updated accordingly. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
✨ Finishing Touches
🧪 Generate unit tests
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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![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/tests/apps/owasp/api/internal/filters/project_health_metrics_test.py (1)
24-27: Good test coverage, but consider additional test cases.The test correctly validates the
is_active=Truecase. However, consider adding test cases foris_active=Falseand edge cases to ensure comprehensive coverage of the boolean filter logic.Consider adding these additional test cases:
def test_filtering_inactive_projects(self): """Test filtering by inactive projects.""" filter_instance = ProjectHealthMetricsFilter(is_active=False) assert filter_instance.is_active is False def test_filtering_no_active_filter(self): """Test when is_active filter is not provided.""" filter_instance = ProjectHealthMetricsFilter(level="flagship", score=50) assert not hasattr(filter_instance, 'is_active') or filter_instance.is_active is None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/owasp/api/internal/filters/project_health_metrics.py(1 hunks)backend/apps/owasp/api/internal/ordering/project_health_metrics.py(2 hunks)backend/tests/apps/owasp/api/internal/filters/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/api/internal/ordering/project_health_metrics_test.py(1 hunks)frontend/src/app/projects/dashboard/metrics/page.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/GradientRadialChart.tsx:113-113
Timestamp: 2025-06-21T12:22:01.889Z
Learning: In the OWASP Nest project, health metrics requirements (like lastCommitDaysRequirement, lastReleaseDaysRequirement) should never be 0. A requirement value of 0 is considered invalid and should result in displaying 0 on the radial chart.
📚 Learning: in the detailscard component (frontend/src/components/carddetailspage.tsx), there's a length check b...
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a length check before rendering HealthMetrics: `healthMetricsData.length > 0`. This ensures that when HealthMetrics is rendered, the data array has at least one element, making accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/projects/dashboard/metrics/page.tsx
📚 Learning: in the detailscard component (frontend/src/components/carddetailspage.tsx), there's a safety check t...
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/HealthMetrics.tsx:30-30
Timestamp: 2025-06-20T16:12:59.256Z
Learning: In the DetailsCard component (frontend/src/components/CardDetailsPage.tsx), there's a safety check that ensures HealthMetrics component is only rendered when healthMetricsData exists and has at least one element: `healthMetricsData && healthMetricsData.length > 0`. This makes accessing data[0] safe within the HealthMetrics component.
Applied to files:
frontend/src/app/projects/dashboard/metrics/page.tsx
📚 Learning: in the owasp nest project's metricspage component (frontend/src/app/projects/dashboard/metrics/page....
Learnt from: ahmedxgouda
PR: OWASP/Nest#1718
File: frontend/src/app/projects/dashboard/metrics/page.tsx:96-96
Timestamp: 2025-07-09T08:37:10.241Z
Learning: In the OWASP Nest project's MetricsPage component (frontend/src/app/projects/dashboard/metrics/page.tsx), the sorting dropdown intentionally uses selectionMode="multiple" to allow users to select multiple sorting criteria simultaneously. This enables complex sorting scenarios where users can sort by multiple fields in sequence.
Applied to files:
frontend/src/app/projects/dashboard/metrics/page.tsx
🧬 Code Graph Analysis (2)
backend/tests/apps/owasp/api/internal/ordering/project_health_metrics_test.py (1)
backend/apps/owasp/api/internal/ordering/project_health_metrics.py (1)
ProjectHealthMetricsOrder(10-18)
backend/tests/apps/owasp/api/internal/filters/project_health_metrics_test.py (2)
backend/apps/owasp/api/internal/filters/project_health_metrics.py (3)
ProjectHealthMetricsFilter(12-27)level(20-22)is_active(25-27)backend/apps/owasp/models/enums/project.py (1)
ProjectLevel(27-34)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (10)
backend/apps/owasp/api/internal/ordering/project_health_metrics.py (2)
13-18: LGTM! Good solution for deterministic pagination.The addition of secondary ordering by
project__nameis a solid approach to ensure deterministic results when scores are equal. The comments clearly explain the rationale, and the implementation correctly uses Django's related field syntax.
3-3: Import change looks good.The change from importing individual
autoto importing the fullstrawberrymodule provides consistency and better namespace clarity.backend/tests/apps/owasp/api/internal/ordering/project_health_metrics_test.py (2)
18-19: LGTM! Test coverage for new ordering field.The updated test correctly validates that both
scoreandproject__namefields are present in the ordering definition.
23-25: LGTM! Comprehensive ordering instance testing.The test properly validates that both ordering fields can be instantiated and accessed correctly.
backend/tests/apps/owasp/api/internal/filters/project_health_metrics_test.py (1)
19-20: LGTM! Filter field validation updated correctly.The test properly validates that the new
is_activefield is included in the filter definition.frontend/src/app/projects/dashboard/metrics/page.tsx (5)
55-57: LGTM! Proper default filtering for active projects.Setting the default filter to include
isActive: truedirectly addresses the PR objective of excluding inactive projects from the dashboard by default.
66-69: LGTM! Filter preservation implemented correctly.The spread operator properly preserves existing filters (including
isActive) when applying health filters, ensuring the active-only behavior is maintained.
187-187: LGTM! Consistent reset behavior.The reset functionality correctly maintains the
isActive: truedefault, ensuring users always see only active projects even after resetting other filters.
105-110: LGTM! Secondary ordering for deterministic pagination.The addition of secondary ordering by
project_Nameascending aligns with the backend changes and ensures consistent, predictable pagination results when scores are equal.
257-262: LGTM! Consistent ordering in pagination.The fetchMore call correctly applies the same secondary ordering logic, ensuring consistent results across pagination operations.
backend/apps/owasp/api/internal/filters/project_health_metrics.py
Outdated
Show resolved
Hide resolved
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
It seems the filtering for active/inactive projects is available for the end-user. We don't want to expose inactive projects at all -- they just should not exist for a user is they are inactive.
| # to ensure unique metrics in pagination. | ||
| # because SQL returns random order if no order is specified. | ||
| # We didn't do this ordering in the model since we order already in the query. | ||
| project__name: strawberry.auto |
There was a problem hiding this comment.
Any reason for having __ in the name?
There was a problem hiding this comment.
strawberry use these attributes in the order_by method. project_name is not an attribute of the metrics but it is an attribute of the project. And like we access nested attributes in django methods, strawberry does the same.
| ordering: [ | ||
| ordering, | ||
| { | ||
| ['project_Name']: 'ASC', |
There was a problem hiding this comment.
What naming convention was used here?
There was a problem hiding this comment.
It is automatically converted by strawberry. Since the attribute is project__name, it is handled like other attributes. Just first underscore removed and capitalize the second name.
There was a problem hiding this comment.
I'm not sure how it works though. Let's merge it as is and improve sorting capabilities later.
|
2 participants
Proposed change
Resolves #1958
Now we should not have neither inactive projects nor duplicates
Checklist
make check-testlocally; all checks and tests passed.