Add button to download pdf detailed metrics #1757
Conversation
Summary by CodeRabbit
""" WalkthroughThis change introduces end-to-end support for downloading detailed health metrics PDFs for individual projects. It removes the old Django management command and its test, adds a backend API endpoint and PDF generation utility, and updates the frontend to use a reusable PDF download button for both overview and project-specific reports. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested labels
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 (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (5)
⏰ 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)
✨ 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. 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
Documentation and Community
|
github-actions
bot
added
frontend
docker
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🔭 Outside diff range comments (2)
backend/apps/common/management/commands/purge_data.py (1)
23-23: Fix hardcoded "GitHub" string in print statement.The print statement always displays "GitHub" regardless of which app is being processed, making logs confusing and incorrect for non-GitHub apps.
Apply this diff to fix the issue:
- print(f"Purged GitHub {model._meta.verbose_name_plural}") + print(f"Purged {nest_app} {model._meta.verbose_name_plural}")backend/tests/apps/common/management/commands/purge_data_test.py (1)
51-51: Fix hardcoded "GitHub" string in test assertion.The test assertion hardcodes "GitHub" regardless of which app is being tested, making it test the incorrect behavior that exists in the main command.
Apply this diff to fix the test:
- mock_print.assert_any_call(f"Purged GitHub {model_name}s") + mock_print.assert_any_call(f"Purged {app_name} {model_name}s")
🧹 Nitpick comments (23)
backend/apps/nest/graphql/nodes/api_key.py (1)
8-19: Secure field selection with good GraphQL patterns.The
ApiKeyNodeimplementation correctly excludes sensitive fields likehashwhile exposing essential information. The field selection aligns well with the frontend TypeScript interface.Consider whether
last_used_atshould be included to help users track API key usage, though the current exclusion may be intentional for privacy reasons.backend/apps/sitemap/views/member.py (1)
12-14: Consider optimizing the filtering for better performance.The current implementation loads all non-bot users into memory before filtering by
is_indexable. For better performance with large user counts, consider moving theis_indexablefilter to the database level if possible.def items(self): """Return list of members for sitemap generation.""" - return [u for u in User.objects.filter(is_bot=False) if u.is_indexable] + # If is_indexable can be converted to a database filter: + # return User.objects.filter(is_bot=False, <indexable_conditions>) + return [u for u in User.objects.filter(is_bot=False) if u.is_indexable]The current approach works correctly but may not scale optimally.
backend/apps/sitemap/views/committee.py (1)
12-14: Consider database-level filtering optimization.Similar to the
MemberSitemap, this implementation loads all active committees before filtering byis_indexable. Consider optimizing by moving the indexable filter to the database level if theis_indexableproperty can be converted to a database query condition.def items(self): """Return list of committees for sitemap generation.""" - return [c for c in Committee.objects.filter(is_active=True) if c.is_indexable] + # If is_indexable can be converted to a database filter: + # return Committee.objects.filter(is_active=True, <indexable_conditions>) + return [c for c in Committee.objects.filter(is_active=True) if c.is_indexable]backend/apps/sitemap/views/chapter.py (1)
12-14: Consider database-level filtering optimization.The current filtering approach works correctly. If
is_indexablecould be expressed as a database query rather than a Python property, it would improve performance for large datasets.def items(self): """Return list of chapters for sitemap generation.""" - return [c for c in Chapter.objects.filter(is_active=True) if c.is_indexable] + # Consider moving is_indexable logic to database query if possible + return Chapter.objects.filter(is_active=True, <indexable_conditions>)However, if
is_indexableinvolves complex computed logic, the current approach is acceptable.backend/apps/sitemap/views/project.py (1)
12-14: Consistent filtering pattern across sitemap classes.The implementation matches the pattern used in other sitemap classes, ensuring consistency. The same optimization opportunity exists here as in
ChapterSitemapregarding database-level filtering foris_indexableif feasible.backend/tests/apps/sitemap/views/member_test.py (1)
7-14: Add test coverage for filtering logic.The test correctly verifies the basic functionality but could benefit from testing the filtering behavior more thoroughly.
Consider adding a test case that verifies objects with
is_indexable=Falseare filtered out:@patch("apps.sitemap.views.member.User") def test_items_filters_non_indexable(self, mock_user): indexable_obj = MagicMock(is_indexable=True) non_indexable_obj = MagicMock(is_indexable=False) mock_user.objects.filter.return_value = [indexable_obj, non_indexable_obj] sitemap = MemberSitemap() result = list(sitemap.items()) assert result == [indexable_obj] assert non_indexable_obj not in resultfrontend/src/components/BarChart.tsx (1)
84-91: Apply optional chaining for cleaner code.The static analysis tool correctly identified an opportunity to use optional chaining.
Apply this diff to use optional chaining:
- if (reverseColors && reverseColors[dataPointIndex]) { + if (reverseColors?.[dataPointIndex]) {frontend/__tests__/unit/data/mockApiKeysData.ts (1)
1-34: Well-structured mock data for API key testing.The mock data structure appropriately covers both API key listing and creation scenarios. The inclusion of both active and potentially expiring keys provides good test coverage for different API key states.
Consider adding a revoked API key example to the mock data to test the full range of API key states:
{ uuid: '2', name: 'mock key 2', isRevoked: false, createdAt: '2025-07-11T07:36:44.115179+00:00', expiresAt: '2025-07-12T00:00:00+00:00', }, + { + uuid: '3', + name: 'revoked key', + isRevoked: true, + createdAt: '2025-07-10T07:36:44.115179+00:00', + expiresAt: null, + },backend/apps/core/api/ninja.py (1)
11-33: Well-implemented API key authentication with minor docstring fix needed.The authentication logic is sound with proper error handling and integration with the
ApiKeymodel. The use of the walrus operator for the authentication check is clean and efficient.Fix the return type annotation in the docstring:
Returns: - APIKey: The APIKey object if the key is valid, otherwise None. + ApiKey: The ApiKey object if the key is valid, otherwise None.frontend/src/components/GeneralCompliantComponent.tsx (1)
15-16: Minor redundancy in CSS classes.There's a redundant
dark:hover:bg-red-600class in theredClassdefinition - it appears twice in the same string.- const redClass = 'bg-red-100 dark:bg-red-700 hover:bg-red-600 dark:hover:bg-red-600' + const redClass = 'bg-red-100 dark:bg-red-700 hover:bg-red-200 dark:hover:bg-red-600'backend/tests/apps/sitemap/urls_test.py (1)
21-31: Consider using more explicit URL pattern matching.While the current approach works, accessing the private
_routeattribute might be fragile. Consider using Django'sreversefunction to test URL resolution instead.- found_paths = {p.pattern._route for p in sitemap_urls.urlpatterns} - for path in expected_paths: - assert path in found_paths + from django.urls import reverse + + # Test that each sitemap URL can be resolved + expected_url_names = [ + "sitemap", + "sitemap-chapters", + "sitemap-committees", + "sitemap-projects", + "sitemap-static", + "sitemap-members" + ] + for url_name in expected_url_names: + try: + reverse(f"sitemap:{url_name}") + except Exception as e: + assert False, f"URL {url_name} not properly registered: {e}"backend/apps/sitemap/urls.py (1)
27-46: Consider refactoring repetitive URL patterns.While the individual sitemap endpoints are correctly implemented, the repetitive pattern could be refactored for better maintainability.
Consider using a loop to generate the individual sitemap URL patterns:
+# Individual sitemap URLs +for section, sitemap_class in sitemaps.items(): + urlpatterns.append( + path( + f"sitemap/{section}.xml", + cached_sitemap_view(sitemaps={section: sitemap_class}), + ) + ) - path( - "sitemap/chapters.xml", - cached_sitemap_view(sitemaps={"chapters": ChapterSitemap}), - ), - path( - "sitemap/committees.xml", - cached_sitemap_view(sitemaps={"committees": CommitteeSitemap}), - ), - path( - "sitemap/members.xml", - cached_sitemap_view(sitemaps={"members": MemberSitemap}), - ), - path( - "sitemap/projects.xml", - cached_sitemap_view(sitemaps={"projects": ProjectSitemap}), - ), - path( - "sitemap/static.xml", - cached_sitemap_view(sitemaps={"static": StaticSitemap}), - ),frontend/src/components/skeletons/ApiKeySkelton.tsx (1)
53-74: Consider adding accessibility improvements.While the skeleton implementation is solid, consider adding accessibility enhancements for screen readers.
Add ARIA labels for better accessibility:
<table className="w-full border-collapse"> + <caption className="sr-only">Loading API keys data</caption> <thead> <tr className="border-b border-gray-200 dark:border-gray-700">And consider adding
aria-busy="true"to the main container:-<div className="flex min-h-[80vh] flex-col items-center p-8"> +<div className="flex min-h-[80vh] flex-col items-center p-8" aria-busy="true" aria-label="Loading API keys">frontend/__tests__/e2e/pages/ProjectsHealthDashboardOverview.spec.ts (1)
28-61: Consider using more specific selectors for better test reliability.While the current text-based assertions work, using more specific selectors could improve test reliability and maintainability.
Consider using data-testid attributes or more specific selectors:
-await expect( - page.getByText( - mockProjectsDashboardOverviewData.projectHealthStats.projectsCountHealthy.toString() - ) -).toBeVisible() +await expect( + page.getByTestId('healthy-projects-count') +).toHaveText( + mockProjectsDashboardOverviewData.projectHealthStats.projectsCountHealthy.toString() +)This approach would require adding
data-testidattributes to the dashboard components but would make the tests more robust against text changes.frontend/src/components/MetricsScoreCircle.tsx (1)
3-26: Consider adding accessibility attributes for better screen reader support.The component implementation is solid with good visual design and responsive behavior. However, consider adding ARIA attributes to improve accessibility.
<div - className={`group relative flex h-20 w-20 flex-col items-center justify-center rounded-full border-2 shadow-lg transition-all duration-300 hover:scale-105 hover:shadow-xl ${scoreStyle}`} + className={`group relative flex h-20 w-20 flex-col items-center justify-center rounded-full border-2 shadow-lg transition-all duration-300 hover:scale-105 hover:shadow-xl ${scoreStyle}`} + role="img" + aria-label={`Health score: ${score} out of 100`} >backend/tests/apps/nest/graphql/queries/api_keys_test.py (1)
8-16: Consider consolidating duplicate mock_info functions.The
mock_infofunction here is nearly identical to the one inbackend/tests/apps/nest/graphql/mutations/api_key_test.py(lines 17-25), with only minor docstring differences. Consider extracting this to a shared test utility module to reduce code duplication.# In a shared test utilities module (e.g., tests/utils.py) def create_mock_graphql_info(user_pk: int = 1, is_authenticated: bool = True): """Return a mocked GraphQL Info object with user context.""" info = MagicMock() info.context = MagicMock() info.context.request = MagicMock() info.context.request.user = MagicMock() info.context.request.user.is_authenticated = is_authenticated info.context.request.user.pk = user_pk return infofrontend/src/app/settings/api-keys/page.tsx (1)
93-100: Consider using a more precise regex patternThe current regex pattern
/[^a-zA-Z0-9\s-]/uses negation to check for invalid characters. Consider using a positive assertion pattern for better clarity and maintainability:-if (newKeyName.match(/[^a-zA-Z0-9\s-]/)) { +if (!newKeyName.match(/^[a-zA-Z0-9\s-]+$/)) {This ensures the entire string consists only of allowed characters and prevents edge cases with empty strings.
backend/apps/nest/models/api_key.py (1)
11-13: Consider adding documentation for these constants.The constants lack inline documentation explaining their purpose and rationale.
-API_KEY_LENGTH = 32 -MAX_ACTIVE_KEYS = 3 -MAX_WORD_LENGTH = 100 +API_KEY_LENGTH = 32 # Length of the raw API key in bytes (256 bits of entropy) +MAX_ACTIVE_KEYS = 3 # Maximum number of active API keys per user +MAX_WORD_LENGTH = 100 # Maximum length for API key namesfrontend/__tests__/unit/pages/ApiKeysPage.test.tsx (1)
150-176: Verify date handling in API key creation test.The test uses
addDaysandformatfromdate-fnsbut the assertion usesexpect.any(Date). This might miss date formatting issues.Consider making the date assertion more specific:
expect(mockCreateMutation).toHaveBeenCalledWith({ variables: expect.objectContaining({ name: expectedVariables.name, - expiresAt: expect.any(Date), + expiresAt: expectedVariables.expiresAt, }), })backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py (1)
41-42: Consider adding PDF metadata.The PDF should include additional metadata for better document management.
buffer = BytesIO() pdf = canvas.Canvas(buffer, pagesize=letter) pdf.setTitle(f"Health Metrics Report for {metrics.project.name}") + pdf.setAuthor("OWASP Nest") + pdf.setSubject("Project Health Metrics") + pdf.setCreator("OWASP Nest Management Command")frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx (1)
59-74: Review header layout structureThe header contains three elements (title, button, and score circle) with
justify-between, which may not provide ideal spacing. Consider restructuring for better alignment.-<div className="flex items-center justify-between"> - <h1 className="text-2xl font-bold">{metrics.projectName}</h1> - <Button - variant="solid" - color="primary" - onPress={async () => { - await fetchMetricsPDF( - `owasp/project-health-metrics/${projectKey}/pdf`, - `${projectKey}-health-metrics.pdf` - ) - }} - > - Download PDF - </Button> - <MetricsScoreCircle score={metrics.score} /> -</div> +<div className="flex items-center justify-between"> + <div className="flex items-center gap-4"> + <h1 className="text-2xl font-bold">{metrics.projectName}</h1> + <MetricsScoreCircle score={metrics.score} /> + </div> + <Button + variant="solid" + color="primary" + onPress={async () => { + await fetchMetricsPDF( + `owasp/project-health-metrics/${projectKey}/pdf`, + `${projectKey}-health-metrics.pdf` + ) + }} + > + Download PDF + </Button> +</div>backend/apps/nest/graphql/mutations/user.py (2)
18-25: Consider removing unusedcodefieldThe
codefield inLogoutResultis never set in the implementation. Consider removing it if not needed, or document its intended use for future implementations.
40-94: Well-implemented authentication flow with robust error handlingThe enhanced GitHub authentication implementation properly validates the user type, checks for a verified primary email, and provides clear error messages for different failure scenarios. The use of Django's session-based authentication is appropriate.
Minor suggestion for the email retrieval logic at lines 47-49: Consider extracting this into a helper method for better readability:
- gh_user_email = next( - (e.email for e in gh_user.get_emails() if e.primary and e.verified), "" - ) + gh_user_email = self._get_primary_verified_email(gh_user)
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py
Outdated
Show resolved
Hide resolved
frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
Outdated
Show resolved
Hide resolved
ahmedxgouda
force-pushed
the
dashboard/pdf-metrics-details-client
branch
from
93f8bb1 to
cbefe73
Compare
github-actions
bot
added
backend
and removed
docker
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
frontend/src/components/BarChart.tsx (1)
84-91: Simplify the condition using optional chaining.The static analysis tool correctly identified an opportunity to use optional chaining for cleaner code.
- if (reverseColors && reverseColors[dataPointIndex]) { + if (reverseColors?.[dataPointIndex]) {frontend/src/server/queries/projectsHealthDashboardQueries.ts (1)
21-53: LGTM! Comprehensive query structure for detailed health metrics.The new
GET_PROJECT_HEALTH_METRICS_DETAILSquery is well-structured and follows GraphQL best practices. The field selection is comprehensive, covering all essential health metrics including compliance flags, temporal data, counts, and scores necessary for the PDF generation feature.The query properly:
- Uses required parameter typing (
String!)- Follows consistent camelCase naming
- Retrieves latest health metrics via
healthMetricsLatest- Includes all fields mentioned in the AI summary for backend integration
Consider grouping related fields with comments for better maintainability:
export const GET_PROJECT_HEALTH_METRICS_DETAILS = gql` query Project($projectKey: String!) { project(key: $projectKey) { healthMetricsLatest { + # Basic project info ageDays ageDaysRequirement createdAt projectName score + + # Repository statistics contributorsCount forksCount starsCount + + # Activity metrics lastCommitDays lastCommitDaysRequirement lastPullRequestDays lastPullRequestDaysRequirement lastReleaseDays lastReleaseDaysRequirement owaspPageLastUpdateDays owaspPageLastUpdateDaysRequirement + + # Issue and PR tracking openIssuesCount openPullRequestsCount totalIssuesCount unassignedIssuesCount unansweredIssuesCount + + # Release information recentReleasesCount totalReleasesCount + + # Compliance flags isFundingRequirementsCompliant isLeaderRequirementsCompliant } } } `
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (28)
.gitignore(1 hunks)backend/apps/owasp/Makefile(1 hunks)backend/apps/owasp/api/v1/project_health_metrics.py(1 hunks)backend/apps/owasp/api/v1/urls.py(1 hunks)backend/apps/owasp/graphql/nodes/project.py(1 hunks)backend/apps/owasp/graphql/nodes/project_health_metrics.py(4 hunks)backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py(1 hunks)backend/apps/owasp/models/project_health_metrics.py(3 hunks)backend/pyproject.toml(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(2 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(1 hunks)backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)frontend/__tests__/unit/data/mockProjectDetailsData.ts(1 hunks)frontend/__tests__/unit/data/mockProjectsDashboardMetricsDetailsData.ts(1 hunks)frontend/src/app/projects/[projectKey]/page.tsx(1 hunks)frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx(1 hunks)frontend/src/components/BarChart.tsx(2 hunks)frontend/src/components/CardDetailsPage.tsx(2 hunks)frontend/src/components/DashboardCard.tsx(1 hunks)frontend/src/components/GeneralCompliantComponent.tsx(1 hunks)frontend/src/components/MetricsScoreCircle.tsx(1 hunks)frontend/src/server/fetchMetricsPDF.ts(1 hunks)frontend/src/server/queries/projectQueries.ts(1 hunks)frontend/src/server/queries/projectsHealthDashboardQueries.ts(1 hunks)frontend/src/types/healthMetrics.ts(1 hunks)frontend/src/types/project.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/tests/unit/data/mockProjectsDashboardMetricsDetailsData.ts
- frontend/src/components/GeneralCompliantComponent.tsx
🚧 Files skipped from review as they are similar to previous changes (20)
- .gitignore
- backend/pyproject.toml
- frontend/src/app/projects/[projectKey]/page.tsx
- cspell/custom-dict.txt
- frontend/tests/unit/data/mockProjectDetailsData.ts
- backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py
- backend/apps/owasp/api/v1/urls.py
- backend/apps/owasp/Makefile
- backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py
- frontend/src/components/CardDetailsPage.tsx
- frontend/src/components/DashboardCard.tsx
- backend/apps/owasp/graphql/nodes/project.py
- backend/apps/owasp/api/v1/project_health_metrics.py
- backend/tests/apps/owasp/graphql/nodes/project_test.py
- frontend/src/components/MetricsScoreCircle.tsx
- backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_test.py
- backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py
- frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
- backend/apps/owasp/models/project_health_metrics.py
- backend/apps/owasp/graphql/nodes/project_health_metrics.py
🧰 Additional context used
🧠 Learnings (5)
frontend/src/server/queries/projectQueries.ts (2)
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.
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.
frontend/src/types/healthMetrics.ts (4)
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.
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.
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.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
frontend/src/server/queries/projectsHealthDashboardQueries.ts (2)
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.
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.
frontend/src/types/project.ts (2)
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.
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.
frontend/src/components/BarChart.tsx (3)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1703
File: frontend/src/components/BarChart.tsx:33-46
Timestamp: 2025-07-03T03:08:03.290Z
Learning: In the OWASP Nest project's BarChart component (frontend/src/components/BarChart.tsx), the days and requirements arrays are guaranteed to always have the same length in their use cases, so input validation for array length matching is not needed.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1633
File: frontend/src/components/GradientRadialChart.tsx:67-116
Timestamp: 2025-06-21T12:21:32.372Z
Learning: The react-apexcharts Chart component does not support ARIA attributes like aria-label and role as direct props. To add accessibility attributes to ApexCharts in React, wrap the Chart component in a container div with the appropriate ARIA attributes.
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.
🧬 Code Graph Analysis (2)
frontend/src/server/fetchMetricsPDF.ts (2)
frontend/src/utils/credentials.ts (1)
API_URL(1-1)frontend/src/app/global-error.tsx (1)
handleAppError(66-86)
frontend/src/types/project.ts (1)
frontend/src/types/healthMetrics.ts (1)
HealthMetricsProps(26-52)
🪛 Biome (1.9.4)
frontend/src/components/BarChart.tsx
[error] 84-84: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
⏰ 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). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
frontend/src/server/fetchMetricsPDF.ts (1)
15-31: LGTM! Proper blob download implementation.The blob handling, URL creation, download triggering, and cleanup logic follows best practices for browser file downloads. Good error handling with the global error handler integration.
frontend/src/types/project.ts (1)
22-22: LGTM! Property rename aligns with backend changes.The rename from
healthMetricstohealthMetricsListmaintains type safety while aligning with the backend GraphQL schema updates mentioned in the AI summary.frontend/src/server/queries/projectQueries.ts (1)
15-15: LGTM! GraphQL query field rename is consistent.The rename from
healthMetricstohealthMetricsListmaintains query functionality while aligning with the backend schema and frontend type changes.frontend/src/components/BarChart.tsx (1)
20-21: LGTM! Well-designed optional prop for color inversion.The
reverseColorsprop provides flexible color logic for different metric visualization needs while maintaining backward compatibility.frontend/src/types/healthMetrics.ts (1)
27-52: LGTM! Comprehensive type extension for detailed metrics.The new properties are well-typed and consistently named, supporting the enhanced health metrics functionality. The additions align with the backend model extensions and provide good type safety for the new dashboard features.
ahmedxgouda
force-pushed
the
dashboard/pdf-metrics-details-client
branch
from
cbefe73 to
04306e3
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/apps/owasp/api/rest/v1/project_health_metrics.py (2)
30-30: Consider using context manager for file handling.The current implementation opens the file via
Path.open()withinFileResponse, but using a context manager would make the resource management more explicit.Consider this approach for better resource management:
- return FileResponse(Path.open(pdf_path, "rb"), as_attachment=True, filename=file_name) + return FileResponse(pdf_path.open("rb"), as_attachment=True, filename=file_name)Note: Django's
FileResponsehandles file closing automatically, so the current implementation is actually safe, but usingpdf_path.open()is more idiomatic.
16-30: Consider architectural improvements for production readiness.The current implementation has several areas that could be improved for better performance and maintainability:
- Asynchronous processing: PDF generation is synchronous and could be slow for large datasets
- File cleanup: Generated PDFs will accumulate over time with no cleanup mechanism
- Caching: No mechanism to avoid regenerating the same PDF repeatedly
- Concurrency: Multiple requests for the same project could cause race conditions
Consider implementing:
- A background task queue (Celery) for PDF generation
- A cleanup job to remove old PDF files
- Caching mechanism based on metrics data freshness
- File locking or unique temporary filenames to handle concurrent requests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/api/rest/v1/project_health_metrics.py(1 hunks)backend/apps/owasp/api/rest/v1/urls.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/owasp/api/rest/v1/urls.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/owasp/api/rest/v1/project_health_metrics.py (1)
1-13: LGTM! Clean imports and proper setup.The imports are well-organized and appropriate for the functionality. Using
pathlib.Pathis a good choice for cross-platform file handling.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/apps/owasp/api/rest/v1/project_health_metrics.py (1)
12-14: Add input validation for project_key parameter.The
project_keyparameter should be validated to prevent potential security issues like path traversal attacks or injection vulnerabilities.Consider adding validation like this:
+import re + @router.get("/{project_key}/pdf") def get_project_health_metrics_pdf(request, project_key: str): """Generate and return a PDF report of project health metrics.""" + # Validate project_key format (alphanumeric, hyphens, underscores only) + if not re.match(r'^[a-zA-Z0-9_-]+$', project_key): + raise HttpError(400, "Invalid project key format") +
🧹 Nitpick comments (2)
backend/apps/owasp/models/project_health_metrics.py (1)
184-189: Consider making PDF layout constants configurable.The hard-coded positioning and font settings work but could be made more maintainable.
Consider extracting constants:
+# PDF Layout Constants +PDF_TITLE_Y = 700 +PDF_SUBTITLE_Y = 680 +PDF_TITLE_X = 300 +DEFAULT_FONT = "Helvetica" +DEFAULT_FONT_SIZE = 12 buffer = BytesIO() pdf = canvas.Canvas(buffer, pagesize=letter) pdf.setTitle(f"Health Metrics Report for {metrics.project.name}") -pdf.setFont("Helvetica", 12) -pdf.drawCentredString(300, 700, f"Health Metrics Report for {metrics.project.name}") -pdf.drawCentredString(300, 680, f"Health Score: {metrics.score:.2f}") +pdf.setFont(DEFAULT_FONT, DEFAULT_FONT_SIZE) +pdf.drawCentredString(PDF_TITLE_X, PDF_TITLE_Y, f"Health Metrics Report for {metrics.project.name}") +pdf.drawCentredString(PDF_TITLE_X, PDF_SUBTITLE_Y, f"Health Score: {metrics.score:.2f}")backend/tests/apps/owasp/models/project_health_metrics_test.py (1)
123-201: Consider adding edge case tests.The current test covers the happy path, but consider adding tests for:
- When no metrics are found (should return None)
- When metrics have null values for optional fields
- Error handling scenarios
Add these test cases:
def test_generate_detailed_pdf_no_metrics_found(self): """Should return None when no metrics are found.""" with patch( "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.get_latest_health_metrics" ) as mock_get_latest: mock_get_latest.return_value.filter.return_value.first.return_value = None result = ProjectHealthMetrics.generate_latest_metrics_pdf("nonexistent") assert result is None def test_generate_detailed_pdf_with_null_values(self): """Should handle metrics with null optional fields.""" # Test implementation for null handling
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/owasp/api/rest/v1/project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py(0 hunks)backend/apps/owasp/models/project_health_metrics.py(2 hunks)backend/tests/apps/owasp/api/rest/v1/urls_test.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_test.py(0 hunks)backend/tests/apps/owasp/models/project_health_metrics_test.py(2 hunks)
💤 Files with no reviewable changes (2)
- backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py
- backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_test.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
⏰ 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). (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (6)
backend/tests/apps/owasp/api/rest/v1/urls_test.py (1)
7-7: LGTM! Router registration follows established pattern.The import and router registration correctly follow the existing test structure and naming conventions.
Also applies to: 19-19
backend/apps/owasp/models/project_health_metrics.py (3)
164-183: LGTM! Method signature and data retrieval logic are well-implemented.The method correctly queries for the latest health metrics using the established pattern and handles the case where no metrics are found.
190-218: Table data construction is comprehensive and well-structured.The table includes all relevant metrics with clear labels and proper data formatting. The compliance status inversion logic is correct.
219-241: PDF table styling and finalization look good.The table styling provides good visual hierarchy with headers and proper alignment. The buffer handling is correct for API consumption.
backend/tests/apps/owasp/models/project_health_metrics_test.py (2)
138-164: Mock setup is comprehensive and realistic.The mock metrics object includes all necessary attributes with realistic values, and the query chain mocking correctly simulates the database interaction.
165-201: Test assertions thoroughly verify PDF generation.The test properly verifies that all PDF generation components are called with correct parameters, including table data structure and canvas operations.
ahmedxgouda
force-pushed
the
dashboard/pdf-metrics-details-client
branch
from
2917f89 to
53bf2ae
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
frontend/src/components/MetricsPDFButton.tsx (1)
22-26: Improve accessibility for the download button.The button lacks proper accessibility attributes and semantic meaning for screen readers.
<FontAwesomeIcon icon={faFileArrowDown} className="ml-2 h-7 w-7 cursor-pointer text-gray-500 transition-colors duration-200 hover:text-gray-700" + role="button" + tabIndex={0} + aria-label={`Download ${fileName} PDF`} + onKeyDown={(e) => { + if (e.key === 'Enter' || e.key === ' ') { + e.preventDefault() + // Trigger the same onClick logic + } + }} onClick={async () => await fetchMetricsPDF(path, fileName)} />backend/tests/apps/owasp/models/project_health_metrics_test.py (1)
163-165: Consider testing the None case for better coverage.The test only covers the success case where metrics are found. Consider adding a test case for when no metrics are found to ensure the method returns None as expected.
Add a separate test method:
@patch( "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.get_latest_health_metrics", ) def test_generate_detailed_pdf_no_metrics(self, mock_get_latest_health_metrics): """Test PDF generation when no metrics are found.""" mock_get_latest_health_metrics.return_value.filter.return_value.first.return_value = None result = ProjectHealthMetrics.generate_latest_metrics_pdf("nonexistent") assert result is Nonebackend/apps/owasp/models/project_health_metrics.py (1)
234-247: Consider extracting table styling to a reusable constant.The table styling configuration is duplicated between the two PDF generation methods, which violates the DRY principle.
Add a class constant for reusable table styling:
class ProjectHealthMetrics(BulkSaveModel, TimestampedModel): """Project health metrics model.""" + + _PDF_TABLE_STYLE = [ + ("BACKGROUND", (0, 0), (-1, 0), "lightgrey"), + ("TEXTCOLOR", (0, 0), (-1, 0), "black"), + ("ALIGN", (0, 0), (-1, -1), "CENTER"), + ("FONTNAME", (0, 0), (-1, 0), "Helvetica-Bold"), + ("BOTTOMPADDING", (0, 0), (-1, 0), 5), + ("BACKGROUND", (0, 1), (-1, -1), "white"), + ] class Meta:Then use it in both methods:
table = Table( table_data, colWidths="*", - style=TableStyle( - [ - ("BACKGROUND", (0, 0), (-1, 0), "lightgrey"), - ("TEXTCOLOR", (0, 0), (-1, 0), "black"), - ("ALIGN", (0, 0), (-1, -1), "CENTER"), - ("FONTNAME", (0, 0), (-1, 0), "Helvetica-Bold"), - ("BOTTOMPADDING", (0, 0), (-1, 0), 5), - ("BACKGROUND", (0, 1), (-1, -1), "white"), - ] - ), + style=TableStyle(cls._PDF_TABLE_STYLE), )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
backend/apps/owasp/api/internal/views/project_health_metrics.py(1 hunks)backend/apps/owasp/api/internal/views/urls.py(1 hunks)backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py(0 hunks)backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py(0 hunks)backend/apps/owasp/models/project_health_metrics.py(2 hunks)backend/settings/urls.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py(0 hunks)backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_test.py(0 hunks)backend/tests/apps/owasp/models/project_health_metrics_test.py(2 hunks)frontend/.env.example(1 hunks)frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx(2 hunks)frontend/src/app/projects/dashboard/page.tsx(2 hunks)frontend/src/components/MetricsPDFButton.tsx(1 hunks)frontend/src/server/fetchMetricsPDF.ts(1 hunks)frontend/src/utils/credentials.ts(1 hunks)
💤 Files with no reviewable changes (4)
- backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_test.py
- backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py
- backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py
- backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py
✅ Files skipped from review due to trivial changes (5)
- frontend/src/utils/credentials.ts
- backend/settings/urls.py
- frontend/.env.example
- frontend/src/app/projects/dashboard/page.tsx
- backend/apps/owasp/api/internal/views/urls.py
🚧 Files skipped from review as they are similar to previous changes (2)
- frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
- frontend/src/server/fetchMetricsPDF.ts
🧰 Additional context used
🪛 GitHub Check: CodeQL
backend/apps/owasp/api/internal/views/project_health_metrics.py
[warning] 25-25: Reflected server-side cross-site scripting
Cross-site scripting vulnerability due to a user-provided value.
🪛 Ruff (0.12.2)
backend/apps/owasp/models/project_health_metrics.py
199-199: Line too long (102 > 99)
(E501)
207-207: Line too long (112 > 99)
(E501)
🪛 GitHub Actions: Run CI/CD
backend/apps/owasp/models/project_health_metrics.py
[error] 199-199: Ruff: Line too long (102 > 99) (E501)
[error] 207-207: Ruff: Line too long (112 > 99) (E501)
⏰ 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). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (6)
backend/apps/owasp/api/internal/views/project_health_metrics.py (1)
10-17: LGTM! Well-structured overview PDF generation.The overview PDF generation view is properly implemented with appropriate decorators and response handling.
backend/tests/apps/owasp/models/project_health_metrics_test.py (2)
131-205: Comprehensive test coverage for detailed PDF generation.The test properly mocks all ReportLab dependencies and verifies the complete PDF generation flow with appropriate assertions.
211-265: Well-structured test for overview PDF generation.The test correctly mocks the dependencies and validates the overview PDF generation with proper table data and formatting expectations.
backend/apps/owasp/models/project_health_metrics.py (3)
164-183: LGTM! Well-implemented PDF generation with proper error handling.The method correctly handles the case where no metrics are found by returning None, and the project key filtering logic is appropriate.
258-319: LGTM! Clean implementation of overview PDF generation.The overview PDF generation method is well-structured with appropriate data formatting and PDF creation logic.
175-178: Project key prefix validated – no changes needed.The
www-project-prefix is used consistently across the codebase (in utility functions, model queries, API resolvers, and numerous tests). No inconsistencies were found.
backend/apps/owasp/api/internal/views/project_health_metrics.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/api/internal/views/project_health_metrics.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
backend/apps/owasp/models/project_health_metrics.py (2)
199-203: Fix the tuple structure in table data.The "Last Pull Request" row has an incorrect tuple structure that will cause rendering issues in the PDF table. Each table cell should be a single string value, not a tuple.
[ "Last Pull Request", - ( - f"{metrics.last_pull_request_days}", - f"/{metrics.last_pull_request_days_requirement} days", - ), + f"{metrics.last_pull_request_days}/" + f"{metrics.last_pull_request_days_requirement} days", ],
209-214: Fix the tuple structure in table data.Similar to the "Last Pull Request" row, the "OWASP Page Last Update" row has an incorrect tuple structure that will cause rendering issues.
[ "OWASP Page Last Update", - ( - f"{metrics.owasp_page_last_update_days}", - f"/{metrics.owasp_page_last_update_days_requirement} days", - ), + f"{metrics.owasp_page_last_update_days}/" + f"{metrics.owasp_page_last_update_days_requirement} days", ],
🧹 Nitpick comments (2)
backend/apps/owasp/models/project_health_metrics.py (2)
242-242: Consider using explicit column widths for better table layout.Using
colWidths="*"may not provide optimal table formatting. Consider specifying explicit column widths for better control over the PDF layout.- colWidths="*", + colWidths=[200, 200], # Adjust widths as needed
164-325: Consider extracting common PDF styling logic.Both PDF generation methods contain similar table styling and canvas setup code. Consider extracting this into helper methods to follow DRY principles.
This would improve maintainability and reduce code duplication. You could create helper methods like
_create_pdf_table_style()and_setup_pdf_canvas().
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/models/project_health_metrics.py(2 hunks)backend/tests/apps/owasp/models/project_health_metrics_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/apps/owasp/models/project_health_metrics_test.py
🧰 Additional context used
🧠 Learnings (1)
backend/apps/owasp/models/project_health_metrics.py (1)
Learnt from: ahmedxgouda
PR: #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.
⏰ 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). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/owasp/models/project_health_metrics.py (2)
3-11: LGTM! Clean import additions for PDF functionality.The new imports are well-organized and necessary for the PDF generation functionality. The imports follow Python conventions with standard library imports first, followed by third-party imports.
264-325: LGTM! Well-structured overview PDF generation.The
generate_overview_pdfmethod is well-implemented with proper structure and error handling considerations. The logic flows clearly and uses appropriate ReportLab components.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/apps/owasp/models/project_health_metrics.py (3)
164-183: Add input validation for project_key parameter.The method lacks validation for the project_key parameter as previously suggested. Consider adding validation to ensure the parameter is valid before using it in the query.
@staticmethod def generate_latest_metrics_pdf(project_key: str) -> BytesIO | None: """Generate a PDF report of the latest health metrics for a project. Args: project_key (str): The key of the OWASP project. Returns: BytesIO: A buffer containing the generated PDF report. """ + if not project_key or not isinstance(project_key, str): + return None + metrics = ( ProjectHealthMetrics.get_latest_health_metrics() .filter(project__key=f"www-project-{project_key}") .first() ) if not metrics: return None
189-189: Handle potential None values in score formatting.The score formatting still assumes
metrics.scoreis not None, but the model field allows null values. This could cause a TypeError as previously noted.- pdf.drawCentredString(300, 680, f"Health Score: {metrics.score:.2f}") + score_text = f"Health Score: {metrics.score:.2f}" if metrics.score is not None else "Health Score: N/A" + pdf.drawCentredString(300, 680, score_text)
184-255: Add comprehensive error handling for PDF generation.The PDF generation logic lacks error handling for potential ReportLab exceptions or memory issues. The code should be wrapped in a try-catch block as previously suggested.
buffer = BytesIO() - pdf = Canvas(buffer, pagesize=letter) - pdf.setTitle(f"Health Metrics Report for {metrics.project.name}") - # ... rest of PDF generation code ... - pdf.showPage() - pdf.save() - buffer.seek(0) - return buffer + try: + pdf = Canvas(buffer, pagesize=letter) + pdf.setTitle(f"Health Metrics Report for {metrics.project.name}") + # ... rest of PDF generation code ... + pdf.showPage() + pdf.save() + buffer.seek(0) + return buffer + except Exception: + return None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/owasp/api/internal/views/project_health_metrics.py(1 hunks)backend/apps/owasp/models/project_health_metrics.py(2 hunks)backend/apps/owasp/utils.py(1 hunks)backend/tests/apps/owasp/models/project_health_metrics_test.py(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/owasp/utils.py
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/api/internal/views/project_health_metrics.py
- backend/tests/apps/owasp/models/project_health_metrics_test.py
🧰 Additional context used
🧠 Learnings (1)
backend/apps/owasp/models/project_health_metrics.py (1)
Learnt from: ahmedxgouda
PR: #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.
⏰ 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). (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (1)
backend/apps/owasp/models/project_health_metrics.py (1)
3-3: LGTM! Appropriate imports for PDF generation.The imports are well-chosen for the PDF generation functionality - BytesIO for in-memory handling, ReportLab for PDF creation, and a utility function for table creation.
Also applies to: 9-10, 15-15
ahmedxgouda
force-pushed
the
dashboard/pdf-metrics-details-client
branch
from
7301ccc to
2845221
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (4)
backend/apps/owasp/utils/pdf.py (3)
13-29: Consider making styling configurable and improving parameter documentation.The function works correctly but could benefit from:
- Better documentation of the
col_widthsparameter (what does"*"mean?)- Consider extracting hardcoded styling into constants or making it configurable
+# Default table styling constants +DEFAULT_HEADER_BG = "#f2f2f2" +DEFAULT_CELL_BG = "#ffffff" +DEFAULT_GRID_COLOR = "#dddddd" + -def create_table(data, col_widths="*"): +def create_table(data, col_widths="*"): - """Create a styled table for PDF generation.""" + """Create a styled table for PDF generation. + + Args: + data: Table data as list of lists + col_widths: Column widths ("*" for equal widths, or list of widths) + """ return Table( data, colWidths=col_widths, style=TableStyle( [ - ("BACKGROUND", (0, 0), (-1, 0), "#f2f2f2"), + ("BACKGROUND", (0, 0), (-1, 0), DEFAULT_HEADER_BG), ("TEXTCOLOR", (0, 0), (-1, 0), "#000000"), ("ALIGN", (0, 0), (-1, -1), "CENTER"), ("FONTNAME", (0, 0), (-1, -1), "Helvetica"), ("BOTTOMPADDING", (0, 0), (-1, -1), 12), - ("BACKGROUND", (0, 1), (-1, -1), "#ffffff"), - ("GRID", (0, 0), (-1, -1), 1, "#dddddd"), + ("BACKGROUND", (0, 1), (-1, -1), DEFAULT_CELL_BG), + ("GRID", (0, 0), (-1, -1), 1, DEFAULT_GRID_COLOR), ] ), )
42-84: Extract magic numbers to improve maintainability.The function contains many hardcoded positioning and sizing values that make it difficult to maintain and understand the layout.
+# PDF layout constants +PAGE_WIDTH = 300 +TITLE_Y_POSITION = 800 +TABLE_WIDTH = 400 +TABLE_HEIGHT = 600 +TABLE_X_POSITION = 100 +TABLE_Y_POSITION = 570 +FOOTER_Y_POSITION = 100 + def generate_metrics_overview_pdf() -> BytesIO: # ... existing code ... - canvas.drawCentredString(300, 800, "OWASP Project Health Metrics Overview") + canvas.drawCentredString(PAGE_WIDTH, TITLE_Y_POSITION, "OWASP Project Health Metrics Overview") # ... table data creation ... table = create_table(table_data) - table.wrapOn(canvas, 400, 600) - table.drawOn(canvas, 100, 570) + table.wrapOn(canvas, TABLE_WIDTH, TABLE_HEIGHT) + table.drawOn(canvas, TABLE_X_POSITION, TABLE_Y_POSITION) canvas.drawCentredString( - 300, 100, f"Generated on: {timezone.now().strftime('%Y-%m-%d %H:%M:%S')}" + PAGE_WIDTH, FOOTER_Y_POSITION, f"Generated on: {timezone.now().strftime('%Y-%m-%d %H:%M:%S')}" )
124-129: Improve string formatting for better readability.The current approach of using
str.join()to bypass line length limits makes the code harder to read and understand.Consider using more conventional approaches for long strings:
[ "Last Pull Request", - # To bypass ruff long line error - "/".join( - [ - str(metrics.last_pull_request_days), - f"{metrics.last_pull_request_days_requirement} days", - ] - ), + (f"{metrics.last_pull_request_days}/" + f"{metrics.last_pull_request_days_requirement} days"), ], # ... similar change for OWASP Page Last Update ... [ "OWASP Page Last Update", - # To bypass ruff long line error - "/".join( - [ - str(metrics.owasp_page_last_update_days), - f"{metrics.owasp_page_last_update_days_requirement} days", - ] - ), + (f"{metrics.owasp_page_last_update_days}/" + f"{metrics.owasp_page_last_update_days_requirement} days"), ],Also applies to: 138-143
backend/tests/apps/owasp/utils/pdf_test.py (1)
25-25: Update test description to reflect current functionality.The test docstring mentions "command" but this is now testing a utility function, not a Django management command.
- """Test that the command executes without errors.""" + """Test that overview PDF generation executes without errors."""
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
backend/apps/owasp/api/internal/views/project_health_metrics.py(1 hunks)backend/apps/owasp/api/internal/views/urls.py(1 hunks)backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py(0 hunks)backend/apps/owasp/utils/pdf.py(1 hunks)backend/settings/urls.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_test.py(0 hunks)backend/tests/apps/owasp/utils/pdf_test.py(1 hunks)frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx(2 hunks)frontend/src/app/projects/dashboard/page.tsx(2 hunks)frontend/src/components/MetricsPDFButton.tsx(1 hunks)frontend/src/server/fetchMetricsPDF.ts(1 hunks)
💤 Files with no reviewable changes (2)
- backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_pdf.py
- backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_test.py
✅ Files skipped from review due to trivial changes (3)
- frontend/src/app/projects/dashboard/page.tsx
- frontend/src/components/MetricsPDFButton.tsx
- backend/apps/owasp/api/internal/views/urls.py
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/settings/urls.py
- frontend/src/app/projects/dashboard/metrics/[projectKey]/page.tsx
- frontend/src/server/fetchMetricsPDF.ts
- backend/apps/owasp/api/internal/views/project_health_metrics.py
ahmedxgouda
force-pushed
the
dashboard/pdf-metrics-details-client
branch
from
2845221 to
ca5ba01
Compare
|
2 participants
Resolves #1750
