Fix: Handle Users with None Email in Admin Interface #1240
+15
−5
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates the admin user edit form to properly detect and handle None email values by converting them to empty strings, making the field optional when appropriate, and refining validation; it also standardizes template displays to show an em dash placeholder for missing emails and full names. Class diagram for updated UserEditForm email handlingclassDiagram
class UserEditForm {
__init__(*args, **kwargs)
clean_email()
fields
initial
}
UserEditForm : __init__ detects None email and sets initial['email'] = ''
UserEditForm : email field required only if email exists
UserEditForm : clean_email() converts empty string to None
UserEditForm : clean_email() checks for duplicates only if email is provided
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `app/eventyay/control/forms/users.py:79-81` </location>
<code_context>
def clean_email(self):
- email = self.cleaned_data['email']
+ email = self.cleaned_data.get('email', '').strip()
+ # Convert empty string to None for users without email
+ if not email:
+ return None
+ # Check for duplicate emails only if email is provided
</code_context>
<issue_to_address>
**suggestion:** Stripping whitespace from email may cause issues for legitimate emails with leading/trailing spaces.
Consider validating and rejecting emails with leading or trailing spaces, rather than silently removing them, to avoid masking user input errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+79
to
+81
| email = self.cleaned_data.get('email', '').strip() | ||
| # Convert empty string to None for users without email | ||
| if not email: |
Contributor
There was a problem hiding this comment.
suggestion: Stripping whitespace from email may cause issues for legitimate emails with leading/trailing spaces.
Consider validating and rejecting emails with leading or trailing spaces, rather than silently removing them, to avoid masking user input errors.
Member
There was a problem hiding this comment.
This seems reasonable. Please also show an error message for invalid emails.
Contributor
There was a problem hiding this comment.
Thanks for your feedback!
To show an error message for invalid emails, you can enhance the clean_email method in your form to use Django's built-in email validation. This way, if a user enters an email with leading/trailing spaces or an invalid format, the form will display a clear error message.
Here's how you can update your clean_email method:
from django.core.validators import validate_email
from django.core.exceptions import ValidationError
def clean_email(self):
email = self.cleaned_data.get('email', '')
if email:
# Check for leading/trailing spaces
if email != email.strip():
raise forms.ValidationError(_('Email address must not have leading or trailing spaces.'), code='invalid')
try:
validate_email(email)
except ValidationError:
raise forms.ValidationError(_('Enter a valid email address.'), code='invalid')
else:
return None
# Check for duplicate emails only if email is provided
if User.objects.filter(Q(email__iexact=email) & ~Q(pk=self.instance.pk)).exists():
raise forms.ValidationError(
self.error_messages['duplicate_identifier'],
code='duplicate_identifier',
)
return emailThis will:
- Reject emails with leading/trailing spaces and show a specific error
- Use Django's email validator for format checking
- Continue to check for duplicates as before
Let me know if you need a full patch or further help integrating this!
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR fixes issues with displaying and editing users who have None as their email address in the Django admin interface. The fix prevents the literal string "None" from appearing and ensures proper form validation for users without email addresses.
Key Changes:
- Enhanced form logic to detect and handle
Noneemail values by converting them to empty strings in the form display - Made email field optional for existing users without email addresses while keeping it required for new users
- Updated templates to display "—" (em dash) placeholder instead of "None" or empty strings
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| app/eventyay/control/forms/users.py | Added logic in __init__ to handle None emails and updated clean_email() to convert empty strings back to None while safely checking for duplicates |
| app/eventyay/control/templates/pretixcontrol/admin/users/form.html | Added default:"—" filter to user email display in the page header |
| app/eventyay/control/templates/pretixcontrol/admin/users/index.html | Added default:"—" filters to both email and fullname columns for consistent placeholder display |
Comments suppressed due to low confidence (1)
app/eventyay/control/forms/users.py:89
- The new logic for handling users with
Noneemail addresses lacks test coverage. Consider adding tests for the following scenarios:
- Editing an existing user with
email = None(native auth) - should show empty field and allow saving without email - Editing an existing user with
email = None(non-native auth) - should show disabled empty field - Converting an email to
Noneby clearing the field for a user without email - Attempting to save duplicate emails when one user has
Noneemail
This would help ensure the fix works correctly and prevents regressions.
def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
# Handle None email values - show empty field instead of "None" text
if self.instance and self.instance.pk and self.instance.email is None:
self.initial['email'] = ''
# Make email not required for existing users without email
self.fields['email'].required = False
else:
self.fields['email'].required = True
self.fields['last_login'].disabled = True
if self.instance and self.instance.auth_backend != 'native':
del self.fields['new_pw']
del self.fields['new_pw_repeat']
self.fields['email'].disabled = True
def clean_email(self):
email = self.cleaned_data.get('email', '').strip()
# Convert empty string to None for users without email
if not email:
return None
# Check for duplicate emails only if email is provided
if User.objects.filter(Q(email__iexact=email) & ~Q(pk=self.instance.pk)).exists():
raise forms.ValidationError(
self.error_messages['duplicate_identifier'],
code='duplicate_identifier',
)
return email
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem (Root Cause)
When users exist in the system with
Noneas their email address (which can happen with certain authentication backends or data migrations), the admin interface displayed several issues:This created a poor user experience and could lead to confusion when managing users in the admin panel.
Solution
This PR implements a comprehensive fix across both the form logic and template rendering:
1. Form Handling (
app/eventyay/control/forms/users.py)UserEditForm.__init__()to detect when an existing user hasNoneemailNoneemail values to empty strings in the form initial data, so the form shows an empty field instead of "None"clean_email()to:None.get()with default to safely handle missing email values2. Template Display (
app/eventyay/control/templates/pretixcontrol/admin/users/)default:"—"instead ofdefault_if_none:""for consistent placeholder displayChanges Made
app/eventyay/control/forms/users.py: Enhanced form initialization and email validationapp/eventyay/control/templates/pretixcontrol/admin/users/form.html: Added default filter for email displayapp/eventyay/control/templates/pretixcontrol/admin/users/index.html: Added default filters for email and fullname displayTesting
Noneemail now display "—" in the admin interfaceRelated Issue
Fixes #1212
Before:
After:
Summary by Sourcery
Improve the Django admin user interface and form logic to properly handle users with null email addresses by displaying placeholders and adjusting form requirements and validation
Bug Fixes:
Enhancements: