Feat/export subscribers #234
Conversation
|
Warning Rate limit exceeded@losndu has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 35 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)
WalkthroughA new management command has been introduced in the Django application to facilitate the export of subscriber data to a CSV file. This command retrieves subscriber records from the Changes
Poem
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: 2
🧹 Nitpick comments (3)
server/apps/newsletter/management/commands/export_subscribers.py (3)
3-3: Remove unnecessary commentThe inline comment "# Correct import path" is redundant and should be removed.
-from apps.newsletter.models import Subscriber # Correct import path +from apps.newsletter.models import Subscriber
5-7: Enhance command help textThe help text could be more descriptive to better explain the command's functionality and output format.
- help = 'Export subscribers to a CSV file' + help = 'Export all newsletter subscribers to a CSV file (email, status, subscription date)'
8-24: Add privacy controls and audit loggingThe command should include privacy controls and audit logging for security purposes:
- Consider GDPR implications of exporting personal data
- Add audit logging for tracking exports
- Consider adding command options for filtering data
Consider implementing these security improvements:
+ def add_arguments(self, parser): + parser.add_argument( + '--days', + type=int, + help='Export only subscribers from the last N days' + ) + def handle(self, *args, **kwargs): + from django.utils import timezone + from datetime import timedelta + from django.core.management.base import CommandError + + # Log export operation + from django.contrib.admin.models import LogEntry, ADDITION + from django.contrib.contenttypes.models import ContentType + + # Filter by days if specified + queryset = Subscriber.objects.all() + if kwargs['days']: + cutoff_date = timezone.now() - timedelta(days=kwargs['days']) + queryset = queryset.filter(subscribed_at__gte=cutoff_date) + + # Create audit log + LogEntry.objects.log_action( + user_id=1, # System user + content_type_id=ContentType.objects.get_for_model(Subscriber).pk, + object_id=None, + object_repr='Subscriber Export', + action_flag=ADDITION, + change_message=f'Exported {queryset.count()} subscribers to CSV' + )
server/apps/newsletter/management/commands/export_subscribers.py
Outdated
Show resolved
Hide resolved
server/apps/newsletter/management/commands/export_subscribers.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
server/apps/newsletter/management/commands/export_subscribers.py (2)
11-19: Add timestamp to filename to prevent overwrites.Consider adding a timestamp to the filename to ensure uniqueness and maintain a history of exports.
- csv_filename = os.path.join(export_dir, 'subscribers_export.csv') + timestamp = timezone.now().strftime('%Y%m%d_%H%M%S') + csv_filename = os.path.join(export_dir, f'subscribers_export_{timestamp}.csv')
42-45: Implement more specific error handling.Consider catching specific exceptions and cleaning up partial files on failure.
- except Exception as e: - # Handle errors and log them - self.stderr.write(self.style.ERROR(f'Error exporting subscribers: {str(e)}')) - raise + except (IOError, OSError) as e: + self.stderr.write(self.style.ERROR(f'File operation error: {str(e)}')) + # Clean up partial file + if os.path.exists(csv_filename): + os.remove(csv_filename) + raise + except csv.Error as e: + self.stderr.write(self.style.ERROR(f'CSV writing error: {str(e)}')) + if os.path.exists(csv_filename): + os.remove(csv_filename) + raise + except Exception as e: + self.stderr.write(self.style.ERROR(f'Unexpected error: {str(e)}')) + if os.path.exists(csv_filename): + os.remove(csv_filename) + raise
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
server/apps/newsletter/management/commands/export_subscribers.py(1 hunks)
🔇 Additional comments (2)
server/apps/newsletter/management/commands/export_subscribers.py (2)
1-10: LGTM! Well-structured imports and class definition.The imports follow Django conventions and the command class is properly defined with helpful documentation.
8-45: Add unit tests for the export command.The command needs test coverage to ensure reliability. Consider testing:
- Empty subscriber list
- Large dataset handling
- Error conditions (file permissions, disk space)
- Data validation
Would you like me to help generate a test suite for this command?
server/apps/newsletter/management/commands/export_subscribers.py
Outdated
Show resolved
Hide resolved
Summary by CodeRabbit