BREAKING CHANGE: votes are now transmitted live via web sockets.#24
BREAKING CHANGE: votes are now transmitted live via web sockets.#24aniebietafia merged 1 commit intomainfrom
Conversation
Signed-off-by: aniebietafia <aniebietafia87@gmail.com>
WalkthroughThis PR refactors WebSocket routing from Changes
Sequence DiagramsequenceDiagram
actor Client
participant View as PollViewSet.vote()
participant Serializer as VoteSerializer
participant DB as Database
participant Signal as post_save Signal
participant Cache as Channel Layer
participant WS as WebSocket Consumer
Client->>View: POST vote (poll_id, option_id)
View->>View: Normalize request.data & inject poll_id
View->>Serializer: Initialize with poll_id + option_id
Serializer->>DB: Fetch Poll by poll_id
Serializer->>DB: Fetch PollOption by (poll, index=option_id)
Serializer->>DB: Validate vote rules & check duplicates
alt Vote Valid
Serializer->>DB: Create Vote instance
DB->>Signal: Trigger post_save signal
Signal->>DB: Atomic increment PollOption.vote_count
Signal->>DB: Fetch updated option counts for poll
Signal->>Signal: Format data: {id: index, vote_count}
Signal->>Cache: group_send poll_update
Cache->>WS: Broadcast updated vote counts
WS->>Client: WebSocket message with new counts
else Vote Invalid
Serializer->>Client: Return validation errors
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
polls/models.py (1)
82-85: Consider usingconstraintsinstead of deprecatedunique_together.
unique_togetheris soft-deprecated in favor ofUniqueConstraintwhich provides more flexibility.class Meta: ordering = ["index"] - # Ensures we can't have same index in the same poll - unique_together = ["poll", "index"] + constraints = [ + models.UniqueConstraint(fields=["poll", "index"], name="unique_poll_option_index") + ]polls/views.py (1)
168-173: Redundant validation—serializer already ensures option belongs to poll.The
VoteSerializer.validate()method (line 56 in serializers.py) already fetches the option viaPollOption.objects.get(poll=poll, index=option_index), guaranteeingoption.poll == poll. This check will always pass after serializer validation succeeds.Consider removing the redundant check:
serializer.is_valid(raise_exception=True) - # Ensure the option belongs to the poll in the URL - if serializer.validated_data["option"].poll != poll: - return Response( - {"error": "Invalid option for this poll."}, - status=status.HTTP_400_BAD_REQUEST, - ) - serializer.save()polls/serializers.py (1)
49-58: Consider exception chaining for better debugging.When re-raising as
ValidationError, chaining withfrom Noneorfrom errimproves traceability.try: poll = Poll.objects.get(pk=poll_id) except Poll.DoesNotExist: - raise serializers.ValidationError("Poll not found.") + raise serializers.ValidationError("Poll not found.") from None try: option = PollOption.objects.get(poll=poll, index=option_index) except PollOption.DoesNotExist: - raise serializers.ValidationError("Invalid option for this poll.") + raise serializers.ValidationError("Invalid option for this poll.") from None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
core/__pycache__/asgi.cpython-313.pycis excluded by!**/*.pycpolls/__pycache__/routing.cpython-313.pycis excluded by!**/*.pyc
📒 Files selected for processing (9)
.github/workflows/ci.yml(1 hunks)core/asgi.py(2 hunks)core/routing.py(1 hunks)polls/migrations/0003_alter_polloption_options_polloption_index_and_more.py(1 hunks)polls/models.py(1 hunks)polls/routing.py(0 hunks)polls/serializers.py(3 hunks)polls/signals.py(2 hunks)polls/views.py(2 hunks)
💤 Files with no reviewable changes (1)
- polls/routing.py
🧰 Additional context used
🧬 Code graph analysis (4)
core/routing.py (1)
polls/consumers.py (1)
PollConsumer(9-50)
polls/signals.py (1)
polls/models.py (2)
Vote(91-107)PollOption(71-88)
polls/serializers.py (2)
polls/models.py (7)
Poll(26-68)PollCategory(8-23)PollOption(71-88)Vote(91-107)Meta(82-85)Meta(103-107)is_expired(67-68)polls/utils.py (2)
get_client_ip(8-15)get_country_from_ip(18-44)
polls/views.py (1)
polls/serializers.py (1)
VoteSerializer(31-119)
🪛 Ruff (0.14.6)
polls/migrations/0003_alter_polloption_options_polloption_index_and_more.py
8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
12-26: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
polls/models.py
83-83: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
85-85: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
polls/serializers.py
27-27: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
28-28: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
38-38: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
39-39: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
52-52: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
52-52: Avoid specifying long messages outside the exception class
(TRY003)
58-58: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
58-58: Avoid specifying long messages outside the exception class
(TRY003)
62-62: Avoid specifying long messages outside the exception class
(TRY003)
65-65: Avoid specifying long messages outside the exception class
(TRY003)
69-71: Avoid specifying long messages outside the exception class
(TRY003)
81-83: Avoid specifying long messages outside the exception class
(TRY003)
149-162: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
163-163: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
190-192: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
217-229: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (1)
- GitHub Check: Run Tests & Lint
🔇 Additional comments (8)
core/routing.py (1)
1-7: LGTM! WebSocket routing correctly configured.The routing setup follows Django Channels conventions. Consider using a stricter UUID regex pattern like
(?P<poll_id>[0-9a-f-]{36})to reject malformed poll IDs at the routing layer, though current validation can occur in the consumer.core/asgi.py (1)
8-8: LGTM! Routing correctly migrated to core module.The WebSocket routing now correctly references
core.routing.websocket_urlpatterns, and security middleware (AllowedHostsOriginValidator,AuthMiddlewareStack) remains properly applied.Also applies to: 21-21
polls/models.py (1)
76-77: LGTM! Index field correctly implemented.The
indexfield with default and the updated__str__representation are well-implemented for the index-based option identification system.Also applies to: 87-88
polls/signals.py (1)
49-53: Good error handling for broadcast failures.Catching exceptions and logging without crashing the request is the right approach—the vote is already persisted, so a broadcast failure shouldn't return a 500 to the user.
polls/views.py (2)
82-87: LGTM! Creator injection correctly implemented.The
perform_createmethod properly injects the authenticated user as the poll creator.
157-165: Data normalization is correct but could be simplified.The conditional handles both
dictandQueryDicttypes, thoughdict(request.data)on aQueryDictmay flatten multi-value keys. For this use case (single values), it works correctly.polls/serializers.py (2)
21-28: LGTM! Index-based ID mapping correctly implemented.Mapping
idtoindexprovides a stable, user-friendly identifier for poll options instead of exposing database PKs.
165-202: LGTM! Atomic poll creation with index assignment.The transaction ensures poll and options are created together, and indices are correctly assigned starting from 1. The duration calculation using
relativedeltacorrectly handles months/years.
| pip install coverage | ||
| coverage run manage.py test | ||
| coverage report --fail-under=40 | ||
| coverage report --fail-under=30 |
There was a problem hiding this comment.
Do not lower coverage threshold for a breaking change PR.
Reducing the coverage threshold from 40% to 30% during a BREAKING CHANGE PR with substantial refactoring (WebSocket routing, model changes, serializer updates, signal handling) is a risky trade-off that masks potential gaps in test coverage for critical changes.
Breaking changes require higher—not lower—test coverage to ensure correctness and prevent regressions. If coverage is dropping, the solution is to improve test coverage for the new/refactored code, not to lower the bar.
Recommendation: Revert the threshold to 40% (or higher) and ensure comprehensive test coverage for all breaking changes, especially:
- WebSocket routing refactoring
PollOption.indexfield introduction- Index-based identifier changes in serializers
- Signal handling updates
If the current code cannot meet the 40% threshold, this indicates inadequate test coverage that should be addressed before merging a breaking change.
🤖 Prompt for AI Agents
.github/workflows/ci.yml around line 72: the CI coverage threshold was lowered
to 30% for a breaking-change PR; revert the change by restoring the threshold to
40% (replace the current line with "coverage report --fail-under=40") and do not
merge with a reduced threshold — instead add/adjust tests to cover the
refactored areas (WebSocket routing, PollOption.index, serializer index-based
IDs, signal handling) so the pipeline passes at the 40% minimum.
| migrations.AddField( | ||
| model_name="polloption", | ||
| name="index", | ||
| field=models.PositiveIntegerField(default=1), | ||
| ), | ||
| migrations.AlterUniqueTogether( | ||
| name="polloption", | ||
| unique_together={("poll", "index")}, | ||
| ), |
There was a problem hiding this comment.
Migration will fail if existing PollOptions share the same poll.
Adding index with default=1 and then applying unique_together on (poll, index) will cause an IntegrityError if any poll already has multiple options—they'll all have index=1.
Split this into two migrations:
- Add the
indexfield (without the constraint) - Run a data migration to assign unique indices per poll
- Apply
unique_together
Example data migration:
from django.db import migrations
def assign_option_indices(apps, schema_editor):
PollOption = apps.get_model('polls', 'PollOption')
from django.db.models import Window, F
from django.db.models.functions import RowNumber
# Group by poll and assign sequential indices
for poll_id in PollOption.objects.values_list('poll_id', flat=True).distinct():
options = PollOption.objects.filter(poll_id=poll_id).order_by('id')
for idx, option in enumerate(options, start=1):
option.index = idx
option.save(update_fields=['index'])
class Migration(migrations.Migration):
dependencies = [("polls", "0003_add_index_field")]
operations = [migrations.RunPython(assign_option_indices, migrations.RunPython.noop)]🤖 Prompt for AI Agents
polls/migrations/0003_alter_polloption_options_polloption_index_and_more.py
lines 17-25: the current migration adds an index field with default=1 and
immediately applies unique_together on (poll, index), which will produce
IntegrityError for polls with multiple options; fix by splitting into three
steps: (1) create a migration that adds the PositiveIntegerField index (keep a
harmless default or allow null temporarily) without any unique constraint, (2)
add a data migration (RunPython) that iterates PollOption grouped by poll_id,
orders each group deterministically (e.g., by id), assigns sequential index
values and saves with update_fields=['index'], and (3) create a follow-up schema
migration that applies AlterUniqueTogether/UniqueConstraint on (poll, index)
after the data migration has ensured uniqueness.
| def validate(self, attrs): | ||
| request = self.context["request"] | ||
| user = request.user if request.user.is_authenticated else None | ||
| request = self.context.get("request") | ||
| user = request.user if request and request.user.is_authenticated else None | ||
| ip_address = get_client_ip(request) |
There was a problem hiding this comment.
Potential AttributeError if request context is missing.
Line 44 calls get_client_ip(request) before verifying request is not None. If serializer is instantiated without context, this will raise AttributeError.
def validate(self, attrs):
request = self.context.get("request")
+ if not request:
+ raise serializers.ValidationError("Request context is required.")
user = request.user if request and request.user.is_authenticated else None
ip_address = get_client_ip(request)📝 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.
| def validate(self, attrs): | |
| request = self.context["request"] | |
| user = request.user if request.user.is_authenticated else None | |
| request = self.context.get("request") | |
| user = request.user if request and request.user.is_authenticated else None | |
| ip_address = get_client_ip(request) | |
| def validate(self, attrs): | |
| request = self.context.get("request") | |
| if not request: | |
| raise serializers.ValidationError("Request context is required.") | |
| user = request.user if request and request.user.is_authenticated else None | |
| ip_address = get_client_ip(request) |
🤖 Prompt for AI Agents
In polls/serializers.py around lines 41 to 44, the code calls
get_client_ip(request) without ensuring request is present, which can raise
AttributeError when context is missing; modify the validate method to first
check if request is truthy (and optionally request.user.is_authenticated) and
only call get_client_ip(request) when request exists, otherwise set ip_address
to None (or an appropriate default), then proceed using that safe value.
| def get_total_votes(self, obj): | ||
| # Sum the pre-calculated vote_counts from options | ||
| return sum(opt.vote_count for opt in obj.options.all()) | ||
|
|
||
| def get_has_voted(self, obj): | ||
| request = self.context.get("request") | ||
| if request and request.user.is_authenticated: | ||
| return Vote.objects.filter(poll=obj, user=request.user).exists() | ||
| return False |
There was a problem hiding this comment.
Potential N+1 queries in list serializer.
get_total_votes iterates obj.options.all() and get_has_voted queries Vote.objects for each poll in the list. Without prefetching, this causes N+1 queries.
Ensure the viewset prefetches the related data:
# In PollViewSet.get_queryset()
return Poll.objects.prefetch_related('options').select_related(...)For has_voted, consider annotating at the queryset level:
from django.db.models import Exists, OuterRef
user_vote_subquery = Vote.objects.filter(poll=OuterRef('pk'), user=request.user)
queryset = queryset.annotate(has_voted=Exists(user_vote_subquery))Then expose via a SerializerMethodField that reads the annotation instead of issuing a query.
🤖 Prompt for AI Agents
In polls/serializers.py around lines 231-239, the serializer methods cause N+1
queries: get_total_votes iterates obj.options.all() without guaranteed prefetch
and get_has_voted issues a Vote query per poll; update PollViewSet.get_queryset
to prefetch_related('options') (and select_related for any FK) so options are
fetched once, and annotate the queryset with a has_voted boolean using
Exists/OuterRef tied to request.user so the serializer can read the annotation
instead of querying; then change get_total_votes to sum the already-prefetched
option.vote_count and get_has_voted to return the annotated field (or fall back
safely if annotation missing).
| PollOption.objects.filter(id=instance.option.id).update( | ||
| vote_count=F("vote_count") + 1 | ||
| ) | ||
| ) | ||
|
|
||
| # Real-time Broadcast using WebSockets | ||
| channel_layer = get_channel_layer() | ||
| poll_id = str(instance.poll.poll_id) | ||
| room_group_name = f"poll_{poll_id}" | ||
| # 2. Real-time Broadcast | ||
| channel_layer = get_channel_layer() | ||
| # Use raw poll_id to avoid extra DB lookup for the Poll object | ||
| poll_id = str(instance.poll_id) | ||
| room_group_name = f"poll_{poll_id}" | ||
|
|
||
| # Fetch fresh counts | ||
| # We fetch all options for this poll to ensure the frontend is fully synced | ||
| options = PollOption.objects.filter(poll_id=poll_id).only( | ||
| "index", "vote_count" | ||
| ) | ||
|
|
||
| # Format data: Map 'index' to 'id' for the frontend | ||
| formatted_data = [ | ||
| {"id": opt.index, "vote_count": opt.vote_count} for opt in options | ||
| ] |
There was a problem hiding this comment.
Potential stale read after atomic increment.
The atomic update() on line 23-25 doesn't refresh the queryset. The subsequent query on line 35 may return stale vote_count values due to Django's query caching or if another concurrent vote is processed between these two operations.
Consider refreshing from the database or using select_for_update() for stronger consistency:
PollOption.objects.filter(id=instance.option.id).update(
vote_count=F("vote_count") + 1
)
# 2. Real-time Broadcast
channel_layer = get_channel_layer()
poll_id = str(instance.poll_id)
room_group_name = f"poll_{poll_id}"
- # Fetch fresh counts
- options = PollOption.objects.filter(poll_id=poll_id).only(
- "index", "vote_count"
- )
+ # Fetch fresh counts - force DB read
+ options = list(PollOption.objects.filter(poll_id=poll_id).only(
+ "index", "vote_count"
+ ))The list() forces evaluation, though in high-concurrency scenarios consider whether eventual consistency is acceptable for the real-time display.
1 participant
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.