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

Bug/56771 meeting timestamp in edit form not the same as in details #16567

Open
wants to merge 13 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/components/settings/time_zone_setting_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

module Settings
##
# A text field to enter numeric values.
# A select field to select a time zone from.
class TimeZoneSettingComponent < ::ApplicationComponent
options :form, :title
options container_class: "-wide"
Expand Down
2 changes: 1 addition & 1 deletion app/models/anonymous_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Member

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.


def rss_key; nil end

Expand Down
2 changes: 1 addition & 1 deletion app/models/deleted_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ def builtin? = true
def admin = false
def name(*_args) = I18n.t("user.deleted")
def mail = nil
def time_zone = nil
def time_zone; ActiveSupport::TimeZone[Setting.user_default_timezone.presence || "Etc/UTC"] end
def rss_key = nil
def destroy = false
end
2 changes: 1 addition & 1 deletion app/models/exports/concerns/csv.rb
Original file line number Diff line number Diff line change
Expand Up @@ -84,7 +84,7 @@ def format_csv(record, attribute)
def csv_export_filename
sane_filename(
"#{Setting.app_title} #{title} \
#{format_time_as_date(Time.zone.now, '%Y-%m-%d')}.csv"
#{format_time_as_date(Time.zone.now, format: '%Y-%m-%d')}.csv"
)
end
end
Expand Down
2 changes: 1 addition & 1 deletion app/models/system_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def name(*_args); "System" end

def mail; nil end

def time_zone; nil end
def time_zone; ActiveSupport::TimeZone[Setting.user_default_timezone.presence || "Etc/UTC"] end

def rss_key; nil end

Expand Down
12 changes: 9 additions & 3 deletions app/models/user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -398,15 +398,21 @@ def log_failed_login
end

def log_successful_login
update_attribute(:last_login_on, Time.now)
update_attribute(:last_login_on, Time.current)
end

def pref
preference || build_preference
end

def time_zone
@time_zone ||= (pref.time_zone.blank? ? nil : ActiveSupport::TimeZone[pref.time_zone])
@time_zone ||= ActiveSupport::TimeZone[pref.time_zone] || ActiveSupport::TimeZone["Etc/UTC"]
ulferts marked this conversation as resolved.
Show resolved Hide resolved
end

def reload(*)
@time_zone = nil

super
end

def wants_comments_in_reverse_order?
Expand Down Expand Up @@ -688,6 +694,6 @@ def log_failed_login_timestamp
end

def self.default_admin_account_changed?
!User.active.find_by_login("admin").try(:current_password).try(:matches_plaintext?, "admin") # rubocop:disable Rails/DynamicFindBy
!User.active.find_by_login("admin").try(:current_password).try(:matches_plaintext?, "admin")
end
end
6 changes: 5 additions & 1 deletion app/models/user_preference.rb
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,11 @@ def high_contrast_theme?
end

def time_zone
super.presence || Setting.user_default_timezone.presence
super.presence || Setting.user_default_timezone.presence || "Etc/UTC"
end

def time_zone?
settings["time_zone"].present?
end

def daily_reminders
Expand Down
2 changes: 1 addition & 1 deletion app/models/work_package/pdf_export/page.rb
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def footer_page_nr
end

def footer_date
format_time(Time.zone.now, true)
format_time(Time.zone.now)
end

def total_page_nr_text
Expand Down
1 change: 0 additions & 1 deletion app/views/admin/settings/users_settings/show.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,6 @@ See COPYRIGHT and LICENSE files for more details.

<%= render Settings::TimeZoneSettingComponent.new(
"user_default_timezone",
container_class: "-slim",
title: I18n.t("tooltip_user_default_timezone")
)
%>
Expand Down
2 changes: 2 additions & 0 deletions app/views/users/_preferences.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,10 @@ See COPYRIGHT and LICENSE files for more details.
<%= render Settings::TimeZoneSettingComponent.new(
"time_zone",
form: pref_fields,
include_blank: false,
container_class: (defined? input_size) ? "-#{input_size}" : "-wide"
)

Copy link
Member

Choose a reason for hiding this comment

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

Unneeded new line

%>
<div class="form--field">
<%= pref_fields.select :theme, theme_options_for_select, container_class: '-middle' %>
Expand Down
29 changes: 16 additions & 13 deletions lib_static/redmine/i18n.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Copy link
Member

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)

return nil unless time

zone = User.current.time_zone
local_date = (if zone
time.in_time_zone(zone)
else
time.utc? ? time.localtime : time
end).to_date
local_date = time.in_time_zone(zone).to_date

if format
local_date.strftime(format)
Expand All @@ -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)
Copy link
Member

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.

Suggested change
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)

return nil unless time

time = time.to_time if time.is_a?(String)
zone = User.current.time_zone
local = if zone
time.in_time_zone(zone)
else
(time.utc? ? time.to_time.localtime : time)
end
local = time.in_time_zone(zone)
Comment on lines -141 to +137
Copy link
Member

Choose a reason for hiding this comment

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

Nice 👏


(include_date ? "#{format_date(local)} " : "") +
(Setting.time_format.blank? ? ::I18n.l(local, format: :time) : local.strftime(Setting.time_format))
(format.blank? ? ::I18n.l(local, format: :time) : local.strftime(format))
end

# Returns the offset to UTC (with utc prepended) currently active
# in the current users time zone. DST is factored in so the offset can
# shift over the course of the year
def formatted_time_zone_offset
# Doing User.current.time_zone and format that will not take heed of DST as it has no notion
# of a current time.
# https://github.com/rails/rails/issues/7297
"UTC#{User.current.time_zone.now.formatted_offset}"
end

def day_name(day)
Expand Down
2 changes: 1 addition & 1 deletion modules/bim/lib/open_project/bim/bcf_xml/exporter.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ def bcf_filename

sane_filename(
"#{Setting.app_title} #{I18n.t(:label_work_package_plural)} \
#{format_time_as_date(Time.now, '%Y-%m-%d')}.bcf"
#{format_time_as_date(Time.current, format: '%Y-%m-%d')}.bcf"
)
end

Expand Down
2 changes: 1 addition & 1 deletion modules/boards/app/components/boards/row_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def name
end

def created_at
safe_join([helpers.format_date(model.created_at), helpers.format_time(model.created_at, false)], " ")
safe_join([helpers.format_date(model.created_at), helpers.format_time(model.created_at, include_date: false)], " ")
end

def type
Expand Down
2 changes: 1 addition & 1 deletion modules/meeting/app/components/meetings/row_component.rb
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ def type
end

def start_time
safe_join([helpers.format_date(model.start_time), helpers.format_time(model.start_time, false)], " ")
safe_join([helpers.format_date(model.start_time), helpers.format_time(model.start_time, include_date: false)], " ")
end

def duration
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,13 +30,13 @@
flex_layout(align_items: :center) do |time|
time.with_column do
render(Primer::Beta::Text.new) do
"#{format_time(@meeting.start_time, false)} - #{format_time(@meeting.end_time, false)}"
"#{format_time(@meeting.start_time, include_date: false)} - #{format_time(@meeting.end_time, include_date:false)}"
end
end

time.with_column(ml: 2) do
render(Primer::Beta::Text.new(color: :subtle, font_size: :small)) do
(User.current.time_zone || Time.zone).to_s[/\((.*?)\)/m, 1]
formatted_time_zone_offset
end
end
end
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,11 +45,11 @@ def render?
private

def start_date_initial_value
@meeting.start_time&.strftime("%Y-%m-%d")
format_time_as_date(@meeting.start_time, format: "%Y-%m-%d")
end

def start_time_initial_value
@meeting.start_time&.strftime("%H:%M")
format_time(@meeting.start_time, include_date: false, format: "%H:%M")
end
end
end
16 changes: 2 additions & 14 deletions modules/meeting/app/controllers/meetings_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@
#++

class MeetingsController < ApplicationController
around_action :set_time_zone
before_action :load_and_authorize_in_optional_project, only: %i[index new show create history]
before_action :verify_activities_module_activated, only: %i[history]
before_action :determine_date_range, only: %i[history]
Expand Down Expand Up @@ -97,8 +96,8 @@ def create # rubocop:disable Metrics/AbcSize

if call.success?
text = I18n.t(:notice_successful_create)
if User.current.time_zone.nil?
link = I18n.t(:notice_timezone_missing, zone: Time.zone)
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')}"
Comment on lines +99 to 102
Copy link
Member

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.

end
Expand Down Expand Up @@ -289,17 +288,6 @@ def load_meetings(query)
.paginate(page: page_param, per_page: per_page_param)
end

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
Comment on lines -292 to -301
Copy link
Member

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).


def build_meeting
@meeting = Meeting.new
@meeting.project = @project
Expand Down
4 changes: 3 additions & 1 deletion modules/meeting/app/forms/meeting/start_time.rb
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@
#++

class Meeting::StartTime < ApplicationForm
include Redmine::I18n

form do |meeting_form|
meeting_form.text_field(
name: :start_time_hour,
Expand All @@ -36,7 +38,7 @@ class Meeting::StartTime < ApplicationForm
label: Meeting.human_attribute_name(:start_time),
leading_visual: { icon: :clock },
required: true,
caption: Time.zone.to_s[/\((.*?)\)/m, 1]
caption: formatted_time_zone_offset
)
end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ class MeetingAgendaItem::MeetingForm < ApplicationForm
label: "#{meeting.project.name}: " \
"#{meeting.title} " \
"#{format_date(meeting.start_time)} " \
"#{format_time(meeting.start_time, false)}",
"#{format_time(meeting.start_time, include_date: false)}",
value: meeting.id
)
end
Expand Down
7 changes: 0 additions & 7 deletions modules/meeting/app/mailers/meeting_mailer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,6 @@ def icalendar_notification(meeting, user, _actor, **)
set_headers @meeting

with_attached_ics(meeting, user) do
timezone = Time.zone || Time.zone_default
@formatted_timezone = format_timezone_offset timezone, @meeting.start_time
subject = "[#{@meeting.project.name}] #{@meeting.title}"
mail(to: user, subject:)
end
Expand Down Expand Up @@ -95,9 +93,4 @@ def set_headers(meeting)
headers["Content-Type"] = 'text/calendar; charset=utf-8; method="PUBLISH"; name="meeting.ics"'
headers["Content-Transfer-Encoding"] = "8bit"
end

def format_timezone_offset(timezone, time)
offset = ::ActiveSupport::TimeZone.seconds_to_utc_offset time.utc_offest_for_timezone(timezone), true
"(GMT#{offset}) #{timezone.name}"
end
end
Original file line number Diff line number Diff line change
Expand Up @@ -98,25 +98,25 @@
end
end

def event_title(event)
case activity
when :meeting
start_time = if event["meeting_start_time"].is_a?(String)
DateTime.parse(event["meeting_start_time"])
else
event["meeting_start_time"]
end
end_time = start_time + event["meeting_duration"].to_f.hours

fstart_with = format_date start_time
fstart_without = format_time start_time, false
fend_without = format_time end_time, false
fstart_without = format_time start_time, include_date: false
fend_without = format_time end_time, include_date: false

"#{I18n.t(:label_meeting)}: #{event['meeting_title']} (#{fstart_with} #{fstart_without}-#{fend_without})"
else
"#{event['meeting_content_type'].constantize.model_name.human}: #{event['meeting_title']}"
end
end

Check notice on line 119 in modules/meeting/app/models/activities/meeting_activity_provider.rb

View workflow job for this annotation

GitHub Actions / rubocop

[rubocop] modules/meeting/app/models/activities/meeting_activity_provider.rb#L101-L119 <Metrics/AbcSize>

Assignment Branch Condition size for event_title is too high. [<5, 20, 3> 20.83/17]
Raw output
modules/meeting/app/models/activities/meeting_activity_provider.rb:101:3: C: Metrics/AbcSize: Assignment Branch Condition size for event_title is too high. [<5, 20, 3> 20.83/17]

def event_type(event)
case activity
Expand Down
4 changes: 2 additions & 2 deletions modules/meeting/app/models/activities/meeting_event_mapper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -204,8 +204,8 @@ def event_title(_journal, data)
end_time = start_time + data[:meeting_duration].to_f.hours

fstart_with = format_date start_time
fstart_without = format_time start_time, false
fend_without = format_time end_time, false
fstart_without = format_time start_time, include_date: false
fend_without = format_time end_time, include_date: false

"#{I18n.t(:label_meeting)}: #{data[:meeting_title]} (#{fstart_with} #{fstart_without}-#{fend_without})"
end
Expand Down
7 changes: 4 additions & 3 deletions modules/meeting/app/models/meeting.rb
Original file line number Diff line number Diff line change
Expand Up @@ -253,14 +253,15 @@ def allowed_participants

def set_initial_values
# set defaults
write_attribute(:start_time, Date.tomorrow + 10.hours) if start_time.nil?
# Start date is set to tomorrow at 10 AM (Current users local time)
write_attribute(:start_time, User.current.time_zone.now.at_midnight + 34.hours) if start_time.nil?
self.duration ||= 1
update_derived_fields
end

def update_derived_fields
@start_date = start_time.to_date.iso8601
@start_time_hour = start_time.strftime("%H:%M")
@start_date = format_time_as_date(start_time, format: "%Y-%m-%d")
@start_time_hour = format_time(start_time, include_date: false, format: "%H:%M")
end

private
Expand Down
2 changes: 1 addition & 1 deletion modules/meeting/app/models/meeting/journalized.rb
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ module Meeting::Journalized
acts_as_event title: Proc.new { |o|
"#{I18n.t(:label_meeting)}: #{o.title} \
#{format_date o.start_time} \
#{format_time o.start_time, false}-#{format_time o.end_time, false})"
#{format_time o.start_time, include_date: false}-#{format_time o.end_time, include_date: false})"
},
url: Proc.new { |o| { controller: "/meetings", action: "show", id: o } },
author: Proc.new(&:user),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
<p><%= t(:text_notificiation_invited) %></p>

<ul>
<li><%=t :label_meeting_date_time %>: <%= format_time_as_date @meeting.start_time %> <%= format_time @meeting.start_time, false %>-<%= format_time @meeting.end_time, false %> <%= @formatted_timezone %></li>
<li><%=t :label_meeting_date_time %>: <%= format_time_as_date @meeting.start_time %> <%= format_time @meeting.start_time, include_date: false %>-<%= format_time @meeting.end_time, include_date: false %> (<%= formatted_time_zone_offset %>)</li>
<li><%=Meeting.human_attribute_name(:location) %>: <%= @meeting.location %></li>
<li><%=Meeting.human_attribute_name(:participants_invited) %>: <%= @meeting.participants.invited.sort.join("; ") %></li>
<li><%=Meeting.human_attribute_name(:participants_attended) %>: <%= @meeting.participants.attended.sort.join("; ") %></li>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@

<%= t(:text_notificiation_invited) %>

<%=t :label_meeting_date_time %>: <%= format_time_as_date @meeting.start_time %> <%= format_time @meeting.start_time, false %>-<%= format_time @meeting.end_time, false %> <%= @formatted_timezone %>
<%=t :label_meeting_date_time %>: <%= format_time_as_date @meeting.start_time %> <%= format_time @meeting.start_time, include_date: false %>-<%= format_time @meeting.end_time, include_date: false %> (<%= formatted_time_zone_offset %>)
<%=Meeting.human_attribute_name(:location) %>: <%= @meeting.location %>
<%=Meeting.human_attribute_name(:participants_invited) %>: <%= @meeting.participants.invited.sort.join("; ") %>
<%=Meeting.human_attribute_name(:participants_attended) %>: <%= @meeting.participants.attended.sort.join("; ") %>
4 changes: 2 additions & 2 deletions modules/meeting/app/views/meeting_mailer/invited.html.erb
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ See COPYRIGHT and LICENSE files for more details.
<%= I18n.t(:label_meeting_date_time) %>
</td>
<td style="<%= placeholder_text_styles %>">
<%= format_time_as_date @meeting.start_time %> <%= format_time @meeting.start_time, false %>
<%= format_time_as_date @meeting.start_time %> <%= format_time @meeting.start_time, include_date: false %>
-
<%= format_time @meeting.end_time, false %> <%= Time.zone %>
<%= format_time @meeting.end_time, include_date: false %> (<%= formatted_time_zone_offset %>)
</td>
</tr>
<% if @meeting.location.present? %>
Expand Down
2 changes: 1 addition & 1 deletion modules/meeting/app/views/meeting_mailer/invited.text.erb
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ See COPYRIGHT and LICENSE files for more details.
<%= @meeting.project.name %>: <%= @meeting.title %> (<%= meeting_url(@meeting) %>)
<%= @meeting.author %>

<%=t :label_meeting_date_time %>: <%= format_time_as_date @meeting.start_time %> <%= format_time @meeting.start_time, false %>-<%= format_time @meeting.end_time, false %> <%= Time.zone %>
<%=t :label_meeting_date_time %>: <%= format_time_as_date @meeting.start_time %> <%= format_time @meeting.start_time, include_date: false %>-<%= format_time @meeting.end_time, include_date: false %> (<%= formatted_time_zone_offset %>)
<%= Meeting.human_attribute_name(:location) %>: <%= @meeting.location %>
<%= Meeting.human_attribute_name(:participants_invited) %>: <%= @meeting.participants.invited.sort.join("; ") %>
<%= Meeting.human_attribute_name(:participants_attended) %>: <%= @meeting.participants.attended.sort.join("; ") %>
Loading