Skip to content

fix for render_datetime() #6252

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

Merged
merged 4 commits into from
Aug 30, 2021
Merged

Conversation

TomeCirun
Copy link
Contributor

Fixes #5946

Proposed fixes:

passing rebase=False to format_datetime() will make render_datetime to respect ckan.display_timezone configuration

Features:

  • includes tests covering changes
  • includes updated documentation
  • includes user-visible changes
  • includes API changes
  • includes bugfix for possible backport

Please [X] all the boxes above that apply

@wardi wardi self-assigned this Jul 15, 2021
@TomeCirun
Copy link
Contributor Author

@wardi what's your opinion over this?

I have already put my research here

Thanks

@wardi
Copy link
Contributor

wardi commented Jul 22, 2021

@smotornyuk you updated this code in #5376 WDYT?

At a minimum it doesn't make sense to pass this to only one of the format_date/format_datetime calls.

@TomeCirun
Copy link
Contributor Author

TomeCirun commented Jul 22, 2021

@wardi Thanks for you reply.

In the template we are passing with_hours=True

{{ h.render_datetime(datetime_obj, with_hours=True) }}

as only this code is going to execute
elif with_hours:

return format_datetime(datetime_, format or fmt_str)

thats why i pass rebase=False only in that place

If you have any suggestion, I m glad to fix it if needed

Thanks.

@smotornyuk
Copy link
Member

It doesn't sound quite right for me.. you are trying to fix consequences, not the source of the problem. Can you just set babel's default locale to the one, specified by ckan.display_timezone? Using BABEL_DEFAULT_TIMEZONE option of flask-babel

And yes, I think that all routes of localised_nice_date should return localized representation, not only with_hours=True.

@TomeCirun
Copy link
Contributor Author

hey @smotornyuk thanks for your reply

I just set the babel's default timezone to the one specified by ckan.display_timezone... working good when providing a timezone(e.g Europe/Zurich)
image

.. but when i set the value: server

image

i got this error pytz.exceptions.UnknownTimeZoneError: 'server'

Any suggestion?
Thanks

PS The source of the problem is because we didnt provide BABEL_DEFAULT_TIMEZONE (by default is set to UTC when no timezone provided)...so when we are calling format_datetime() by default rebasing happens and babel's get_timezone() returns UTC

So .. to prevent this...we can:
-pass rebase=False to all format_datetime()
or
-provide BABEL_DEFAULT_TIMEZONE value to the one specified by ckan.display_timezone (just keep on mind that when ckan.display_timezone is set to server we get this error: pytz.exceptions.UnknownTimeZoneError: 'server' )

What's your opinion over this?

@smotornyuk
Copy link
Member

There is a helper, get_display_timezone, that solves our issue with server value. in this special case it returns tzlocal.get_localzone() object. As any tzinfo object, this one has zone property that can be passed to the Babel's config.

So you can "unwrap" value of ckan.display_timezone here , right before babel is initialized

@TomeCirun
Copy link
Contributor Author

@smotornyuk Thanks for your help.

@TomeCirun
Copy link
Contributor Author

hey @wardi can you check my last PR? I did it as @smotornyuk suggest.

Thanks

@TomeCirun
Copy link
Contributor Author

Hey @wardi it's me again 😊 can you please check my PR ?

Thanks

@wardi
Copy link
Contributor

wardi commented Aug 30, 2021

@smotornyuk I'll have to rely on you again, does this seem like a sensible flask config change?

@smotornyuk
Copy link
Member

Yep. I would say that it's the only right way to configure flask-babel timezone in CKAN)

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

Successfully merging this pull request may close these issues.

render_datetime helper does not respect ckan.display_timezone configuration
4 participants