-
Notifications
You must be signed in to change notification settings - Fork 48
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 webhook for create/update event #195
Implement webhook for create/update event #195
Conversation
Reviewer's Guide by SourceryThis pull request implements webhooks for create/update events in the pretalx application. It adds new URL patterns for webhook endpoints, introduces task processing for organiser, team, and event webhooks, and implements webhook handler functions with JWT authentication and permission checks. 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 @odkhang - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider improving error handling in webhook processing tasks. Currently, exceptions are logged but not handled, which could lead to silent failures.
- While JWT validation is a good start, consider implementing additional security measures such as rate limiting and IP whitelisting for the webhook endpoints.
- To prevent potential race conditions in asynchronous tasks, consider using database transactions when creating or updating objects.
- The PR lacks tests. Please add comprehensive unit and integration tests for the new webhook functionality to ensure reliability and ease of maintenance.
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟡 Security: 1 issue found
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
if not team: | ||
raise Http404("No Team matches the given query.") | ||
# Update the team object with new data from team_data | ||
for field, value in team_data.items(): |
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.
🚨 issue (security): Potential security risk with setattr in loop
Using setattr in a loop with user-provided data could be dangerous if team_data contains unexpected fields. Consider explicitly updating only allowed fields.
|
||
|
||
@csrf_exempt | ||
def organiser_webhook(request): |
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: Refactor duplicate code in webhook handlers
There's significant code duplication in the three webhook handler functions for checking the Authorization header and token. Consider refactoring this into a decorator or middleware to reduce duplication.
def webhook_auth_required(view_func):
@wraps(view_func)
def wrapper(request, *args, **kwargs):
auth_header = request.headers.get('Authorization')
if not auth_header or not auth_header.startswith('Bearer '):
return JsonResponse({'error': 'Invalid Authorization header'}, status=401)
token = auth_header.split()[1]
# Token validation logic here
return view_func(request, *args, **kwargs)
return wrapper
@csrf_exempt
@webhook_auth_required
def organiser_webhook(request):
organiser.full_clean() | ||
organiser.save() | ||
logger.info(f"Organiser {organiser.name} created successfully.") | ||
# Create an Administrator team for new organiser |
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: Separate team creation logic
Consider moving the logic for creating the 'Administrators' team into a separate function for better separation of concerns.
def create_administrator_team(organiser):
TeamForm(organiser=organiser).save()
# Create an Administrator team for new organiser
create_administrator_team(organiser)
DELETE = "delete" | ||
|
||
|
||
@shared_task |
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.
issue (complexity): Consider refactoring the webhook processing logic into a more generic structure.
The current implementation introduces unnecessary complexity through repetitive code structures. Consider the following improvements:
- Replace the
Action
class with string constants:
CREATE = "create"
UPDATE = "update"
DELETE = "delete"
- Create a generic
process_webhook
function:
@shared_task
def process_webhook(entity_type, data):
try:
action = data.get("action")
if action not in (CREATE, UPDATE, DELETE):
logger.error(f"Unknown action: {action}")
return
entity_handlers = {
"organiser": handle_organiser,
"team": handle_team,
"event": handle_event,
}
handler = entity_handlers.get(entity_type)
if not handler:
logger.error(f"Unknown entity type: {entity_type}")
return
handler(action, data)
except ValidationError as e:
logger.error("Validation error:", e.message_dict)
except Exception as e:
logger.error(f"Error processing {entity_type}:", e)
- Implement entity-specific handler functions:
def handle_organiser(action, data):
if action == CREATE:
organiser = Organiser(name=data.get("name"), slug=data.get("slug"))
organiser.full_clean()
organiser.save()
create_admin_team(organiser)
elif action == UPDATE:
organiser = Organiser.objects.get(slug=data.get("slug"))
organiser.name = data.get("name")
organiser.full_clean()
organiser.save()
elif action == DELETE:
# Implement delete logic here
pass
logger.info(f"Organiser {data.get('name')} {action}d successfully.")
# Implement similar handlers for team and event
This approach reduces code duplication, improves maintainability, and allows for easier addition of new entity types in the future. The generic process_webhook
function handles common logic, while entity-specific functions manage unique processing steps.
This PR closes/references issue #XX. It does so by …
How has this been tested?
Checklist
Summary by Sourcery
Implement webhooks to handle create, update, and delete actions for organisers, teams, and events, and add corresponding URL routes to the application.
New Features:
Enhancements: