feature/Implementation of management command. #2073
Conversation
Summary by CodeRabbit
WalkthroughAdds official OWASP project-level syncing, a compliance-detection command, model fields/properties and migrations for compliance tracking and penalty weight, compliance-aware scoring (penalties), Makefile wiring to run sync/metrics during data sync, and extensive unit/integration tests for the flows. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, 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: 19
🧹 Nitpick comments (23)
backend/apps/owasp/models/project_health_metrics.py (1)
46-50: Default=True can mask non-compliance until the detection job runs; ensure job ordering and consider indexing
- With default=True, new metrics won’t get penalized unless the compliance detection job has already flipped the flag. Since owasp_update_project_health_scores only computes scores where score IS NULL, running scoring before detection can permanently skip penalties for that day’s rows.
- Ensure the pipeline runs “detect compliance” before “update scores” every time.
- If you’ll frequently filter on this flag (e.g., reports, dashboards), add a DB index.
Proposed index (requires a follow-up migration):
- is_level_compliant = models.BooleanField( + is_level_compliant = models.BooleanField( verbose_name="Is project level compliant", default=True, help_text="Whether the project's local level matches the official OWASP level", + db_index=True, )Would you like me to add a combined Makefile target that enforces detect-before-score ordering?
backend/apps/owasp/Makefile (2)
64-66: Mark new target as .PHONY to avoid filename collisionsDeclare the target as phony to prevent unexpected behavior if a file with the same name exists.
- + .PHONY: owasp-detect-project-level-compliance owasp-detect-project-level-compliance: @echo "Detecting OWASP project level compliance" @CMD="python manage.py owasp_detect_project_level_compliance" $(MAKE) exec-backend-command
64-66: Add an orchestrator target to enforce detect-before-score orderingTo ensure penalties are applied correctly, add a convenience target that runs detection before scoring.
Example (add anywhere appropriate in this Makefile):
.PHONY: owasp-refresh-health-scores owasp-refresh-health-scores: @echo "Refreshing OWASP health scores (detect compliance -> update scores)" @$(MAKE) owasp-detect-project-level-compliance @$(MAKE) owasp-update-project-health-scoresbackend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
66-69: Fix Ruff E501: wrap the long warning string across linesThis satisfies the line-length rule and improves readability.
- self.stdout.write( - self.style.WARNING( - f"Applied {penalty_percentage}% compliance penalty to {metric.project.name} " - f"(penalty: {penalty_amount:.2f}, final score: {score:.2f})" - ) - ) + self.stdout.write( + self.style.WARNING( + f"Applied {penalty_percentage}% compliance penalty to " + f"{metric.project.name} (penalty: {penalty_amount:.2f}, " + f"final score: {score:.2f})" + ) + )backend/apps/owasp/migrations/0047_add_is_level_compliant_field.py (1)
12-18: Migration adds a NOT NULL boolean with default; consider operational impact on large tablesThis is correct and matches the model, but adding a NOT NULL column with a default can rewrite the entire table on some RDBMS, which may be heavy for large datasets.
If table size is large, consider a two-step approach in future:
- Add nullable column without default.
- Backfill in a separate migration/command.
- Then set default=True and NOT NULL in a final migration.
backend/apps/owasp/models/project_health_requirements.py (1)
60-64: Consider DecimalField for user-facing percentagesIf you expect admins to set this via the admin UI and want to avoid float rounding artifacts, a DecimalField with suitable precision (e.g., max_digits=5, decimal_places=2) can be safer.
backend/apps/owasp/utils/project_level_fetcher.py (6)
5-6: Drop deprecated typing.Dict; prefer built-in genericsRemove the import and use dict[...] in annotations.
-import json import logging -from typing import Dict +import jsonIf you switch to response.json() (see below), you can drop the json import entirely and adjust the exception handling.
15-15: Use built-in generics and keep return type preciseSwitch to dict[str, str] | None.
-def fetch_official_project_levels(timeout: int = 30) -> Dict[str, str] | None: +def fetch_official_project_levels(timeout: int = 30) -> dict[str, str] | None:
31-36: Use logger.error for non-exceptional invalid format, not logger.exceptionlogger.exception is intended within an except block; using it here adds a misleading traceback.
- if not isinstance(data, list): - logger.exception( - "Invalid project levels data format", - extra={"expected": "list", "got": type(data).__name__} - ) + if not isinstance(data, list): + logger.error( + "Invalid project levels data format", + extra={"expected": "list", "got": type(data).__name__}, + ) return None
28-57: Whitespace/style nits flagged by RuffThere are multiple trailing whitespace and blank-line-with-whitespace instances (W291/W293) in this block. Please run the formatter/ruff to clean them up.
26-27: Optional: Identify as a well-behaved clientGitHub raw endpoints may apply tighter rate limits without a UA. Consider adding a descriptive User-Agent.
- response = requests.get( + response = requests.get( OWASP_PROJECT_LEVELS_URL, timeout=timeout, - headers={"Accept": "application/json"}, + headers={ + "Accept": "application/json", + "User-Agent": "Nest/owasp-level-fetcher (+https://github.com/OWASP/Nest)", + }, )
1-62: Add unit tests for data shape, filtering, and type coercionPer the PR objectives, please add tests that:
- verify handling of non-list payloads (returns None),
- filter out invalid entries and normalize names via strip,
- coerce numeric levels to strings,
- propagate network/JSON errors as None.
I can scaffold tests using requests-mock and parametrized inputs if helpful. Do you want me to draft them?
backend/apps/owasp/utils/__init__.py (1)
6-6: Add a trailing newlineStyle nit flagged by Ruff (W292).
-__all__ = ["ComplianceDetector", "ComplianceReport", "fetch_official_project_levels"] +__all__ = ["ComplianceDetector", "ComplianceReport", "fetch_official_project_levels"] +backend/apps/owasp/utils/compliance_detector.py (3)
55-58: Use elif to reduce indentation and match linter suggestionSmall readability win.
- else: - # Project not found in official data - mark as non-compliant - if metric.is_level_compliant: - metric.is_level_compliant = False + elif metric.is_level_compliant: + # Project not found in official data - mark as non-compliant + metric.is_level_compliant = False
66-79: Return updated count (optional) and tighten logging contextReturning the number of updates can aid orchestration/metrics; not required, but useful.
Example:
-def detect_and_update_compliance(official_levels: dict[str, str]) -> None: +def detect_and_update_compliance(official_levels: dict[str, str]) -> int: @@ - if metrics_to_update: + if metrics_to_update: ProjectHealthMetrics.objects.bulk_update( metrics_to_update, ['is_level_compliant'], batch_size=100 ) logger.info( "Updated compliance status for projects", extra={"updated_count": len(metrics_to_update)} ) - else: + return len(metrics_to_update) + else: logger.info("No compliance status changes needed") + return 0This change requires updating callers.
1-79: Tests required for detection logic to match PR objectivesPlease add tests that:
- exercise mismatches/matches (including name stripping),
- ensure projects missing from official data are marked non-compliant,
- validate that only latest metrics are updated,
- assert bulk_update is called with the expected set and that N+1 does not occur (can be inferred via query count if you have pytest-django/pytest asserts).
I can draft pytest/pytest-django tests with model factories to cover the above. Want me to scaffold them?
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (7)
19-34: Align with formatter/linter: switch to double quotes for CLI argsRuff flags single quotes (Q000). Use double quotes for consistency with the codebase style.
Apply:
- parser.add_argument( - '--dry-run', - action='store_true', - help='Show what would be changed without making actual updates' - ) + parser.add_argument( + "--dry-run", + action="store_true", + help="Show what would be changed without making actual updates", + ) - parser.add_argument( - '--verbose', - action='store_true', - help='Enable verbose output for debugging' - ) + parser.add_argument( + "--verbose", + action="store_true", + help="Enable verbose output for debugging", + ) - parser.add_argument( - '--timeout', - type=int, - default=30, - help='HTTP timeout for fetching project levels (default: 30 seconds)' - ) + parser.add_argument( + "--timeout", + type=int, + default=30, + help="HTTP timeout for fetching project levels (default: 30 seconds)", + )
25-28: Prefer Django’s built-in --verbosity over a custom --verbose flagDjango commands already support -v/--verbosity. Consider removing the custom --verbose flag or, at minimum, wiring both together to control logging.
- parser.add_argument( - "--verbose", - action="store_true", - help="Enable verbose output for debugging", - ) + # Prefer Django's built-in -v/--verbosity. If a separate flag is desired, + # keep it, but it's usually redundant. + parser.add_argument( + "--verbose", + action="store_true", + help="Enable verbose output for debugging (redundant with --verbosity)", + )And later (see next comment) gate logging on either verbose or verbosity >= 2.
36-46: Honor Django verbosity for logging; fix quote styleUse verbosity to control logger level in addition to the custom flag. Also address Q000 (double quotes).
- start_time = time.time() - dry_run = options['dry_run'] - verbose = options['verbose'] - timeout = options['timeout'] + start_time = time.time() + dry_run = options["dry_run"] + verbose = options["verbose"] + timeout = options["timeout"] + verbosity = int(options.get("verbosity", 1)) @@ - if verbose: - logging.getLogger('apps.owasp.utils').setLevel(logging.DEBUG) + if verbose or verbosity >= 2: + logging.getLogger("apps.owasp.utils").setLevel(logging.DEBUG)
85-98: Prefer numeric structured fields in logs over pre-formatted stringsThis makes logs easier to query/aggregate. Keep units in the key name.
- logger.info( + logger.info( "Compliance detection completed successfully", extra={ - "execution_time": f"{execution_time:.2f}s", + "execution_time_s": round(execution_time, 2), "dry_run": dry_run, "total_projects": report.total_projects_checked, "compliant_projects": len(report.compliant_projects), "non_compliant_projects": len(report.non_compliant_projects), "local_only_projects": len(report.local_only_projects), "official_only_projects": len(report.official_only_projects), - "compliance_rate": f"{report.compliance_rate:.1f}%" + # Assuming compliance_rate is in [0, 100]; adjust if it is [0, 1]. + "compliance_rate_pct": round(report.compliance_rate, 1), } )Is ComplianceReport.compliance_rate expressed as [0..100] or [0..1]? If [0..1], we must multiply by 100 before rounding/printing. I can adjust once confirmed.
115-126: Include mismatch details when availableIf the report includes local/official level differences, surface them for faster triage; gracefully fallback to names-only when not available.
def _log_compliance_findings(self, report): """Log and display detailed compliance findings.""" # Log level mismatches for non-compliant projects if report.non_compliant_projects: - self.stderr.write(f"Found {len(report.non_compliant_projects)} non-compliant projects:") - for project_name in report.non_compliant_projects: - self.stderr.write(f" - {project_name}") - logger.warning( - "Level mismatch detected", - extra={"project": project_name} - ) + count = len(report.non_compliant_projects) + self.stderr.write(f"Found {count} non-compliant projects:") + # Support both list[str] and dict[str, dict[str,str]] forms + if isinstance(report.non_compliant_projects, dict): + items = report.non_compliant_projects.items() + else: + items = ((name, None) for name in report.non_compliant_projects) + for project_name, details in items: + if isinstance(details, dict) and {"local_level", "official_level"} <= details.keys(): + self.stderr.write( + f" - {project_name}: local={details['local_level']} != official={details['official_level']}" + ) + logger.warning( + "Level mismatch detected", + extra={ + "project": project_name, + "local_level": details["local_level"], + "official_level": details["official_level"], + }, + ) + else: + self.stderr.write(f" - {project_name}") + logger.warning( + "Level mismatch detected", + extra={"project": project_name}, + )
129-131: Break long lines flagged by Ruff (E501)Several user-facing lines exceed 99 chars. Wrap or split to satisfy the linter.
- self.stdout.write(f"Found {len(report.local_only_projects)} projects that exist locally but not in official data:") + self.stdout.write( + f"Found {len(report.local_only_projects)} projects that exist locally " + "but not in official data:" + ) @@ - self.stdout.write(f"Found {len(report.official_only_projects)} projects in official data but not locally:") + self.stdout.write( + f"Found {len(report.official_only_projects)} projects in official data " + "but not locally:" + ) @@ - self.stdout.write(f"Found {len(report.compliant_projects)} compliant projects") + self.stdout.write( + f"Found {len(report.compliant_projects)} compliant projects" + )Also applies to: 139-141, 149-149
19-34: Fix linter items en masse (quotes, trailing whitespace, long lines)Ruff hints (Q000, W293, E501) identify quote style, trailing whitespace in blank lines, and long lines. Recommend running ruff --fix and reflowing long strings where needed to keep CI green.
No diff provided to avoid churn; the targeted snippets above include most of the flagged lines.
Also applies to: 39-41, 45-45, 54-55, 61-61, 64-64, 69-69, 73-77, 81-83, 86-98, 118-121, 129-135, 139-145
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (11)
backend/apps/owasp/Makefile(1 hunks)backend/apps/owasp/constants.py(1 hunks)backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_scores.py(1 hunks)backend/apps/owasp/migrations/0047_add_is_level_compliant_field.py(1 hunks)backend/apps/owasp/migrations/0048_add_compliance_penalty_weight.py(1 hunks)backend/apps/owasp/models/project_health_metrics.py(1 hunks)backend/apps/owasp/models/project_health_requirements.py(1 hunks)backend/apps/owasp/utils/__init__.py(1 hunks)backend/apps/owasp/utils/compliance_detector.py(1 hunks)backend/apps/owasp/utils/project_level_fetcher.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (5)
backend/apps/owasp/migrations/0047_add_is_level_compliant_field.py (1)
backend/apps/owasp/migrations/0048_add_compliance_penalty_weight.py (1)
Migration(6-18)
backend/apps/owasp/migrations/0048_add_compliance_penalty_weight.py (1)
backend/apps/owasp/migrations/0047_add_is_level_compliant_field.py (1)
Migration(6-18)
backend/apps/owasp/utils/compliance_detector.py (2)
backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
project_health_metrics(25-42)backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(16-240)get_latest_health_metrics(165-179)
backend/apps/owasp/utils/__init__.py (1)
backend/apps/owasp/utils/project_level_fetcher.py (1)
fetch_official_project_levels(15-60)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
backend/apps/owasp/utils/project_level_fetcher.py (1)
fetch_official_project_levels(15-60)
🪛 Ruff (0.12.2)
backend/apps/owasp/utils/project_level_fetcher.py
5-5: typing.Dict is deprecated, use dict instead
(UP035)
15-15: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
28-28: Blank line contains whitespace
Remove whitespace from blank line
(W293)
30-30: Blank line contains whitespace
Remove whitespace from blank line
(W293)
33-33: Trailing whitespace
Remove trailing whitespace
(W291)
40-40: Blank line contains whitespace
Remove whitespace from blank line
(W293)
44-44: Blank line contains whitespace
Remove whitespace from blank line
(W293)
47-47: Blank line contains whitespace
Remove whitespace from blank line
(W293)
48-50: Use a single if statement instead of nested if statements
(SIM102)
53-53: Consider moving this statement to an else block
(TRY300)
57-57: Trailing whitespace
Remove trailing whitespace
(W291)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py
67-67: Line too long (101 > 99)
(E501)
backend/apps/owasp/migrations/0047_add_is_level_compliant_field.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
backend/apps/owasp/migrations/0048_add_compliance_penalty_weight.py
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
9-9: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
14-14: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
backend/apps/owasp/utils/compliance_detector.py
17-17: Blank line contains whitespace
Remove whitespace from blank line
(W293)
18-18: Missing blank line after last section ("Args")
Add blank line after "Args"
(D413)
22-22: Blank line contains whitespace
Remove whitespace from blank line
(W293)
24-24: Local variable active_projects is assigned to but never used
Remove assignment to unused variable active_projects
(F841)
25-25: Blank line contains whitespace
Remove whitespace from blank line
(W293)
30-30: Blank line contains whitespace
Remove whitespace from blank line
(W293)
35-35: Blank line contains whitespace
Remove whitespace from blank line
(W293)
40-40: Blank line contains whitespace
Remove whitespace from blank line
(W293)
45-45: Blank line contains whitespace
Remove whitespace from blank line
(W293)
55-57: Use elif instead of else then if, to reduce indentation
Convert to elif
(PLR5501)
60-60: Blank line contains whitespace
Remove whitespace from blank line
(W293)
65-65: Blank line contains whitespace
Remove whitespace from blank line
(W293)
69-69: Trailing whitespace
Remove trailing whitespace
(W291)
70-70: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
73-73: Blank line contains whitespace
Remove whitespace from blank line
(W293)
79-79: No newline at end of file
Add trailing newline
(W292)
backend/apps/owasp/utils/__init__.py
6-6: No newline at end of file
Add trailing newline
(W292)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
33-33: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
39-39: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Blank line contains whitespace
Remove whitespace from blank line
(W293)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Blank line contains whitespace
Remove whitespace from blank line
(W293)
49-49: Blank line contains whitespace
Remove whitespace from blank line
(W293)
52-52: Blank line contains whitespace
Remove whitespace from blank line
(W293)
56-56: Blank line contains whitespace
Remove whitespace from blank line
(W293)
59-59: Abstract raise to an inner function
(TRY301)
59-59: Avoid specifying long messages outside the exception class
(TRY003)
59-59: Exception must not use a string literal, assign to variable first
Assign to variable; remove string literal
(EM101)
60-60: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Line too long (101 > 99)
(E501)
62-62: Blank line contains whitespace
Remove whitespace from blank line
(W293)
67-67: Blank line contains whitespace
Remove whitespace from blank line
(W293)
70-70: Blank line contains whitespace
Remove whitespace from blank line
(W293)
78-78: Blank line contains whitespace
Remove whitespace from blank line
(W293)
82-82: Line too long (137 > 99)
(E501)
84-84: Blank line contains whitespace
Remove whitespace from blank line
(W293)
99-99: Blank line contains whitespace
Remove whitespace from blank line
(W293)
102-102: Use explicit conversion flag
Replace with conversion flag
(RUF010)
103-103: Blank line contains whitespace
Remove whitespace from blank line
(W293)
104-104: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
112-112: Blank line contains whitespace
Remove whitespace from blank line
(W293)
113-113: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
119-119: Line too long (100 > 99)
(E501)
126-126: Blank line contains whitespace
Remove whitespace from blank line
(W293)
129-129: Line too long (127 > 99)
(E501)
136-136: Blank line contains whitespace
Remove whitespace from blank line
(W293)
139-139: Line too long (119 > 99)
(E501)
146-146: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🔇 Additional comments (2)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (2)
8-10: Import root validated — 'apps.owasp' imports are consistent; no change requiredSearch found many occurrences of "from apps.owasp" and zero occurrences of "from backend.apps.owasp". The file under review (backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py) matches the repository-wide pattern (e.g., backend/settings/urls.py, backend/apps/owasp/**).
No code changes needed. Ensure your runtime/deployment config keeps backend/ on PYTHONPATH so the top-level "apps" package is importable.
82-83: Confirm compliance_rate scale used in UI outputI couldn't find where report.compliance_rate is computed in the repo — the only occurrences are the print/log lines in the management command. If compliance_rate is a fraction in [0..1], the displayed value will be off by 100x; if it's already in [0..100], it's fine. Please confirm.
Files to check:
- backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
- Summary print and compliance output (around lines 82–83)
- logger.info extra "compliance_rate" (around line 96)
Suggested fix if compliance_rate is a fraction:
- self.stdout.write(f"Compliance rate: {report.compliance_rate:.1f}%") + self.stdout.write(f"Compliance rate: {report.compliance_rate * 100:.1f}%") ... - "compliance_rate": f"{report.compliance_rate:.1f}%" + "compliance_rate": f"{report.compliance_rate * 100:.1f}%"
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (4)
36-36: Add tests for the management command workflow (dry-run/update/failure paths)Per issue #2039 acceptance criteria, please add pytest tests covering fetch failure, empty payload, dry-run vs update behavior, and summary/log outputs using Django’s call_command with mocks.
I can scaffold tests under backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py that patch fetch_official_project_levels and the detector and assert stdout/stderr and DB updates. Want me to generate them?
74-79: Wrap DB update in a transaction to avoid partial state on errorsIf update_compliance_status raises mid-way, you may end up with partial updates. Use transaction.atomic().
Apply this diff:
if not dry_run: self.stdout.write("Updating compliance status in database...") - detector.update_compliance_status(report) + from django.db import transaction + with transaction.atomic(): + detector.update_compliance_status(report) self.stdout.write("Compliance status updated successfully")
69-71: Guard against None/invalid report from detector before proceedingIf detect_compliance_issues returns None or an unexpected type, subsequent attribute access will crash. Fail fast with a CommandError.
Apply this diff:
- report = detector.detect_compliance_issues(official_levels) + report = detector.detect_compliance_issues(official_levels) + if report is None: + raise CommandError("ComplianceDetector.detect_compliance_issues returned no report")
103-116: Use logger.exception with traceback and chain the error; also apply explicit conversion flag and perf_counter
- Prefer logger.exception(...) to automatically log the traceback.
- Chain the CommandError with "from e" to preserve context.
- Use perf_counter for timing.
- Use {e!s} instead of str(e) (RUF010).
Apply this diff:
- except Exception as e: - execution_time = time.time() - start_time - error_msg = f"Compliance detection failed after {execution_time:.2f}s: {str(e)}" - - logger.error( - "Compliance detection failed", - extra={ - "execution_time": f"{execution_time:.2f}s", - "error": str(e) - }, - exc_info=True - ) - - raise CommandError(error_msg) + except Exception as e: + execution_time = time.perf_counter() - start_time + error_msg = f"Compliance detection failed after {execution_time:.2f}s: {e!s}" + + logger.exception( + "Compliance detection failed", + extra={ + "execution_time_s": round(execution_time, 2), + "error": e.__class__.__name__, + }, + ) + raise CommandError(error_msg) from e
🧹 Nitpick comments (7)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (7)
38-38: Use a monotonic timer for measuring durationtime.time() is wall-clock and can jump. Use time.perf_counter() for reliable elapsed time measurement.
Apply this diff:
- start_time = time.time() + start_time = time.perf_counter()
83-87: Use perf_counter for elapsed time and split long summary lines (E501)
- Measure with perf_counter to match the start timer.
- Break the long f-string into wrapped parts to satisfy line-length rules.
Apply this diff:
- execution_time = time.time() - start_time - self.stdout.write(f"\nCompliance detection completed in {execution_time:.2f}s") - self.stdout.write(f"Summary: {len(report.compliant_projects)} compliant, {len(report.non_compliant_projects)} non-compliant") - self.stdout.write(f"Compliance rate: {report.compliance_rate:.1f}%") + execution_time = time.perf_counter() - start_time + self.stdout.write(f"\nCompliance detection completed in {execution_time:.2f}s") + self.stdout.write( + f"Summary: {len(report.compliant_projects)} compliant, " + f"{len(report.non_compliant_projects)} non-compliant" + ) + self.stdout.write(f"Compliance rate: {report.compliance_rate:.1f}%")
89-101: Structured logging: emit numeric values for metrics instead of formatted stringsFor machine-readable logs, prefer numbers (seconds, counts, rate) over preformatted strings. This also reduces formatting overhead.
Apply this diff:
- logger.info( - "Compliance detection completed successfully", - extra={ - "execution_time": f"{execution_time:.2f}s", - "dry_run": dry_run, - "total_projects": report.total_projects_checked, - "compliant_projects": len(report.compliant_projects), - "non_compliant_projects": len(report.non_compliant_projects), - "local_only_projects": len(report.local_only_projects), - "official_only_projects": len(report.official_only_projects), - "compliance_rate": f"{report.compliance_rate:.1f}%" - } - ) + logger.info( + "Compliance detection completed successfully", + extra={ + "execution_time_s": round(execution_time, 2), + "dry_run": bool(dry_run), + "total_projects": int(report.total_projects_checked), + "compliant_projects": len(report.compliant_projects), + "non_compliant_projects": len(report.non_compliant_projects), + "local_only_projects": len(report.local_only_projects), + "official_only_projects": len(report.official_only_projects), + "compliance_rate_pct": round(report.compliance_rate, 1), + }, + )
121-125: Break long stderr line to satisfy E501 and improve readabilityApply this diff:
- self.stderr.write(f"Found {len(report.non_compliant_projects)} non-compliant projects:") + self.stderr.write( + f"Found {len(report.non_compliant_projects)} non-compliant projects:" + )
131-138: Break long stdout line to satisfy E501 and improve readabilityApply this diff:
- self.stdout.write(f"Found {len(report.local_only_projects)} projects that exist locally but not in official data:") + self.stdout.write( + f"Found {len(report.local_only_projects)} projects that exist locally " + f"but not in official data:" + )
141-148: Break long stdout line to satisfy E501 and improve readabilityApply this diff:
- self.stdout.write(f"Found {len(report.official_only_projects)} projects in official data but not locally:") + self.stdout.write( + f"Found {len(report.official_only_projects)} projects in official data " + f"but not locally:" + )
20-34: Code style: unify quotes to double quotes per Ruff Q000 and trim trailing whitespace (W293)Multiple lines use single quotes and contain trailing whitespace flagged by Ruff. Consider running ruff --fix or switching to double quotes consistently for CLI option strings to satisfy Q000 and removing blank-line whitespace.
Also applies to: 39-41, 45-45
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
backend/apps/owasp/utils/project_level_fetcher.py (1)
fetch_official_project_levels(15-60)
🪛 Ruff (0.12.2)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
20-20: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
21-21: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
25-25: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
26-26: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
30-30: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
33-33: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
39-39: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
40-40: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Blank line contains whitespace
Remove whitespace from blank line
(W293)
45-45: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
46-46: Blank line contains whitespace
Remove whitespace from blank line
(W293)
49-49: Blank line contains whitespace
Remove whitespace from blank line
(W293)
52-52: Blank line contains whitespace
Remove whitespace from blank line
(W293)
60-60: Abstract raise to an inner function
(TRY301)
65-65: Blank line contains whitespace
Remove whitespace from blank line
(W293)
70-70: Blank line contains whitespace
Remove whitespace from blank line
(W293)
73-73: Blank line contains whitespace
Remove whitespace from blank line
(W293)
81-81: Blank line contains whitespace
Remove whitespace from blank line
(W293)
85-85: Line too long (137 > 99)
(E501)
87-87: Blank line contains whitespace
Remove whitespace from blank line
(W293)
102-102: Blank line contains whitespace
Remove whitespace from blank line
(W293)
105-105: Use explicit conversion flag
Replace with conversion flag
(RUF010)
106-106: Blank line contains whitespace
Remove whitespace from blank line
(W293)
107-107: Logging .exception(...) should be used instead of .error(..., exc_info=True)
(G201)
115-115: Blank line contains whitespace
Remove whitespace from blank line
(W293)
116-116: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
122-122: Line too long (100 > 99)
(E501)
129-129: Blank line contains whitespace
Remove whitespace from blank line
(W293)
132-132: Line too long (127 > 99)
(E501)
139-139: Blank line contains whitespace
Remove whitespace from blank line
(W293)
142-142: Line too long (119 > 99)
(E501)
149-149: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
57-61: Good: empty payload treated as failure with a clear messageThis resolves the earlier edge case and the Ruff guideline about not embedding string literals directly in raise. Looks solid.
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (5)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (5)
8-9: Verify utils API: ensure ComplianceDetector exists (or switch to functional API)Past analysis indicates
apps.owasp.utils.compliance_detectormight expose a procedural API (e.g.,detect_and_update_compliance) rather than aComplianceDetectorclass/report. If the class isn’t defined, this import and subsequent usage will fail. Align the command with the actual utils API (either restore the class/report or adopt the functional API and adjust the command).Run this to confirm and surface the API in the repo:
#!/bin/bash set -euo pipefail echo "=== Search for ComplianceDetector class ===" ast-grep --pattern $'class ComplianceDetector {\n $$$\n}' || true rg -n '^class\s+ComplianceDetector\b' -S || true echo "=== Search for functional API ===" rg -n 'def\s+detect_and_update_compliance\s*\(' -S || true echo "=== Where is compliance_detector imported/re-exported? ===" rg -n '\bcompliance_detector\b' -S || true # Show the utils/compliance_detector.py if present if [ -f backend/apps/owasp/utils/compliance_detector.py ]; then echo "=== backend/apps/owasp/utils/compliance_detector.py (head) ===" sed -n '1,200p' backend/apps/owasp/utils/compliance_detector.py fi # Show utils __init__.py, in case it re-exports symbols if [ -f backend/apps/owasp/utils/__init__.py ]; then echo "=== backend/apps/owasp/utils/__init__.py ===" sed -n '1,200p' backend/apps/owasp/utils/__init__.py fi
36-36: Add tests for the management command workflow (dry-run, failures, updates)Per issue #2039, tests should exercise: fetch failure (None/empty), dry-run vs update modes, detailed logs, and summary. I can scaffold pytest tests using Django’s
call_commandwith mocks.Do you want me to generate them under backend/tests/apps/owasp/management/commands/?
66-73: Guard against None/invalid report from detectorDefensively handle a None or invalid report before using it. This also improves error messaging early.
detector = ComplianceDetector() report = detector.detect_compliance_issues(official_levels) + if report is None: + msg = "ComplianceDetector.detect_compliance_issues returned no report" + self.stderr.write(msg) + raise CommandError(msg)
3-11: Import transaction at module scope (pairs with the fix above)Add the transaction import at the top and keep imports consistent.
import logging import time from django.core.management.base import BaseCommand, CommandError +from django.db import transaction from apps.owasp.utils.compliance_detector import ComplianceDetector from apps.owasp.utils.project_level_fetcher import fetch_official_project_levels
74-81: Fix fatal SyntaxError: unindented import breaks the try-block
from django.db import transactionat Line 74 is at module indentation while you are insidehandle()’stry:block. This creates a SyntaxError (“Expected except or finally after try”). Move the import to module scope and remove the in-function import.Apply this diff to remove the stray import here:
- from django.db import transactionThen add the import at the module top (see my separate comment for Lines 3–11).
🧹 Nitpick comments (4)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (4)
88-90: Fix E501: split long summary linesWrap the long f-string to satisfy line-length limits and improve readability.
- self.stdout.write(f"Summary: {len(report.compliant_projects)} compliant, {len(report.non_compliant_projects)} non-compliant") + self.stdout.write( + f"Summary: {len(report.compliant_projects)} compliant, " + f"{len(report.non_compliant_projects)} non-compliant" + ) - self.stdout.write(f"Compliance rate: {report.compliance_rate:.1f}%") + self.stdout.write( + f"Compliance rate: {report.compliance_rate:.1f}%" + )
91-104: Prefer numeric values in structured logsAvoid embedding units in structured log fields so they remain machine-friendly; keep units in message if needed.
- "execution_time": f"{execution_time:.2f}s", + "execution_time_s": round(execution_time, 2), @@ - "compliance_rate": f"{report.compliance_rate:.1f}%" + "compliance_rate_pct": round(report.compliance_rate, 1),
44-46: Leverage Django’s built-inverbosityoption instead of custom--verboseDjango already provides
--verbosity(0–3). Consider removing--verboseand usingoptions["verbosity"] >= 2to set debug logging.Example:
verbosity = int(options.get("verbosity", 1)) if verbosity >= 2: logging.getLogger('apps.owasp.utils').setLevel(logging.DEBUG) logger.setLevel(logging.DEBUG)
121-156: Line length and minor logging improvements in _log_compliance_findingsWrap long writes to satisfy E501 and keep logs tidy.
- if report.non_compliant_projects: - self.stderr.write(f"Found {len(report.non_compliant_projects)} non-compliant projects:") + if report.non_compliant_projects: + self.stderr.write( + f"Found {len(report.non_compliant_projects)} non-compliant projects:" + ) @@ - if report.local_only_projects: - self.stdout.write(f"Found {len(report.local_only_projects)} projects that exist locally but not in official data:") + if report.local_only_projects: + self.stdout.write( + f"Found {len(report.local_only_projects)} projects that exist locally " + f"but not in official data:" + ) @@ - if report.official_only_projects: - self.stdout.write(f"Found {len(report.official_only_projects)} projects in official data but not locally:") + if report.official_only_projects: + self.stdout.write( + f"Found {len(report.official_only_projects)} projects in official data " + f"but not locally:" + )Optional: add a type hint if
ComplianceReportexists in utils:def _log_compliance_findings(self, report: "ComplianceReport") -> None: ...
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these settings in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
backend/apps/owasp/utils/project_level_fetcher.py (1)
fetch_official_project_levels(15-60)
🪛 Ruff (0.12.2)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
42-42: Blank line contains whitespace
(W293)
46-46: Blank line contains whitespace
(W293)
49-49: Blank line contains whitespace
(W293)
52-52: Blank line contains whitespace
(W293)
65-65: Blank line contains whitespace
(W293)
70-70: Blank line contains whitespace
(W293)
73-73: Blank line contains whitespace
(W293)
74-74: SyntaxError: Expected except or finally after try block
77-77: SyntaxError: Unexpected indentation
84-84: Blank line contains whitespace
(W293)
88-88: Line too long (137 > 99)
(E501)
90-90: Blank line contains whitespace
(W293)
105-105: Blank line contains whitespace
(W293)
106-106: SyntaxError: unindent does not match any outer indentation level
106-106: SyntaxError: Expected a statement
106-106: SyntaxError: Expected a statement
106-107: SyntaxError: Expected an expression
107-107: SyntaxError: Unexpected indentation
109-109: Blank line contains whitespace
(W293)
118-118: Blank line contains whitespace
(W293)
121-121: SyntaxError: unindent does not match any outer indentation level
125-125: Line too long (100 > 99)
(E501)
132-132: Blank line contains whitespace
(W293)
135-135: Line too long (127 > 99)
(E501)
142-142: Blank line contains whitespace
(W293)
145-145: Line too long (119 > 99)
(E501)
152-152: Blank line contains whitespace
(W293)
🔇 Additional comments (2)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (2)
57-61: Good: empty or None payload treated as failure with clear messagechecking
is None or not official_levelsand assigning tomsgbefore raising fixes both functional and lint concerns.
79-83: DB update wrapped in a transaction — goodUsing
transaction.atomic()makes the update all-or-nothing and prevents partial state on failures.
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
Outdated
Show resolved
Hide resolved
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…hub.com/divyanshu-vr/Nest into feature/ProjectLevelComplianceDetection
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/apps/owasp/utils/__init__.py (1)
3-6: Re-exports aligned with actual API — good fixSwitching to export detect_and_update_compliance and fetch_official_project_levels resolves the earlier ImportError risk.
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
68-71: Dry-run is not honored — command still updates the DBThe command calls detect_and_update_compliance unconditionally, so a dry-run will persist changes. Gate the update or pass a dry_run flag to the utility.
- # Steps 2-4: Detect and update in one procedural call - self.stdout.write("Detecting and updating compliance issues...") - detect_and_update_compliance(official_levels) + # Steps 2-4: Detect and update (or preview in dry-run) + if dry_run: + self.stdout.write("Detecting compliance issues (dry-run)...") + updated_count = detect_and_update_compliance( + official_levels, + dry_run=True, + ) + self.stdout.write(f"DRY RUN: would update {updated_count} project(s)") + else: + self.stdout.write("Detecting and updating compliance issues...") + updated_count = detect_and_update_compliance( + official_levels, + dry_run=False, + ) + self.stdout.write(f"Compliance status updated for {updated_count} project(s)")Note: This requires detect_and_update_compliance to accept dry_run and return the update count (see companion change suggested in compliance_detector.py).
🧹 Nitpick comments (8)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
67-72: Reduce log noise and fix the long line (E501) by gating and refactoring messageAvoid logging when the penalty is 0% and reflow the message to satisfy the linter.
- self.stdout.write( - self.style.WARNING( - f"Applied {penalty_percentage}% compliance penalty to {metric.project.name} " - f"(penalty: {penalty_amount:.2f}, final score: {score:.2f})" - ) - ) + if penalty_amount > 0.0: + msg = ( + f"Applied {penalty_percentage}% compliance penalty to " + f"{metric.project.name} (penalty: {penalty_amount:.2f}, " + f"final score: {score:.2f})" + ) + self.stdout.write(self.style.WARNING(msg))backend/apps/owasp/utils/project_level_fetcher.py (1)
3-5: Tidy imports, modernize types, and set a polite User-Agent
- Remove unused import (json).
- Prefer built-in generics: dict[str, str] over typing.Dict.
- Add a User-Agent header to be a good API citizen when calling GitHub endpoints.
- Clean up minor whitespace flagged by Ruff.
- import json import logging -from typing import Dict +from typing import Dict # TODO: Remove if unused elsewhere in the module @@ -def fetch_official_project_levels(timeout: int = 30) -> Dict[str, str] | None: +def fetch_official_project_levels(timeout: int = 30) -> dict[str, str] | None: @@ - response = requests.get( - OWASP_PROJECT_LEVELS_URL, - timeout=timeout, - headers={"Accept": "application/json"}, - ) + response = requests.get( + OWASP_PROJECT_LEVELS_URL, + timeout=timeout, + headers={ + "Accept": "application/json", + "User-Agent": "OWASP-Nest/ComplianceFetcher", + }, + ) @@ - logger.exception( - "Invalid project levels data format", + logger.exception( + "Invalid project levels data format", extra={"expected": "list", "got": type(data).__name__} )Note: You can also drop the leftover
from typing import Dictentirely if nothing else uses it.Also applies to: 15-15, 26-33, 35-35, 42-42
backend/apps/owasp/utils/__init__.py (1)
6-6: Add trailing newline (W292)Ensure the file ends with a single newline to satisfy linters.
-__all__ = ["detect_and_update_compliance", "fetch_official_project_levels"] +__all__ = ["detect_and_update_compliance", "fetch_official_project_levels"] +backend/apps/owasp/utils/compliance_detector.py (2)
9-9: Remove unused import
Projectis not used in this module.-from apps.owasp.models.project import Project
56-60: Preferelifto reduce nestingUse
eliffor clarity.- else: - # Project not found in official data - mark as non-compliant - if metric.is_level_compliant: + elif metric.is_level_compliant: + # Project not found in official data - mark as non-compliant metric.is_level_compliant = False metrics_to_update.append(metric)backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (3)
7-7: Remove unused import (F401)
transactionis not used in this command.-from django.db import transaction
73-84: Compute counts via DB to avoid materializing the querysetAvoid iterating the queryset in Python; let the DB count rows.
- latest_metrics = ProjectHealthMetrics.get_latest_health_metrics() - total = len(latest_metrics) - compliant = sum(1 for m in latest_metrics if m.is_level_compliant) - non_compliant = total - compliant + latest_metrics = ProjectHealthMetrics.get_latest_health_metrics() + total = latest_metrics.count() + compliant = latest_metrics.filter(is_level_compliant=True).count() + non_compliant = total - compliant
111-111: Add trailing newline (W292)Ensure the file ends with a single newline.
- raise CommandError(error_msg) from e - + raise CommandError(error_msg) from e +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_scores.py(1 hunks)backend/apps/owasp/migrations/0048_add_compliance_penalty_weight.py(1 hunks)backend/apps/owasp/utils/__init__.py(1 hunks)backend/apps/owasp/utils/compliance_detector.py(1 hunks)backend/apps/owasp/utils/project_level_fetcher.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/migrations/0048_add_compliance_penalty_weight.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-29T00:41:32.198Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1676
File: backend/apps/owasp/graphql/filters/project_health_metrics.py:17-22
Timestamp: 2025-06-29T00:41:32.198Z
Learning: In the OWASP Nest codebase, when implementing GraphQL filters that convert string values to enums (like ProjectLevel), do not catch ValueError exceptions for invalid values. Let the errors propagate to provide proper error responses to GraphQL clients rather than silently ignoring invalid input.
Applied to files:
backend/apps/owasp/utils/project_level_fetcher.py
📚 Learning: 2025-08-04T15:55:08.017Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1967
File: backend/apps/owasp/api/internal/filters/project_health_metrics.py:24-27
Timestamp: 2025-08-04T15:55:08.017Z
Learning: In the OWASP Nest project, the `is_active` filter in `ProjectHealthMetricsFilter` is intentionally designed as a workaround to eliminate inactive projects from the dashboard. It only filters FOR active projects when `value=True`, and returns an empty Q() when `value=False` to avoid showing inactive projects. This is not meant to be a general boolean filter but a specific solution to exclude inactive projects from the project health metrics dashboard.
Applied to files:
backend/apps/owasp/utils/compliance_detector.py
🧬 Code Graph Analysis (3)
backend/apps/owasp/utils/__init__.py (2)
backend/apps/owasp/utils/compliance_detector.py (1)
detect_and_update_compliance(15-80)backend/apps/owasp/utils/project_level_fetcher.py (1)
fetch_official_project_levels(15-62)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (4)
backend/apps/owasp/utils/compliance_detector.py (1)
detect_and_update_compliance(15-80)backend/apps/owasp/utils/project_level_fetcher.py (1)
fetch_official_project_levels(15-62)backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
project_health_metrics(25-42)backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(16-240)get_latest_health_metrics(165-179)
backend/apps/owasp/utils/compliance_detector.py (2)
backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
project_health_metrics(25-42)backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(16-240)get_latest_health_metrics(165-179)
🪛 Ruff (0.12.2)
backend/apps/owasp/utils/project_level_fetcher.py
3-3: json imported but unused
Remove unused import: json
(F401)
5-5: typing.Dict is deprecated, use dict instead
(UP035)
15-15: Use dict instead of Dict for type annotation
Replace with dict
(UP006)
35-35: Trailing whitespace
Remove trailing whitespace
(W291)
42-42: Blank line contains whitespace
Remove whitespace from blank line
(W293)
55-55: Consider moving this statement to an else block
(TRY300)
backend/apps/owasp/utils/__init__.py
6-6: No newline at end of file
Add trailing newline
(W292)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
7-7: django.db.transaction imported but unused
Remove unused import: django.db.transaction
(F401)
22-22: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
23-23: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
24-24: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
27-27: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
28-28: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
29-29: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
32-32: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
35-35: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
41-41: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
42-42: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
43-43: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
44-44: Blank line contains whitespace
Remove whitespace from blank line
(W293)
47-47: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
48-48: Blank line contains whitespace
Remove whitespace from blank line
(W293)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
54-54: Blank line contains whitespace
Remove whitespace from blank line
(W293)
62-62: Abstract raise to an inner function
(TRY301)
67-67: Blank line contains whitespace
Remove whitespace from blank line
(W293)
71-71: Blank line contains whitespace
Remove whitespace from blank line
(W293)
79-79: Blank line contains whitespace
Remove whitespace from blank line
(W293)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
98-98: Blank line contains whitespace
Remove whitespace from blank line
(W293)
111-111: Blank line contains whitespace
Remove whitespace from blank line
(W293)
111-111: No newline at end of file
Add trailing newline
(W292)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py
69-69: Line too long (101 > 99)
(E501)
backend/apps/owasp/utils/compliance_detector.py
9-9: apps.owasp.models.project.Project imported but unused
Remove unused import: apps.owasp.models.project.Project
(F401)
15-15: Unused function argument: official_levels
(ARG001)
17-17: Blank line contains whitespace
Remove whitespace from blank line
(W293)
18-18: Missing blank line after last section ("Args")
Add blank line after "Args"
(D413)
22-22: Blank line contains whitespace
Remove whitespace from blank line
(W293)
29-29: Blank line contains whitespace
Remove whitespace from blank line
(W293)
34-34: Blank line contains whitespace
Remove whitespace from blank line
(W293)
38-38: Undefined name normalized_official_levels
(F821)
39-39: Undefined name normalized_official_levels
(F821)
46-46: Blank line contains whitespace
Remove whitespace from blank line
(W293)
56-58: Use elif instead of else then if, to reduce indentation
Convert to elif
(PLR5501)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
66-66: Blank line contains whitespace
Remove whitespace from blank line
(W293)
70-70: Trailing whitespace
Remove trailing whitespace
(W291)
71-71: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
74-74: Blank line contains whitespace
Remove whitespace from blank line
(W293)
80-80: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (3)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (2)
60-66: Penalty logic and clamping look correctClamping [0, 100] and flooring the score at 0.0 avoids bad configs from causing negative scores or inflating penalties. Good defensive change.
73-74: Final score bound check is appropriateBounding the final score to [0, 100] is consistent with model validators and prevents drift. LGTM.
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
99-111: Good failure handling and structured loggingCatching, timing with perf_counter, using logger.exception, and chaining the CommandError are all solid practices here.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
cron/production (1)
5-5: Nit: log file naming consistency with existing cron linesOther lines (3–4) don’t use the “.log” suffix. For consistency with existing entries, consider dropping “.log” here or standardizing all log files in a separate change. Ensure logrotate picks up the chosen filename.
-25 17 * * * cd /home/production; make owasp-detect-project-level-compliance > /var/log/nest/production/detect-project-level-compliance.log 2>&1 +25 17 * * * cd /home/production; make owasp-detect-project-level-compliance > /var/log/nest/production/detect-project-level-compliance 2>&1backend/apps/owasp/utils/project_level_fetcher.py (1)
45-50: Normalize numeric levels to avoid “2.0” vs “2” false mismatchesIf upstream provides float-like numbers (e.g., 2.0), we’ll store "2.0" while local may be "2". Canonicalize floats with .is_integer() to stringified ints to reduce false negatives.
- ): - project_levels[project_name.strip()] = str(level) + ): + # Canonicalize numeric levels: 2.0 -> "2", keep other values as-is + if isinstance(level, float) and level.is_integer(): + level_str = str(int(level)) + else: + level_str = str(level) + project_levels[project_name.strip()] = level_strbackend/apps/owasp/utils/compliance_detector.py (2)
14-17: Make dry_run keyword-only to prevent accidental positional misuseMinor API hardening: enforce keyword-only for dry_run. This prevents subtle call-site bugs when adding parameters later.
-def detect_and_update_compliance( - official_levels: dict[str, str], - dry_run: bool = False, -) -> int: +def detect_and_update_compliance( + official_levels: dict[str, str], + *, + dry_run: bool = False, +) -> int:
91-91: Ensure trailing newline at EOFSome linters flag missing EOF newline (W292). Add a newline to keep linters happy.
- return len(metrics_to_update) + return len(metrics_to_update) +backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (4)
64-69: Compute summary via DB aggregation to avoid loading all rowslen(queryset) and iterating to sum pulls all rows into memory. Use aggregation with filtered Count for scalability.
- latest_metrics = ProjectHealthMetrics.get_latest_health_metrics() - total = len(latest_metrics) - compliant = sum(1 for m in latest_metrics if m.is_level_compliant) - non_compliant = total - compliant - compliance_rate = (compliant / total * 100) if total else 0.0 + from django.db.models import Count, Q + stats = ProjectHealthMetrics.get_latest_health_metrics().aggregate( + total=Count("id"), + compliant=Count("id", filter=Q(is_level_compliant=True)), + ) + total = stats.get("total") or 0 + compliant = stats.get("compliant") or 0 + non_compliant = total - compliant + compliance_rate = (compliant / total * 100.0) if total else 0.0Add the import at the top of the file:
from django.db.models import Count, Q
79-88: Prefer numeric fields in structured logs; include updated_countMake logs easier to query by storing numerics, not formatted strings. Also include updated_count.
logger.info( "Compliance detection completed successfully", extra={ - "execution_time": f"{execution_time:.2f}s", + "execution_time_s": round(execution_time, 2), "dry_run": dry_run, "total_projects": total, "compliant_projects": compliant, "non_compliant_projects": non_compliant, - "compliance_rate": f"{compliance_rate:.1f}%", + "compliance_rate_pct": round(compliance_rate, 1), + "updated_count": updated_count, }, )
35-60: End-to-end tests for command behavior (dry-run, empty payload, logging, counts)Per #2039 acceptance criteria, please add pytest tests for:
- fetch failure or empty payload → CommandError and stderr message
- dry-run mode → no DB writes; “DRY RUN” messaging
- normal run → DB writes occur; final summary reflects counts
- verbose flag → utils loggers switch to DEBUG (can assert logger level or emitted debug logs)
I can scaffold tests using call_command and unittest.mock for the fetcher and detector.
Would you like me to generate a test module under backend/tests/apps/owasp/management/commands/ covering these cases?
101-101: Ensure trailing newline at EOFKeep linters happy (W292).
- raise CommandError(error_msg) from e + raise CommandError(error_msg) from e +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (5)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py(1 hunks)backend/apps/owasp/migrations/0048_add_compliance_penalty_weight.py(1 hunks)backend/apps/owasp/utils/compliance_detector.py(1 hunks)backend/apps/owasp/utils/project_level_fetcher.py(1 hunks)cron/production(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/migrations/0048_add_compliance_penalty_weight.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-08-04T15:55:08.017Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1967
File: backend/apps/owasp/api/internal/filters/project_health_metrics.py:24-27
Timestamp: 2025-08-04T15:55:08.017Z
Learning: In the OWASP Nest project, the `is_active` filter in `ProjectHealthMetricsFilter` is intentionally designed as a workaround to eliminate inactive projects from the dashboard. It only filters FOR active projects when `value=True`, and returns an empty Q() when `value=False` to avoid showing inactive projects. This is not meant to be a general boolean filter but a specific solution to exclude inactive projects from the project health metrics dashboard.
Applied to files:
backend/apps/owasp/utils/compliance_detector.py
📚 Learning: 2025-06-29T00:41:32.198Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#1676
File: backend/apps/owasp/graphql/filters/project_health_metrics.py:17-22
Timestamp: 2025-06-29T00:41:32.198Z
Learning: In the OWASP Nest codebase, when implementing GraphQL filters that convert string values to enums (like ProjectLevel), do not catch ValueError exceptions for invalid values. Let the errors propagate to provide proper error responses to GraphQL clients rather than silently ignoring invalid input.
Applied to files:
backend/apps/owasp/utils/project_level_fetcher.py
🧬 Code Graph Analysis (2)
backend/apps/owasp/utils/compliance_detector.py (2)
backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
project_health_metrics(25-42)backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(16-240)get_latest_health_metrics(165-179)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (4)
backend/apps/owasp/api/internal/queries/project_health_metrics.py (1)
project_health_metrics(25-42)backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(16-240)get_latest_health_metrics(165-179)backend/apps/owasp/utils/compliance_detector.py (1)
detect_and_update_compliance(14-91)backend/apps/owasp/utils/project_level_fetcher.py (1)
fetch_official_project_levels(13-59)
🪛 Ruff (0.12.2)
backend/apps/owasp/utils/compliance_detector.py
15-15: Trailing whitespace
Remove trailing whitespace
(W291)
16-16: Boolean-typed positional argument in function definition
(FBT001)
16-16: Boolean default positional argument in function definition
(FBT002)
19-19: Blank line contains whitespace
Remove whitespace from blank line
(W293)
23-23: Blank line contains whitespace
Remove whitespace from blank line
(W293)
24-24: Missing blank line after last section ("Returns")
Add blank line after "Returns"
(D413)
30-30: Trailing whitespace
Remove trailing whitespace
(W291)
43-43: Blank line contains whitespace
Remove whitespace from blank line
(W293)
80-80: Trailing whitespace
Remove trailing whitespace
(W291)
81-81: Trailing whitespace
Remove trailing whitespace
(W291)
90-90: Blank line contains whitespace
Remove whitespace from blank line
(W293)
91-91: No newline at end of file
Add trailing newline
(W292)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
55-55: Abstract raise to an inner function
(TRY301)
101-101: No newline at end of file
Add trailing newline
(W292)
backend/apps/owasp/utils/project_level_fetcher.py
52-52: Consider moving this statement to an else block
(TRY300)
🔇 Additional comments (2)
backend/apps/owasp/utils/project_level_fetcher.py (1)
13-59: Fetcher looks solid and robustGood use of Accept header, response.json(), shape validation, and defensive parsing. Logging on failures is appropriate.
backend/apps/owasp/utils/compliance_detector.py (1)
28-69: Good normalization, dry-run behavior, and bulk_update usage
- Keys and levels normalization avoids whitespace/case mismatches.
- select_related("project") prevents N+1.
- dry_run is honored; updates are batched.
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
cron/production (1)
5-5: LGTM: detection runs before scoring nowScheduling compliance detection at 17:21 ensures the score update at 17:22 uses fresh compliance flags. This aligns with earlier feedback.
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
15-17: Tests still needed per acceptance criteriaDraft noted; when you add tests, cover fetch failures/empty payload, dry-run vs update behavior, summary counts, and logging.
I can scaffold pytest tests using call_command and mocks for fetch_official_project_levels and detect_and_update_compliance. Want me to generate them?
🧹 Nitpick comments (5)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (5)
66-70: Avoid materializing the entire QuerySet; use DB-side countsCurrent approach iterates the full result set to compute counts. Use .count() and a filtered count to reduce memory and latency on large datasets.
- latest_metrics = ProjectHealthMetrics.get_latest_health_metrics() - total = len(latest_metrics) - compliant = sum(1 for m in latest_metrics if m.is_level_compliant) - non_compliant = total - compliant + latest_metrics = ProjectHealthMetrics.get_latest_health_metrics() + total = latest_metrics.count() + compliant = latest_metrics.filter(is_level_compliant=True).count() + non_compliant = total - compliant
42-44: Verbose mode should enable this command’s logger tooYou’re setting DEBUG only for apps.owasp.utils. Also set this module’s logger (and optionally apps.owasp) so verbose mode covers all relevant logs.
if verbose: - logging.getLogger("apps.owasp.utils").setLevel(logging.DEBUG) + logging.getLogger("apps.owasp.utils").setLevel(logging.DEBUG) + logging.getLogger("apps.owasp").setLevel(logging.DEBUG) + logger.setLevel(logging.DEBUG)
81-91: Standardize telemetry key for execution timeSuccess log uses execution_time while error log uses execution_time_s. Unify to a single key for easier aggregation.
logger.info( "Compliance detection completed successfully", extra={ - "execution_time": f"{execution_time:.2f}s", + "execution_time_s": round(execution_time, 2), "dry_run": dry_run, "total_projects": total, "compliant_projects": compliant, "non_compliant_projects": non_compliant, "compliance_rate": f"{compliance_rate:.1f}%", }, )
92-103: Avoid double-wrapping CommandErrorFailures raised intentionally via fail(...) become CommandError, which are then caught and re-wrapped here. Let CommandError bubble unchanged; keep structured logging for unexpected exceptions.
- except Exception as e: + except CommandError: + # Let explicit command failures propagate without double-wrapping + raise + except Exception as e: execution_time = time.perf_counter() - start_time error_msg = f"Compliance detection failed after {execution_time:.2f}s: {e!s}" logger.exception( "Compliance detection failed", extra={ "execution_time_s": round(execution_time, 2), "error": e.__class__.__name__, }, ) raise CommandError(error_msg) from e
103-103: Add trailing newline (W292)File is missing a trailing newline; add one to satisfy linters.
- raise CommandError(error_msg) from e + raise CommandError(error_msg) from e +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py(1 hunks)cron/production(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (3)
backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(16-240)get_latest_health_metrics(165-179)backend/apps/owasp/utils/compliance_detector.py (1)
detect_and_update_compliance(14-91)backend/apps/owasp/utils/project_level_fetcher.py (1)
fetch_official_project_levels(13-59)
🪛 Ruff (0.12.2)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
54-54: Abstract raise to an inner function
(TRY301)
103-103: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
56-61: Good: treat empty payload as failure and report count on successThe None/empty check prevents misleading “success” with zero records, and the success message is concise.
|
@arkid15r Do these look good? If yes, I’ll go ahead and start creating the necessary test files. |
There was a problem hiding this comment.
This has to be reshaped a lot:
- add
project_level_officialfield forowasp::Projectmodel: the project is not compliant when its project_level != project_level_official - update project sync process with populating the new field
- move the new logic to existing project health/score related commands
Also you need to start following the contribution guidelines and run code quality checks if you expect your PRs to be reviewed.
…hub.com/divyanshu-vr/Nest into feature/ProjectLevelComplianceDetection
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
60-75: Thanks for implementing penalty clamping and final score boundsClamping the penalty to [0, 100] and capping the final score 0–100 addresses earlier feedback and guards against bad data. 👍
🧹 Nitpick comments (42)
backend/tests/apps/owasp/models/project_test.py (1)
135-155: Solid coverage for is_level_compliant; consider a couple of edge casesThe parametrized matrix and default case look good. Two optional improvements:
- Add a case where raw strings are used instead of TextChoices to ensure equality still behaves as expected (Django stores string values on assignment).
- Add a case where local level is set to OTHER and official is non-OTHER to assert False explicitly (complements existing tests).
Would you like me to add those tests?
backend/apps/owasp/migrations/0049_remove_projecthealthrequirements_owasp_compliance_penalty_weight_0_100_and_more.py (1)
13-19: Style: consider ignoring Ruff Q000 in migrations or switch to double quotesRuff flags single quotes here. Since migrations are auto-generated, either:
- Add a per-file ignore for Q000 in this migration, or
- Switch to double quotes to satisfy the linter.
Apply this diff if you prefer quotes change:
- ('owasp', '0048_add_compliance_penalty_weight'), + ("owasp", "0048_add_compliance_penalty_weight"), @@ - model_name='projecthealthrequirements', - name='compliance_penalty_weight', - field=models.FloatField(default=10.0, help_text='Percentage penalty applied to non-compliant projects (0-100)', validators=[django.core.validators.MinValueValidator(0.0), django.core.validators.MaxValueValidator(100.0)], verbose_name='Compliance penalty weight (%)'), + model_name="projecthealthrequirements", + name="compliance_penalty_weight", + field=models.FloatField( + default=10.0, + help_text="Percentage penalty applied to non-compliant projects (0-100)", + validators=[ + django.core.validators.MinValueValidator(0.0), + django.core.validators.MaxValueValidator(100.0), + ], + verbose_name="Compliance penalty weight (%)", + ),backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
60-75: Avoid noisy warnings when penalty is 0% and wrap long lines (fix E501)Currently a warning is emitted even when the clamped penalty is 0%, and the f-string line length exceeds 99 chars. Recommend logging only when a non-zero penalty applies and wrapping the message.
Apply this diff in this block:
- if not metric.project.is_level_compliant: + if not metric.project.is_level_compliant: penalty_percentage = float(getattr(requirements, "compliance_penalty_weight", 0.0)) # Clamp to [0, 100] penalty_percentage = max(0.0, min(100.0, penalty_percentage)) penalty_amount = score * (penalty_percentage / 100.0) score = max(0.0, score - penalty_amount) - self.stdout.write( - self.style.WARNING( - f"Applied {penalty_percentage}% compliance penalty to {metric.project.name} " - f"(penalty: {penalty_amount:.2f}, final score: {score:.2f}) " - f"[Local: {metric.project.level}, Official: {metric.project.project_level_official}]" - ) - ) + if penalty_percentage > 0.0: + msg = ( + f"Applied {penalty_percentage}% compliance penalty to {metric.project.name} " + f"(penalty: {penalty_amount:.2f}, final score: {score:.2f}) " + f"[Local: {metric.project.level}, Official: {metric.project.project_level_official}]" + ) + self.stdout.write(self.style.WARNING(msg))backend/apps/owasp/models/project.py (2)
63-69: New official level field is appropriate; consider indexing if queried oftenAdding project_level_official with choices/default is aligned with the workflow. If you’ll frequently query by official level (e.g., reports), consider adding a DB index.
Example migration snippet:
migrations.AddIndex( model_name="project", index=models.Index(fields=["project_level_official"], name="project_official_level_idx"), )
161-165: Property-based compliance is simple and effective; confirm acceptance criteria on persistenceThe derived property is fine for checks/penalties. The linked issue mentioned “introduce a new boolean field to mark projects as non-compliant.” If a persistent flag is still required for analytics or filtering, confirm whether:
- A stored boolean exists elsewhere (e.g., on ProjectHealthMetrics), or
- The property suffices and the issue can be updated accordingly.
If you need a stored flag on Project with automatic maintenance, I can draft the model/migration and signal logic.
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py (4)
17-23: Fixture doesn’t need yield; remove unused Command instanceThe autouse fixture doesn’t tear down and the instantiated Command isn’t used.
Apply this diff:
- @pytest.fixture(autouse=True) - def _setup(self): + @pytest.fixture(autouse=True) + def _setup(self): """Set up test environment.""" self.stdout = StringIO() - self.command = Command() - yield + returnAnd drop the unused import:
-from apps.owasp.management.commands.owasp_detect_project_level_compliance import Command
60-88: Mixed scenario coverage is clear; tiny readability tweakOptional: use keyword for the boolean to avoid FBT003 lint in function calls.
Example:
- self.create_mock_project("OWASP ZAP", "flagship", "flagship", True), + self.create_mock_project("OWASP ZAP", "flagship", "flagship", is_compliant=True),
126-147: Default official levels info path: watch for special characters in CIYou assert on a line containing “ℹ”. Depending on CI locale, that glyph can be stripped/changed. If you hit flaky output, consider asserting on the tail of the message without the symbol.
Example:
assert "projects have default official levels" in output
148-175: Parametric rate calc test is strong; minor lint fixes and EOF newline
- A couple of long lines >99 chars and trailing whitespace are flagged by Ruff.
- Add a trailing newline at EOF to silence W292.
Run “ruff --fix” on this file, or I can push a formatted patch if you prefer.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (7)
85-152: Good end-to-end penalty scenario; tighten stdout assertions and avoid over-coupling to formattingThe scenario is solid. Two nits:
- Asserting substrings like "penalty:" and "final score:" risks brittleness if formatting changes. Prefer explicit, separate assertions for each expected token or a regex that tolerates spacing.
- Since Project.is_level_compliant is a property in the model, setting a boolean attribute on a MagicMock works here because mock_metric.project is itself a MagicMock. If we later switch to spec-enforced mocks, use PropertyMock to avoid surprises.
Would you like me to switch these checks to precise regex assertions and wire PropertyMock for is_level_compliant now?
141-142: Remove unused variable to satisfy Ruff (F841)expected_score is assigned but not used.
- expected_score = 72.0 - +
150-150: Split combined assertion for clarity (PT018)Break this into two asserts for better failure messages.
- assert "penalty:" in output and "final score:" in output + assert "penalty:" in output + assert "final score:" in output
153-210: Compliant path looks good; consider asserting that no penalty line is printedThe ≥ comparison is fine, but also assert absence of the "Applied X% compliance penalty" line for stronger signal.
output = self.stdout.getvalue() - assert "compliance penalty" not in output + assert "compliance penalty" not in output + assert "Applied " not in output
211-253: Zero-penalty case: assertion intent is good; keep only the base-score check and explicit log checkTwo small nits:
- expected_base_score is unused.
- Keep the explicit "Applied 0.0% compliance penalty" check as-is; it's valuable.
- expected_base_score = 90.0 # All fields meet requirements - # With zero penalty, score should be the base score + # With zero penalty, score should remain at the base score assert mock_metric.score >= 90.0 # Should be base score or higher
291-338: Nice clamping coverage; also assert that values are numerically clamped in score, not just logsYou already assert the log message. Consider also validating the relationship between prior (base) score and final score for each clamp, e.g., 50% cuts the base in half.
I can add a small helper to compute a predictable base score in-test and assert the exact clamped result for (-10→0, 150→100, 50→50).
109-330: Trailing whitespace and style nits (W293/E501/PT018/F841)There are several trailing whitespace lines (W293), a couple of long lines (E501), one combined assertion (PT018), and a few unused variables (F841). Running ruff --fix and black should clean these up quickly.
I can push a formatting-only commit that fixes W293/E501/PT018/F841 across this file.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (6)
3-3: Remove unused importjson(F401)Not used anywhere in this module.
-from unittest.mock import MagicMock, patch -import json +from unittest.mock import MagicMock, patch
79-108: Normalize patch decorators to double quotes (Q000) and keep style consistentHouse style prefers double quotes. This is cosmetic but keeps diffs tidy.
-@patch('requests.get') +@patch("requests.get")Also applies to: 109-141, 142-165, 232-242
165-201: Selective update logic looks correct; assert that unchanged projects are not passed to bulk_saveGood job updating only changed projects. You already assert the list length. As a micro-hardening, also assert that bulk_save is called with fields=["project_level_official"] for intent clarity.
- mock_bulk_save.assert_called_once() + mock_bulk_save.assert_called_once() + _, kwargs = mock_bulk_save.call_args + assert kwargs.get("fields") == ["project_level_official"]
220-231: Level-mapping coverage is comprehensive; consider parametrization for brevityThe loop is fine. Using pytest.mark.parametrize would reduce boilerplate and make failures pinpoint which case failed.
I can rewrite this block with @pytest.mark.parametrize if you want a lighter test body.
243-275: Integration test assertions align with command behavior; avoid brittle count wordingYou assert "Updated official levels for 1 projects". If that wording changes ("project(s)"), this will fail. Consider asserting the presence of the number and keyword separately or using a regex.
- assert "Updated official levels for 1 projects" in self.stdout.getvalue() + output = self.stdout.getvalue() + assert "Updated official levels for " in output and "1" in output
3-297: Minor style/lint: trailing whitespace, long lines, and an unused local (F841/W293/E501)
- Several W293 instances (blank lines with spaces).
- A few long lines > 99 characters.
- mock_bulk_save assigned but not used in one test (F841).
Run ruff --fix and black on this file; I can push a formatting-only change if helpful.
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (6)
3-11: Remove unused importsStringIO and ProjectHealthMetrics are not used.
-import logging -from io import StringIO +import logging @@ -from apps.owasp.models.project_health_metrics import ProjectHealthMetrics
33-41: Avoid callingselect_related()without fields; narrow the query to the needed columnsYou only need name, level, and project_level_official. Reducing the selected columns lowers memory and DB IO.
- active_projects = Project.objects.filter(is_active=True).select_related() + active_projects = ( + Project.objects + .filter(is_active=True) + .only("name", "level", "project_level_official") + )
49-52: Long line and Unicode marker: simplify and wrap for portability
- The line exceeds the style limit.
- The Unicode cross is fine, but avoid surprises in terminals. Consider plain ASCII and rely on style to emphasize.
- f"✗ {project.name}: Local={project.level}, Official={project.project_level_official}" + ( + f"{project.name}: Local={project.level}, " + f"Official={project.project_level_official}" + )
68-73: Prefer clearer, wrapped messaging and avoid heavy Unicode glyphs in logsTwo nits:
- Wrap long lines.
- Consider avoiding "⚠" to ensure logs render well in all environments (CI, Windows terminals). Style.WARNING already provides emphasis.
- self.stdout.write(f"\n{self.style.WARNING('⚠ WARNING: Found ' + str(non_compliant_count) + ' non-compliant projects')}") + self.stdout.write( + "\n" + self.style.WARNING(f"WARNING: Found {non_compliant_count} non-compliant projects") + )
85-96: Use of_metais acceptable in Django, but replace NOTICE and unify quotes
- Accessing _meta is a standard Django pattern; ignore SLF001 here.
- Django’s default style has SUCCESS/WARNING/ERROR; NOTICE may not exist depending on version. If this repo doesn’t augment color_style, this will raise at runtime.
If NOTICE isn’t guaranteed, switch to self.stdout.write for info lines or self.style.SUCCESS/ERROR/WARNING. Also prefer double quotes per code style.
- default_level = Project._meta.get_field('project_level_official').default + default_level = Project._meta.get_field("project_level_official").default @@ - self.stdout.write( - f"\n{self.style.NOTICE('ℹ INFO: ' + str(projects_without_official_level) + ' projects have default official levels')}" - ) - self.stdout.write("Run 'owasp_update_project_health_metrics' to sync official levels from OWASP GitHub.") + self.stdout.write( + f"\nINFO: {projects_without_official_level} projects have default official levels" + ) + self.stdout.write( + "Run 'owasp_update_project_health_metrics' to sync official levels from OWASP GitHub." + )
14-27: Add basic tests for this command (acceptance criteria)Per #2039, we still need tests that execute this command and assert:
- Correct counts and compliance rate with mixed data.
- Verbose output on/off behavior.
- Info notice printed when default official levels are present.
I can scaffold backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py with pytest + call_command and MagicMocks.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (5)
34-80: Robust fetch with good validation; minor logging nitThe fetch path and validation look good. Two tiny nits:
- Prefer {e!s} over str(e) in logger extras (RUF010).
- After validating to list, consider early-returning empty dict in place of None to allow a distinct “empty payload” branch, if you want to distinguish failure vs. empty.
- logger.exception( + logger.exception( "Failed to fetch project levels", - extra={"url": OWASP_PROJECT_LEVELS_URL, "error": str(e)}, + extra={"url": OWASP_PROJECT_LEVELS_URL, "error": f"{e!s}"}, )
81-127: Update mapping is correct; guard against missing/blank project namesTwo suggestions:
- project.name can theoretically be None/blank; guard before .strip() to avoid AttributeError.
- Use constants for mapped levels if available (e.g., ProjectLevel.LAB) to avoid typos creeping in.
- for project in Project.objects.filter(is_active=True): - normalized_project_name = project.name.strip().lower() + for project in Project.objects.filter(is_active=True): + if not project.name: + continue + normalized_project_name = project.name.strip().lower() @@ - mapped_level = level_mapping.get(official_level, "other") + mapped_level = level_mapping.get(official_level, "other")
129-142: Handle empty payloads explicitly to avoid “success” phrasingWhen official_levels is an empty dict, you currently print a failure warning. That’s fine; optionally, you can tighten the condition and wording to be explicit.
- if official_levels: + if official_levels is not None and len(official_levels) > 0: self.stdout.write(f"Successfully fetched {len(official_levels)} official project levels") self.update_official_levels(official_levels) else: - self.stdout.write(self.style.WARNING("Failed to fetch official project levels, continuing without updates")) + self.stdout.write( + self.style.WARNING("Failed to fetch official project levels or received empty payload; continuing without updates") + )
166-176: NOTICE style usage may not be portable across Django versionsUsing self.style.NOTICE can raise AttributeError in some Django versions. If your project defines NOTICE, ignore this. Otherwise, swap to plain write or SUCCESS/WARNING.
- self.stdout.write(self.style.NOTICE(f"Evaluating metrics for project: {project.name}")) + self.stdout.write(f"Evaluating metrics for project: {project.name}")
138-141: Wrap long lines (E501)Two lines exceed the configured limit. Wrap to satisfy E501.
- self.stdout.write(f"Successfully fetched {len(official_levels)} official project levels") + self.stdout.write( + f"Successfully fetched {len(official_levels)} official project levels" + ) @@ - self.stdout.write(self.style.WARNING("Failed to fetch official project levels, continuing without updates")) + self.stdout.write( + self.style.WARNING( + "Failed to fetch official project levels, continuing without updates" + ) + )backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py (9)
5-5: Remove unused importjson(F401)Not used in this module.
-from unittest.mock import MagicMock, patch -import json +from unittest.mock import MagicMock, patch
20-24: Fixture should return rather than yield (PT022)There’s no teardown; use return to be explicit.
def _setup(self): """Set up test environment.""" self.stdout = StringIO() - yield + return
26-45: Helper naming/docstrings and whitespace nits
- Docstrings should be imperative (D401).
- Remove trailing/blank-line whitespace (W291/W293).
- def create_mock_project(self, name, local_level, official_level=None): - """Helper to create a mock project with specified levels.""" + def create_mock_project(self, name, local_level, official_level=None): + """Create a mock project with specified levels.""" @@ - return project
80-132: Great full-workflow coverage; avoid brittle exact phrasing in stdout assertionsMessages like "Updated official levels for 2 projects" can change. Consider checking the intent rather than exact wording (e.g., contains "Updated official levels" and "2").
- assert "Updated official levels for 2 projects" in output + assert "Updated official levels" in output and "2" in output
141-168: Penalty logging assertions are meaningful; also assert that compliant projects don’t log penaltiesYou already assert the non-compliant path; add negative assertions for the compliant ones here too for stronger signal.
output = self.stdout.getvalue() + assert "compliance penalty to OWASP ZAP" not in output + assert "compliance penalty to OWASP Missing" not in output assert "compliance penalty to OWASP WebGoat" in output
169-211: Level mapping test is solid; switch decorators to double quotes (Q000)Cosmetic: prefer double quotes to align with codebase style.
- @patch('requests.get') + @patch("requests.get")
256-307: Targeted logging checks are helpful; consider parametrizing scenariosThe table-driven approach is nice; pytest.mark.parametrize would make this tighter and give better failure messages per case.
I can convert this to a parametrized test and keep your readable scenario names.
308-332: Edge-case clamping: good assert; add explicit check for clamped percentage in stdoutYou assert score==0.0. Also assert the log reflects a 100% clamp for completeness.
output = self.stdout.getvalue() assert "Applied 100.0% compliance penalty" in output + assert "[Local: lab, Official: flagship]" in output
1-332: General style fixes (Q000/W293/E501/F841)Multiple trailing whitespace, long lines, and a few unused locals (e.g., mock_project_bulk_save, mock_metrics_bulk_save, mock_scores_bulk_save). Running ruff --fix and black will resolve these cleanly.
Happy to push a formatting-only change across this test module.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (12)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_scores.py(1 hunks)backend/apps/owasp/migrations/0049_remove_projecthealthrequirements_owasp_compliance_penalty_weight_0_100_and_more.py(1 hunks)backend/apps/owasp/models/project.py(2 hunks)backend/apps/owasp/models/project_health_requirements.py(2 hunks)backend/apps/owasp/utils/__init__.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py(1 hunks)backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py(1 hunks)backend/tests/apps/owasp/models/project_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/owasp/utils/init.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/models/project_health_requirements.py
🧰 Additional context used
🧬 Code graph analysis (9)
backend/apps/owasp/models/project.py (1)
backend/apps/owasp/models/enums/project.py (1)
ProjectLevel(37-44)
backend/tests/apps/owasp/models/project_test.py (2)
backend/apps/owasp/models/enums/project.py (1)
ProjectLevel(37-44)backend/apps/owasp/models/project.py (2)
Project(31-385)is_level_compliant(162-164)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (1)
backend/apps/owasp/models/project.py (2)
Project(31-385)bulk_save(352-360)
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py (2)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
Command(14-96)backend/apps/owasp/models/project.py (2)
Project(31-385)is_level_compliant(162-164)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
fetch_official_project_levels(34-79)update_official_levels(81-127)backend/apps/owasp/models/project.py (1)
Project(31-385)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (3)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(16-235)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)backend/apps/owasp/models/project.py (3)
is_level_compliant(162-164)is_funding_requirements_compliant(150-152)is_leader_requirements_compliant(155-159)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (3)
backend/apps/owasp/models/project.py (2)
Project(31-385)is_level_compliant(162-164)backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (3)
Command(17-176)add_arguments(20-32)handle(129-176)backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (2)
Command(9-84)handle(12-84)
backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py (5)
backend/apps/owasp/models/project.py (4)
Project(31-385)is_funding_requirements_compliant(150-152)is_leader_requirements_compliant(155-159)is_level_compliant(162-164)backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(16-235)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (1)
_setup(17-29)backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (1)
_setup(16-34)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
backend/apps/owasp/models/project.py (1)
is_level_compliant(162-164)
🪛 Ruff (0.12.2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
40-40: Missing blank line after last section ("Returns")
Add blank line after "Returns"
(D413)
72-72: Consider moving this statement to an else block
(TRY300)
83-83: Blank line contains whitespace
Remove whitespace from blank line
(W293)
86-86: Blank line contains whitespace
Remove whitespace from blank line
(W293)
87-87: Missing blank line after last section ("Returns")
Add blank line after "Returns"
(D413)
92-92: Blank line contains whitespace
Remove whitespace from blank line
(W293)
98-98: Blank line contains whitespace
Remove whitespace from blank line
(W293)
115-115: Blank line contains whitespace
Remove whitespace from blank line
(W293)
120-120: Blank line contains whitespace
Remove whitespace from blank line
(W293)
126-126: Blank line contains whitespace
Remove whitespace from blank line
(W293)
132-132: Blank line contains whitespace
Remove whitespace from blank line
(W293)
138-138: Line too long (105 > 99)
(E501)
141-141: Line too long (124 > 99)
(E501)
142-142: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py
22-22: No teardown in fixture _setup, use return instead of yield
Replace yield with return
(PT022)
25-25: First line of docstring should be in imperative mood: "Helper to create a mock project."
(D401)
38-38: Boolean positional value in function call
(FBT003)
39-39: Boolean positional value in function call
(FBT003)
40-40: Boolean positional value in function call
(FBT003)
45-45: Blank line contains whitespace
Remove whitespace from blank line
(W293)
47-47: Blank line contains whitespace
Remove whitespace from blank line
(W293)
49-49: Blank line contains whitespace
Remove whitespace from blank line
(W293)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
64-64: Boolean positional value in function call
(FBT003)
65-65: Boolean positional value in function call
(FBT003)
66-66: Boolean positional value in function call
(FBT003)
71-71: Blank line contains whitespace
Remove whitespace from blank line
(W293)
73-73: Blank line contains whitespace
Remove whitespace from blank line
(W293)
75-75: Blank line contains whitespace
Remove whitespace from blank line
(W293)
77-77: Blank line contains whitespace
Remove whitespace from blank line
(W293)
84-84: Blank line contains whitespace
Remove whitespace from blank line
(W293)
92-92: Boolean positional value in function call
(FBT003)
93-93: Boolean positional value in function call
(FBT003)
98-98: Blank line contains whitespace
Remove whitespace from blank line
(W293)
100-100: Blank line contains whitespace
Remove whitespace from blank line
(W293)
102-102: Blank line contains whitespace
Remove whitespace from blank line
(W293)
104-104: Blank line contains whitespace
Remove whitespace from blank line
(W293)
113-113: Blank line contains whitespace
Remove whitespace from blank line
(W293)
115-115: Blank line contains whitespace
Remove whitespace from blank line
(W293)
117-117: Blank line contains whitespace
Remove whitespace from blank line
(W293)
119-119: Blank line contains whitespace
Remove whitespace from blank line
(W293)
129-129: Boolean positional value in function call
(FBT003)
130-130: Boolean positional value in function call
(FBT003)
130-130: Line too long (102 > 99)
(E501)
135-135: Blank line contains whitespace
Remove whitespace from blank line
(W293)
139-139: Blank line contains whitespace
Remove whitespace from blank line
(W293)
141-141: Blank line contains whitespace
Remove whitespace from blank line
(W293)
143-143: Blank line contains whitespace
Remove whitespace from blank line
(W293)
145-145: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
157-157: Line too long (105 > 99)
(E501)
159-159: Line too long (110 > 99)
(E501)
165-165: Blank line contains whitespace
Remove whitespace from blank line
(W293)
168-168: Blank line contains whitespace
Remove whitespace from blank line
(W293)
170-170: Blank line contains whitespace
Remove whitespace from blank line
(W293)
172-172: Blank line contains whitespace
Remove whitespace from blank line
(W293)
175-175: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py
3-3: json imported but unused
Remove unused import: json
(F401)
79-79: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
109-109: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
118-118: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
130-130: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
142-142: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
185-185: Blank line contains whitespace
Remove whitespace from blank line
(W293)
187-187: Blank line contains whitespace
Remove whitespace from blank line
(W293)
189-189: Blank line contains whitespace
Remove whitespace from blank line
(W293)
194-194: Blank line contains whitespace
Remove whitespace from blank line
(W293)
222-222: Local variable mock_bulk_save is assigned to but never used
Remove assignment to unused variable mock_bulk_save
(F841)
223-223: Blank line contains whitespace
Remove whitespace from blank line
(W293)
226-226: Blank line contains whitespace
Remove whitespace from blank line
(W293)
229-229: Blank line contains whitespace
Remove whitespace from blank line
(W293)
232-232: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
259-259: Line too long (122 > 99)
(E501)
261-261: Blank line contains whitespace
Remove whitespace from blank line
(W293)
263-263: Blank line contains whitespace
Remove whitespace from blank line
(W293)
265-265: Blank line contains whitespace
Remove whitespace from blank line
(W293)
270-270: Blank line contains whitespace
Remove whitespace from blank line
(W293)
291-291: Line too long (114 > 99)
(E501)
293-293: Blank line contains whitespace
Remove whitespace from blank line
(W293)
295-295: Blank line contains whitespace
Remove whitespace from blank line
(W293)
297-297: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/apps/owasp/migrations/0049_remove_projecthealthrequirements_owasp_compliance_penalty_weight_0_100_and_more.py
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
10-10: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
15-15: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
16-16: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
17-17: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py
109-109: Blank line contains whitespace
Remove whitespace from blank line
(W293)
113-113: Blank line contains whitespace
Remove whitespace from blank line
(W293)
119-119: Blank line contains whitespace
Remove whitespace from blank line
(W293)
123-123: Blank line contains whitespace
Remove whitespace from blank line
(W293)
126-126: Blank line contains whitespace
Remove whitespace from blank line
(W293)
130-130: Blank line contains whitespace
Remove whitespace from blank line
(W293)
139-139: Found commented-out code
Remove commented-out code
(ERA001)
141-141: Local variable expected_score is assigned to but never used
Remove assignment to unused variable expected_score
(F841)
142-142: Blank line contains whitespace
Remove whitespace from blank line
(W293)
146-146: Blank line contains whitespace
Remove whitespace from blank line
(W293)
150-150: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
176-176: Blank line contains whitespace
Remove whitespace from blank line
(W293)
180-180: Blank line contains whitespace
Remove whitespace from blank line
(W293)
186-186: Blank line contains whitespace
Remove whitespace from blank line
(W293)
189-189: Blank line contains whitespace
Remove whitespace from blank line
(W293)
192-192: Blank line contains whitespace
Remove whitespace from blank line
(W293)
196-196: Blank line contains whitespace
Remove whitespace from blank line
(W293)
202-202: Local variable expected_score is assigned to but never used
Remove assignment to unused variable expected_score
(F841)
203-203: Blank line contains whitespace
Remove whitespace from blank line
(W293)
206-206: Blank line contains whitespace
Remove whitespace from blank line
(W293)
215-215: Blank line contains whitespace
Remove whitespace from blank line
(W293)
219-219: Line too long (102 > 99)
(E501)
224-224: Blank line contains whitespace
Remove whitespace from blank line
(W293)
230-230: Blank line contains whitespace
Remove whitespace from blank line
(W293)
233-233: Blank line contains whitespace
Remove whitespace from blank line
(W293)
236-236: Blank line contains whitespace
Remove whitespace from blank line
(W293)
240-240: Blank line contains whitespace
Remove whitespace from blank line
(W293)
246-246: Local variable expected_base_score is assigned to but never used
Remove assignment to unused variable expected_base_score
(F841)
249-249: Blank line contains whitespace
Remove whitespace from blank line
(W293)
258-258: Blank line contains whitespace
Remove whitespace from blank line
(W293)
262-262: Line too long (102 > 99)
(E501)
267-267: Blank line contains whitespace
Remove whitespace from blank line
(W293)
273-273: Blank line contains whitespace
Remove whitespace from blank line
(W293)
276-276: Blank line contains whitespace
Remove whitespace from blank line
(W293)
279-279: Blank line contains whitespace
Remove whitespace from blank line
(W293)
283-283: Blank line contains whitespace
Remove whitespace from blank line
(W293)
295-295: Blank line contains whitespace
Remove whitespace from blank line
(W293)
299-299: Line too long (102 > 99)
(E501)
304-304: Blank line contains whitespace
Remove whitespace from blank line
(W293)
310-310: Blank line contains whitespace
Remove whitespace from blank line
(W293)
313-313: Blank line contains whitespace
Remove whitespace from blank line
(W293)
320-320: Blank line contains whitespace
Remove whitespace from blank line
(W293)
323-323: Blank line contains whitespace
Remove whitespace from blank line
(W293)
327-327: Blank line contains whitespace
Remove whitespace from blank line
(W293)
330-330: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
4-4: io.StringIO imported but unused
Remove unused import: io.StringIO
(F401)
9-9: apps.owasp.models.project_health_metrics.ProjectHealthMetrics imported but unused
Remove unused import: apps.owasp.models.project_health_metrics.ProjectHealthMetrics
(F401)
16-16: Blank line contains whitespace
Remove whitespace from blank line
(W293)
30-30: Blank line contains whitespace
Remove whitespace from blank line
(W293)
32-32: Blank line contains whitespace
Remove whitespace from blank line
(W293)
35-35: Blank line contains whitespace
Remove whitespace from blank line
(W293)
38-38: Blank line contains whitespace
Remove whitespace from blank line
(W293)
50-50: Line too long (109 > 99)
(E501)
53-53: Blank line contains whitespace
Remove whitespace from blank line
(W293)
59-59: Blank line contains whitespace
Remove whitespace from blank line
(W293)
67-67: Blank line contains whitespace
Remove whitespace from blank line
(W293)
69-69: Line too long (132 > 99)
(E501)
70-70: Line too long (109 > 99)
(E501)
73-73: Blank line contains whitespace
Remove whitespace from blank line
(W293)
84-84: Blank line contains whitespace
Remove whitespace from blank line
(W293)
86-86: Private member accessed: _meta
(SLF001)
86-86: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
91-91: Blank line contains whitespace
Remove whitespace from blank line
(W293)
94-94: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
94-94: Line too long (134 > 99)
(E501)
96-96: Line too long (117 > 99)
(E501)
96-96: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py
5-5: json imported but unused
Remove unused import: json
(F401)
24-24: No teardown in fixture _setup, use return instead of yield
Replace yield with return
(PT022)
27-27: First line of docstring should be in imperative mood: "Helper to create a mock project with specified levels."
(D401)
33-33: Blank line contains whitespace
Remove whitespace from blank line
(W293)
35-35: Trailing whitespace
Remove trailing whitespace
(W291)
43-43: Blank line contains whitespace
Remove whitespace from blank line
(W293)
47-47: First line of docstring should be in imperative mood: "Helper to create a mock health metric for a project."
(D401)
50-50: Blank line contains whitespace
Remove whitespace from blank line
(W293)
54-54: Line too long (102 > 99)
(E501)
58-58: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
65-65: First line of docstring should be in imperative mood: "Helper to create mock health requirements."
(D401)
69-69: Blank line contains whitespace
Remove whitespace from blank line
(W293)
73-73: Line too long (102 > 99)
(E501)
77-77: Blank line contains whitespace
Remove whitespace from blank line
(W293)
80-80: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
82-82: Line too long (109 > 99)
(E501)
94-94: Line too long (115 > 99)
(E501)
95-95: Line too long (122 > 99)
(E501)
96-96: Line too long (105 > 99)
(E501)
116-116: Local variable mock_project_bulk_save is assigned to but never used
Remove assignment to unused variable mock_project_bulk_save
(F841)
117-117: Local variable mock_metrics_bulk_save is assigned to but never used
Remove assignment to unused variable mock_metrics_bulk_save
(F841)
117-117: Line too long (122 > 99)
(E501)
119-119: Blank line contains whitespace
Remove whitespace from blank line
(W293)
121-121: Blank line contains whitespace
Remove whitespace from blank line
(W293)
123-123: Blank line contains whitespace
Remove whitespace from blank line
(W293)
127-127: Line too long (102 > 99)
(E501)
128-128: Blank line contains whitespace
Remove whitespace from blank line
(W293)
140-140: Blank line contains whitespace
Remove whitespace from blank line
(W293)
141-141: Line too long (124 > 99)
(E501)
142-142: Line too long (129 > 99)
(E501)
143-143: Local variable mock_scores_bulk_save is assigned to but never used
Remove assignment to unused variable mock_scores_bulk_save
(F841)
143-143: Line too long (121 > 99)
(E501)
145-145: Blank line contains whitespace
Remove whitespace from blank line
(W293)
148-148: Blank line contains whitespace
Remove whitespace from blank line
(W293)
150-150: Blank line contains whitespace
Remove whitespace from blank line
(W293)
153-153: Blank line contains whitespace
Remove whitespace from blank line
(W293)
157-157: Blank line contains whitespace
Remove whitespace from blank line
(W293)
160-160: Blank line contains whitespace
Remove whitespace from blank line
(W293)
163-163: Blank line contains whitespace
Remove whitespace from blank line
(W293)
166-166: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
169-169: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
192-192: Local variable mock_bulk_save is assigned to but never used
Remove assignment to unused variable mock_bulk_save
(F841)
195-195: Blank line contains whitespace
Remove whitespace from blank line
(W293)
197-197: Blank line contains whitespace
Remove whitespace from blank line
(W293)
199-199: Blank line contains whitespace
Remove whitespace from blank line
(W293)
205-205: Blank line contains whitespace
Remove whitespace from blank line
(W293)
212-212: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
223-223: Blank line contains whitespace
Remove whitespace from blank line
(W293)
225-225: Blank line contains whitespace
Remove whitespace from blank line
(W293)
227-227: Blank line contains whitespace
Remove whitespace from blank line
(W293)
232-232: Blank line contains whitespace
Remove whitespace from blank line
(W293)
243-243: Blank line contains whitespace
Remove whitespace from blank line
(W293)
245-245: Blank line contains whitespace
Remove whitespace from blank line
(W293)
247-247: Blank line contains whitespace
Remove whitespace from blank line
(W293)
252-252: Blank line contains whitespace
Remove whitespace from blank line
(W293)
268-268: Blank line contains whitespace
Remove whitespace from blank line
(W293)
273-273: Blank line contains whitespace
Remove whitespace from blank line
(W293)
286-286: Line too long (124 > 99)
(E501)
287-287: Line too long (129 > 99)
(E501)
290-290: Blank line contains whitespace
Remove whitespace from blank line
(W293)
293-293: Blank line contains whitespace
Remove whitespace from blank line
(W293)
295-295: Blank line contains whitespace
Remove whitespace from blank line
(W293)
297-297: Blank line contains whitespace
Remove whitespace from blank line
(W293)
301-301: Blank line contains whitespace
Remove whitespace from blank line
(W293)
314-314: Blank line contains whitespace
Remove whitespace from blank line
(W293)
316-316: Line too long (102 > 99)
(E501)
317-317: Blank line contains whitespace
Remove whitespace from blank line
(W293)
318-318: Line too long (124 > 99)
(E501)
319-319: Line too long (129 > 99)
(E501)
322-322: Blank line contains whitespace
Remove whitespace from blank line
(W293)
325-325: Blank line contains whitespace
Remove whitespace from blank line
(W293)
327-327: Blank line contains whitespace
Remove whitespace from blank line
(W293)
330-330: Blank line contains whitespace
Remove whitespace from blank line
(W293)
332-332: No newline at end of file
Add trailing newline
(W292)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py
69-69: Line too long (101 > 99)
(E501)
71-71: Line too long (109 > 99)
(E501)
🔇 Additional comments (8)
backend/apps/owasp/migrations/0049_remove_projecthealthrequirements_owasp_compliance_penalty_weight_0_100_and_more.py (1)
13-19: DB-level constraints and validators: nice hardeningAltering the field to FloatField with min/max validators and a clearer verbose_name is a good complement to the runtime clamp. LGTM.
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py (3)
34-59: Good happy-path coverage for “all compliant” scenarioAssertions validate headers, counts, rate, and success line. Looks good.
89-108: Verbose mode test is useful; assert header toggle if applicableIf the command prints an initial “Analyzing…” header, assert its presence here too to ensure stdout wiring remains intact.
109-125: No projects scenario handled; LGTMZero-counts and success message assertions look correct.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (1)
254-290: Max-penalty case is crisp; LGTMThe 100% clamping to 0.0 is asserted correctly and aligns with the command’s logic. No changes suggested.
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
60-67: Great, succinct summary blockThe summary math and formatting are clear. No changes suggested.
backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py (2)
212-235: API failure path: LGTMThe test ensures the command proceeds to metrics evaluation and preserves official level on failure. Looks good.
236-255: Skip flag path is correctEnsures the fetch is skipped and processing continues. Good coverage.
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py
Outdated
Show resolved
Hide resolved
…alth_scores.py Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
49-58: Compute score only after guarding requirements; remove early direct dict indexing to avoid KeyError and stale requirementsYou added a safe .get(...) guard (lines 60-69), but the code still does a direct index lookup at Line 49 and computes the score (Lines 51-58) before the guard runs. This reintroduces the original KeyError risk and means the guard is too late. Unify the retrieval and scoring under the guarded path.
Apply this refactor to merge retrieval + guard and then compute the score:
- requirements = project_health_requirements[metric.project.level] - - score = 0.0 - for field, weight in forward_fields.items(): - if int(getattr(metric, field)) >= int(getattr(requirements, field)): - score += weight - - for field, weight in backward_fields.items(): - if int(getattr(metric, field)) <= int(getattr(requirements, field)): - score += weight - - # Fetch requirements for this project level, skip if missing - requirements = project_health_requirements.get(metric.project.level) - if requirements is None: - self.stdout.write( - self.style.WARNING( - f"Missing ProjectHealthRequirements for level '{metric.project.level}' — " - f"skipping scoring for {metric.project.name}" - ) - ) - continue + # Fetch requirements for this project level, skip if missing + requirements = project_health_requirements.get(metric.project.level) + if requirements is None: + self.stdout.write( + self.style.WARNING( + f"Missing ProjectHealthRequirements for level '{metric.project.level}' — " + f"skipping scoring for {metric.project.name}" + ) + ) + continue + + # Calculate the score based on requirements. + score = 0.0 + for field, weight in forward_fields.items(): + if int(getattr(metric, field)) >= int(getattr(requirements, field)): + score += weight + + for field, weight in backward_fields.items(): + if int(getattr(metric, field)) <= int(getattr(requirements, field)): + score += weightAlso applies to: 60-69
🧹 Nitpick comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
79-85: Wrap long log lines to satisfy Ruff E501 and improve readabilityThe warning message exceeds the 99-char limit (Ruff E501). Build the message in a variable with implicit string concatenation or split across multiple f-strings.
- self.stdout.write( - self.style.WARNING( - f"Applied {penalty_percentage}% compliance penalty to {metric.project.name} " - f"(penalty: {penalty_amount:.2f}, final score: {score:.2f}) " - f"[Local: {metric.project.level}, Official: {metric.project.project_level_official}]" - ) - ) + msg = ( + f"Applied {penalty_percentage}% compliance penalty to {metric.project.name} " + f"(penalty: {penalty_amount:.2f}, final score: {score:.2f}) " + f"[Local: {metric.project.level}, " + f"Official: {metric.project.project_level_official}]" + ) + self.stdout.write(self.style.WARNING(msg))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
backend/apps/owasp/models/project.py (1)
is_level_compliant(162-164)
🪛 Ruff (0.12.2)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py
80-80: Line too long (101 > 99)
(E501)
82-82: Line too long (109 > 99)
(E501)
🔇 Additional comments (2)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (2)
73-77: Nice: penalty and final score clamped to valid boundsGood defensive coding: clamping the penalty to [0, 100] and the final score to [0, 100] prevents misconfigurations from corrupting scores.
Also applies to: 86-86
71-78: Ignore incorrect method call onis_level_compliant
Theis_level_compliantaccessor is defined as a@propertyonProject(backend/apps/owasp/models/project.py, lines 161–163), so using it without parentheses is correct. No change is needed to call syntax, and altering it to a method call would break existing behavior and tests.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (19)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (6)
3-3: Remove unused importjson.
jsonis never used in this module. Drop it to keep the test lean and satisfy Ruff F401.- import json
111-115: Avoid hard-coding the OWASP levels URL in assertions; import and assert against the command’s constant.This prevents fragile test failures if the constant changes upstream while behavior remains correct.
-from apps.owasp.management.commands.owasp_update_project_health_metrics import Command +from apps.owasp.management.commands.owasp_update_project_health_metrics import ( + Command, + OWASP_PROJECT_LEVELS_URL, +)- mock_get.assert_called_once_with( - "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/project_levels.json", - timeout=30, - headers={"Accept": "application/json"} - ) + mock_get.assert_called_once_with( + OWASP_PROJECT_LEVELS_URL, + timeout=30, + headers={"Accept": "application/json"}, + )Also applies to: 10-10
87-87: Unify quotes to double quotes to match codebase style.Minor style nits flagged by Ruff (Q000). Update the patch decorators to use double quotes consistently.
- @patch('requests.get') + @patch("requests.get")Also applies to: 117-117, 126-126, 138-138, 150-150, 239-239
191-207: Assert bulk_save field list for stronger behavior verification.In addition to checking the updated instances, verify
Project.bulk_saveis called withfields=["project_level_official"]so the test guards against accidental over-updates.mock_bulk_save.assert_called_once() saved_projects = mock_bulk_save.call_args[0][0] assert len(saved_projects) == 1 assert saved_projects[0] == project1 + # Also assert only the official level field is updated + assert mock_bulk_save.call_args.kwargs.get("fields") == ["project_level_official"]
209-238: Consider parametrizing and adding normalization edge cases.The mapping checks are solid. Add a case-insensitivity/whitespace normalization example to prevent regressions (e.g., " FLAGSHIP " -> "flagship"). Parametrizing with
pytest.mark.parametrizewill also simplify the loop.Would you like me to push a parametrized test covering
" FLAGSHIP ","Lab", and"3 "variants?
41-60: Optional: Use datetime objects for date-like fields to future-proof tests.Currently strings are fine because the test doesn’t compute deltas. If the command evolves to use date arithmetic, these will fail. Consider using timezone-aware datetimes.
I can provide a drop-in helper to generate aware datetimes if you want to future-proof this test.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (3)
14-16: Break long patch target strings to satisfy line-length limits and improve readability.Keeps within E501 and avoids horizontal scrolling.
-METRICS_FILTER_PATCH = "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.objects.filter" -REQUIREMENTS_ALL_PATCH = "apps.owasp.models.project_health_requirements.ProjectHealthRequirements.objects.all" +METRICS_FILTER_PATCH = ( + "apps.owasp.models.project_health_metrics." + "ProjectHealthMetrics.objects.filter" +) +REQUIREMENTS_ALL_PATCH = ( + "apps.owasp.models.project_health_requirements." + "ProjectHealthRequirements.objects.all" +)
146-151: Split compound assertion to aid failure diagnostics.Ruff PT018 suggests separating these for clearer failure messages.
- assert "penalty:" in output and "final score:" in output + assert "penalty:" in output + assert "final score:" in output
286-333: Great clamping tests; consider adding a compliant-project variant.You already test clamping for non-compliant projects. Adding a compliant case ensures clamping logic does not accidentally penalize compliant projects when weights are out-of-range.
If you’d like, I can add a short parametrized test covering compliant projects with penalties set to [-10, 150, 50].
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py (5)
23-28: Usereturninstead ofyieldin autouse fixture (no teardown logic).Simplifies the fixture and satisfies Ruff PT022.
@pytest.fixture(autouse=True) def _setup(self): """Set up test environment.""" self.stdout = StringIO() self.command = Command() - yield + return
44-47: Avoid boolean positional args; use keyword for clarity.Improves readability and conforms to Ruff FBT003 guidance.
- self.create_mock_project(OWASP_ZAP_NAME, "flagship", "flagship", True), - self.create_mock_project("OWASP Top 10", "flagship", "flagship", True), - self.create_mock_project(OWASP_WEBGOAT_NAME, "production", "production", True), + self.create_mock_project(OWASP_ZAP_NAME, "flagship", "flagship", is_compliant=True), + self.create_mock_project("OWASP Top 10", "flagship", "flagship", is_compliant=True), + self.create_mock_project(OWASP_WEBGOAT_NAME, "production", "production", is_compliant=True),
70-73: Apply the same keyword-arg pattern to other calls.Keeps call sites consistent and more readable.
- self.create_mock_project(OWASP_ZAP_NAME, "flagship", "flagship", True), - self.create_mock_project(OWASP_WEBGOAT_NAME, "lab", "production", False), - self.create_mock_project("OWASP Top 10", "production", "flagship", False), + self.create_mock_project(OWASP_ZAP_NAME, "flagship", "flagship", is_compliant=True), + self.create_mock_project(OWASP_WEBGOAT_NAME, "lab", "production", is_compliant=False), + self.create_mock_project("OWASP Top 10", "production", "flagship", is_compliant=False),- self.create_mock_project(OWASP_ZAP_NAME, "flagship", "flagship", True), - self.create_mock_project(OWASP_WEBGOAT_NAME, "lab", "production", False), + self.create_mock_project(OWASP_ZAP_NAME, "flagship", "flagship", is_compliant=True), + self.create_mock_project(OWASP_WEBGOAT_NAME, "lab", "production", is_compliant=False),Also applies to: 98-100
143-145: Remove unused nested filter/count mocking.The command computes “projects with default official levels” via in-memory iteration, not a DB count. This mock is unused noise.
- mock_filter.return_value.filter.return_value.count.return_value = 1
169-171: Use the module-level patch constants for consistency.Keep patch targets centralized to reduce drift and typos.
- with patch("apps.owasp.models.project.Project.objects.filter") as mock_filter, \ - patch("sys.stdout", new=StringIO()) as mock_stdout: + with patch(PROJECT_FILTER_PATCH) as mock_filter, \ + patch(STDOUT_PATCH, new=StringIO()) as mock_stdout:backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py (5)
5-5: Remove unused importjson.Not used anywhere in the test; drop it to satisfy Ruff F401.
-import json
28-33: Fixture does not needyield; return is sufficient.There’s no teardown. This also clears Ruff PT022.
@pytest.fixture(autouse=True) def _setup(self): """Set up test environment.""" self.stdout = StringIO() - yield + return
88-88: Use double quotes in patch decorators for consistency.Align with project style (Ruff Q000).
- @patch('requests.get') + @patch("requests.get")Also applies to: 177-177, 220-220
122-140: Assert partial-field bulk update for official level sync.You’re patching
Project.bulk_savebut not asserting thefieldsargument. Adding it makes the integration test catch accidental full saves.with patch(PROJECT_FILTER_PATCH) as mock_projects, \ patch(PROJECT_BULK_SAVE_PATCH), \ patch(METRICS_BULK_SAVE_PATCH), \ patch(STDOUT_PATCH, new=self.stdout): @@ call_command("owasp_update_project_health_metrics") @@ assert "Updated official levels for 2 projects" in output + + # New: verify only official levels were requested to be updated + # (ensures partial update, not full model write) + from apps.owasp.models.project import Project as _Project + _Project.bulk_save.assert_called() + _, kwargs = _Project.bulk_save.call_args + assert kwargs.get("fields") == ["project_level_official"]
176-219: Level-mapping integration looks good; consider asserting stdout for mapping counts.You verify field mappings on models. Adding a lightweight stdout assertion (e.g., “Successfully fetched 4 official project levels”) would increase confidence in the integration flow.
I can extend this test with an additional stdout assertion if you want stronger user-facing coverage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py(5 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py(4 hunks)backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py (3)
backend/apps/owasp/models/project.py (4)
Project(31-385)is_funding_requirements_compliant(150-152)is_leader_requirements_compliant(155-159)is_level_compliant(162-164)backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(16-235)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (3)
backend/apps/owasp/models/project.py (3)
is_funding_requirements_compliant(150-152)is_leader_requirements_compliant(155-159)is_level_compliant(162-164)backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(16-235)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py (2)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
Command(14-96)backend/apps/owasp/models/project.py (2)
Project(31-385)is_level_compliant(162-164)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (3)
Command(17-176)fetch_official_project_levels(34-79)update_official_levels(81-127)backend/apps/owasp/models/project.py (1)
Project(31-385)
🪛 Ruff (0.12.2)
backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py
5-5: json imported but unused
Remove unused import: json
(F401)
20-20: Line too long (101 > 99)
(E501)
21-21: Line too long (110 > 99)
(E501)
32-32: No teardown in fixture _setup, use return instead of yield
Replace yield with return
(PT022)
35-35: First line of docstring should be in imperative mood: "Helper to create a mock project with specified levels."
(D401)
41-41: Blank line contains whitespace
Remove whitespace from blank line
(W293)
43-43: Trailing whitespace
Remove trailing whitespace
(W291)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
55-55: First line of docstring should be in imperative mood: "Helper to create a mock health metric for a project."
(D401)
58-58: Blank line contains whitespace
Remove whitespace from blank line
(W293)
62-62: Line too long (102 > 99)
(E501)
66-66: Blank line contains whitespace
Remove whitespace from blank line
(W293)
69-69: Blank line contains whitespace
Remove whitespace from blank line
(W293)
73-73: First line of docstring should be in imperative mood: "Helper to create mock health requirements."
(D401)
77-77: Blank line contains whitespace
Remove whitespace from blank line
(W293)
81-81: Line too long (102 > 99)
(E501)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
88-88: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
90-90: Line too long (109 > 99)
(E501)
102-102: Line too long (115 > 99)
(E501)
103-103: Line too long (122 > 99)
(E501)
104-104: Line too long (105 > 99)
(E501)
127-127: Blank line contains whitespace
Remove whitespace from blank line
(W293)
129-129: Blank line contains whitespace
Remove whitespace from blank line
(W293)
131-131: Blank line contains whitespace
Remove whitespace from blank line
(W293)
135-135: Line too long (102 > 99)
(E501)
136-136: Blank line contains whitespace
Remove whitespace from blank line
(W293)
148-148: Blank line contains whitespace
Remove whitespace from blank line
(W293)
153-153: Blank line contains whitespace
Remove whitespace from blank line
(W293)
156-156: Blank line contains whitespace
Remove whitespace from blank line
(W293)
158-158: Blank line contains whitespace
Remove whitespace from blank line
(W293)
161-161: Blank line contains whitespace
Remove whitespace from blank line
(W293)
165-165: Blank line contains whitespace
Remove whitespace from blank line
(W293)
168-168: Blank line contains whitespace
Remove whitespace from blank line
(W293)
171-171: Blank line contains whitespace
Remove whitespace from blank line
(W293)
174-174: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
177-177: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
203-203: Blank line contains whitespace
Remove whitespace from blank line
(W293)
205-205: Blank line contains whitespace
Remove whitespace from blank line
(W293)
207-207: Blank line contains whitespace
Remove whitespace from blank line
(W293)
213-213: Blank line contains whitespace
Remove whitespace from blank line
(W293)
220-220: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
231-231: Blank line contains whitespace
Remove whitespace from blank line
(W293)
233-233: Blank line contains whitespace
Remove whitespace from blank line
(W293)
235-235: Blank line contains whitespace
Remove whitespace from blank line
(W293)
240-240: Blank line contains whitespace
Remove whitespace from blank line
(W293)
251-251: Blank line contains whitespace
Remove whitespace from blank line
(W293)
253-253: Blank line contains whitespace
Remove whitespace from blank line
(W293)
255-255: Blank line contains whitespace
Remove whitespace from blank line
(W293)
260-260: Blank line contains whitespace
Remove whitespace from blank line
(W293)
276-276: Blank line contains whitespace
Remove whitespace from blank line
(W293)
281-281: Blank line contains whitespace
Remove whitespace from blank line
(W293)
298-298: Blank line contains whitespace
Remove whitespace from blank line
(W293)
301-301: Blank line contains whitespace
Remove whitespace from blank line
(W293)
303-303: Blank line contains whitespace
Remove whitespace from blank line
(W293)
305-305: Blank line contains whitespace
Remove whitespace from blank line
(W293)
309-309: Blank line contains whitespace
Remove whitespace from blank line
(W293)
322-322: Blank line contains whitespace
Remove whitespace from blank line
(W293)
324-324: Line too long (102 > 99)
(E501)
325-325: Blank line contains whitespace
Remove whitespace from blank line
(W293)
330-330: Blank line contains whitespace
Remove whitespace from blank line
(W293)
333-333: Blank line contains whitespace
Remove whitespace from blank line
(W293)
335-335: Blank line contains whitespace
Remove whitespace from blank line
(W293)
338-338: Blank line contains whitespace
Remove whitespace from blank line
(W293)
340-340: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py
14-14: Line too long (101 > 99)
(E501)
15-15: Line too long (110 > 99)
(E501)
82-82: Line too long (101 > 99)
(E501)
109-109: Blank line contains whitespace
Remove whitespace from blank line
(W293)
113-113: Blank line contains whitespace
Remove whitespace from blank line
(W293)
119-119: Blank line contains whitespace
Remove whitespace from blank line
(W293)
123-123: Blank line contains whitespace
Remove whitespace from blank line
(W293)
126-126: Blank line contains whitespace
Remove whitespace from blank line
(W293)
130-130: Blank line contains whitespace
Remove whitespace from blank line
(W293)
139-139: Found commented-out code
Remove commented-out code
(ERA001)
141-141: Blank line contains whitespace
Remove whitespace from blank line
(W293)
145-145: Blank line contains whitespace
Remove whitespace from blank line
(W293)
149-149: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
175-175: Blank line contains whitespace
Remove whitespace from blank line
(W293)
179-179: Blank line contains whitespace
Remove whitespace from blank line
(W293)
185-185: Blank line contains whitespace
Remove whitespace from blank line
(W293)
188-188: Blank line contains whitespace
Remove whitespace from blank line
(W293)
191-191: Blank line contains whitespace
Remove whitespace from blank line
(W293)
195-195: Blank line contains whitespace
Remove whitespace from blank line
(W293)
202-202: Blank line contains whitespace
Remove whitespace from blank line
(W293)
211-211: Blank line contains whitespace
Remove whitespace from blank line
(W293)
215-215: Line too long (102 > 99)
(E501)
220-220: Blank line contains whitespace
Remove whitespace from blank line
(W293)
226-226: Blank line contains whitespace
Remove whitespace from blank line
(W293)
229-229: Blank line contains whitespace
Remove whitespace from blank line
(W293)
232-232: Blank line contains whitespace
Remove whitespace from blank line
(W293)
236-236: Blank line contains whitespace
Remove whitespace from blank line
(W293)
244-244: Blank line contains whitespace
Remove whitespace from blank line
(W293)
253-253: Blank line contains whitespace
Remove whitespace from blank line
(W293)
257-257: Line too long (102 > 99)
(E501)
262-262: Blank line contains whitespace
Remove whitespace from blank line
(W293)
268-268: Blank line contains whitespace
Remove whitespace from blank line
(W293)
271-271: Blank line contains whitespace
Remove whitespace from blank line
(W293)
274-274: Blank line contains whitespace
Remove whitespace from blank line
(W293)
278-278: Blank line contains whitespace
Remove whitespace from blank line
(W293)
290-290: Blank line contains whitespace
Remove whitespace from blank line
(W293)
294-294: Line too long (102 > 99)
(E501)
299-299: Blank line contains whitespace
Remove whitespace from blank line
(W293)
305-305: Blank line contains whitespace
Remove whitespace from blank line
(W293)
308-308: Blank line contains whitespace
Remove whitespace from blank line
(W293)
315-315: Blank line contains whitespace
Remove whitespace from blank line
(W293)
318-318: Blank line contains whitespace
Remove whitespace from blank line
(W293)
322-322: Blank line contains whitespace
Remove whitespace from blank line
(W293)
325-325: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py
28-28: No teardown in fixture _setup, use return instead of yield
Replace yield with return
(PT022)
31-31: First line of docstring should be in imperative mood: "Helper to create a mock project."
(D401)
44-44: Boolean positional value in function call
(FBT003)
45-45: Boolean positional value in function call
(FBT003)
46-46: Boolean positional value in function call
(FBT003)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
53-53: Blank line contains whitespace
Remove whitespace from blank line
(W293)
55-55: Blank line contains whitespace
Remove whitespace from blank line
(W293)
57-57: Blank line contains whitespace
Remove whitespace from blank line
(W293)
70-70: Boolean positional value in function call
(FBT003)
71-71: Boolean positional value in function call
(FBT003)
72-72: Boolean positional value in function call
(FBT003)
77-77: Blank line contains whitespace
Remove whitespace from blank line
(W293)
79-79: Blank line contains whitespace
Remove whitespace from blank line
(W293)
81-81: Blank line contains whitespace
Remove whitespace from blank line
(W293)
83-83: Blank line contains whitespace
Remove whitespace from blank line
(W293)
90-90: Blank line contains whitespace
Remove whitespace from blank line
(W293)
98-98: Boolean positional value in function call
(FBT003)
99-99: Boolean positional value in function call
(FBT003)
104-104: Blank line contains whitespace
Remove whitespace from blank line
(W293)
106-106: Blank line contains whitespace
Remove whitespace from blank line
(W293)
108-108: Blank line contains whitespace
Remove whitespace from blank line
(W293)
110-110: Blank line contains whitespace
Remove whitespace from blank line
(W293)
119-119: Blank line contains whitespace
Remove whitespace from blank line
(W293)
121-121: Blank line contains whitespace
Remove whitespace from blank line
(W293)
123-123: Blank line contains whitespace
Remove whitespace from blank line
(W293)
125-125: Blank line contains whitespace
Remove whitespace from blank line
(W293)
135-135: Boolean positional value in function call
(FBT003)
136-136: Boolean positional value in function call
(FBT003)
136-136: Line too long (105 > 99)
(E501)
141-141: Blank line contains whitespace
Remove whitespace from blank line
(W293)
145-145: Blank line contains whitespace
Remove whitespace from blank line
(W293)
147-147: Blank line contains whitespace
Remove whitespace from blank line
(W293)
149-149: Blank line contains whitespace
Remove whitespace from blank line
(W293)
151-151: String contains ambiguous ℹ (INFORMATION SOURCE). Did you mean i (LATIN SMALL LETTER I)?
(RUF001)
163-163: Line too long (105 > 99)
(E501)
165-165: Line too long (110 > 99)
(E501)
171-171: Blank line contains whitespace
Remove whitespace from blank line
(W293)
174-174: Blank line contains whitespace
Remove whitespace from blank line
(W293)
176-176: Blank line contains whitespace
Remove whitespace from blank line
(W293)
178-178: Blank line contains whitespace
Remove whitespace from blank line
(W293)
181-181: No newline at end of file
Add trailing newline
(W292)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py
3-3: json imported but unused
Remove unused import: json
(F401)
87-87: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
117-117: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
126-126: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
138-138: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
150-150: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
193-193: Blank line contains whitespace
Remove whitespace from blank line
(W293)
195-195: Blank line contains whitespace
Remove whitespace from blank line
(W293)
197-197: Blank line contains whitespace
Remove whitespace from blank line
(W293)
202-202: Blank line contains whitespace
Remove whitespace from blank line
(W293)
230-230: Blank line contains whitespace
Remove whitespace from blank line
(W293)
233-233: Blank line contains whitespace
Remove whitespace from blank line
(W293)
236-236: Blank line contains whitespace
Remove whitespace from blank line
(W293)
239-239: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
268-268: Blank line contains whitespace
Remove whitespace from blank line
(W293)
270-270: Blank line contains whitespace
Remove whitespace from blank line
(W293)
272-272: Blank line contains whitespace
Remove whitespace from blank line
(W293)
277-277: Blank line contains whitespace
Remove whitespace from blank line
(W293)
298-298: Blank line contains whitespace
Remove whitespace from blank line
(W293)
300-300: Blank line contains whitespace
Remove whitespace from blank line
(W293)
302-302: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🔇 Additional comments (4)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (1)
87-172: Nice coverage of success, HTTP error, invalid JSON, invalid format, and entry filtering.These tests exercise the critical branches in
fetch_official_project_levels()and will catch most regressions early.backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (1)
86-174: Penalty flow tests are thorough and align with the PR goals.Good coverage for penalty application, compliant vs. non-compliant paths, and log outputs. Clamping and edge cases below further strengthen this test suite.
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py (1)
40-65: Solid end-to-end assertions for the “all compliant” path.The summary and success message checks align with the command’s user-facing behavior.
backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py (1)
316-340: Excellent clamp-to-100% penalty test.Clear, focused, and it validates both the final score and the log message.
…hub.com/divyanshu-vr/Nest into feature/ProjectLevelComplianceDetection
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (1)
26-37: Fixture pattern with yield + patch managers is correct; ignore PT022 here.Yield is necessary to keep the patch contexts alive during each test; switching to
returnwould undo patches before tests execute.
🧹 Nitpick comments (16)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (8)
3-3: Remove unused import 'json'.
jsonisn't used anywhere in this module. Clean it up to appease Ruff F401.-import json
74-76: Prefer passing stdout to call_command over patching sys.stdout.Using
call_command(..., stdout=self.stdout)avoids global stdout patching and reduces coupling to print behavior.- with patch(STDOUT_PATCH, new=self.stdout): - call_command("owasp_update_project_health_metrics") + call_command("owasp_update_project_health_metrics", stdout=self.stdout)
111-115: Avoid hard-coding the levels JSON URL in tests; assert against the module constant.Reduces brittleness if the source URL changes in code.
- mock_get.assert_called_once_with( - "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/project_levels.json", - timeout=30, - headers={"Accept": "application/json"} - ) + mock_get.assert_called_once_with( + OWASP_PROJECT_LEVELS_URL, + timeout=30, + headers={"Accept": "application/json"}, + )Add this import near the top of the file:
from apps.owasp.management.commands.owasp_update_project_health_metrics import OWASP_PROJECT_LEVELS_URL
150-172: Solid validation for entry filtering.Covers empty name, missing fields, and numeric level coercion. Consider adding a case with surrounding whitespace and mixed case to assert normalization, e.g.,
" Incubator "→"incubator".
173-208: Enhance assertions: verify queryset filter params and bulk_save fields.Assert we filter by
is_active=Trueand thatbulk_saveis called withfields=["project_level_official"]to lock in contract.with patch(PROJECT_FILTER_PATCH) as mock_filter, \ patch(PROJECT_BULK_SAVE_PATCH) as mock_bulk_save: @@ updated_count = self.command.update_official_levels(official_levels) @@ mock_bulk_save.assert_called_once() saved_projects = mock_bulk_save.call_args[0][0] assert len(saved_projects) == 1 assert saved_projects[0] == project1 + # Verify ORM filter and fields written + mock_filter.assert_called_once_with(is_active=True) + assert mock_bulk_save.call_args.kwargs.get("fields") == ["project_level_official"]
209-244: Parametrize level-mapping cases to simplify loop and isolate failures.Using
pytest.mark.parametrizeimproves readability and error reporting when one case fails.Example refactor (outside current hunk):
@pytest.mark.parametrize( "official_level,expected_mapped", [ ("2", "incubator"), ("3", "lab"), ("3.5", "production"), ("4", "flagship"), ("incubator", "incubator"), ("lab", "lab"), ("production", "production"), ("flagship", "flagship"), ("unknown", "other"), ], ) def test_update_official_levels_level_mapping(self, official_level, expected_mapped): ...
245-286: Integration test reads well; add minimal call verifications.Optionally assert that both
Project.bulk_save(for official levels) andProjectHealthMetrics.bulk_savewere invoked to tighten the integration contract.
1-313: Style nits from Ruff (quotes and trailing whitespace).
- Prefer double quotes consistently (Q000) where your project enforces it.
- Remove trailing whitespace on blank lines (W293).
These are cosmetic; consider running your formatter/ruff --fix.backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py (8)
5-5: Remove unused import 'json'.
jsonisn't referenced in this module.-import json
28-33: Use return instead of yield in this fixture.No teardown or active patch contexts here;
returnis simpler and avoids PT022.- def _setup(self): + def _setup(self): """Set up test environment.""" self.stdout = StringIO() - yield + return
101-140: End-to-end happy path is strong; consider asserting bulk_save fields.After calling
owasp_update_project_health_metrics, assert thatProject.bulk_savereceivedfields=["project_level_official"]for stricter behavioral guarantees.
149-156: Assert ORM usage in scoring step.Optionally verify
select_related("project")and thatProjectHealthMetrics.bulk_savewrites thescorefield to catch accidental regressions.mock_metrics_filter.return_value.select_related.return_value = metrics mock_requirements.return_value = requirements call_command("owasp_update_project_health_scores") + + # Optional: verify ORM/fields contract + mock_metrics_filter.return_value.select_related.assert_called() + # If your code specifies the related field explicitly: + # mock_metrics_filter.return_value.select_related.assert_called_with("project")
172-176: Split combined assertion for better failure diagnostics (PT018).Two separate asserts make it clear which token was missing.
- assert "penalty:" in output and "final score:" in output + assert "penalty:" in output + assert "final score:" in output
1-340: Style consistency: quotes and trailing whitespace.
- Convert single to double quotes where project conventions require (Q000).
- Remove trailing whitespace and add a final newline (W293, W292).
These are non-blocking polish items; a formatter run will handle them.
88-100: Optional: also assert request headers/timeout for fetch.To fully cover the HTTP call contract, assert the
headers={"Accept": "application/json"}andtimeouthere as well, mirroring the unit test.
120-121: Consider adding a case-insensitive/whitespace-trimming test for project names.E.g., official payload uses
" owasp zap "and the code still matches"OWASP ZAP".Example (outside current hunk):
mock_response.json.return_value = [{"name": " owasp zap ", "level": "flagship"}]
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py(5 hunks)backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (3)
Command(17-176)fetch_official_project_levels(34-79)update_official_levels(81-127)backend/apps/owasp/models/project.py (1)
Project(31-385)
backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py (6)
backend/apps/owasp/models/project.py (4)
Project(31-385)is_funding_requirements_compliant(150-152)is_leader_requirements_compliant(155-159)is_level_compliant(162-164)backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(16-235)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (1)
_setup(27-37)backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py (2)
_setup(24-28)create_mock_project(30-38)backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (1)
_setup(22-34)
🪛 Ruff (0.12.2)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py
3-3: json imported but unused
Remove unused import: json
(F401)
87-87: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
117-117: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
126-126: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
138-138: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
150-150: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
193-193: Blank line contains whitespace
Remove whitespace from blank line
(W293)
195-195: Blank line contains whitespace
Remove whitespace from blank line
(W293)
197-197: Blank line contains whitespace
Remove whitespace from blank line
(W293)
202-202: Blank line contains whitespace
Remove whitespace from blank line
(W293)
231-231: Blank line contains whitespace
Remove whitespace from blank line
(W293)
234-234: Blank line contains whitespace
Remove whitespace from blank line
(W293)
237-237: Blank line contains whitespace
Remove whitespace from blank line
(W293)
245-245: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
274-274: Blank line contains whitespace
Remove whitespace from blank line
(W293)
276-276: Blank line contains whitespace
Remove whitespace from blank line
(W293)
278-278: Blank line contains whitespace
Remove whitespace from blank line
(W293)
283-283: Blank line contains whitespace
Remove whitespace from blank line
(W293)
304-304: Blank line contains whitespace
Remove whitespace from blank line
(W293)
306-306: Blank line contains whitespace
Remove whitespace from blank line
(W293)
308-308: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py
5-5: json imported but unused
Remove unused import: json
(F401)
20-20: Line too long (101 > 99)
(E501)
21-21: Line too long (110 > 99)
(E501)
32-32: No teardown in fixture _setup, use return instead of yield
Replace yield with return
(PT022)
35-35: First line of docstring should be in imperative mood: "Helper to create a mock project with specified levels."
(D401)
41-41: Blank line contains whitespace
Remove whitespace from blank line
(W293)
43-43: Trailing whitespace
Remove trailing whitespace
(W291)
51-51: Blank line contains whitespace
Remove whitespace from blank line
(W293)
55-55: First line of docstring should be in imperative mood: "Helper to create a mock health metric for a project."
(D401)
58-58: Blank line contains whitespace
Remove whitespace from blank line
(W293)
62-62: Line too long (102 > 99)
(E501)
66-66: Blank line contains whitespace
Remove whitespace from blank line
(W293)
69-69: Blank line contains whitespace
Remove whitespace from blank line
(W293)
73-73: First line of docstring should be in imperative mood: "Helper to create mock health requirements."
(D401)
77-77: Blank line contains whitespace
Remove whitespace from blank line
(W293)
81-81: Line too long (102 > 99)
(E501)
85-85: Blank line contains whitespace
Remove whitespace from blank line
(W293)
88-88: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
90-90: Line too long (109 > 99)
(E501)
102-102: Line too long (115 > 99)
(E501)
103-103: Line too long (122 > 99)
(E501)
104-104: Line too long (105 > 99)
(E501)
127-127: Blank line contains whitespace
Remove whitespace from blank line
(W293)
129-129: Blank line contains whitespace
Remove whitespace from blank line
(W293)
131-131: Blank line contains whitespace
Remove whitespace from blank line
(W293)
135-135: Line too long (102 > 99)
(E501)
136-136: Blank line contains whitespace
Remove whitespace from blank line
(W293)
148-148: Blank line contains whitespace
Remove whitespace from blank line
(W293)
153-153: Blank line contains whitespace
Remove whitespace from blank line
(W293)
156-156: Blank line contains whitespace
Remove whitespace from blank line
(W293)
158-158: Blank line contains whitespace
Remove whitespace from blank line
(W293)
161-161: Blank line contains whitespace
Remove whitespace from blank line
(W293)
165-165: Blank line contains whitespace
Remove whitespace from blank line
(W293)
168-168: Blank line contains whitespace
Remove whitespace from blank line
(W293)
171-171: Blank line contains whitespace
Remove whitespace from blank line
(W293)
174-174: Assertion should be broken down into multiple parts
Break down assertion into multiple parts
(PT018)
177-177: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
203-203: Blank line contains whitespace
Remove whitespace from blank line
(W293)
205-205: Blank line contains whitespace
Remove whitespace from blank line
(W293)
207-207: Blank line contains whitespace
Remove whitespace from blank line
(W293)
213-213: Blank line contains whitespace
Remove whitespace from blank line
(W293)
220-220: Single quotes found but double quotes preferred
Replace single quotes with double quotes
(Q000)
231-231: Blank line contains whitespace
Remove whitespace from blank line
(W293)
233-233: Blank line contains whitespace
Remove whitespace from blank line
(W293)
235-235: Blank line contains whitespace
Remove whitespace from blank line
(W293)
240-240: Blank line contains whitespace
Remove whitespace from blank line
(W293)
251-251: Blank line contains whitespace
Remove whitespace from blank line
(W293)
253-253: Blank line contains whitespace
Remove whitespace from blank line
(W293)
255-255: Blank line contains whitespace
Remove whitespace from blank line
(W293)
260-260: Blank line contains whitespace
Remove whitespace from blank line
(W293)
276-276: Blank line contains whitespace
Remove whitespace from blank line
(W293)
281-281: Blank line contains whitespace
Remove whitespace from blank line
(W293)
298-298: Blank line contains whitespace
Remove whitespace from blank line
(W293)
301-301: Blank line contains whitespace
Remove whitespace from blank line
(W293)
303-303: Blank line contains whitespace
Remove whitespace from blank line
(W293)
305-305: Blank line contains whitespace
Remove whitespace from blank line
(W293)
309-309: Blank line contains whitespace
Remove whitespace from blank line
(W293)
322-322: Blank line contains whitespace
Remove whitespace from blank line
(W293)
324-324: Line too long (102 > 99)
(E501)
325-325: Blank line contains whitespace
Remove whitespace from blank line
(W293)
330-330: Blank line contains whitespace
Remove whitespace from blank line
(W293)
333-333: Blank line contains whitespace
Remove whitespace from blank line
(W293)
335-335: Blank line contains whitespace
Remove whitespace from blank line
(W293)
338-338: Blank line contains whitespace
Remove whitespace from blank line
(W293)
340-340: No newline at end of file
Add trailing newline
(W292)
🔇 Additional comments (8)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (4)
84-86: Assertion on human-readable output looks good.Message-based verification is helpful here and aligns with the command’s UX expectations.
117-125: Good coverage for HTTP error path.Using
requests.exceptions.RequestExceptionside-effect mirrors real network failures well.
126-149: Good handling of invalid JSON and invalid payload types.Covers two distinct failure modes: parser error and schema mismatch.
287-313: Skip flag behavior is verified correctly.Nice coverage ensuring the workflow proceeds without fetching official levels.
backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py (4)
34-53: Helper is clear and pragmatic.Good defaults and field hydration reduce duplication across tests.
177-219: Level-mapping integration is comprehensive.Nice coverage across numeric, decimal, string, and unknown values with compliance checks.
221-243: API failure path is well-covered.Graceful degradation and continued metrics evaluation are correctly asserted.
316-340: Edge-case penalty clamping test is valuable.Confirms clamping to 100% and resulting zero score; solid guard against extreme configuration.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
54-63: KeyError still possible: unsafe dict index remains before the safe .get guardLine 54 indexes the dict and will raise before your None-check at lines 66–75 can run. Move retrieval/guard before scoring; then compute.
Apply:
- requirements = project_health_requirements[metric.project.level] - - score = 0.0 - for field, weight in forward_fields.items(): - if int(getattr(metric, field)) >= int(getattr(requirements, field)): - score += weight - - for field, weight in backward_fields.items(): - if int(getattr(metric, field)) <= int(getattr(requirements, field)): - score += weight - - # Fetch requirements for this project level, skip if missing - requirements = project_health_requirements.get(metric.project.level) + # Fetch requirements for this project level, skip if missing + requirements = project_health_requirements.get(metric.project.level) if requirements is None: self.stdout.write( self.style.WARNING( f"Missing ProjectHealthRequirements for level '{metric.project.level}' — " f"skipping scoring for {metric.project.name}" ) ) continue - - total_projects_scored += 1 + total_projects_scored += 1 + + score = 0.0 + for field, weight in forward_fields.items(): + if float(getattr(metric, field)) >= float(getattr(requirements, field)): + score += weight + + for field, weight in backward_fields.items(): + if float(getattr(metric, field)) <= float(getattr(requirements, field)): + score += weightAlso applies to: 65-75
🧹 Nitpick comments (23)
backend/apps/owasp/Makefile (1)
36-39: Optional: allow a configurable HTTP timeout for the syncForward a timeout to the command for flaky networks (keeps default 30s otherwise).
If you want, I can draft a small guard in Make to pass
--timeout $(timeout)only when provided.backend/Makefile (1)
109-115: Ensure requirements are seeded before metrics/scoringIf ProjectHealthRequirements are populated by a dedicated target, add it before metrics.
Proposed minimal change:
sync-data: \ update-data \ + owasp-update-project-health-requirements \ owasp-update-project-health-metrics \ owasp-update-project-health-scores \ enrich-data \ index-databackend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
57-63: Avoid lossy int() casts for metric/requirement comparisonsSome fields may be non-integers (days, ratios). Use float to preserve precision.
Included in the diff above.
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (4)
41-42: select_related() with no fields is a no-opDrop it or specify related fields to prefetch.
- active_projects = Project.objects.filter(is_active=True).select_related() + active_projects = Project.objects.filter(is_active=True)
95-112: Move import to module scopeMinor style/perf: importing enum inside handle is unnecessary.
- from apps.owasp.models.enums.project import ProjectLevel +from apps.owasp.models.enums.project import ProjectLevel
60-73: Optional: use count() and precomputed totalsYou already built compliant/non-compliant lists; consider using their lengths directly and avoid re-counting the queryset.
24-33: Tests pending for this reporting commandPer issue #2039, add tests for summary output and per-project lines (verbose/non-verbose).
I can scaffold pytest tests using call_command and factory projects; want me to open a test file template?
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (3)
60-65: Use logger.error for data-shape validation (not logger.exception)No exception is being handled here; logger.exception implies a traceback.
- logger.exception( + logger.error( "Invalid project levels data format", extra={"expected": "list", "got": type(data).__name__}, )
102-123: Prefer enum values for level mapping to avoid string driftUse ProjectLevel for target values and keep normalization in one place.
+from apps.owasp.models.enums.project import ProjectLevel @@ - level_mapping = { - "incubator": "incubator", - "lab": "lab", - "production": "production", - "flagship": "flagship", - "2": "incubator", - "3": "lab", - "3.5": "production", - "4": "flagship", - } - mapped_level = level_mapping.get(official_level, "other") + level_mapping = { + "incubator": ProjectLevel.INCUBATOR, + "lab": ProjectLevel.LAB, + "production": ProjectLevel.PRODUCTION, + "flagship": ProjectLevel.FLAGSHIP, + "2": ProjectLevel.INCUBATOR, + "3": ProjectLevel.LAB, + "3.5": ProjectLevel.PRODUCTION, + "4": ProjectLevel.FLAGSHIP, + } + mapped_level = (level_mapping.get(official_level) or ProjectLevel.OTHER)
142-155: Graceful fallback on empty fetch is fine; consider surfacing non-zero exit in CI modeCurrent behavior continues without updates. If this will run in CI, consider optional
--strict-fetchto raise on failure.backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (7)
60-78: Set explicit boolean for leader compliance to avoid MagicMock leakage.
is_leader_requirements_compliantis derived from a property on the real model; with a spec’d MagicMock it becomes a MagicMock unless you set it explicitly. Add it totest_datato keep types boolean.test_data = { "name": TEST_PROJECT_NAME, "contributors_count": 10, "created_at": "2023-01-01", "forks_count": 2, "is_funding_requirements_compliant": True, + "is_leader_requirements_compliant": True, "released_at": "2023-02-01", "pushed_at": "2023-03-01",Also applies to: 88-90, 102-104
30-33: Use the command’s URL constant to reduce brittleness.Import and assert against
OWASP_PROJECT_LEVELS_URLfrom the command module instead of a hard-coded duplicate.-OWASP_LEVELS_URL = ( - "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_data/project_levels.json" -) +from apps.owasp.management.commands.owasp_update_project_health_metrics import ( + OWASP_PROJECT_LEVELS_URL, +)- mock_get.assert_called_once_with( - OWASP_LEVELS_URL, timeout=TIMEOUT_30_SECONDS, headers={"Accept": "application/json"} - ) + mock_get.assert_called_once_with( + OWASP_PROJECT_LEVELS_URL, + timeout=TIMEOUT_30_SECONDS, + headers={"Accept": "application/json"}, + )Also applies to: 129-131
221-225: Strengthen assertion: validate fields kwarg in bulk_save.Also assert the
fieldsargument to guarantee partial updates.- mock_bulk_save.assert_called_once() - saved_projects = mock_bulk_save.call_args[0][0] + mock_bulk_save.assert_called_once() + args, kwargs = mock_bulk_save.call_args + saved_projects = args[0] + assert kwargs.get("fields") == ["project_level_official"] assert len(saved_projects) == 1 assert saved_projects[0] == project1
246-262: Cover name/level normalization (case/whitespace) in mapping.Add cases like
" LAB ","FLAGSHIP", and name casing differences; current tests don’t exercise normalization paths.Would you like me to add a focused test (e.g.,
test_update_official_levels_name_and_value_normalization) that proves lowercasing, trimming, and name matching work end-to-end?
333-355: Also assert “no updates” path messaging.When no projects need updates, the command writes “No official level updates needed”. A small test would guard this.
Here’s a ready-to-add test:
def test_update_official_levels_no_changes(self): project = MagicMock(spec=Project) project._state = ModelState() project.name = OWASP_ZAP_NAME project.project_level_official = FLAGSHIP_LEVEL official_levels = {OWASP_ZAP_NAME: FLAGSHIP_LEVEL} with patch(PROJECT_FILTER_PATCH) as mock_filter, patch(PROJECT_BULK_SAVE_PATCH) as mock_bulk_save, patch(STDOUT_PATCH, new=self.stdout): mock_filter.return_value = [project] updated = self.command.update_official_levels(official_levels) assert updated == 0 mock_bulk_save.assert_not_called() assert "No official level updates needed" in self.stdout.getvalue()
92-93: Prefer passing stdout to call_command over patching sys.stdout.This isolates command output without globally patching
sys.stdout.-with patch(STDOUT_PATCH, new=self.stdout): - call_command("owasp_update_project_health_metrics") +call_command("owasp_update_project_health_metrics", stdout=self.stdout)Apply similarly to other invocations in this file.
Also applies to: 305-309, 344-351, 390-396
60-78: Use datetime objects for datetime fields.The test currently uses strings for
created_at,released_at, etc. Using aware datetimes makes failures more meaningful if serialization or arithmetic is added later.If helpful, I can patch the test to use
django.utils.timezone.now()values.backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (3)
24-33: Remove unused constant FULL_SCORE_THRESHOLD.It’s defined but never used.
-FULL_SCORE_THRESHOLD = 90.0
103-184: Reduce duplication by using a builder for “meets requirements” metrics.Multiple tests manually set 17+ fields. Introduce a small helper to cut noise and ease changes.
def make_metric_and_requirements(meet_all=True): m = MagicMock(spec=ProjectHealthMetrics) r = MagicMock(spec=ProjectHealthRequirements) # forward fields for f in ["age_days","contributors_count","forks_count","open_pull_requests_count", "recent_releases_count","stars_count","total_pull_requests_count","total_releases_count"]: setattr(m, f, 10 if meet_all else 5) setattr(r, f, 5) # booleans m.is_funding_requirements_compliant = True r.is_funding_requirements_compliant = True m.is_leader_requirements_compliant = True r.is_leader_requirements_compliant = True # backward fields for f in ["last_commit_days","last_pull_request_days","last_release_days", "open_issues_count","owasp_page_last_update_days","unanswered_issues_count","unassigned_issues_count"]: setattr(m, f, 1 if meet_all else 10) setattr(r, f, 5) return m, rUse this in penalty scenarios to keep tests terse.
388-392: Add score bounds assertion.Clamp logic is exercised; assert final score within [0, 100] to catch regressions.
assert ( abs(mock_metric.score - PENALTY_ZERO_PERCENT) < FLOAT_PRECISION ) # Use approximate comparison for float +assert 0.0 - FLOAT_PRECISION <= mock_metric.score <= 100.0 + FLOAT_PRECISIONbackend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py (3)
204-208: Remove dead patching of filter().count().
handle()computes default-level count via a generator; this extra patch isn’t used.- # Mock the filter for projects without official levels - mock_filter.return_value.select_related.return_value = projects - mock_filter.return_value.filter.return_value.count.return_value = 1 + mock_filter.return_value.select_related.return_value = projects
245-259: Use the STDOUT_PATCH constant consistently.For consistency with the rest of the file, prefer
STDOUT_PATCHover a raw"sys.stdout"string here.- with ( - patch("apps.owasp.models.project.Project.objects.filter") as mock_filter, - patch("sys.stdout", new=StringIO()) as mock_stdout, - ): + with ( + patch("apps.owasp.models.project.Project.objects.filter") as mock_filter, + patch(STDOUT_PATCH, new=StringIO()) as mock_stdout, + ):
44-84: Optional: assert logger summary for observability.Patch the module logger and assert the structured
extrapayload, ensuring ops signals don’t regress.Example:
@patch("apps.owasp.management.commands.owasp_detect_project_level_compliance.logger") def test_logger_summary(self, mock_logger): projects = [] with patch(PROJECT_FILTER_PATCH) as mock_filter, patch(STDOUT_PATCH, new=self.stdout): mock_filter.return_value.select_related.return_value = projects call_command("owasp_detect_project_level_compliance") mock_logger.info.assert_called() _, kwargs = mock_logger.info.call_args assert kwargs["extra"]["total_projects"] == 0 assert kwargs["extra"]["compliance_rate"] == "0.0%"Also applies to: 85-136, 137-170
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
backend/Makefile(2 hunks)backend/apps/owasp/Makefile(1 hunks)backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_scores.py(3 hunks)backend/apps/owasp/migrations/0047_add_is_level_compliant_field.py(1 hunks)backend/apps/owasp/migrations/0049_remove_projecthealthrequirements_owasp_compliance_penalty_weight_0_100_and_more.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py(5 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py(4 hunks)backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/apps/owasp/migrations/0047_add_is_level_compliant_field.py
- backend/apps/owasp/migrations/0049_remove_projecthealthrequirements_owasp_compliance_penalty_weight_0_100_and_more.py
- backend/tests/apps/owasp/management/commands/project_level_compliance_integration_test.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-22T06:36:42.593Z
Learnt from: ahmedxgouda
PR: OWASP/Nest#2089
File: backend/apps/nest/models/google_account_authorization.py:61-62
Timestamp: 2025-08-22T06:36:42.593Z
Learning: In the OWASP/Nest project, ruff linting rules discourage using logger.error() immediately before raising exceptions as it creates redundant logging. The preferred approach is to remove the logging call and let the caller handle logging the exception appropriately.
Applied to files:
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py
🧬 Code graph analysis (6)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (3)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (3)
Command(19-194)fetch_official_project_levels(41-87)update_official_levels(89-135)backend/apps/owasp/models/project.py (1)
Project(31-385)backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(16-235)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
backend/apps/owasp/models/project.py (2)
Project(31-385)bulk_save(352-360)backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(16-235)bulk_save(149-157)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
backend/apps/owasp/models/project.py (1)
is_level_compliant(162-164)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (3)
backend/apps/owasp/models/project.py (7)
is_funding_requirements_compliant(150-152)is_leader_requirements_compliant(155-159)open_pull_requests_count(217-219)recent_releases_count(268-273)unanswered_issues_count(281-283)unassigned_issues_count(286-288)is_level_compliant(162-164)backend/apps/owasp/models/project_health_metrics.py (6)
ProjectHealthMetrics(16-235)age_days(86-88)last_commit_days(96-98)last_pull_request_days(106-112)last_release_days(120-122)owasp_page_last_update_days(130-136)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)
backend/tests/apps/owasp/management/commands/owasp_detect_project_level_compliance_test.py (2)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
Command(12-112)backend/apps/owasp/models/project.py (2)
Project(31-385)is_level_compliant(162-164)
backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (2)
backend/apps/owasp/models/project.py (2)
Project(31-385)is_level_compliant(162-164)backend/apps/owasp/models/enums/project.py (1)
ProjectLevel(37-44)
🔇 Additional comments (6)
backend/apps/owasp/Makefile (1)
36-39: New target wiring looks goodTarget runs the intended management command via the existing exec-backend-command pattern. No issues.
backend/Makefile (2)
109-115: Good ordering: scoring now part of sync-dataIncluding “update-project-health-metrics” and “update-project-health-scores” before enrichment/indexing is sensible.
129-143: Official-level sync is correctly placed in update-dataRuns after aggregation, before events. This aligns with the two-step flow.
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (1)
79-81: Confirmed is_level_compliant is a @Property
Theis_level_compliantaccessor is decorated with@propertyat line 161 inbackend/apps/owasp/models/project.py, so usingmetric.project.is_level_compliantas an attribute is correct.backend/apps/owasp/management/commands/owasp_detect_project_level_compliance.py (1)
46-58: No changes needed:is_level_compliantis already a @Property — the attribute is correctly defined and used.backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (1)
91-101: Guard against future behavior change in empty result handling.Some implementations skip bulk updates for empty lists. Consider asserting “not called” instead of forcing a call with
[].If you adopt skip-on-empty (typical), adjust the assertion:
- self.mock_bulk_save.assert_called_once_with([], fields=["score"]) + self.mock_bulk_save.assert_not_called()Confirm which behavior you want across commands for consistency.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (3)
126-128: Gate per-project “Updating score…” logs behind verbosityReduce noisy output for large runs; show at verbosity >= 2.
- self.stdout.write( - self.style.NOTICE(f"Updating score for project: {metric.project.name}") - ) + if options.get("verbosity", 1) >= 2: + self.stdout.write( + self.style.NOTICE(f"Updating score for project: {metric.project.name}") + )
93-107: Optional: always show compliance summary (even when 0 penalties) under higher verbosityHelps operators confirm the check ran. Keep silent by default.
- if penalties_applied > 0: + if penalties_applied > 0 or self._options.get("verbosity", 1) >= 2:And store options once at the beginning of
handle:- def handle(self, *args, **options): - forward_fields, backward_fields = self._get_field_weights() + def handle(self, *args, **options): + self._options = options + forward_fields, backward_fields = self._get_field_weights()
12-36: Minor: extract weights to constants for reuse and to assert total=100Keeps scoring stable and self-documented.
+TOTAL_SCORE = 100.0 +FORWARD_WEIGHTS = { + "age_days": 6.0, + "contributors_count": 6.0, + "forks_count": 6.0, + "is_funding_requirements_compliant": 5.0, + "is_leader_requirements_compliant": 5.0, + "open_pull_requests_count": 6.0, + "recent_releases_count": 6.0, + "stars_count": 6.0, + "total_pull_requests_count": 6.0, + "total_releases_count": 6.0, +} +BACKWARD_WEIGHTS = { + "last_commit_days": 6.0, + "last_pull_request_days": 6.0, + "last_release_days": 6.0, + "open_issues_count": 6.0, + "owasp_page_last_update_days": 6.0, + "unanswered_issues_count": 6.0, + "unassigned_issues_count": 6.0, +} def _get_field_weights(self): """Return the field weights for scoring calculations.""" - forward_fields = { ... } - backward_fields = { ... } - return forward_fields, backward_fields + return FORWARD_WEIGHTS, BACKWARD_WEIGHTSbackend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (2)
31-32: Remove unused FULL_SCORE_THRESHOLD constantDead constant; drop to avoid confusion.
-FULL_SCORE_THRESHOLD = 90.0
175-184: Optional: assert compliance summary is printed when a penalty is appliedStrengthens behavioral coverage for the summary output.
output = self.stdout.getvalue() assert ( f"Applied {PENALTY_TWENTY_PERCENT}% compliance penalty to {NON_COMPLIANT_PROJECT_NAME}" in output ) assert "penalty:" in output assert "final score:" in output assert f"[Local: {LAB_LEVEL}, Official: {FLAGSHIP_LEVEL}]" in output + assert "Compliance Summary:" in output
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py(4 hunks)backend/apps/owasp/migrations/0049_remove_projecthealthrequirements_owasp_compliance_penalty_weight_0_100_and_more.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/migrations/0049_remove_projecthealthrequirements_owasp_compliance_penalty_weight_0_100_and_more.py
🧰 Additional context used
🧬 Code graph analysis (2)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (2)
backend/apps/owasp/models/project.py (1)
is_level_compliant(162-164)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (3)
backend/apps/owasp/models/project.py (7)
is_funding_requirements_compliant(150-152)is_leader_requirements_compliant(155-159)open_pull_requests_count(217-219)recent_releases_count(268-273)unanswered_issues_count(281-283)unassigned_issues_count(286-288)is_level_compliant(162-164)backend/apps/owasp/models/project_health_metrics.py (6)
ProjectHealthMetrics(16-235)age_days(86-88)last_commit_days(96-98)last_pull_request_days(106-112)last_release_days(120-122)owasp_page_last_update_days(130-136)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)
🔇 Additional comments (3)
backend/apps/owasp/management/commands/owasp_update_project_health_scores.py (3)
56-60: Good: penalty percentage is clamped to [0, 100]Defensive clamp prevents negative or >100 penalties. Nice.
130-139: Good: guard for missing requirements avoids KeyError and skips safelyGraceful skip with a clear warning keeps the command resilient.
55-71: No change required:is_level_compliantis a @Property returning bool
Theis_level_compliantattribute is defined with@propertyinbackend/apps/owasp/models/project.py(lines 161–164), sometric.project.is_level_compliantalready yields a boolean. The suggested diff is unnecessary.Likely an incorrect or invalid review comment.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py
Show resolved
Hide resolved
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py
Show resolved
Hide resolved
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py
Show resolved
Hide resolved
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py
Show resolved
Hide resolved
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py
Show resolved
Hide resolved
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (1)
78-80: Good: is_level_compliant mocked as callable (matches model semantics)This addresses prior feedback and prevents false positives.
🧹 Nitpick comments (12)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (12)
11-33: Trim unused constants; keep test surface minimal
- Unused: PRODUCTION_LEVEL (Line 27), FULL_SCORE_THRESHOLD (Line 31). Remove to avoid drift and noise.
- PRODUCTION_LEVEL = "production" ... - FULL_SCORE_THRESHOLD = 90.0
17-23: Patch the stable query entry-point to avoid brittle ORM internalsPatching objects.filter risks breaking if get_latest_health_metrics() changes internals (Subqueries, extra filters). Patch the method instead; your existing select_related chaining still works on the returned queryset.
-METRICS_FILTER_PATCH = ( - "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.objects.filter" -) +METRICS_FILTER_PATCH = ( + "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.get_latest_health_metrics" +)
35-41: Remove unused Command instantiationself.command is never used; drop it (and the Command import) to keep tests lean.
- self.command = Command()Also remove the import:
-from apps.owasp.management.commands.owasp_update_project_health_scores import Command
42-45: Consider autospeccing patchesAutospec catches signature misuse (e.g., bulk_save fields kw). Minor safety net.
- patch(METRICS_BULK_SAVE_PATCH) as bulk_save_patch, + patch(METRICS_BULK_SAVE_PATCH, autospec=True) as bulk_save_patch,
86-87: Prefer passing stdout to call_command over patching sys.stdoutReduces global side effects and is idiomatic for Django management commands.
- with patch(STDOUT_PATCH, new=self.stdout): - call_command("owasp_update_project_health_scores") + call_command("owasp_update_project_health_scores", stdout=self.stdout)Apply similarly to all tests using STDOUT_PATCH.
98-100: Use pytest.approx for float assertionsMore readable and purpose-built for float comparisons.
-assert ( - abs(mock_metric.score - EXPECTED_SCORE) < FLOAT_PRECISION -) # Use approximate comparison for float +assert mock_metric.score == pytest.approx(EXPECTED_SCORE, abs=FLOAT_PRECISION)
104-185: Compute expected score programmatically to avoid brittle coupling to weightsHard-coding “100 then minus 20%” embeds internal weighting assumptions. Derive base_score from field categories in the test to keep it resilient to future weight adjustments.
Example (outline):
- Build lists of “forward”, “backward”, and “boolean” fields used by the command.
- Compute base_score from those lengths and configured per-field weights used by the command, then apply the penalty.
176-185: Make penalty log assertion robust to float formatting (20 vs 20.0)Use general format specifier to match both.
-assert ( - f"Applied {PENALTY_TWENTY_PERCENT}% compliance penalty to {NON_COMPLIANT_PROJECT_NAME}" - in output -) +assert ( + f"Applied {PENALTY_TWENTY_PERCENT:g}% compliance penalty to {NON_COMPLIANT_PROJECT_NAME}" + in output +)Apply similarly where penalty percent appears in other tests.
259-327: Don’t require a zero-penalty log lineRequiring “Applied 0%…” is brittle; implementations often skip logging no-ops. Assert that score is unchanged; if a log appears, validate its content, but don’t fail if it’s absent.
-assert f"Applied {PENALTY_ZERO_PERCENT}% compliance penalty" in output +if "compliance penalty" in output: + assert f"Applied {PENALTY_ZERO_PERCENT:g}% compliance penalty" in output
328-393: Use pytest.approx for zeroed score tooConsistent float assertions.
-assert ( - abs(mock_metric.score - PENALTY_ZERO_PERCENT) < FLOAT_PRECISION -) # Use approximate comparison for float +assert mock_metric.score == pytest.approx(PENALTY_ZERO_PERCENT, abs=FLOAT_PRECISION)
394-474: Parametrize clamping cases for clearer reportingpytest.mark.parametrize will surface which tuple failed and removes loop boilerplate. Also make the penalty log assertion tolerant to 50 vs 50.0 formatting.
Example:
@pytest.mark.parametrize( "input_penalty,expected_penalty,expected_score", [(-10.0, 0.0, base_score), (150.0, 100.0, 0.0), (50.0, 50.0, base_score * 0.5)], ) def test_handle_penalty_weight_clamping(...): ... assert f"Applied {expected_penalty:g}% compliance penalty" in output assert mock_metric.score == pytest.approx(expected_score, abs=FLOAT_PRECISION)
42-45: Return iterable QuerySet-like mocks, not raw listsIf the command uses queryset methods (count, exists), returning a plain list could break. Safer to return a MagicMock with iter and needed methods, or set .all().iterator().
Sketch:
qs = MagicMock() qs.__iter__.return_value = iter([mock_metric]) self.mock_metrics.return_value.select_related.return_value = qsAlso applies to: 161-164, 242-245, 312-315, 381-384, 457-460
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (3)
backend/apps/owasp/models/project.py (7)
is_level_compliant(162-164)is_funding_requirements_compliant(150-152)is_leader_requirements_compliant(155-159)open_pull_requests_count(217-219)recent_releases_count(268-273)unanswered_issues_count(281-283)unassigned_issues_count(286-288)backend/apps/owasp/models/project_health_metrics.py (6)
ProjectHealthMetrics(16-235)age_days(86-88)last_commit_days(96-98)last_pull_request_days(106-112)last_release_days(120-122)owasp_page_last_update_days(130-136)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)
🔇 Additional comments (2)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (2)
186-258: LGTM: covers compliant path and absence of penaltyClear expectations and stdout checks; aligns with model’s is_level_compliant().
475-487: Drop this check—bulk_save([])is invoked unconditionally inhandle, so the test is correct.Likely an incorrect or invalid review comment.
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (1)
79-79: Do not patchis_level_compliantas a PropertyMock or at the MagicMock class level; use an instance-callable.
is_level_compliantis a method (callable) on Project. Assigning a PropertyMock ontype(mock_metric.project)mutates the MagicMock class globally and returns a non-callable, risking TypeError and cross-test contamination. Patch the instance with a callable instead.Apply this diff at each annotated line:
- type(mock_metric.project).is_level_compliant = PropertyMock(return_value=True) + mock_metric.project.is_level_compliant = MagicMock(return_value=True)- type(mock_metric.project).is_level_compliant = PropertyMock(return_value=False) + mock_metric.project.is_level_compliant = MagicMock(return_value=False)Also update imports to remove PropertyMock:
-from unittest.mock import MagicMock, PropertyMock, patch +from unittest.mock import MagicMock, patchAlso applies to: 152-152, 234-234, 304-304, 373-373, 443-443
🧹 Nitpick comments (2)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (2)
98-101: Usepytest.approxfor float assertions.Improves readability and avoids hand-rolled tolerances.
- assert ( - abs(mock_metric.score - EXPECTED_SCORE) < FLOAT_PRECISION - ) # Use approximate comparison for float + assert mock_metric.score == pytest.approx(EXPECTED_SCORE, abs=FLOAT_PRECISION)- assert abs(mock_metric.score - 80.0) < FLOAT_PRECISION # Should be 80 after 20% penalty + assert mock_metric.score == pytest.approx(80.0, abs=FLOAT_PRECISION)- assert abs(mock_metric.score - 100.0) < FLOAT_PRECISION # Should be maximum score + assert mock_metric.score == pytest.approx(100.0, abs=FLOAT_PRECISION)- assert abs(mock_metric.score - 100.0) < FLOAT_PRECISION # Should be maximum score + assert mock_metric.score == pytest.approx(100.0, abs=FLOAT_PRECISION)- assert ( - abs(mock_metric.score - PENALTY_ZERO_PERCENT) < FLOAT_PRECISION - ) # Use approximate comparison for float + assert mock_metric.score == pytest.approx(PENALTY_ZERO_PERCENT, abs=FLOAT_PRECISION)- assert abs(mock_metric.score - expected_score) < FLOAT_PRECISION + assert mock_metric.score == pytest.approx(expected_score, abs=FLOAT_PRECISION)Also applies to: 173-175, 253-254, 321-323, 391-392, 473-474
485-486: Fix comment to match the assertion behavior.The comment says "no bulk save was called" but the test asserts a call with an empty list.
- # Verify no bulk save was called and success message shown + # Verify bulk save was called with an empty list and success message shown
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (3)
backend/apps/owasp/models/project.py (1)
is_level_compliant(162-164)backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(16-235)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(10-70)
🔇 Additional comments (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_scores_test.py (1)
324-327: Test assertion for zero-weight penalty is valid – the_apply_compliance_penaltymethod logs “Applied 0.0% compliance penalty…” whenever a project is non-compliant, even if the penalty weight is zero, so the existing assertion is correct.
divyanshu-vr
changed the title
There was a problem hiding this comment.
@divyanshu-vr is this an AI generated code or you over complicated it yourself?
| @echo "Updating OWASP project health metrics" | ||
| @CMD="python manage.py owasp_update_project_health_metrics" $(MAKE) exec-backend-command | ||
|
|
||
| owasp-sync-official-project-levels: |
There was a problem hiding this comment.
This is not right coupling decision. I suggested to implement the official project levels during the project data sync. The health metrics is the next step after data sync is done.
| owasp-update-project-health-metrics \ | ||
| owasp-update-project-health-scores \ |
There was a problem hiding this comment.
The health metric generation is a separate step, we should not mix them.
| github-update-related-organizations \ | ||
| github-update-users \ | ||
| owasp-aggregate-projects \ | ||
| owasp-sync-official-project-levels \ |
There was a problem hiding this comment.
The logic behind this target should be a part of github-update-owasp-organization
| } | ||
| return forward_fields, backward_fields | ||
|
|
||
| def _calculate_base_score(self, metric, requirements, forward_fields, backward_fields): |
There was a problem hiding this comment.
I didn't check this code deeper -- why do we need this? The official level mismatch should be just another metric field. I think the most similar one is is_funding_requirements_compliant. Please simplify your code according to the existing process.
| unassigned_issues_count = models.PositiveIntegerField( | ||
| verbose_name="Unassigned issues", default=0 | ||
| ) | ||
| compliance_penalty_weight = models.FloatField( |
There was a problem hiding this comment.
It's out of the feature scope. We have the scoring system already in place and there is no need to introduce additional fields.
| from apps.owasp.models.project import Project | ||
|
|
||
| # Test constants | ||
| OWASP_ZAP_NAME = "OWASP ZAP" |
There was a problem hiding this comment.
This is not part of OWASP
@arkid15r it is partially ai generated, like the tests and where should the files go. Turns out a picked a really complicated issue to work on and couldn't give my 100% to it. Yeah, i realized this thing was overcomplicated. are there any files u found worth keeping? because I'm thinking to scrap this pr. |
Yes, please don't kill this PR contents -- let's keep it for reference. I think it's easier to give someone else an option either to reuse your code or implement it from scratch. |
3 participants
Proposed change
Resolves #2039
Add the PR description here.
Checklist
make check-testlocally; all checks and tests passed.