REFACTOR: Clean up some code associated with topic timers.

app/models/topic.rb

@@ -397,7 +397,7 @@ class Topic < ActiveRecord::Base
 
   def reload(options=nil)
     @post_numbers = nil
-    @topic_timer = nil
+    @public_topic_timer = nil
     super(options)
   end
 
@@ -957,7 +957,7 @@ SQL
   end
 
   def public_topic_timer
-    topic_timers.where(deleted_at: nil, public_type: true).first
+    @public_topic_timer ||= topic_timers.find_by(deleted_at: nil, public_type: true)
   end
 
   # Valid arguments for the time:
@@ -973,11 +973,9 @@ SQL
   #  * 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)
-    topic_timer = if TopicTimer.public_types[status_type]
+    topic_timer_options = { status_type: status_type, topic: self }
-      TopicTimer.find_or_initialize_by( status_type: status_type, topic: self )
+    topic_timer_options.merge!(user: by_user) unless TopicTimer.public_types[status_type]
-    else
+    topic_timer = TopicTimer.find_or_initialize_by(topic_timer_options)
-      TopicTimer.find_or_initialize_by( status_type: status_type, topic: self, user: by_user )
-    end
 
     if time.blank?
       topic_timer.trash!(trashed_by: by_user || Discourse.system_user)
@@ -1004,12 +1002,7 @@ SQL
       is_timestamp = time.is_a?(String)
       now = utc.now
 
-      if is_timestamp && m = /^(\d{1,2}):(\d{2})(?:\s*[AP]M)?$/i.match(time.strip)
+      if is_timestamp && time.include?("-") && timestamp = utc.parse(time)
-        # a time of day in client's time zone, like "15:00"
-        topic_timer.execute_at = utc.local(now.year, now.month, now.day, m[1].to_i, m[2].to_i)
-        topic_timer.execute_at += timezone_offset * 60 if timezone_offset
-        topic_timer.execute_at += 1.day if topic_timer.execute_at < now
-      elsif 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

spec/models/topic_spec.rb

@@ -1154,34 +1154,6 @@ describe Topic do
       end
     end
 
-    it "can take a time later in the day" do
-      Timecop.freeze(now) do
-        topic.set_or_create_timer(TopicTimer.types[:close], '13:00', {by_user: admin})
-        expect(topic.topic_timers.first.execute_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_or_create_timer(TopicTimer.types[:close], '13:00', {by_user: admin, timezone_offset: 240})
-        expect(topic.topic_timers.first.execute_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_or_create_timer(TopicTimer.types[:close], '5:00', {by_user: admin})
-        expect(topic.topic_timers.first.execute_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_or_create_timer(TopicTimer.types[:close], '1:00', {by_user: admin, timezone_offset: 240})
-        expect(topic.topic_timers.first.execute_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_or_create_timer(TopicTimer.types[:close], '2013-11-22 5:00', {by_user: admin})

spec/models/topic_timer_spec.rb

@@ -27,10 +27,13 @@ RSpec.describe TopicTimer, type: :model do
       end
 
       it 'should allow users to have their own private topic timer' do
-        Fabricate(:topic_timer, topic: topic, user: admin, status_type: TopicTimer.types[:reminder])
+        expect do
-        expect {
+          Fabricate(:topic_timer,
-          Fabricate(:topic_timer, topic: topic, user: Fabricate(:admin), status_type: TopicTimer.types[:reminder])
+            topic: topic,
-        }.to_not raise_error
+            user: Fabricate(:admin),
+            status_type: TopicTimer.types[:reminder]
+          )
+        end.to_not raise_error
       end
     end
 
@@ -220,7 +223,7 @@ RSpec.describe TopicTimer, type: :model do
       end
     end
 
-    describe 'public_type' do
+    describe '#public_type' do
       [:close, :open, :delete].each do |public_type|
         it "is true for #{public_type}" do
           timer = Fabricate(:topic_timer, status_type: described_class.types[public_type])