Create a health evaluation criteria #1550
Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThe changes introduce new Django management commands and supporting model enhancements to populate and score OWASP project health metrics. Model fields and properties are added or adjusted for metric calculation, and database migrations update schema accordingly. Comprehensive tests for the commands and metrics logic are included, alongside Makefile updates for command invocation. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Suggested reviewers
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
backend/apps/owasp/models/project.py (2)
209-214: Optimize pull request query for better performance.The current implementation retrieves the entire pull request object just to get the creation timestamp. Consider using
values_listfor better performance.@property def pull_request_last_created_at(self) -> datetime.datetime | None: """Return last created pull request.""" - if pull_request := self.pull_requests.order_by("-created_at").first(): - return pull_request.created_at - return None + result = self.pull_requests.values_list("created_at", flat=True).order_by("-created_at").first() + return result
161-164: Consider adding database indexes for performance.These new properties perform filtering and counting operations that could benefit from database indexes on the foreign key relationships and filter fields.
Consider adding indexes for:
issues.comments_count(for unanswered_issues_count)issues.assignees(for unassigned_issues_count)pull_requests.state(for open_pull_requests_count)These indexes would improve the performance of the health metrics calculations, especially as the dataset grows.
Also applies to: 190-193, 204-207, 243-246, 248-251
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
14-14: Remove unused parameters.The
handlemethod has unusedargsandoptionsparameters that can be removed or prefixed with underscore to indicate they're intentionally unused.-def handle(self, *args, **options): +def handle(self, *_args, **_options):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 14-14: Missing function or method docstring
(C0116)
[warning] 14-14: Unused argument 'args'
(W0613)
[warning] 14-14: Unused argument 'options'
(W0613)
38-40: Address TODO comments for completeness.The TODO comments indicate missing functionality for OWASP page updates, funding compliance, and scoring. Consider creating GitHub issues to track these requirements.
Would you like me to help create detailed GitHub issues for these TODO items to ensure they're properly tracked and implemented?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py(1 hunks)backend/apps/owasp/migrations/0035_rename_total_pull_request_count_projecthealthmetrics_total_pull_requests_count.py(1 hunks)backend/apps/owasp/models/project.py(7 hunks)backend/apps/owasp/models/project_health_metrics.py(1 hunks)cspell/custom-dict.txt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/models/project.py (1)
backend/apps/github/models/repository.py (1)
published_releases(163-169)
🪛 Pylint (3.3.7)
backend/apps/owasp/migrations/0035_rename_total_pull_request_count_projecthealthmetrics_total_pull_requests_count.py
[convention] 1-1: Missing module docstring
(C0114)
[convention] 1-1: Module name "0035_rename_total_pull_request_count_projecthealthmetrics_total_pull_requests_count" doesn't conform to snake_case naming style
(C0103)
[error] 3-3: Unable to import 'django.db'
(E0401)
[convention] 6-6: Missing class docstring
(C0115)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
[error] 3-3: Unable to import 'django.core.management.base'
(E0401)
[convention] 11-11: Missing class docstring
(C0115)
[convention] 14-14: Missing function or method docstring
(C0116)
[warning] 14-14: Unused argument 'args'
(W0613)
[warning] 14-14: Unused argument 'options'
(W0613)
[refactor] 11-11: Too few public methods (1/2)
(R0903)
backend/apps/owasp/models/project.py
[error] 8-8: Unable to import 'django.db'
(E0401)
[error] 9-9: Unable to import 'django.utils'
(E0401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (4)
cspell/custom-dict.txt (1)
30-30: Add contributor name to custom dictionary
The new entryahmedxgoudais correctly added and alphabetized.backend/apps/owasp/models/project.py (1)
176-179:✅ Verification successful
Verify that
leaders_rawattribute exists.The
leaders_countproperty referencesself.leaders_raw, but this attribute is not defined in the current model. Please ensure this attribute exists or provide the correct reference.
🏁 Script executed:
#!/bin/bash # Description: Search for leaders_raw definition in the Project model and related files # Expected: Find where leaders_raw is defined or used echo "Searching for leaders_raw in Project model files..." rg -A 3 -B 3 "leaders_raw" --type py echo -e "\nSearching for leaders field definitions..." ast-grep --pattern 'leaders$_ = $_'Length of output: 11746
No action needed:
leaders_rawis defined in the base model
TheProjectmodel inherits theleaders_rawJSONField frombackend/apps/owasp/models/common.py, soself.leaders_rawis valid andleaders_countcan safely return its length.backend/apps/owasp/models/project_health_metrics.py (1)
54-56: LGTM! Field rename improves naming consistency.The rename from
total_pull_request_counttototal_pull_requests_countaligns with thepull_requests_countproperty in the Project model and follows consistent plural naming conventions.backend/apps/owasp/migrations/0035_rename_total_pull_request_count_projecthealthmetrics_total_pull_requests_count.py (1)
1-17: LGTM! Standard Django migration for field rename.The migration correctly renames the field using Django's
RenameFieldoperation with proper dependencies. This will handle the database schema change seamlessly.🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[convention] 1-1: Module name "0035_rename_total_pull_request_count_projecthealthmetrics_total_pull_requests_count" doesn't conform to snake_case naming style
(C0103)
[error] 3-3: Unable to import 'django.db'
(E0401)
[convention] 6-6: Missing class docstring
(C0115)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (3)
11-12: Add class and method docstrings for better documentation.The command class and handle method are missing docstrings, which would improve code documentation and maintainability.
class Command(BaseCommand): + """Django management command to update OWASP project health metrics.""" help = "Update OWASP project health metrics."🧰 Tools
🪛 Pylint (3.3.7)
[convention] 11-11: Missing class docstring
(C0115)
[refactor] 11-11: Too few public methods (1/2)
(R0903)
14-14: Address unused arguments and add method docstring.The handle method has unused arguments and lacks documentation.
- def handle(self, *args, **options): + def handle(self, *args, **options): + """Handle the management command execution."""Consider using
_prefix for unused arguments:- def handle(self, *args, **options): + def handle(self, *_args, **_options):🧰 Tools
🪛 Pylint (3.3.7)
[convention] 14-14: Missing function or method docstring
(C0116)
[warning] 14-14: Unused argument 'args'
(W0613)
[warning] 14-14: Unused argument 'options'
(W0613)
15-33: Consider performance optimization with select_related and prefetch_related.The current implementation queries each project individually without optimizing database access. This could be slow for large datasets.
- projects = Project.objects.all() + projects = Project.objects.all().select_related().prefetch_related( + 'repositories', 'issues', 'pull_requests', 'published_releases' + )This optimization aligns with the past review suggestion for performance improvements.
backend/apps/owasp/models/project.py (2)
172-175: Potential performance concern with issues_count property.This property triggers a database query each time it's accessed. Consider caching or using the existing synthetic field if performance becomes an issue.
If this property is accessed frequently, consider adding
@lru_cacheor using the existingopen_issues_countfield pattern:+from functools import lru_cache @property +@lru_cache(maxsize=1) def issues_count(self) -> int: """Return count of issues.""" return self.issues.count()
201-204: Consider performance optimization for pull request queries.Both
open_pull_requests_countandpull_requests_countexecute separate database queries that could be expensive for projects with many pull requests.Consider caching these properties if they're accessed frequently:
+from functools import lru_cache @property +@lru_cache(maxsize=1) def open_pull_requests_count(self) -> int: """Return count of open pull requests.""" return self.pull_requests.filter(state="open").count() @property +@lru_cache(maxsize=1) def pull_requests_count(self) -> int: """Return count of pull requests.""" return self.pull_requests.count()Also applies to: 215-218
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py(1 hunks)backend/apps/owasp/models/project.py(7 hunks)backend/tests/apps/owasp/models/project_health_metrics_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
backend/apps/owasp/models/project.py (2)
Project(24-358)leaders_count(188-190)backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(9-67)
backend/apps/owasp/models/project.py (2)
backend/apps/owasp/graphql/nodes/project.py (2)
repositories(75-77)issues_count(38-40)backend/apps/github/models/repository.py (1)
published_releases(163-169)
🪛 Pylint (3.3.7)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
[error] 3-3: Unable to import 'django.core.management.base'
(E0401)
[convention] 11-11: Missing class docstring
(C0115)
[convention] 14-14: Missing function or method docstring
(C0116)
[warning] 14-14: Unused argument 'args'
(W0613)
[warning] 14-14: Unused argument 'options'
(W0613)
[refactor] 11-11: Too few public methods (1/2)
(R0903)
backend/apps/owasp/models/project.py
[error] 8-8: Unable to import 'django.db'
(E0401)
[error] 9-9: Unable to import 'django.utils'
(E0401)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (7)
backend/tests/apps/owasp/models/project_health_metrics_test.py (1)
75-75: LGTM! Field name correction aligns with model changes.The test parameter name has been correctly updated to match the renamed field
total_pull_requests_countin theProjectHealthMetricsmodel, ensuring test consistency with the actual model schema.backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (1)
28-29:✅ Verification successful
Verify field mapping correctness.
Please ensure that the
issues_countandpull_requests_countproperties exist on the Project model and return the expected values for total counts.
🏁 Script executed:
#!/bin/bash # Description: Verify that the Project model has the required properties for field mapping # Expected: Find property definitions for issues_count and pull_requests_count ast-grep --pattern $'@property def issues_count($$$): $$$' ast-grep --pattern $'@property def pull_requests_count($$$): $$$'Length of output: 136
🏁 Script executed:
#!/bin/bash # Search for the Project model class definition and context rg -n "class Project" -C 5 # Search for any occurrences of issues_count and pull_requests_count in the repo rg -n "issues_count" . rg -n "pull_requests_count" .Length of output: 26400
🏁 Script executed:
#!/bin/bash # Verify @property decorator on issues_count and pull_requests_count in Project model rg -n -C2 "def issues_count" backend/apps/owasp/models/project.py rg -n -C2 "def pull_requests_count" backend/apps/owasp/models/project.pyLength of output: 449
Field mappings verified
The
Projectmodel defines both properties with the expected behavior inbackend/apps/owasp/models/project.py:
@property def issues_count(self)(lines 173–175) returnsself.issues.count()@property def pull_requests_count(self)(lines 216–218) returnsself.pull_requests.count()The mappings in
owasp_update_project_health_metrics.pyare correct.backend/apps/owasp/models/project.py (5)
145-156: LGTM! Funding compliance logic is well-implemented.The funding requirements compliance check correctly handles edge cases (zero repositories) and uses appropriate filtering to determine compliance across all repositories.
187-190: LGTM! Leaders count implementation is efficient.The property correctly uses the length of the existing
leaders_rawattribute, avoiding additional database queries.
220-225: LGTM! Efficient implementation for last pull request timestamp.The use of the walrus operator with
order_byandfirst()is an efficient way to get the latest pull request without fetching unnecessary data.
247-253: Excellent fix for the dynamic time calculation!The implementation correctly addresses the past review feedback by calculating
recent_perioddynamically inside the property method instead of using a module-level constant. This ensures the 60-day cutoff is always current.
255-263: LGTM! Issue filtering logic is correct.Both
unanswered_issues_countandunassigned_issues_countuse appropriate filtering conditions:
comments_count=0for unanswered issuesassignees__isnull=Truefor unassigned issuesThe logic is sound and follows Django ORM best practices.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.py (1)
14-30: Consider externalizing the weight mapping configuration.The hardcoded weight mapping makes the scoring system inflexible. Consider moving this to a configuration file, database table, or Django settings to allow easier adjustments without code changes.
This could be moved to Django settings:
# In settings.py HEALTH_METRICS_WEIGHTS = { "age_days": 6.0, "contributors_count": 6.0, # ... rest of weights } # In command from django.conf import settings weight_mapping = settings.HEALTH_METRICS_WEIGHTSbackend/apps/owasp/models/project.py (1)
172-174: Consider performance impact of repeated count() operations.Several new properties perform
count()operations on potentially large querysets. If these properties are accessed frequently (e.g., in list views or bulk operations), they could impact performance.Consider:
- Using
select_related()andprefetch_related()when accessing these properties in bulk- Caching these values in the existing synthetic fields (like the current
commits_count,contributors_count, etc.)- Adding database indexes on frequently filtered fields
For immediate improvement, document that these properties should be used judiciously in performance-critical code.
Also applies to: 187-189, 201-203, 222-224, 254-259, 262-264, 267-269, 272-274
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/Makefile(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.py(1 hunks)backend/apps/owasp/models/project.py(7 hunks)backend/apps/owasp/models/project_health_metrics.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/Makefile
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/apps/owasp/models/project_health_metrics.py (1)
backend/apps/owasp/models/project.py (2)
pull_request_last_created_at(227-231)owasp_page_last_updated_at(206-210)
backend/apps/owasp/models/project.py (3)
backend/apps/owasp/graphql/nodes/project.py (3)
repositories_count(80-82)repositories(75-77)issues_count(38-40)backend/apps/github/models/user.py (1)
issues(43-50)backend/apps/github/models/repository.py (1)
published_releases(163-169)
🪛 Pylint (3.3.7)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.py
[error] 3-3: Unable to import 'django.core.management.base'
(E0401)
[convention] 9-9: Missing class docstring
(C0115)
[convention] 12-12: Missing function or method docstring
(C0116)
[warning] 12-12: Unused argument 'args'
(W0613)
[warning] 12-12: Unused argument 'options'
(W0613)
[refactor] 9-9: Too few public methods (1/2)
(R0903)
backend/apps/owasp/models/project_health_metrics.py
[error] 5-5: Unable to import 'django.utils'
(E0401)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
[error] 3-3: Unable to import 'django.core.management.base'
(E0401)
[convention] 11-11: Missing class docstring
(C0115)
[convention] 14-14: Missing function or method docstring
(C0116)
[warning] 14-14: Unused argument 'args'
(W0613)
[warning] 14-14: Unused argument 'options'
(W0613)
[refactor] 11-11: Too few public methods (1/2)
(R0903)
backend/apps/owasp/models/project.py
[error] 8-8: Unable to import 'django.db'
(E0401)
[error] 9-9: Unable to import 'django.utils'
(E0401)
🔇 Additional comments (8)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.py (2)
32-48: LGTM! Solid scoring logic implementation.The scoring algorithm correctly:
- Fetches requirements based on project level
- Compares metric values against requirement thresholds
- Accumulates weights for metrics that meet requirements
- Updates and saves the calculated score
50-53: 🛠️ Refactor suggestionInclude exception details in error logging.
The error handling catches appropriate exception types but doesn't include the actual exception message, making debugging difficult.
- except (AttributeError, ValueError, TypeError) as e: + except (AttributeError, ValueError, TypeError) as e: self.stdout.write( - self.style.ERROR(f"Error updating project {metric.project.name}: {e}") + self.style.ERROR(f"Error updating project {metric.project.name}: {e}") )Likely an incorrect or invalid review comment.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (1)
14-61: Excellent implementation with comprehensive error handling.This command demonstrates good practices:
- Comprehensive field mappings between models
- Proper error handling with specific exception types
- Progress tracking with success/error counts
- Clear logging for debugging
- Compliance checking for leader requirements
The field mappings correctly align with the model structures and the error handling prevents individual project failures from stopping the entire operation.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 14-14: Missing function or method docstring
(C0116)
[warning] 14-14: Unused argument 'args'
(W0613)
[warning] 14-14: Unused argument 'options'
(W0613)
backend/apps/owasp/models/project_health_metrics.py (3)
55-57: LGTM! Field rename improves naming consistency.The rename from
total_pull_request_counttototal_pull_requests_countaligns with the plural naming convention used by other count fields in the model.
66-100: Consistent and well-structured property implementations.All five new properties follow the same reliable pattern:
- Check for null datetime values
- Calculate difference from current time
- Return 0 as a sensible default
This provides a clean API for the health scoring system to access temporal metrics.
66-100:✅ Verification successful
Verify timezone compatibility in datetime calculations.
The new properties use
timezone.now()(timezone-aware) to calculate differences with datetime fields. Ensure that the datetime fields being compared (created_at,last_committed_at, etc.) are also timezone-aware to avoidTypeErrorwhen subtracting naive and aware datetimes.
🏁 Script executed:
#!/bin/bash # Description: Check if datetime fields in the model are timezone-aware # Expected: Should find timezone settings and field configurations echo "Checking timezone configuration..." rg -A 3 -B 3 "USE_TZ|TIME_ZONE" --type py echo -e "\nChecking DateTimeField definitions in project models..." rg "DateTimeField.*auto_now|DateTimeField.*default.*timezone" --type pyLength of output: 6991
🏁 Script executed:
#!/bin/bash # Inspect field definitions in ProjectHealthMetrics model rg -n -A3 -B3 "class ProjectHealthMetrics" --max-count=1 backend/apps/owasp/models/project_health_metrics.py rg -n -A2 "created_at\s*=" backend/apps/owasp/models/project_health_metrics.py rg -n -A2 "last_committed_at\s*=" backend/apps/owasp/models/project_health_metrics.py rg -n -A2 "pull_request_last_created_at\s*=" backend/apps/owasp/models/project_health_metrics.py rg -n -A2 "last_released_at\s*=" backend/apps/owasp/models/project_health_metrics.py rg -n -A2 "owasp_page_last_updated_at\s*=" backend/apps/owasp/models/project_health_metrics.pyLength of output: 1754
No timezone issues detected. With
USE_TZ = True(TIME_ZONE = "UTC") in settings and all relevant fields defined asDateTimeField, Django stores them as timezone-aware datetimes. Subtractingtimezone.now()from these fields will not raise aTypeError.backend/apps/owasp/models/project.py (2)
145-155: Excellent funding compliance logic implementation.The compliance checking correctly:
- Returns
Truefor projects with no repositories (sensible default)- Counts repositories with compliant funding policies
- Requires ALL repositories to be compliant for overall compliance
This ensures strict adherence to funding requirements across the entire project scope.
206-210: Clean datetime property implementations.The
owasp_page_last_updated_atandpull_request_last_created_atproperties provide clean access to temporal data needed for health metrics calculations. The null-safe implementations handle missing data gracefully.Also applies to: 227-231
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
52-54: Include exception details in error logging for better debugging.The current exception handling only logs the project name but omits the actual error details, making debugging difficult.
- except (AttributeError, ValueError, TypeError): - self.stdout.write(self.style.ERROR(f"Error updating project {project.name}")) + except (AttributeError, ValueError, TypeError) as e: + self.stdout.write(self.style.ERROR(f"Error updating project {project.name}: {e}"))
14-14: Add method docstring for better documentation.Consider adding a docstring to describe the method's purpose and behavior.
def handle(self, *args, **options): + """Update project health metrics for all projects."""🧰 Tools
🪛 Pylint (3.3.7)
[convention] 14-14: Missing function or method docstring
(C0116)
[warning] 14-14: Unused argument 'args'
(W0613)
[warning] 14-14: Unused argument 'options'
(W0613)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.py (1)
12-12: Add method docstring for better documentation.Consider adding a docstring to describe the scoring algorithm and method behavior.
def handle(self, *args, **options): + """Calculate and update health metric scores for all projects based on requirements."""🧰 Tools
🪛 Pylint (3.3.7)
[convention] 12-12: Missing function or method docstring
(C0116)
[warning] 12-12: Unused argument 'args'
(W0613)
[warning] 12-12: Unused argument 'options'
(W0613)
backend/apps/owasp/models/project.py (1)
171-174: Consider performance implications of computed count properties.These properties perform database queries each time they're accessed. While this provides accurate real-time counts, it could impact performance if called frequently in loops or views.
For the health metrics use case, this is likely acceptable since these are typically called during batch processing. However, consider caching or using annotated querysets if performance becomes an issue.
Also applies to: 222-224
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/Makefile(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.py(1 hunks)backend/apps/owasp/models/project.py(7 hunks)backend/apps/owasp/models/project_health_metrics.py(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/Makefile
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/apps/owasp/models/project_health_metrics.py (1)
backend/apps/owasp/models/project.py (2)
pull_request_last_created_at(227-231)owasp_page_last_updated_at(206-210)
backend/apps/owasp/models/project.py (2)
backend/apps/owasp/graphql/nodes/project.py (3)
repositories_count(80-82)repositories(75-77)issues_count(38-40)backend/apps/github/models/repository.py (1)
published_releases(163-169)
🪛 Pylint (3.3.7)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.py
[error] 3-3: Unable to import 'django.core.management.base'
(E0401)
[convention] 9-9: Missing class docstring
(C0115)
[convention] 12-12: Missing function or method docstring
(C0116)
[warning] 12-12: Unused argument 'args'
(W0613)
[warning] 12-12: Unused argument 'options'
(W0613)
[refactor] 9-9: Too few public methods (1/2)
(R0903)
backend/apps/owasp/models/project_health_metrics.py
[error] 5-5: Unable to import 'django.utils'
(E0401)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
[error] 3-3: Unable to import 'django.core.management.base'
(E0401)
[convention] 11-11: Missing class docstring
(C0115)
[convention] 14-14: Missing function or method docstring
(C0116)
[warning] 14-14: Unused argument 'args'
(W0613)
[warning] 14-14: Unused argument 'options'
(W0613)
[refactor] 11-11: Too few public methods (1/2)
(R0903)
backend/apps/owasp/models/project.py
[error] 8-8: Unable to import 'django.db'
(E0401)
[error] 9-9: Unable to import 'django.utils'
(E0401)
🔇 Additional comments (8)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (1)
16-34: Well-structured field mapping approach.The field mappings dictionary provides a clean and maintainable way to map between Project and ProjectHealthMetrics fields. This approach makes it easy to add or modify field mappings in the future.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.py (2)
14-30: Well-designed scoring system with appropriate weights.The weight mapping provides a clear and configurable approach to score calculation. The higher weights (8.0) for key metrics like stars_count and total_pull_requests_count appropriately emphasize their importance in project health assessment.
36-48: Efficient scoring logic with proper error handling.The scoring algorithm correctly compares metric values against requirements and accumulates weighted scores. Using
update_fields=["score"]optimizes the database update by only updating the score field.backend/apps/owasp/models/project_health_metrics.py (2)
55-57: Field rename improves naming consistency.The rename from
total_pull_request_counttototal_pull_requests_countimproves consistency with other plural count fields in the model.
66-100: Excellent implementation of computed time-based properties.The five new properties follow a consistent and robust pattern:
- Consistent naming convention with
_dayssuffix- Proper null checking before calculation
- Sensible default return value of 0 when datetime is not set
- Clear and descriptive docstrings
This implementation provides valuable computed metrics for the health scoring system.
backend/apps/owasp/models/project.py (3)
145-155: Well-implemented funding compliance check.The logic correctly handles the edge case where projects with no repositories are considered compliant, and properly calculates compliance by comparing compliant repositories against the total count.
206-210: Clean implementation of timestamp properties.Both
owasp_page_last_updated_atandpull_request_last_created_atfollow good patterns:
- Proper null checking with walrus operator
- Clear logic for retrieving related data
- Appropriate return type annotations
These properties provide essential timestamp data for the health metrics system.
Also applies to: 227-231
253-259: Excellent fix for dynamic time calculation.The recent releases count now correctly calculates the 60-day period dynamically using
timezone.now() - datetime.timedelta(days=60)within the property, addressing the previous issue where the constant was evaluated at import time.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (1)
13-27: Well-structured test setup with minor improvement opportunity.The fixture properly isolates the command under test using appropriate patches. Consider these refinements:
@pytest.fixture(autouse=True) def _setup(self): """Set up test environment.""" self.stdout = StringIO() self.command = Command() with ( patch("apps.owasp.models.project.Project.objects.all") as projects_patch, patch( "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.objects" ) as metrics_patch, ): self.mock_projects = projects_patch self.mock_metrics = metrics_patch yield + # Add any cleanup logic here if needed in the future🧰 Tools
🪛 Pylint (3.3.7)
[warning] 16-16: Attribute 'stdout' defined outside init
(W0201)
[warning] 17-17: Attribute 'command' defined outside init
(W0201)
[warning] 24-24: Attribute 'mock_projects' defined outside init
(W0201)
[warning] 25-25: Attribute 'mock_metrics' defined outside init
(W0201)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_score_test.py (1)
13-29: Good test setup with minor formatting issue.The fixture setup is well-structured for testing score calculations. Fix the line length issue:
patch( - "apps.owasp.models.project_health_requirements.ProjectHealthRequirements.objects.get" + "apps.owasp.models.project_health_requirements." + "ProjectHealthRequirements.objects.get" ) as requirements_patch,🧰 Tools
🪛 Pylint (3.3.7)
[convention] 23-23: Line too long (101/100)
(C0301)
[warning] 16-16: Attribute 'stdout' defined outside init
(W0201)
[warning] 17-17: Attribute 'command' defined outside init
(W0201)
[warning] 26-26: Attribute 'mock_metrics' defined outside init
(W0201)
[warning] 27-27: Attribute 'mock_requirements' defined outside init
(W0201)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_score_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_score_test.py (2)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-103)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(9-57)
🪛 Pylint (3.3.7)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py
[convention] 1-1: Missing module docstring
(C0114)
[error] 5-5: Unable to import 'django.core.management'
(E0401)
[convention] 12-12: Missing class docstring
(C0115)
[warning] 16-16: Attribute 'stdout' defined outside init
(W0201)
[warning] 17-17: Attribute 'command' defined outside init
(W0201)
[warning] 24-24: Attribute 'mock_projects' defined outside init
(W0201)
[warning] 25-25: Attribute 'mock_metrics' defined outside init
(W0201)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_score_test.py
[convention] 23-23: Line too long (101/100)
(C0301)
[convention] 1-1: Missing module docstring
(C0114)
[error] 5-5: Unable to import 'django.core.management'
(E0401)
[convention] 12-12: Missing class docstring
(C0115)
[warning] 16-16: Attribute 'stdout' defined outside init
(W0201)
[warning] 17-17: Attribute 'command' defined outside init
(W0201)
[warning] 26-26: Attribute 'mock_metrics' defined outside init
(W0201)
[warning] 27-27: Attribute 'mock_requirements' defined outside init
(W0201)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (7)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (4)
1-10: Good import structure for Django management command testing.The imports are well-organized and include all necessary components for testing Django management commands with mocking.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 5-5: Unable to import 'django.core.management'
(E0401)
28-77: Excellent comprehensive test case for successful scenario.This test demonstrates good practices:
- Comprehensive test data covering all field mappings
- Proper use of MagicMock with spec for type safety
- Verification of both command output and method calls
- Clear test structure and assertions
The field mapping approach makes the test maintainable and easy to understand.
78-88: Good edge case coverage for empty projects scenario.This test ensures the command handles empty datasets gracefully and provides appropriate feedback to users.
89-104: Essential error handling test case.This test ensures robust error handling and proper user feedback when individual project updates fail. Good coverage of exception scenarios.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_score_test.py (3)
1-10: Appropriate imports for score calculation testing.The imports correctly include the necessary models and testing utilities for the health metrics scoring command.
🧰 Tools
🪛 Pylint (3.3.7)
[convention] 1-1: Missing module docstring
(C0114)
[error] 5-5: Unable to import 'django.core.management'
(E0401)
79-98: Good error handling coverage for requirements retrieval failure.This test ensures the command handles database errors gracefully and provides meaningful error feedback when project health requirements cannot be retrieved.
30-78:✅ Verification successful
Well-structured score calculation test with verification needed.
The test properly covers the successful scoring scenario and verifies key interactions. However, ensure the scoring logic aligns with the actual implementation:
The test assumes simple weight summation - verify this matches the actual scoring algorithm.
🏁 Script executed:
#!/bin/bash # Description: Verify the actual scoring algorithm implementation in the command # Expected: Score calculation logic that matches the test assumptions ast-grep --pattern 'def handle($$$) { $$$ }' # Also check for score calculation logic rg -A 10 -B 5 "score.*=" backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.pyLength of output: 1134
Scoring logic verification complete: test assumptions match implementation
The command’s scoring algorithm simply sums each field’s weight when the metric value meets or exceeds the requirement value. Your test correctly mirrors this behavior by setting identical metric and requirement values, resulting in the expected total weight sum.
aramattamara
left a comment
aramattamara
left a comment
There was a problem hiding this comment.
Thanks for working on this. You'll need to fix some issues and make improvements.
| from apps.owasp.models.project import Project | ||
| from apps.owasp.models.project_health_metrics import ProjectHealthMetrics | ||
|
|
||
| MINIMUM_LEADERS = 3 |
There was a problem hiding this comment.
Could you add a comment on what it is, and why is 3?
There was a problem hiding this comment.
Actually it should be 2, it was stated in the description of the original idea: #711
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_score.py
Outdated
Show resolved
Hide resolved
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Great implementation, I wonder why you changed the weights part at the end -- let's keep it flexible.
I think later we'll need more tests for different project attributes vs final score cases.
| for field in forward_fields: | ||
| if getattr(metric, field) >= getattr(requirements, field): | ||
| score += 6.0 | ||
| for field in backward_fields: | ||
| if getattr(metric, field) <= getattr(requirements, field): | ||
| score += 6.0 |
There was a problem hiding this comment.
This approach removes flexibility your code used to have -- a custom weight per metric. Please revert it to the previous approach.
There was a problem hiding this comment.
I thought It was suggested by Kate here: #1550 (comment)
Maybe I got it wrong.
| score += 5.0 | ||
| if metric.is_project_leaders_requirements_compliant: | ||
| score += 5.0 |
There was a problem hiding this comment.
These fields also need to be in the same mapping/config of metrics/weights.
There was a problem hiding this comment.
Okay, but these are not parts of the ProjectHealthRequirements model, so we need an additional if statement
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (2)
43-45:self.style.NOTICEissue already flaggedSame
AttributeErrorrisk as in the metrics command. See earlier comment.
49-54:Nonevalues will crash the comparison
>=/<=onNoneraisesTypeError. Guard before comparing:lhs = getattr(metric, field) rhs = getattr(requirements, field) if lhs is not None and rhs is not None and lhs >= rhs: ...
🧹 Nitpick comments (5)
backend/apps/owasp/migrations/0040_alter_projecthealthmetrics_is_leader_requirements_compliant.py (1)
17-20: Verbose name wording is a bit clunky
"Is leader requirements compliant"reads awkwardly (“leader requirements” vs “leaders’ requirements” / “leader-requirements”).
If you anticipate additional migrations soon, consider renaming to something clearer such as"Leader requirements compliant"for consistency with other verbose names.backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (1)
34-44: Avoid loading entire table & N+1 queries
Project.objects.all()will materialise every row and, unless theProjectproperties are computed withselect_related/prefetch_related, may trigger additional queries per project.- for project in Project.objects.all(): + for project in ( + Project.objects + .all() + .select_related() # adjust as needed + .iterator(chunk_size=1000) # stream rows to keep memory low + ):backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
20-34: Patching thefilter/allcall-chain is fragilePatching deep attributes (
objects.filter) works but is brittle: any internal refactor (e.g. addingselect_for_update) will bypass the mock.
Patch the manager itself and configure its return value instead:-with patch("apps.owasp.models.project_health_metrics.ProjectHealthMetrics.objects.filter") as metrics_patch: +with patch.object(ProjectHealthMetrics, "objects", autospec=True) as metrics_manager:This keeps the test resilient to query-building changes.
backend/apps/owasp/models/project_health_metrics.py (2)
66-98: Repeatedtimezone.now()calls – cache for performanceEach property independently calls
timezone.now().
When scoring hundreds of metrics this results in redundant system-time lookups.+_NOW = timezone.now ... - return (timezone.now() - self.created_at).days if self.created_at else 0 + now = _NOW() + return (now - self.created_at).days if self.created_at else 0Do the same for the other “*_days” helpers or compute
nowonce in the calling command.
99-109:bulk_savewrapper leaks model referenceThe helper hard-codes
ProjectHealthMetrics; forwardingclsmakes it future-proof and testable:- BulkSaveModel.bulk_save(ProjectHealthMetrics, metrics, fields=fields) + BulkSaveModel.bulk_save(type(metrics[0]) if metrics else ProjectHealthMetrics, metrics, fields=fields)Minor, but keeps the API consistent with the mix-in pattern used elsewhere.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py(1 hunks)backend/apps/owasp/migrations/0038_remove_projecthealthmetrics_is_project_leaders_requirements_compliant_and_more.py(1 hunks)backend/apps/owasp/migrations/0039_rename_is_leaders_requirements_compliant_projecthealthmetrics_is_leader_requirements_compliant.py(1 hunks)backend/apps/owasp/migrations/0040_alter_projecthealthmetrics_is_leader_requirements_compliant.py(1 hunks)backend/apps/owasp/models/project.py(7 hunks)backend/apps/owasp/models/project_health_metrics.py(4 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py(1 hunks)backend/tests/apps/owasp/models/project_health_metrics_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/tests/apps/owasp/models/project_health_metrics_test.py
- backend/apps/owasp/models/project.py
🧰 Additional context used
🧬 Code Graph Analysis (5)
backend/apps/owasp/admin.py (1)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-112)
backend/apps/owasp/migrations/0038_remove_projecthealthmetrics_is_project_leaders_requirements_compliant_and_more.py (2)
backend/apps/owasp/migrations/0039_rename_is_leaders_requirements_compliant_projecthealthmetrics_is_leader_requirements_compliant.py (1)
Migration(6-20)backend/apps/owasp/migrations/0040_alter_projecthealthmetrics_is_leader_requirements_compliant.py (1)
Migration(6-22)
backend/apps/owasp/migrations/0039_rename_is_leaders_requirements_compliant_projecthealthmetrics_is_leader_requirements_compliant.py (2)
backend/apps/owasp/migrations/0038_remove_projecthealthmetrics_is_project_leaders_requirements_compliant_and_more.py (1)
Migration(6-23)backend/apps/owasp/migrations/0040_alter_projecthealthmetrics_is_leader_requirements_compliant.py (1)
Migration(6-22)
backend/apps/owasp/migrations/0040_alter_projecthealthmetrics_is_leader_requirements_compliant.py (2)
backend/apps/owasp/migrations/0038_remove_projecthealthmetrics_is_project_leaders_requirements_compliant_and_more.py (1)
Migration(6-23)backend/apps/owasp/migrations/0039_rename_is_leaders_requirements_compliant_projecthealthmetrics_is_leader_requirements_compliant.py (1)
Migration(6-20)
backend/apps/owasp/models/project_health_metrics.py (2)
backend/apps/common/models.py (2)
BulkSaveModel(8-30)TimestampedModel(33-40)backend/apps/owasp/models/project.py (5)
Meta(35-40)is_leader_requirements_compliant(151-155)pull_request_last_created_at(227-231)owasp_page_last_updated_at(206-210)bulk_save(336-344)
🪛 Pylint (3.3.7)
backend/apps/owasp/admin.py
[refactor] 167-167: Too few public methods (1/2)
(R0903)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
[refactor] 9-9: Too few public methods (1/2)
(R0903)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
[refactor] 9-9: Too few public methods (1/2)
(R0903)
backend/apps/owasp/migrations/0038_remove_projecthealthmetrics_is_project_leaders_requirements_compliant_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0039_rename_is_leaders_requirements_compliant_projecthealthmetrics_is_leader_requirements_compliant.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0040_alter_projecthealthmetrics_is_leader_requirements_compliant.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/models/project_health_metrics.py
[refactor] 13-13: Too few public methods (0/2)
(R0903)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
[refactor] 14-14: Too few public methods (1/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (2)
backend/apps/owasp/migrations/0039_rename_is_leaders_requirements_compliant_projecthealthmetrics_is_leader_requirements_compliant.py (1)
14-19: Potential historical data lossThe preceding migration 0038 removed the original field and re-added a fresh one with a default
False, then this migration merely renames it.
Anyis_project_leaders_requirements_compliantdata present before 0038 is now irretrievable. If those values mattered, the safer path would have been aRenameField(preserves data) or a two-stepAddField+RunPythoncopy before theRemoveField.Please verify that erasing historical compliance flags is acceptable.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
47-54: KeyError when project level missing
requirements = project_health_requirements[metric.project.level]assumes every level is present.
Use.get()and handle the missing case to prevent the whole command from crashing on an unrecognised level.requirements = project_health_requirements.get(metric.project.level) if requirements is None: self.stdout.write(self.style.ERROR( f"No health requirements configured for level {metric.project.level}" )) continue
| operations = [ | ||
| migrations.RemoveField( | ||
| model_name="projecthealthmetrics", | ||
| name="is_project_leaders_requirements_compliant", | ||
| ), | ||
| migrations.AddField( | ||
| model_name="projecthealthmetrics", | ||
| name="is_leaders_requirements_compliant", | ||
| field=models.BooleanField( | ||
| default=False, verbose_name="Is project leaders requirements compliant" | ||
| ), | ||
| ), |
There was a problem hiding this comment.
Remove-then-add pattern silently drops existing column values
RemoveField followed immediately by AddField resets every project’s compliance flag to the default False.
Unless intentional, swap the operations for a RenameField or add a RunPython step that backfills the new column from the old one before dropping it.
🤖 Prompt for AI Agents
In
backend/apps/owasp/migrations/0038_remove_projecthealthmetrics_is_project_leaders_requirements_compliant_and_more.py
around lines 11 to 22, the migration uses RemoveField followed by AddField which
causes existing data to be lost by resetting the column values to default. To
fix this, replace the RemoveField and AddField with a RenameField operation to
preserve existing data, or alternatively, add a RunPython data migration step
that copies values from the old field to the new field before removing the old
field.
| } | ||
| project_health_metrics = [] | ||
| for project in Project.objects.all(): | ||
| self.stdout.write(self.style.NOTICE(f"Evaluating metrics for project: {project.name}")) |
There was a problem hiding this comment.
self.style.NOTICE will raise AttributeError
Django’s color_style does not define a NOTICE style. Use one of the built-ins (SUCCESS, WARNING, ERROR, …) or output plain text.
- self.stdout.write(self.style.NOTICE(f"..."))
+ self.stdout.write(self.style.WARNING(f"..."))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| self.stdout.write(self.style.NOTICE(f"Evaluating metrics for project: {project.name}")) | |
| for project in projects: | |
| - self.stdout.write(self.style.NOTICE(f"Evaluating metrics for project: {project.name}")) | |
| + self.stdout.write(self.style.WARNING(f"Evaluating metrics for project: {project.name}")) | |
| metrics = ProjectHealthMetrics(project=project) | |
| # … rest of loop … |
🤖 Prompt for AI Agents
In backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
at line 35, replace the use of self.style.NOTICE with a valid style such as
self.style.SUCCESS, self.style.WARNING, or self.style.ERROR, or simply output
the message as plain text without styling to avoid the AttributeError caused by
the undefined NOTICE style.
| fields_weights = { | ||
| "age_days": (5, 6), | ||
| "contributors_count": (5, 6), | ||
| "forks_count": (5, 6), | ||
| "last_release_days": (5, 6), | ||
| "last_commit_days": (5, 6), | ||
| "open_issues_count": (5, 6), | ||
| "open_pull_requests_count": (5, 6), | ||
| "owasp_page_last_update_days": (5, 6), | ||
| "last_pull_request_days": (5, 6), | ||
| "recent_releases_count": (5, 6), | ||
| "stars_count": (5, 6), | ||
| "total_pull_requests_count": (5, 6), | ||
| "total_releases_count": (5, 6), | ||
| "unanswered_issues_count": (5, 6), | ||
| "unassigned_issues_count": (5, 6), | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
EXPECTED_SCORE is a magic-number that will break on any weight/logic change
The hard-coded 58.0 couples the test to the current implementation details.
Derive the expected value from fields_weights (and the command’s algorithm) inside the test instead, so future tuning of weights or formula won’t require chasing down failing tests.
🤖 Prompt for AI Agents
In
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
around lines 38 to 55, the test uses a hard-coded EXPECTED_SCORE value of 58.0
which tightly couples the test to the current weights and logic. To fix this,
remove the hard-coded EXPECTED_SCORE and instead calculate the expected score
dynamically within the test by applying the same algorithm used in the command
to the fields_weights dictionary. This ensures the test remains valid and does
not break when weights or scoring logic change.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
Show resolved
Hide resolved
| class ProjectHealthMetricsAdmin(admin.ModelAdmin): | ||
| autocomplete_fields = ("project",) | ||
| list_filter = ("project__level",) | ||
| list_display = ( | ||
| "project", | ||
| "score", | ||
| "contributors_count", | ||
| "stars_count", | ||
| "forks_count", | ||
| "open_issues_count", | ||
| "open_pull_requests_count", | ||
| "recent_releases_count", | ||
| ) | ||
| search_fields = ("project__name",) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid shadowing the model field & use @admin.display
Naming the helper the same as the FK (project) shadows the real relation in list_display.
Rename the helper for clarity and annotate with @admin.display (Django ≥3.2):
- list_display = (
- "project",
+ list_display = (
+ "project_name",
"score",
@@
- def project(self, obj):
- """Display project name."""
- return obj.project.name if obj.project else "N/A"
+ @admin.display(description="Project")
+ def project_name(self, obj):
+ return obj.project.name if obj.project_id else "N/A"This also removes an unnecessary DB hit; consider adding list_select_related = ("project",) for further optimisation.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class ProjectHealthMetricsAdmin(admin.ModelAdmin): | |
| autocomplete_fields = ("project",) | |
| list_filter = ("project__level",) | |
| list_display = ( | |
| "project", | |
| "score", | |
| "contributors_count", | |
| "stars_count", | |
| "forks_count", | |
| "open_issues_count", | |
| "open_pull_requests_count", | |
| "recent_releases_count", | |
| ) | |
| search_fields = ("project__name",) | |
| class ProjectHealthMetricsAdmin(admin.ModelAdmin): | |
| autocomplete_fields = ("project",) | |
| list_filter = ("project__level",) | |
| list_display = ( | |
| "project_name", | |
| "score", | |
| "contributors_count", | |
| "stars_count", | |
| "forks_count", | |
| "open_issues_count", | |
| "open_pull_requests_count", | |
| "recent_releases_count", | |
| ) | |
| search_fields = ("project__name",) | |
| @admin.display(description="Project") | |
| def project_name(self, obj): | |
| return obj.project.name if obj.project_id else "N/A" |
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 167-167: Too few public methods (1/2)
(R0903)
🤖 Prompt for AI Agents
In backend/apps/owasp/admin.py around lines 167 to 180, the method named
'project' in ProjectHealthMetricsAdmin shadows the actual foreign key field
'project' in list_display, causing confusion and potential issues. Rename this
helper method to a distinct name that does not conflict with the FK field,
decorate it with @admin.display for clarity and proper admin behavior, and add
list_select_related = ("project",) to the admin class to optimize database
queries by reducing unnecessary hits when accessing related project data.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
45-47:self.style.NOTICEis still invalid – command will crash
Django’scolor_style()doesn’t expose aNOTICEattribute; calling it raisesAttributeErrorat runtime.
Use one of the supported styles (SUCCESS,WARNING,ERROR,HTTP_INFO, etc.) or drop styling altogether.backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
11-11: Hard-coded EXPECTED_SCORE is brittle
The literal58.0couples the test to current weights; any future tuning will break the test.
Derive the value inside the test fromforward_fields/backward_fieldsso it stays in sync.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
[refactor] 9-9: Too few public methods (1/2)
(R0903)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
[refactor] 14-14: Too few public methods (1/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
60-61:Nonevalues will triggerTypeErrorin comparisons
<=againstNoneis invalid. Either guard withis not Nonechecks or coerce to a numeric sentinel before comparing.
| requirements = project_health_requirements[metric.project.level] | ||
| score = 0.0 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Un-guarded lookup may raise KeyError when requirements missing
project_health_requirements[metric.project.level] assumes that every level in metrics has a matching requirements entry.
If the DB is inconsistent the command will crash – the exact problem flagged in an earlier review.
Add dict.get() with fallback handling or a try/except to log and skip the metric.
🤖 Prompt for AI Agents
In
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
around lines 49 to 50, the code directly accesses project_health_requirements
with metric.project.level which can raise a KeyError if the level is missing.
Modify this to use dict.get() with a default fallback or wrap the access in a
try/except block to catch KeyError, log an appropriate warning, and skip
processing that metric to prevent the command from crashing.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (3)
68-71: Bulk save lacks error handling and countsThe earlier review suggested wrapping
bulk_savein try/except and emitting the number of rows saved. That feedback has not been addressed.- ProjectHealthMetrics.bulk_save(project_health_metrics, fields=["score"]) - self.stdout.write(self.style.SUCCESS("Updated projects health metrics score successfully.")) + try: + ProjectHealthMetrics.bulk_save(project_health_metrics, fields=["score"]) + self.stdout.write( + self.style.SUCCESS(f"Updated {len(project_health_metrics)} project health scores.") + ) + except Exception as exc: + self.stderr.write(self.style.ERROR(f"Failed to update scores: {exc}")) + raise
45-47:⚠️ Potential issue
self.style.NOTICEwill raiseAttributeError
BaseCommand.stylein Django exposesSUCCESS,ERROR,WARNING, etc. – it has noNOTICE. The command will crash at runtime.- self.stdout.write(self.style.NOTICE(...)) + self.stdout.write(self.style.WARNING(...))
49-50:⚠️ Potential issuePotential
KeyErrorwhen requirements for a level are missingDirect dictionary access assumes every
metric.project.levelexists inproject_health_requirements. A missing row will crash the command.- requirements = project_health_requirements[metric.project.level] + requirements = project_health_requirements.get(metric.project.level) + if requirements is None: + self.stderr.write( + self.style.ERROR( + f"No health requirements found for level {metric.project.level}; " + f"skipping {metric.project.name}" + ) + ) + continue
🧹 Nitpick comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
12-33: Consolidateforward_fields&backward_fieldsinto single mappingThe two hard-coded dicts differ only by “direction”. A single structure such as
metric_mapping = { "contributors_count": (6.0, 1), "age_days": (6.0, -1), ... }would (a) eliminate duplication, (b) make weight changes atomic, (c) permit simple iteration with a uniform comparison (
directionmultiplier trick).
This keeps configuration in one place and prevents future drift.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (4)
backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(10-112)bulk_save(100-108)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(9-57)backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
Command(9-45)handle(12-45)backend/apps/owasp/models/project.py (1)
bulk_save(336-344)
🪛 Pylint (3.3.7)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
[refactor] 9-9: Too few public methods (1/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
51-61: Boolean compliance fields still compare against requirements modelInside the
elifbranch we callgetattr(requirements, field)even though those boolean fields do not exist onProjectHealthRequirements. While currently masked by the precedingif, future refactors could re-order logic and introduce anAttributeError. A safer pattern is to separate boolean fields out of the shared loop entirely.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (2)
backend/tests/apps/owasp/models/project_health_metrics_test.py (1)
69-92: 🛠️ Refactor suggestionCoverage gap: new boolean defaults in
get_level_requirementsnot asserted
ProjectHealthRequirementsgainedis_funding_requirements_compliantandis_leader_requirements_compliant, but the corresponding assertions are missing intest_get_level_requirements.Add:
assert "is_funding_requirements_compliant" in defaults assert "is_leader_requirements_compliant" in defaultsand extend the
command_defaultscomparison loop so that mismatches are detected.backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py (1)
13-19: 🛠️ Refactor suggestionDouble-check consistency of the newly-added compliance flags
is_funding_requirements_compliantis set toTruefor all levels whileis_leader_requirements_compliantisFalseonly forINCUBATOR.
Is that intentional? In earlier discussions (issue #1540) “leadership & funding” were grouped; having asymmetric defaults can create false-positive compliance for funding at the lowest level or false-negative for leadership in ad-hoc levels not explicitly listed.If the intent is to require both flags to be
False(or inherit some graduated thresholds) forINCUBATOR, update the dictionary accordingly; otherwise add a doc-string / inline comment that clarifies the policy so future maintainers do not “fix” this perceived inconsistency.Also applies to: 34-40, 54-60, 74-80
♻️ Duplicate comments (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
45-47:⚠️ Potential issue
self.style.NOTICEwill raiseAttributeErrorin Django
Stylehas noNOTICEattribute in any supported Django version. The command will blow up as soon as it tries to print the message.- self.stdout.write( - self.style.NOTICE(f"Updating score for project: {metric.project.name}") - ) + self.stdout.write( + self.style.WARNING(f"Updating score for project: {metric.project.name}") + )backend/apps/owasp/admin.py (1)
167-188: 🛠️ Refactor suggestionMethod name shadows FK field & adds an N+1 query
Defining aproject()helper with the same name as theprojectfield overrides the FK inlist_display, is confusing, and forces an extra query per row. Rename the helper, use@admin.display, and enablelist_select_related:class ProjectHealthMetricsAdmin(admin.ModelAdmin): autocomplete_fields = ("project",) + list_select_related = ("project",) @@ - list_display = ( - "project", + list_display = ( + "project_name", @@ - def project(self, obj): - """Display project name.""" - return obj.project.name if obj.project else "N/A" + @admin.display(description="Project") + def project_name(self, obj): + return obj.project.name if obj.project_id else "N/A"🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 167-167: Too few public methods (1/2)
(R0903)
🧹 Nitpick comments (6)
backend/apps/owasp/models/project_health_requirements.py (1)
27-32: Nit: streamlineverbose_namewording and stay consistentAll other
BooleanFields (e.g. inProjectHealthMetrics) omit the leading “Is …”.
Consider the shorter variant for consistency and to avoid the double boolean-verb pattern (“Is … compliant” → “Funding requirements compliant”).-verbose_name="Is funding requirements compliant" +verbose_name="Funding requirements compliant" -verbose_name="Is leader requirements compliant" +verbose_name="Leader requirements compliant"Purely cosmetic – feel free to ignore if you prefer the current wording.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_requirements_test.py (1)
44-50: Parameterized arglevelis now unused – trim to avoid confusion
levelwas kept in the parametrization but the value is never referenced inside the test body after the command signature change. This can mislead future readers and pylint will flag it as an unused parameter.- @pytest.mark.parametrize( - ("level", "expected_output", "display_name"), + @pytest.mark.parametrize( + ("expected_output", "display_name"), [ - ("flagship", "Created health requirements for Flagship projects", "Flagship"), - ("incubator", "Created health requirements for Incubator projects", "Incubator"), + ("Created health requirements for Flagship projects", "Flagship"), + ("Created health requirements for Incubator projects", "Incubator"), ], ) - def test_handle_successful_creation(self, level, expected_output, display_name): + def test_handle_successful_creation(self, expected_output, display_name):This keeps the intent while dropping the dead parameter.
backend/tests/apps/owasp/models/project_health_metrics_test.py (1)
14-16: Minor determinism issue:timezone.now()evaluated at import time
expected_daysis computed when the module is imported, whileage_days,last_commit_days, … are calculated during test execution. If the suite runs across midnight UTC the test can become flaky.Simple fix: freeze time or capture
nowinside the test:-("age_days", (timezone.now() - FIXED_DATE).days), +( + "age_days", + lambda: (timezone.now() - TestProjectHealthMetricsModel.FIXED_DATE).days, +), ... # and in the test: expected_days = expected_days() if callable(expected_days) else expected_days assert getattr(metrics, field_name) == expected_daysOr use
freezegun/pytest-freezegunto freezetimezone.now()to a fixed point.backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
53-58: Boolean fields cast tointobscure intent & skipNonechecks
Casting booleans tointworks (True → 1,False → 0) but hides the business rule. Consider an explicit comparison for the two compliance flags and drop the cast for numeric fields. Also, if any metric/requirement isNone,int(None)will raiseTypeError.backend/apps/owasp/models/project_health_metrics.py (1)
71-102: Minor: repetitive date-difference properties could be DRYed up
All five properties share the same pattern. A tiny helper would reduce duplication:def _days_since(self, dt): return (timezone.now() - dt).days if dt else 0 age_days = property(lambda self: self._days_since(self.created_at)) last_commit_days = property(lambda self: self._days_since(self.last_committed_at)) ...Purely cosmetic; ignore if readability suffers.
backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py (1)
123-132: Use Django’s stdout helpers instead of bareManagement commands should leverage
self.stdout.write()(optionally withself.style.SUCCESS/NOTICE/ERROR) to respect verbosity flags and redirection.- print( - f"{'Created' if created else 'Updated'} health requirements for " - f"{level_name} projects" - ) + action = "Created" if created else "Updated" + self.stdout.write(self.style.SUCCESS(f"{action} health requirements for {level_name} projects"))This keeps command output consistent with other Django commands and plays nicely with
--verbosity& logging capture in tests.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py(5 hunks)backend/apps/owasp/migrations/0041_projecthealthrequirements_is_funding_requirements_compliant_and_more.py(1 hunks)backend/apps/owasp/migrations/0042_alter_projecthealthmetrics_score.py(1 hunks)backend/apps/owasp/models/project.py(7 hunks)backend/apps/owasp/models/project_health_metrics.py(4 hunks)backend/apps/owasp/models/project_health_requirements.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_requirements_test.py(1 hunks)backend/tests/apps/owasp/models/project_health_metrics_test.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/models/project.py
🧰 Additional context used
🧬 Code Graph Analysis (6)
backend/apps/owasp/migrations/0042_alter_projecthealthmetrics_score.py (1)
backend/apps/owasp/migrations/0041_projecthealthrequirements_is_funding_requirements_compliant_and_more.py (1)
Migration(6-26)
backend/apps/owasp/models/project_health_requirements.py (1)
backend/apps/owasp/models/project.py (2)
is_funding_requirements_compliant(146-148)is_leader_requirements_compliant(151-155)
backend/apps/owasp/migrations/0041_projecthealthrequirements_is_funding_requirements_compliant_and_more.py (1)
backend/apps/owasp/migrations/0042_alter_projecthealthmetrics_score.py (1)
Migration(7-27)
backend/apps/owasp/admin.py (1)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-113)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (4)
backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(10-113)bulk_save(105-113)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(9-63)backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py (2)
Command(9-132)handle(121-132)backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
Command(9-45)handle(12-45)
backend/apps/owasp/models/project_health_metrics.py (3)
backend/apps/common/models.py (2)
BulkSaveModel(8-30)TimestampedModel(33-40)backend/apps/owasp/models/project.py (5)
Meta(35-40)is_leader_requirements_compliant(151-155)pull_request_last_created_at(225-229)owasp_page_last_updated_at(206-208)bulk_save(334-342)backend/apps/owasp/admin.py (1)
project(186-188)
🪛 Pylint (3.3.7)
backend/apps/owasp/migrations/0042_alter_projecthealthmetrics_score.py
[refactor] 7-7: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0041_projecthealthrequirements_is_funding_requirements_compliant_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/admin.py
[refactor] 167-167: Too few public methods (1/2)
(R0903)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
[refactor] 9-9: Too few public methods (1/2)
(R0903)
backend/apps/owasp/models/project_health_metrics.py
[refactor] 13-13: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/owasp/migrations/0041_projecthealthrequirements_is_funding_requirements_compliant_and_more.py (1)
12-25: Adding non-nullable columns with a DB-level default may lock large tables
BooleanField(default=True)is translated intoALTER TABLE … ADD COLUMN … BOOL NOT NULL DEFAULT true;.
On sizable tables this causes a full rewrite and exclusive lock in most SQL engines (PostgreSQL ≤13, MySQL, …).If downtime is a concern, consider the two-step pattern:
- Add the column nullable, default
NULL.- Run a data migration to set existing rows to
True.- Alter the column to
NOT NULL DEFAULT True.Not critical for small datasets, but worth a double-check.
backend/apps/owasp/migrations/0042_alter_projecthealthmetrics_score.py (1)
16-25: LGTM – validation tightened correctlySwitching
scoreto a nullableFloatFieldwith range validators resolves the previous inability to represent “not yet scored” projects while guarding the 0-100 constraint. No concerns.
| self.mock_requirements.update_or_create.side_effect = CommandError(error_message) | ||
|
|
||
| with pytest.raises(CommandError, match=error_message) as exc_info: | ||
| call_command("owasp_update_project_health_requirements") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Mock only once or vary per call to assert correct branching
update_or_create is called for every project level in the command loop, yet the mock currently always returns the same object with the same get_level_display().
The command will therefore print identical messages for all levels and the test still passes.
To really assert that each level is processed, make side_effect return distinct mocks per call and check call_count:
mocks = []
for name in ["Flagship", "Incubator", "Lab", "Production", "Other"]:
m = MagicMock(spec=ProjectHealthRequirements)
m.get_level_display.return_value = name
mocks.append((m, True))
self.mock_requirements.update_or_create.side_effect = mocks
call_command("owasp_update_project_health_requirements")
assert self.mock_requirements.update_or_create.call_count == len(mocks)This catches regressions where some levels might be skipped.
🤖 Prompt for AI Agents
In
backend/tests/apps/owasp/management/commands/owasp_update_project_health_requirements_test.py
around lines 54 to 57, the mock for update_or_create always returns the same
object, causing the test to miss verifying that each project level is processed
distinctly. Fix this by setting the side_effect of update_or_create to a list of
distinct mocks, each with a unique get_level_display return value for every
project level. Then, after calling the command, assert that update_or_create was
called the expected number of times to ensure all levels are handled.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
45-47:self.style.NOTICEwill crash – Django doesn’t expose this styleExactly the same issue was raised earlier; unchanged code will still raise
AttributeErrorat runtime.
Use an existing style such asself.style.WARNINGor plainself.stdout.write(...).
🧹 Nitpick comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
12-33: Hard-coded forward/backward dicts are duplicative – use a single mapping with directionYou now maintain two near-identical dicts that differ only by “comparison direction”. A single structure:
metric_weights = { "age_days": (6.0, 1), "last_commit_days": (6.0, -1), ... }would remove duplication and make later tuning easier.
Refactor when convenient to keep logic DRY.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
[refactor] 9-9: Too few public methods (1/2)
(R0903)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
[refactor] 14-14: Too few public methods (1/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
35-43: PotentialKeyError: missing requirements for a level
requirements = project_health_requirements[metric.project.level]assumes the DB is consistent.
Use.get()with a graceful fallback (e.g., log & continue) to avoid aborting the entire job.backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
38-68: Test omits compliance fields – real command will raise/int-convert mocksThe command accesses
is_funding_requirements_compliant&is_leader_requirements_compliant, but the fixture never sets corresponding values onmock_requirements.
int(MagicMock())is undefined behaviour and may mask bugs.
Populate the boolean fields on both metric & requirement mocks to reflect real data.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
Show resolved
Hide resolved
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
Show resolved
Hide resolved
* Creating a command to update metrics - initial step * Add missing properties to the project model and update the metrics script * Apply make-check and check-spelling * Update tests * Apply coderabbitai suggestion * Add is_funding_requirements_compliant to the metrics script * Apply coderabbitai suggestion * Add owasp_page_last_updated_at to the metrics * Refactor is_funding_requirements_compliant logic * Add command to calculate score * Remove TODO * Add metrics test * Add metrics_score test * Update metrics test * Apply suggestions and add tests * Rename score script to be plural * Change the logic to create multiple ProjectHealthMetrics objects for a project and apply suggestions * Apply coderabbitai suggestion * Make open issues count negative direction * Evaluate the funding and project leaders requirement in the score and add tests * Apply suggestions * Apply coderabbitai suggestion * Apply coderabbitai suggetion * Fix tests * Update code * Apply suggestions * Fix if condition * Update code * Make older projects have higher scores --------- Co-authored-by: Kate Golovanova <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]>
4 participants
Resolves #1540