Add health score as a ranking factor. #1667
Conversation
…pe outside the Project Class to avoid circular imports
Summary by CodeRabbit
WalkthroughThe changes introduce two new enums, Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes were found. All modifications are directly related to the centralization of enum usage and the integration of the health score into the project index, as required by the linked issue. ✨ 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: 0
🧹 Nitpick comments (1)
backend/apps/owasp/models/project.py (1)
112-116: Consider performance implications of the health_score property.The property implementation is correct but could cause performance issues due to the database query in
last_health_metrics. Consider addingselect_relatedorprefetch_relatedwhen this property is accessed frequently.Consider caching or optimizing this property:
@property def health_score(self) -> float | None: """Return project health score.""" - return self.last_health_metrics.score if self.last_health_metrics else None + last_metrics = self.last_health_metrics + return last_metrics.score if last_metrics else None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/apps/owasp/index/project.py(2 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py(5 hunks)backend/apps/owasp/models/enums.py(1 hunks)backend/apps/owasp/models/mixins/project.py(1 hunks)backend/apps/owasp/models/project.py(5 hunks)backend/apps/owasp/models/project_health_requirements.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_requirements_test.py(2 hunks)backend/tests/apps/owasp/models/project_health_requirements_test.py(2 hunks)backend/tests/apps/owasp/models/project_test.py(5 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
backend/apps/owasp/models/enums.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
[refactor] 27-27: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (21)
backend/apps/owasp/models/mixins/project.py (1)
100-103: ```shell
#!/bin/bashSearch for health_score occurrences across the repository
rg -n 'health_score' -C 3
</details> <details> <summary>backend/tests/apps/owasp/management/commands/owasp_update_project_health_requirements_test.py (2)</summary> `9-9`: **LGTM - consistent enum import refactoring.** The import change from `Project` to `ProjectLevel` aligns with the centralization of enums to prevent circular imports. --- `64-68`: **LGTM - updated enum references consistently.** All references to `Project.ProjectLevel.*` have been correctly updated to use `ProjectLevel.*` directly, maintaining the same test functionality. </details> <details> <summary>backend/apps/owasp/models/project_health_requirements.py (2)</summary> `6-6`: **LGTM - centralized enum import.** The import change from `Project` to `ProjectLevel` supports the enum centralization effort to prevent circular imports. --- `19-19`: **LGTM - updated choices reference.** The choices reference has been correctly updated to use `ProjectLevel.choices` directly, maintaining the same field validation behavior. </details> <details> <summary>backend/tests/apps/owasp/models/project_health_requirements_test.py (3)</summary> `4-4`: **LGTM - consistent enum import refactoring.** The import change supports the centralization of enums to prevent circular imports. --- `11-11`: **LGTM - updated enum values reference.** The reference to `ProjectLevel.values` maintains the same test functionality with the new enum structure. --- `35-39`: **LGTM - updated test parameters consistently.** All parameterized test values have been correctly updated to use `ProjectLevel.*` directly, maintaining the same test coverage. </details> <details> <summary>backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py (3)</summary> `5-5`: **LGTM - centralized enum import.** The import change from `Project` to `ProjectLevel` aligns with the enum centralization effort. --- `13-13`: **LGTM - updated dictionary keys consistently.** All dictionary keys in `LEVEL_REQUIREMENTS` have been correctly updated to use `ProjectLevel.*` directly, maintaining the same configuration structure. Also applies to: 33-33, 53-53, 73-73 --- `123-123`: **LGTM - updated choices iteration.** The iteration over `ProjectLevel.choices` maintains the same command functionality with the new enum structure. </details> <details> <summary>backend/tests/apps/owasp/models/project_test.py (3)</summary> `7-7`: **LGTM! Clean enum import refactoring.** The import statement correctly references the new centralized enums module. --- `40-43`: **LGTM! Consistent enum usage in parameterized tests.** All parameterized test values have been correctly updated to use the imported enums instead of the nested model enums. Also applies to: 52-55, 64-67 --- `131-132`: **LGTM! Assertions updated to use imported enums.** The test assertions correctly use the imported enums, maintaining test logic while using the new centralized enum definitions. </details> <details> <summary>backend/apps/owasp/index/project.py (2)</summary> `52-52`: **LGTM! Health score added to ranking with appropriate priority.** The health score is positioned logically in the custom ranking - after project level but before stars count, which makes sense for project ranking priorities. --- `30-30`: I couldn’t locate the `idx_health_score` definition with the previous pattern. Let’s search more broadly: ```shell #!/bin/bash # 1) Locate the ProjectIndexMixin class rg -n "class ProjectIndexMixin" . # 2) Search for any idx_health_score occurrences (method, property, or attribute) rg -n "idx_health_score" .backend/apps/owasp/models/enums.py (1)
1-35: LGTM! Well-defined enums with comprehensive documentation.The enum definitions are clear, well-documented, and appropriately categorize OWASP project types and levels. The detailed comments for ProjectType provide valuable context about each category's purpose.
The static analysis warnings about "too few public methods" are false positives - enums don't need public methods beyond their choices.
backend/apps/owasp/models/project.py (4)
20-20: LGTM! Clean import updates.The imports correctly reference the new centralized enums module and the ProjectHealthMetrics model.
Also applies to: 23-23
120-120: LGTM! Enum references updated correctly.All type checking properties now correctly use the imported enums instead of the nested model enums.
Also applies to: 125-125, 142-142
294-299: LGTM! from_github method updated with correct enum usage.The enum references in the
from_githubmethod have been correctly updated to use the imported enums. The logic for mapping project levels and types remains intact.Also applies to: 305-305
168-177: ```shell
#!/bin/bashLocate the ProjectHealthMetrics model class and inspect its 'project' field definition
rg -n "class ProjectHealthMetrics" .
rg -n -C3 "project\s*=" $(rg -l "class ProjectHealthMetrics" .)</details> </blockquote></details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
aramattamara
left a comment
aramattamara
left a comment
There was a problem hiding this comment.
LGTM! With one minor comment.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
backend/apps/owasp/models/enums/project.py (1)
1-2: Optionally expose the enums via__all__Explicitly listing public symbols helps static analysers and prevents accidental re-export of unrelated globals.
+__all__ = ["ProjectType", "ProjectLevel"]Also applies to: 35-35
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/apps/owasp/index/project.py(2 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py(5 hunks)backend/apps/owasp/models/enums/project.py(1 hunks)backend/apps/owasp/models/mixins/project.py(2 hunks)backend/apps/owasp/models/project.py(5 hunks)backend/apps/owasp/models/project_health_requirements.py(2 hunks)backend/tests/apps/owasp/management/commands/owasp_update_project_health_requirements_test.py(2 hunks)backend/tests/apps/owasp/models/project_health_requirements_test.py(2 hunks)backend/tests/apps/owasp/models/project_test.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- backend/tests/apps/owasp/management/commands/owasp_update_project_health_requirements_test.py
- backend/apps/owasp/models/project_health_requirements.py
- backend/tests/apps/owasp/models/project_test.py
- backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py
- backend/tests/apps/owasp/models/project_health_requirements_test.py
- backend/apps/owasp/models/mixins/project.py
- backend/apps/owasp/models/project.py
- backend/apps/owasp/index/project.py
🧰 Additional context used
🧠 Learnings (1)
backend/apps/owasp/models/enums/project.py (1)
Learnt from: yashgoyal0110
PR: OWASP/Nest#0
File: :0-0
Timestamp: 2025-04-06T06:57:42.144Z
Learning: In the OWASP/Nest project, type annotations have been added to Python functions using the -> notation to specify return types (e.g., -> None, -> str) and parameter typing.
🪛 Pylint (3.3.7)
backend/apps/owasp/models/enums/project.py
[refactor] 6-6: Too few public methods (0/2)
(R0903)
[refactor] 27-27: Too few public methods (0/2)
(R0903)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
| CODE = "code", "Code" | ||
|
|
||
| # These projects seek to communicate information or raise awareness about a topic in | ||
| # application security. Note that documentation projects should focus on an online-first | ||
| # deliverable, where appropriate, but can take any media form. | ||
| DOCUMENTATION = "documentation", "Documentation" | ||
|
|
||
| # Some projects fall outside the above categories. Most are created to offer OWASP | ||
| # operational support. | ||
| OTHER = "other", "Other" | ||
|
|
||
| # These are typically software or utilities that help developers and security | ||
| # professionals test, secure, or monitor applications. | ||
| TOOL = "tool", "Tool" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Wrap enum labels in _() for internationalisation
The second element of each TextChoices tuple is shown in forms / admin and should be localisable.
Without _() they’ll be extracted as plain strings, leaving them untranslated for non-English users.
- CODE = "code", "Code"
+ CODE = "code", _("Code")
...
- DOCUMENTATION = "documentation", "Documentation"
+ DOCUMENTATION = "documentation", _("Documentation")
...
- OTHER = "other", "Other"
+ OTHER = "other", _("Other")
...
- TOOL = "tool", "Tool"
+ TOOL = "tool", _("Tool")
@@
- OTHER = "other", "Other"
- INCUBATOR = "incubator", "Incubator"
- LAB = "lab", "Lab"
- PRODUCTION = "production", "Production"
- FLAGSHIP = "flagship", "Flagship"
+ OTHER = "other", _("Other")
+ INCUBATOR = "incubator", _("Incubator")
+ LAB = "lab", _("Lab")
+ PRODUCTION = "production", _("Production")
+ FLAGSHIP = "flagship", _("Flagship")Also applies to: 30-34
🤖 Prompt for AI Agents
In backend/apps/owasp/models/enums/project.py around lines 11 to 24 and also
lines 30 to 34, the second element of each TextChoices tuple is not wrapped in
the _() function for internationalization. To fix this, import the _ function
from django.utils.translation and wrap each label string (the second element in
the tuple) with _() to enable translation in forms and admin interfaces.
| from django.db.models import TextChoices | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add gettext_lazy import for translatable labels
Django convention is to wrap human-readable enum labels in gettext_lazy so they can be picked up by the i18n tooling.
Importing it here avoids repeating the import in every consumer that needs translations.
-from django.db.models import TextChoices
+from django.db.models import TextChoices
+from django.utils.translation import gettext_lazy as _📝 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.
| from django.db.models import TextChoices | |
| from django.db.models import TextChoices | |
| from django.utils.translation import gettext_lazy as _ |
🤖 Prompt for AI Agents
In backend/apps/owasp/models/enums/project.py around lines 3 to 4, the
gettext_lazy function from django.utils.translation is not imported, which is
needed to wrap human-readable enum labels for translation. Add the import
statement for gettext_lazy at the top of the file to enable wrapping enum labels
for i18n support and avoid repeated imports in consumers.
3 participants
Resolves #1612
I had to move the
ProjectLevelandProjectTypeoutside theProjectmodel to avoid circular imports when importingProjectHealthMetricsin theProjectmodel. This is because theProjectmodel already imported in theProjectHealthRequirementsmodel which is imported in theProjectHealthMetricsmodel