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

Added test for DateTimeField validation when server has timezone with… #4987

Merged
merged 4 commits into from Apr 27, 2017

Conversation

halfstrik
Copy link
Contributor

@halfstrik halfstrik commented Mar 16, 2017

… DST and input is a native time in a DST shift interval.

Added pytz to requirements-testing.txt to reproduce the case.

Note: Before submitting this pull request, please review our contributing guidelines.

Description

PR reproduces an issue with DateTimeField in case when server TIME_ZONE has Day Light Saving Time (DST) shift and field given native datetime in DST shift.
In my case server has pytz installed, I don't know how to reproduce it without it.
Corresponding issue #4986

… DST and input is a native time in a DST shift interval.

Added pytz to requirements-testing.txt to reproduce the case.
return timezone.make_aware(value, field_timezone)
try:
return timezone.make_aware(value, field_timezone)
except Exception:
Copy link
Collaborator

@xordoquy xordoquy Mar 20, 2017

Choose a reason for hiding this comment

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

Could we have a more specific exception here ?

Copy link
Contributor Author

@halfstrik halfstrik Mar 20, 2017

Choose a reason for hiding this comment

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

Here pytz raises AmbiguousTimeError or NonExistentTimeError both have parent pytz.exceptions.InvalidTimeError(Exception).
django-rest-framework doesn't have pytz in requirements (except -testing added in this PR) and I think it's not worth to depend on it here only to catch specific exceptions. You can have no pytz but still can use drf.

Copy link
Collaborator

@xordoquy xordoquy Mar 21, 2017

Choose a reason for hiding this comment

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

What sort of exception would there be without pytz ?
If the issue is just the non requirement of pytz, we have a compat module that will deal with that.

Copy link
Contributor Author

@halfstrik halfstrik Mar 21, 2017

Choose a reason for hiding this comment

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

Without pytz I can't get timezone pytz.timezone('America/New_York') object, so if pytz is not installed I got: AttributeError: 'NoneType' object has no attribute 'timezone'.
Can you please point me on that compact module, I'll give it a try.

Copy link
Collaborator

@xordoquy xordoquy Mar 21, 2017

Choose a reason for hiding this comment

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

compat module is in the rest_framework directory.
It should either import the pytz exceptions if available or mock them to avoid breaking code.
And I don't think you already have the AttributeError when pytz is not installed do you ?

@@ -1085,6 +1085,7 @@ class DateTimeField(Field):
default_error_messages = {
'invalid': _('Datetime has wrong format. Use one of these formats instead: {format}.'),
'date': _('Expected a datetime but got a date.'),
'make_aware': _('Datetime can not be represented in timezone "{timezone}".')
Copy link
Collaborator

@xordoquy xordoquy Mar 20, 2017

Choose a reason for hiding this comment

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

What about 'Invalid datetime for the timezone "{timezone}".' ?

@halfstrik
Copy link
Contributor Author

halfstrik commented Mar 22, 2017

Hope this time I get it right, compat module is neat

@tomchristie tomchristie added this to the 3.6.3 Release milestone Mar 24, 2017
@tomchristie
Copy link
Member

tomchristie commented Apr 27, 2017

Nice!

@tomchristie tomchristie merged commit 5ba2368 into encode:master Apr 27, 2017
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants