-
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
Trigger hide/unhidden schedule to tickets system #212
base: development
Are you sure you want to change the base?
Conversation
Reviewer's Guide by SourceryThis pull request implements a feature to trigger an API call to the tickets system when a schedule is hidden or unhidden. It adds functionality to send a POST request with JWT authentication to update the schedule visibility status in the tickets system. Sequence DiagramsequenceDiagram
participant U as User
participant V as ScheduleView
participant T as trigger_public_schedule
participant TS as Tickets System
U->>V: Toggle schedule visibility
V->>V: Update event feature flags
V->>T: Call task asynchronously
T->>T: Generate JWT token
T->>TS: POST request with visibility status
TS-->>T: Response
T->>T: Handle response/retry if needed
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 the dispatch function. Silently ignoring all exceptions could hide important issues.
- Please add unit tests for the new trigger_public_schedule task to ensure functionality and prevent regressions.
- It's recommended to use environment variables for sensitive information like the EVENTYAY_TICKET_BASE_PATH instead of hardcoding it in the settings.
Here's what I looked at during the review
- 🟡 General issues: 4 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
src/pretalx/orga/views/schedule.py
Outdated
except Exception as e: | ||
# Ignore the error if the task fails | ||
pass |
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 (bug_risk): Consider more specific exception handling
Catching and silently ignoring all exceptions can hide important errors. Consider catching specific exceptions and logging the error for debugging purposes.
except (TaskError, ConnectionError) as e:
logger.warning(f"Task failed: {e}")
except Exception as e:
logger.error(f"Unexpected error in task: {e}")
return jwt_token | ||
|
||
|
||
def set_header_token(user_email): |
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): Consider caching the generated token
If multiple requests are made in quick succession, generating a new token for each request could be inefficient. Consider caching the token and reusing it within its expiration period.
def set_header_token(user_email):
cache_key = f"sso_token_{user_email}"
token = cache.get(cache_key)
if not token:
token = generate_sso_token(user_email)
cache.set(cache_key, token, timeout=3600) # Cache for 1 hour
bind=True, | ||
name="pretalx.orga.trigger_public_schedule", | ||
max_retries=3, | ||
default_retry_delay=60, |
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): Implement exponential backoff for retries
Instead of a fixed retry delay, consider implementing an exponential backoff strategy. This can help reduce unnecessary load on the server in case of persistent issues.
default_retry_delay=60, | |
retry_backoff=True, | |
retry_kwargs={'max_retries': 3}, | |
retry_jitter=True, |
src/pretalx/orga/tasks.py
Outdated
response.raise_for_status() # Raise exception for bad status codes | ||
except requests.RequestException as e: | ||
logger.error( | ||
"Error happen when trigger hide/unhidden schedule to tickets component: %s", |
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: Enhance error logging with more context
Consider including more context in the error log, such as the event_slug and organiser_slug. This will make debugging easier in production environments.
"Error happen when trigger hide/unhidden schedule to tickets component: %s", | |
f"Error occurred when triggering hide/unhide schedule for tickets component. Event: {event_slug}, Organiser: {organiser_slug}. Error: {e}", |
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.
This sounds like a good suggestion.
max_retries=3, | ||
default_retry_delay=60, | ||
) | ||
def trigger_public_schedule( |
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 token generation and request handling into separate functions.
While the new trigger_public_schedule
function adds necessary functionality, we can reduce its complexity without losing any features. Here are some suggestions:
- Extract token generation and header setting into a separate function:
def get_auth_headers(user_email):
jwt_payload = {
"email": user_email,
"has_perms": "orga.edit_schedule",
"exp": datetime.utcnow() + timedelta(hours=1),
"iat": datetime.utcnow(),
}
jwt_token = jwt.encode(jwt_payload, settings.SECRET_KEY, algorithm="HS256")
return {
"Authorization": f"Bearer {jwt_token}",
"Content-Type": "application/json",
}
- Simplify the main function and use a context manager for the request:
@app.task(
bind=True,
name="pretalx.orga.trigger_public_schedule",
max_retries=3,
default_retry_delay=60,
)
def trigger_public_schedule(
self, is_show_schedule, event_slug, organiser_slug, user_email
):
headers = get_auth_headers(user_email)
payload = {"is_show_schedule": is_show_schedule}
ticket_uri = urljoin(
settings.EVENTYAY_TICKET_BASE_PATH,
f"api/v1/{organiser_slug}/{event_slug}/schedule-public/",
)
try:
with requests.post(ticket_uri, json=payload, headers=headers) as response:
response.raise_for_status()
except requests.RequestException as e:
logger.error(
"Error when triggering hide/unhide schedule in tickets component: %s",
e,
)
raise self.retry(exc=e)
These changes simplify the main function, improve separation of concerns, and make the code more maintainable without losing any functionality.
This PR is part of issue fossasia/eventyay-tickets#353
This PR implement:
How has this been tested?
Checklist
Summary by Sourcery
Trigger an API call to the tickets system when the schedule visibility changes, using a Celery task with JWT authorization and retry logic.
New Features:
Enhancements: