Owasp.org post preview #1043
Owasp.org post preview #1043
Conversation
Summary by CodeRabbit
WalkthroughThe pull request introduces multiple new functionalities for managing OWASP blog posts. It includes the addition of a GraphQL API for accessing post data, a management command to synchronize posts from a GitHub repository, and the creation of a Changes
Suggested reviewers
✨ 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: 3
🧹 Nitpick comments (6)
backend/apps/owasp/graphql/nodes/post.py (1)
6-6: Minor: Consider removing extra blank lineThere's an extra blank line that could be removed for better code style consistency.
backend/apps/owasp/migrations/0023_post.py (1)
25-25: Consider adding an index for the date fieldSince you're querying recent posts (based on the implementation in the query file), adding an index to the date field could improve query performance.
- ("date", models.DateTimeField(verbose_name="Publication date")), + ("date", models.DateTimeField(db_index=True, verbose_name="Publication date")),backend/apps/owasp/management/commands/owasp_sync_posts.py (2)
31-34: Improve URL transformation logic.The current URL transformation might not correctly map raw content URLs to repository URLs if file paths are complex. Consider a more robust approach to ensure consistent URL structure.
- url = download_url.replace( - "https://raw.githubusercontent.com/OWASP/owasp.github.io/main", - "https://github.com/OWASP/owasp.github.io/tree/main", - ) + # Extract path components to construct a more reliable URL + if "raw.githubusercontent.com" in download_url: + path_parts = download_url.split("/") + if len(path_parts) >= 5: # Has enough parts to contain org/repo/branch + org = path_parts[3] + repo = path_parts[4] + branch = path_parts[5] + file_path = "/".join(path_parts[6:]) + url = f"https://github.com/{org}/{repo}/blob/{branch}/{file_path}" + else: + url = download_url.replace( + "https://raw.githubusercontent.com/OWASP/owasp.github.io/main", + "https://github.com/OWASP/owasp.github.io/blob/main", + ) + else: + url = download_url
41-41: Add command output and logging.The command doesn't provide any feedback about its execution. Adding output would help track progress and confirm success.
+ self.stdout.write(self.style.SUCCESS(f"Processing {len(files)} potential post files")) + + # After processing all files + self.stdout.write(self.style.SUCCESS(f"Found {len(posts)} valid posts")) + Post.bulk_save(posts, fields=["title", "date", "author", "author_image", "url"]) + + self.stdout.write(self.style.SUCCESS(f"Successfully synced {len(posts)} posts from OWASP GitHub repository"))backend/apps/owasp/models/post.py (2)
30-33: Make recent_posts method more flexible.The current method is hardcoded to return exactly 5 posts. Consider making it more flexible by adding a parameter for the number of posts to return and potentially more ordering options.
@staticmethod - def recent_posts(): + def recent_posts(limit=5, ordering="-date"): """Get recent posts.""" - return Post.objects.all().order_by("-date")[:5] + return Post.objects.all().order_by(ordering)[:limit]
8-33: Add indexes for performance optimization.Since this model will likely be queried by date frequently, consider adding an index on the date field to improve query performance, especially as the number of posts grows.
class Meta: db_table = "owasp_post" verbose_name_plural = "Posts" + indexes = [ + models.Index(fields=['-date'], name='post_date_idx'), + ]
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/owasp/graphql/nodes/post.py(1 hunks)backend/apps/owasp/graphql/queries/post.py(1 hunks)backend/apps/owasp/management/commands/owasp_sync_posts.py(1 hunks)backend/apps/owasp/migrations/0023_post.py(1 hunks)backend/apps/owasp/models/__init__.py(1 hunks)backend/apps/owasp/models/post.py(1 hunks)
⏰ 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 backend tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (5)
backend/apps/owasp/models/__init__.py (1)
1-1: Import looks good!This import statement correctly exposes the Post model at the package level, following Python's common pattern for module organization.
backend/apps/owasp/graphql/nodes/post.py (1)
1-19: Node implementation looks clean and well-structuredThe PostNode class is properly implemented, with clear docstrings and appropriate field definitions. The structure follows good practices for GraphQL schema definition in Django.
backend/apps/owasp/migrations/0023_post.py (1)
11-33: Migration looks well-structuredThe Post model migration is properly defined with appropriate field types and constraints.
backend/apps/owasp/graphql/queries/post.py (1)
15-17:✅ Verification successful
Verify the implementation of Post.recent_posts
The resolver accesses
Post.recent_postsdirectly, which suggests it's a property or class method on the Post model. Ensure this property:
- Returns the expected number of posts (5, as specified in the docstring)
- Returns them in the correct order (most recent first)
- Is implemented efficiently (e.g., with proper database queries)
Without seeing the Post model implementation, I can't verify these details.
🏁 Script executed:
#!/bin/bash # Check the implementation of recent_posts in the Post model fd -t f "post.py" backend/apps/owasp/models/ -x catLength of output: 1030
Reviewed Post.recent_posts implementation – No changes required
- The
recent_postsstatic method in the Post model correctly orders posts in descending order by date and slices the queryset to return only 5 posts.- It meets the specifications outlined in the resolver’s docstring: returning the 5 most recent posts efficiently via the Django ORM.
backend/apps/owasp/models/post.py (1)
8-19: Consider adding content field to store post content.The current model stores metadata about posts but doesn't have a field for the actual content. This might be intentional if content is always fetched from GitHub, but consider adding a TextField for content if you want to cache it locally.
If the content is intended to be stored locally to reduce GitHub API calls, you could add:
title = models.CharField(verbose_name="Title") url = models.URLField(verbose_name="URL", unique=True) + content = models.TextField( + verbose_name="Post content", + blank=True, + help_text="The markdown content of the post" + )Is the content intentionally excluded, or would storing it locally be beneficial for your use case?
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
backend/Makefile (3)
101-103: Consider updating the echo message for consistency.The new
owasp-sync-poststarget follows the same pattern as other commands, but unlike most other commands, it doesn't include an echo message to inform users about what's happening.owasp-sync-posts: + @echo "Synchronizing OWASP posts" @CMD="python manage.py owasp_sync_posts" $(MAKE) exec-backend-command-it
101-103: Consider using non-interactive mode for automation.The command uses
exec-backend-command-itwhich is interactive. However, most other similar commands in the Makefile use the non-interactiveexec-backend-command. Unless this command specifically requires user input, consider using the non-interactive version for better automation in CI/CD pipelines.owasp-sync-posts: - @CMD="python manage.py owasp_sync_posts" $(MAKE) exec-backend-command-it + @CMD="python manage.py owasp_sync_posts" $(MAKE) exec-backend-command
101-139: Consider adding the new command to the update-data target.The new
owasp-sync-postscommand isn't included in theupdate-datatarget, which contains other data synchronization commands. Consider adding it to ensure posts are synchronized when running the full data update process.update-data: \ github-update-owasp-organization \ owasp-scrape-chapters \ owasp-scrape-committees \ owasp-scrape-projects \ github-update-project-related-repositories \ owasp-aggregate-projects \ owasp-update-events \ + owasp-sync-posts \ owasp-update-sponsorsbackend/apps/owasp/migrations/0023_post.py (1)
22-30: Consider adding an index on published_at field.Since the
recent_postsquery will filter by thepublished_atfield to get the most recent posts, adding an index would improve query performance.("nest_updated_at", models.DateTimeField(auto_now=True)), ("author_name", models.CharField(max_length=100, verbose_name="Author name")), ( "author_image_url", models.URLField(blank=True, default="", verbose_name="Author image URL"), ), -("published_at", models.DateTimeField(verbose_name="Publication date")), +("published_at", models.DateTimeField(db_index=True, verbose_name="Publication date")), ("title", models.CharField(max_length=200, verbose_name="Title")), ("url", models.URLField(verbose_name="URL")),backend/apps/owasp/management/commands/owasp_sync_posts.py (1)
34-37: URL construction might be fragile.The current URL replacement assumes a specific format that might break if the repository structure changes. Consider extracting just the filename from the URL and constructing a more robust blog URL.
- url = download_url.replace( - "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_posts/", - "https://owasp.org/blog/", - ) + # Extract just the filename from the URL for more robust construction + filename = repository_file.get("name") + # Remove .md extension and construct the blog URL + blog_path = filename[:-3] if filename.endswith('.md') else filename + url = f"https://owasp.org/blog/{blog_path}"backend/apps/owasp/graphql/queries/post.py (2)
15-17: Update docstring to reflect the limit parameter.The docstring for
resolve_recent_postsmentions returning "the 5 most recent posts", but the function accepts alimitparameter that can be different from 5.- def resolve_recent_posts(root, info, limit): - """Return the 5 most recent posts.""" + def resolve_recent_posts(root, info, limit): + """Return the most recent posts, limited by the specified count (default: 5).""" return Post.recent_posts[:limit]
10-17: Consider adding error handling and pagination.The GraphQL query lacks error handling and pagination for fetching posts. For a production API, consider adding proper error handling and pagination to handle large datasets efficiently.
Here's an improved implementation with error handling and pagination:
class PostQuery(BaseQuery): """GraphQL queries for Post model.""" - recent_posts = graphene.List(PostNode, limit=graphene.Int(default_value=5)) + recent_posts = graphene.Field( + graphene.List(PostNode), + limit=graphene.Int(default_value=5), + offset=graphene.Int(default_value=0), + description="Retrieve recent posts with pagination support" + ) - def resolve_recent_posts(root, info, limit): - """Return the 5 most recent posts.""" - return Post.recent_posts[:limit] + def resolve_recent_posts(root, info, limit, offset=0): + """Return the most recent posts with pagination support. + + Args: + limit: Maximum number of posts to return (default: 5) + offset: Number of posts to skip (default: 0) + """ + try: + return Post.recent_posts[offset:offset+limit] + except Exception as e: + # Log the error + info.context.logger.error(f"Error fetching recent posts: {e}") + return []backend/apps/owasp/admin.py (1)
74-79: Consider adding a list_filter for published_atThe PostAdmin configuration is well-structured, but adding date filtering capabilities would improve usability for administrators, especially as the number of posts grows over time.
class PostAdmin(admin.ModelAdmin): """Admin configuration for Post model.""" list_display = ("author_name", "author_image_url", "published_at", "title", "url") search_fields = ("author_name", "author_image_url", "published_at", "title", "url") + list_filter = ("published_at",)backend/apps/owasp/models/post.py (1)
26-28: Add type hinting for better code readabilityAdding type annotations to the parameters and return value would improve code maintainability and help catch errors during development.
@staticmethod -def bulk_save(posts, fields=None): +def bulk_save(posts: list, fields: list = None) -> None: """Bulk save posts.""" BulkSaveModel.bulk_save(Post, posts, fields=fields)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/Makefile(1 hunks)backend/apps/owasp/admin.py(3 hunks)backend/apps/owasp/graphql/queries/post.py(1 hunks)backend/apps/owasp/management/commands/owasp_sync_posts.py(1 hunks)backend/apps/owasp/migrations/0023_post.py(1 hunks)backend/apps/owasp/models/post.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend unit tests
🔇 Additional comments (6)
backend/apps/owasp/migrations/0023_post.py (1)
11-36: LGTM! The Post model migration is well-structured.The migration file correctly defines the Post model with appropriate fields, constraints, and table configuration.
backend/apps/owasp/management/commands/owasp_sync_posts.py (1)
13-52:⚠️ Potential issueAdd error handling and validation for API requests and data processing.
The command lacks robust error handling for API requests, YAML parsing, and required field validation. This could lead to silent failures or invalid data being stored.
Apply these changes to improve error handling and validation:
def handle(self, *args, **options): - post_repository_content = get_repository_file_content( - "https://api.github.com/repos/OWASP/owasp.github.io/contents/_posts" - ) - repository_files = json.loads(post_repository_content) + try: + post_repository_content = get_repository_file_content( + "https://api.github.com/repos/OWASP/owasp.github.io/contents/_posts" + ) + repository_files = json.loads(post_repository_content) + except Exception as e: + self.stderr.write(self.style.ERROR(f"Failed to fetch or parse repository contents: {e}")) + return + posts = [] + processed_count = 0 + skipped_count = 0 for repository_file in repository_files: if repository_file.get("name", "").endswith(".md"): download_url = repository_file.get("download_url") - post_content = get_repository_file_content(download_url) - - if post_content.startswith("---"): - yaml_content = re.search(r"^---\s*(.*?)\s*---", post_content, re.DOTALL) - metadata = yaml.safe_load(yaml_content.group(1)) or {} - - title = metadata.get("title") - published_at = metadata.get("date") - author_name = metadata.get("author") - author_image_url = metadata.get("author_image") or "" - url = download_url.replace( - "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_posts/", - "https://owasp.org/blog/", - ) - - post = Post( - title=title, - published_at=published_at, - author_name=author_name, - author_image_url=author_image_url, - url=url, - ) - - posts.append(post) + try: + post_content = get_repository_file_content(download_url) + + if post_content.startswith("---"): + yaml_content = re.search(r"^---\s*(.*?)\s*---", post_content, re.DOTALL) + if not yaml_content: + self.stderr.write(self.style.WARNING(f"Could not extract YAML from {repository_file.get('name')}")) + skipped_count += 1 + continue + + try: + metadata = yaml.safe_load(yaml_content.group(1)) or {} + except yaml.YAMLError as e: + self.stderr.write(self.style.ERROR(f"Failed to parse YAML for {repository_file.get('name')}: {e}")) + skipped_count += 1 + continue + + title = metadata.get("title") + published_at = metadata.get("date") + author_name = metadata.get("author") + author_image_url = metadata.get("author_image") or "" + + # Validate required fields + if not all([title, published_at, author_name]): + self.stderr.write(self.style.WARNING( + f"Skipping {repository_file.get('name')}: Missing required fields (title, date, or author)" + )) + skipped_count += 1 + continue + + url = download_url.replace( + "https://raw.githubusercontent.com/OWASP/owasp.github.io/main/_posts/", + "https://owasp.org/blog/", + ) + + post = Post( + title=title, + published_at=published_at, + author_name=author_name, + author_image_url=author_image_url, + url=url, + ) + + posts.append(post) + processed_count += 1 + except Exception as e: + self.stderr.write(self.style.ERROR(f"Error processing {repository_file.get('name')}: {e}")) + skipped_count += 1 - Post.bulk_save( - posts, fields=["title", "published_at", "author_name", "author_image_url", "url"] - ) + if posts: + try: + Post.bulk_save( + posts, fields=["title", "published_at", "author_name", "author_image_url", "url"] + ) + self.stdout.write(self.style.SUCCESS(f"Successfully processed {processed_count} posts")) + except Exception as e: + self.stderr.write(self.style.ERROR(f"Failed to save posts: {e}")) + else: + self.stdout.write(self.style.WARNING("No posts found to process")) + + if skipped_count > 0: + self.stdout.write(self.style.WARNING(f"Skipped {skipped_count} posts due to errors or missing data"))backend/apps/owasp/models/post.py (4)
11-13: LGTM! Meta configuration properly set.The Meta class is well-configured with appropriate table name and verbose plural name.
15-19: Field lengths seem appropriate and naming follows project conventions.The model fields are well-defined with appropriate data types, verbose names, and max_length constraints where needed.
16-16: Consider adding help_text to field definitionsAdding help_text to model fields would improve developer experience when working with this model in forms or the admin interface.
- author_image_url = models.URLField(verbose_name="Author image URL", blank=True, default="") + author_image_url = models.URLField( + verbose_name="Author image URL", + blank=True, + default="", + help_text="URL to the author's profile image" + )
8-9: Model design follows project patternsThe Post model properly inherits from BulkSaveModel and TimestampedModel, which aligns with the project's patterns and provides necessary functionality for bulk operations and timestamps.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/apps/owasp/graphql/nodes/post.py (1)
7-19:⚠️ Potential issueFix the typo in the field name.
There's a spelling error in the field name "publised_at" which should be "published_at". This typo will cause issues when trying to access this field through GraphQL queries.
fields = ( "author_name", "author_image_url", - "publised_at", + "published_at", "title", "url", )
🧹 Nitpick comments (1)
backend/apps/owasp/graphql/nodes/post.py (1)
5-6: Remove extra blank lines.There are multiple consecutive blank lines between imports and the class definition. According to PEP 8, you should limit blank lines to two between top-level definitions and one between method definitions.
from apps.owasp.models.post import Post - class PostNode(BaseNode):
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
backend/apps/owasp/graphql/nodes/post.py(1 hunks)backend/apps/owasp/graphql/queries/post.py(1 hunks)backend/apps/owasp/management/commands/owasp_sync_posts.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- backend/apps/owasp/management/commands/owasp_sync_posts.py
- backend/apps/owasp/graphql/queries/post.py
⏰ 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 unit tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (1)
backend/apps/owasp/graphql/nodes/post.py (1)
1-5: Code follows best practices for imports and documentation.The imports are correctly organized, and the module has a clear docstring that describes its purpose.
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py (1)
1-81:⚠️ Potential issueDuplicate test file needs to be refactored or removed
This file is an exact duplicate of
backend/tests/owasp/models/post_test.py. Based on the filename, this should be testing the management command for syncing posts, not the Post model directly.You have two options:
- Remove this file and create a proper test for the sync_posts command
- Refactor to use a shared test class that both files can import
For option 1, create a proper test for the management command:
from unittest.mock import patch, Mock import pytest from io import StringIO from django.core.management import call_command class TestOwaspSyncPostsCommand: @patch("apps.owasp.management.commands.owasp_sync_posts.Command._fetch_posts") @patch("apps.owasp.models.post.Post.update_data") def test_handle_syncs_posts(self, mock_update_data, mock_fetch_posts): # Setup mock post data mock_posts = [ {"url": "https://example.com/1", "title": "Post 1"}, {"url": "https://example.com/2", "title": "Post 2"} ] mock_fetch_posts.return_value = mock_posts mock_update_data.side_effect = lambda data: Mock(url=data["url"]) # Call the command out = StringIO() call_command("owasp_sync_posts", stdout=out) # Assert assert mock_fetch_posts.called assert mock_update_data.call_count == len(mock_posts) assert "Synced 2 posts" in out.getvalue()🧰 Tools
🪛 Ruff (0.8.2)
22-22:
datetime.datetime()called without atzinfoargument(DTZ001)
37-37: Line too long (174 > 99)
(E501)
37-37:
datetime.datetime()called without atzinfoargument(DTZ001)
38-38: Line too long (174 > 99)
(E501)
38-38:
datetime.datetime()called without atzinfoargument(DTZ001)
58-58: Line too long (153 > 99)
(E501)
59-59: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
76-76: Blank line contains whitespace
Remove whitespace from blank line
(W293)
78-78: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🪛 GitHub Actions: Run CI/CD
[error] 22-26: DTZ001
datetime.datetime()called without atzinfoargument. Pass adatetime.timezoneobject to thetzinfoparameter.
♻️ Duplicate comments (2)
backend/apps/owasp/models/post.py (2)
19-19: 🛠️ Refactor suggestionAdd unique constraint to URL field
Posts should have unique URLs to prevent duplicates. This is important for the sync_posts command mentioned in the PR description.
- url = models.URLField(verbose_name="URL") + url = models.URLField(verbose_name="URL", unique=True)
30-33: 🛠️ Refactor suggestionAvoid hardcoding all() in model method
The
recent_posts()method currently returns all posts. Consider adding a parameter to limit the number of results, especially since the PR mentions fetching only the five most recent posts.@staticmethod -def recent_posts(): +def recent_posts(limit=None): """Get recent posts.""" - return Post.objects.all().order_by("-published_at") + queryset = Post.objects.all().order_by("-published_at") + if limit is not None: + queryset = queryset[:limit] + return queryset
🧹 Nitpick comments (3)
backend/tests/owasp/models/post_test.py (2)
58-58: Fix long lineThis line exceeds the 99-character line length limit specified in your code style guidelines.
- data = {"url": "https://existing.com", "title": "Updated Title", "author_name": "Updated Author", "author_image_url": "https://updatedimage.com"} + data = { + "url": "https://existing.com", + "title": "Updated Title", + "author_name": "Updated Author", + "author_image_url": "https://updatedimage.com" + }🧰 Tools
🪛 Ruff (0.8.2)
58-58: Line too long (153 > 99)
(E501)
🪛 GitHub Actions: Run CI/CD
[error] 58-100: E501 Line too long (153 > 99)
59-61: Remove whitespace from blank linesThe blank lines contain whitespace, which is flagged by the static analysis tools.
data = {"url": "https://existing.com", "title": "Updated Title", "author_name": "Updated Author", "author_image_url": "https://updatedimage.com"} - + result = Post.update_data(data) - + mock_get.assert_called_once_with(url=data["url"])And similarly for lines 76-78.
Also applies to: 76-78
🧰 Tools
🪛 Ruff (0.8.2)
59-59: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/apps/owasp/models/post.py (1)
15-18: Add help_text to model fields for better documentationAdding help_text would improve developer experience when using this model in forms or admin interfaces.
- author_name = models.CharField(verbose_name="Author name", max_length=100) - author_image_url = models.URLField(verbose_name="Author image URL", blank=True, default="") - published_at = models.DateTimeField(verbose_name="Publication date") - title = models.CharField(verbose_name="Title", max_length=200) + author_name = models.CharField( + verbose_name="Author name", + max_length=100, + help_text="The name of the post author" + ) + author_image_url = models.URLField( + verbose_name="Author image URL", + blank=True, + default="", + help_text="URL to the author's profile image" + ) + published_at = models.DateTimeField( + verbose_name="Publication date", + help_text="When the post was published" + ) + title = models.CharField( + verbose_name="Title", + max_length=200, + help_text="The title of the post" + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
backend/apps/owasp/graphql/nodes/post.py(1 hunks)backend/apps/owasp/management/commands/owasp_sync_posts.py(1 hunks)backend/apps/owasp/migrations/0023_post.py(1 hunks)backend/apps/owasp/models/post.py(1 hunks)backend/tests/owasp/management/commands/owasp_sync_posts_test.py(1 hunks)backend/tests/owasp/models/post_test.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- backend/apps/owasp/graphql/nodes/post.py
- backend/apps/owasp/management/commands/owasp_sync_posts.py
- backend/apps/owasp/migrations/0023_post.py
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py
22-22: datetime.datetime() called without a tzinfo argument
(DTZ001)
37-37: Line too long (174 > 99)
(E501)
37-37: datetime.datetime() called without a tzinfo argument
(DTZ001)
38-38: Line too long (174 > 99)
(E501)
38-38: datetime.datetime() called without a tzinfo argument
(DTZ001)
58-58: Line too long (153 > 99)
(E501)
59-59: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
76-76: Blank line contains whitespace
Remove whitespace from blank line
(W293)
78-78: Blank line contains whitespace
Remove whitespace from blank line
(W293)
backend/tests/owasp/models/post_test.py
22-22: datetime.datetime() called without a tzinfo argument
(DTZ001)
37-37: Line too long (174 > 99)
(E501)
37-37: datetime.datetime() called without a tzinfo argument
(DTZ001)
38-38: Line too long (174 > 99)
(E501)
38-38: datetime.datetime() called without a tzinfo argument
(DTZ001)
58-58: Line too long (153 > 99)
(E501)
59-59: Blank line contains whitespace
Remove whitespace from blank line
(W293)
61-61: Blank line contains whitespace
Remove whitespace from blank line
(W293)
76-76: Blank line contains whitespace
Remove whitespace from blank line
(W293)
78-78: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🪛 GitHub Actions: Run CI/CD
backend/tests/owasp/management/commands/owasp_sync_posts_test.py
[error] 22-26: DTZ001 datetime.datetime() called without a tzinfo argument. Pass a datetime.timezone object to the tzinfo parameter.
[error] 37-100: E501 Line too long (174 > 99)
[error] 38-100: DTZ001 datetime.datetime() called without a tzinfo argument. Pass a datetime.timezone object to the tzinfo parameter.
[error] 58-100: E501 Line too long (153 > 99)
backend/tests/owasp/models/post_test.py
[error] 22-26: DTZ001 datetime.datetime() called without a tzinfo argument. Pass a datetime.timezone object to the tzinfo parameter.
[error] 37-100: E501 Line too long (174 > 99)
[error] 38-100: DTZ001 datetime.datetime() called without a tzinfo argument. Pass a datetime.timezone object to the tzinfo parameter.
[error] 58-100: E501 Line too long (153 > 99)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py (2)
14-16: Remove unnecessary parentheses from pytest fixture decoratorsAccording to pytest best practices and your linting rules, you should use
@pytest.fixturewithout empty parentheses.- @pytest.fixture() + @pytest.fixture def command(self): return Command() - @pytest.fixture() + @pytest.fixture def mock_repository_files(self): return [ - @pytest.fixture() + @pytest.fixture def mock_post_content(self): return """---Also applies to: 18-20, 31-33
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
150-150: Add a newline at the end of fileAdd a trailing newline to fix the linting error and pipeline failure.
[], fields=["title", "published_at", "author_name", "author_image_url", "url"] ) +🧰 Tools
🪛 Ruff (0.8.2)
150-150: No newline at end of file
Add trailing newline
(W292)
backend/tests/owasp/models/post_test.py (1)
88-98: LGTM! Clean test for recent_posts methodThe test effectively verifies that recent_posts applies the correct ordering.
Add a newline at the end of the file to comply with style guidelines.
mock_queryset.order_by.assert_called_once_with("-published_at") assert result == mock_queryset.order_by.return_value +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py(1 hunks)backend/tests/owasp/models/post_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py
14-14: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
18-18: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
31-31: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
150-150: No newline at end of file
Add trailing newline
(W292)
🪛 GitHub Actions: Run CI/CD
backend/tests/owasp/management/commands/owasp_sync_posts_test.py
[error] 147-147: Process completed with exit code 1. Pre-commit hook(s) made changes.
🔇 Additional comments (10)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py (4)
9-17: LGTM! Good test class structureThe test class is well-organized with appropriate constants and fixtures. The tests cover the main scenarios including successful processing and error handling cases.
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
42-101: Well-structured test for the happy pathThis test thoroughly verifies the behavior of the command when processing is successful. It includes good assertions to check:
- The correct API endpoints are called
- The expected number of calls are made
- The update_data method is called with the correct parameters
- The bulk_save method is called with the expected parameters
The mocking approach cleanly isolates the test from actual API calls.
102-123: Comprehensive error handling testGood test coverage for the scenario when posts don't have front matter. The test verifies that:
- The API calls are still made
- No updates are attempted
- The bulk_save method is called with an empty list
This ensures the command handles malformed post data gracefully.
124-150: Good edge case coverageThis test appropriately handles the case when the update_data method returns None, ensuring that the bulk_save method is still called with an empty list. This verifies that the command properly handles this edge case without errors.
🧰 Tools
🪛 Ruff (0.8.2)
150-150: No newline at end of file
Add trailing newline
(W292)
🪛 GitHub Actions: Run CI/CD
[error] 147-147: Process completed with exit code 1. Pre-commit hook(s) made changes.
backend/tests/owasp/models/post_test.py (6)
1-8: LGTM! Good imports organizationThe imports are well-organized, with standard library imports first, followed by third-party imports, and then local imports.
9-25: Well-designed parameterized test for str methodGood use of pytest's parameterize to test multiple cases with a single test function. The test covers both a normal title and an empty string.
26-32: Effective mocking of bulk_save methodThis test properly mocks the parent class method and verifies it's called with the correct parameters.
33-64: Comprehensive parameterized test for from_dict methodGood test design with parameterized test cases covering:
- Full data set with all fields
- Partial data set with only a subset of fields
The assertions verify that each field is correctly updated.
65-83: Thorough test for update_data methodThis test properly mocks the database query and verifies:
- The correct object is retrieved using the URL
- The from_dict method is called with the provided data
- The save method is called
- The result is the updated post
Good coverage of the expected behavior.
84-87: Good edge case testing for update_dataThis test confirms that the update_data method returns None when URL is missing, which is important for error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py (2)
14-14: Remove unnecessary parentheses from pytest.fixture decoratorsThe pytest fixture decorators should not have empty parentheses according to best practices. While functionally equivalent, the preferred style is to use
@pytest.fixturewithout parentheses.- @pytest.fixture() + @pytest.fixtureApply this change to all three fixture decorators in the file (lines 14, 18, and 31).
Also applies to: 18-18, 31-31
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
1-151: Consider adding API failure test casesThe current tests cover successful API calls and content parsing scenarios, but there are no tests for API failures like network errors or invalid responses.
Consider adding a test case that simulates API failures by having mock_get_content raise exceptions, ensuring the command handles these gracefully. Example:
@mock.patch("apps.owasp.management.commands.owasp_sync_posts.get_repository_file_content") @mock.patch("apps.owasp.models.post.Post.bulk_save") def test_handle_api_failure(self, mock_bulk_save, mock_get_content, command): mock_get_content.side_effect = Exception("API connection failed") # Command should handle the exception gracefully command.handle() # No data should be saved when the API fails mock_bulk_save.assert_called_once_with( [], fields=["title", "published_at", "author_name", "author_image_url", "url"] )🧰 Tools
🪛 Ruff (0.8.2)
14-14: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
18-18: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
31-31: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py
14-14: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
18-18: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
31-31: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: CodeQL (python)
- GitHub Check: Run frontend e2e tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (3)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py (3)
42-101: Well-structured test for successful processing pathThe test thoroughly validates the command's behavior when successfully processing posts. It checks all essential aspects:
- Correct number of API calls
- Correct API endpoints being called
- Expected data transformations
- Proper bulk saving of processed data
102-123: Good error handling test for missing front matterThis test properly verifies that the command handles posts without front matter gracefully:
- No update_data calls are made
- The bulk_save is still called with an empty list
- The command continues processing without errors
This ensures robustness when dealing with potentially malformed content.
124-150: Appropriate handling of None return valuesThe test verifies that the command properly handles None values returned from update_data, ensuring these posts are excluded from bulk saving. This defensive programming approach prevents potential issues with invalid data.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/apps/common/utils.py (1)
20-36: Consider adding error loggingWhile the function handles non-matching paths gracefully by returning the original path, it might be helpful to log warnings when a path doesn't match the expected pattern. This would assist in diagnosing issues if unexpected URL formats are encountered.
def get_blog_url(path): """Return OWASP blog URL for a given path.""" pattern = re.compile( r"(https://raw\.githubusercontent\.com/OWASP/owasp\.github\.io/main/_posts/)" r"(\d{4})-(\d{2})-(\d{2})-" r"(.+)" r"\.md$" ) match = pattern.match(path) if match: year = match.group(2) month = match.group(3) day = match.group(4) slug = match.group(5) return f"{OWASP_BLOG_URL}{year}/{month}/{day}/{slug}.html" + # Consider adding logging here + # import logging + # logger = logging.getLogger(__name__) + # logger.warning(f"Path did not match expected blog post format: {path}") return pathbackend/tests/owasp/management/commands/owasp_sync_posts_test.py (2)
14-16: Fix pytest fixture syntaxAccording to pytest best practices, you should remove the parentheses from the
@pytest.fixturedecorators.- @pytest.fixture() + @pytest.fixture def command(self): return Command() - @pytest.fixture() + @pytest.fixture def mock_repository_files(self): return [ - @pytest.fixture() + @pytest.fixture def mock_post_content(self): return """---Also applies to: 18-29, 31-40
🧰 Tools
🪛 Ruff (0.8.2)
14-14: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
1-151: Consider parameterizing test cases to improve maintainabilityThe test cases follow similar patterns but with different input data and expected outcomes. Consider using pytest's parameterization to reduce code duplication and make it easier to add more test cases in the future.
Here's an example of how you could parameterize the tests:
@pytest.mark.parametrize( "post_content,update_return,expected_update_calls,expected_bulk_save_items", [ ( """--- title: Test Post date: 2023-01-01 author: John Doe author_image: https://example.com/john.jpg --- This is the content of the test post.""", [mock.Mock(), mock.Mock()], 2, 2, ), ( "This is a post without front matter", [], 0, 0, ), ( """--- title: Test Post date: 2023-01-01 author: John Doe author_image: https://example.com/john.jpg --- This is the content of the test post.""", [None, None], 2, 0, ), ], ) @mock.patch("apps.owasp.management.commands.owasp_sync_posts.get_repository_file_content") @mock.patch("apps.owasp.models.post.Post.update_data") @mock.patch("apps.owasp.models.post.Post.bulk_save") def test_handle_cases( self, mock_bulk_save, mock_update_data, mock_get_content, post_content, update_return, expected_update_calls, expected_bulk_save_items, command, mock_repository_files, ): # Test implementation🧰 Tools
🪛 Ruff (0.8.2)
14-14: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
18-18: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
31-31: Use
@pytest.fixtureover@pytest.fixture()Remove parentheses
(PT001)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
backend/apps/common/constants.py(1 hunks)backend/apps/common/utils.py(1 hunks)backend/apps/owasp/management/commands/owasp_sync_posts.py(1 hunks)backend/tests/owasp/management/commands/owasp_sync_posts_test.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- backend/apps/common/constants.py
🚧 Files skipped from review as they are similar to previous changes (1)
- backend/apps/owasp/management/commands/owasp_sync_posts.py
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/tests/owasp/management/commands/owasp_sync_posts_test.py
14-14: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
18-18: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
31-31: Use @pytest.fixture over @pytest.fixture()
Remove parentheses
(PT001)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (python)
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (4)
backend/apps/common/utils.py (1)
20-36: Good implementation of the blog URL formatterThe new
get_blog_urlfunction effectively extracts components from GitHub raw content URLs and constructs proper OWASP blog URLs. The regex pattern is well-designed to match the expected format of OWASP blog post paths.backend/tests/owasp/management/commands/owasp_sync_posts_test.py (3)
42-101: Great test coverage for successful processing pathThe test thoroughly validates the command's behavior when processing posts successfully. It properly mocks dependencies and verifies all important aspects of the implementation, including API calls, data transformation, and database operations.
102-123: Good error handling test for posts without front matterThis test effectively ensures that posts without proper front matter are skipped and don't cause any errors. The command correctly handles invalid input without attempting to create invalid post entries.
124-151: Well-handled edge case for update_data returning NoneThe test properly validates that the command continues processing even when the update_data method returns None, ensuring the bulk_save method is still called with the correct parameters. This verifies the robustness of the implementation.
Resolves #994
title,date,author,author_image,url)sync_postscommand