Skip to content
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

Open
wants to merge 3 commits into
base: development
Choose a base branch
from

Conversation

odkhang
Copy link
Collaborator

@odkhang odkhang commented Oct 1, 2024

This PR is part of issue fossasia/eventyay-tickets#353
This PR implement:

  1. When schedule is hide/unhidden, will trigger an API call to tickets system. (using jwt token for authorize)

How has this been tested?

Checklist

  • I have added tests to cover my changes.

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:

  • Implement a feature to trigger an API call to the tickets system when the schedule is hidden or unhidden, using a JWT token for authorization.

Enhancements:

  • Add a Celery task to handle the API call for triggering the public schedule, with retry logic and error handling.

Copy link

sourcery-ai bot commented Oct 1, 2024

Reviewer's Guide by Sourcery

This 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 Diagram

sequenceDiagram
    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
Loading

File-Level Changes

Change Details Files
Implement schedule visibility update trigger
  • Add try-except block to handle potential exceptions
  • Create and call a new asynchronous task 'trigger_public_schedule'
  • Pass event and user information to the new task
src/pretalx/orga/views/schedule.py
Create new task file for handling schedule visibility updates
  • Implement JWT token generation for authentication
  • Create function to set request headers with JWT token
  • Implement 'trigger_public_schedule' task with retry logic
  • Send POST request to tickets system API with schedule visibility status
src/pretalx/orga/tasks.py

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it.

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

Copy link

@sourcery-ai sourcery-ai bot left a 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

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

Comment on lines 253 to 255
except Exception as e:
# Ignore the error if the task fails
pass
Copy link

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):
Copy link

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,
Copy link

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.

Suggested change
default_retry_delay=60,
retry_backoff=True,
retry_kwargs={'max_retries': 3},
retry_jitter=True,

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",
Copy link

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.

Suggested change
"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}",

max_retries=3,
default_retry_delay=60,
)
def trigger_public_schedule(
Copy link

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:

  1. 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",
    }
  1. 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants