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
Show resolved
Hide resolved
backend/tests/apps/owasp/management/commands/owasp_update_project_health_metrics_test.py
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.
| @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
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
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
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_update_project_health_metrics_scores.py
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.
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]>
* Implemented Authentication using nextauth (#1512) * implemented authentication using next-auth * update code * type fix * updated migration * added backend test cases * added frontend unit test cases * added e2e test case * pre-commit * fixes e2e test cases * updated ci/cd * updated code * upgraded mutaitons from graphene to strawberry * updated code * Update code * Update tests * fixes * fix test * added relation * Update code * Update pnpm-lock.yaml --------- Co-authored-by: Kate Golovanova <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> * Run make update * Bump python from 3.13.3-alpine to 3.13.4-alpine in /backend/docker (#1556) Bumps python from 3.13.3-alpine to 3.13.4-alpine. --- updated-dependencies: - dependency-name: python dependency-version: 3.13.4-alpine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump python from 3.13.3-alpine to 3.13.4-alpine in /schema/docker (#1557) Bumps python from 3.13.3-alpine to 3.13.4-alpine. --- updated-dependencies: - dependency-name: python dependency-version: 3.13.4-alpine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump python from 3.13.3-alpine to 3.13.4-alpine in /docs/docker (#1559) Bumps python from 3.13.3-alpine to 3.13.4-alpine. --- updated-dependencies: - dependency-name: python dependency-version: 3.13.4-alpine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Run make update * docs: add Next.js to tech stack after migration (#1565) * Update CONTRIBUTING.md * Update CONTRIBUTING.md --------- Co-authored-by: Arkadii Yakovets <[email protected]> * Update event sync process: fix KeyError 'start-date' * Run make update * Add test coverage for `csrf.py` (#1564) * Add test coverage for csrf.py Signed-off-by: bandhan-majumder <[email protected]> * Update code --------- Signed-off-by: bandhan-majumder <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> * Update frontend/pnpm-lock.yaml * Fix Authentication related bugs (#1569) * handle empty auth credentials * update test cases * upgrade code * update code * remove check route * fix test case * fixes and update usermenu --------- Co-authored-by: Arkadii Yakovets <[email protected]> * Merge main * Migrate frontend checks to local environment * Update login page route (#1603) * fix route * format fix * introduce flag for auth * update env * changed default value * fix test cases * fix e2 test cases * Add dynamic variable for isAuthEnabled * Clean up * Clean up and fix tests * Update code * Fix code quality issues --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Kate <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> * Implement GraphQL resolvers for project health metrics (#1577) * 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]> * Fix test cases and update code (#1635) * update code * fix test case * Update middleware.test.ts * Update code * Update code * Update docker configuration * Update deps --------- Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: bandhan-majumder <[email protected]> Co-authored-by: Raj gupta <[email protected]> Co-authored-by: Kate Golovanova <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: RISHIRAJ MUKHERJEE <[email protected]> Co-authored-by: Bandhan Majumder <[email protected]> Co-authored-by: Ahmed Gouda <[email protected]>
* Implemented Authentication using nextauth (#1512) * implemented authentication using next-auth * update code * type fix * updated migration * added backend test cases * added frontend unit test cases * added e2e test case * pre-commit * fixes e2e test cases * updated ci/cd * updated code * upgraded mutaitons from graphene to strawberry * updated code * Update code * Update tests * fixes * fix test * added relation * Update code * Update pnpm-lock.yaml --------- Co-authored-by: Kate Golovanova <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> * Run make update * Bump python from 3.13.3-alpine to 3.13.4-alpine in /backend/docker (#1556) Bumps python from 3.13.3-alpine to 3.13.4-alpine. --- updated-dependencies: - dependency-name: python dependency-version: 3.13.4-alpine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump python from 3.13.3-alpine to 3.13.4-alpine in /schema/docker (#1557) Bumps python from 3.13.3-alpine to 3.13.4-alpine. --- updated-dependencies: - dependency-name: python dependency-version: 3.13.4-alpine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Bump python from 3.13.3-alpine to 3.13.4-alpine in /docs/docker (#1559) Bumps python from 3.13.3-alpine to 3.13.4-alpine. --- updated-dependencies: - dependency-name: python dependency-version: 3.13.4-alpine dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * Run make update * docs: add Next.js to tech stack after migration (#1565) * Update CONTRIBUTING.md * Update CONTRIBUTING.md --------- Co-authored-by: Arkadii Yakovets <[email protected]> * Update event sync process: fix KeyError 'start-date' * Run make update * Add test coverage for `csrf.py` (#1564) * Add test coverage for csrf.py Signed-off-by: bandhan-majumder <[email protected]> * Update code --------- Signed-off-by: bandhan-majumder <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> * Update frontend/pnpm-lock.yaml * Fix Authentication related bugs (#1569) * handle empty auth credentials * update test cases * upgrade code * update code * remove check route * fix test case * fixes and update usermenu --------- Co-authored-by: Arkadii Yakovets <[email protected]> * setup mentorship app * created mentor model * created mentee model * created program model * created module model and update relations * updated fields and remove unnecessary migrations * format fix * use through model * cspell update * format fix * Merge main * Migrate frontend checks to local environment * Update login page route (#1603) * fix route * format fix * introduce flag for auth * update env * changed default value * fix test cases * fix e2 test cases * Add dynamic variable for isAuthEnabled * Clean up * Clean up and fix tests * Update code * Fix code quality issues --------- Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Kate <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> * Implement GraphQL resolvers for project health metrics (#1577) * 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]> * update models and add enrollment model * Fix test cases and update code (#1635) * update code * fix test case * Update middleware.test.ts * Update code * Update code * fixes * updated suggestion * fix format * Update code * Update code * Restore lock files * Reformat migration * Update code * Update code --------- Signed-off-by: dependabot[bot] <[email protected]> Signed-off-by: bandhan-majumder <[email protected]> Co-authored-by: Kate Golovanova <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: Arkadii Yakovets <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: RISHIRAJ MUKHERJEE <[email protected]> Co-authored-by: Bandhan Majumder <[email protected]> Co-authored-by: Ahmed Gouda <[email protected]>
2 participants
Resolves #1539
ProjectNodethat returns the metrics of the project.ProjectHealthMetricsandProjectmodels with new fields and properties which will be helpful for filtering.