feat(bounty): add Bounty model, serializer, and API endpoints: Phase1 #5268
+192
−16
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
👋 Hi @Ankit-singh-dot! This pull request needs a peer review before it can be merged. Please request a review from a team member who is not:
Once a valid peer review is submitted, this check will pass automatically. Thank you! |
github-actions
bot
added
files-changed: 6
Contributor
WalkthroughAdds a new Bounty feature: a Django Bounty model and migration, ModelSerializer(s), a BountyViewSet with create and issue-total endpoints, and DRF route registration for Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "BountyViewSet"
participant S as "BountySerializer"
participant M as "Bounty model"
participant DB as "Database"
rect rgb(235,245,255)
Note over Client,DB: Create bounty (POST /api/bounties/)
Client->>API: POST /api/bounties/ (payload)
API->>S: S(data=payload)
S->>S: serializer.is_valid()
alt origin_comment_id provided
API->>M: M.objects.filter(origin_comment_id=...).exists()
M->>DB: SELECT ... WHERE origin_comment_id=?
DB-->>M: result
alt exists
API-->>Client: 400/409 Duplicate response
else
API->>M: M.objects.create(**validated_data)
M->>DB: INSERT bounty
DB-->>M: created row
API-->>Client: 201 Created + serialized bounty
end
else
API->>M: M.objects.create(**validated_data)
M->>DB: INSERT bounty
DB-->>M: created row
API-->>Client: 201 Created + serialized bounty
end
end
sequenceDiagram
participant Client
participant API as "BountyViewSet"
participant M as "Bounty model"
participant DB as "Database"
rect rgb(250,245,235)
Note over Client,DB: Issue total (GET /api/bounties/issue-total?repo=...&issue=...)
Client->>API: GET /api/bounties/issue-total?repo=...&issue=...
API->>M: M.objects.filter(repo_full_name=..., issue_number=...).aggregate(Sum('amount'))
M->>DB: SELECT SUM(amount) ... WHERE repo_full_name=? AND issue_number=?
DB-->>M: {total: X}
API-->>Client: 200 OK {repo, issue_number, total: X}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
Contributor
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
![sentry[bot]](https://avatars.githubusercontent.com/u/1396951?s=60&v=4){kind=small}
blt/urls.py
Outdated
Comment on lines
402
to
403
| router = routers.DefaultRouter() | ||
| router.register(r"bounties", BountyViewSet, basename="bounty") |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (9)
blt/urls.py (1)
373-374: Remove duplicate import.The
routersimport on line 373 duplicates line 19. TheBountyViewSetimport on line 374 is correctly placed.Apply this diff:
-from rest_framework import routers from website.api.views import BountyViewSetwebsite/serializers.py (1)
25-25: Remove duplicate import.The
serializersimport duplicates line 2.Apply this diff:
-from rest_framework import serializers from website.views.models_bounty import Bountywebsite/test_feed.py (1)
128-145: Remove unused nested serializer definition.This
BountySerializerclass is defined inside the test method but never used. The testtest_rss_feed_link_on_page(lines 117-124) doesn't reference it. Additionally,BountySerializeris already defined inwebsite/serializers.py(lines 312-315). Remove this unused code.Apply this diff:
rss_url = reverse("activity_feed_rss") self.assertContains(response, rss_url) self.assertContains(response, "Subscribe to RSS Feed") - - - - class BountySerializer(serializers.ModelSerializer): - class Meta: - model = Bounty - fields = [ - "id", - "repo_full_name", - "issue_number", - "issue_url", - "sponsor_github", - "sponsor_user", - "amount", - "currency", - "created_at", - "paid", - "paid_at", - "origin_comment_id", - ] - read_only_fields = ["id", "created_at", "paid", "paid_at"]website/views/models_bounty.py (3)
17-17: Clarify the comment for the amount field.The comment "currency in USD (or store cents)" is misleading. Since
amountis aDecimalFieldwithdecimal_places=2, it stores dollars (e.g., 50.00 for $50), not cents (5000 for $50). Update the comment to reflect the actual storage format.Apply this diff:
- amount = models.DecimalField(max_digits=12, decimal_places=2) # currency in USD (or store cents) + amount = models.DecimalField(max_digits=12, decimal_places=2) # e.g., 50.00 for $50 USD
26-26: Consider adding a unique constraint on origin_comment_id.Since
origin_comment_idis used for duplicate prevention in the API (website/api/views.py line 1983), adding a database-level unique constraint would provide an additional safety layer. However, because the field is nullable and GitHub might send events without comment IDs, you'd need a partial unique constraint.Apply this diff to add a partial unique constraint (requires Django 2.2+):
class Meta: indexes = [ models.Index(fields=["repo_full_name", "issue_number"]), models.Index(fields=["sponsor_github"]), ] + constraints = [ + models.UniqueConstraint( + fields=["origin_comment_id"], + condition=models.Q(origin_comment_id__isnull=False), + name="unique_origin_comment_id" + ), + ] ordering = ["-created_at"]Note: This requires a new migration. Verify this aligns with your GitHub webhook behavior.
17-18: Add validation for amount and currency fields.Consider adding model-level validation to ensure
amountis positive andcurrencyis a valid currency code. This prevents data integrity issues at the database layer.Add validators to the model:
from django.core.validators import MinValueValidator from django.core.exceptions import ValidationError class Bounty(models.Model): # ... existing fields ... amount = models.DecimalField( max_digits=12, decimal_places=2, validators=[MinValueValidator(0.01)] ) currency = models.CharField(max_length=10, default="USD") def clean(self): super().clean() # Validate currency is a supported code SUPPORTED_CURRENCIES = ["USD", "EUR", "GBP"] # extend as needed if self.currency not in SUPPORTED_CURRENCIES: raise ValidationError(f"Currency {self.currency} is not supported")Alternatively, use
choicesfor the currency field if the list is fixed.website/api/views.py (3)
35-36: Remove duplicate imports.Both
viewsets(line 35, already on line 27) andSum(line 36, already on line 22) are duplicate imports.Apply this diff:
-from rest_framework import viewsets, status -from django.db.models import Sum from website.duplicate_checker import check_for_duplicates, find_similar_bugs, format_similar_bug
1983-1987: Return appropriate HTTP status for duplicate bounties.Returning
HTTP_200_OKfor an existing bounty is semantically incorrect. Consider usingHTTP_409_CONFLICTto indicate the resource already exists, or return a consistent 200 with acreated: falseflag.Apply this diff:
# Prevent duplicate entries if origin_comment_id and Bounty.objects.filter(origin_comment_id=origin_comment_id).exists(): + existing = Bounty.objects.get(origin_comment_id=origin_comment_id) + serializer = self.get_serializer(existing) return Response( - {"detail": "Bounty for this GitHub comment already exists."}, - status=status.HTTP_200_OK, + {"detail": "Bounty for this GitHub comment already exists.", "bounty": serializer.data}, + status=status.HTTP_409_CONFLICT, )Alternatively, for idempotent behavior, return 200 with the existing bounty data and a
created: falsefield.
2008-2019: Preserve decimal precision in the total field.Converting
totalfromDecimaltofloaton line 2019 can introduce floating-point precision errors. For financial data, preserve precision by returning the Decimal as a string or keeping it as Decimal (DRF serializes Decimal correctly).Apply this diff:
total = ( Bounty.objects .filter(repo_full_name=repo, issue_number=issue_number) .aggregate(total=Sum("amount")) .get("total") or 0 ) return Response( { "repo": repo, - "issue_number": int(issue_number), - "total": float(total) + "issue_number": issue_number, + "total": str(total) if total else "0.00" } )If
issue_numberis validated asintearlier (per previous comment), no need to cast it again.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
blt/urls.py(2 hunks)website/api/views.py(3 hunks)website/migrations/0261_bounty.py(1 hunks)website/serializers.py(2 hunks)website/test_feed.py(2 hunks)website/views/models_bounty.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
website/views/models_bounty.py (1)
website/serializers.py (16)
Meta(34-36)Meta(58-74)Meta(78-80)Meta(84-86)Meta(97-99)Meta(107-109)Meta(113-115)Meta(119-121)Meta(125-127)Meta(139-142)Meta(149-151)Meta(155-173)Meta(177-185)Meta(198-200)Meta(212-237)Meta(248-266)
website/serializers.py (2)
website/views/models_bounty.py (1)
Bounty(6-36)website/test_feed.py (1)
BountySerializer(128-145)
blt/urls.py (1)
website/api/views.py (1)
BountyViewSet(1960-2021)
website/api/views.py (3)
website/views/models_bounty.py (1)
Bounty(6-36)website/serializers.py (1)
BountySerializer(312-315)website/test_feed.py (1)
BountySerializer(128-145)
website/test_feed.py (2)
website/views/models_bounty.py (1)
Bounty(6-36)website/serializers.py (17)
BountySerializer(312-315)Meta(34-36)Meta(58-74)Meta(78-80)Meta(84-86)Meta(97-99)Meta(107-109)Meta(113-115)Meta(119-121)Meta(125-127)Meta(139-142)Meta(149-151)Meta(155-173)Meta(177-185)Meta(198-200)Meta(212-237)Meta(248-266)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (2)
website/migrations/0261_bounty.py (1)
1-45: LGTM!The migration correctly creates the Bounty model with appropriate fields, indexes, and ordering. The schema aligns with the model definition in website/views/models_bounty.py.
website/test_feed.py (1)
5-6: Verify the Bounty import path.The import
from .models import Bountymay fail because Bounty is defined inwebsite.views.models_bounty, not inwebsite.models. If you need to import Bounty here, usefrom website.views.models_bounty import Bountyor add it towebsite.models.__init__.py.#!/bin/bash # Verify Bounty model import paths and check if it's exported from website.models rg -n "class Bounty" --type=py rg -n "from.*Bounty" --type=py | head -20 # Check if Bounty is in website/models/__init__.py or website/models.py cat website/models.py 2>/dev/null | grep -i bounty || echo "Bounty not found in models.py" cat website/models/__init__.py 2>/dev/null | grep -i bounty || echo "Bounty not found in __init__.py"
blt/urls.py
Outdated
Comment on lines
402
to
403
| router = routers.DefaultRouter() | ||
| router.register(r"bounties", BountyViewSet, basename="bounty") |
Contributor
There was a problem hiding this comment.
Critical: Router re-instantiation overwrites existing routes.
Re-instantiating the router on line 402 discards all previously registered routes (lines 394-401: issues, userissues, profile, domain, timelogs, activitylogs, organizations, jobs). Only the bounty route will be accessible when router.urls is included on line 797.
Apply this diff to register the bounty route on the existing router:
router.register(r"organizations", OrganizationViewSet, basename="organizations")
router.register(r"jobs", JobViewSet, basename="jobs")
-router = routers.DefaultRouter()
router.register(r"bounties", BountyViewSet, basename="bounty")
handler404 = "website.views.core.handler404"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In blt/urls.py around lines 402-403, the code re-instantiates router (router =
routers.DefaultRouter()) which discards routes registered on lines ~394-401;
remove the new instantiation and call router.register(r"bounties",
BountyViewSet, basename="bounty") on the existing router instance so the bounty
route is added without overwriting previously registered routes.
github-project-automation
bot
moved this from Backlog
to Ready
in 📌 OWASP BLT Project Board
github-actions
bot
added
the
changes-requested
Contributor
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
Comment on lines
399
to
407
| router.register(r"activitylogs", ActivityLogViewSet, basename="activitylogs") | ||
| router.register(r"organizations", OrganizationViewSet, basename="organizations") | ||
| router.register(r"jobs", JobViewSet, basename="jobs") | ||
|
|
||
| router = routers.DefaultRouter() | ||
| router.register(r"issues", IssueViewSet, basename="issues") | ||
| router.register(r"userissues", UserIssueViewSet, basename="userissues") | ||
| handler404 = "website.views.core.handler404" | ||
| handler500 = "website.views.core.handler500" | ||
|
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
website/serializers.py (1)
311-314: Explicitly declare serializer fields and mark read-only fields.This concern was already raised in a previous review. Using
fields = "__all__"exposes all model fields without granular control. For security and API clarity, explicitly list fields and mark system-managed fields (id,created_at,paid,paid_at) as read-only. The test file already shows the recommended pattern.
🧹 Nitpick comments (3)
website/views/models_bounty.py (2)
1-5: Consider relocating the model towebsite/models.pyorwebsite/models/directory.Placing a Django model in
website/views/is unconventional. Models are typically inmodels.pyor amodels/package for better discoverability and alignment with Django conventions.
26-27: Consider adding a unique constraint onorigin_comment_id.The API view checks for duplicate
origin_comment_idbefore creation, but this check is susceptible to race conditions. Adding a database-level unique constraint (excluding null values) would provide stronger guarantees for idempotent bounty creation.# Optional: reference to GitHub comment that created this bounty (if from GH comment) - origin_comment_id = models.BigIntegerField(null=True, blank=True) + origin_comment_id = models.BigIntegerField(null=True, blank=True, unique=True)Note:
unique=Truewithnull=Trueallows multiple NULL values in most databases (PostgreSQL, SQLite), which is the desired behavior here.website/api/views.py (1)
1957-1966: Consider restricting allowed HTTP methods.
ModelViewSetexposeslist,create,retrieve,update,partial_update, anddestroyby default. If onlycreateand the customissue_totalaction are needed for Phase 1, restrict the methods to prevent unintended data modification:class BountyViewSet(viewsets.ModelViewSet): queryset = Bounty.objects.all() serializer_class = BountySerializer permission_classes = [IsAuthenticated] + http_method_names = ["get", "post", "head", "options"]Or use
viewsets.GenericViewSetwith explicit mixins for finer control.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (6)
blt/urls.py(3 hunks)website/api/views.py(3 hunks)website/migrations/0261_bounty.py(1 hunks)website/serializers.py(2 hunks)website/test_feed.py(2 hunks)website/views/models_bounty.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- website/test_feed.py
- blt/urls.py
🧰 Additional context used
🧬 Code graph analysis (2)
website/serializers.py (2)
website/views/models_bounty.py (2)
Bounty(7-37)Meta(29-34)website/test_feed.py (2)
BountySerializer(127-144)Meta(128-144)
website/api/views.py (2)
website/serializers.py (1)
BountySerializer(311-314)website/views/models_bounty.py (1)
Bounty(7-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: check_peer_review
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (7)
website/serializers.py (1)
25-25: LGTM!The import follows the established pattern in this file.
website/views/models_bounty.py (1)
7-37: Model design looks solid.Good choices:
DecimalFieldfor monetary amounts prevents floating-point precision issues- Appropriate indexes for expected query patterns
get_user_model()for proper user model referenceSET_NULLon FK deletion preserves bounty historywebsite/api/views.py (4)
60-60: LGTM!Import additions are correct and follow the established patterns in this file.
Also applies to: 77-77
1957-1966: Permission classes are now set - previous concern addressed.The
IsAuthenticatedpermission is now in place. However, the docstring mentions "GitHub Action & Slack integration" - verify that these integrations can authenticate properly with the current permission setup.
1995-2001: Previous concern addressed.The
issue_numberparameter validation is now properly implemented with a try/except block.
2003-2016: LGTM!The aggregation query correctly uses the composite index on
(repo_full_name, issue_number). The response properly converts Decimal to float for JSON serialization.website/migrations/0261_bounty.py (1)
1-44: LGTM!The migration correctly reflects the Bounty model definition with:
- All fields properly typed
- Composite index on
(repo_full_name, issue_number)for efficient lookups- Index on
sponsor_githubfor sponsor queries- Proper FK reference using
settings.AUTH_USER_MODEL- Correct ordering configuration
Contributor
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
Contributor
❌ Pre-commit checks failedThe pre-commit hooks found issues that need to be fixed. Please run the following commands locally to fix them: # Install pre-commit if you haven't already
pip install pre-commit
# Run pre-commit on all files
pre-commit run --all-files
# Or run pre-commit on staged files only
pre-commit runAfter running these commands, the pre-commit hooks will automatically fix most issues. 💡 Tip: You can set up pre-commit to run automatically on every commit by running: pre-commit installPre-commit outputFor more information, see the pre-commit documentation. |
blt/urls.py
Outdated
| router.register(r"organizations", OrganizationViewSet, basename="organizations") | ||
| router.register(r"jobs", JobViewSet, basename="jobs") | ||
|
|
||
| router = routers.DefaultRouter() |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
+318
to
+328
| "issue_url", | ||
| "sponsor_github", | ||
| "sponsor_user", | ||
| "amount", | ||
| "currency", | ||
| "created_at", | ||
| "paid", | ||
| "paid_at", | ||
| "origin_comment_id", | ||
| ] | ||
| read_only_fields = ["id", "created_at", "paid", "paid_at"] |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Comment on lines
1972
to
1978
| origin_comment_id = serializer.validated_data.get("origin_comment_id") | ||
|
|
||
| if origin_comment_id and Bounty.objects.filter(origin_comment_id=origin_comment_id).exists(): | ||
| return Response( | ||
| {"detail": "Bounty for this GitHub comment already exists."}, | ||
| status=status.HTTP_200_OK, | ||
| ) |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
github-actions
bot
added
pre-commit: passed
Comment on lines
+725
to
+735
| .get("total") | ||
| or 0 | ||
| ) | ||
|
|
||
| return Response( | ||
| { | ||
| "repo": repo, | ||
| "issue_number": issue_number, | ||
| "total": float(total), | ||
| } | ||
| ) |
There was a problem hiding this comment.
Bug: The @action decorator and issue_total function are defined at the module level, not as methods within a ViewSet class, which will cause a server startup crash.
Severity: CRITICAL | Confidence: High
🔍 Detailed Analysis
The @action decorator from Django REST Framework is applied to the issue_total function at the module level in website/api/views.py. This decorator is designed to work exclusively on methods within a ViewSet class. Because the function is not a method of a class, the decorator will fail to apply correctly when the module is imported. This will raise an AttributeError or a similar exception during the application's initialization phase, preventing the Django server from starting up successfully.
💡 Suggested Fix
Move the issue_total function and its @action decorator inside the OrganizationViewSet class by indenting them correctly. This will register the function as a valid action on the viewset.
🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.
Location: website/api/views.py#L703-L735
Potential issue: The `@action` decorator from Django REST Framework is applied to the
`issue_total` function at the module level in `website/api/views.py`. This decorator is
designed to work exclusively on methods within a `ViewSet` class. Because the function
is not a method of a class, the decorator will fail to apply correctly when the module
is imported. This will raise an `AttributeError` or a similar exception during the
application's initialization phase, preventing the Django server from starting up
successfully.
Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 7124782
Contributor
❌ Tests failedThe Django tests found issues that need to be fixed. Please review the test output below and fix the failing tests. How to run tests locally# Install dependencies
poetry install --with dev
# Run all tests
poetry run python manage.py test
# Run tests with verbose output
poetry run python manage.py test -v 3
# Run a specific test
poetry run python manage.py test app.tests.TestClass.test_methodTest output (last 100 lines)For more information, see the Django testing documentation. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
website/api/views.py (1)
1982-2004: Duplicate bounty prevention only works withorigin_comment_id.The duplicate check on lines 1988-1993 only executes when
origin_comment_idis provided. If this field is omitted or null, multiple identical bounties can be created for the same issue (samerepo_full_name,issue_number,sponsor_github).While the
origin_comment_idapproach works for GitHub Action integration (Phase 2), the API should also prevent duplicates from other sources (e.g., manual creation, Slack commands in Phase 3).Consider adding fallback duplicate detection:
def create(self, request, *args, **kwargs): serializer = self.get_serializer(data=request.data) serializer.is_valid(raise_exception=True) origin_comment_id = serializer.validated_data.get("origin_comment_id") + repo_full_name = serializer.validated_data.get("repo_full_name") + issue_number = serializer.validated_data.get("issue_number") + sponsor_github = serializer.validated_data.get("sponsor_github") # Early duplicate check if origin_comment_id and Bounty.objects.filter(origin_comment_id=origin_comment_id).exists(): return Response( {"detail": "Bounty for this GitHub comment already exists."}, status=status.HTTP_200_OK, ) + + # Fallback duplicate check: same sponsor, same issue + if sponsor_github and Bounty.objects.filter( + repo_full_name=repo_full_name, + issue_number=issue_number, + sponsor_github=sponsor_github, + paid=False + ).exists(): + return Response( + {"detail": "You already have an unpaid bounty for this issue."}, + status=status.HTTP_200_OK, + ) try: self.perform_create(serializer)Alternatively, add a database-level unique constraint on
(repo_full_name, issue_number, sponsor_github, paid)where paid=False to prevent duplicate active bounties from the same sponsor.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (2)
blt/urls.py(1 hunks)website/api/views.py(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
website/api/views.py (3)
website/serializers.py (1)
BountySerializer(311-328)website/test_feed.py (1)
BountySerializer(127-144)website/views/models_bounty.py (1)
Bounty(7-37)
blt/urls.py (1)
website/api/views.py (1)
BountyViewSet(1977-2004)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run Tests
- GitHub Check: docker-test
🔇 Additional comments (3)
blt/urls.py (1)
401-401: LGTM! Router re-initialization issue resolved.The bounty route registration is correctly added to the existing router instance. The critical bug flagged in previous reviews (router re-initialization that discarded all prior registrations) has been fixed.
website/api/views.py (2)
21-21: LGTM! Imports correctly added.The new imports (
IntegrityError,BountySerializer,Bounty) are properly placed and necessary for the new bounty functionality.Also applies to: 59-59, 75-75
1977-1980: LGTM! Permission classes properly configured.The
BountyViewSetnow includespermission_classes = [IsAuthenticated]as recommended in previous reviews. The queryset exposes all bounties to authenticated users, which is appropriate for Phase 1.
Comment on lines
+702
to
+735
|
|
||
| @action(detail=False, methods=["get"], url_path="issue-total") | ||
| def issue_total(self, request): | ||
| repo = request.query_params.get("repo") | ||
| issue_number = request.query_params.get("issue_number") | ||
|
|
||
| if not repo or not issue_number: | ||
| return Response( | ||
| {"detail": "Parameters 'repo' and 'issue_number' are required"}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
||
| try: | ||
| issue_number = int(issue_number) | ||
| except (ValueError, TypeError): | ||
| return Response( | ||
| {"detail": "Parameter 'issue_number' must be a valid integer"}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
||
| total = ( | ||
| Bounty.objects.filter(repo_full_name=repo, issue_number=issue_number) | ||
| .aggregate(total=Sum("amount")) | ||
| .get("total") | ||
| or 0 | ||
| ) | ||
|
|
||
| return Response( | ||
| { | ||
| "repo": repo, | ||
| "issue_number": issue_number, | ||
| "total": float(total), | ||
| } | ||
| ) |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move issue_total action to BountyViewSet.
The issue_total action is currently added to OrganizationViewSet (line 694), but this is bounty-specific functionality. This creates an endpoint at /api/v1/organizations/issue-total/ instead of the more logical /api/v1/bounties/issue-total/.
The implementation itself is correct (proper validation, aggregation logic), but the architectural placement is wrong.
Apply this diff to move the action to the correct ViewSet:
class OrganizationViewSet(viewsets.ModelViewSet):
queryset = Organization.objects.all()
serializer_class = OrganizationSerializer
permission_classes = (IsAuthenticatedOrReadOnly,)
filter_backends = (filters.SearchFilter,)
search_fields = ("id", "name")
http_method_names = ("get", "post", "put")
-
-@action(detail=False, methods=["get"], url_path="issue-total")
-def issue_total(self, request):
- repo = request.query_params.get("repo")
- issue_number = request.query_params.get("issue_number")
-
- if not repo or not issue_number:
- return Response(
- {"detail": "Parameters 'repo' and 'issue_number' are required"},
- status=status.HTTP_400_BAD_REQUEST,
- )
-
- try:
- issue_number = int(issue_number)
- except (ValueError, TypeError):
- return Response(
- {"detail": "Parameter 'issue_number' must be a valid integer"},
- status=status.HTTP_400_BAD_REQUEST,
- )
-
- total = (
- Bounty.objects.filter(repo_full_name=repo, issue_number=issue_number)
- .aggregate(total=Sum("amount"))
- .get("total")
- or 0
- )
-
- return Response(
- {
- "repo": repo,
- "issue_number": issue_number,
- "total": float(total),
- }
- )Then add it to BountyViewSet (after line 2004):
class BountyViewSet(viewsets.ModelViewSet):
queryset = Bounty.objects.all()
serializer_class = BountySerializer
permission_classes = [IsAuthenticated]
def create(self, request, *args, **kwargs):
serializer = self.get_serializer(data=request.data)
serializer.is_valid(raise_exception=True)
origin_comment_id = serializer.validated_data.get("origin_comment_id")
# Early duplicate check
if origin_comment_id and Bounty.objects.filter(origin_comment_id=origin_comment_id).exists():
return Response(
{"detail": "Bounty for this GitHub comment already exists."},
status=status.HTTP_200_OK,
)
try:
self.perform_create(serializer)
return Response(serializer.data, status=status.HTTP_201_CREATED)
except IntegrityError:
# If two requests arrive at the same time
return Response(
{"detail": "Bounty for this GitHub comment already exists."},
status=status.HTTP_200_OK,
)
+
+ @action(detail=False, methods=["get"], url_path="issue-total")
+ def issue_total(self, request):
+ repo = request.query_params.get("repo")
+ issue_number = request.query_params.get("issue_number")
+
+ if not repo or not issue_number:
+ return Response(
+ {"detail": "Parameters 'repo' and 'issue_number' are required"},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
+
+ try:
+ issue_number = int(issue_number)
+ except (ValueError, TypeError):
+ return Response(
+ {"detail": "Parameter 'issue_number' must be a valid integer"},
+ status=status.HTTP_400_BAD_REQUEST,
+ )
+
+ total = (
+ Bounty.objects.filter(repo_full_name=repo, issue_number=issue_number)
+ .aggregate(total=Sum("amount"))
+ .get("total")
+ or 0
+ )
+
+ return Response(
+ {
+ "repo": repo,
+ "issue_number": issue_number,
+ "total": float(total),
+ }
+ )📝 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.
Suggested change
| @action(detail=False, methods=["get"], url_path="issue-total") | |
| def issue_total(self, request): | |
| repo = request.query_params.get("repo") | |
| issue_number = request.query_params.get("issue_number") | |
| if not repo or not issue_number: | |
| return Response( | |
| {"detail": "Parameters 'repo' and 'issue_number' are required"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| try: | |
| issue_number = int(issue_number) | |
| except (ValueError, TypeError): | |
| return Response( | |
| {"detail": "Parameter 'issue_number' must be a valid integer"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| total = ( | |
| Bounty.objects.filter(repo_full_name=repo, issue_number=issue_number) | |
| .aggregate(total=Sum("amount")) | |
| .get("total") | |
| or 0 | |
| ) | |
| return Response( | |
| { | |
| "repo": repo, | |
| "issue_number": issue_number, | |
| "total": float(total), | |
| } | |
| ) | |
| @action(detail=False, methods=["get"], url_path="issue-total") | |
| def issue_total(self, request): | |
| repo = request.query_params.get("repo") | |
| issue_number = request.query_params.get("issue_number") | |
| if not repo or not issue_number: | |
| return Response( | |
| {"detail": "Parameters 'repo' and 'issue_number' are required"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| try: | |
| issue_number = int(issue_number) | |
| except (ValueError, TypeError): | |
| return Response( | |
| {"detail": "Parameter 'issue_number' must be a valid integer"}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| total = ( | |
| Bounty.objects.filter(repo_full_name=repo, issue_number=issue_number) | |
| .aggregate(total=Sum("amount")) | |
| .get("total") | |
| or 0 | |
| ) | |
| return Response( | |
| { | |
| "repo": repo, | |
| "issue_number": issue_number, | |
| "total": float(total), | |
| } | |
| ) |
🤖 Prompt for AI Agents
In website/api/views.py around lines 702-735 the issue_total action is placed on
OrganizationViewSet but belongs on BountyViewSet; remove the entire method from
OrganizationViewSet and insert the same @action(detail=False, methods=["get"],
url_path="issue-total") def issue_total(self, request): ... implementation into
BountyViewSet after line 2004, preserving the validation, casting, aggregation
and Response logic; ensure proper indentation and imports remain, then run
tests/route checks to confirm the endpoint is now exposed at
/api/v1/bounties/issue-total/.
Contributor
Author
|
@CodeRabbit what i have to resolve now what should be next step and how to tackle form there ? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 16624 🏁 Script executed: Length of output: 8400 🏁 Script executed: Length of output: 28318 🏁 Script executed: Length of output: 4916 🏁 Script executed: Length of output: 1134 @Ankit-singh-dot I've identified the two issues causing the CI failures. Here's exactly what you need to fix: 🔴 Issue 1: Missing Import (Test Failure)Problem: You're using Fix: Add from website.api.views import (
ActivityLogViewSet,
AuthApiViewset,
BountyViewSet, # ← ADD THIS LINE
BugHuntApiViewset,
BugHuntApiViewsetV2,
CheckDuplicateBugApiView,
# ... rest of importsLocation: Around line 23-51 in 🔴 Issue 2: PR Description Format (Workflow Failure)Problem: Your PR description says "adding feature issue number : #3767", but GitHub requires specific keywords to link issues. Fix: Edit your PR description and change the first line from: To: You can use any of these keywords: ✅ Next Steps
Once you push these fixes, the CI checks should pass! The pre-commit check is already passing ✅, so you're almost there. Would you like me to help with anything else once these are fixed? |
Contributor
|
💬 Reminder: Unresolved Conversations Hi @Ankit-singh-dot! This pull request has 5 unresolved conversations that need to be addressed. Please review and resolve the pending discussions so we can move forward with merging this PR. Thank you! 🙏 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
adding feature issue number : #3767
Phase 1: Bounty Backend (Models + API Endpoints)
This PR introduces the foundational backend components required for the upcoming /bounty $X feature described in issue #3767.
It includes the Bounty model, serializer, and REST API endpoints that will be consumed by GitHub Actions and Slack commands in later phases.
What This PR Adds
A new database model to store bounty contributions:
repo_full_name — GitHub repository path (org/repo)
issue_number — Issue number that received the bounty
issue_url — Full GitHub issue URL
sponsor_github — GitHub username of the sponsor
amount, currency — Monetary value of the bounty
origin_comment_id — GitHub comment ID for idempotent creation
paid, paid_at — Integration points with existing payout webhook
created_at — automatic timestamp
This model is structured to support both GitHub and Slack bounty commands.
A simple DRF serializer exposing the full Bounty model.
Used by API endpoints and future ingestion scripts.
POST /api/bounties/
Creates a new bounty record.
Includes safety check to prevent duplicate creation when GitHub sends repeated webhooks (origin_comment_id).
GET /api/bounties/issue-total/?repo=org/repo&issue_number=123
Returns the aggregated bounty total for a specific issue.
This will be used by Phase 2 GitHub Actions to update bounty labels dynamically.
The new API endpoints are registered under:
/api/bounties/
These routes will be consumed by the Phase 2 automation.
Local Testing Completed
Migration successfully applied (0261_bounty)
Serializer imports verified
ViewSet import verified
API endpoints load without errors
=> Why This PR Is Phase 1 Only
To keep contributions reviewable and maintainable, the bounty feature is being delivered in stages:
Phase 1 — Backend (this PR)
Model + API
Phase 2 — GitHub Action Integration
Detect /bounty $X, update labels, post confirmation comment
Phase 3 — Slack /bounty Command
Slack support + GitHub updates
This PR creates the backend foundation required for future phases.
=>>Next Steps After Merge
Once this PR is approved, Phase 2 will introduce:
GitHub Action workflow (bounty-detection.yml)
Node parser for /bounty
Automatic label handling
Confirmation comments via GitHub API
REQUEST FOR REVIEW
Please review the model fields, serializer exposure, and API design.
Feedback is welcome before proceeding with Phase 2.
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.