fix/category-slug: added category slug field #200
Conversation
|
Warning Rate limit exceeded@happychuks has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 11 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces several changes to the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
CodeRabbit Configuration File (
|
![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
🧹 Outside diff range and nitpick comments (6)
server/apps/research/serializers/category_serializer.py (1)
6-6: LGTM! Consider adding slug format validation.The slug field implementation looks good with appropriate read-only flag and max length. The Meta fields are correctly updated to include the new field.
Consider adding a validator to ensure the slug format matches your requirements:
def validate_slug(value): if not re.match(r'^[-a-zA-Z0-9_]+$', value): raise serializers.ValidationError("Slug must contain only letters, numbers, underscores or hyphens") return value slug = serializers.SlugField( read_only=True, max_length=255, help_text='URL-friendly version of the category name.', validators=[validate_slug] )Also applies to: 9-9
server/apps/research/admin/category_admin.py (1)
8-8: LGTM! Consider adding slug to search fields.The admin configuration looks good with slug correctly marked as read-only and included in list display.
Consider adding 'slug' to search_fields to allow admins to search by slug:
- search_fields = ('name',) + search_fields = ('name', 'slug')Also applies to: 13-13
server/apps/research/models/category.py (1)
22-39: Add maximum attempts limit to slug generationThe while loop for generating unique slugs could potentially run indefinitely. Add a maximum number of attempts to prevent performance issues.
def generate_slug(self): + MAX_ATTEMPTS = 100 if not self.name: raise ValueError("Name is required to generate slug") base_slug = slugify(self.name) slug = base_slug num = 1 with transaction.atomic(): + attempts = 0 while ( Category.objects.select_for_update() .filter(slug=slug) .exclude(id=self.id) # Exclude current instance when updating .exists() ): + if attempts >= MAX_ATTEMPTS: + raise ValueError(f"Could not generate unique slug after {MAX_ATTEMPTS} attempts") slug = f"{base_slug}-{num}" num += 1 + attempts += 1 return slugserver/apps/research/views.py (3)
105-106: Update docstring to reflect category_slug parameterThe method signature has changed but the docstring hasn't been updated to reflect the new parameter name.
@action(detail=False, methods=['get'], url_path=r'category/(?P<category_slug>[-\w]+)') def retrieve_by_category(self, request, category_slug=None): - """Retrieve article list by category.""" + """Retrieve article list by category slug. + + Args: + request: The HTTP request + category_slug: The URL-friendly identifier of the category + + Returns: + Response: List of articles in the specified category + """
109-111: Improve error message specificityThe error message could be more specific about which category was not found.
- instances = Article.objects.filter(categories__slug=category_slug) - if not instances.exists(): - return Response({'error': 'No articles found for this category'}, status=status.HTTP_404_NOT_FOUND) + # First verify if the category exists + if not Category.objects.filter(slug=category_slug).exists(): + return Response( + {'error': f'Category with slug "{category_slug}" does not exist'}, + status=status.HTTP_404_NOT_FOUND + ) + + instances = Article.objects.filter(categories__slug=category_slug) + if not instances.exists(): + return Response( + {'error': f'No articles found in category "{category_slug}"'}, + status=status.HTTP_404_NOT_FOUND + )
105-105: Consider API versioning for breaking changeChanging the URL parameter from
categorytocategory_slugis a breaking change that could affect API clients. Consider:
- Adding API versioning
- Temporarily supporting both old and new endpoints
- Documenting the change in API documentation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
server/apps/research/admin/category_admin.py(1 hunks)server/apps/research/migrations/0016_category_slug.py(1 hunks)server/apps/research/models/category.py(1 hunks)server/apps/research/serializers/category_serializer.py(1 hunks)server/apps/research/views.py(1 hunks)
🔇 Additional comments (1)
server/apps/research/migrations/0016_category_slug.py (1)
1-23: Verify the impact on existing data and URLs.
Since this migration modifies how categories are identified in URLs, we should verify the impact on existing references.
✅ Verification successful
Let me try another search strategy to gather more information about the category usage.
Let me check the URL patterns and model relationships more specifically.
Based on the gathered information, I can now provide a final response:
The slug addition is safe and properly handled
The implementation has several safeguards in place:
- The URL pattern already uses slug-based routing (
category/(?P<category>[-\w]+)/$) - The Category model includes proper slug generation with uniqueness handling
- The migration includes a function to handle duplicate slugs
- Articles reference categories through a ManyToManyField, which uses IDs internally
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impacts of the slug changes
# Look for direct category ID references in templates
echo "Checking templates for category references..."
rg -l "category.id|category_id" --type html
# Look for category-related URL patterns
echo "Checking URL patterns..."
rg -l "category.*id|category_id" --type python
# Look for potential foreign key references
echo "Checking model relationships..."
ast-grep --pattern 'models.ForeignKey("Category"'
Length of output: 410
Script:
#!/bin/bash
# Search for Category model usage and URL patterns
# Look for Category model references
echo "Checking Category model usage..."
rg -l "Category" --type-add 'python:*.py'
# Look for URL patterns with 'category'
echo "Checking URL patterns with 'category'..."
rg -l "path.*category" --type-add 'python:*.py'
# Check views for category handling
echo "Checking views for category handling..."
rg -A 5 "def.*category" --type-add 'python:*.py'
# Look for templates with category
echo "Checking templates with category..."
fd -e html -x rg -l "category"
Length of output: 1703
Script:
#!/bin/bash
# Check URL patterns in urls.py
echo "Checking URL patterns in urls.py..."
rg -A 3 "path.*category" server/apps/research/urls.py
# Check Category model definition
echo "Checking Category model..."
cat server/apps/research/models/category.py
# Check Article-Category relationship
echo "Checking Article model for Category relationship..."
cat server/apps/research/models/article.py
# Check if there are any other migrations modifying category
echo "Checking previous category-related migrations..."
rg -l "category" server/apps/research/migrations/*.py
Length of output: 11162
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
server/apps/research/models/category.py (2)
9-9: Consider adding db_index for better query performanceSince the slug field will be used for lookups in URL routing, adding an index would improve query performance.
- slug = models.SlugField(max_length=255, blank=True) + slug = models.SlugField(max_length=255, blank=True, db_index=True)
27-44: Add maximum attempts limit for slug generationWhile the slug generation logic is robust, it should have a maximum number of attempts to prevent potential infinite loops in case of extremely high collision rates.
def generate_slug(self): if not self.name: raise ValueError("Name is required to generate slug") base_slug = slugify(self.name) slug = base_slug num = 1 + max_attempts = 1000 # Prevent infinite loops with transaction.atomic(): while ( Category.objects.select_for_update() .filter(slug=slug) .exclude(id=self.id) # Exclude current instance when updating .exists() ): + if num > max_attempts: + raise ValueError(f"Could not generate unique slug after {max_attempts} attempts") slug = f"{base_slug}-{num}" num += 1 return slug
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
server/apps/research/migrations/0016_category_slug.py(1 hunks)server/apps/research/models/category.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- server/apps/research/migrations/0016_category_slug.py
🧰 Additional context used
🪛 Ruff (0.8.0)
server/apps/research/models/category.py
21-21: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
🔇 Additional comments (2)
server/apps/research/models/category.py (2)
3-4: LGTM: Imports are appropriate
The added imports for slugify and transaction are necessary for the slug generation functionality and database atomicity.
9-9: Verify the migration strategy for existing categories
Since the slug field is being added to an existing model with blank=True, verify that:
- The migration handles existing records appropriately
- There's a plan to populate slugs for existing categories
✅ Verification successful
Migration and slug handling are properly implemented
The changes are well-handled with:
- A data migration (0016_category_slug.py) that handles duplicate slugs for existing records
- A robust
save()method in the Category model that:- Auto-generates unique slugs using the name field
- Handles duplicates with numerical suffixes
- Uses transactions for thread safety
- The slug field is already being used in views for article filtering by category
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's a migration file for this change
fd --type f "category.*slug.*py$" server/apps/research/migrations/
# Check for any data migration logic
rg -l "migrations.RunPython" server/apps/research/migrations/
Length of output: 184
Script:
#!/bin/bash
# Check the content of the migration file
cat server/apps/research/migrations/0016_category_slug.py
# Check if there are any views or templates using the category slug
rg -g '!migrations' -l "category.*slug" server/apps/research
Length of output: 1001
Script:
#!/bin/bash
# Check how the slug is used in views.py
rg -A 5 -B 5 "category.*slug" server/apps/research/views.py
# Check if there's a signal or save method that auto-generates slugs
ast-grep --pattern 'def save($$$) { $$$ }' server/apps/research/models/category.py
Length of output: 1025
Script:
#!/bin/bash
# Check for any signals or pre_save handlers that might populate the slug
rg -l "pre_save" server/apps/research/
rg -l "signals" server/apps/research/
# Check the complete category model for any slug-related logic
cat server/apps/research/models/category.py
Length of output: 1486
Summary by CodeRabbit
New Features
slugfield to theCategorymodel, enhancing URL-friendly identification.slugfield and mark it as read-only.category_slug.Bug Fixes
Categorymodel.Documentation
slugfield in theCategorySerializer.