From 4aa0d88c6c1f5ab0aadd2438e4b81934f792b216 Mon Sep 17 00:00:00 2001 From: Sam Date: Thu, 4 Dec 2014 13:46:52 +1100 Subject: [PATCH] FEATURE: search private messages option --- .../discourse/controllers/search.js.es6 | 14 ++++- .../routes/build-user-topic-list-route.js.es6 | 7 ++- app/controllers/search_controller.rb | 13 +++-- config/locales/client.en.yml | 1 + lib/search.rb | 39 +++++++++++--- spec/components/search_spec.rb | 53 +++++++++++++++++++ 6 files changed, 114 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/search.js.es6 b/app/assets/javascripts/discourse/controllers/search.js.es6 index 24c327a012f..4efcd31dca5 100644 --- a/app/assets/javascripts/discourse/controllers/search.js.es6 +++ b/app/assets/javascripts/discourse/controllers/search.js.es6 @@ -4,13 +4,23 @@ var _dontSearch = false; export default Em.Controller.extend(Discourse.Presence, { + contextType: function(key, value){ + if(arguments.length > 1) { + // a bit hacky, consider cleaning this up, need to work through all observers though + var context = $.extend({}, this.get('searchContext')); + context.type = value; + this.set('searchContext', context); + } + return this.get('searchContext.type'); + }.property('searchContext'), + contextChanged: function(){ if (this.get('searchContextEnabled')) { _dontSearch = true; this.set('searchContextEnabled', false); _dontSearch = false; } - }.observes("searchContext"), + }.observes('searchContext'), searchContextDescription: function(){ var ctx = this.get('searchContext'); @@ -22,6 +32,8 @@ export default Em.Controller.extend(Discourse.Presence, { return I18n.t('search.context.user', {username: Em.get(ctx, 'user.username')}); case 'category': return I18n.t('search.context.category', {category: Em.get(ctx, 'category.name')}); + case 'private_messages': + return I18n.t('search.context.private_messages'); } } }.property('searchContext'), diff --git a/app/assets/javascripts/discourse/routes/build-user-topic-list-route.js.es6 b/app/assets/javascripts/discourse/routes/build-user-topic-list-route.js.es6 index a65533f9e02..ae1ff7ba3bc 100644 --- a/app/assets/javascripts/discourse/routes/build-user-topic-list-route.js.es6 +++ b/app/assets/javascripts/discourse/routes/build-user-topic-list-route.js.es6 @@ -25,7 +25,12 @@ export default function (viewName, path) { showParticipants: true }); - this.controllerFor('user').set("pmView", viewName); + this.controllerFor('user').set('pmView', viewName); + this.controllerFor('search').set('contextType', 'private_messages'); + }, + + deactivate: function(){ + this.controllerFor('search').set('contextType', 'user'); } }); } diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 9103e4ebf62..f78087a4f95 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -3,7 +3,7 @@ require_dependency 'search' class SearchController < ApplicationController def self.valid_context_types - %w{user topic category} + %w{user topic category private_messages} end def query @@ -21,16 +21,19 @@ class SearchController < ApplicationController raise Discourse::InvalidParameters.new(:search_context) unless SearchController.valid_context_types.include?(search_context[:type]) raise Discourse::InvalidParameters.new(:search_context) if search_context[:id].blank? - klass = search_context[:type].classify.constantize - # A user is found by username context_obj = nil - if search_context[:type] == 'user' - context_obj = klass.find_by(username_lower: params[:search_context][:id].downcase) + if ['user','private_messages'].include? search_context[:type] + context_obj = User.find_by(username_lower: params[:search_context][:id].downcase) else + klass = search_context[:type].classify.constantize context_obj = klass.find_by(id: params[:search_context][:id]) end + if search_context[:type] == 'private_messages' + search_args[:type_filter] = 'private_messages' + end + guardian.ensure_can_see!(context_obj) search_args[:search_context] = context_obj end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 450b54858b5..069dbee3acc 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -769,6 +769,7 @@ en: user: "Search posts by @{{username}}" category: "Search the \"{{category}}\" category" topic: "Search this topic" + private_messages: "Search private messages" site_map: "go to another topic list or category" go_back: 'go back' diff --git a/lib/search.rb b/lib/search.rb index 2330ddc6944..6bb40781217 100644 --- a/lib/search.rb +++ b/lib/search.rb @@ -17,7 +17,7 @@ class Search end def self.facets - %w(topic category user) + %w(topic category user private_messages) end def self.long_locale @@ -277,10 +277,20 @@ class Search def posts_query(limit, opts=nil) opts ||= {} posts = Post - .joins(:post_search_data, {:topic => :category}) + .joins(:post_search_data, :topic) + .joins("LEFT JOIN categories ON categories.id = topics.category_id") .where("topics.deleted_at" => nil) .where("topics.visible") - .where("topics.archetype <> ?", Archetype.private_message) + + if opts[:private_messages] + posts = posts.where("topics.archetype = ?", Archetype.private_message) + + unless @guardian.is_admin? + posts = posts.where("topics.id IN (SELECT topic_id FROM topic_allowed_users WHERE user_id = ?)", @guardian.user.id) + end + else + posts = posts.where("topics.archetype <> ?", Archetype.private_message) + end if @search_context.present? && @search_context.is_a?(Topic) posts = posts.joins('JOIN users u ON u.id = posts.user_id') @@ -336,7 +346,13 @@ class Search if @search_context.present? if @search_context.is_a?(User) - posts = posts.where("posts.user_id = #{@search_context.id}") + + if opts[:private_messages] + posts = posts.where("topics.id IN (SELECT topic_id FROM topic_allowed_users WHERE user_id = ?)", @search_context.id) + else + posts = posts.where("posts.user_id = #{@search_context.id}") + end + elsif @search_context.is_a?(Category) posts = posts.where("topics.category_id = #{@search_context.id}") elsif @search_context.is_a?(Topic) @@ -407,9 +423,10 @@ class Search end end - def aggregate_search + def aggregate_search(opts = {}) - post_sql = posts_query(@limit, aggregate_search: true) + post_sql = posts_query(@limit, aggregate_search: true, + private_messages: opts[:private_messages]) .select('topics.id', 'min(post_number) post_number') .group('topics.id') .to_sql @@ -417,6 +434,10 @@ class Search # double wrapping so we get correct row numbers post_sql = "SELECT *, row_number() over() row_number FROM (#{post_sql}) xxx" + # p Topic.exec_sql(post_sql).to_a + # puts post_sql + # p Topic.exec_sql("SELECT topic_id FROM topic_allowed_users WHERE user_id = 2").to_a + posts = Post.includes(:topic => :category) .joins("JOIN (#{post_sql}) x ON x.id = posts.topic_id AND x.post_number = posts.post_number") .order('row_number') @@ -426,6 +447,12 @@ class Search end end + def private_messages_search + raise Discourse::InvalidAccess.new("anonymous can not search PMs") unless @guardian.user + + aggregate_search(private_messages: true) + end + def topic_search if @search_context.is_a?(Topic) posts = posts_query(@limit).where('posts.topic_id = ?', @search_context.id).includes(:topic => :category) diff --git a/spec/components/search_spec.rb b/spec/components/search_spec.rb index a80d6483150..1c62078d566 100644 --- a/spec/components/search_spec.rb +++ b/spec/components/search_spec.rb @@ -96,6 +96,59 @@ describe Search do end end + context 'private messages' do + + let(:topic) { + Fabricate(:topic, + category_id: nil, + archetype: 'private_message') + } + + let(:post) { Fabricate(:post, topic: topic) } + let(:reply) { Fabricate(:post, topic: topic, + raw: 'hello from mars, we just landed') } + + + + it 'searches correctly' do + + expect do + Search.execute('mars', type_filter: 'private_messages') + end.to raise_error(Discourse::InvalidAccess) + + TopicAllowedUser.create!(user_id: reply.user_id, topic_id: topic.id) + TopicAllowedUser.create!(user_id: post.user_id, topic_id: topic.id) + + + results = Search.execute('mars', + type_filter: 'private_messages', + guardian: Guardian.new(reply.user)) + + results.posts.length.should == 1 + + # does not leak out + results = Search.execute('mars', + type_filter: 'private_messages', + guardian: Guardian.new(Fabricate(:user))) + + results.posts.length.should == 0 + + Fabricate(:topic, category_id: nil, archetype: 'private_message') + Fabricate(:post, topic: topic, raw: 'another secret pm from mars, testing') + + + # admin can search everything with correct context + results = Search.execute('mars', + type_filter: 'private_messages', + search_context: post.user, + guardian: Guardian.new(Fabricate(:admin))) + + results.posts.length.should == 1 + + end + + end + context 'topics' do let(:post) { Fabricate(:post) } let(:topic) { post.topic}