From ea8cf1a208d198ae1d7ff1e795830766c01be7d0 Mon Sep 17 00:00:00 2001 From: Neil Lalonde Date: Wed, 27 May 2015 12:22:34 -0400 Subject: [PATCH] FIX: topic auto-close uses the client's time zone --- .../controllers/edit-topic-auto-close.js.es6 | 1 + app/controllers/topics_controller.rb | 4 +- app/models/topic.rb | 18 ++++-- lib/topic_creator.rb | 2 +- spec/controllers/topics_controller_spec.rb | 6 +- spec/models/topic_spec.rb | 57 +++++++++++++++---- 6 files changed, 67 insertions(+), 21 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/edit-topic-auto-close.js.es6 b/app/assets/javascripts/discourse/controllers/edit-topic-auto-close.js.es6 index 24319137d..7fa4458b4 100644 --- a/app/assets/javascripts/discourse/controllers/edit-topic-auto-close.js.es6 +++ b/app/assets/javascripts/discourse/controllers/edit-topic-auto-close.js.es6 @@ -36,6 +36,7 @@ export default ObjectController.extend(ModalFunctionality, { data: { auto_close_time: time, auto_close_based_on_last_post: this.get("model.details.auto_close_based_on_last_post"), + timezone_offset: (new Date().getTimezoneOffset()) } }).then(function(result){ if (result.success) { diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 30e9c483b..e7dfbab3b 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -195,14 +195,14 @@ class TopicsController < ApplicationController end def autoclose - params.permit(:auto_close_time) + params.permit(:auto_close_time, :timezone_offset) params.require(:auto_close_based_on_last_post) topic = Topic.find_by(id: params[:topic_id].to_i) guardian.ensure_can_moderate!(topic) topic.auto_close_based_on_last_post = params[:auto_close_based_on_last_post] - topic.set_auto_close(params[:auto_close_time], current_user) + topic.set_auto_close(params[:auto_close_time], {by_user: current_user, timezone_offset: params[:timezone_offset] ? params[:timezone_offset].to_i : nil}) if topic.save render json: success_json.merge!({ diff --git a/app/models/topic.rb b/app/models/topic.rb index c6111c005..1d0f6e457 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -760,8 +760,13 @@ class Topic < ActiveRecord::Base # * A timestamp, like "2013-11-25 13:00", when the topic should close. # * A timestamp with timezone in JSON format. (e.g., "2013-11-26T21:00:00.000Z") # * nil, to prevent the topic from automatically closing. - def set_auto_close(arg, by_user=nil) + # Options: + # * by_user: User who is setting the auto close time + # * timezone_offset: (Integer) offset from UTC in minutes of the given argument. Default 0. + def set_auto_close(arg, opts={}) self.auto_close_hours = nil + by_user = opts[:by_user] + offset_minutes = opts[:timezone_offset] if self.auto_close_based_on_last_post num_hours = arg.to_f @@ -773,12 +778,17 @@ class Topic < ActiveRecord::Base self.auto_close_at = nil end else + utc = Time.find_zone("UTC") if arg.is_a?(String) && m = /^(\d{1,2}):(\d{2})(?:\s*[AP]M)?$/i.match(arg.strip) - now = Time.zone.now - self.auto_close_at = Time.zone.local(now.year, now.month, now.day, m[1].to_i, m[2].to_i) + # a time of day in client's time zone, like "15:00" + now = utc.now + self.auto_close_at = utc.local(now.year, now.month, now.day, m[1].to_i, m[2].to_i) + self.auto_close_at += offset_minutes * 60 if offset_minutes self.auto_close_at += 1.day if self.auto_close_at < now - elsif arg.is_a?(String) && arg.include?("-") && timestamp = Time.zone.parse(arg) + elsif arg.is_a?(String) && arg.include?("-") && timestamp = utc.parse(arg) + # a timestamp in client's time zone, like "2015-5-27 12:00" self.auto_close_at = timestamp + self.auto_close_at += offset_minutes * 60 if offset_minutes self.errors.add(:auto_close_at, :invalid) if timestamp < Time.zone.now else num_hours = arg.to_f diff --git a/lib/topic_creator.rb b/lib/topic_creator.rb index 13d26eb15..8213bbeb8 100644 --- a/lib/topic_creator.rb +++ b/lib/topic_creator.rb @@ -107,7 +107,7 @@ class TopicCreator def setup_auto_close_time(topic) return unless @opts[:auto_close_time].present? return unless @guardian.can_moderate?(topic) - topic.set_auto_close(@opts[:auto_close_time], @user) + topic.set_auto_close(@opts[:auto_close_time], {by_user: @user}) end def process_private_message(topic) diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index 8e5b83b19..c3f05529c 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -871,8 +871,8 @@ describe TopicsController do end it "can set a topic's auto close time and 'based on last post' property" do - Topic.any_instance.expects(:set_auto_close).with("24", @admin) - xhr :put, :autoclose, topic_id: @topic.id, auto_close_time: '24', auto_close_based_on_last_post: true + Topic.any_instance.expects(:set_auto_close).with("24", {by_user: @admin, timezone_offset: -240}) + xhr :put, :autoclose, topic_id: @topic.id, auto_close_time: '24', auto_close_based_on_last_post: true, timezone_offset: -240 json = ::JSON.parse(response.body) expect(json).to have_key('auto_close_at') expect(json).to have_key('auto_close_hours') @@ -880,7 +880,7 @@ describe TopicsController do it "can remove a topic's auto close time" do Topic.any_instance.expects(:set_auto_close).with(nil, anything) - xhr :put, :autoclose, topic_id: @topic.id, auto_close_time: nil, auto_close_based_on_last_post: false + xhr :put, :autoclose, topic_id: @topic.id, auto_close_time: nil, auto_close_based_on_last_post: false, timezone_offset: -240 end end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 168e1ec91..673c7e9b6 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -951,7 +951,7 @@ describe Topic do job_args[:user_id] == topic_closer.id end topic = Fabricate(:topic, user: topic_creator) - topic.set_auto_close(7, topic_closer).save + topic.set_auto_close(7, {by_user: topic_closer}).save end it "ignores the category's default auto-close" do @@ -1079,42 +1079,77 @@ describe Topic do it 'can take a number of hours as an integer' do Timecop.freeze(now) do - topic.set_auto_close(72, admin) + topic.set_auto_close(72, {by_user: admin}) + expect(topic.auto_close_at).to eq(3.days.from_now) + end + end + + it 'can take a number of hours as an integer, with timezone offset' do + Timecop.freeze(now) do + topic.set_auto_close(72, {by_user: admin, timezone_offset: 240}) expect(topic.auto_close_at).to eq(3.days.from_now) end end it 'can take a number of hours as a string' do Timecop.freeze(now) do - topic.set_auto_close('18', admin) + topic.set_auto_close('18', {by_user: admin}) + expect(topic.auto_close_at).to eq(18.hours.from_now) + end + end + + it 'can take a number of hours as a string, with timezone offset' do + Timecop.freeze(now) do + topic.set_auto_close('18', {by_user: admin, timezone_offset: 240}) expect(topic.auto_close_at).to eq(18.hours.from_now) end end it "can take a time later in the day" do Timecop.freeze(now) do - topic.set_auto_close('13:00', admin) + topic.set_auto_close('13:00', {by_user: admin}) expect(topic.auto_close_at).to eq(Time.zone.local(2013,11,20,13,0)) end end + it "can take a time later in the day, with timezone offset" do + Timecop.freeze(now) do + topic.set_auto_close('13:00', {by_user: admin, timezone_offset: 240}) + expect(topic.auto_close_at).to eq(Time.zone.local(2013,11,20,17,0)) + end + end + it "can take a time for the next day" do Timecop.freeze(now) do - topic.set_auto_close('5:00', admin) + topic.set_auto_close('5:00', {by_user: admin}) + expect(topic.auto_close_at).to eq(Time.zone.local(2013,11,21,5,0)) + end + end + + it "can take a time for the next day, with timezone offset" do + Timecop.freeze(now) do + topic.set_auto_close('1:00', {by_user: admin, timezone_offset: 240}) expect(topic.auto_close_at).to eq(Time.zone.local(2013,11,21,5,0)) end end it "can take a timestamp for a future time" do Timecop.freeze(now) do - topic.set_auto_close('2013-11-22 5:00', admin) + topic.set_auto_close('2013-11-22 5:00', {by_user: admin}) expect(topic.auto_close_at).to eq(Time.zone.local(2013,11,22,5,0)) end end + it "can take a timestamp for a future time, with timezone offset" do + Timecop.freeze(now) do + topic.set_auto_close('2013-11-22 5:00', {by_user: admin, timezone_offset: 240}) + expect(topic.auto_close_at).to eq(Time.zone.local(2013,11,22,9,0)) + end + end + it "sets a validation error when given a timestamp in the past" do Timecop.freeze(now) do - topic.set_auto_close('2013-11-19 5:00', admin) + topic.set_auto_close('2013-11-19 5:00', {by_user: admin}) expect(topic.auto_close_at).to eq(Time.zone.local(2013,11,19,5,0)) expect(topic.errors[:auto_close_at]).to be_present end @@ -1122,23 +1157,23 @@ describe Topic do it "can take a timestamp with timezone" do Timecop.freeze(now) do - topic.set_auto_close('2013-11-25T01:35:00-08:00', admin) + topic.set_auto_close('2013-11-25T01:35:00-08:00', {by_user: admin}) expect(topic.auto_close_at).to eq(Time.utc(2013,11,25,9,35)) end end it 'sets auto_close_user to given user if it is a staff or TL4 user' do - topic.set_auto_close(3, admin) + topic.set_auto_close(3, {by_user: admin}) expect(topic.auto_close_user_id).to eq(admin.id) end it 'sets auto_close_user to given user if it is a TL4 user' do - topic.set_auto_close(3, trust_level_4) + topic.set_auto_close(3, {by_user: trust_level_4}) expect(topic.auto_close_user_id).to eq(trust_level_4.id) end it 'sets auto_close_user to system user if given user is not staff or a TL4 user' do - topic.set_auto_close(3, Fabricate.build(:user, id: 444)) + topic.set_auto_close(3, {by_user: Fabricate.build(:user, id: 444)}) expect(topic.auto_close_user_id).to eq(admin.id) end