Skip to content

Conversation

@hk2166
Copy link

@hk2166 hk2166 commented Dec 17, 2025

#1525
feat: add preferred login provider selection for social auth

  • Add 'preferred' field to ProviderConfig schema to track default login method
  • Update social auth settings page with radio buttons to select preferred provider
  • Enforce single preferred provider constraint in SocialLoginView
  • Reorder login UI to display preferred provider first with emphasis
  • Add visual separator ("or use the following:") for secondary login options
  • Pre-select "Keep me logged in" checkbox when preferred provider is configured
  • Fix template syntax issues with blocktrans and with tags

Allows administrators to designate one social login provider (Wikimedia,
GitHub, Google) as the primary/default option, which will be prominently
displayed at the top of the login page while other methods remain accessible
below.

Copilot AI review requested due to automatic review settings December 17, 2025 13:27
Copy link
Contributor

@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.

Sorry @hk2166, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds functionality for administrators to designate one social login provider (Wikimedia, GitHub, Google) as the preferred/default login option, which will be displayed prominently at the top of the login page.

Key Changes:

  • Added 'preferred' boolean field to the ProviderConfig schema to track the default login method
  • Updated the social auth settings page with radio buttons for selecting a preferred provider
  • Modified the login page template to reorder UI elements, displaying the preferred provider first with emphasis and showing a separator for secondary login options
  • Pre-selects the "Keep me logged in" checkbox when a preferred provider is configured

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 14 comments.

Show a summary per file
File Description
app/eventyay/plugins/socialauth/schemas/login_providers.py Adds 'preferred' field to ProviderConfig schema
app/eventyay/plugins/socialauth/views.py Updates POST handler to save preferred provider selection
app/eventyay/plugins/socialauth/templates/socialauth/social_auth_settings.html Adds radio button UI for selecting preferred provider and fixes blocktrans syntax
app/eventyay/eventyay_common/views/auth.py Pre-selects "Keep me logged in" when preferred provider exists
app/eventyay/eventyay_common/templates/eventyay_common/auth/login.html Reorders login buttons to show preferred provider first with visual separator
setup.md Adds Docker setup instructions (unrelated to social auth feature)
deployment/eventyay.cfg Adds deployment configuration template (unrelated to social auth feature)
deployment/docker-compose.yml Adds build context and volume mounts for development (unrelated to social auth feature)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

<input type="radio" name="preferred_login_provider" value="{{ provider }}" {% if settings.preferred %}checked{% endif %}/>
{% trans "Make this login the preferred login method" %}
</label>
</div>
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The radio button group for preferred login provider lacks a "None" or "No preferred provider" option. Once a user selects a preferred provider, they cannot deselect it to return to having no preferred provider. Consider adding a radio button option with an empty value or a special "none" value to allow administrators to clear the preferred provider selection.

Suggested change
</div>
</div>
{% if forloop.first %}
<div class="radio">
<label>
<input type="radio" name="preferred_login_provider" value=""/>
{% trans "No preferred login method" %}
</label>
</div>
{% endif %}

Copilot uses AI. Check for mistakes.
volumes:
- ./data/data:/data
- ./eventyay.cfg:/etc/eventyay.cfg:ro
- ../app/eventyay:/usr/src/app/eventyay
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

Mounting the local source code directory as a volume ('../app/eventyay:/usr/src/app/eventyay') in production deployment configuration is risky. This pattern is typically used for development hot-reloading but can lead to inconsistent deployments, version mismatches, and security issues in production. The code should be baked into the Docker image at build time rather than mounted at runtime for production deployments.

Copilot uses AI. Check for mistakes.
Comment on lines +65 to +73
{% comment %} Check if there are any other enabled providers {% endcomment %}
{% with has_other_providers=False %}
{% for provider, settings in login_providers.items %}
{% if settings.state and not settings.preferred %}
{% with has_other_providers=True %}{% endwith %}
{% endif %}
{% endfor %}
{% endwith %}

Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The variable name 'settings' shadows the 'settings' parameter from the outer loop, making it impossible to access the provider settings inside the inner loop. This will cause the condition to always evaluate to False since you're checking 'settings.get('preferred', False)' which refers to the inner 'settings' variable (initialized to False) rather than the provider settings dictionary.

The 'with' tag in Django templates is meant to create new context variables, not to update existing ones. This pattern doesn't work as intended. Consider using a custom template tag or filter, or restructuring the logic to avoid this nested loop pattern.

Suggested change
{% comment %} Check if there are any other enabled providers {% endcomment %}
{% with has_other_providers=False %}
{% for provider, settings in login_providers.items %}
{% if settings.state and not settings.preferred %}
{% with has_other_providers=True %}{% endwith %}
{% endif %}
{% endfor %}
{% endwith %}

Copilot uses AI. Check for mistakes.
<a href='{% url "plugins:socialauth:social.oauth.login" provider %}{% append_next request.GET.next %}'
data-method="post" class="btn btn-primary btn-block btn-lg" style="font-weight: bold;">
{% with provider|capfirst as provider_capitalized %}
{% blocktranslate %}Login with {{ provider_capitalized }}{% endblocktranslate %}
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The 'blocktranslate' tag requires explicit variable naming when used with the 'with' tag. The correct syntax should be 'blocktranslate with name=provider_capitalized' instead of just 'blocktranslate'. Without the 'with' keyword, Django won't properly interpolate the variable into the translation string.

Copilot uses AI. Check for mistakes.
Comment on lines +148 to +150
settings.get('preferred', False)
for settings in login_providers.values()
if settings.get('state', False)
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The variable name 'settings' in the loop parameter shadows the Django 'settings' module that's being accessed on line 148. This makes the code confusing and error-prone. Consider renaming the loop variable to something like 'provider_settings' or 'config' to avoid the name collision and improve clarity.

Suggested change
settings.get('preferred', False)
for settings in login_providers.values()
if settings.get('state', False)
provider_settings.get('preferred', False)
for provider_settings in login_providers.values()
if provider_settings.get('state', False)

Copilot uses AI. Check for mistakes.

# Initialize form with keep_logged_in pre-selected if there's a preferred provider
initial_data = {}
if has_preferred_provider and 'keep_logged_in' in LoginForm(backend=backend, request=request).fields:
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

Creating a LoginForm instance just to check if 'keep_logged_in' exists in the fields is inefficient. The LoginForm class definition shows that 'keep_logged_in' is always present as a field (line 24 of auth.py forms). This check is unnecessary and creates an extra form instance that's immediately discarded. Simply remove the condition or verify the field existence in a more efficient way.

Suggested change
if has_preferred_provider and 'keep_logged_in' in LoginForm(backend=backend, request=request).fields:
if has_preferred_provider:

Copilot uses AI. Check for mistakes.
Comment on lines 54 to +62
{% for provider, settings in login_providers.items %}
{% if settings.state %}
{% if settings.state and settings.preferred %}
<a href='{% url "plugins:socialauth:social.oauth.login" provider %}{% append_next request.GET.next %}'
data-method="post" class="btn btn-primary btn-block btn-lg" style="font-weight: bold;">
{% with provider|capfirst as provider_capitalized %}
{% blocktranslate %}Login with {{ provider_capitalized }}{% endblocktranslate %}
{% endwith %}
</a>
{% endif %}
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The preferred provider button is displayed if settings.preferred is True, but there's no check for whether the provider actually has valid credentials configured. If an admin sets a provider as preferred but hasn't configured valid client_id and secret, the button will be displayed but clicking it will likely fail. Consider checking both settings.preferred and that credentials are configured (settings.client_id and settings.secret are non-empty) before displaying the preferred button.

Copilot uses AI. Check for mistakes.
Comment on lines +1 to +12
docker compose up -d --build

<!-- create super user -->
docker exec -ti eventyay-next-web python manage.py createsuperuser


Web interface: http://localhost:8000
Admin panel: http://localhost:8000/admin


<!-- stop the application -->
docker compose down No newline at end of file
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The configuration files (setup.md, eventyay.cfg) and docker-compose.yml changes appear to be development/deployment setup files that aren't related to the social auth feature described in the PR. These files configure Docker containers with volume mounts for local development (including '../app/eventyay' mounts) and contain sensitive placeholder values like 'CHANGEME' for secrets. These changes seem unrelated to the PR's stated purpose of adding preferred login provider selection.

Copilot uses AI. Check for mistakes.
- ./data/static:/home/app/web/eventyay/static.dist
- ./data/data:/data
- ./eventyay.cfg:/etc/eventyay.cfg:ro
- ../app/eventyay:/usr/src/app/eventyay
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

Mounting the local source code directory as a volume ('../app/eventyay:/usr/src/app/eventyay') in production deployment configuration is risky. This pattern is typically used for development hot-reloading but can lead to inconsistent deployments, version mismatches, and security issues in production. The code should be baked into the Docker image at build time rather than mounted at runtime for production deployments.

Copilot uses AI. Check for mistakes.
Comment on lines +171 to +172

# Update preferred status
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The logic for updating the preferred status is inside the provider loop, which means it runs for every provider even though the preferred provider selection is independent of individual provider operations. Consider moving this logic outside the loop to make the code clearer and avoid unnecessary repeated checks. You could collect the preferred provider once and then set all providers' preferred status in a single pass after the main loop.

Suggested change
# Update preferred status
# Update preferred status for all providers in a single pass
for provider in LoginProviders.model_fields.keys():

Copilot uses AI. Check for mistakes.
Copy link
Member

@ArnavBallinCode ArnavBallinCode left a comment

Choose a reason for hiding this comment

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

please wait for this feature to be implemented first , this pr doesnt make sense without it, you can work on this once pr #1523 is merged

Copy link
Member

@mariobehling mariobehling left a comment

Choose a reason for hiding this comment

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

Please check AI comments and add screenshots.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

3 participants