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

Add fields to input client id and secret for login providers, fix collapse state on login UI #491

Merged
merged 6 commits into from
Jan 3, 2025

Conversation

HungNgien
Copy link
Collaborator

@HungNgien HungNgien commented Dec 31, 2024

Updates:

  • Allow admin to set client id and secret in admin settings
  • Add doc for social login setup
  • Fix the auto collapse error

image
Add client id and secret fields

image
Resolve the issue where the login form auto-collapses upon page refresh.

Summary by Sourcery

Bug Fixes:

  • Prevent the login form from unexpectedly collapsing upon page refresh.

Copy link

sourcery-ai bot commented Dec 31, 2024

Reviewer's Guide by Sourcery

This pull request implements a mechanism to persist the open/closed state of the login form across page refreshes. This is achieved by storing the state in localStorage using JavaScript and restoring it when the page loads.

Sequence diagram for login form state persistence

sequenceDiagram
    participant User
    participant Browser
    participant LocalStorage
    participant LoginForm

    Note over Browser: Page Load
    Browser->>LocalStorage: Get stored state
    LocalStorage-->>Browser: Return state
    Browser->>LoginForm: Apply stored state (open/closed)

    Note over User: User interaction
    User->>LoginForm: Click toggle button
    LoginForm->>Browser: Toggle form visibility
    Browser->>LocalStorage: Save new state
Loading

State diagram for login form collapse states

stateDiagram-v2
    [*] --> Closed: Initial State
    Closed --> Open: Toggle Button Click
    Open --> Closed: Toggle Button Click

    note right of Open
        State persists after
        page refresh
    end note

    note right of Closed
        State persists after
        page refresh
    end note
Loading

File-Level Changes

Change Details Files
Added JavaScript code to manage the state of the login form.
  • Added a new JavaScript file to handle saving and restoring the state of the login form.
  • Implemented logic to store the state of the login form in localStorage whenever the toggle button is clicked.
  • Added logic to restore the state of the login form from localStorage when the page loads.
src/pretix/static/pretixcontrol/js/ui/collapse_state.js
Modified the login form template to include the new JavaScript file and add an ID to the toggle button.
  • Added an ID to the login form's toggle button to allow the JavaScript to target it.
  • Included the new JavaScript file in the base template to make it available to the login page.
src/pretix/control/templates/pretixcontrol/auth/login.html
src/pretix/control/templates/pretixcontrol/auth/base.html

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.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time. You can also use
    this command to specify where the summary should be inserted.

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

@HungNgien HungNgien marked this pull request as ready for review December 31, 2024 04:56
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 @HungNgien - I've reviewed your changes - here's some feedback:

Overall Comments:

  • Please remove the console.log statement before merging as it shouldn't be included in production code.
Here's what I looked at during the review
  • 🟡 General issues: 2 issues found
  • 🟢 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.

}

// Save state on toggle
toggleLogin.addEventListener('click', function () {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The collapse state is being stored based on the current class rather than the final state after transition.

Consider using Bootstrap's 'shown.bs.collapse' and 'hidden.bs.collapse' events to ensure the correct state is stored after the transition completes.

const collapseStateKey = 'loginFormCollapseState';

// Restore state from localStorage
const storedState = localStorage.getItem(collapseStateKey);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Add error handling for localStorage operations

localStorage might be unavailable in private browsing mode or if quota is exceeded. Consider wrapping these operations in try-catch blocks.

Suggested implementation:

    // Restore state from localStorage
    let storedState = null;
    try {
        storedState = localStorage.getItem(collapseStateKey);
        console.log(storedState);
    } catch (e) {
        console.warn('Failed to read from localStorage:', e);
    }

    if (storedState === 'open') {
        if (loginForm.classList.contains('in')) {
            try {
                localStorage.setItem(collapseStateKey, 'closed');
            } catch (e) {
                console.warn('Failed to write to localStorage:', e);
            }
        } else {

@mariobehling mariobehling requested a review from hongquan January 1, 2025 17:25
@HungNgien HungNgien changed the title Save the state of the Login form when the page is refreshed Add fields to input client id and secret for login providers, fix collapse state on login UI Jan 3, 2025
@mariobehling mariobehling merged commit 55db753 into fossasia:development Jan 3, 2025
4 of 7 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.

2 participants