-
Notifications
You must be signed in to change notification settings - Fork 5
feat: Implement atomic transaction for activating broadcast messages … #11
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,6 +2,7 @@ | |
| from rest_framework.decorators import action, api_view, permission_classes | ||
| from rest_framework.response import Response | ||
| from rest_framework.permissions import IsAuthenticated, AllowAny | ||
| from django.db import transaction | ||
| from .models import BroadcastMessage | ||
| from .serializers import BroadcastMessageSerializer | ||
| from dashboard.models import UserDetails | ||
|
|
@@ -46,8 +47,16 @@ def set_active(self, request, pk=None): | |
| if message.user != request.user and not request.user.is_staff: | ||
| return Response({'error': 'Permission denied'}, status=status.HTTP_403_FORBIDDEN) | ||
|
|
||
| message.active = True | ||
| message.save() | ||
| with transaction.atomic(): | ||
| message = BroadcastMessage.objects.select_for_update().get(pk=pk) | ||
|
|
||
| BroadcastMessage.objects.select_for_update().filter( | ||
| user=message.user, active=True | ||
| ).update(active=False) | ||
|
|
||
| message.active = True | ||
| message.save() | ||
|
Comment on lines
+50
to
+58
|
||
|
|
||
| return Response({'message': 'Message set as active'}) | ||
|
|
||
|
|
||
|
|
@@ -57,13 +66,11 @@ def get_user_broadcast(request, user_slug): | |
| try: | ||
| user_details = UserDetails.objects.select_related('user').get(_slug=user_slug) | ||
|
|
||
| # Get active broadcast message if exists | ||
| active_message = BroadcastMessage.objects.filter( | ||
| user=user_details.user, | ||
| active=True | ||
| ).first() | ||
|
|
||
| # Build response with user details and active message | ||
| response_data = { | ||
| 'username': user_details.user.username, | ||
| 'user_username': user_details.user.username, | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,17 +1,23 @@ | ||
| from django.db import models | ||
| from django.db import models, transaction | ||
| from django.conf import settings | ||
|
|
||
|
|
||
| # Create your models here. | ||
| class BroadcastMessage(models.Model): | ||
| user = models.ForeignKey(settings.AUTH_USER_MODEL, on_delete=models.CASCADE, related_name='messages') | ||
| message = models.TextField() | ||
| active = models.BooleanField(default=True) | ||
|
|
||
| def save(self, *args, **kwargs): | ||
| if self.active: | ||
| BroadcastMessage.objects.filter(user=self.user, active=True).update(active=False) | ||
| super().save(*args, **kwargs) | ||
| with transaction.atomic(): | ||
|
|
||
| BroadcastMessage.objects.select_for_update().filter( | ||
| user=self.user | ||
| ).update(active=False) | ||
| # Save this message as active | ||
| super().save(*args, **kwargs) | ||
|
mihf05 marked this conversation as resolved.
|
||
| else: | ||
| super().save(*args, **kwargs) | ||
|
|
||
| def __str__(self): | ||
| return f'{self.user.username}: {self.message[:20]}' | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The atomic transaction implementation for preventing race conditions in the set_active endpoint lacks test coverage. Consider adding tests that verify: 1) concurrent set_active requests only result in one active message, 2) staff users can properly activate messages for other users, and 3) the transaction properly handles edge cases like activating an already-active message.