From f0ef307d2d98a892c0422b4248befa6dd6863048 Mon Sep 17 00:00:00 2001 From: Joffrey JAFFEUX Date: Thu, 7 Dec 2017 14:42:58 +0100 Subject: [PATCH] FIX: topic timer offset applied two times timezone offset was calculated and sent from browser to server, it would be applied on utc time generated from '2013-11-22 5:00' format for example and then sent back to browser which would display it thinking it's UTC time using `moment(utc time)` when it's in fact an UTC time we have offseted with the initial user timezone. This is impossible to automatically test in the current app state. Easiest reproduction is in live browser after setting your timezone to `America/New_York`, when setting a topic timer to later_today, after save, the time under the topic should be off to something roughly equal +1/-1 hour to your timezone offset. --- .../discourse/models/topic-timer.js.es6 | 3 +-- app/controllers/topics_controller.rb | 3 +-- app/models/topic.rb | 4 +--- spec/models/topic_spec.rb | 18 ------------------ 4 files changed, 3 insertions(+), 25 deletions(-) diff --git a/app/assets/javascripts/discourse/models/topic-timer.js.es6 b/app/assets/javascripts/discourse/models/topic-timer.js.es6 index 909ff9c3652..12b2b31ce9a 100644 --- a/app/assets/javascripts/discourse/models/topic-timer.js.es6 +++ b/app/assets/javascripts/discourse/models/topic-timer.js.es6 @@ -6,8 +6,7 @@ const TopicTimer = RestModel.extend({}); TopicTimer.reopenClass({ updateStatus(topicId, time, basedOnLastPost, statusType, categoryId) { let data = { - time: time, - timezone_offset: (new Date().getTimezoneOffset()), + time, status_type: statusType }; diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 69717e3587c..7e0e8a58f8c 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -287,7 +287,7 @@ class TopicsController < ApplicationController end def timer - params.permit(:time, :timezone_offset, :based_on_last_post, :category_id) + params.permit(:time, :based_on_last_post, :category_id) params.require(:status_type) status_type = @@ -302,7 +302,6 @@ class TopicsController < ApplicationController options = { by_user: current_user, - timezone_offset: params[:timezone_offset]&.to_i, based_on_last_post: params[:based_on_last_post] } diff --git a/app/models/topic.rb b/app/models/topic.rb index 8d4fd350dfb..4293b629cf7 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -1032,10 +1032,9 @@ SQL # * `nil` to delete the topic's status update. # Options: # * by_user: User who is setting the topic's status update. - # * timezone_offset: (Integer) offset from UTC in minutes of the given argument. # * based_on_last_post: True if time should be based on timestamp of the last post. # * category_id: Category that the update will apply to. - def set_or_create_timer(status_type, time, by_user: nil, timezone_offset: 0, based_on_last_post: false, category_id: SiteSetting.uncategorized_category_id) + def set_or_create_timer(status_type, time, by_user: nil, based_on_last_post: false, category_id: SiteSetting.uncategorized_category_id) return delete_topic_timer(status_type, by_user: by_user) if time.blank? public_topic_timer = !!TopicTimer.public_types[status_type] @@ -1067,7 +1066,6 @@ SQL if is_timestamp && time.include?("-") && timestamp = utc.parse(time) # a timestamp in client's time zone, like "2015-5-27 12:00" topic_timer.execute_at = timestamp - topic_timer.execute_at += timezone_offset * 60 if timezone_offset topic_timer.errors.add(:execute_at, :invalid) if timestamp < now else num_hours = time.to_f diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 969f0c52ba9..f0a8bf0f4d4 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -1236,24 +1236,12 @@ describe Topic do expect(topic.topic_timers.first.execute_at).to eq(3.days.from_now) end - it 'can take a number of hours as an integer, with timezone offset' do - freeze_time now - topic.set_or_create_timer(TopicTimer.types[:close], 72, by_user: admin, timezone_offset: 240) - expect(topic.topic_timers.first.execute_at).to eq(3.days.from_now) - end - it 'can take a number of hours as a string' do freeze_time now topic.set_or_create_timer(TopicTimer.types[:close], '18', by_user: admin) expect(topic.topic_timers.first.execute_at).to eq(18.hours.from_now) end - it 'can take a number of hours as a string, with timezone offset' do - freeze_time now - topic.set_or_create_timer(TopicTimer.types[:close], '18', by_user: admin, timezone_offset: 240) - expect(topic.topic_timers.first.execute_at).to eq(18.hours.from_now) - end - it 'can take a number of hours as a string and can handle based on last post' do freeze_time now topic.set_or_create_timer(TopicTimer.types[:close], '18', by_user: admin, based_on_last_post: true) @@ -1266,12 +1254,6 @@ describe Topic do expect(topic.topic_timers.first.execute_at).to eq(Time.zone.local(2013, 11, 22, 5, 0)) end - it "can take a timestamp for a future time, with timezone offset" do - freeze_time now - topic.set_or_create_timer(TopicTimer.types[:close], '2013-11-22 5:00', by_user: admin, timezone_offset: 240) - expect(topic.topic_timers.first.execute_at).to eq(Time.zone.local(2013, 11, 22, 9, 0)) - end - it "sets a validation error when given a timestamp in the past" do freeze_time now topic.set_or_create_timer(TopicTimer.types[:close], '2013-11-19 5:00', by_user: admin)