-
Notifications
You must be signed in to change notification settings - Fork 43
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
Implement script to create customer account for customer placed order before #277
Implement script to create customer account for customer placed order before #277
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new management command script to create customer accounts for orders that do not have an associated customer account. The script takes organizer and event slugs as input, fetches the relevant orders, checks for existing customer accounts based on order emails, creates new customer accounts if necessary, and links the customer accounts to the orders. File-Level Changes
Tips
|
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.
Hey @lcduong - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using database transactions to ensure atomicity of operations. This will prevent partial updates if the script fails midway.
- For better performance with large datasets, consider implementing batch processing for customer creation and order updates.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
|
||
|
||
class Command(BaseCommand): | ||
help = 'Create SocialApp entries for Eventyay-ticket Provider' |
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.
suggestion: Update the command's help text to accurately describe its functionality
The current help text doesn't match the command's actual functionality. Consider updating it to something like 'Create customer accounts for existing orders in an event'.
help = 'Create SocialApp entries for Eventyay-ticket Provider' | |
help = 'Create customer accounts for existing orders in an event' |
event = Event.objects.get(slug=event_slug, organizer=organizer) | ||
except Organizer.DoesNotExist: | ||
self.stderr.write(self.style.ERROR('Organizer not found.')) | ||
sys.exit(1) |
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.
suggestion: Replace sys.exit(1) with raising CommandError for better error handling
Using sys.exit(1) is not the recommended way to handle errors in Django management commands. Consider raising CommandError instead, which allows for better error reporting and doesn't abruptly terminate the process.
from django.core.management.base import CommandError
# ... (earlier in the code)
except Organizer.DoesNotExist:
raise CommandError('Organizer not found.')
except Event.DoesNotExist:
raise CommandError('Event not found.')
if not customer: | ||
name_parts_data = { | ||
"_scheme": "full", | ||
"full_name":order.email.split("@")[0] |
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.
suggestion: Reconsider using email local part as full name
Using the email's local part as the full name might lead to inappropriate or nonsensical names. Consider using a placeholder like 'Unknown' or leaving it blank, allowing users to update it later.
"full_name":order.email.split("@")[0] | |
"full_name": "Unknown Customer" |
with scopes_disabled(): | ||
orders = Order.objects.filter(event=event) | ||
# Get all orders email and check if they have a customer account or not | ||
for order in orders: |
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.
suggestion (performance): Optimize database queries for better performance
For events with many orders, querying the database for each order could be slow. Consider optimizing by first fetching all unique emails from orders, then bulk creating customers for emails without accounts.
for order in orders: | |
unique_emails = orders.values_list('email', flat=True).distinct() | |
existing_customers = set(Customer.objects.filter(email__in=unique_emails).values_list('email', flat=True)) | |
new_customers = [Customer(email=email) for email in unique_emails if email and email not in existing_customers] | |
Customer.objects.bulk_create(new_customers) |
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.
Looks fine to me, although the suggestions by sourcery-ai also sound reasonable
… before (fossasia#277) * implement script to create customer account for customer placed order before
… before (fossasia#277) * implement script to create customer account for customer placed order before
… before (fossasia#277) * implement script to create customer account for customer placed order before
… before (fossasia#277) * implement script to create customer account for customer placed order before
… before (fossasia#277) * implement script to create customer account for customer placed order before
… before (fossasia#277) * implement script to create customer account for customer placed order before
… before (#277) * implement script to create customer account for customer placed order before
… before (fossasia#277) * implement script to create customer account for customer placed order before
… before (fossasia#277) * implement script to create customer account for customer placed order before
This PR is partly of this issue fossasia/eventyay-talk#166
Implement a script to create customer account for customer who placed order before.
By execute this script create_customer_account: python manage.py create_customer_account
then input organizer and event short name.
Summary by Sourcery
Add a script to create customer accounts for orders that do not have an associated customer account. The script scans all orders for a specified event and links existing customer accounts or creates new ones if necessary.
New Features: