Add Project Health Dashboard models#904
Conversation
github-actions
bot
added
docs
Dishant1804
force-pushed
the
project_details
branch
from
13fac62 to
a2da45e
Compare
github-actions
bot
added
backend
and removed
docs
|
@arkid15r I have implemented the necessary requirements for the initial setup, let me know the changes required (if any) :) |
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
@Dishant1804 thanks for the PR!
I had looked at the code briefly (scrolled down to the very bottom) and didn't have any high-level comments/suggestions. It looks good so far and I'll take another more detailed look tomorrow.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Some naming suggestions to address:
backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py
Outdated
Show resolved
Hide resolved
Dishant1804
force-pushed
the
project_details
branch
from
c520f71 to
993cdf1
Compare
i have fixed it now :) |
Summary by CodeRabbit
WalkthroughThis pull request introduces new functionality for managing OWASP project health metrics. A management command is added to update project health requirements. Two new models, ProjectHealthMetrics and ProjectHealthRequirements, are implemented through both migrations and model files to capture various health metrics and thresholds. In addition, new test files are provided for the command and models, new Makefile targets are added for these commands, and the models are registered in the Django admin interface. Changes
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 review |
✅ Actions performedReview triggered.
|
![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 (10)
backend/tests/owasp/models/project_health_requirements_test.py (2)
40-41: Consider handling empty string case more robustly.The test for empty string level might not be a valid real-world scenario, as Django would likely validate against empty strings for choice fields. Consider either removing this test case or adding validation in the model to explicitly handle empty strings.
46-49: Consider using model defaults in tests.Instead of asserting that each field equals 0, consider creating a helper method that verifies the fields against their defined default values in the model. This would make the test more resilient to future changes in default values.
- @pytest.mark.parametrize("field", POSITIVE_INTEGER_FIELDS) - def test_positive_integer_fields_default_to_zero(self, field): - assert getattr(ProjectHealthRequirements(), field) == 0 + @pytest.mark.parametrize("field", POSITIVE_INTEGER_FIELDS) + def test_positive_integer_fields_default_values(self, field): + model_field = ProjectHealthRequirements._meta.get_field(field) + instance = ProjectHealthRequirements() + assert getattr(instance, field) == model_field.defaultbackend/tests/owasp/models/project_health_metrics_test.py (1)
14-14: Remove parentheses from@pytest.fixturefor style consistency.Static analysis (PT001) suggests using
@pytest.fixtureinstead of@pytest.fixture(). This is a minor style preference but will help maintain consistency with common Pytest patterns.- @pytest.fixture() + @pytest.fixture🧰 Tools
🪛 Ruff (0.8.2)
14-14: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py (3)
20-85: Consider a more flexible weighting approach.Hardcoding 15 conditions with an equal weight of
100 / 15may require frequent updates if more conditions are added later. A more dynamic weighting mechanism (e.g., storing and iterating over conditions in a list or dict) could simplify updates.
87-107: Retrieve latest PR date using a single query for performance gains.Instead of iterating over all repositories to find the latest PR, consider an aggregated approach (e.g., a MAX on
pull_requests__created_at) to reduce query overhead for larger datasets.
126-190: Consider pagination or batching for large project sets.When dealing with many projects, processing them in one pass might become expensive. Adding pagination or batching (e.g., processing N projects at a time) can improve scalability and reliability.
backend/tests/owasp/management/commands/owasp_update_project_health_metrics_test.py (1)
15-15: Simplify pytest fixture decorators.According to the static analysis hints and best practices for pytest, you can remove the empty parentheses from the fixture decorators.
- @pytest.fixture() + @pytest.fixtureAlso applies to: 19-19, 46-46
🧰 Tools
🪛 Ruff (0.8.2)
15-15: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py (2)
113-135: Consider adding default value validation.The method correctly merges default values with level-specific requirements, but there's no validation to ensure that the values make logical sense (e.g., ensuring that time windows aren't negative).
You could add validation to ensure that critical values are within reasonable bounds before returning them:
def get_level_requirements(self, level): """Get default requirements based on project level.""" defaults = { "contributors_count": 1, "age_days": 0, "forks_count": 0, "last_release_days": 0, "last_commit_days": 1, "open_issues_count": 0, "open_pull_requests_count": 0, "owasp_page_last_update_days": 0, "last_pull_request_days": 0, "recent_releases_count": 0, "recent_releases_time_window_days": 0, "stars_count": 0, "total_pull_requests_count": 0, "total_releases_count": 0, "unanswered_issues_count": 0, "unassigned_issues_count": 0, } defaults.update(self.level_requirements.get(level, {})) + # Ensure critical values aren't negative + for key, value in defaults.items(): + if key.endswith('_days') or key.endswith('_count'): + defaults[key] = max(0, value) + return defaults
137-160: Consider adding more detailed reporting.The command does a good job of handling both single-level and all-level updates, but the output could be more informative about what specific values were updated.
You could enhance the output to show which values were updated, especially when requirements already exist:
action = "Created" if created else "Updated" - print(f"{action} health requirements for {requirements.get_level_display()} projects") + print(f"{action} health requirements for {requirements.get_level_display()} projects:") + if not created: + # Show what fields were updated + for field, value in defaults.items(): + print(f" - {field}: {value}")backend/apps/owasp/migrations/0016_projecthealthmetrics.py (1)
35-45: Consider adding detailed help text for compliance fields.The boolean fields for compliance requirements have verbose names but lack help text explaining what constitutes compliance for these fields.
Adding help text would make it clearer for developers and administrators what these fields represent:
"is_funding_requirements_compliant", models.BooleanField( - default=False, verbose_name="Is funding requirements compliant" + default=False, + verbose_name="Is funding requirements compliant", + help_text="Indicates whether the project meets all funding requirements" ), ), ( "is_project_leaders_requirements_compliant", models.BooleanField( - default=False, verbose_name="Is project leaders requirements compliant" + default=False, + verbose_name="Is project leaders requirements compliant", + help_text="Indicates whether the project meets all leadership requirements" ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py(1 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py(1 hunks)backend/apps/owasp/migrations/0016_projecthealthmetrics.py(1 hunks)backend/apps/owasp/migrations/0017_projecthealthrequirements_and_more.py(1 hunks)backend/apps/owasp/models/project_health_metrics.py(1 hunks)backend/apps/owasp/models/project_health_requirements.py(1 hunks)backend/tests/owasp/management/commands/owasp_update_project_health_metrics_test.py(1 hunks)backend/tests/owasp/management/commands/owasp_update_project_health_requirements_test.py(1 hunks)backend/tests/owasp/models/project_health_metrics_test.py(1 hunks)backend/tests/owasp/models/project_health_requirements_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/tests/owasp/models/project_health_metrics_test.py
14-14: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
backend/tests/owasp/management/commands/owasp_update_project_health_metrics_test.py
15-15: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
19-19: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
46-46: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
🔇 Additional comments (16)
backend/apps/owasp/migrations/0017_projecthealthrequirements_and_more.py (1)
1-106: Migration looks well-structured and complete.This migration creates a new
ProjectHealthRequirementsmodel with appropriate fields and constraints, while removing the previousProjectHealthMetricsmodel. The field definitions, constraints, and meta options are all properly configured.backend/apps/owasp/models/project_health_requirements.py (1)
9-58: Model implementation matches migration and follows Django best practices.The
ProjectHealthRequirementsmodel is well-structured with appropriate field definitions, meta options, and a helpful string representation method. The model reuses the project level choices from theProjectmodel and follows consistent naming conventions.backend/tests/owasp/models/project_health_requirements_test.py (1)
8-61: Test coverage looks comprehensive.The test class provides good coverage of the model's functionality, including string representation, default values, and validation for both valid and invalid level choices.
backend/apps/owasp/models/project_health_metrics.py (1)
9-68: Model implementation looks solid, despite being scheduled for removal.This model is well-structured with appropriate field definitions and relationships. The code follows Django best practices with good documentation and a helpful string representation method. Note that according to the migration file, this model is being deleted and replaced by the new
ProjectHealthRequirementsmodel.backend/tests/owasp/models/project_health_metrics_test.py (3)
21-33: Great use of parameterization for string representation testing.These tests comprehensively validate how various project names affect the model’s
__str__output, ensuring robust coverage of edge cases (e.g., empty string, None).
35-57: Thorough validation checks for boundary conditions.The score validation tests effectively cover minimum, maximum, and out-of-range values, as well as
None. This ensures the model correctly raisesValidationErrorfor invalid input.
63-84: Comprehensive testing of default field values.These parameterized tests confirm that the default counts and boolean fields initialize as expected, reducing the likelihood of unnoticed regressions.
backend/tests/owasp/management/commands/owasp_update_project_health_requirements_test.py (4)
14-24: Autouse fixture initialization is well-structured.This approach ensures each test begins with a clean and consistent environment. No issues found here.
25-54: Good coverage for successful requirements creation scenarios.Each parameterized level is tested, verifying the correct success messages and ensuring the command's behavior aligns with user expectations.
55-64: Effective exception handling test.Simulating a database error confirms that the command raises a
CommandErrorwith the expected message. This adds reliability to the management command’s robustness.
65-100: Comprehensive testing of level requirements generation.Verifying that each project level yields the correct default requirements helps maintain consistency in the command’s logic.
backend/tests/owasp/management/commands/owasp_update_project_health_metrics_test.py (3)
67-86: LGTM! Well-structured test parameterization.The parameterized test setup looks good. You're testing various offset and project count combinations, which will help ensure the command handles pagination correctly.
90-135: Good test implementation with proper mocking and assertions.The test thoroughly verifies the command's functionality by:
- Mocking all necessary dependencies
- Capturing standard output to verify progress messages
- Checking that bulk_update is called with the correct fields
- Restoring stdout properly in a finally block
This comprehensive approach ensures the command behaves as expected under normal conditions.
136-155: LGTM! Good error handling test.The exception handling test is well-implemented with parameterized error messages and proper verification that exceptions are propagated correctly.
backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py (1)
12-103: LGTM! Well-structured requirements dictionary.The requirements for each project level are clearly defined and organized in a dictionary with project levels as keys, as suggested in the previous review comments.
backend/apps/owasp/migrations/0016_projecthealthmetrics.py (1)
8-124: LGTM! Well-structured model creation migration.The migration creates a comprehensive ProjectHealthMetrics model with appropriate field types, constraints, and relationships. The model has:
- Standard auto-ID and timestamp fields
- Proper validators for the score field (0-100 range)
- Clear verbose names for all fields
- A OneToOne relationship with the Project model
- Appropriate model options including table name and verbose name
This provides a solid foundation for tracking project health metrics.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
I removed some unusable code and updated what looked good. Please consider changing your approach to contributing process.
backend/apps/owasp/migrations/0017_projecthealthrequirements_and_more.py
Outdated
Show resolved
Hide resolved
backend/apps/owasp/management/commands/owasp_update_project_health_metrics.py
Outdated
Show resolved
Hide resolved
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
backend/apps/owasp/admin.py (1)
146-147: Consider adding custom admin classes for the new models.The new models are registered with the default Django admin interface. For consistency with other models in this file (like ProjectAdmin) and better usability, consider implementing custom admin classes with appropriate list_display, list_filter, and search_fields.
+class ProjectHealthMetricsAdmin(admin.ModelAdmin): + list_display = ("project", "nest_updated_at", "has_recent_commits", "has_recent_releases") + list_filter = ("has_recent_commits", "has_recent_releases") + search_fields = ("project__name",) + +class ProjectHealthRequirementsAdmin(admin.ModelAdmin): + list_display = ("level", "contributors_count", "stars_count", "nest_updated_at") + list_filter = ("level",) + -admin.site.register(ProjectHealthMetrics) -admin.site.register(ProjectHealthRequirements) +admin.site.register(ProjectHealthMetrics, ProjectHealthMetricsAdmin) +admin.site.register(ProjectHealthRequirements, ProjectHealthRequirementsAdmin)backend/Makefile (2)
81-83: Fix the echo message to match the command's purpose.The echo message states "Updating OWASP project health requirements" but the command is
owasp_update_project_health_metrics. These should be consistent to avoid confusion.- @echo "Updating OWASP project health requirements" + @echo "Updating OWASP project health metrics"
85-87: Fix the echo message to match the command's purpose.The echo message states "Updating OWASP project health metrics" but the command is
owasp_update_project_health_requirements. These should be consistent to avoid confusion.- @echo "Updating OWASP project health metrics" + @echo "Updating OWASP project health requirements"backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py (2)
95-117: Enhance docstring for get_level_requirements method.The current docstring could be improved to clarify what the method returns.
- def get_level_requirements(self, level): - """Get default requirements based on project level.""" + def get_level_requirements(self, level): + """ + Get default requirements based on project level. + + Args: + level: The project level code (e.g., 'incubator', 'lab', etc.) + + Returns: + dict: Dictionary containing requirement threshold values for the specified level + """
118-141: Improve consistency in the handle method output messages.When no specific level is provided, the command prints different messages for creation and existence, but not for updating existing requirements. Consider making the messages consistent with the behavior when a specific level is provided.
requirements, created = ProjectHealthRequirements.objects.get_or_create( level=level_code, defaults=defaults ) + if not created: + # Update the existing record with the latest requirements + for key, value in defaults.items(): + setattr(requirements, key, value) + requirements.save() - if created: - print(f"Created default health requirements for {level_choice[1]} projects") - else: - print(f"Health requirements already exist for {level_choice[1]} projects") + action = "Created" if created else "Updated" + print(f"{action} health requirements for {level_choice[1]} projects")
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
backend/Makefile(1 hunks)backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py(1 hunks)backend/apps/owasp/migrations/0017_projecthealthrequirements_and_more.py(1 hunks)backend/tests/owasp/management/commands/owasp_update_project_health_requirements_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/tests/owasp/management/commands/owasp_update_project_health_requirements_test.py
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend unit tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
🔇 Additional comments (3)
backend/apps/owasp/admin.py (1)
10-11: Imports are properly structured.The new model imports are correctly positioned with the other model imports.
backend/apps/owasp/migrations/0017_projecthealthrequirements_and_more.py (1)
6-104: Migration looks good.The migration creates the ProjectHealthRequirements model with appropriate fields, constraints, and meta options. The level field is properly defined with choices and uniqueness constraint.
backend/apps/owasp/management/commands/owasp_update_project_health_requirements.py (1)
12-85: LEVEL_REQUIREMENTS dictionary is well-structured.The requirements dictionary is organized by project level with appropriate threshold values for each metric. The structure follows the recommendation from previous review comments to use a dictionary with levels as keys.
| if level: | ||
| defaults = self.get_level_requirements(level) | ||
| requirements, created = ProjectHealthRequirements.objects.get_or_create( | ||
| level=level, defaults=defaults | ||
| ) | ||
|
|
||
| action = "Created" if created else "Updated" | ||
| print(f"{action} health requirements for {requirements.get_level_display()} projects") |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider updating existing requirements.
The command checks if requirements exist, but it doesn't update them if the values in LEVEL_REQUIREMENTS have changed. Consider adding logic to update existing records with the latest requirement values.
if level:
defaults = self.get_level_requirements(level)
requirements, created = ProjectHealthRequirements.objects.get_or_create(
level=level, defaults=defaults
)
+
+ if not created:
+ # Update the existing record with the latest requirements
+ for key, value in defaults.items():
+ setattr(requirements, key, value)
+ requirements.save()
action = "Created" if created else "Updated"
print(f"{action} health requirements for {requirements.get_level_display()} projects")📝 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.
| if level: | |
| defaults = self.get_level_requirements(level) | |
| requirements, created = ProjectHealthRequirements.objects.get_or_create( | |
| level=level, defaults=defaults | |
| ) | |
| action = "Created" if created else "Updated" | |
| print(f"{action} health requirements for {requirements.get_level_display()} projects") | |
| if level: | |
| defaults = self.get_level_requirements(level) | |
| requirements, created = ProjectHealthRequirements.objects.get_or_create( | |
| level=level, defaults=defaults | |
| ) | |
| if not created: | |
| # Update the existing record with the latest requirements | |
| for key, value in defaults.items(): | |
| setattr(requirements, key, value) | |
| requirements.save() | |
| action = "Created" if created else "Updated" | |
| print(f"{action} health requirements for {requirements.get_level_display()} projects") |
* projectHealthMetrics, projectHealthRequirements models along with respective commands and tests * project health/requirements setup * timezone from django utils * checks and lints * suggestions implemented * recreated migrations * permissons to migration files * checks and lints * Update code --------- Co-authored-by: Arkadii Yakovets <2201626+arkid15r@users.noreply.github.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
2 participants
Resolves #798
Implemented initial project health/requirements along with commands and respective test cases