From 1f6adbea5cf8ade5c589213906afabfc812d2eda Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Sat, 27 Jan 2018 18:21:22 +0530 Subject: [PATCH] FEATURE: log private message views --- app/models/user_history.rb | 6 +- app/services/staff_action_logger.rb | 7 ++ config/locales/client.en.yml | 1 + config/locales/server.en.yml | 3 + config/site_settings.yml | 1 + lib/topic_view.rb | 5 +- spec/components/topic_view_spec.rb | 99 +++++++++++++---------- spec/services/staff_action_logger_spec.rb | 18 +++++ 8 files changed, 93 insertions(+), 47 deletions(-) diff --git a/app/models/user_history.rb b/app/models/user_history.rb index 980ba8728f0..7bd0f68587b 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -65,7 +65,8 @@ class UserHistory < ActiveRecord::Base notified_about_get_a_room: 47, change_name: 48, post_locked: 49, - post_unlocked: 50) + post_unlocked: 50, + check_personal_message: 51) end # Staff actions is a subset of all actions, used to audit actions taken by staff users. @@ -108,7 +109,8 @@ class UserHistory < ActiveRecord::Base :backup_download, :backup_destroy, :post_locked, - :post_unlocked] + :post_unlocked, + :check_personal_message] end def self.staff_action_ids diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index 6f57021af12..6261e655809 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -379,6 +379,13 @@ class StaffActionLogger new_value: state)) end + def log_check_personal_message(topic, opts = {}) + raise Discourse::InvalidParameters.new(:topic) unless topic && topic.is_a?(Topic) + UserHistory.create(params(opts).merge(action: UserHistory.actions[:check_personal_message], + topic_id: topic.id, + context: topic.relative_url)) + end + private def params(opts = nil) diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index dc948cbbdb6..823fd110480 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3227,6 +3227,7 @@ en: custom_staff: "plugin custom action" post_locked: "post locked" post_unlocked: "post unlocked" + check_personal_message: "check personal message" screened_emails: title: "Screened Emails" description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed." diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index d698d82fbab..694378b9181 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -1463,6 +1463,9 @@ en: show_inactive_accounts: "Allow logged in users to browse profiles of inactive accounts." hide_suspension_reasons: "Don't display suspension reasons publically on user profiles." + + log_personal_messages_views: "Log personal message views by Admin for other users/groups." + user_website_domains_whitelist: "User website will be verified against these domains. Pipe-delimited list." allow_profile_backgrounds: "Allow users to upload profile backgrounds." diff --git a/config/site_settings.yml b/config/site_settings.yml index e02399e1515..1b19897afcc 100644 --- a/config/site_settings.yml +++ b/config/site_settings.yml @@ -425,6 +425,7 @@ users: hide_suspension_reasons: default: false client: true + log_personal_messages_views: false groups: enable_group_directory: diff --git a/lib/topic_view.rb b/lib/topic_view.rb index 1ad15dc8798..041f52fe3ef 100644 --- a/lib/topic_view.rb +++ b/lib/topic_view.rb @@ -489,8 +489,9 @@ class TopicView raise Discourse::NotFound if @topic.blank? # Special case: If the topic is private and the user isn't logged in, ask them # to log in! - if @topic.present? && @topic.private_message? && @user.blank? - raise Discourse::NotLoggedIn.new + if @topic.present? && @topic.private_message? + raise Discourse::NotLoggedIn.new if @user.blank? + StaffActionLogger.new(@user).log_check_personal_message(@topic) if SiteSetting.log_personal_messages_views && @topic.all_allowed_users.where(id: @user.id).blank? end raise Discourse::InvalidAccess.new("can't see #{@topic}", @topic) unless @guardian.can_see?(@topic) end diff --git a/spec/components/topic_view_spec.rb b/spec/components/topic_view_spec.rb index 3e8897ae533..1291da2e355 100644 --- a/spec/components/topic_view_spec.rb +++ b/spec/components/topic_view_spec.rb @@ -4,13 +4,13 @@ require 'topic_view' describe TopicView do let(:topic) { create_topic } - let(:coding_horror) { Fabricate(:coding_horror) } + let(:evil_trout) { Fabricate(:evil_trout) } let(:first_poster) { topic.user } - let(:topic_view) { TopicView.new(topic.id, coding_horror) } + let(:topic_view) { TopicView.new(topic.id, evil_trout) } it "raises a not found error if the topic doesn't exist" do - expect { TopicView.new(1231232, coding_horror) }.to raise_error(Discourse::NotFound) + expect { TopicView.new(1231232, evil_trout) }.to raise_error(Discourse::NotFound) end # see also spec/controllers/topics_controller_spec.rb TopicsController::show::permission errors @@ -28,23 +28,23 @@ describe TopicView do context "chunk_size" do it "returns `chunk_size` by default" do - expect(TopicView.new(topic.id, coding_horror).chunk_size).to eq(TopicView.chunk_size) + expect(TopicView.new(topic.id, evil_trout).chunk_size).to eq(TopicView.chunk_size) end it "returns `slow_chunk_size` when slow_platform is true" do - tv = TopicView.new(topic.id, coding_horror, slow_platform: true) + tv = TopicView.new(topic.id, evil_trout, slow_platform: true) expect(tv.chunk_size).to eq(TopicView.slow_chunk_size) end it "returns `print_chunk_size` when print param is true" do - tv = TopicView.new(topic.id, coding_horror, print: true) + tv = TopicView.new(topic.id, evil_trout, print: true) expect(tv.chunk_size).to eq(TopicView.print_chunk_size) end end context "with a few sample posts" do let!(:p1) { Fabricate(:post, topic: topic, user: first_poster, percent_rank: 1) } - let!(:p2) { Fabricate(:post, topic: topic, user: coding_horror, percent_rank: 0.5) } + let!(:p2) { Fabricate(:post, topic: topic, user: evil_trout, percent_rank: 0.5) } let!(:p3) { Fabricate(:post, topic: topic, user: first_poster, percent_rank: 0) } let(:moderator) { Fabricate(:moderator) } @@ -52,7 +52,7 @@ describe TopicView do it "it can find the best responses" do - best2 = TopicView.new(topic.id, coding_horror, best: 2) + best2 = TopicView.new(topic.id, evil_trout, best: 2) expect(best2.posts.count).to eq(2) expect(best2.posts[0].id).to eq(p2.id) expect(best2.posts[1].id).to eq(p3.id) @@ -67,7 +67,7 @@ describe TopicView do expect(best.posts.pluck(:id)).to match_array([p2.id, p3.id]) # should get no results for trust level too low - best = TopicView.new(topic.id, nil, best: 99, min_trust_level: coding_horror.trust_level + 1) + best = TopicView.new(topic.id, nil, best: 99, min_trust_level: evil_trout.trust_level + 1) expect(best.posts.count).to eq(0) # should filter out the posts with a score that is too low @@ -81,11 +81,11 @@ describe TopicView do # should punch through posts if the score is high enough p2.update_column(:score, 100) - best = TopicView.new(topic.id, nil, best: 99, bypass_trust_level_score: 100, min_trust_level: coding_horror.trust_level + 1) + best = TopicView.new(topic.id, nil, best: 99, bypass_trust_level_score: 100, min_trust_level: evil_trout.trust_level + 1) expect(best.posts.count).to eq(1) # 0 means ignore - best = TopicView.new(topic.id, nil, best: 99, bypass_trust_level_score: 0, min_trust_level: coding_horror.trust_level + 1) + best = TopicView.new(topic.id, nil, best: 99, bypass_trust_level_score: 0, min_trust_level: evil_trout.trust_level + 1) expect(best.posts.count).to eq(0) # If we restrict to posts a moderator liked, return none @@ -109,6 +109,19 @@ describe TopicView do expect { TopicView.new(topic.id, nil) }.to raise_error(Discourse::NotLoggedIn) end + it "logs personal message views if log_check_personal_message is enabled" do + SiteSetting.log_personal_messages_views = true + private_message = Fabricate(:private_message_topic) + allowed_user = private_message.topic_allowed_users.first.user + + TopicView.new(private_message.id, allowed_user) + expect(UserHistory.where(action: UserHistory.actions[:check_personal_message]).count).to eq(0) + + evil_trout.admin = true + TopicView.new(private_message.id, evil_trout) + expect(UserHistory.where(action: UserHistory.actions[:check_personal_message]).count).to eq(1) + end + it "provides an absolute url" do expect(topic_view.absolute_url).to be_present end @@ -161,11 +174,11 @@ describe TopicView do context '.post_counts_by_user' do it 'returns the two posters with their appropriate counts' do - Fabricate(:post, topic: topic, user: coding_horror, post_type: Post.types[:whisper]) + Fabricate(:post, topic: topic, user: evil_trout, post_type: Post.types[:whisper]) - expect(topic_view.post_counts_by_user.to_a).to match_array([[first_poster.id, 2], [coding_horror.id, 2]]) + expect(topic_view.post_counts_by_user.to_a).to match_array([[first_poster.id, 2], [evil_trout.id, 2]]) - expect(TopicView.new(topic.id, first_poster).post_counts_by_user.to_a).to match_array([[first_poster.id, 2], [coding_horror.id, 1]]) + expect(TopicView.new(topic.id, first_poster).post_counts_by_user.to_a).to match_array([[first_poster.id, 2], [evil_trout.id, 1]]) end it "doesn't return counts for posts with authors who have been deleted" do @@ -178,7 +191,7 @@ describe TopicView do context '.participants' do it 'returns the two participants hashed by id' do - expect(topic_view.participants.to_a).to match_array([[first_poster.id, first_poster], [coding_horror.id, coding_horror]]) + expect(topic_view.participants.to_a).to match_array([[first_poster.id, first_poster], [evil_trout.id, evil_trout]]) end end @@ -188,7 +201,7 @@ describe TopicView do end it 'returns the like' do - PostAction.act(coding_horror, p1, PostActionType.types[:like]) + PostAction.act(evil_trout, p1, PostActionType.types[:like]) expect(topic_view.all_post_actions[p1.id][PostActionType.types[:like]]).to be_present end end @@ -200,14 +213,14 @@ describe TopicView do it 'returns the active flags' do PostAction.act(moderator, p1, PostActionType.types[:off_topic]) - PostAction.act(coding_horror, p1, PostActionType.types[:off_topic]) + PostAction.act(evil_trout, p1, PostActionType.types[:off_topic]) expect(topic_view.all_active_flags[p1.id][PostActionType.types[:off_topic]].count).to eq(2) end it 'returns only the active flags' do PostAction.act(moderator, p1, PostActionType.types[:off_topic]) - PostAction.act(coding_horror, p1, PostActionType.types[:off_topic]) + PostAction.act(evil_trout, p1, PostActionType.types[:off_topic]) PostAction.defer_flags!(p1, moderator) @@ -223,12 +236,12 @@ describe TopicView do # random user has nothing expect(topic_view.read?(1)).to eq(false) - coding_horror.created_at = 2.days.ago + evil_trout.created_at = 2.days.ago # a real user that just read it should have it marked - PostTiming.process_timings(coding_horror, topic.id, 1, [[1, 1000]]) - expect(TopicView.new(topic.id, coding_horror).read?(1)).to eq(true) - expect(TopicView.new(topic.id, coding_horror).topic_user).to be_present + PostTiming.process_timings(evil_trout, topic.id, 1, [[1, 1000]]) + expect(TopicView.new(topic.id, evil_trout).read?(1)).to eq(true) + expect(TopicView.new(topic.id, evil_trout).topic_user).to be_present end end @@ -262,11 +275,11 @@ describe TopicView do context 'whispers' do it "handles their visibility properly" do - p1 = Fabricate(:post, topic: topic, user: coding_horror) - p2 = Fabricate(:post, topic: topic, user: coding_horror, post_type: Post.types[:whisper]) - p3 = Fabricate(:post, topic: topic, user: coding_horror) + p1 = Fabricate(:post, topic: topic, user: evil_trout) + p2 = Fabricate(:post, topic: topic, user: evil_trout, post_type: Post.types[:whisper]) + p3 = Fabricate(:post, topic: topic, user: evil_trout) - ch_posts = TopicView.new(topic.id, coding_horror).posts + ch_posts = TopicView.new(topic.id, evil_trout).posts expect(ch_posts.map(&:id)).to eq([p1.id, p2.id, p3.id]) anon_posts = TopicView.new(topic.id).posts @@ -280,12 +293,12 @@ describe TopicView do context '.posts' do # Create the posts in a different order than the sort_order - let!(:p5) { Fabricate(:post, topic: topic, user: coding_horror) } - let!(:p2) { Fabricate(:post, topic: topic, user: coding_horror) } + let!(:p5) { Fabricate(:post, topic: topic, user: evil_trout) } + let!(:p2) { Fabricate(:post, topic: topic, user: evil_trout) } let!(:p6) { Fabricate(:post, topic: topic, user: Fabricate(:user), deleted_at: Time.now) } - let!(:p4) { Fabricate(:post, topic: topic, user: coding_horror, deleted_at: Time.now) } + let!(:p4) { Fabricate(:post, topic: topic, user: evil_trout, deleted_at: Time.now) } let!(:p1) { Fabricate(:post, topic: topic, user: first_poster) } - let!(:p7) { Fabricate(:post, topic: topic, user: coding_horror, deleted_at: Time.now) } + let!(:p7) { Fabricate(:post, topic: topic, user: evil_trout, deleted_at: Time.now) } let!(:p3) { Fabricate(:post, topic: topic, user: first_poster) } before do @@ -305,11 +318,11 @@ describe TopicView do # does not contain contains_gaps with default filtering expect(topic_view.contains_gaps?).to eq(false) # contains contains_gaps when filtered by username" do - expect(TopicView.new(topic.id, coding_horror, username_filters: ['eviltrout']).contains_gaps?).to eq(true) + expect(TopicView.new(topic.id, evil_trout, username_filters: ['eviltrout']).contains_gaps?).to eq(true) # contains contains_gaps when filtered by summary - expect(TopicView.new(topic.id, coding_horror, filter: 'summary').contains_gaps?).to eq(true) + expect(TopicView.new(topic.id, evil_trout, filter: 'summary').contains_gaps?).to eq(true) # contains contains_gaps when filtered by best - expect(TopicView.new(topic.id, coding_horror, best: 5).contains_gaps?).to eq(true) + expect(TopicView.new(topic.id, evil_trout, best: 5).contains_gaps?).to eq(true) end end @@ -324,10 +337,10 @@ describe TopicView do topic.save! expect { - TopicView.new(topic.id, coding_horror).posts.count + TopicView.new(topic.id, evil_trout).posts.count }.to raise_error(Discourse::InvalidAccess) - expect(TopicView.new(t2.id, coding_horror, post_ids: [p1.id, p2.id]).posts.count).to eq(0) + expect(TopicView.new(t2.id, evil_trout, post_ids: [p1.id, p2.id]).posts.count).to eq(0) end @@ -345,7 +358,7 @@ describe TopicView do describe "filter_posts_near" do def topic_view_near(post, show_deleted = false) - TopicView.new(topic.id, coding_horror, post_number: post.post_number, show_deleted: show_deleted) + TopicView.new(topic.id, evil_trout, post_number: post.post_number, show_deleted: show_deleted) end it "snaps to the lower boundary" do @@ -370,7 +383,7 @@ describe TopicView do end it "gaps deleted posts to an admin" do - coding_horror.admin = true + evil_trout.admin = true near_view = topic_view_near(p3) expect(near_view.desired_post).to eq(p3) expect(near_view.posts).to eq([p2, p3, p5]) @@ -379,7 +392,7 @@ describe TopicView do end it "returns deleted posts to an admin with show_deleted" do - coding_horror.admin = true + evil_trout.admin = true near_view = topic_view_near(p3, true) expect(near_view.desired_post).to eq(p3) expect(near_view.posts).to eq([p2, p3, p4]) @@ -387,7 +400,7 @@ describe TopicView do end it "gaps deleted posts by nuked users to an admin" do - coding_horror.admin = true + evil_trout.admin = true near_view = topic_view_near(p5) expect(near_view.desired_post).to eq(p5) # note: both p4 and p6 get skipped @@ -397,7 +410,7 @@ describe TopicView do end it "returns deleted posts by nuked users to an admin with show_deleted" do - coding_horror.admin = true + evil_trout.admin = true near_view = topic_view_near(p5, true) expect(near_view.desired_post).to eq(p5) expect(near_view.posts).to eq([p4, p5, p6]) @@ -414,7 +427,7 @@ describe TopicView do end it 'gaps deleted posts to admins' do - coding_horror.admin = true + evil_trout.admin = true near_view = topic_view_near(p5) expect(near_view.posts).to eq([p1, p2, p3, p5]) expect(near_view.gaps.before).to eq(p5.id => [p4.id]) @@ -422,7 +435,7 @@ describe TopicView do end it 'returns deleted posts to admins' do - coding_horror.admin = true + evil_trout.admin = true near_view = topic_view_near(p5, true) expect(near_view.posts).to eq([p1, p2, p3, p4, p5, p6, p7]) expect(near_view.contains_gaps?).to eq(false) @@ -435,7 +448,7 @@ describe TopicView do let(:tag1) { Fabricate(:tag) } let(:tag2) { Fabricate(:tag, topic_count: 2) } - subject { TopicView.new(topic.id, coding_horror).page_title } + subject { TopicView.new(topic.id, evil_trout).page_title } context "uncategorized topic" do context "topic_page_title_includes_category is false" do diff --git a/spec/services/staff_action_logger_spec.rb b/spec/services/staff_action_logger_spec.rb index b8159b28fda..7d8535fe3c5 100644 --- a/spec/services/staff_action_logger_spec.rb +++ b/spec/services/staff_action_logger_spec.rb @@ -434,4 +434,22 @@ describe StaffActionLogger do expect(user_history.previous_value).to eq('t') end end + + describe 'log_check_personal_message' do + let(:personal_message) { Fabricate(:private_message_topic) } + + subject(:log_check_personal_message) { described_class.new(admin).log_check_personal_message(personal_message) } + + it 'raises an error when topic is nil' do + expect { logger.log_check_personal_message(nil) }.to raise_error(Discourse::InvalidParameters) + end + + it 'raises an error when topic is not a Topic' do + expect { logger.log_check_personal_message(1) }.to raise_error(Discourse::InvalidParameters) + end + + it 'creates a new UserHistory record' do + expect { log_check_personal_message }.to change { UserHistory.count }.by(1) + end + end end