Implement GraphQL resolvers for project health metrics #1577
Conversation
Summary by CodeRabbit
WalkthroughA new GraphQL node, Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 11
🔭 Outside diff range comments (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
34-80: 🛠️ Refactor suggestionComputed score is prepared but never asserted
expected_scoreis calculated yet no assertion validates that the command sets it or thatbulk_savereceives it.Add:
self.mock_bulk_save.assert_called_once() assert mock_metric.score == expected_scoreto guarantee the core behaviour.
🧹 Nitpick comments (13)
cspell/custom-dict.txt (1)
30-30: Dictionary entry is OK – but keep the list alphabetically grouped.
ahmedxgoudafits, yet the custom dictionary is meant to remain alphabetically-sorted.
Please confirm the ordering rule (case-insensitive, ASCII sort) and move the token if necessary to avoid merge noise later.backend/tests/apps/owasp/graphql/nodes/project_test.py (1)
47-51: Solid field-existence test – consider guarding against non-list return types.
field.type.of_typeraisesAttributeErrorwhen the field is not a list.
To avoid false negatives you can defensively unwrap like:-assert field.type.of_type is ProjectHealthMetricsNode +inner_type = getattr(field.type, "of_type", field.type) +assert inner_type is ProjectHealthMetricsNodePurely optional, but it keeps the assertion failure cleaner if the schema changes.
backend/apps/owasp/migrations/0036_alter_projecthealthmetrics_score.py (1)
19-26: Consider usingDecimalFieldinstead ofFloatFieldfor monetary-like scoresFloats introduce rounding error and can behave inconsistently across DB back-ends. Because the value is bounded (0-100) and will likely be displayed/compared, a
DecimalField(max_digits=5, decimal_places=2)(or even anIntegerFieldrepresenting a percentage) gives exactness without extra effort.- field=models.FloatField( + field=models.DecimalField( + max_digits=5, + decimal_places=2, help_text="Project health score (0-100)", null=True, validators=[ django.core.validators.MinValueValidator(0.0), django.core.validators.MaxValueValidator(100.0), ], ),backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
45-68:iscomparison may give false negatives with Strawberry types
field.typeis often aForwardRef,StrawberryOptional, ortyping.Annotatedwrapper.
Identity checks withiscan therefore fail when the definition is rebuilt (e.g. after schema reload). Use==/typing.get_origin/strawberry.scalarshelpers instead:- assert field.type is expected_type + # Strawberry wraps built-ins; use equality instead of identity + assert field.type == expected_typeThis keeps the test robust across Strawberry upgrades.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
37-43: PotentialDoesNotExistcrash when requirements row missing
ProjectHealthRequirements.objects.get(level=metric.project.level)will raise and interrupt the whole batch if a row for that level hasn’t been seeded.
Wrap withtry/except ProjectHealthRequirements.DoesNotExist(and log/skip) or preload the requirements into a dict for O(1) lookups without N+1 queries.backend/Makefile (1)
106-113: Echo message and target naming are out of sync
owasp-update-project-health-metrics-scoresechoes “metrics score” (singular) while the target name and command both use the plural scores. Align the wording to avoid future confusion and log-grep mismatches.- @echo "Updating OWASP project health metrics score" + @echo "Updating OWASP project health metrics scores"backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
52-55:project_nameresolver breaks type safety & static analysis
self.projectisn’t defined on the dataclass wrapper, leading to the pylint warning you saw. Use the root object:- return self.project.name + return self._root.project.nameor expose
projectviaselect_relatedin queries and let clients pickproject { name }.🧰 Tools
🪛 Pylint (3.3.7)
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
backend/tests/apps/owasp/models/project_health_metrics_test.py (1)
100-120: Time-based tests can become flaky
timezone.now()is evaluated during test collection, whileage_daysetc. compute it during execution. A midnight rollover between collection and execution (rare but possible in CI) will makeexpected_daysoff-by-one.Freeze time or compute
expected_daysinside the test:expected_days = (timezone.now() - self.FIXED_DATE).daysor use
freezegun.backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py (1)
11-35: Couples tests to Strawberry internalsRelying on
__strawberry_definition__ties the spec to a private API and fails static checkers. Prefer Strawberry’s public helpers:from strawberry.utils.typing import get_type_definition definition = get_type_definition(ProjectHealthMetricsQuery)This keeps tests stable across library updates.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 16-16: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
[error] 24-24: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
backend/apps/owasp/graphql/queries/project_health_metrics.py (2)
63-67:order_by/distinct("project__key")combo may not guarantee “latest metrics”
distinct("project__key")keeps the first row per key — here the one with the alphabetically smallest key because oforder_by("project__key", …).
You presumably want the most recent metric (-nest_created_at,-score).
The PostgreSQL rule is: allorder_bycolumns must start with thedistinctcolumns; you can still keep descending dates by wrapping them in a sub-query or usingWindowexpressions.Consider:
sub = ( ProjectHealthMetrics.objects .order_by("project__key", "-nest_created_at") .distinct("project__key") )or switch to
Window+RowNumber()for clarity.
73-83: Minor: import & constant placementImporting
Qinside the resolver and re-computingrecent_periodon every call is harmless but a micro-inefficiency. Pull both to module level (alongside the other constants) to keep the hot path lean.backend/apps/owasp/models/project_health_metrics.py (1)
66-100:@propertystorm – consider@cached_propertyEach property fires its own DB-less calculation, but when exposed via GraphQL the same field can be accessed multiple times per object, multiplying work.
Replacing with
@cached_property(or Strawberry’s resolver-level caching) would compute once per instance.backend/apps/owasp/models/project.py (1)
188-192:leaders_rawcan beNone→TypeError
len(None)will explode. Defensive cast:- return len(self.leaders_raw) + return len(self.leaders_raw or [])
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
backend/Makefile(1 hunks)backend/apps/owasp/graphql/nodes/project.py(2 hunks)backend/apps/owasp/graphql/nodes/project_health_metrics.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(2 hunks)backend/apps/owasp/graphql/queries/project_health_metrics.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_metrics_scores.py(1 hunks)backend/apps/owasp/migrations/0035_rename_total_pull_request_count_projecthealthmetrics_total_pull_requests_count.py(1 hunks)backend/apps/owasp/migrations/0036_alter_projecthealthmetrics_score.py(1 hunks)backend/apps/owasp/migrations/0037_alter_projecthealthmetrics_project.py(1 hunks)backend/apps/owasp/models/project.py(8 hunks)backend/apps/owasp/models/project_health_metrics.py(3 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(2 hunks)backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/models/project_health_metrics_test.py(3 hunks)cspell/custom-dict.txt(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(24-55)backend/tests/apps/owasp/graphql/nodes/project_test.py (2)
test_meta_configuration(16-40)_get_field_by_name(42-45)
backend/tests/apps/owasp/graphql/nodes/project_test.py (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(24-55)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
_get_field_by_name(35-43)
backend/apps/owasp/graphql/queries/__init__.py (1)
backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
ProjectHealthMetricsQuery(19-84)
backend/apps/owasp/migrations/0037_alter_projecthealthmetrics_project.py (1)
backend/apps/owasp/migrations/0036_alter_projecthealthmetrics_score.py (1)
Migration(7-28)
backend/apps/owasp/migrations/0036_alter_projecthealthmetrics_score.py (1)
backend/apps/owasp/migrations/0037_alter_projecthealthmetrics_project.py (1)
Migration(7-22)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (3)
backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(10-114)bulk_save(102-110)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(9-57)backend/apps/owasp/models/project.py (1)
bulk_save(338-346)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-114)
backend/apps/owasp/models/project_health_metrics.py (4)
backend/apps/common/models.py (2)
BulkSaveModel(8-30)TimestampedModel(33-40)backend/apps/owasp/models/project.py (4)
Meta(39-44)pull_request_last_created_at(229-233)owasp_page_last_updated_at(208-212)bulk_save(338-346)backend/apps/owasp/models/project_health_requirements.py (1)
Meta(12-15)backend/apps/owasp/graphql/nodes/committee.py (1)
stars_count(40-42)
backend/apps/owasp/models/project.py (2)
backend/apps/owasp/graphql/nodes/project.py (3)
repositories(82-84)issues_count(45-47)repositories_count(87-89)backend/apps/github/models/repository.py (1)
published_releases(163-169)
🪛 Pylint (3.3.7)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
[error] 39-39: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
backend/apps/owasp/migrations/0035_rename_total_pull_request_count_projecthealthmetrics_total_pull_requests_count.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0037_alter_projecthealthmetrics_project.py
[refactor] 7-7: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0036_alter_projecthealthmetrics_score.py
[refactor] 7-7: Too few public methods (0/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/management/commands/owasp_update_project_health_metrics.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] 12-12: Too few public methods (1/2)
(R0903)
backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py
[error] 16-16: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
[error] 24-24: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
[refactor] 37-37: Too few public methods (1/2)
(R0903)
backend/apps/owasp/graphql/queries/project_health_metrics.py
[refactor] 23-23: Too many arguments (11/5)
(R0913)
[refactor] 23-23: Too many local variables (16/15)
(R0914)
[refactor] 19-19: Too few public methods (1/2)
(R0903)
backend/apps/owasp/graphql/nodes/project_health_metrics.py
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
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 frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (7)
backend/tests/apps/owasp/graphql/nodes/project_test.py (1)
9-10: Import looks good.The extra import is necessary for the new test and matches existing import style.
backend/apps/owasp/graphql/queries/__init__.py (1)
8-21:✅ Verification successful
Ensure new query mix-in does not clash with existing fields.
ProjectHealthMetricsQueryintroducesunhealthy_projects.
BecauseOwaspQueryuses multiple inheritance, duplicate field names silently override each other based on MRO. A quick grep shows no priorunhealthy_projects, but please double-check before merging.
🏁 Script executed:
#!/bin/bash # Detect duplicate strawberry field names among all *Query classes rg --no-heading --line-number -A1 '@strawberry\.field' backend/apps/owasp/graphql/queries \ | awk '/@strawberry\.field/{getline; sub(/^.*def[ \t]+/, "", $0); sub(/\(.*/, "", $0); print}' \ | sort | uniq -dLength of output: 203
No duplicate Strawberry GraphQL fields detected across query mix-ins
I ran a check across
backend/apps/owasp/graphql/queriesfor duplicate@strawberry.field–decorated methods and confirmed that no existing field name clashes withunhealthy_projects.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 14-14: Too many ancestors (9/7)
(R0901)
backend/apps/owasp/migrations/0035_rename_total_pull_request_count_projecthealthmetrics_total_pull_requests_count.py (1)
11-17:✅ Verification successful
Migration renames the column – check for stale references.
The rename is correct, but stray usages of
total_pull_request_countwill now explode at runtime.
Please run a quick search to ensure no lingering references exist.
🏁 Script executed:
#!/bin/bash # Fail if the old field name is still referenced in source/tests rg -n 'total_pull_request_count' --glob '!*/migrations/*'Length of output: 509
No stale references to
total_pull_request_countoutside migrations
A ripgrep search (excludingmigrations/) returned no occurrences of the old field name in source or tests. The only hits are within migration files (as expected). You can safely merge this migration.backend/apps/owasp/migrations/0037_alter_projecthealthmetrics_project.py (1)
16-19: Verify data integrity when downgrading fromOneToOneFieldtoForeignKeyChanging to a FK will allow multiple
ProjectHealthMetricsrows per project.
If historical data relied on the uniqueness guarantee, duplicate rows could now emerge (e.g. the new update-metrics command creates a fresh record every run).
Please double-check that either
- the existing table is empty, or
- a UNIQUE constraint (
UniqueConstraint("project", "created_at")?) or anexcludeclause is added, or- the update command is idempotent (uses
update_or_create).
Otherwise the table may grow unbounded and GraphQL might return confusing duplicates.backend/Makefile (1)
110-113: Missing phony dependency in composite workflowsUnlike
owasp-update-project-health-metrics, this new target isn’t referenced from any composite recipes (update-data,sync-data, etc.). Confirm that the target is intentionally manual-only; otherwise add it to the appropriate higher-level rule so scores stay in sync with metrics.backend/tests/apps/owasp/models/project_health_metrics_test.py (1)
39-61: Inconsistent validation rule forscore = NoneDefault
scoreisNone, yet explicitly settingscore=Noneis treated as invalid intest_score_validation. Either:
- Allow
Nonewhen explicitly assigned, or- Change default to a sentinel (e.g.,
0.0) and enforce non-null.Current mixture suggests a hidden bug or unclear requirement.
backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py (1)
40-61: Manager patch only coversselect_relatedbut not.all()The resolver ends with
.all(); ensure it’s stubbed to avoid hitting the real DB:mocked_objects.select_related.return_value.order_by.return_value.all.return_value = [mock_node]Without this the call chain will raise
AttributeError.
| for project in projects: | ||
| self.stdout.write(self.style.NOTICE(f"Evaluating metrics for project: {project.name}")) | ||
| metrics = ProjectHealthMetrics(project=project) | ||
|
|
||
| # Update metrics based on requirements | ||
| for metric_field, project_field in field_mappings.items(): | ||
| setattr(metrics, metric_field, getattr(project, project_field)) | ||
|
|
||
| to_save.append(metrics) | ||
|
|
||
| ProjectHealthMetrics.bulk_save(to_save) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Idempotency / duplication risk
Each run creates a brand-new ProjectHealthMetrics instance, leading to unlimited rows per project.
Use update_or_create or fetch the latest metrics and update in-place, or add a uniqueness constraint as mentioned in the migration review.
- metrics = ProjectHealthMetrics(project=project)
+ metrics, _ = ProjectHealthMetrics.objects.get_or_create(project=project)📝 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.
| for project in projects: | |
| self.stdout.write(self.style.NOTICE(f"Evaluating metrics for project: {project.name}")) | |
| metrics = ProjectHealthMetrics(project=project) | |
| # Update metrics based on requirements | |
| for metric_field, project_field in field_mappings.items(): | |
| setattr(metrics, metric_field, getattr(project, project_field)) | |
| to_save.append(metrics) | |
| ProjectHealthMetrics.bulk_save(to_save) | |
| for project in projects: | |
| self.stdout.write(self.style.NOTICE(f"Evaluating metrics for project: {project.name}")) | |
| - metrics = ProjectHealthMetrics(project=project) | |
| + metrics, _ = ProjectHealthMetrics.objects.get_or_create(project=project) | |
| # Update metrics based on requirements | |
| for metric_field, project_field in field_mappings.items(): | |
| setattr(metrics, metric_field, getattr(project, project_field)) | |
| to_save.append(metrics) | |
| ProjectHealthMetrics.bulk_save(to_save) |
🤖 Prompt for AI Agents
In backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
around lines 35 to 45, the code creates new ProjectHealthMetrics instances for
each project every run, causing duplicate rows. Modify the logic to use
update_or_create or fetch the existing ProjectHealthMetrics for each project and
update its fields in place before saving. This ensures only one metrics record
per project and prevents duplication.
| field_mappings = { | ||
| "contributors_count": "contributors_count", | ||
| "created_at": "created_at", | ||
| "forks_count": "forks_count", | ||
| "is_funding_requirements_compliant": "is_funding_requirements_compliant", | ||
| "is_project_leaders_requirements_compliant": "is_leaders_requirements_compliant", | ||
| "last_released_at": "released_at", | ||
| "last_committed_at": "pushed_at", | ||
| "open_issues_count": "open_issues_count", | ||
| "open_pull_requests_count": "open_pull_requests_count", | ||
| "owasp_page_last_updated_at": "owasp_page_last_updated_at", | ||
| "pull_request_last_created_at": "pull_request_last_created_at", | ||
| "recent_releases_count": "recent_releases_count", | ||
| "stars_count": "stars_count", | ||
| "total_issues_count": "issues_count", | ||
| "total_pull_requests_count": "pull_requests_count", | ||
| "total_releases_count": "releases_count", | ||
| "unanswered_issues_count": "unanswered_issues_count", | ||
| "unassigned_issues_count": "unassigned_issues_count", | ||
| } |
There was a problem hiding this comment.
Broken attribute mapping – command will raise AttributeError
is_leaders_requirements_compliant, released_at, and pushed_at do not exist on the Project model (the correct names are is_project_leaders_requirements_compliant, last_released_at, last_committed_at).
- "is_project_leaders_requirements_compliant": "is_leaders_requirements_compliant",
- "last_released_at": "released_at",
- "last_committed_at": "pushed_at",
+ "is_project_leaders_requirements_compliant": "is_project_leaders_requirements_compliant",
+ "last_released_at": "last_released_at",
+ "last_committed_at": "last_committed_at",Failing to fix this will abort the command on the first project processed.
🤖 Prompt for AI Agents
In backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
between lines 14 and 33, the field_mappings dictionary incorrectly maps some
keys to non-existent Project model attributes: replace
"is_leaders_requirements_compliant" with
"is_project_leaders_requirements_compliant", "released_at" with
"last_released_at", and "pushed_at" with "last_committed_at" to match the actual
model attribute names and prevent AttributeError during command execution.
| weight_mapping = { | ||
| "age_days": (6.0, -1), | ||
| "contributors_count": (6.0, 1), | ||
| "forks_count": (6.0, 1), | ||
| "last_release_days": (6.0, -1), | ||
| "last_commit_days": (6.0, -1), | ||
| "open_issues_count": (6.0, 1), | ||
| "open_pull_requests_count": (6.0, 1), | ||
| "owasp_page_last_update_days": (6.0, -1), | ||
| "last_pull_request_days": (6.0, -1), | ||
| "recent_releases_count": (6.0, 1), | ||
| "stars_count": (8.0, 1), | ||
| "total_pull_requests_count": (8.0, 1), | ||
| "total_releases_count": (8.0, 1), | ||
| "unanswered_issues_count": (8.0, -1), | ||
| "unassigned_issues_count": (8.0, -1), | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Direction flags for “open” counts look inverted
Higher numbers of open_issues_count and open_pull_requests_count generally indicate worse health.
Their direction should therefore be -1, not 1, otherwise projects with many unresolved items receive a higher score.
- "open_issues_count": (6.0, 1),
- "open_pull_requests_count": (6.0, 1),
+ "open_issues_count": (6.0, -1),
+ "open_pull_requests_count": (6.0, -1),Correct direction for open-item metrics
Higher counts of open issues or pull requests indicate poorer project health, so their direction flags should be negative.
- File:
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
• Update the two entries inweight_mapping:
- "open_issues_count": (6.0, 1),
- "open_pull_requests_count": (6.0, 1),
+ "open_issues_count": (6.0, -1),
+ "open_pull_requests_count": (6.0, -1),📝 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.
| weight_mapping = { | |
| "age_days": (6.0, -1), | |
| "contributors_count": (6.0, 1), | |
| "forks_count": (6.0, 1), | |
| "last_release_days": (6.0, -1), | |
| "last_commit_days": (6.0, -1), | |
| "open_issues_count": (6.0, 1), | |
| "open_pull_requests_count": (6.0, 1), | |
| "owasp_page_last_update_days": (6.0, -1), | |
| "last_pull_request_days": (6.0, -1), | |
| "recent_releases_count": (6.0, 1), | |
| "stars_count": (8.0, 1), | |
| "total_pull_requests_count": (8.0, 1), | |
| "total_releases_count": (8.0, 1), | |
| "unanswered_issues_count": (8.0, -1), | |
| "unassigned_issues_count": (8.0, -1), | |
| } | |
| weight_mapping = { | |
| "age_days": (6.0, -1), | |
| "contributors_count": (6.0, 1), | |
| "forks_count": (6.0, 1), | |
| "last_release_days": (6.0, -1), | |
| "last_commit_days": (6.0, -1), | |
| "open_issues_count": (6.0, -1), | |
| "open_pull_requests_count": (6.0, -1), | |
| "owasp_page_last_update_days": (6.0, -1), | |
| "last_pull_request_days": (6.0, -1), | |
| "recent_releases_count": (6.0, 1), | |
| "stars_count": (8.0, 1), | |
| "total_pull_requests_count": (8.0, 1), | |
| "total_releases_count": (8.0, 1), | |
| "unanswered_issues_count": (8.0, -1), | |
| "unassigned_issues_count": (8.0, -1), | |
| } |
🤖 Prompt for AI Agents
In
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
around lines 14 to 30, the direction flags for "open_issues_count" and
"open_pull_requests_count" in the weight_mapping dictionary are incorrectly set
to 1, which implies higher counts improve health. Change their direction values
from 1 to -1 to correctly reflect that higher counts of open issues and pull
requests indicate worse project health.
| @strawberry.field | ||
| def age_days(self) -> int: | ||
| """Resolve project age in days.""" | ||
| return self.age_days | ||
|
|
||
| @strawberry.field | ||
| def last_commit_days(self) -> int: | ||
| """Resolve last commit age in days.""" | ||
| return self.last_commit_days | ||
|
|
||
| @strawberry.field | ||
| def last_pull_request_days(self) -> int: | ||
| """Resolve last pull request age in days.""" | ||
| return self.last_pull_request_days | ||
|
|
||
| @strawberry.field | ||
| def last_release_days(self) -> int: | ||
| """Resolve last release age in days.""" | ||
| return self.last_release_days | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Infinite recursion in all resolver methods
Each resolver returns the same attribute name it is defining, causing a self-referential lookup that yields the bound method instead of the computed value and eventually raises RecursionError at runtime.
- def age_days(self) -> int:
- return self.age_days
+ def age_days(self) -> int:
+ # `self` is a dataclass proxy to the underlying Django instance
+ return self._root.age_days # `_root` is provided by strawberry-djangoApply the same pattern (or self._root.<prop>) to last_commit_days, last_pull_request_days, last_release_days, and owasp_page_last_update_days.
Alternatively, expose these fields via fields=[…] and drop custom resolvers entirely.
🤖 Prompt for AI Agents
In backend/apps/owasp/graphql/nodes/project_health_metrics.py around lines 27 to
46, the resolver methods for age_days, last_commit_days, last_pull_request_days,
and last_release_days cause infinite recursion by returning the same attribute
name they define. To fix this, change each resolver to return the corresponding
value from the underlying data source, such as using self._root.age_days,
self._root.last_commit_days, self._root.last_pull_request_days, and
self._root.last_release_days instead of returning self.age_days, etc.
Alternatively, remove these custom resolvers and expose these fields directly
via the fields parameter.
There was a problem hiding this comment.
@ahmedxgouda have you checked if this suggestion is valid?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
No, it is not. Even strawberry type does not have this attribute.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py
Outdated
Show resolved
Hide resolved
| @property | ||
| def age_days(self) -> int: | ||
| """Calculate project age in days.""" | ||
| if self.created_at: | ||
| return (timezone.now() - self.created_at).days | ||
| return 0 | ||
|
|
||
| @property | ||
| def last_commit_days(self) -> int: | ||
| """Calculate days since last commit.""" | ||
| if self.last_committed_at: | ||
| return (timezone.now() - self.last_committed_at).days | ||
| return 0 | ||
|
|
||
| @property | ||
| def last_pull_request_days(self) -> int: | ||
| """Calculate days since last pull request.""" | ||
| if self.pull_request_last_created_at: | ||
| return (timezone.now() - self.pull_request_last_created_at).days | ||
| return 0 | ||
|
|
||
| @property | ||
| def last_release_days(self) -> int: | ||
| """Calculate days since last release.""" | ||
| if self.last_released_at: | ||
| return (timezone.now() - self.last_released_at).days | ||
| return 0 | ||
|
|
||
| @property | ||
| def owasp_page_last_update_days(self) -> int: | ||
| """Calculate days since OWASP page last update.""" | ||
| if self.owasp_page_last_updated_at: | ||
| return (timezone.now() - self.owasp_page_last_updated_at).days | ||
| return 0 | ||
|
|
There was a problem hiding this comment.
Timezone-aware arithmetic may explode on naïve datetimes
timezone.now() returns an aware datetime; if created_at, last_committed_at, … are naïve (e.g. populated from GitHub data without tzinfo), subtraction raises TypeError.
Guard by normalising:
- return (timezone.now() - self.created_at).days
+ created = timezone.make_aware(self.created_at, timezone.utc) if timezone.is_naive(self.created_at) else self.created_at
+ return (timezone.now() - created).daysRepeat for the other date-difference properties or factor into a helper.
📝 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.
| @property | |
| def age_days(self) -> int: | |
| """Calculate project age in days.""" | |
| if self.created_at: | |
| return (timezone.now() - self.created_at).days | |
| return 0 | |
| @property | |
| def last_commit_days(self) -> int: | |
| """Calculate days since last commit.""" | |
| if self.last_committed_at: | |
| return (timezone.now() - self.last_committed_at).days | |
| return 0 | |
| @property | |
| def last_pull_request_days(self) -> int: | |
| """Calculate days since last pull request.""" | |
| if self.pull_request_last_created_at: | |
| return (timezone.now() - self.pull_request_last_created_at).days | |
| return 0 | |
| @property | |
| def last_release_days(self) -> int: | |
| """Calculate days since last release.""" | |
| if self.last_released_at: | |
| return (timezone.now() - self.last_released_at).days | |
| return 0 | |
| @property | |
| def owasp_page_last_update_days(self) -> int: | |
| """Calculate days since OWASP page last update.""" | |
| if self.owasp_page_last_updated_at: | |
| return (timezone.now() - self.owasp_page_last_updated_at).days | |
| return 0 | |
| @property | |
| def age_days(self) -> int: | |
| """Calculate project age in days.""" | |
| if self.created_at: | |
| created = ( | |
| timezone.make_aware(self.created_at, timezone.utc) | |
| if timezone.is_naive(self.created_at) | |
| else self.created_at | |
| ) | |
| return (timezone.now() - created).days | |
| return 0 |
🤖 Prompt for AI Agents
In backend/apps/owasp/models/project_health_metrics.py around lines 66 to 100,
the date difference properties use timezone.now() which returns an aware
datetime, but the stored datetime fields like created_at or last_committed_at
may be naïve, causing TypeError on subtraction. Fix this by normalizing these
datetime fields to be timezone-aware before subtraction, either by localizing
naïve datetimes to the correct timezone or converting them to aware datetimes.
Apply this normalization consistently to all such properties or refactor the
logic into a helper method to avoid repetition.
backend/apps/owasp/models/project.py
Outdated
| @property | ||
| def is_funding_requirements_compliant(self) -> bool: | ||
| """Indicate whether project is compliant with funding requirements.""" | ||
| return not self.repositories.filter(is_funding_policy_compliant=False).exists() | ||
|
|
||
| @property | ||
| def is_leaders_requirements_compliant(self) -> bool: | ||
| """Indicate whether project is compliant with project leaders requirements.""" | ||
| return self.leaders_count >= MINIMUM_LEADERS | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Heavy per-property queries – cache or pre-annotate
Properties like issues_count, open_pull_requests_count, recent_releases_count, unanswered_issues_count, etc. each trigger an extra query. Rendering a list of 50 projects will execute dozens of queries per row.
Options:
- Denormalise into
ProjectHealthMetrics(seems partially done already). - Use
@cached_propertyso the query runs once per request. - In list resolvers, fetch via
.annotate(...)and read from instance attributes.
This will keep GraphQL response times predictable under load.
Also applies to: 188-277
🤖 Prompt for AI Agents
In backend/apps/owasp/models/project.py between lines 149 and 158, the
properties like is_funding_requirements_compliant and others trigger separate
database queries each time they are accessed, causing performance issues when
rendering multiple projects. To fix this, refactor these properties to use
@cached_property to cache the query result per instance or modify the list
resolvers to use .annotate(...) to prefetch and store these counts as attributes
on the Project instances. Alternatively, consider denormalizing these metrics
into a related ProjectHealthMetrics model to reduce query overhead. Apply
similar changes to the properties and methods between lines 188 and 277 as well.
ahmedxgouda
force-pushed
the
dashboard/graphql-health-queries
branch
from
61cf84b to
0692934
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (6)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
27-45:⚠️ Potential issueInfinite recursion & shadowing of dataclass fields – resolvers will crash at runtime
Each resolver (
age_days,last_commit_days,last_pull_request_days,last_release_days,owasp_page_last_update_days) returns the same attribute it is defining.
Because the dataclass already has attributes with those names (inherited from the model), the method definition shadows the attribute, soself.age_daysnow points back to the bound method – leading to an endless self-call andRecursionErroron the first query.Apply the following patch (pattern repeated for the other four resolvers) to read from the underlying Django instance instead:
- @strawberry.field - def age_days(self) -> int: - """Resolve project age in days.""" - return self.age_days + @strawberry.field + def age_days(self) -> int: + """Resolve project age in days.""" + return self._root.age_days…and analogously use
self._root.last_commit_days, etc.Failing to fix this will break every GraphQL request using this node.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
20-23:⚠️ Potential issueBroken attribute mappings – command will raise
AttributeError
is_leaders_requirements_compliant,released_at, andpushed_atare not present on theProjectmodel (they were renamed). Use the correct attribute names:- "is_project_leaders_requirements_compliant": "is_leaders_requirements_compliant", - "last_released_at": "released_at", - "last_committed_at": "pushed_at", + "is_project_leaders_requirements_compliant": "is_project_leaders_requirements_compliant", + "last_released_at": "last_released_at", + "last_committed_at": "last_committed_at",This bug will abort the command on the very first project processed.
36-46: 🛠️ Refactor suggestionIdempotency issue – duplicates every run
A fresh
ProjectHealthMetricsinstance is created on each execution, leading to unbounded duplicate rows per project. Reuse or upsert instead:- metrics = ProjectHealthMetrics(project=project) + metrics, _ = ProjectHealthMetrics.objects.get_or_create(project=project)You can then update fields in-place and use
bulk_savewithfields=[…]to avoid duplication.backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
21-29: 🛠️ Refactor suggestionPatching only
.filterdoes not stub the full ORM chainThe command under test calls
ProjectHealthMetrics.objects.select_related("project").filter(...).
Patching.objects.filterleaves the precedingselect_related()untouched, so a real DB queryset may still execute. Patch the manager instead:- patch( - "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.objects.filter" - ) as metrics_patch, + patch( + "apps.owasp.models.project_health_metrics.ProjectHealthMetrics.objects" + ) as metrics_manager_patch,Then in the fixture:
self.mock_metrics = metrics_manager_patch.select_related.return_value.filterPrevents accidental database access during tests.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
20-23:⚠️ Potential issue
open_pull_requests_countdirection must be-1, not1– same issue as previously flaggedHigher counts of open PRs indicate worse health, so the direction flag should be negative.
This was pointed out in an earlier review but is still unresolved.- "open_pull_requests_count": (6.0, 1), + "open_pull_requests_count": (6.0, -1),backend/apps/owasp/models/project_health_metrics.py (1)
78-112:timezone.now()subtraction may crash with naïve datetimes – previously reportedIf any of the stored datetimes are naïve (
tzinfo=None), subtracting from an awaretimezone.now()raisesTypeError.
Normalise before arithmetic or store everything as aware datetimes.- if self.created_at: - return (timezone.now() - self.created_at).days + if self.created_at: + created = ( + timezone.make_aware(self.created_at, timezone.utc) + if timezone.is_naive(self.created_at) + else self.created_at + ) + return (timezone.now() - created).daysApply the same fix to the other date-difference properties or extract a helper.
🧹 Nitpick comments (1)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
16-33:project_namemissing fromexpected_field_namessetThe parametrised test later asserts the presence & type of
project_name, yet it is not included inexpected_field_names.
Add it to keep the two tests consistent:expected_field_names = { "age_days", + "project_name",Without this, the meta-configuration test still passes (since subset) but the intent of enumerating the public schema is defeated.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
backend/apps/owasp/graphql/nodes/project.py(2 hunks)backend/apps/owasp/graphql/nodes/project_health_metrics.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(2 hunks)backend/apps/owasp/graphql/queries/project_health_metrics.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_metrics_scores.py(1 hunks)backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py(1 hunks)backend/apps/owasp/models/project.py(8 hunks)backend/apps/owasp/models/project_health_metrics.py(4 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/tests/apps/owasp/graphql/nodes/project_test.py
- backend/apps/owasp/graphql/queries/init.py
- backend/apps/owasp/graphql/nodes/project.py
- backend/apps/owasp/models/project.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
backend/apps/owasp/models/project_health_metrics.py (4)
backend/apps/common/models.py (2)
BulkSaveModel(8-30)TimestampedModel(33-40)backend/apps/owasp/models/project.py (5)
Meta(39-44)has_no_recent_commits(256-259)pull_request_last_created_at(229-233)owasp_page_last_updated_at(208-212)bulk_save(344-352)backend/apps/owasp/models/project_health_requirements.py (1)
Meta(12-15)backend/apps/owasp/graphql/nodes/committee.py (1)
stars_count(40-42)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-126)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (3)
backend/apps/owasp/models/project_health_metrics.py (2)
ProjectHealthMetrics(10-126)bulk_save(114-122)backend/apps/owasp/models/project_health_requirements.py (1)
ProjectHealthRequirements(9-57)backend/apps/owasp/models/project.py (2)
is_funding_requirements_compliant(150-152)bulk_save(344-352)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(24-55)backend/tests/apps/owasp/graphql/nodes/project_test.py (2)
test_meta_configuration(16-40)_get_field_by_name(42-45)
🪛 Pylint (3.3.7)
backend/apps/owasp/models/project_health_metrics.py
[refactor] 13-13: Too few public methods (0/2)
(R0903)
backend/apps/owasp/graphql/nodes/project_health_metrics.py
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
backend/apps/owasp/graphql/queries/project_health_metrics.py
[refactor] 17-17: Too many arguments (11/5)
(R0913)
[refactor] 13-13: 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_projecthealthmetrics_has_long_open_issues_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
[error] 39-39: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
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 (3)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
65-67: Potential false negative on type assertion
field.typein Strawberry is the GraphQL-side type; for scalars that’s usually the Python builtin, but wrappers (e.g.strawberry.auto,typing.Optional[int], etc.) change the value.
If any schema evolution wraps these fields the test will fail spuriously. Consider asserting againstfield.type.annotationor usingissubclass(field.type, expected_type)instead.backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py (1)
11-32: Migration OK – fields added with sensible defaultsBoolean fields are added with
default=False, no data migrations required. Looks good.backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
75-76: Typo in ordering field – should benest_created_at?
TimestampedModelprovidesnest_created_at; confirm the spelling or queries will error out.
| @strawberry.field | ||
| def owasp_page_last_update_days(self) -> int: | ||
| """Resolve OWASP page last update age in days.""" | ||
| return self.owasp_page_last_update_days | ||
|
|
||
| @strawberry.field | ||
| def project_name(self) -> str: | ||
| """Resolve project node.""" | ||
| return self.project.name |
There was a problem hiding this comment.
project_name resolver accesses a non-existent attribute
self.project is not part of the exposed fields=[…] list, so it is not copied onto the dataclass instance. Accessing it will raise AttributeError.
Use the root model instead:
- def project_name(self) -> str:
- """Resolve project node."""
- return self.project.name
+ def project_name(self) -> str:
+ """Resolve project name."""
+ return self._root.project.name📝 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.
| @strawberry.field | |
| def owasp_page_last_update_days(self) -> int: | |
| """Resolve OWASP page last update age in days.""" | |
| return self.owasp_page_last_update_days | |
| @strawberry.field | |
| def project_name(self) -> str: | |
| """Resolve project node.""" | |
| return self.project.name | |
| @strawberry.field | |
| def owasp_page_last_update_days(self) -> int: | |
| """Resolve OWASP page last update age in days.""" | |
| return self.owasp_page_last_update_days | |
| @strawberry.field | |
| def project_name(self) -> str: | |
| """Resolve project name.""" | |
| return self._root.project.name |
🧰 Tools
🪛 Pylint (3.3.7)
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
🤖 Prompt for AI Agents
In backend/apps/owasp/graphql/nodes/project_health_metrics.py around lines 47 to
55, the project_name resolver tries to access self.project which is not included
in the dataclass fields and thus not available, causing an AttributeError. To
fix this, modify the resolver to access the project name via the root model
object that contains the original data, ensuring the attribute exists and can be
accessed safely.
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: 1
♻️ Duplicate comments (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
20-22:⚠️ Potential issue
open_pull_requests_countshould penalise, not reward, high numbersA large number of open PRs signals worse project health, yet the
directionflag is set to1, awarding points instead of deducting them.
Swap the sign to-1to align with the other “open / unanswered / unassigned” counters.- "open_pull_requests_count": (6.0, 1), + "open_pull_requests_count": (6.0, -1),backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
20-29: Still patching a single method – fragile against future ORM chainingThe fixture mocks
ProjectHealthMetrics.objects.filter, which is fine now but will break as soon as the command addsselect_related,order_by, etc.
Patch the manager itself and stub the whole chain to make the test resilient.
🧹 Nitpick comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
13-14: Prefetchprojectto avoid per-row look-upsThe scoring accesses
metric.project.*, which currently triggers an extra query for every metric.
Addselect_related("project")when loadingmetrics.- metrics = ProjectHealthMetrics.objects.filter(score__isnull=True) + metrics = ( + ProjectHealthMetrics.objects + .select_related("project") + .filter(score__isnull=True) + )
📜 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 backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
56-68: Expected score will change after fixing the PR-direction bugAdjusting the
directionforopen_pull_requests_countto-1(see command review) increases the computed score by+6.
Update the constant to keep the test green:-EXPECTED_SCORE = 40.0 +EXPECTED_SCORE = 46.0
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
Outdated
Show resolved
Hide resolved
ahmedxgouda
force-pushed
the
dashboard/graphql-health-queries
branch
from
d01cacd to
c73f29c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (10)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (2)
52-55:project_nameaccesses a non-existent attribute
self.projectis not part of the dataclass fields copied by Strawberry-Django, so this will raiseAttributeError.- def project_name(self) -> str: - """Resolve project node.""" - return self.project.name + def project_name(self) -> str: + """Resolve project name.""" + return self._root.project.name🧰 Tools
🪛 Pylint (3.3.7)
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
27-45:⚠️ Potential issueInfinite recursion – each resolver calls itself
return self.age_days(and the other similar returns) will recursively invoke the very same resolver, blowing the stack at query time.- def age_days(self) -> int: - """Resolve project age in days.""" - return self.age_days + def age_days(self) -> int: + """Resolve project age in days.""" + # `self._root` is the underlying `ProjectHealthMetrics` instance + return self._root.age_daysApply the same pattern to
last_commit_days,last_pull_request_days,last_release_days, andowasp_page_last_update_days.backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
18-23: Broken field mappings →AttributeError& wrong data
is_project_leaders_requirements_compliant,last_released_at, andlast_committed_atare mapped to attributes that do not exist on theProjectmodel (is_leaders_requirements_compliant,released_at,pushed_at).
The command will crash on the first project.- "is_project_leaders_requirements_compliant": "is_leaders_requirements_compliant", - "last_released_at": "released_at", - "last_committed_at": "pushed_at", + "is_project_leaders_requirements_compliant": "is_project_leaders_requirements_compliant", + "last_released_at": "last_released_at", + "last_committed_at": "last_committed_at",
36-44: Unbounded growth – duplicate metrics each runThe command creates a fresh
ProjectHealthMetricsrow every execution.
Useupdate_or_create(or fetch the latest record) to keep one metrics row per project.- metrics = ProjectHealthMetrics(project=project) + metrics, _ = ProjectHealthMetrics.objects.get_or_create(project=project)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
21-29: Still patching thefilterattribute instead of the whole managerPrior feedback pointed out that patching
ProjectHealthMetrics.objects.filterdoes not intercept the precedingselect_related(…)call, leaving a DB-hitting manager in place.
Patch the manager itself and stub the complete chain (select_related → filter) to guarantee the ORM never touches the database during the test.backend/apps/owasp/graphql/queries/project_health_metrics.py (3)
25-30:no_recent_releasesshould follow the tri-state conventionAll other boolean filters accept
None(no filtering).no_recent_releasesstill defaults toFalse, so callers cannot explicitly ask for projects with recent releases.- no_recent_releases: bool = False, + no_recent_releases: bool | None = None,
48-50: Boolean argument ignored – always filters for non-compliancePassing
contributors_count_requirement_compliant=Truestill applies__lt, returning the opposite set.- if contributors_count_requirement_compliant is not None: - filters["contributors_count__lt"] = CONTRIBUTORS_COUNT_REQUIREMENT + if contributors_count_requirement_compliant is not None: + op = "lt" if not contributors_count_requirement_compliant else "gte" + filters[f"contributors_count__{op}"] = CONTRIBUTORS_COUNT_REQUIREMENT
45-47: 🛠️ Refactor suggestionFilter only when
no_recent_releases is True– unexpected asymmetryCoupled with the default
False, the current condition makes the parameter effectively a one-way switch. Align with other filters:-if no_recent_releases: - filters["recent_releases_count"] = 0 +if no_recent_releases is not None: + filters["recent_releases_count"] = 0 if no_recent_releases else 1backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
14-22:open_pull_requests_counttreated as a “higher is better” metricA large number of open PRs normally signals poor project health, yet it sits in
forward_fields, awarding points when the count exceeds the requirement.- "open_pull_requests_count", - "recent_releases_count", + "recent_releases_count",- "open_pull_requests_count", + "open_pull_requests_count",Move
open_pull_requests_counttobackward_fields(score when<=requirement).backend/apps/owasp/models/project_health_metrics.py (1)
78-109: Naïve vs. aware datetime arithmetic may raiseTypeError
timezone.now()returns an aware datetime, whereas GitHub-sourced timestamps (created_at,last_committed_at, …) can be naïve. Subtracting them will crash at runtime.- return (timezone.now() - self.created_at).days if self.created_at else 0 + if not self.created_at: + return 0 + created = ( + timezone.make_aware(self.created_at, timezone.utc) + if timezone.is_naive(self.created_at) + else self.created_at + ) + return (timezone.now() - created).daysApply the same guarding to the other date-difference properties or extract a helper to avoid repetition.
🧹 Nitpick comments (3)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
56-68:EXPECTED_SCOREis hard-coded: future weight tweaks will break the testThe expected score (40.0) is manually derived from the current mapping. Any future change to
forward_fields/backward_fieldsor per-field weight will silently invalidate this assertion.
Consider calculating the expectation inside the test from the samefields_weightsdict to keep the test self-adjusting.backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py (1)
60-68: Chain-mocking leavesfilter().distinct()unchecked
unhealthy_projectsalways calls.filter()(becauselow_score=True). The test sets up the mock but never asserts that.filter()is invoked with the expected kwargs, so a regression in the filter logic would go unnoticed. Add an assertion onmock_queryset.filter.call_args.backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py (1)
12-31: Adddb_index=Trueto boolean flags that are queried through GraphQL
unhealthy_projectsfilters directly on these four flags. On a largeProjectHealthMetricstable a full-table scan for every flag combination will get expensive. Adding single-column indexes keeps the filter cost O(log n) instead of O(n).- field=models.BooleanField(default=False, verbose_name="Has long open issues"), + field=models.BooleanField( + default=False, + verbose_name="Has long open issues", + db_index=True, + ), ... - field=models.BooleanField(default=False, verbose_name="Has long unanswered issues"), + field=models.BooleanField( + default=False, + verbose_name="Has long unanswered issues", + db_index=True, + ), ... - field=models.BooleanField(default=False, verbose_name="Has long unassigned issues"), + field=models.BooleanField( + default=False, + verbose_name="Has long unassigned issues", + db_index=True, + ), ... - field=models.BooleanField(default=False, verbose_name="Has no recent commits"), + field=models.BooleanField( + default=False, + verbose_name="Has no recent commits", + db_index=True, + ),Add a follow-up migration if this one is already applied in staging.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/Makefile(1 hunks)backend/apps/owasp/graphql/nodes/project.py(2 hunks)backend/apps/owasp/graphql/nodes/project_health_metrics.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(2 hunks)backend/apps/owasp/graphql/queries/project_health_metrics.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_metrics_scores.py(1 hunks)backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py(1 hunks)backend/apps/owasp/models/project.py(8 hunks)backend/apps/owasp/models/project_health_metrics.py(4 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(2 hunks)backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py(1 hunks)cspell/custom-dict.txt(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- cspell/custom-dict.txt
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/tests/apps/owasp/graphql/nodes/project_test.py
- backend/apps/owasp/graphql/queries/init.py
- backend/apps/owasp/graphql/nodes/project.py
- backend/Makefile
- backend/apps/owasp/models/project.py
🧰 Additional context used
🧬 Code Graph Analysis (4)
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(39-44)has_no_recent_commits(256-259)pull_request_last_created_at(229-233)owasp_page_last_updated_at(208-212)bulk_save(344-352)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-124)
backend/apps/owasp/graphql/queries/project_health_metrics.py (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(24-55)backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-124)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(24-55)backend/tests/apps/owasp/graphql/nodes/project_test.py (2)
test_meta_configuration(16-40)_get_field_by_name(42-45)
🪛 Pylint (3.3.7)
backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py
[error] 17-17: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
[error] 25-25: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
[refactor] 47-47: 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)
backend/apps/owasp/graphql/nodes/project_health_metrics.py
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
backend/apps/owasp/graphql/queries/project_health_metrics.py
[refactor] 17-17: Too many arguments (11/5)
(R0913)
[refactor] 13-13: 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_projecthealthmetrics_has_long_open_issues_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
[error] 39-39: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
[refactor] 14-14: Too few public methods (1/2)
(R0903)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py
[refactor] 13-13: 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 (3)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py (1)
14-28:⚠️ Potential issuePatches are torn down before the test runs
The context-manager inside the
autousefixture exits before the test body is executed, soself.mock_projectsandself.mock_bulk_saveare inactive whencall_commandis invoked. Assertions will silently pass even though the real DB is touched.Convert the patches to persistent patchers:
def _setup(self): ... - with ( - patch("apps.owasp.models.project.Project.objects.all") as projects_patch, - patch("apps.owasp.models.project_health_metrics.ProjectHealthMetrics.bulk_save") as bulk_save_patch, - ): - self.mock_projects = projects_patch - self.mock_bulk_save = bulk_save_patch - yield + projects_patcher = patch("apps.owasp.models.project.Project.objects.all") + bulk_save_patcher = patch("apps.owasp.models.project_health_metrics.ProjectHealthMetrics.bulk_save") + self.mock_projects = projects_patcher.start() + self.mock_bulk_save = bulk_save_patcher.start() + yield + projects_patcher.stop() + bulk_save_patcher.stop()Likely an incorrect or invalid review comment.
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
10-11: Sanity check looks fineVerifying that the node is a Strawberry type is a helpful smoke test.
backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py (1)
1-1: ConfirmGenerated by Django 5.2.1aligns with the runtime versionThe migration header pins the file to Django 5.2.1. If the deployed environment is still on 5.1 or earlier, the generated SQL may differ subtly (e.g.,
deferrablehandling).
Verify that the project’srequirements/Docker image is already bumped to 5.2.1 before merging.
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_scores.py
Outdated
Show resolved
Hide resolved
ahmedxgouda
force-pushed
the
dashboard/graphql-health-queries
branch
2 times, most recently
from
30447ab to
9972b8c
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (6)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (2)
27-45:⚠️ Potential issueInfinite recursion – resolvers return themselves
Each resolver returns the same attribute it defines (
self.age_days,self.last_commit_days, …).
At runtime this calls the resolver again, producing an endless loop →RecursionError.- @strawberry.field - def age_days(self) -> int: - return self.age_days + @strawberry.field + def age_days(self) -> int: + # `self._root` is the underlying Django model instance. + return self._root.age_daysApply the same fix to the four other resolvers (
last_commit_days,last_pull_request_days,last_release_days,owasp_page_last_update_days).
Alternatively, drop the custom resolvers and list those properties infields=[…].
52-55:⚠️ Potential issue
project_nameresolver accesses missing attribute
self.projectis not part of the dataclass fields, so this raisesAttributeError.
Use the root model instead:- def project_name(self) -> str: - return self.project.name + def project_name(self) -> str: + return self._root.project.name🧰 Tools
🪛 Pylint (3.3.7)
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
20-33:⚠️ Potential issueBroken attribute mapping – will crash and/or write invalid data
•
pushed_at,released_at,pull_requests_count,releases_count,
open_pull_requests_count,total_pull_requests_count,
total_releases_countdo not exist on theProjector
ProjectHealthMetricsmodels.
•has_recent_commitsexists on metrics but not on project.The first access raises
AttributeError, killing the command, and the latter
will silently skip a field on save.- "last_committed_at": "pushed_at", - "last_released_at": "released_at", - "open_pull_requests_count": "open_pull_requests_count", - "total_pull_requests_count": "pull_requests_count", - "total_releases_count": "releases_count", + "last_committed_at": "last_committed_at", + "last_released_at": "last_released_at", + # remove nonexistent *_pull_requests_count and *_releases_count mappingsDouble-check the remaining keys match real model attributes.
34-45:⚠️ Potential issueIdempotency / duplication risk
ProjectHealthMetrics(project=project)unconditionally creates a new row every
run – the table will grow unbounded. Replace withupdate_or_create(or add a
unique constraint) and update fields in-place to keep one record per project.- metrics = ProjectHealthMetrics(project=project) + metrics, _ = ProjectHealthMetrics.objects.update_or_create( + project=project, defaults={} + )backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
21-33: 🛠️ Refactor suggestionStill patching only
.filter– ORM chain not fully stubbedThe command now does
ProjectHealthMetrics.objects.filter(...).select_related(...).
Patchingobjects.filterstubs the first call, but any additional method chained beforefilterin future refactors will hit the real DB.
Safer:-patch("apps.owasp.models.project_health_metrics.ProjectHealthMetrics.objects.filter") as metrics_patch, +patch("apps.owasp.models.project_health_metrics.ProjectHealthMetrics.objects") as metrics_manager,and then:
metrics_manager.filter.return_value.select_related.return_value = [mock_metric]Same applies to
ProjectHealthRequirements—patch the manager, not.all.backend/apps/owasp/models/project_health_metrics.py (1)
76-107:⚠️ Potential issueTimezone-naïve vs. aware datetime subtraction is still unresolved
These five properties reuse the pattern
(timezone.now() - self.<field>).days
which will explode withTypeError: can't subtract offset-naive and offset-aware datetimeswhen
<field>is naïve (many GitHub dates are). This issue was flagged in the previous review and remains unfixed.Suggested helper-based fix:
+from django.utils import timezone + +# helper (private) +def _days_since(dt): + if not dt: + return 0 + aware_dt = timezone.make_aware(dt, timezone.utc) if timezone.is_naive(dt) else dt + return (timezone.now() - aware_dt).days ... @property def age_days(self) -> int: """Calculate project age in days.""" - return (timezone.now() - self.created_at).days if self.created_at else 0 + return _days_since(self.created_at) @property def last_commit_days(self) -> int: """Calculate days since last commit.""" - return (timezone.now() - self.last_committed_at).days if self.last_committed_at else 0 + return _days_since(self.last_committed_at) @property def last_pull_request_days(self) -> int: """Calculate days since last pull request.""" - return ( - (timezone.now() - self.pull_request_last_created_at).days - if self.pull_request_last_created_at - else 0 - ) + return _days_since(self.pull_request_last_created_at) ... def last_release_days(self) -> int: """Calculate days since last release.""" - return (timezone.now() - self.last_released_at).days if self.last_released_at else 0 + return _days_since(self.last_released_at) ... def owasp_page_last_update_days(self) -> int: """Calculate days since OWASP page last update.""" - return ( - (timezone.now() - self.owasp_page_last_updated_at).days - if self.owasp_page_last_updated_at - else 0 - ) + return _days_since(self.owasp_page_last_updated_at)
🧹 Nitpick comments (6)
backend/apps/owasp/admin.py (1)
167-185: Missingshort_descriptionfor custom columnAdmin uses the method name
projectas the column header. Add
short_descriptionto produce a friendlier heading and avoid translation
warnings.def project(self, obj): """Display project name.""" return obj.project.name if obj.project else "N/A" + + project.short_description = "Project"🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 167-167: Too few public methods (1/2)
(R0903)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
64-67: Type assertion brittle to Strawberry internals
field.type is expected_typewill break if Strawberry wraps scalars
(e.g.Annotated[int, …]). Prefer==orissubclassto make the test future-proof.- assert field.type is expected_type + assert field.type == expected_type or issubclass(field.type, expected_type)backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
20-36: Method signature is growing – consider encapsulating filter arguments
unhealthy_projectsalready accepts 10 optional booleans pluslimit.
Long parameter lists hurt readability and stability when adding new filters.Suggestion: introduce an input type / dataclass (e.g.,
@strawberry.inputProjectHealthFilter) and accept a singlefiltersargument.
This keeps the schema extensible while avoidingR0913warnings.backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py (1)
15-22: Tightly coupled to Strawberry internalsAccessing
__strawberry_definition__ties the test to Strawberry’s private API and triggers static-analysis errors.
Prefer the public helperstrawberry.typeinspection utilities or rely on schema generation tests:schema = strawberry.Schema(query=ProjectHealthMetricsQuery) assert "unhealthyProjects" in str(schema)Reduces brittleness against library updates.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 19-19: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
backend/apps/owasp/models/project_health_metrics.py (2)
58-62: Allow blank values for nullablescorefield
scoreis nullable but notblank=True. In Django admin / forms this raises validation errors (“This field is required”) even though the DB allowsNULL.- score = models.FloatField( - null=True, + score = models.FloatField( + null=True, + blank=True, validators=[MinValueValidator(0.0), MaxValueValidator(100.0)], help_text="Project health score (0-100)", )
109-119: Staticbulk_savewrapper is redundant
BulkSaveModel.bulk_savealready exposes the same signature; this wrapper adds no extra logic and risks diverging if the base implementation changes.Consider deleting
ProjectHealthMetrics.bulk_saveand relying directly on the inherited static-method:BulkSaveModel.bulk_save(ProjectHealthMetrics, metrics, fields=fields)call-sites can instead do:
ProjectHealthMetrics.bulk_save(metrics, fields=[...])without the override.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/graphql/nodes/project.py(2 hunks)backend/apps/owasp/graphql/nodes/project_health_metrics.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(2 hunks)backend/apps/owasp/graphql/queries/project_health_metrics.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_metrics_scores.py(1 hunks)backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.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/migrations/0041_merge_20250613_0336.py(1 hunks)backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py(1 hunks)backend/apps/owasp/models/project.py(8 hunks)backend/apps/owasp/models/project_health_metrics.py(4 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(2 hunks)backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py(1 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 (4)
- backend/tests/apps/owasp/graphql/nodes/project_test.py
- backend/apps/owasp/graphql/nodes/project.py
- backend/apps/owasp/graphql/queries/init.py
- backend/apps/owasp/models/project.py
🧰 Additional context used
🧬 Code Graph Analysis (6)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-122)
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 (7)
Meta(35-40)has_recent_commits(136-139)is_funding_requirements_compliant(152-154)is_leader_requirements_compliant(157-161)pull_request_last_created_at(233-237)owasp_page_last_updated_at(212-216)bulk_save(342-350)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(24-55)backend/tests/apps/owasp/graphql/nodes/project_test.py (2)
test_meta_configuration(16-40)_get_field_by_name(42-45)
🪛 Pylint (3.3.7)
backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_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/graphql/nodes/project_health_metrics.py
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
backend/apps/owasp/graphql/queries/project_health_metrics.py
[refactor] 17-17: Too many arguments (11/5)
(R0913)
[refactor] 13-13: 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_projecthealthmetrics_has_long_open_issues_and_more.py
[refactor] 6-6: Too few public methods (0/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/migrations/0041_merge_20250613_0336.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/graphql/nodes/project_health_metrics_test.py
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
[error] 39-39: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py
[error] 19-19: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
[error] 27-27: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
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 (7)
backend/apps/owasp/migrations/0040_alter_projecthealthmetrics_is_leader_requirements_compliant.py (1)
15-21: Verify existing NULLs before enforcingdefault=False
BooleanField(default=False)will implicitly setnull=False.
If older rows still containNULL(e.g., from the previousBooleanField(null=True)definition), thisALTER TABLE … SET NOT NULLwill fail at runtime.Either add a
RunPythonto coerceNULL→Falsebefore theAlterField, or run an explicit SQLUPDATEin a data migration.backend/apps/owasp/migrations/0039_rename_is_leaders_requirements_compliant_projecthealthmetrics_is_leader_requirements_compliant.py (1)
14-20: Rename looks good; remember to update code & testsThe pure
RenameFieldoperation is safe and reversible.
Please ensure model/property references, GraphQL schema, and fixtures now useis_leader_requirements_compliant.backend/apps/owasp/migrations/0041_merge_20250613_0336.py (1)
6-12: Merge migration is fineNo operations – serves only to linearise history. ✅
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py (1)
6-32: Migration looks goodFields are added with defaults and verbose names; no data-migration needed.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/tests/apps/owasp/models/project_health_metrics_test.py (1)
40-50: Ambiguous validation rule forNonescore
test_score_validationasserts that explicitly settingscore=Noneis invalid, yettest_default_scoreexpects the default value to beNone.
Ifscoreis nullable in the model, explicitly passingNoneshould probably also be valid. Clarify the business rule and align the tests/model accordingly.backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
73-76: Field namenest_created_atseems suspiciousOrdering with
"-nest_created_at"implies the model has anest_created_atcolumn, which is uncommon and easy to mistype (created_at,next_created_at, …).
Double-check the model; a typo here silently returns unordered results.backend/apps/owasp/models/project_health_metrics.py (1)
17-22: Clarify multiplicity & enforce DB integritySwitching
projectfromOneToOneFieldtoForeignKeyopens the door to multipleProjectHealthMetricsrows per project.
If that is intentional (e.g. snapshots over time), consider:
- Adding a uniqueness or partial-index (
unique_together,constraints=[models.UniqueConstraint(..., condition=…)]) that captures the desired multiplicity (e.g. only one active metrics row).- Surfacing an ordering (
get_latest_by,ordering = ["-nest_created_at"]) to makeproject.health_metrics.first()deterministic.Without this, callers that previously did
project.health_metrics(singular) may now hit unexpected queryset vs. object semantics.
...tions/0038_remove_projecthealthmetrics_is_project_leaders_requirements_compliant_and_more.py
Outdated
Show resolved
Hide resolved
| @pytest.mark.parametrize( | ||
| ("field_name", "expected_days"), | ||
| [ | ||
| ("age_days", (timezone.now() - FIXED_DATE).days), | ||
| ("last_commit_days", (timezone.now() - FIXED_DATE).days), | ||
| ("last_pull_request_days", (timezone.now() - FIXED_DATE).days), | ||
| ("last_release_days", (timezone.now() - FIXED_DATE).days), | ||
| ("owasp_page_last_update_days", (timezone.now() - FIXED_DATE).days), | ||
| ], | ||
| ) | ||
| def test_handle_days_calculation(self, field_name, expected_days): | ||
| """Should return correct days for date fields.""" | ||
| metrics = ProjectHealthMetrics() | ||
| metrics.created_at = self.FIXED_DATE | ||
| metrics.last_committed_at = self.FIXED_DATE | ||
| metrics.last_released_at = self.FIXED_DATE | ||
| metrics.owasp_page_last_updated_at = self.FIXED_DATE | ||
| metrics.pull_request_last_created_at = self.FIXED_DATE | ||
|
|
||
| assert getattr(metrics, field_name) == expected_days |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential midnight flakiness in test_handle_days_calculation
expected_days is computed during test collection (timezone.now() in the parametrisation).
If the test suite spans midnight, the runtime call inside the property will use “tomorrow’s” date, yielding an off-by-one and a sporadic failure.
-("age_days", (timezone.now() - FIXED_DATE).days),
+# Freeze time instead of sampling twice
+("age_days", 365), # any deterministic constantConsider using freezegun/django.utils.timezone.override inside the test or calculating expected_days after freezing time.
🏁 Script executed:
#!/bin/bash
set -e
# Locate the test file
TEST_FILE=$(fd project_health_metrics_test.py -t f)
echo "Test file path: $TEST_FILE"
echo
# Show the top of the file to inspect FIXED_DATE and any freezing logic
sed -n '1,200p' "$TEST_FILE"Length of output: 1644
🏁 Script executed:
#!/bin/bash
set -e
# Locate the specific test file under models directory
FILE=$(fd models/project_health_metrics_test.py -t f)
echo "Using file: $FILE"
echo "===== File Contents (1-200) ====="
sed -n '1,200p' "$FILE"
echo
echo "===== Grep for FIXED_DATE ====="
grep -n "FIXED_DATE" "$FILE" || true
echo
echo "===== Grep for timezone.now ====="
grep -n "timezone.now" "$FILE" || true
echo
echo "===== Grep for freeze/override usage ====="
grep -n -E "freeze|override" "$FILE" || trueLength of output: 240
#!/bin/bash
set -e
FILE="backend/tests/apps/owasp/models/project_health_metrics_test.py"
echo "===== Showing start of $FILE ====="
sed -n '1,200p' "$FILE" || { echo "Failed to read $FILE"; exit 1; }
echo
echo "===== Searching for FIXED_DATE ====="
grep -n "FIXED_DATE" "$FILE" || echo "No FIXED_DATE found"
echo
echo "===== Searching for timezone.now ====="
grep -n "timezone.now" "$FILE" || echo "No timezone.now found"
echo
echo "===== Searching for freeze or override usage ====="
grep -n -E "freeze|override" "$FILE" || echo "No freeze/override usage found"Address potential midnight flakiness in test_handle_days_calculation
The test currently computes expected_days at collection time using timezone.now(), then calls the same expression inside the property at runtime. If the suite crosses midnight, these two “now” values can span different days and trigger an off–by–one failure.
To fix this, freeze time once at the start of the test and compute expected_days against that frozen clock. For example, using freezegun:
- @pytest.mark.parametrize(
- ("field_name", "expected_days"),
- [
- ("age_days", (timezone.now() - FIXED_DATE).days),
- …,
- ],
- )
+ from freezegun import freeze_time
+
+ @freeze_time(FIXED_DATE)
+ @pytest.mark.parametrize(
+ ("field_name", "expected_days"),
+ [
+ ("age_days", 0),
+ ("last_commit_days", 0),
+ ("last_pull_request_days", 0),
+ ("last_release_days", 0),
+ ("owasp_page_last_update_days", 0),
+ ],
+ )Or alternatively, use django.utils.timezone.override as a context manager/fixture to pin timezone.now() to FIXED_DATE before computing any expectations.
📝 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.
| @pytest.mark.parametrize( | |
| ("field_name", "expected_days"), | |
| [ | |
| ("age_days", (timezone.now() - FIXED_DATE).days), | |
| ("last_commit_days", (timezone.now() - FIXED_DATE).days), | |
| ("last_pull_request_days", (timezone.now() - FIXED_DATE).days), | |
| ("last_release_days", (timezone.now() - FIXED_DATE).days), | |
| ("owasp_page_last_update_days", (timezone.now() - FIXED_DATE).days), | |
| ], | |
| ) | |
| def test_handle_days_calculation(self, field_name, expected_days): | |
| """Should return correct days for date fields.""" | |
| metrics = ProjectHealthMetrics() | |
| metrics.created_at = self.FIXED_DATE | |
| metrics.last_committed_at = self.FIXED_DATE | |
| metrics.last_released_at = self.FIXED_DATE | |
| metrics.owasp_page_last_updated_at = self.FIXED_DATE | |
| metrics.pull_request_last_created_at = self.FIXED_DATE | |
| assert getattr(metrics, field_name) == expected_days | |
| from freezegun import freeze_time | |
| @freeze_time(FIXED_DATE) | |
| @pytest.mark.parametrize( | |
| ("field_name", "expected_days"), | |
| [ | |
| ("age_days", 0), | |
| ("last_commit_days", 0), | |
| ("last_pull_request_days", 0), | |
| ("last_release_days", 0), | |
| ("owasp_page_last_update_days", 0), | |
| ], | |
| ) | |
| def test_handle_days_calculation(self, field_name, expected_days): | |
| """Should return correct days for date fields.""" | |
| metrics = ProjectHealthMetrics() | |
| metrics.created_at = self.FIXED_DATE | |
| metrics.last_committed_at = self.FIXED_DATE | |
| metrics.last_released_at = self.FIXED_DATE | |
| metrics.owasp_page_last_updated_at = self.FIXED_DATE | |
| metrics.pull_request_last_created_at = self.FIXED_DATE | |
| assert getattr(metrics, field_name) == expected_days |
🤖 Prompt for AI Agents
In backend/tests/apps/owasp/models/project_health_metrics_test.py around lines
100 to 119, the test_handle_days_calculation parametrizes expected_days using
timezone.now() at test collection time, which can cause flakiness if the test
runs across midnight. To fix this, freeze time at the start of the test using a
tool like freezegun or django.utils.timezone.override, then compute
expected_days inside the frozen time context so both expected and actual values
use the same fixed current time, ensuring consistent and reliable test results.
There was a problem hiding this comment.
@arkid15r What do you think of this? It is related to the previous PR, but idk if this is correct
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
Outdated
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: 3
♻️ Duplicate comments (2)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (1)
36-45:⚠️ Potential issueIdempotency / duplication problem persists – switch to
update_or_createEach run instantiates a new
ProjectHealthMetrics(project=project)and later bulk-inserts it, resulting in unbounded duplicates per project.
Reuse the existing row instead:- metrics = ProjectHealthMetrics(project=project) + metrics, _ = ProjectHealthMetrics.objects.get_or_create(project=project)With that change
bulk_savecan be dropped entirely and you can simply accumulate the instances in a list and callbulk_update.backend/apps/owasp/models/project_health_metrics.py (1)
80-87: 🛠️ Refactor suggestionNaïve/aware datetime mix may raise
TypeError
timezone.now()is aware; stored timestamps coming from GitHub/APIs are often naïve.
Subtracting the two explodes at runtime.- return (timezone.now() - self.created_at).days if self.created_at else 0 + if not self.created_at: + return 0 + created = ( + timezone.make_aware(self.created_at, timezone.utc) + if timezone.is_naive(self.created_at) + else self.created_at + ) + return (timezone.now() - created).daysRefactor the same guard into a small helper and reuse it in the other
*_daysproperties to avoid duplication.
🧹 Nitpick comments (2)
backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
47-49: Minor: build filter key without manual dunder jugglingCurrent f-string concatenation is correct but brittle. A clearer variant:
op = "gte" if is_contributors_requirement_compliant else "lt" filters[f"contributors_count__{op}"] = CONTRIBUTORS_COUNT_REQUIREMENTPure readability suggestion – no functional change.
backend/apps/owasp/models/project_health_metrics.py (1)
111-120:bulk_savewrapper duplicates the one inBulkSaveModelSince
BulkSaveModelalready exposesbulk_save, re-declaring it here adds
maintenance surface without extra value. Consumers can directly call:BulkSaveModel.bulk_save(ProjectHealthMetrics, metrics, fields)Recommend deleting this wrapper unless you intend to attach extra behaviour.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/owasp/graphql/queries/project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py(1 hunks)backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py(1 hunks)backend/apps/owasp/models/project.py(8 hunks)backend/apps/owasp/models/project_health_metrics.py(4 hunks)backend/tests/apps/owasp/graphql/queries/project_health_metrics_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 (1)
backend/apps/owasp/models/project_health_metrics.py (2)
backend/apps/common/models.py (1)
BulkSaveModel(8-30)backend/apps/owasp/models/project.py (7)
Meta(35-40)has_no_recent_commits(136-139)is_funding_requirements_compliant(152-154)is_leader_requirements_compliant(157-161)pull_request_last_created_at(233-237)owasp_page_last_updated_at(212-216)bulk_save(342-350)
🪛 Pylint (3.3.7)
backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.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/apps/owasp/graphql/queries/project_health_metrics.py
[refactor] 17-17: Too many arguments (11/5)
(R0913)
[refactor] 13-13: 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/tests/apps/owasp/graphql/queries/project_health_metrics_test.py
[error] 19-19: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
[error] 27-27: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (1)
20-30: Attribute mapping drift – verifypushed_atstill exists
"last_committed_at": "pushed_at"assumes theProjectmodel kept the legacy
pushed_atfield. The current model snippet showslast_committed_at; if
pushed_atwas removed the command will raiseAttributeError.- "last_committed_at": "pushed_at", + "last_committed_at": "last_committed_at",Please grep the codebase or run the proposed shell check to confirm.
backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py
Outdated
Show resolved
Hide resolved
ahmedxgouda
force-pushed
the
dashboard/graphql-health-queries
branch
from
2ebde4f to
2f08e36
Compare
…Metrics objects of the project
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (1)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_requirements_test.py (1)
25-40: 🛠️ Refactor suggestion
levelparameter is unused in the test body – misleading parametrisationThe
@pytest.mark.parametrizedecorator varieslevel, yet the test never references the parameter when calling the command or asserting behaviour.
This hides potential regressions (e.g. per-level branching in the command) and gives a false sense of coverage.Consider either:
-@pytest.mark.parametrize(("level", "expected_output", "display_name"), [...]) +@pytest.mark.parametrize(("level", "expected_output", "display_name"), [...])and assert that
update_or_createwas invoked with the expectedlevel, or collapse the parametrisation if the value truly does not matter.
♻️ Duplicate comments (10)
backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py (1)
11-20: Data loss risk when flipping boolean semantics (already raised earlier)Dropping
has_no_recent_commitsand addinghas_recent_commitswithout aRunPythonstep silently nullifies existing data.
Please add a forward data migration that copies and inverts the old values, then remove the legacy column.backend/apps/owasp/graphql/nodes/project_health_metrics.py (2)
27-51: 🛠️ Refactor suggestion
⚠️ Potential issueInfinite recursion in resolver methods – still unfixed
The five resolvers return the property they are defining, leading to an endless self-reference (
self.age_days → self.age_days → …) and aRecursionErrorat runtime.- @strawberry.field - def age_days(self) -> int: - return self.age_days + @strawberry.field + def age_days(self) -> int: + # Strawberry-Django sets the underlying Django instance on `self._root` + return self._root.age_daysApply the same pattern to
last_commit_days,last_pull_request_days,last_release_days, andowasp_page_last_update_days.Failing to fix will break every GraphQL query that selects any of these fields.
52-55:⚠️ Potential issue
project_nameresolver accesses a non-existent attribute
self.projectis not part of the exposedfields; it will raiseAttributeError. Use the root instance instead:- def project_name(self) -> str: - return self.project.name + def project_name(self) -> str: + return self._root.project.name🧰 Tools
🪛 Pylint (3.3.7)
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (2)
35-43: 🛠️ Refactor suggestionIdempotency / duplication risk
A fresh
ProjectHealthMetricsinstance is created on every run, producing unbounded rows per project. Useupdate_or_create(or fetch & update) to keep one metrics record per project.- metrics = ProjectHealthMetrics(project=project) + metrics, _ = ProjectHealthMetrics.objects.get_or_create(project=project)This also avoids bloating the
bulk_savelist.
12-33:⚠️ Potential issueField map contains invalid & mis-named attributes – command will explode
The mapping still references attributes that do not exist on either model:
• Metrics fields not present on
ProjectHealthMetrics:
open_pull_requests_count,total_issues_count,total_pull_requests_count,total_releases_count.• Project attributes apparently mis-named:
pushed_at,released_at(spec should belast_committed_at,last_released_at),pull_request_last_created_at(check actual model).
setattrwill silently create ad-hoc attributes; the subsequentbulk_savewill raiseFieldError.- "last_committed_at": "pushed_at", - "last_released_at": "released_at", - "open_pull_requests_count": "open_pull_requests_count", - "total_issues_count": "issues_count", - "total_pull_requests_count": "pull_requests_count", - "total_releases_count": "releases_count", + "last_committed_at": "last_committed_at", + "last_released_at": "last_released_at", + # remove nonexistent metric fields or add them to the modelPlease audit the full list against both models before this command hits production.
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (2)
17-23: 🛠️ Refactor suggestion
open_pull_requests_countscored in the wrong directionA higher number of open PRs indicates worse health, yet the metric is placed
inforward_fields, rewarding larger values.
Move it tobackward_fields(or set a negative weight) to avoid inflating
scores for unhealthy projects.- "open_pull_requests_count": 6.0, + # Higher is worse + backward_fields = { + ... + "open_pull_requests_count": 6.0,
54-55:⚠️ Potential issuePotential
KeyErrorwhen requirements for a level are missing
project_health_requirements[metric.project.level]assumes every level has
configured requirements.
Guard against missing entries to prevent the whole command from aborting.- requirements = project_health_requirements[metric.project.level] + requirements = project_health_requirements.get(metric.project.level) + if requirements is None: + self.stderr.write( + self.style.WARNING( + f"No health requirements for level '{metric.project.level}'. Skipping." + ) + ) + continuebackend/apps/owasp/graphql/queries/project_health_metrics.py (1)
73-75:⚠️ Potential issueOrdering uses non-existent field
nest_created_at
ProjectHealthMetricshascreated_at, notnest_created_at; the current
query will raiseFieldError.- .order_by("project__key", "-nest_created_at") + .order_by("project__key", "-created_at")backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py (1)
105-109: Still using removed fieldhas_recent_commits
This was flagged in the previous review and the model no longer contains that attribute (replaced withhas_no_recent_commits).
Leaving it here will raiseAttributeErrorwhen the mock is accessed.- mock_metrics_filtered_different_project.has_recent_commits = False + mock_metrics_filtered_different_project.has_no_recent_commits = Truebackend/apps/owasp/models/project_health_metrics.py (1)
84-114: 🛠️ Refactor suggestion
⚠️ Potential issueNaïve vs aware datetime arithmetic can raise
TypeError
timezone.now()returns an aware datetime.
If any stored timestamp (created_at,last_committed_at, …) is naïve (e.g. data imported from GitHub), subtracting will explode at runtime.Normalize the stored value before subtraction:
-from django.utils import timezone +from django.utils import timezone, datetime_safe def _days_since(dt: datetime.datetime | None) -> int: if not dt: return 0 - return (timezone.now() - dt).days + aware = ( + timezone.make_aware(dt, timezone.utc) + if timezone.is_naive(dt) + else dt + ) + return (timezone.now() - aware).daysThen reuse
_days_sincein the five properties to remove duplication.This guards against hidden
TypeErrors and follows DRY.
🧹 Nitpick comments (10)
backend/apps/owasp/migrations/0041_projecthealthrequirements_is_funding_requirements_compliant_and_more.py (1)
11-25: Potential long-running lock when adding non-nullable fields with defaultAdding two
BooleanField(default=True)columns will trigger a full table rewrite on Postgres < 11 and hold an exclusive lock.
To mitigate production impact:
- Add the fields with
null=True, default=None(fast metadata change).- Run a
RunPythonto populateTruefor existing rows.- Alter the fields to
null=False, default=True.Optional but worth considering for large tables.
backend/apps/owasp/migrations/0041_merge_20250613_0336.py (1)
6-12: Nit: add a comment explaining the merge reasonA short docstring referencing the diverging branches (
0038vs0040) would help future maintainers understand why this empty merge exists.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
12-33: Test does not include all exposed fields
expected_field_namesomits several fields that the node actually exposes (last_commit_days,last_pull_request_days, etc.); conversely it includesscorebut the subsequent type-check parametrization doesn’t. Consider synchronising the two lists to avoid false positives / missed regressions.🧰 Tools
🪛 Pylint (3.3.7)
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
backend/apps/owasp/admin.py (1)
186-189: Method name shadows the model field and reduces clarityHaving a callable named
projectinlist_displayshadows the real FK fieldproject.
Rename it (e.g.project_name) and updatelist_displayto avoid confusion and future maintenance slips.- def project(self, obj): + def project_name(self, obj): ... - list_display = ( - "project", + list_display = ( + "project_name",backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py (1)
26-33:has_no_recent_commitsflag never updatedThis boolean exists on the model and is exposed via GraphQL filters, but the
command never sets it.
Compute it here (e.g. whenlast_commit_daysexceeds the requirement) or
document that another process is responsible.backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py (1)
129-132: Preferself.stdout.writeover bareUsing Django’s provided stream respects verbosity flags and allows easier
redirection during tests.- print( + self.stdout.write(backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py (3)
14-22: Possible brittle assertion on private Strawberry internalsThe test directly introspects
__strawberry_definition__, which is considered an internal implementation detail ofstrawberry.
Upgrading Strawberry could rename/move this attribute and instantly break the test even though the public API contract remains intact.Consider asserting the field’s existence via Strawberry’s public helpers instead of private attributes (e.g.
strawberry.schema.Schema.from_types(...)then inspecting the schema).
This keeps the test focused on behaviour rather than internals.🧰 Tools
🪛 Pylint (3.3.7)
[error] 19-19: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
60-70: Mock chain is incomplete –distinct()never returns mock
mock_queryset.order_by.return_value.distinct.return_value = mock_querysetworks,
butmock_queryset.order_by.return_valueitself is never assigned, so
mock_queryset.order_by().distinct()returnsNone.
Set both links for full safety:ordered = MagicMock() mock_queryset.order_by.return_value = ordered ordered.distinct.return_value = orderedThis prevents silent
NoneTypeerrors if the resolver accesses the intermediate queryset.
145-155: Hard-coded filter expectations risk divergenceThe assertion pins the resolver’s filter signature exactly.
Any legitimate change to the business rules (e.g. threshold tweak from2to3) will red-bar the suite.Consider asserting that
filter()was called with a subset of mandatory predicates instead of the full kwargs dict, e.g.:called_kwargs = mock_queryset.filter.call_args.kwargs assert called_kwargs["has_no_recent_commits"] is True assert called_kwargs["score__lt"] == 50This keeps the test valuable without coupling it to every constant.
backend/apps/owasp/models/project_health_metrics.py (1)
116-126:bulk_savewrapper duplicates logic inBulkSaveModel
BulkSaveModel.bulk_savealready handles batching; this thin wrapper just forwards arguments.
Unless you intend to add extra logic later, you can drop this staticmethod and call the mix-in directly:ProjectHealthMetrics.bulk_save = BulkSaveModel.bulk_save # or just use BulkSaveModel.bulk_save(...)This reduces indirection and keeps one source of truth.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/graphql/nodes/project.py(2 hunks)backend/apps/owasp/graphql/nodes/project_health_metrics.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(2 hunks)backend/apps/owasp/graphql/queries/project_health_metrics.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_metrics_scores.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py(5 hunks)backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py(1 hunks)backend/apps/owasp/migrations/0041_merge_20250613_0336.py(1 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/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py(1 hunks)backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py(1 hunks)backend/apps/owasp/models/project.py(8 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/graphql/nodes/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(2 hunks)backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.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 due to trivial changes (1)
- backend/apps/owasp/models/project_health_requirements.py
🚧 Files skipped from review as they are similar to previous changes (5)
- backend/tests/apps/owasp/graphql/nodes/project_test.py
- backend/apps/owasp/graphql/queries/init.py
- backend/apps/owasp/graphql/nodes/project.py
- backend/tests/apps/owasp/models/project_health_metrics_test.py
- backend/apps/owasp/models/project.py
🧰 Additional context used
🧬 Code Graph Analysis (7)
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 (7)
Meta(35-40)has_no_recent_commits(136-139)is_funding_requirements_compliant(152-154)is_leader_requirements_compliant(157-161)pull_request_last_created_at(231-235)owasp_page_last_updated_at(212-214)bulk_save(340-348)
backend/apps/owasp/admin.py (1)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-125)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-125)
backend/apps/owasp/graphql/queries/project_health_metrics.py (3)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(24-55)backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-125)backend/apps/owasp/models/project.py (3)
is_funding_requirements_compliant(152-154)has_no_recent_commits(136-139)is_leader_requirements_compliant(157-161)
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/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/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(24-55)backend/tests/apps/owasp/graphql/nodes/project_test.py (2)
test_meta_configuration(16-40)_get_field_by_name(42-45)
🪛 Pylint (3.3.7)
backend/apps/owasp/models/project_health_metrics.py
[refactor] 13-13: 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/graphql/nodes/project_health_metrics.py
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
backend/apps/owasp/graphql/queries/project_health_metrics.py
[refactor] 17-17: Too many arguments (11/5)
(R0913)
[refactor] 13-13: 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_projecthealthmetrics_has_long_open_issues_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0041_merge_20250613_0336.py
[refactor] 6-6: 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/migrations/0042_alter_projecthealthmetrics_score.py
[refactor] 7-7: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
[error] 39-39: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py
[error] 19-19: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
[error] 27-27: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
[refactor] 14-14: Too few public methods (1/2)
(R0903)
🔇 Additional comments (4)
backend/apps/owasp/migrations/0042_alter_projecthealthmetrics_score.py (1)
16-25: Looks good – adds proper validationChanging
scoreto nullable with 0-100 validators is sensible and non-breaking.backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py (1)
12-31: Boolean flags added correctlyFields are well-named, defaulted, and come with clear verbose names.
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py (1)
75-86:EXPECTED_SCORE(34.0) does not match the command’s weighting logicWith the current weights (7 backward fields × 6 = 42 points) the score
calculated for the provided data will be 42, not 34.
Either updateEXPECTED_SCOREor adjust the weighting constants so the
test reflects real business rules.backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py (1)
31-46:unhealthy_projects_field.type.of_typemay not be stable
of_typeis only present when the field is wrapped in a list/optional.
Depending on how Strawberry resolves generics,unhealthy_projects_field.typemay already be the expected node class, makingof_typeNoneand causing the test to fail spuriously.Recommend guarding the assertion or using
strawberry.unset. Example:assert ( getattr(unhealthy_projects_field.type, "of_type", unhealthy_projects_field.type) is ProjectHealthMetricsNode )
backend/tests/apps/owasp/management/commands/owasp_update_project_health_requirements_test.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py
Outdated
Show resolved
Hide resolved
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
Outdated
Show resolved
Hide resolved
ahmedxgouda
force-pushed
the
dashboard/graphql-health-queries
branch
from
2f08e36 to
26c5ce7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py (1)
11-20:⚠️ Potential issueUndoing the previous migration re-introduces the same data-loss problem
The column you just added in 0042 is immediately removed here without copying values back, causing a second wipe-out and a third full-table rewrite. The churn also breaks zero-downtime deploys.
Squash 0042 + 0043 into one rename-with-data-copy migration, or apply the
RenameFieldstrategy suggested in 0042.backend/apps/owasp/graphql/nodes/project_health_metrics.py (2)
27-46:⚠️ Potential issueInfinite recursion – resolvers return themselves
All five resolvers call the very attribute they are defining, so GraphQL will try to resolve the bound method, recurse, and blow the stack.
- def age_days(self) -> int: - return self.age_days + def age_days(self) -> int: + return self._root.age_days ... - def last_commit_days(self) -> int: - return self.last_commit_days + def last_commit_days(self) -> int: + return self._root.last_commit_days ... - def last_pull_request_days(self) -> int: - return self.last_pull_request_days + def last_pull_request_days(self) -> int: + return self._root.last_pull_request_days ... - def last_release_days(self) -> int: - return self.last_release_days + def last_release_days(self) -> int: + return self._root.last_release_days ... - def owasp_page_last_update_days(self) -> int: - return self.owasp_page_last_update_days + def owasp_page_last_update_days(self) -> int: + return self._root.owasp_page_last_update_daysExact same issue was pointed out in the previous review but is still unresolved.
52-55:⚠️ Potential issue
project_namecrashes –self.projectis not present
selfis the Strawberry dataclass proxy; only the names infields=[…]are copied onto it, soprojectisn’t available. Accessing it yieldsAttributeError.- def project_name(self) -> str: - """Resolve project node.""" - return self.project.name + def project_name(self) -> str: + """Resolve project name.""" + return self._root.project.nameSame remark as the previous review.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
72-76:⚠️ Potential issueTypo in
order_by– fieldnest_created_atdoes not exist
ProjectHealthMetricsonly hascreated_at; using the misspelled column will raiseFieldErrorat runtime.- .order_by("project__key", "-nest_created_at") + .order_by("project__key", "-created_at")This was flagged earlier and remains unfixed.
backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py (2)
78-79:⚠️ Potential issueTest asserts on non-existent column
nest_created_atThe code under test now orders by
created_at, notnest_created_at. Mirror that in the assertion to keep the test relevant.- mock_queryset.order_by.assert_called_with("project__key", "-nest_created_at") + mock_queryset.order_by.assert_called_with("project__key", "-created_at")
105-109:⚠️ Potential issueMock still uses removed
has_recent_commitsfieldThe model field was renamed to
has_no_recent_commits; keeping the old name makes the spec-based mock reject the assignment.- mock_metrics_filtered_different_project.has_recent_commits = False + mock_metrics_filtered_different_project.has_no_recent_commits = True
🧹 Nitpick comments (2)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (1)
45-68: Type assertion is brittle – Strawberry wraps scalars
field.type is expected_typewill fail if the schema later marks a field asOptional[int]or addsstrawberry.scalar. Safer:- assert field.type is expected_type + from typing import get_origin + actual = get_origin(field.type) or field.type + assert actual is expected_typeKeeps the test future-proof while still guarding against wrong base types.
backend/apps/owasp/graphql/queries/project_health_metrics.py (1)
17-36: Signature with 11 primitives – consider an input objectThe resolver now takes eleven optional scalars plus
limit, breaching the “five-arg” rule and making the API cumbersome to evolve. Expose a dedicated Strawberry input type (or dataclass) and accept a singlefiltersargument instead.@strawberry.input class ProjectHealthFilters: is_contributors_requirement_compliant: bool | None = None is_funding_requirements_compliant: bool | None = None has_no_recent_commits: bool | None = None has_recent_releases: bool | None = None is_leader_requirements_compliant: bool | None = None has_long_open_issues: bool | None = None has_long_unanswered_issues: bool | None = None has_long_unassigned_issues: bool | None = None has_low_score: bool = True @strawberry.field def unhealthy_projects(self, *, filters: ProjectHealthFilters = ProjectHealthFilters(), limit: int = 20) -> list[ProjectHealthMetricsNode]: ...🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 17-17: Too many arguments (11/5)
(R0913)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
backend/apps/owasp/graphql/nodes/project.py(2 hunks)backend/apps/owasp/graphql/nodes/project_health_metrics.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(2 hunks)backend/apps/owasp/graphql/queries/project_health_metrics.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_metrics_scores.py(2 hunks)backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py(1 hunks)backend/apps/owasp/migrations/0041_merge_20250613_0336.py(1 hunks)backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py(1 hunks)backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py(1 hunks)backend/apps/owasp/models/project.py(1 hunks)backend/apps/owasp/models/project_health_metrics.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_test.py(2 hunks)backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py(1 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py(3 hunks)backend/tests/apps/owasp/models/project_health_metrics_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
- backend/apps/owasp/graphql/queries/init.py
- backend/tests/apps/owasp/graphql/nodes/project_test.py
- backend/apps/owasp/models/project.py
- backend/tests/apps/owasp/models/project_health_metrics_test.py
- backend/apps/owasp/models/project_health_metrics.py
- backend/apps/owasp/graphql/nodes/project.py
- backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_scores_test.py
- backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
🧰 Additional context used
🧬 Code Graph Analysis (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
backend/apps/owasp/models/project_health_metrics.py (1)
ProjectHealthMetrics(10-125)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(24-55)backend/tests/apps/owasp/graphql/nodes/project_test.py (2)
test_meta_configuration(16-40)_get_field_by_name(42-45)
🪛 Pylint (3.3.7)
backend/apps/owasp/graphql/nodes/project_health_metrics.py
[error] 55-55: Instance of 'ProjectHealthMetricsNode' has no 'project' member
(E1101)
backend/apps/owasp/graphql/queries/project_health_metrics.py
[refactor] 17-17: Too many arguments (11/5)
(R0913)
[refactor] 13-13: Too few public methods (1/2)
(R0903)
backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0041_merge_20250613_0336.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0043_remove_projecthealthmetrics_has_recent_commits_and_more.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
[error] 39-39: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
backend/tests/apps/owasp/graphql/queries/project_health_metrics_test.py
[error] 19-19: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
[error] 27-27: Class 'ProjectHealthMetricsQuery' has no 'strawberry_definition' member
(E1101)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
backend/apps/owasp/migrations/0041_merge_20250613_0336.py (1)
6-12: Merge migration looks correct – no action required.🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 6-6: Too few public methods (0/2)
(R0903)
backend/apps/owasp/migrations/0038_projecthealthmetrics_has_long_open_issues_and_more.py (1)
11-32: LGTM – fields added with sane defaults and verbose names.
| operations = [ | ||
| migrations.RemoveField( | ||
| model_name="projecthealthmetrics", | ||
| name="has_no_recent_commits", | ||
| ), | ||
| migrations.AddField( | ||
| model_name="projecthealthmetrics", | ||
| name="has_recent_commits", | ||
| field=models.BooleanField(default=False, verbose_name="Has recent commits"), | ||
| ), | ||
| ] |
There was a problem hiding this comment.
Field swap drops data and triggers 2 full-table rewrites – replace with atomic rename + data copy
RemoveField(has_no_recent_commits) → AddField(has_recent_commits) discards every existing value and rewrites the table twice (once for the drop, once for the add). In production this will:
- Nullify historical metrics for thousands of rows.
- Generate avoidable downtime on large tables.
Prefer a single RenameField combined with a RunPython step that inverts the value, e.g.:
- migrations.RemoveField(
- model_name="projecthealthmetrics",
- name="has_no_recent_commits",
- ),
- migrations.AddField(
- model_name="projecthealthmetrics",
- name="has_recent_commits",
- field=models.BooleanField(default=False, verbose_name="Has recent commits"),
- ),
+ migrations.RenameField(
+ model_name="projecthealthmetrics",
+ old_name="has_no_recent_commits",
+ new_name="has_recent_commits",
+ ),
+ migrations.RunPython(
+ code=lambda apps, _: apps.get_model("owasp", "ProjectHealthMetrics")
+ .objects.update(has_recent_commits=models.F("has_recent_commits").bitwise_not()),
+ reverse_code=migrations.RunPython.noop,
+ )This keeps the data, performs one table rewrite, and documents intent.
📝 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.
| operations = [ | |
| migrations.RemoveField( | |
| model_name="projecthealthmetrics", | |
| name="has_no_recent_commits", | |
| ), | |
| migrations.AddField( | |
| model_name="projecthealthmetrics", | |
| name="has_recent_commits", | |
| field=models.BooleanField(default=False, verbose_name="Has recent commits"), | |
| ), | |
| ] | |
| operations = [ | |
| migrations.RenameField( | |
| model_name="projecthealthmetrics", | |
| old_name="has_no_recent_commits", | |
| new_name="has_recent_commits", | |
| ), | |
| migrations.RunPython( | |
| code=lambda apps, _: apps | |
| .get_model("owasp", "ProjectHealthMetrics") | |
| .objects | |
| .update(has_recent_commits=models.F("has_recent_commits").bitwise_not()), | |
| reverse_code=migrations.RunPython.noop, | |
| ), | |
| ] |
🤖 Prompt for AI Agents
In
backend/apps/owasp/migrations/0042_remove_projecthealthmetrics_has_no_recent_commits_and_more.py
around lines 11 to 21, the current migration removes the field
has_no_recent_commits and adds has_recent_commits, causing data loss and two
full-table rewrites. To fix this, replace the RemoveField and AddField with a
single RenameField operation to rename has_no_recent_commits to
has_recent_commits, then add a RunPython migration step that inverts the boolean
values to preserve the correct semantics. This approach keeps existing data
intact, reduces table rewrites to one, and clearly documents the data
transformation intent.
There was a problem hiding this comment.
I suggest to keep it simple while focusing on the working BE/FE PoC. For now we just need a project health metrics endpoint (I think having a separate endpoint would be more beneficial as it may be quite a lot of data -- a bunch of metric values for the recent 30 days).
Please also look into addressing the bot's comments.
| @strawberry.field | ||
| def age_days(self) -> int: | ||
| """Resolve project age in days.""" | ||
| return self.age_days | ||
|
|
||
| @strawberry.field | ||
| def last_commit_days(self) -> int: | ||
| """Resolve last commit age in days.""" | ||
| return self.last_commit_days | ||
|
|
||
| @strawberry.field | ||
| def last_pull_request_days(self) -> int: | ||
| """Resolve last pull request age in days.""" | ||
| return self.last_pull_request_days | ||
|
|
||
| @strawberry.field | ||
| def last_release_days(self) -> int: | ||
| """Resolve last release age in days.""" | ||
| return self.last_release_days | ||
|
|
There was a problem hiding this comment.
@ahmedxgouda have you checked if this suggestion is valid?
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (3)
1-5: Prefer explicit# pylint: disableover blanket suppression of metaprogramming warningsPyLint correctly complains about a missing
__strawberry_definition__attribute, because it does not understand Strawberry’s runtime patching.
Silently ignoring that hint is fine, but adding an explicit disable at import time makes the intent clear and avoids repeated noise in CI:-import pytest +# pylint: disable=no-member +import pytest
8-11:test_project_health_metrics_node_inheritancedoes not actually check inheritanceThe test only asserts the presence of the Strawberry definition attribute, not that the node inherits from a base class (e.g.
strawberry.relay.Nodeor a customBaseNode).
Either rename the test totest_has_strawberry_definitionor extend the assertion to cover the intended inheritance behaviour.
36-45: Micro-optimisation: avoid recreating the iterator for every test
__strawberry_definition__.fieldsis evaluated repeatedly.
Cache it once at module scope to save a tiny bit of work and keep the helper simpler:-FIELD_MAP = {f.name: f for f in ProjectHealthMetricsNode.__strawberry_definition__.fields} - -def _get_field_by_name(self, name): - return FIELD_MAP.get(name) +FIELD_MAP = {f.name: f for f in ProjectHealthMetricsNode.__strawberry_definition__.fields} + +def _get_field_by_name(self, name): + return FIELD_MAP.get(name)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py(1 hunks)backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/graphql/nodes/project_health_metrics.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py (2)
backend/apps/owasp/graphql/nodes/project_health_metrics.py (1)
ProjectHealthMetricsNode(25-51)backend/tests/apps/owasp/graphql/nodes/project_test.py (2)
test_meta_configuration(16-40)_get_field_by_name(42-45)
🪛 Pylint (3.3.7)
backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
[error] 40-40: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
⏰ 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)
| def test_meta_configuration(self): | ||
| field_names = { | ||
| field.name for field in ProjectHealthMetricsNode.__strawberry_definition__.fields | ||
| } | ||
| expected_field_names = { | ||
| "age_days", | ||
| "contributors_count", | ||
| "forks_count", | ||
| "is_funding_requirements_compliant", | ||
| "is_leader_requirements_compliant", | ||
| "last_commit_days", | ||
| "last_pull_request_days", | ||
| "last_release_days", | ||
| "open_issues_count", | ||
| "open_pull_requests_count", | ||
| "owasp_page_last_update_days", | ||
| "recent_releases_count", | ||
| "score", | ||
| "stars_count", | ||
| "unanswered_issues_count", | ||
| "unassigned_issues_count", | ||
| } | ||
| assert expected_field_names.issubset(field_names) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Keep assertion & parametrised lists in sync – score covered here but not below
score is verified to exist in expected_field_names but its type is never checked in the parametrised test later on.
Add it to the parametrised table (with the correct type, probably int or float) to avoid blind spots.
@@
"score",
@@
assert expected_field_names.issubset(field_names)Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Pylint (3.3.7)
[error] 14-14: Class 'ProjectHealthMetricsNode' has no 'strawberry_definition' member
(E1101)
🤖 Prompt for AI Agents
In backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py around
lines 12 to 34, the field "score" is included in the expected_field_names set
but missing from the parametrised test that checks field types. To fix this, add
"score" with its correct type (likely int or float) to the parametrised test
list to ensure its type is validated and keep the assertions consistent.
| @pytest.mark.parametrize( | ||
| ("field_name", "expected_type"), | ||
| [ | ||
| ("age_days", int), | ||
| ("contributors_count", int), | ||
| ("forks_count", int), | ||
| ("is_funding_requirements_compliant", bool), | ||
| ("is_leader_requirements_compliant", bool), | ||
| ("last_commit_days", int), | ||
| ("last_pull_request_days", int), | ||
| ("last_release_days", int), | ||
| ("open_issues_count", int), | ||
| ("open_pull_requests_count", int), | ||
| ("owasp_page_last_update_days", int), | ||
| ("stars_count", int), | ||
| ("recent_releases_count", int), | ||
| ("unanswered_issues_count", int), | ||
| ("unassigned_issues_count", int), | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing entry and drop duplication risk
See earlier note – add "score" with its expected python type.
@@
("stars_count", int),
("recent_releases_count", int),
+ ("score", int), # or float, depending on implementation
("unanswered_issues_count", int),
("unassigned_issues_count", int),📝 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.
| @pytest.mark.parametrize( | |
| ("field_name", "expected_type"), | |
| [ | |
| ("age_days", int), | |
| ("contributors_count", int), | |
| ("forks_count", int), | |
| ("is_funding_requirements_compliant", bool), | |
| ("is_leader_requirements_compliant", bool), | |
| ("last_commit_days", int), | |
| ("last_pull_request_days", int), | |
| ("last_release_days", int), | |
| ("open_issues_count", int), | |
| ("open_pull_requests_count", int), | |
| ("owasp_page_last_update_days", int), | |
| ("stars_count", int), | |
| ("recent_releases_count", int), | |
| ("unanswered_issues_count", int), | |
| ("unassigned_issues_count", int), | |
| ], | |
| ) | |
| @pytest.mark.parametrize( | |
| ("field_name", "expected_type"), | |
| [ | |
| ("age_days", int), | |
| ("contributors_count", int), | |
| ("forks_count", int), | |
| ("is_funding_requirements_compliant", bool), | |
| ("is_leader_requirements_compliant", bool), | |
| ("last_commit_days", int), | |
| ("last_pull_request_days", int), | |
| ("last_release_days", int), | |
| ("open_issues_count", int), | |
| ("open_pull_requests_count", int), | |
| ("owasp_page_last_update_days", int), | |
| ("stars_count", int), | |
| ("recent_releases_count", int), | |
| ("score", int), # or float, depending on implementation | |
| ("unanswered_issues_count", int), | |
| ("unassigned_issues_count", int), | |
| ], | |
| ) |
🤖 Prompt for AI Agents
In backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py around
lines 46 to 65, the parameterized test list is missing the "score" field and
risks duplication issues. Add the tuple ("score", float) or the appropriate
expected Python type for "score" to the list of field_name and expected_type
pairs to ensure complete test coverage and avoid duplication.
There was a problem hiding this comment.
Score could be None, so it is not possible to include within this test because the test checks if it is not None. What do you think? @arkid15r
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| def test_field_types(self, field_name, expected_type): | ||
| field = self._get_field_by_name(field_name) | ||
| assert field is not None | ||
| assert field.type is expected_type |
There was a problem hiding this comment.
🛠️ Refactor suggestion
is identity check can be brittle for Strawberry types
field.type may be wrapped in Annotated, ForwardRef, or Optional.
Using is risks false negatives. Compare structural equality instead:
- assert field.type is expected_type
+ from typing import get_origin
+
+ actual = get_origin(field.type) or field.type
+ assert actual is expected_type📝 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.
| def test_field_types(self, field_name, expected_type): | |
| field = self._get_field_by_name(field_name) | |
| assert field is not None | |
| assert field.type is expected_type | |
| def test_field_types(self, field_name, expected_type): | |
| field = self._get_field_by_name(field_name) | |
| assert field is not None | |
| from typing import get_origin | |
| actual = get_origin(field.type) or field.type | |
| assert actual is expected_type |
🤖 Prompt for AI Agents
In backend/tests/apps/owasp/graphql/nodes/project_health_metrics_test.py around
lines 66 to 69, the test uses 'is' to compare field.type and expected_type,
which can cause false negatives due to wrappers like Annotated, ForwardRef, or
Optional. Replace the identity check with a structural equality check, such as
using '==' or an appropriate type comparison method, to ensure the test
accurately verifies the field type regardless of such wrappers.
|
* Add project_health_metrics node and query * Add health field to the ProjectNode that represents sll ProjectHealthMetrics objects of the project * Add tests * Update filtering and add fields to models * Update filtering * Update tests * Save new boolean values * Add boolean mapping * Add query tests * Merge migrations * Update filtering, add migrations, and update scripts * Update tests and queries * Add test with filters * Update filtering * Update tests * Merge migrations * Revert unnecessary work and apply suggestions * Remove has_no_recent_commits from project * Add missing fields for FE query * Remove project name from the test * Clean migrations * Update code --------- Co-authored-by: Arkadii Yakovets <[email protected]>
Resolves #1539
ProjectNodethat returns the metrics of the project.ProjectHealthMetricsandProjectmodels with new fields and properties which will be helpful for filtering.