commit #977
commit #977
Conversation
Summary by CodeRabbit
WalkthroughThis pull request updates snapshot handling across multiple layers of the application. It enhances the admin interface by adding a "title" display and search capability, introduces new GraphQL types and queries for snapshots, and updates the underlying Snapshot model with new fields and a custom save method. The migration file is updated to support these schema changes, and corresponding tests and frontend queries are added to cover the new functionality. 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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
backend/tests/owasp/models/snapshot_test.py (1)
32-36: Consider adding more comprehensive tests for the save methodWhile this test verifies the basic attributes, it would be beneficial to add tests that specifically verify the custom
savelogic that automatically sets the key if it's not provided.def test_auto_key_generation(self): """Test that key is automatically generated if not provided.""" snapshot = MagicMock(spec=Snapshot) snapshot.key = None # Call the real save method on our mock Snapshot.save(snapshot, *[], **{}) # Verify key was set with the expected format assert snapshot.key is not None assert len(snapshot.key) == 7 assert "-" in snapshot.keybackend/apps/owasp/models/snapshot.py (1)
21-21: Consider adding format validation for the key fieldWhile the comment specifies the key format as "YYYY-mm", there's no validation to ensure the key follows this format when manually set. Consider adding a validator or using a regex pattern to enforce the format.
-key = models.CharField(max_length=7, unique=True) # Format: YYYY-mm +from django.core.validators import RegexValidator + +key = models.CharField( + max_length=7, + unique=True, + validators=[ + RegexValidator( + regex=r"^\d{4}-\d{2}$", + message="Key must be in the format YYYY-mm", + ) + ], +) # Format: YYYY-mmbackend/apps/owasp/graphql/queries/snapshot.py (1)
30-32: Consider adding pagination support for better scalabilityWhile the current implementation works well for a small number of snapshots, as the application grows, true pagination with cursor-based navigation would be more efficient than slicing.
- def resolve_recent_snapshots(root, info, limit): - """Resolve recent snapshots.""" - return Snapshot.objects.order_by("-created_at")[:limit] + def resolve_recent_snapshots(root, info, limit, cursor=None): + """Resolve recent snapshots with optional cursor-based pagination.""" + queryset = Snapshot.objects.order_by("-created_at") + if cursor: + # Implement cursor-based pagination + queryset = queryset.filter(created_at__lt=cursor) + return queryset[:limit]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
backend/apps/owasp/admin.py(2 hunks)backend/apps/owasp/graphql/nodes/snapshot.py(1 hunks)backend/apps/owasp/graphql/queries/__init__.py(1 hunks)backend/apps/owasp/graphql/queries/snapshot.py(1 hunks)backend/apps/owasp/migrations/0015_snapshot.py(2 hunks)backend/apps/owasp/models/snapshot.py(3 hunks)backend/tests/owasp/models/snapshot_test.py(2 hunks)frontend/src/api/queries/snapshotQueries.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (18)
backend/tests/owasp/models/snapshot_test.py (2)
15-16: Good addition of test setup variables for new fields!The new attributes (
titleandkey) have been appropriately added to the mock Snapshot object, which aligns with the model changes.
33-36: Great test coverage for new attributes!The new test method correctly verifies that the title and key attributes are properly assigned to the mock Snapshot object.
backend/apps/owasp/admin.py (2)
124-125: Good addition to admin list display!Adding the title field to the list display enhances the usability of the admin interface.
137-140: Appropriate search fields added!Adding both title and key fields to search_fields allows admins to efficiently find snapshots.
backend/apps/owasp/graphql/queries/__init__.py (2)
6-6: Good addition of SnapshotQuery import!This import is necessary for including snapshot query functionality in GraphQL.
10-10: Correctly updated OwaspQuery class!The OwaspQuery class now properly includes SnapshotQuery, which will make snapshot data available through the GraphQL API.
backend/apps/owasp/models/snapshot.py (3)
4-4: Appropriate timezone import!The
now()function from django.utils.timezone is correctly imported for generating the key field.
20-21: Good field additions to the model!Both title and key fields are well-defined with appropriate constraints. The unique constraint on key ensures we don't have duplicate snapshot keys.
38-42: Well-implemented save method!The method correctly sets the key field using the current date if not already provided.
frontend/src/api/queries/snapshotQueries.ts (1)
3-40: Well-structured GraphQL query implementationThe GraphQL query is well-designed, providing comprehensive access to snapshot data including nested entities. The structure follows GraphQL best practices with appropriate field selection.
backend/apps/owasp/graphql/queries/snapshot.py (3)
13-16: LGTM: Well-defined snapshot field with proper type safetyThe GraphQL field is correctly defined with a required key parameter, ensuring type safety.
18-21: Appropriate implementation of paginated snapshot listThe recent_snapshots field is well-implemented with a sensible default limit of 8 items.
23-28: Error handling follows best practicesThe resolver properly handles the DoesNotExist exception and returns None, which is the appropriate GraphQL response for a missing entity.
backend/apps/owasp/graphql/nodes/snapshot.py (3)
13-16: Good practice using constants for limit valuesUsing constants for limit values makes the code more maintainable and easier to update when requirements change.
19-29: Well-structured GraphQL node definitionThe SnapshotNode class is well-organized with clear field definitions for both scalar and list types.
31-39: Proper model field mappingThe Meta class correctly specifies the underlying model and exposes the appropriate fields.
backend/apps/owasp/migrations/0015_snapshot.py (2)
8-8: Dependency update is appropriateUpdating the dependency to align with the latest github app migration is correct.
22-25: Good schema enhancement with metadata fieldsThe addition of title, key, and timestamp fields improves the model's usability and traceability.
However, you should verify that there's logic in the model's save method to generate unique keys:
#!/bin/bash # Check if the Snapshot model has a custom save method for handling keys echo "Checking Snapshot model for key generation:" rg -A 10 "def save\(" backend/apps/owasp/models/snapshot.py
| def resolve_new_issues(self, info): | ||
| """Resolve recent new issues.""" | ||
| return self.new_issues.order_by("-created_at")[:RECENT_ISSUES_LIMIT] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Security consideration: consider adding access control checks
The resolvers don't include access control checks. Consider adding authorization logic to ensure users can only access snapshots they have permission to view.
🏁 Script executed:
#!/bin/bash
# Check if there are any permission checks elsewhere in the codebase
# Look for authorization patterns in other resolvers
echo "Looking for authorization patterns in GraphQL resolvers..."
rg -A 5 "def resolve_" --type py backend/apps/owasp/graphql/ | rg -i "permission|access|auth"
echo "Checking if BaseQuery implements access control..."
rg -A 10 "class BaseQuery" --type py backend/apps/common/graphql/Length of output: 681
Security: Add Authorization Checks in Snapshot Resolver
At backend/apps/owasp/graphql/nodes/snapshot.py (lines 57-59), the resolve_new_issues method currently returns data without verifying if the requesting user has appropriate permissions. There are no existing access control checks in this resolver, and similar patterns elsewhere in the codebase (e.g., in project.py) also lack explicit authorization logic, while the common base (BaseQuery in backend/apps/common/graphql/queries.py) does not seem to enforce such permissions either.
- Action: Integrate authorization logic—possibly by leveraging a common access control framework or utility—to ensure that only users with the proper permissions can query these snapshots.
| def resolve_new_projects(self, info): | ||
| """Resolve recent new projects.""" | ||
| return self.new_projects.order_by("-created_at")[:RECENT_PROJECTS_LIMIT] | ||
|
|
||
| def resolve_new_releases(self, info): | ||
| """Resolve recent new releases.""" | ||
| return self.new_releases.order_by("-published_at")[:RECENT_RELEASES_LIMIT] |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Consider adding tests for field resolvers
Implement unit tests for these resolver methods to ensure they correctly handle edge cases like empty collections.
🏁 Script executed:
#!/bin/bash
# Check if there are existing tests for resolver methods
echo "Looking for existing resolver tests..."
rg -A 5 "test.*resolve" --type py backend/tests/Length of output: 11189
Add Unit Tests for Snapshot Field Resolvers
The implementation of the resolve_new_projects and resolve_new_releases methods in backend/apps/owasp/graphql/nodes/snapshot.py currently lacks dedicated unit tests. While tests for similar GraphQL field resolvers exist elsewhere in the codebase, there are no tests covering these methods. Please add tests that:
- Verify that the resolvers return values in the correct descending order (by
created_atandpublished_atrespectively). - Confirm that the slice limits (
RECENT_PROJECTS_LIMITandRECENT_RELEASES_LIMIT) are correctly applied. - Handle cases where the underlying collections are empty.
Making these additions will ensure consistent behavior with the resolver patterns implemented and tested in other parts of the project.
|
Merged w/ #981 |
fixes: #915