-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Bug/56771 meeting timestamp in edit form not the same as in details #16567
base: dev
Are you sure you want to change the base?
Bug/56771 meeting timestamp in edit form not the same as in details #16567
Conversation
The system did this anyway, but now it becomes explicit
With the server now always running in UTC time, using the local time makes no sense any more
c234011
to
4c61ce1
Compare
Rubocop is complaining about the complexity of a method touched within the PR. The PR only changes the call signature of a method called from within the complained about method. Therefore, I deem it ok to not improve that method. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really nice refactoring 👏 It simplifies the code dealing with time zones and removes a lot of useless code which is always a good sign.
I only have some minor improvement suggestions.
Feel free to merge whenever you see fit.
For that long method rubocop is complaining about, if it's ok to keep it, then it could be # rubocop:disable Metrics/AbcSize
.
@@ -49,7 +49,7 @@ def name(*_args); I18n.t(:label_user_anonymous) end | |||
|
|||
def mail; nil end | |||
|
|||
def time_zone; nil end | |||
def time_zone; ActiveSupport::TimeZone[Setting.user_default_timezone.presence || "Etc/UTC"] end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not rely on the default implementation of User#time_zone
? Implementation is
class User
def time_zone
@time_zone ||= ActiveSupport::TimeZone[pref.time_zone] || ActiveSupport::TimeZone["Etc/UTC"]
end
end
As a pref
record does not exist for AnonymousUser
, it would be automatically built without touching the db and its time_zone
will return the right value, so it should work as expected.
Same applies for DeletedUser
and SystemUser
.
container_class: (defined? input_size) ? "-#{input_size}" : "-wide" | ||
) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unneeded new line
local = if zone | ||
time.in_time_zone(zone) | ||
else | ||
(time.utc? ? time.to_time.localtime : time) | ||
end | ||
local = time.in_time_zone(zone) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice 👏
unless User.current.pref.time_zone? | ||
link = I18n.t(:notice_timezone_missing, zone: formatted_time_zone_offset) | ||
text += " #{view_context.link_to(link, { controller: '/my', action: :settings, anchor: 'pref_time_zone' }, | ||
class: 'link_to_profile')}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried it. When there is no time zone set for the user, the message is displayed inviting the user to set a time zone, but the notification disappears after 5 seconds (or so). This is extremely short to read and take action. I suggest the success toast should not autoclose in this case. Probably worth a new bug work package for it.
def set_time_zone(&) | ||
zone = User.current.time_zone | ||
if zone.nil? | ||
localzone = Time.current.utc_offset | ||
localzone -= 3600 if Time.current.dst? | ||
zone = ::ActiveSupport::TimeZone[localzone] | ||
end | ||
|
||
Time.use_zone(zone, &) | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Goodbye buggy code (a dst is not always 1h).
@@ -133,18 +129,25 @@ def format_time_as_date(time, format = nil) | |||
end | |||
end | |||
|
|||
def format_time(time, include_date = true) | |||
def format_time(time, include_date: true, format: Setting.time_format) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's hard to know that this method uses user time zone without reading its code. We could rename it, but I propose adding some docs for it.
def format_time(time, include_date: true, format: Setting.time_format) | |
# Formats the given time as a time string according to the user's time zone | |
# and optional specified format. | |
# | |
# @param time [Time, String] The time to format. Can be a Time object or a | |
# String. | |
# @param include_date [Boolean] Whether to include the date in the formatted | |
# output. Defaults to true. | |
# @param format [String] The strftime format to use for the time. Defaults | |
# to the format in `Setting.time_format`. | |
# @return [String, nil] The formatted time string, or nil if the time is not | |
# provided. | |
def format_time(time, include_date: true, format: Setting.time_format) |
@@ -116,15 +116,11 @@ def link_regex | |||
# Format the time to a date in the user time zone if one is set. | |||
# If none is set and the time is in utc time zone (meaning it came from active record), format the date in the system timezone | |||
# otherwise just use the date in the time zone attached to the time. | |||
def format_time_as_date(time, format = nil) | |||
def format_time_as_date(time, format: nil) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation is not up-to-date anymore
Here is a proposal:
# Formats the given time as a date string according to the user's time zone and
# optional specified format.
#
# @param time [Time, String] The time to format. Can be a Time object or a String.
# @param format [String, nil] The strftime format to use for the date. If nil, the default
# date format from `Setting.date_format` is used.
# @return [String, nil] The formatted date string, or nil if the time is not provided.
def format_time_as_date(time, format: nil)
after do | ||
Time.zone = nil | ||
end | ||
describe "#format_time_as_date" do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are no tests taking time parameter as string. Not sure if it's used that way in our code and how it deals with time strings without timezone information.
Not sure it's worth it though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same for #format_time
.
Ticket
https://community.openproject.org/wp/56771
What are you trying to accomplish?
The PR ensures that times and time zones displayed in the meetings module are always in the time zone of the current user. The bug originally reported in the work package could not be reproduced but before, different code paths have been used for rendering the time zone and the calculation on times was in part achieved by setting the request's time to be in the current user's time.
What approach did you choose and why?
User.current.time_zone
will now always return a value. It will never benil
. Either the time zone selected by the user or the time zone set as the default (Setting.user_default_timezone
) is returned. This already used to be the case for logged in users. For those, in case no default is set, UTC is now returned. Internally, it was already handled as such as both server and database are running on UTC. The internal users (e.g.AnonymousUser
) follow the same pattern.format_time
andformat_date_as_time
have been simplified. There used to be code in there that was working with the localtime. Since a few rails versions back, the server is always running in UTC so there is no need for this code. The only times now necessary are UTC and the user's local time. The server's local time should have no effect. Those methods get their signature updated to use named parameters.formatted_time_zone_offset
is added toI18n
which will give the currently active time offset to the user's time zone compared to UTC. This is now employed throughout the meeting module.Setting.user_default_timezone
select and set the value to UTC as a default but this is not done in the scope of this PR.Merge checklist