Fix: Prevent past date selection in event creation (#595) #599
Conversation
This file contains 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
Reviewer's Guide by SourceryThis pull request fixes an issue where users could select past dates when creating or updating events, due to timezone discrepancies. It ensures that both the frontend and backend validate the event start and end dates against the user's timezone, preventing the selection of past dates. Sequence diagram for event creation with timezone validationsequenceDiagram
actor User
participant Browser
participant Server
User->>Browser: Enters event details, including date_from
Browser->>Server: Submits form data (date_from in user's timezone)
Server->>Server: Converts date_from to user's timezone
alt date_from is in the past
Server->>Server: Raises ValidationError
Server-->>Browser: Returns error message
Browser-->>User: Displays error message
else date_from is in the future
Server->>Server: Saves event
Server-->>Browser: Returns success
Browser-->>User: Displays success
end
Updated class diagram for EventWizardBasicsForm and EventUpdateFormclassDiagram
class EventWizardBasicsForm {
+__init__(*args, **kwargs)
+clean_domain()
+convert_to_user_timezone(dt)
+clean_date_from()
+clean_date_to()
+save(commit=True)
}
class EventUpdateForm {
+__init__(*args, **kwargs)
+convert_to_user_timezone(dt)
+clean_date_from()
+clean_date_to()
+save(commit=True)
}
EventWizardBasicsForm --|> EventForm : inherits from
EventUpdateForm --|> EventWizardBasicsForm : inherits from
note for EventWizardBasicsForm.convert_to_user_timezone "Converts datetime to user's timezone"
note for EventWizardBasicsForm.clean_date_from "Validates date_from is in the future"
note for EventWizardBasicsForm.clean_date_to "Validates date_to is in the future and after date_from"
note for EventUpdateForm.convert_to_user_timezone "Converts datetime to user's timezone"
note for EventUpdateForm.clean_date_from "Validates date_from is in the future"
note for EventUpdateForm.clean_date_to "Validates date_to is in the future and after date_from"
File-Level Changes
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}
There was a problem hiding this comment.
Hey @Gagan-Ram - I've reviewed your changes - here's some feedback:
Overall Comments:
- The
convert_to_user_timezonemethod is duplicated in both forms, consider moving it to a utility module. - Consider caching the user's timezone to avoid repeated calls to
get_current_timezone_name().
Here's what I looked at during the review
- 🟡 General issues: 2 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+249
to
+258
| @staticmethod | ||
| def convert_to_user_timezone(dt): | ||
| """ | ||
| Converts a given datetime to the user's timezone. | ||
| If the datetime is None, returns None. | ||
| """ | ||
| if dt is None: | ||
| return None | ||
| user_timezone_str = get_current_timezone_name() | ||
| user_tz = timezone(user_timezone_str) |
There was a problem hiding this comment.
suggestion: Reduce duplication in timezone conversion methods.
The methods convert_to_user_timezone (along with clean_date_from and clean_date_to) are defined twice in this file. Consider abstracting them into a shared mixin or helper module to avoid duplicate code and ensure consistency.
Comment on lines
165
to
+171
| if "timezone" not in self.initial: | ||
| self.initial["timezone"] = get_current_timezone_name() | ||
|
|
||
| selected_timezone = self.initial.get("timezone") or get_current_timezone().zone | ||
| tz = timezone(selected_timezone) | ||
| min_date = now().astimezone(tz).date() | ||
| self.fields["date_from"].widget = SplitDateTimePickerWidget(min_date=min_date) |
There was a problem hiding this comment.
suggestion: Consider consolidating repeated initialization logic.
The block setting the 'timezone' in initial data and assigning the SplitDateTimePickerWidget appears in two places. Refactoring this logic into a shared method or base class could improve maintainability and reduce potential inconsistencies.
| user_timezone_str = get_current_timezone_name() | ||
| user_tz = timezone(user_timezone_str) | ||
| naive_dt = dt.replace(tzinfo=None) | ||
| return user_tz.localize(naive_dt) |
There was a problem hiding this comment.
issue (complexity): Consider creating a mixin or utility module to encapsulate the duplicated timezone conversion and date validation logic to reduce redundancy and improve maintainability
Consider extracting the duplicated timezone conversion and date cleaning logic into a shared mixin or utility module. For example, you could create a `UserTimezoneMixin`:
```python
from django.utils.timezone import now, get_current_timezone_name
from pytz import timezone
from django.core.exceptions import ValidationError
from django.utils.translation import gettext as _
class UserTimezoneMixin:
@staticmethod
def convert_to_user_timezone(dt):
"""Converts a given datetime to the user's timezone."""
if dt is None:
return None
user_tz = timezone(get_current_timezone_name())
naive_dt = dt.replace(tzinfo=None)
return user_tz.localize(naive_dt)
def clean_date_from_with_timezone(self, date_from):
"""Ensures event start date is not in the past."""
date_from_tz = self.convert_to_user_timezone(date_from)
user_now = now().astimezone(timezone(get_current_timezone_name()))
if date_from and date_from_tz < user_now:
raise ValidationError(_("The event start date and time cannot be in the past."))
return date_from_tz
def clean_date_to_with_timezone(self, date_from, date_to):
"""Ensures event end date is valid and after start date."""
date_from_tz = self.convert_to_user_timezone(date_from)
date_to_tz = self.convert_to_user_timezone(date_to)
user_today = now().astimezone(timezone(get_current_timezone_name())).date()
if date_to_tz and date_to_tz.date() < user_today:
raise ValidationError(_("The event end date and time cannot be in the past."))
if date_from_tz and date_to_tz and date_to_tz <= date_from_tz:
raise ValidationError(_("The event end date and time must be after the start date and time."))
return date_to_tzThen update your forms to inherit from this mixin. For example:
class MyEventForm(UserTimezoneMixin, forms.Form):
# form fields ...
def clean_date_from(self):
date_from = self.cleaned_data.get("date_from")
return self.clean_date_from_with_timezone(date_from)
def clean_date_to(self):
date_from = self.cleaned_data.get("date_from")
date_to = self.cleaned_data.get("date_to")
return self.clean_date_to_with_timezone(date_from, date_to)This refactoring removes redundancy and centralizes the timezone and date validation logic.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR ensure that the date_from field is correctly restricted to prevent past date selection based on the user's timezone.
Fixes: #595
Changes Made
How to Test
Clarification of code
Question:
What was the need for converting the date_from value to the user's specific timezone? Why can't we use a simple now()?
Answer:
The conversion is necessary because now() defaults to UTC. If we were to use now() directly, users in different timezones (e.g., Asia/Kolkata, which is UTC+5:30) might see incorrect behavior.
For example, a user in India could still select a past date until 5:30 AM UTC, which is unintended. This happens because, without timezone conversion, the system might consider a new day in UTC while it's still the previous day for the user.
By explicitly converting now() to the user’s timezone before setting min_date, we ensure:
This approach ensures that users always see the expected minimum date, avoiding cases where they could unintentionally select past dates due to timezone mismatches.
Summary by Sourcery
Implement timezone-aware date validation for event creation to prevent users from selecting past dates
Bug Fixes:
Enhancements: