From b28b418363ae1d925e7a0369c5f192005060d9b0 Mon Sep 17 00:00:00 2001 From: Dan Ungureanu Date: Mon, 11 Mar 2019 11:19:58 +0200 Subject: [PATCH] FIX: Various improvements to post notices. - Notices are visible only by poster and trust level 2+ users. - Notices are not generated for non-human or staged users. - Notices are deleted when post is deleted. --- app/models/post.rb | 3 +- app/serializers/post_serializer.rb | 4 ++- lib/post_creator.rb | 2 ++ spec/components/post_creator_spec.rb | 36 ++++++++++++++---------- spec/models/post_spec.rb | 7 +---- spec/serializers/post_serializer_spec.rb | 25 ++++++++++++++++ 6 files changed, 54 insertions(+), 23 deletions(-) diff --git a/app/models/post.rb b/app/models/post.rb index 73f52ecb6eb..8e03538f527 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -188,13 +188,13 @@ class Post < ActiveRecord::Base def trash!(trashed_by = nil) self.topic_links.each(&:destroy) + self.delete_post_notices super(trashed_by) end def recover! super update_flagged_posts_count - delete_post_notices recover_public_post_actions TopicLink.extract_from(self) QuotedPost.extract_from(self) @@ -385,6 +385,7 @@ class Post < ActiveRecord::Base def delete_post_notices self.custom_fields.delete("post_notice_type") self.custom_fields.delete("post_notice_time") + self.save_custom_fields end def recover_public_post_actions diff --git a/app/serializers/post_serializer.rb b/app/serializers/post_serializer.rb index 916e2cdb6ea..3f65da2bf3d 100644 --- a/app/serializers/post_serializer.rb +++ b/app/serializers/post_serializer.rb @@ -370,6 +370,8 @@ class PostSerializer < BasicPostSerializer end def include_post_notice_type? + return false if scope.user&.id != object.user_id && !scope.user&.has_trust_level?(TrustLevel[2]) + post_notice_type.present? end @@ -378,7 +380,7 @@ class PostSerializer < BasicPostSerializer end def include_post_notice_time? - post_notice_time.present? + include_post_notice_type? && post_notice_time.present? end def locked diff --git a/lib/post_creator.rb b/lib/post_creator.rb index 2dd11e649db..6d88d1adf26 100644 --- a/lib/post_creator.rb +++ b/lib/post_creator.rb @@ -514,6 +514,8 @@ class PostCreator end def create_post_notice + return if @user.id < 0 || @user.staged + last_post_time = Post.where(user_id: @user.id) .order(created_at: :desc) .limit(1) diff --git a/spec/components/post_creator_spec.rb b/spec/components/post_creator_spec.rb index 86ebf54ba1c..78b2d6437fd 100644 --- a/spec/components/post_creator_spec.rb +++ b/spec/components/post_creator_spec.rb @@ -1263,29 +1263,35 @@ describe PostCreator do context "#create_post_notice" do let(:user) { Fabricate(:user) } - let(:new_user) { Fabricate(:user) } - let(:returning_user) { Fabricate(:user) } + let(:staged) { Fabricate(:staged) } - it "generates post notices" do - # new users - post = PostCreator.create(new_user, title: "one of my first topics", raw: "one of my first posts") + it "generates post notices for new users" do + post = PostCreator.create(user, title: "one of my first topics", raw: "one of my first posts") expect(post.custom_fields["post_notice_type"]).to eq("first") - post = PostCreator.create(new_user, title: "another one of my first topics", raw: "another one of my first posts") + post = PostCreator.create(user, title: "another one of my first topics", raw: "another one of my first posts") expect(post.custom_fields["post_notice_type"]).to eq(nil) - - # returning users - SiteSetting.returning_users_days = 30 - old_post = Fabricate(:post, user: returning_user, created_at: 31.days.ago) - post = PostCreator.create(returning_user, title: "this is a returning topic", raw: "this is a post") - expect(post.custom_fields["post_notice_type"]).to eq("returning") - expect(post.custom_fields["post_notice_time"]).to eq(old_post.created_at.iso8601) end - it "does not generate post notices" do - Fabricate(:post, user: user, created_at: 3.days.ago) + it "generates post notices for returning users" do + SiteSetting.returning_users_days = 30 + old_post = Fabricate(:post, user: user, created_at: 31.days.ago) + + post = PostCreator.create(user, title: "this is a returning topic", raw: "this is a post") + expect(post.custom_fields["post_notice_type"]).to eq("returning") + expect(post.custom_fields["post_notice_time"]).to eq(old_post.created_at.iso8601) + post = PostCreator.create(user, title: "this is another topic", raw: "this is my another post") expect(post.custom_fields["post_notice_type"]).to eq(nil) expect(post.custom_fields["post_notice_time"]).to eq(nil) end + + it "does not generate for non-human or staged users" do + [Discourse.system_user, staged].each do |user| + expect(user.posts.size).to eq(0) + post = PostCreator.create(user, title: "#{user.name}'s first topic", raw: "#{user.name}'s first post") + expect(post.custom_fields["post_notice_type"]).to eq(nil) + expect(post.custom_fields["post_notice_time"]).to eq(nil) + end + end end end diff --git a/spec/models/post_spec.rb b/spec/models/post_spec.rb index 84418a6bddb..93eaf3600f4 100644 --- a/spec/models/post_spec.rb +++ b/spec/models/post_spec.rb @@ -143,14 +143,9 @@ describe Post do post } - before do - post.trash! - post.reload - end - describe 'recovery' do it 'deletes notices' do - expect { post.recover! } + expect { post.trash! } .to change { post.custom_fields.length }.from(2).to(0) end end diff --git a/spec/serializers/post_serializer_spec.rb b/spec/serializers/post_serializer_spec.rb index 1ca29e2b2d3..4925b79b83d 100644 --- a/spec/serializers/post_serializer_spec.rb +++ b/spec/serializers/post_serializer_spec.rb @@ -174,4 +174,29 @@ describe PostSerializer do end + context "a post with notices" do + let(:user) { Fabricate(:user, trust_level: 1) } + let(:user_tl1) { Fabricate(:user, trust_level: 1) } + let(:user_tl2) { Fabricate(:user, trust_level: 2) } + + let(:post) { + post = Fabricate(:post, user: user) + post.custom_fields["post_notice_type"] = "returning" + post.custom_fields["post_notice_time"] = 1.day.ago + post.save_custom_fields + post + } + + def json_for_user(user) + PostSerializer.new(post, scope: Guardian.new(user), root: false).as_json + end + + it "will not show for poster and TL2+ users" do + expect(json_for_user(nil)[:post_notice_type]).to eq(nil) + expect(json_for_user(user)[:post_notice_type]).to eq("returning") + expect(json_for_user(user_tl1)[:post_notice_type]).to eq(nil) + expect(json_for_user(user_tl2)[:post_notice_type]).to eq("returning") + end + end + end