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

Accessing the video link does not direct user with ticket to the video room #221

Merged
merged 1 commit into from
Oct 4, 2024

Conversation

lcduong
Copy link
Contributor

@lcduong lcduong commented Oct 4, 2024

This PR closes/references issue #220 . It does so by:
root cause is that attendee identifier from talk and ticket not matches when using auto login.
solution: when login using cookie, also consider attendee identifier

How has this been tested?

Checklist

  • I have added tests to cover my changes.

Summary by Sourcery

Resolve the issue of users not being directed to the video room when accessing the video link by updating the auto-login process to consider the attendee identifier.

Bug Fixes:

  • Fix the issue where accessing the video link does not direct users with a ticket to the video room by ensuring the attendee identifier is considered during auto-login.

Copy link

sourcery-ai bot commented Oct 4, 2024

Reviewer's Guide by Sourcery

This pull request addresses an issue where users with tickets were not being directed to the video room when accessing the video link. The core of the change involves modifying the auto-login process to consider the attendee identifier when logging in using a cookie. Additionally, the PR introduces checks to prevent auto-login attempts on certain paths.

No diagrams generated as the changes look simple and do not need a visual representation.

File-Level Changes

Change Details Files
Modified auto-login process to consider attendee identifier
  • Added a check to skip auto-login for '/callback' and '/signup' paths
  • Included 'customer_identifier' from the payload when updating user information
  • Assigned 'customer_identifier' to user's 'code' field during auto-login
src/pretalx/common/middleware/event.py
Enhanced authentication check in the middleware
  • Extended the condition to skip auto-login when user is authenticated or on specific paths
src/pretalx/common/middleware/event.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 @lcduong - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Please add tests to cover these changes, especially for the new login behavior and SSO token handling.
  • Can you provide a brief explanation of the security implications of these changes, particularly regarding the modification of the user authentication flow?
Here's what I looked at during the review
  • 🟢 General issues: all looks good
  • 🟢 Security: all looks good
  • 🟢 Testing: all looks good
  • 🟢 Complexity: all looks good
  • 🟢 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.

@mariobehling mariobehling merged commit cdbc117 into fossasia:development Oct 4, 2024
8 checks passed
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.

3 participants