Sync customer data from ticket component #174
Conversation
Reviewer's Guide by SourceryThis pull request introduces a new management command script to sync customer accounts from the Eventyay ticket component. The script retrieves customer data via an API, checks if the customer email already exists in the local database, and creates new user accounts with unusable passwords if they do not exist. The new users are also associated with a SocialAccount linked to the ticket organizer. The script includes error handling for missing organizers, failed API requests, and user creation issues. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @lcduong - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider using environment variables or a secure configuration method for sensitive data like tokens instead of using input(). This will improve security and make the script more suitable for production use.
- Improve error handling by catching specific exceptions and providing more detailed error messages. This will make troubleshooting easier in case of failures.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟡 Security: 1 issue found
- 🟢 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.
| try: | ||
| Organiser.objects.get(slug=organizer_slug) | ||
| except Organiser.DoesNotExist: | ||
| self.stderr.write(self.style.ERROR('Organizer not found.')) | ||
| return |
There was a problem hiding this comment.
suggestion: Consider using more specific exception handling
Catching all exceptions might hide important errors. Consider catching and handling specific exceptions (e.g., requests.RequestException) to provide more informative error messages and potentially handle different error scenarios differently.
| try: | |
| Organiser.objects.get(slug=organizer_slug) | |
| except Organiser.DoesNotExist: | |
| self.stderr.write(self.style.ERROR('Organizer not found.')) | |
| return | |
| try: | |
| Organiser.objects.get(slug=organizer_slug) | |
| except Organiser.DoesNotExist: | |
| self.stderr.write(self.style.ERROR('Organizer not found.')) | |
| return | |
| except Organiser.MultipleObjectsReturned: | |
| self.stderr.write(self.style.ERROR('Multiple organizers found with the same slug.')) | |
| return |
| parser.add_argument('--ticket-url', type=str, help='Ticket Url') | ||
| parser.add_argument('--ticket-token', type=str, help='Ticket token') | ||
|
|
||
| def handle(self, *args, **options): |
There was a problem hiding this comment.
suggestion: Refactor the handle method for better separation of concerns
The handle method is currently responsible for argument handling, API requests, and database operations. Consider breaking these responsibilities into separate methods for improved readability and maintainability.
def handle(self, *args, **options):
self.get_input_data(options)
self.make_api_request()
self.update_database()
def get_input_data(self, options):
self.organizer_slug = options.get('ticket-organizer-slug') or input('Enter Ticket Organizer Slug: ')
self.ticket_url = options.get('ticket-url') or input('Enter Ticket Url: ')
def make_api_request(self):
# API request logic here
def update_database(self):
# Database update logic here
| except Organiser.DoesNotExist: | ||
| self.stderr.write(self.style.ERROR('Organizer not found.')) | ||
| return | ||
| url = f'{ticket_url}/api/v1/organizers/{organizer_slug}/customers/' |
There was a problem hiding this comment.
🚨 suggestion (security): Use urljoin for safer URL construction
Directly inserting user input into URLs can lead to security vulnerabilities. Consider using urllib.parse.urljoin() or a similar method to construct URLs safely.
| url = f'{ticket_url}/api/v1/organizers/{organizer_slug}/customers/' | |
| from urllib.parse import urljoin | |
| url = urljoin(ticket_url, f'api/v1/organizers/{organizer_slug}/customers/') |
| except Exception as e: | ||
| self.stderr.write(self.style.ERROR('Error when creating customer: ' + customer['email'])) | ||
| continue |
There was a problem hiding this comment.
suggestion: Improve error handling when creating customers
The current error handling silently continues on any exception. Consider logging the specific error (e.g., str(e)) for easier debugging. Also, you might want to handle specific exceptions differently based on their type.
except Exception as e:
error_message = f"Error when creating customer {customer['email']}: {str(e)}"
self.stderr.write(self.style.ERROR(error_message))
logger.error(error_message)
if isinstance(e, IntegrityError):
self.stderr.write(self.style.WARNING("Customer may already exist. Skipping."))
elif isinstance(e, ValidationError):
self.stderr.write(self.style.WARNING("Invalid customer data. Skipping."))
continue
This PR closes/references issue #166 . It does so by
python manage.py sync_customer_accountinput data:
How it work:
How has this been tested?
Checklist
Summary by Sourcery
Add a management command to sync customer accounts from the Eventyay ticket component to the talk component, creating new user accounts if they do not already exist.
New Features: