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

Fix NbGraderAPI.timezone handling #871

Merged
merged 1 commit into from
Aug 24, 2017
Merged

Conversation

thotypous
Copy link
Contributor

Before this patch

Currently, the user may specify, e.g.

c = get_config()
c.NbGraderAPI.timezone = 'America/Sao_Paulo' 

Then it would correctly convert all timestamps to 'America/Sao_Paulo'. However, it would also be displayed in the formgrader when the user sets the assignment's due date:

Timezone displayed in the formgrader before this patch

When the user sends this form, the timezone string is concatenated to the time and passed to utils.parse_utc, which cannot handle this string format.

After this patch

This patch converts NbGraderAPI.timezone to a format readable by parse_utc before displaying it to the user in the formgrader. This way the user does not need to change the default value presented in the Timezone field: it will just use the configured timezone out-of-the-box.

Timezone displayed in the formgrader after this patch

Note

The dateutil.tz.gettz function does not support reading a timezone in the numeric format (e.g. -0300 or UTC-3), therefore it is not possible to use the numeric format to configure NbGraderAPI.timezone.

In other words, this patch is the only way to get the timezones working correctly in the Web UI. It also takes into account that the timezone offset might change during the course due to daylight savings.

Copy link
Member

@jhamrick jhamrick left a comment

Choose a reason for hiding this comment

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

This is great, thanks for the contribution! 🎉 I had a lot of frustration with the timezones when implementing the functionality originally, so I'm glad to see this improving it 😄

Two things:

  1. The tests are failing because they still expect string timezones, like 'UTC'. Can you fix them to expect the numerical offset timezones that you've implemented here? Even better would be to add some additional tests too, to test for timezones other than UTC (though I realize this might be complicated to figure out how to do).

  2. Could you change the text in the formgrader from "Timezone (optional)" to "Timezone as UTC offset (optional)" so that people aren't confused about what format to use?

P.S. I should mention I am going to be away for 6 weeks beginning August 28th, and won't be able to review stuff while I'm away. If you continue working on this I'm happy to review it when I get back, or there are other nbgrader maintainers who may be able to take a look at it.

@jhamrick jhamrick added this to the 0.6.0 milestone Aug 21, 2017
Converts NbGraderAPI.timezone to a format readable by parse_utc before
displaying it to the user in the formgrader.
@jhamrick
Copy link
Member

Just noticed you updated the PR---for future reference (and for other projects), I don't get an email about it, so it is helpful to leave a comment or something so the maintainers know to review the PR again 😄

In any case, looks good, thanks!

@jhamrick jhamrick merged commit 274d15e into jupyter:master Aug 24, 2017
@thotypous
Copy link
Contributor Author

Thanks @jhamrick ! Sorry for not commenting here, I was still looking at writing the tests you recommended, but then I got a little overwhelmed with other stuff. If I get the chance to work on it I will open another PR. Thanks again for the review and for the merge :)

rkdarst added a commit to AaltoSciComp/jupyterhub-aalto that referenced this pull request Aug 4, 2019
- See: jupyter/nbgrader#871
- This is translated to a numeric timezone offset for each assignment,
  which doesn't seem ideal to me but at least reduces user work.
rkdarst added a commit to AaltoSciComp/jupyterhub-aalto that referenced this pull request Aug 4, 2019
- See: jupyter/nbgrader#871
- This is translated to a numeric timezone offset for each assignment,
  which doesn't seem ideal to me but at least reduces user work.
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.

2 participants