From 4382a0bb0792fedbfeb0d1572d6e2793ee9bb1e6 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 22 May 2017 15:01:33 +0800 Subject: [PATCH 1/2] Rename `PostTimestampChanger` -> `TopicTimestampChanger`. --- app/controllers/topics_controller.rb | 6 ++- app/jobs/regular/publish_topic_to_category.rb | 2 +- ..._changer.rb => topic_timestamp_changer.rb} | 4 +- ...pec.rb => topic_timestamp_changer_spec.rb} | 38 +++++++++++-------- 4 files changed, 30 insertions(+), 20 deletions(-) rename app/services/{post_timestamp_changer.rb => topic_timestamp_changer.rb} (92%) rename spec/services/{post_timestamp_changer_spec.rb => topic_timestamp_changer_spec.rb} (61%) diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 6a79974bae..5e820873d6 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -558,8 +558,10 @@ class TopicsController < ApplicationController guardian.ensure_can_change_post_timestamps! begin - PostTimestampChanger.new( topic_id: params[:topic_id].to_i, - timestamp: params[:timestamp].to_i ).change! + TopicTimestampChanger.new( + topic_id: params[:topic_id].to_i, + timestamp: params[:timestamp].to_i + ).change! render json: success_json rescue ActiveRecord::RecordInvalid diff --git a/app/jobs/regular/publish_topic_to_category.rb b/app/jobs/regular/publish_topic_to_category.rb index 3b375a3aee..9127b6841e 100644 --- a/app/jobs/regular/publish_topic_to_category.rb +++ b/app/jobs/regular/publish_topic_to_category.rb @@ -7,7 +7,7 @@ module Jobs topic = topic_timer.topic return if topic.blank? - PostTimestampChanger.new(timestamp: Time.zone.now, topic: topic).change! do + TopicTimestampChanger.new(timestamp: Time.zone.now, topic: topic).change! do if topic.private_message? topic = TopicConverter.new(topic, Discourse.system_user) .convert_to_public_topic(topic_timer.category_id) diff --git a/app/services/post_timestamp_changer.rb b/app/services/topic_timestamp_changer.rb similarity index 92% rename from app/services/post_timestamp_changer.rb rename to app/services/topic_timestamp_changer.rb index f2a9b3682a..4755baea2e 100644 --- a/app/services/post_timestamp_changer.rb +++ b/app/services/topic_timestamp_changer.rb @@ -1,4 +1,4 @@ -class PostTimestampChanger +class TopicTimestampChanger def initialize(params) @topic = params[:topic] || Topic.with_deleted.find(params[:topic_id]) @posts = @topic.posts @@ -9,12 +9,14 @@ class PostTimestampChanger def change! ActiveRecord::Base.transaction do last_posted_at = @timestamp + now = Time.zone.now @posts.each do |post| if post.is_first_post? update_post(post, @timestamp) else new_created_at = Time.at(post.created_at.to_f + @time_difference) + new_created_at = now if new_created_at > now last_posted_at = new_created_at if new_created_at > last_posted_at update_post(post, new_created_at) end diff --git a/spec/services/post_timestamp_changer_spec.rb b/spec/services/topic_timestamp_changer_spec.rb similarity index 61% rename from spec/services/post_timestamp_changer_spec.rb rename to spec/services/topic_timestamp_changer_spec.rb index 7c3651bf23..ba77a804f0 100644 --- a/spec/services/post_timestamp_changer_spec.rb +++ b/spec/services/topic_timestamp_changer_spec.rb @@ -1,6 +1,6 @@ require 'rails_helper' -describe PostTimestampChanger do +describe TopicTimestampChanger do describe "change!" do let(:old_timestamp) { Time.zone.now } let(:new_timestamp) { old_timestamp + 1.day } @@ -9,25 +9,31 @@ describe PostTimestampChanger do let!(:p2) { Fabricate(:post, topic: topic, created_at: old_timestamp + 1.day) } let(:params) { { topic_id: topic.id, timestamp: new_timestamp.to_f } } - it 'changes the timestamp of the topic and opening post' do - PostTimestampChanger.new(params).change! + context 'new timestamp is in the future' do + let(:new_timestamp) { old_timestamp + 2.day } - topic.reload - [:created_at, :updated_at, :bumped_at].each do |column| - expect(topic.public_send(column)).to be_within_one_second_of(new_timestamp) + it 'changes the timestamp of the topic and opening post' do + Timecop.freeze do + TopicTimestampChanger.new(params).change! + + topic.reload + [:created_at, :updated_at, :bumped_at].each do |column| + expect(topic.public_send(column)).to be_within_one_second_of(new_timestamp) + end + + p1.reload + [:created_at, :updated_at].each do |column| + expect(p1.public_send(column)).to be_within_one_second_of(new_timestamp) + end + + expect(topic.last_posted_at).to be_within_one_second_of(p2.reload.created_at) + end end - - p1.reload - [:created_at, :updated_at].each do |column| - expect(p1.public_send(column)).to be_within_one_second_of(new_timestamp) - end - - expect(topic.last_posted_at).to be_within_one_second_of(p2.reload.created_at) end describe 'predated timestamp' do it 'updates the timestamp of posts in the topic with the time difference applied' do - PostTimestampChanger.new(params).change! + TopicTimestampChanger.new(params).change! p2.reload [:created_at, :updated_at].each do |column| @@ -40,7 +46,7 @@ describe PostTimestampChanger do let(:new_timestamp) { old_timestamp - 1.day } it 'updates the timestamp of posts in the topic with the time difference applied' do - PostTimestampChanger.new(params).change! + TopicTimestampChanger.new(params).change! p2.reload [:created_at, :updated_at].each do |column| @@ -53,7 +59,7 @@ describe PostTimestampChanger do $redis.set AdminDashboardData.stats_cache_key, "X" $redis.set About.stats_cache_key, "X" - PostTimestampChanger.new(params).change! + TopicTimestampChanger.new(params).change! expect($redis.get(AdminDashboardData.stats_cache_key)).to eq(nil) expect($redis.get(About.stats_cache_key)).to eq(nil) From 238a15630023897b4cbc851ac912f098becb31fa Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Mon, 22 May 2017 16:03:49 +0800 Subject: [PATCH 2/2] FIX: `TopicTimestampChanger` should not allow timestamps in the future. --- .../controllers/change-timestamp.js.es6 | 3 +- app/controllers/topics_controller.rb | 4 +- app/services/topic_timestamp_changer.rb | 15 +++-- spec/services/topic_timestamp_changer_spec.rb | 57 +++++++++++-------- 4 files changed, 46 insertions(+), 33 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/change-timestamp.js.es6 b/app/assets/javascripts/discourse/controllers/change-timestamp.js.es6 index c24cf785c9..dd22cef179 100644 --- a/app/assets/javascripts/discourse/controllers/change-timestamp.js.es6 +++ b/app/assets/javascripts/discourse/controllers/change-timestamp.js.es6 @@ -1,6 +1,7 @@ import ModalFunctionality from 'discourse/mixins/modal-functionality'; import computed from 'ember-addons/ember-computed-decorators'; import DiscourseURL from 'discourse/lib/url'; +import Topic from 'discourse/models/topic'; // Modal related to changing the timestamp of posts export default Ember.Controller.extend(ModalFunctionality, { @@ -43,7 +44,7 @@ export default Ember.Controller.extend(ModalFunctionality, { const self = this, topic = this.get('topicController.model'); - Discourse.Topic.changeTimestamp( + Topic.changeTimestamp( topic.get('id'), this.get('createdAt').unix() ).then(function() { diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index 5e820873d6..bc4455de50 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -560,11 +560,11 @@ class TopicsController < ApplicationController begin TopicTimestampChanger.new( topic_id: params[:topic_id].to_i, - timestamp: params[:timestamp].to_i + timestamp: params[:timestamp].to_f ).change! render json: success_json - rescue ActiveRecord::RecordInvalid + rescue ActiveRecord::RecordInvalid, TopicTimestampChanger::InvalidTimestampError render json: failed_json, status: 422 end end diff --git a/app/services/topic_timestamp_changer.rb b/app/services/topic_timestamp_changer.rb index 4755baea2e..a979d0a347 100644 --- a/app/services/topic_timestamp_changer.rb +++ b/app/services/topic_timestamp_changer.rb @@ -1,22 +1,27 @@ class TopicTimestampChanger - def initialize(params) - @topic = params[:topic] || Topic.with_deleted.find(params[:topic_id]) + class InvalidTimestampError < StandardError; end + + def initialize(timestamp:, topic: nil, topic_id: nil) + @topic = topic || Topic.with_deleted.find(topic_id) @posts = @topic.posts - @timestamp = Time.at(params[:timestamp]) + @current_timestamp = Time.zone.now + @timestamp = Time.zone.at(timestamp) + + raise InvalidTimestampError if @timestamp.to_f > @current_timestamp.to_f + @time_difference = calculate_time_difference end def change! ActiveRecord::Base.transaction do last_posted_at = @timestamp - now = Time.zone.now @posts.each do |post| if post.is_first_post? update_post(post, @timestamp) else new_created_at = Time.at(post.created_at.to_f + @time_difference) - new_created_at = now if new_created_at > now + new_created_at = @current_timestamp if new_created_at > @current_timestamp last_posted_at = new_created_at if new_created_at > last_posted_at update_post(post, new_created_at) end diff --git a/spec/services/topic_timestamp_changer_spec.rb b/spec/services/topic_timestamp_changer_spec.rb index ba77a804f0..1e9821adeb 100644 --- a/spec/services/topic_timestamp_changer_spec.rb +++ b/spec/services/topic_timestamp_changer_spec.rb @@ -4,53 +4,60 @@ describe TopicTimestampChanger do describe "change!" do let(:old_timestamp) { Time.zone.now } let(:new_timestamp) { old_timestamp + 1.day } - let!(:topic) { Fabricate(:topic, created_at: old_timestamp) } + let(:topic) { Fabricate(:topic, created_at: old_timestamp) } let!(:p1) { Fabricate(:post, topic: topic, created_at: old_timestamp) } let!(:p2) { Fabricate(:post, topic: topic, created_at: old_timestamp + 1.day) } - let(:params) { { topic_id: topic.id, timestamp: new_timestamp.to_f } } context 'new timestamp is in the future' do let(:new_timestamp) { old_timestamp + 2.day } + it 'should raise the right error' do + expect { TopicTimestampChanger.new(topic: topic, timestamp: new_timestamp.to_f).change! } + .to raise_error(TopicTimestampChanger::InvalidTimestampError) + end + end + + context 'new timestamp is in the past' do + let(:new_timestamp) { old_timestamp - 2.day } + it 'changes the timestamp of the topic and opening post' do Timecop.freeze do - TopicTimestampChanger.new(params).change! + TopicTimestampChanger.new(topic: topic, timestamp: new_timestamp.to_f).change! topic.reload [:created_at, :updated_at, :bumped_at].each do |column| - expect(topic.public_send(column)).to be_within_one_second_of(new_timestamp) + expect(topic.public_send(column)).to be_within(1.second).of(new_timestamp) end p1.reload [:created_at, :updated_at].each do |column| - expect(p1.public_send(column)).to be_within_one_second_of(new_timestamp) + expect(p1.public_send(column)).to be_within(1.second).of(new_timestamp) end - expect(topic.last_posted_at).to be_within_one_second_of(p2.reload.created_at) + p2.reload + [:created_at, :updated_at].each do |column| + expect(p2.public_send(column)).to be_within(1.second).of(new_timestamp + 1.day) + end + + expect(topic.last_posted_at).to be_within(1.second).of(p2.reload.created_at) end end - end - describe 'predated timestamp' do - it 'updates the timestamp of posts in the topic with the time difference applied' do - TopicTimestampChanger.new(params).change! + describe 'when posts have timestamps in the future' do + let(:new_timestamp) { Time.zone.now } + let(:p3) { Fabricate(:post, topic: topic, created_at: new_timestamp + 3.day) } - p2.reload - [:created_at, :updated_at].each do |column| - expect(p2.public_send(column)).to be_within_one_second_of(old_timestamp + 2.day) - end - end - end + it 'should set the new timestamp as the default timestamp' do + Timecop.freeze do + p3 + TopicTimestampChanger.new(topic: topic, timestamp: new_timestamp.to_f).change! - describe 'backdated timestamp' do - let(:new_timestamp) { old_timestamp - 1.day } + p3.reload - it 'updates the timestamp of posts in the topic with the time difference applied' do - TopicTimestampChanger.new(params).change! - - p2.reload - [:created_at, :updated_at].each do |column| - expect(p2.public_send(column)).to be_within_one_second_of(old_timestamp) + [:created_at, :updated_at].each do |column| + expect(p3.public_send(column)).to be_within(1.second).of(new_timestamp) + end + end end end end @@ -59,7 +66,7 @@ describe TopicTimestampChanger do $redis.set AdminDashboardData.stats_cache_key, "X" $redis.set About.stats_cache_key, "X" - TopicTimestampChanger.new(params).change! + TopicTimestampChanger.new(topic: topic, timestamp: Time.zone.now.to_f).change! expect($redis.get(AdminDashboardData.stats_cache_key)).to eq(nil) expect($redis.get(About.stats_cache_key)).to eq(nil)