-
Notifications
You must be signed in to change notification settings - Fork 3
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
fix: add fix #241
fix: add fix #241
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 |
---|---|---|
@@ -0,0 +1,28 @@ | ||
# Generated by Django 5.0.8 on 2025-01-15 18:42 | ||
|
||
from django.db import migrations, models | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('newsletter', '0011_alter_newsletter_scheduled_send_time'), | ||
] | ||
|
||
operations = [ | ||
migrations.AddField( | ||
model_name='subscriber', | ||
name='beehiiv_id', | ||
field=models.CharField(blank=True, max_length=255, null=True), | ||
), | ||
migrations.AddField( | ||
model_name='subscriber', | ||
name='sync_error', | ||
field=models.TextField(blank=True, null=True), | ||
), | ||
migrations.AddField( | ||
model_name='subscriber', | ||
name='synced_to_beehiiv', | ||
field=models.BooleanField(default=False), | ||
), | ||
] | ||
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,25 @@ | ||
# Generated by Django 5.0.8 on 2025-01-15 18:43 | ||
|
||
from django.db import migrations | ||
|
||
|
||
class Migration(migrations.Migration): | ||
|
||
dependencies = [ | ||
('newsletter', '0012_subscriber_beehiiv_id_subscriber_sync_error_and_more'), | ||
] | ||
|
||
operations = [ | ||
migrations.RemoveField( | ||
model_name='subscriber', | ||
name='beehiiv_id', | ||
), | ||
migrations.RemoveField( | ||
model_name='subscriber', | ||
name='sync_error', | ||
), | ||
migrations.RemoveField( | ||
model_name='subscriber', | ||
name='synced_to_beehiiv', | ||
), | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,42 @@ | ||
from django.shortcuts import render | ||
from django.http import JsonResponse | ||
from .models import Subscriber | ||
from django.views.decorators.csrf import csrf_exempt | ||
from django.db import IntegrityError, transaction | ||
import logging | ||
|
||
logger = logging.getLogger(__name__) | ||
from django.db import IntegrityError | ||
from django.shortcuts import render | ||
from .models import Subscriber | ||
|
||
@csrf_exempt | ||
@csrf_exempt | ||
def subscribe(request): | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add security measures to compensate for CSRF exemption. The
|
||
""" | ||
Handles POST requests to subscribe an email address. | ||
""" | ||
if request.method == 'POST': | ||
email = request.POST.get('email') | ||
|
||
# Validate email | ||
if not email: | ||
return JsonResponse({'message': 'Email is required'}, status=400) | ||
|
||
Comment on lines
+15
to
17
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Enhance email validation. Current validation only checks if email is present. Add proper email format validation and length checks. # Validate email
if not email:
return JsonResponse({'message': 'Email is required'}, status=400)
+
+# Validate email format
+from django.core.validators import validate_email
+from django.core.exceptions import ValidationError
+try:
+ validate_email(email)
+except ValidationError:
+ return JsonResponse({'message': 'Invalid email format'}, status=400)
+
+# Check maximum length
+if len(email) > 254: # RFC 5321
+ return JsonResponse({'message': 'Email too long'}, status=400)
|
||
try: | ||
with transaction.atomic(): | ||
subscriber = Subscriber.objects.create(email=email, is_active=True) | ||
logger.info(f"Triggering Beehiiv sync for subscriber: {subscriber.id}") | ||
|
||
# Create a new subscriber | ||
Subscriber.objects.create(email=email, is_active=True) | ||
return JsonResponse({'message': 'Subscription successful'}, status=200) | ||
|
||
except IntegrityError: | ||
# Handle case where email is already subscribed | ||
return JsonResponse({'message': 'Email already subscribed'}, status=400) | ||
except Exception as e: | ||
logger.error(f"Error in subscription process: {str(e)}") | ||
return JsonResponse({'message': 'An error occurred'}, status=500) | ||
|
||
# If the request method is not POST, return a 405 Method Not Allowed response | ||
return JsonResponse({'message': 'Method not allowed'}, status=405) | ||
|
||
def unsubscribe(request, email): | ||
""" | ||
Handles unsubscribing an email address. | ||
""" | ||
try: | ||
subscriber = Subscriber.objects.get(email=email) | ||
subscriber.is_active = False | ||
subscriber.save() | ||
return render(request, 'newsletter/unsubscribe_success.html', {'email': email}) | ||
|
||
except Subscriber.DoesNotExist: | ||
return render(request, 'newsletter/unsubscribe_fail.html', {'email': None}) |
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.
🛠️ Refactor suggestion
Consider squashing migrations to avoid unnecessary database operations.
Since these fields (
beehiiv_id
,sync_error
,synced_to_beehiiv
) are removed in the next migration, we should squash these migrations to prevent unnecessary database operations during deployment.Consider squashing migrations using: