Add button to download health stats PDF #1756
Conversation
Summary by CodeRabbit
WalkthroughThis change introduces a "Download as PDF" button to the project health dashboard frontend, which calls a new backend API endpoint to generate and serve a PDF overview of OWASP project health metrics. The backend implements PDF generation as a utility function, exposes it via a new API view and URL, and removes the previous Django management command. Associated tests are updated to reflect the new architecture. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15–20 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Possibly related PRs
Suggested reviewers
✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (6)
cspell/custom-dict.txt (1)
5-5: Confirm necessity of all-caps entryBOTTOMPADDING.Most dictionary entries are lowercase unless they map to case-sensitive identifiers. If this is not a literal constant used in code (e.g.,
styles.BOTTOMPADDINGin ReportLab), consider switching to lowercase to avoid future duplicates likebottompadding.frontend/src/app/projects/dashboard/page.tsx (1)
99-108: Consider adding error handling for the PDF download operation.The UI layout and button implementation look good, but consider adding error handling for the async PDF download operation to improve user experience.
<Button variant="solid" color="primary" - onPress={async () => await fetchMetricsOverviewPDF()} + onPress={async () => { + try { + await fetchMetricsOverviewPDF() + } catch (error) { + console.error('Failed to download PDF:', error) + // Consider showing a toast notification or error message + } + }} > Download PDF </Button>backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py (1)
48-48: Consider making table column widths more explicit.Using
colWidths="*"may not provide optimal layout for different content lengths.- table = Table(table_data, colWidths="*") + table = Table(table_data, colWidths=[200, 100]) # Explicit widths for better controlfrontend/src/server/fetchMetricsOverivewPDF.ts (1)
19-22: Improve blob validation.The current blob validation is insufficient. A blob can exist but be empty or invalid.
const pdfBlob = await response.blob() - if (!pdfBlob) { + if (!pdfBlob || pdfBlob.size === 0) { throw new AppError(500, 'PDF blob is empty or undefined') } + + // Validate it's actually a PDF + if (pdfBlob.type && !pdfBlob.type.includes('pdf')) { + throw new AppError(500, 'Invalid file type received') + }backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py (2)
34-52: Extract test data to avoid duplicating production logic.The test duplicates the table data creation logic from the production code, making it brittle when the format changes.
Consider extracting the table data creation to a shared utility or testing against the actual structure rather than duplicating the logic:
- table_data = [ - ["Metric", "Value"], - ["Healthy Projects", f"{metrics_stats.projects_count_healthy}"], - # ... rest of table_data - ] + # Test should verify the structure rather than duplicate the logic + expected_metrics = [ + 'projects_count_healthy', 'projects_count_unhealthy', + 'projects_count_need_attention', 'average_score' + ]
16-18: Add test coverage for error scenarios.The test only covers the happy path. Consider adding tests for error scenarios like missing data or PDF generation failures.
Add additional test methods:
def test_command_handles_missing_stats(self, mock_get_stats): """Test command behavior when stats retrieval fails.""" mock_get_stats.side_effect = Exception("Database error") # Verify error handling def test_command_handles_pdf_generation_failure(self, mock_canvas): """Test command behavior when PDF generation fails.""" mock_canvas.side_effect = Exception("PDF generation error") # Verify error handling
📜 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 (10)
.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/management/commands/owasp_generate_project_health_metrics_overview_pdf.py(1 hunks)backend/pyproject.toml(1 hunks)backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)frontend/src/app/projects/dashboard/page.tsx(2 hunks)frontend/src/server/fetchMetricsOverivewPDF.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/src/app/projects/dashboard/page.tsx (3)
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#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.
🧬 Code Graph Analysis (4)
frontend/src/app/projects/dashboard/page.tsx (2)
frontend/src/types/button.ts (1)
Button(4-9)frontend/src/server/fetchMetricsOverivewPDF.ts (1)
fetchMetricsOverviewPDF(4-38)
frontend/src/server/fetchMetricsOverivewPDF.ts (2)
frontend/src/utils/credentials.ts (1)
API_URL(1-1)frontend/src/app/global-error.tsx (1)
AppError(53-64)
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py (2)
backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(16-219)get_stats(161-219)backend/settings/base.py (1)
Base(9-202)
backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py (3)
backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
project_health_stats(22-29)backend/apps/owasp/graphql/nodes/project_health_stats.py (1)
ProjectHealthStatsNode(7-21)backend/settings/base.py (1)
Base(9-202)
🪛 GitHub Actions: Run CI/CD
frontend/src/app/projects/dashboard/page.tsx
[error] 19-19: CSpell: Unknown word 'Overivew'. Suggested fix: 'Overview'.
🔇 Additional comments (7)
cspell/custom-dict.txt (1)
91-91:pdfgenaddition looks good.Matches the naming convention of other lowercase technical terms in this file.
backend/pyproject.toml (1)
57-57: ReportLab dependency verified: version 4.4.2 is current and secure
- backend/pyproject.toml (line 57):
reportlab = "^4.4.2"
• Latest release as of July 2025
• No known vulnerabilities in 4.4.2 (all prior CVEs patched in 3.6.13+)You can safely merge this dependency addition.
.gitignore (1)
25-25: LGTM! Appropriate exclusion of generated PDF files.Adding
backend/apps/owasp/api/v1/urls.py (2)
9-9: LGTM! Clean import addition.The import follows the established naming convention and pattern used by other routers in the file.
17-17: LGTM! Consistent router registration.The router registration follows the established pattern and uses an appropriate URL path that aligns with REST conventions.
backend/apps/owasp/Makefile (1)
21-24: LGTM! Well-structured Makefile target.The new target follows the established conventions:
- Consistent naming pattern with other OWASP targets
- Proper echo message and command execution structure
- Logical placement in the target sequence
frontend/src/app/projects/dashboard/page.tsx (1)
15-15: LGTM! Appropriate button component import.The Button import from @heroui/button is correctly added and used consistently with the project's UI library.
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py
Outdated
Show resolved
Hide resolved
...ts/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py
Outdated
Show resolved
Hide resolved
...ts/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
backend/apps/owasp/api/v1/urls.py (1)
9-10: Maintain import ordering & consistent URL naming.
- Keep imports alphabetically sorted to reduce merge conflicts—insert the new line between
projectandstaff(if any).- The path segment mixes singular nouns (
/projects) with a compound one (/project-health-metrics). Consider/project-health-metrics➜/project-health-metricsis fine, but double-check that the frontend hard-codes the same slug; a mismatch will break the button.No functional blocker, just style & consistency.
Also applies to: 17-18
frontend/src/server/fetchMetricsOverivewPDF.ts (1)
4-38: Well-implemented PDF fetch function with minor improvements needed.The function follows good practices with proper error handling, blob management, and DOM cleanup. Consider these minor improvements:
const pdfBlob = await response.blob() - if (!pdfBlob) { + if (!pdfBlob || pdfBlob.size === 0) { throw new AppError(500, 'PDF blob is empty or undefined') } const pdfUrl = window.URL.createObjectURL(pdfBlob) const link = document.createElement('a') link.href = pdfUrl link.setAttribute('download', 'owasp-project-health-metrics-overview.pdf') document.body.appendChild(link) link.click() document.body.removeChild(link) + window.URL.revokeObjectURL(pdfUrl)Also, consider removing the
Content-Typeheader as it's not typically needed for GET requests.backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py (1)
19-76: Solid PDF generation implementation with room for improvement.The command effectively generates a styled PDF report. Consider these enhancements for robustness:
- Add error handling for data retrieval:
def handle(self, *args, **options): + try: metrics_stats = ProjectHealthMetrics.get_stats() + except Exception as e: + self.stdout.write(self.style.ERROR(f"Failed to retrieve metrics: {e}")) + return
- Add validation for metrics data:
metrics_stats = ProjectHealthMetrics.get_stats() + if not metrics_stats: + self.stdout.write(self.style.ERROR("No metrics data available")) + return
- Consider using constants for styling values to improve maintainability:
# At the top of the class TITLE_Y = 800 TABLE_Y = 570 FOOTER_Y = 100backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py (1)
9-64: Comprehensive test coverage with opportunities for enhancement.The test effectively mocks dependencies and verifies the command execution flow. Consider these improvements for better coverage:
- Test error conditions:
def test_command_execution_with_data_error(self, mock_get_stats): mock_get_stats.side_effect = Exception("Database error") with pytest.raises(Exception): call_command("owasp_generate_project_health_metrics_overview_pdf")
- Verify success message output:
# In the main test method from io import StringIO out = StringIO() call_command("owasp_generate_project_health_metrics_overview_pdf", stdout=out) self.assertIn("PDF overview generated successfully", out.getvalue())
- Extract table data to a shared constant to reduce duplication between test and implementation.
📜 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 (10)
.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/management/commands/owasp_generate_project_health_metrics_overview_pdf.py(1 hunks)backend/pyproject.toml(1 hunks)backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)frontend/src/app/projects/dashboard/page.tsx(2 hunks)frontend/src/server/fetchMetricsOverivewPDF.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/src/app/projects/dashboard/page.tsx (3)
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/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#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.
🧬 Code Graph Analysis (3)
frontend/src/app/projects/dashboard/page.tsx (1)
frontend/src/server/fetchMetricsOverivewPDF.ts (1)
fetchMetricsOverviewPDF(4-38)
frontend/src/server/fetchMetricsOverivewPDF.ts (2)
frontend/src/utils/credentials.ts (1)
API_URL(1-1)frontend/src/app/global-error.tsx (1)
AppError(53-64)
backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py (3)
backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
project_health_stats(22-29)backend/apps/owasp/graphql/nodes/project_health_stats.py (1)
ProjectHealthStatsNode(7-21)backend/settings/base.py (1)
Base(9-202)
🪛 GitHub Actions: Run CI/CD
frontend/src/app/projects/dashboard/page.tsx
[error] 19-19: CSpell: Unknown word 'Overivew'. Suggested fix: 'Overview'.
🔇 Additional comments (5)
backend/pyproject.toml (1)
57-57: Evaluate the impact of adding a heavy, C-extension dependency.
reportlabships native code.
• A compilation toolchain (e.g.,libfreetype, a C compiler) is now required in CI/CD and container images.
• Cold-start time and package size increase—consider moving it to an optional extra if PDF generation is not on the hot path.At minimum, document this new prerequisite in the deployment notes and update Dockerfiles/CI images to install build essentials (
build-essential,libfreetype6-dev, etc.).cspell/custom-dict.txt (1)
5-5: Dictionary entries look good.Both terms are domain-specific and correctly cased; no action needed.
Also applies to: 91-91
.gitignore (1)
25-25: 👍 Ignoring generated PDFs is appropriate.Prevents accidental commits of large binaries and keeps the repo clean.
frontend/src/app/projects/dashboard/page.tsx (1)
99-108: Well-implemented PDF download button.The button implementation follows good practices with proper async handling, appropriate styling, and clean layout positioning using flexbox.
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py (1)
12-12: Confirmed: custom settings import is validI verified that
backend/settings/base.pydefines aBase(Configuration)class with aBASE_DIRattribute, and that your production/staging/local/test modules all subclassBase. The import in the management command:import settings.base # … Path(settings.base.Base.BASE_DIR) # valid: Base.BASE_DIR is defined in base.pywill correctly resolve to the intended project root directory.
No changes required if this direct‐import pattern is intentional. For consistency with other parts of the code (which use
from django.conf import settings; settings.BASE_DIR), you may optionally switch to the standard Django settings interface, but functionally the current approach is correct.
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (5)
backend/apps/owasp/api/v1/project_health_metrics.py (1)
18-18: Consider using a configurable path for the reports directory.The hardcoded path construction might break if the directory structure changes. Consider making the reports directory configurable through Django settings.
- pdf_path = Path(settings.BASE_DIR) / "reports" / "owasp_project_health_metrics_overview.pdf" + reports_dir = getattr(settings, 'REPORTS_DIR', Path(settings.BASE_DIR) / "reports") + pdf_path = Path(reports_dir) / "owasp_project_health_metrics_overview.pdf"backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py (1)
61-62: Consider dynamic positioning for better layout.The fixed coordinates (400, 600) for table wrapping and (100, 570) for drawing might not accommodate varying content sizes well.
Consider calculating positions based on content size or using ReportLab's higher-level document templates for more flexible layouts.
frontend/src/server/fetchMetricsOverivewPDF.ts (2)
26-26: Filename inconsistency with backend.The frontend uses
owasp-project-health-metrics-overview.pdfwhile the backend API returnsowasp_project_health_metrics_overview.pdf(with underscores). This creates inconsistent naming.- link.setAttribute('download', 'owasp-project-health-metrics-overview.pdf') + link.setAttribute('download', 'owasp_project_health_metrics_overview.pdf')
20-22: Unnecessary blob validation.The check for
!pdfBlobis unnecessary sinceresponse.blob()will not return null or undefined.const pdfBlob = await response.blob() - if (!pdfBlob) { - throw new AppError(500, 'PDF blob is empty or undefined') - }backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py (1)
34-52: Consider extracting table data construction to reduce duplication.The table data construction duplicates logic from the main command, which could lead to maintenance issues if the format changes.
Consider extracting the table data construction logic into a shared utility function that both the command and test can use, or at least add a comment indicating this should be kept in sync with the main command.
📜 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 (10)
.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/management/commands/owasp_generate_project_health_metrics_overview_pdf.py(1 hunks)backend/pyproject.toml(1 hunks)backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py(1 hunks)cspell/custom-dict.txt(2 hunks)frontend/src/app/projects/dashboard/page.tsx(2 hunks)frontend/src/server/fetchMetricsOverivewPDF.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/src/app/projects/dashboard/page.tsx (3)
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/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#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.
🧬 Code Graph Analysis (4)
frontend/src/app/projects/dashboard/page.tsx (2)
frontend/src/types/button.ts (1)
Button(4-9)frontend/src/server/fetchMetricsOverivewPDF.ts (1)
fetchMetricsOverviewPDF(4-38)
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py (2)
backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(16-219)get_stats(161-219)backend/settings/base.py (1)
Base(9-202)
frontend/src/server/fetchMetricsOverivewPDF.ts (2)
frontend/src/utils/credentials.ts (1)
API_URL(1-1)frontend/src/app/global-error.tsx (1)
AppError(53-64)
backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py (3)
backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
project_health_stats(22-29)backend/apps/owasp/graphql/nodes/project_health_stats.py (1)
ProjectHealthStatsNode(7-21)backend/settings/base.py (1)
Base(9-202)
🪛 GitHub Actions: Run CI/CD
frontend/src/app/projects/dashboard/page.tsx
[error] 19-19: CSpell: Unknown word 'Overivew' detected. Suggested fix: 'Overview'.
🔇 Additional comments (9)
cspell/custom-dict.txt (1)
5-5: LGTM! Dictionary entries support PDF functionality.The additions of "BOTTOMPADDING" and "pdfgen" are appropriate for the new PDF generation feature. BOTTOMPADDING aligns with ReportLab table styling constants, and pdfgen is a reasonable term for PDF generation functionality.
Also applies to: 91-91
backend/pyproject.toml (1)
57-57: LGTM! ReportLab dependency addition is appropriate.The addition of
reportlab = "^4.4.2"is correct for PDF generation functionality. ReportLab is a well-established library, and the version constraint allows for safe patch updates..gitignore (1)
25-25: LGTM! PDF files appropriately excluded from version control.Adding
backend/apps/owasp/api/v1/urls.py (2)
9-9: LGTM! Import follows established patterns.The import statement for
project_health_metrics_routerfollows the same naming convention and structure as other router imports in the file.
17-17: LGTM! Router registration is properly structured.The route
/project-health-metricsfollows RESTful conventions with kebab-case and integrates consistently with existing router registrations.frontend/src/app/projects/dashboard/page.tsx (2)
15-15: LGTM! Button import is appropriate.The Button import from @heroui/button is correctly added to support the new download functionality.
99-108: LGTM! Header layout and button implementation are well-structured.The header restructuring with flexbox provides proper spacing between the title and download button. The button styling and async onPress handler are implemented correctly.
backend/apps/owasp/Makefile (1)
21-23: LGTM! Follows established patterns.The new Makefile target correctly follows the existing convention and naming pattern used by other OWASP commands in this file.
backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py (1)
16-18: Comprehensive test coverage with good mocking strategy.The test effectively mocks all external dependencies and verifies the core workflow of the PDF generation command. The mock setup ensures the test is isolated and fast.
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py
Outdated
Show resolved
Hide resolved
...ts/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py
Outdated
Show resolved
Hide resolved
...ts/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/app/projects/dashboard/page.tsx(2 hunks)frontend/src/server/fetchMetricsOverviewPDF.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/app/projects/dashboard/page.tsx
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/server/fetchMetricsOverviewPDF.ts (2)
frontend/src/utils/credentials.ts (1)
API_URL(1-1)frontend/src/app/global-error.tsx (1)
AppError(53-64)
⏰ 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 backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
frontend/src/server/fetchMetricsOverviewPDF.ts (5)
1-2: LGTM! Imports are clean and necessary.Both imports are properly used within the function and follow the established patterns in the codebase.
4-5: Good function design for a download operation.The async function signature returning
Promise<void>is appropriate for a side-effect operation like triggering a file download.
14-17: Excellent error handling for HTTP responses.Proper use of
response.okcheck with descriptive error messages and preserving the original HTTP status code in the AppError.
30-37: Excellent error handling pattern.Proper preservation of AppError instances while wrapping unexpected errors in a consistent format. Good use of optional chaining for safe property access.
6-12: Fix incorrect request header and verify URL constructionThe
Content-Typeheader describes the payload you’re sending, not what you expect back. For a GET request returning a PDF, replace it with anAcceptheader. Also double-check whetherAPI_URLends with a slash—if not, you’ll need to add one before the path.• File: frontend/src/server/fetchMetricsOverviewPDF.ts
• Lines: 6–12Suggested diff:
const response = await fetch(`${API_URL}owasp/project-health-metrics/overview-pdf`, { method: 'GET', headers: { - 'Content-Type': 'application/pdf', + 'Accept': 'application/pdf', }, credentials: 'include', })• Ensure
${API_URL}ends with…/or prepend/to the path if necessary.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/apps/owasp/api/rest/v1/project_health_metrics.py (2)
18-18: Consider making the PDF path configurable.The hardcoded path construction could be brittle and makes the code less maintainable. Consider using Django settings or a configuration parameter.
- pdf_path = Path(settings.BASE_DIR) / "reports" / "owasp_project_health_metrics_overview.pdf" + reports_dir = getattr(settings, 'REPORTS_DIR', Path(settings.BASE_DIR) / "reports") + pdf_path = Path(reports_dir) / "owasp_project_health_metrics_overview.pdf"
21-25: Verify proper file handle management in FileResponse.The current implementation opens the file but relies on Django's FileResponse to manage the file handle. While this should work correctly, consider adding explicit context management for clarity.
- return FileResponse( - Path.open(pdf_path, "rb"), - as_attachment=True, - filename="owasp_project_health_metrics_overview.pdf", - ) + return FileResponse( + pdf_path.open("rb"), + as_attachment=True, + filename="owasp_project_health_metrics_overview.pdf", + )
📜 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 (4)
backend/apps/owasp/api/rest/v1/project_health_metrics.py(1 hunks)backend/apps/owasp/api/rest/v1/urls.py(1 hunks)backend/pyproject.toml(1 hunks)backend/tests/apps/owasp/api/rest/v1/urls_test.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/pyproject.toml
⏰ 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 (5)
backend/apps/owasp/api/rest/v1/urls.py (2)
9-9: LGTM! Import follows established pattern.The import statement correctly follows the existing pattern for router imports.
17-17: LGTM! Router registration follows established pattern.The router registration correctly follows the existing pattern with appropriate kebab-case naming convention for the API path.
backend/tests/apps/owasp/api/rest/v1/urls_test.py (2)
7-7: LGTM! Test import correctly mirrors main URL changes.The import statement properly adds the new router for testing coverage.
19-19: LGTM! Test coverage maintained for new router.The addition to EXPECTED_ROUTERS ensures the new router registration is properly tested.
backend/apps/owasp/api/rest/v1/project_health_metrics.py (1)
14-25: Global auth & rate limiting are already applied
Theoverview‐pdfendpoint inheritsApiKeyAuth()andAuthRateThrottle("10/s")from the globalNinjaAPIinbackend/settings/api/v1.py, so it’s already protected and rate-limited. No further changes needed.
ahmedxgouda
force-pushed
the
dashboard/pdf-overview-client
branch
from
af7454e to
70efc9c
Compare
|
@ahmedxgouda could you resolve conflicts here? 🙏🏼 Thanks! |
|
Of course, this PR was just depending on the PDF overview script PR. I will handle this @kasya |
ahmedxgouda
force-pushed
the
dashboard/pdf-overview-client
branch
from
290d053 to
17979fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/apps/owasp/api/rest/v1/project_health_metrics.py (1)
14-14: Consider adding response headers for better client handling.Adding appropriate Content-Type and Content-Length headers would improve client compatibility and user experience.
Apply this diff to add proper response headers:
try: pdf = ProjectHealthMetrics.generate_overview_pdf() + response = FileResponse( + pdf, + as_attachment=True, + filename="owasp_project_health_metrics_overview.pdf", + ) + response['Content-Type'] = 'application/pdf' + pdf.seek(0, 2) # Seek to end to get size + response['Content-Length'] = pdf.tell() + pdf.seek(0) # Reset to beginning + return response - return FileResponse( - pdf, - as_attachment=True, - filename="owasp_project_health_metrics_overview.pdf", - )backend/tests/apps/owasp/models/project_health_metrics_test.py (2)
131-131: Fix misleading test docstring.The docstring mentions "command executes without errors" but this tests the model method, not a command.
Apply this diff to fix the docstring:
- """Test that the command executes without errors.""" + """Test that PDF generation executes without errors."""
123-175: Add assertion for buffer.seek(0) call and consider testing error scenarios.The test is comprehensive but missing verification of the buffer.seek(0) call and could benefit from error scenario testing.
Apply this diff to add the missing assertion:
canvas.showPage.assert_called_once() canvas.save.assert_called_once() + mock_bytes_io.return_value.seek.assert_called_once_with(0)Consider adding an error scenario test:
@patch("apps.owasp.models.project_health_metrics.ProjectHealthMetrics.get_stats") @patch("reportlab.pdfgen.canvas.Canvas") def test_generate_overview_pdf_error_handling(self, mock_canvas, mock_get_stats): """Test that PDF generation raises appropriate errors when Canvas fails.""" mock_get_stats.return_value = ProjectHealthStatsNode( projects_count_healthy=10, projects_count_unhealthy=5, projects_count_need_attention=3, average_score=75.0, total_contributors=150, total_forks=200, total_stars=300, projects_percentage_healthy=66.67, projects_percentage_need_attention=20.00, projects_percentage_unhealthy=13.33, monthly_overall_scores=[], monthly_overall_scores_months=[], ) mock_canvas.side_effect = Exception("Canvas creation failed") with pytest.raises(Exception, match="Failed to generate PDF overview"): ProjectHealthMetrics.generate_overview_pdf()
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/owasp/api/rest/v1/project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf.py(0 hunks)backend/apps/owasp/models/project_health_metrics.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_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_overview_pdf.py
- backend/tests/apps/owasp/management/commands/owasp_generate_project_health_metrics_overview_pdf_test.py
⏰ 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 (1)
3-4: LGTM: Clean imports for PDF generation.The new imports are well-organized and necessary for the PDF generation functionality.
Also applies to: 9-10
backend/tests/apps/owasp/models/project_health_metrics_test.py (1)
1-2: LGTM: Clean test imports.The new imports are properly organized and necessary for the PDF generation test.
Also applies to: 7-7
Signed-off-by: Ahmed Gouda <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/apps/owasp/api/internal/views/project_health_metrics.py (1)
8-8: Consider additional production hardening.Beyond authentication and error handling, consider these enhancements:
- HTTP method restriction: Add
@require_http_methods(["GET"])to prevent non-GET requests- Rate limiting: PDF generation may be resource-intensive; consider implementing rate limiting
- CSRF protection: Evaluate if
@csrf_exemptis needed based on how the frontend calls this endpoint
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/owasp/api/internal/views/project_health_metrics.py(1 hunks)backend/apps/owasp/api/internal/views/urls.py(1 hunks)backend/settings/urls.py(1 hunks)frontend/.env.example(1 hunks)frontend/src/server/fetchMetricsOverviewPDF.ts(1 hunks)frontend/src/utils/credentials.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- frontend/src/utils/credentials.ts
- backend/apps/owasp/api/internal/views/urls.py
- frontend/.env.example
🚧 Files skipped from review as they are similar to previous changes (1)
- frontend/src/server/fetchMetricsOverviewPDF.ts
⏰ 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 backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/settings/urls.py (2)
17-17: LGTM: Clean import following Django conventions.The import path follows standard Django app organization patterns.
23-23: LGTM: Appropriate URL namespacing.The "owasp/" prefix provides clear namespacing for OWASP-specific endpoints and follows REST API best practices.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
backend/tests/apps/owasp/utils/pdf_test.py (1)
43-48: Consider adding test case for null average_score.The conditional formatting logic for
average_scoreis implemented in the main function, but the test only covers the non-null case. Consider adding a test case whereaverage_scoreisNoneto ensure the "N/A" fallback works correctly.Add a test case for null average_score:
def test_generate_overview_pdf_null_average_score(self, ...): """Test PDF generation when average_score is None.""" metrics_stats = ProjectHealthStatsNode( # ... other fields ... average_score=None, # Test null case # ... other fields ... ) # ... rest of test setup and assertions ...
📜 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/utils/pdf.py(1 hunks)backend/tests/apps/owasp/utils/pdf_test.py(3 hunks)frontend/src/server/fetchMetricsOverviewPDF.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/owasp/api/internal/views/project_health_metrics.py
⏰ 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 (3)
frontend/src/server/fetchMetricsOverviewPDF.ts (1)
1-39: LGTM! Past review comments have been addressed.The implementation correctly addresses the previous feedback:
- Memory cleanup with
window.URL.revokeObjectURL(pdfUrl)on line 31- Improved blob validation checking
pdfBlob.size === 0on line 20The function properly handles PDF download with error handling, date-stamped filenames, and proper resource cleanup.
backend/apps/owasp/utils/pdf.py (1)
34-37: Good defensive programming for nullable values.The conditional formatting for
average_scorewith fallback to "N/A" properly handles potential null values.backend/tests/apps/owasp/utils/pdf_test.py (1)
9-77: Well-structured test with proper mocking.The test properly validates the PDF generation function with comprehensive mocking. The test data and assertions align well with the implementation.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
frontend/src/server/fetchMetricsOverviewPDF.ts (1)
25-26: Consider using a more readable date format in filename.The current date format uses underscores and might be less intuitive for users. Consider using a hyphenated format which is more standard.
- const date = new Date().toISOString().split('T')[0].replaceAll('-', '_') - const fileName = `owasp_metrics_overview_${date}.pdf` + const date = new Date().toISOString().split('T')[0] + const fileName = `owasp-metrics-overview-${date}.pdf`frontend/.env.example (1)
17-17: Consider reordering environment variables for consistency.The static analysis tool suggests ordering
NEXTAUTH_URLbeforeNEXT_PUBLIC_API_URLfor better organization, though this is a minor style preference.+NEXTAUTH_URL=http://localhost:3000/ NEXT_PUBLIC_API_URL=http://localhost:8000/ NEXT_PUBLIC_CSRF_URL=http://localhost:8000/csrf/ NEXT_PUBLIC_ENVIRONMENT=local NEXT_PUBLIC_GRAPHQL_URL=http://localhost:8000/graphql/ NEXT_PUBLIC_GTM_ID= NEXT_PUBLIC_IDX_URL=http://localhost:8000/idx/ NEXT_PUBLIC_IS_PROJECT_HEALTH_ENABLED=true NEXT_PUBLIC_RELEASE_VERSION= NEXT_PUBLIC_SENTRY_DSN= NEXT_SENTRY_AUTH_TOKEN= NEXT_SERVER_CSRF_URL=http://backend:8000/csrf/ NEXT_SERVER_DISABLE_SSR=false NEXT_SERVER_GITHUB_CLIENT_ID= NEXT_SERVER_GITHUB_CLIENT_SECRET= NEXT_SERVER_GRAPHQL_URL=http://backend:8000/graphql/ NEXTAUTH_SECRET=<your-nextauth-secret> -NEXTAUTH_URL=http://localhost:3000/
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
CONTRIBUTING.md(1 hunks)backend/apps/owasp/utils/pdf.py(1 hunks)backend/tests/apps/owasp/utils/pdf_test.py(1 hunks)frontend/.env.example(2 hunks)frontend/src/server/fetchMetricsOverviewPDF.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- CONTRIBUTING.md
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/utils/pdf.py
- backend/tests/apps/owasp/utils/pdf_test.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/server/fetchMetricsOverviewPDF.ts (2)
frontend/src/utils/credentials.ts (1)
API_URL(1-1)frontend/src/app/global-error.tsx (1)
handleAppError(66-86)
🪛 dotenv-linter (3.3.0)
frontend/.env.example
[warning] 17-17: [UnorderedKey] The NEXTAUTH_URL key should go before the NEXT_PUBLIC_API_URL key
⏰ 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)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (2)
frontend/src/server/fetchMetricsOverviewPDF.ts (1)
4-39: LGTM! Previous feedback has been properly addressed.The implementation correctly addresses the previous review comments:
- Memory cleanup is handled with
window.URL.revokeObjectURL(pdfUrl)on line 31- Blob validation uses
pdfBlob.size === 0check on line 19 instead of the ineffective falsy checkThe function is well-structured with proper error handling, meaningful filename generation with date stamps, and clean DOM manipulation.
frontend/.env.example (1)
1-1: API URL change aligns with backend restructuring.The removal of the
/api/v1/suffix from the API URL correctly aligns with the backend restructuring mentioned in the PR summary, where the new PDF endpoint is served under the base API URL.
3 participants
Resolves #1749
