From e3925278e2907b1979696d6318a80fcc3b96ed1c Mon Sep 17 00:00:00 2001 From: Arpit Jalan Date: Tue, 28 Nov 2017 23:24:27 +0530 Subject: [PATCH] FEATURE: support search click through tracking for user, category and tags https://meta.discourse.org/t/search-logs-page/73281/11?u=techapj This commit adds following features: - support for tracking click through to user, tag and category - new filter for search type (header, full page) This commit also removes "most viewed topic" field from search logs page because we are now tracking multiple click through entities, so topic is not a special entity anymore. This also improves query perf. The query now takes `20.5ms` to runs, as opposed to `655.9ms` previously. --- .../controllers/admin-logs-search-logs.js.es6 | 9 ++- .../routes/admin-logs-search-logs.js.es6 | 9 ++- .../admin/templates/logs/search-logs.hbs | 7 +-- .../widgets/search-menu-results.js.es6 | 4 ++ .../stylesheets/common/admin/admin_base.scss | 16 +++--- .../admin/search_logs_controller.rb | 3 +- app/controllers/search_controller.rb | 12 +++- app/models/search_log.rb | 34 +++++++----- app/serializers/search_logs_serializer.rb | 11 ---- config/locales/client.en.yml | 5 +- ...me_clicked_topic_id_to_search_result_id.rb | 13 +++++ spec/controllers/search_controller_spec.rb | 55 +++++++++++++++++-- spec/models/search_log_spec.rb | 8 +-- .../helpers/create-pretender.js.es6 | 2 +- 14 files changed, 126 insertions(+), 62 deletions(-) create mode 100644 db/migrate/20171128172835_rename_clicked_topic_id_to_search_result_id.rb diff --git a/app/assets/javascripts/admin/controllers/admin-logs-search-logs.js.es6 b/app/assets/javascripts/admin/controllers/admin-logs-search-logs.js.es6 index 1de32762a6a..83436849fe7 100644 --- a/app/assets/javascripts/admin/controllers/admin-logs-search-logs.js.es6 +++ b/app/assets/javascripts/admin/controllers/admin-logs-search-logs.js.es6 @@ -1,4 +1,11 @@ export default Ember.Controller.extend({ loading: false, - period: "all" + period: "all", + searchType: "all", + + searchTypeOptions: [ + {id: 'all', name: I18n.t('admin.logs.search_logs.types.all_search_types')}, + {id: 'header', name: I18n.t('admin.logs.search_logs.types.header')}, + {id: 'full_page', name: I18n.t('admin.logs.search_logs.types.full_page')} + ] }); diff --git a/app/assets/javascripts/admin/routes/admin-logs-search-logs.js.es6 b/app/assets/javascripts/admin/routes/admin-logs-search-logs.js.es6 index 013bdb4655a..41094b54fa5 100644 --- a/app/assets/javascripts/admin/routes/admin-logs-search-logs.js.es6 +++ b/app/assets/javascripts/admin/routes/admin-logs-search-logs.js.es6 @@ -6,20 +6,19 @@ export default Discourse.Route.extend({ }, queryParams: { - period: { - refreshModel: true - } + period: { refreshModel: true }, + searchType: { refreshModel: true } }, model(params) { this._params = params; - return ajax('/admin/logs/search_logs.json', { data: { period: params.period } }).then(search_logs => { + return ajax('/admin/logs/search_logs.json', { data: { period: params.period, search_type: params.searchType } }).then(search_logs => { return search_logs.map(sl => Ember.Object.create(sl)); }); }, setupController(controller, model) { const params = this._params; - controller.setProperties({ model, period: params.period }); + controller.setProperties({ model, period: params.period, searchType: params.searchType }); } }); diff --git a/app/assets/javascripts/admin/templates/logs/search-logs.hbs b/app/assets/javascripts/admin/templates/logs/search-logs.hbs index 5b606de9d6d..bfd87144da7 100644 --- a/app/assets/javascripts/admin/templates/logs/search-logs.hbs +++ b/app/assets/javascripts/admin/templates/logs/search-logs.hbs @@ -1,5 +1,6 @@

{{period-chooser period=period}} + {{combo-box content=searchTypeOptions value=searchType class='search-logs-filter'}}


@@ -11,7 +12,6 @@
{{i18n 'admin.logs.search_logs.term'}}
{{i18n 'admin.logs.search_logs.searches'}}
{{i18n 'admin.logs.search_logs.click_through'}}
-
{{i18n 'admin.logs.search_logs.most_viewed_topic'}}
{{i18n 'admin.logs.search_logs.unique'}}
@@ -20,11 +20,6 @@
{{item.term}}
{{item.searches}}
{{item.click_through}}
-
- {{#if item.clicked_topic_id}} - {{item.topic_title}} - {{/if}} -
{{item.unique}}
{{/each}} diff --git a/app/assets/javascripts/discourse/widgets/search-menu-results.js.es6 b/app/assets/javascripts/discourse/widgets/search-menu-results.js.es6 index 705f9e685b0..f00fcf8277f 100644 --- a/app/assets/javascripts/discourse/widgets/search-menu-results.js.es6 +++ b/app/assets/javascripts/discourse/widgets/search-menu-results.js.es6 @@ -24,9 +24,13 @@ function createSearchResult({ type, linkField, builder }) { return attrs.results.map(r => { let searchResultId; + if (type === "topic") { searchResultId = r.get('topic_id'); + } else { + searchResultId = r.get('id'); } + return h('li', this.attach('link', { href: r.get(linkField), contents: () => builder.call(this, r, attrs.term), diff --git a/app/assets/stylesheets/common/admin/admin_base.scss b/app/assets/stylesheets/common/admin/admin_base.scss index 9926a73a996..513a076947d 100644 --- a/app/assets/stylesheets/common/admin/admin_base.scss +++ b/app/assets/stylesheets/common/admin/admin_base.scss @@ -227,6 +227,11 @@ $mobile-breakpoint: 700px; .select-box-kit.dropdown-select-box { width: auto; } + + .search-logs-filter { + width: 200px; + float: right; + } } .admin-container .controls { @@ -1267,20 +1272,13 @@ table.api-keys { .search-logs-list{ .col { text-align: center; - width: 10%; + width: 15%; } .col.term { - width: 30%; + width: 45%; text-align: left; } - - .col.topic { - width: 35%; - text-align: left; - text-overflow: ellipsis; - white-space: nowrap; - } } .log-details-modal { diff --git a/app/controllers/admin/search_logs_controller.rb b/app/controllers/admin/search_logs_controller.rb index a7981eb7383..38b45ce512e 100644 --- a/app/controllers/admin/search_logs_controller.rb +++ b/app/controllers/admin/search_logs_controller.rb @@ -2,7 +2,8 @@ class Admin::SearchLogsController < Admin::AdminController def index period = params[:period] || "all" - render_serialized(SearchLog.trending(period.to_sym), SearchLogsSerializer) + search_type = params[:search_type] || "all" + render_serialized(SearchLog.trending(period&.to_sym, search_type&.to_sym), SearchLogsSerializer) end end diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index b0d44da59dc..ad74f595745 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -77,7 +77,8 @@ class SearchController < ApplicationController params.require(:search_result_type) params.require(:search_result_id) - if params[:search_result_type] == 'topic' + search_result_type = params[:search_result_type].downcase.to_sym + if SearchLog.search_result_types.has_key?(search_result_type) attributes = { id: params[:search_log_id] } if current_user.present? attributes[:user_id] = current_user.id @@ -85,8 +86,15 @@ class SearchController < ApplicationController attributes[:ip_address] = request.remote_ip end + if search_result_type == :tag + search_result_id = Tag.find_by_name(params[:search_result_id])&.id + else + search_result_id = params[:search_result_id] + end + SearchLog.where(attributes).update_all( - clicked_topic_id: params[:search_result_id] + search_result_type: SearchLog.search_result_types[search_result_type], + search_result_id: search_result_id ) end diff --git a/app/models/search_log.rb b/app/models/search_log.rb index 8308a244437..4ab7672ca41 100644 --- a/app/models/search_log.rb +++ b/app/models/search_log.rb @@ -1,7 +1,6 @@ require_dependency 'enum' class SearchLog < ActiveRecord::Base - belongs_to :topic, foreign_key: :clicked_topic_id validates_presence_of :term, :ip_address def self.search_types @@ -11,6 +10,15 @@ class SearchLog < ActiveRecord::Base ) end + def self.search_result_types + @search_result_types ||= Enum.new( + topic: 1, + user: 2, + category: 3, + tag: 4 + ) + end + def self.log(term:, search_type:, ip_address:, user_id: nil) search_type = search_types[search_type] @@ -49,19 +57,19 @@ class SearchLog < ActiveRecord::Base end end - def self.trending(period = :all) - SearchLog.select("term, - COUNT(*) AS searches, - SUM(CASE - WHEN clicked_topic_id IS NOT NULL THEN 1 - ELSE 0 - END) AS click_through, - MODE() WITHIN GROUP (ORDER BY clicked_topic_id) AS clicked_topic_id, - COUNT(DISTINCT ip_address) AS unique") - .includes(:topic) + def self.trending(period = :all, search_type = :all) + result = SearchLog.select("term, + COUNT(*) AS searches, + SUM(CASE + WHEN search_result_id IS NOT NULL THEN 1 + ELSE 0 + END) AS click_through, + COUNT(DISTINCT ip_address) AS unique") .where('created_at > ?', start_of(period)) - .group(:term) - .order('COUNT(DISTINCT ip_address) DESC') + + result = result.where('search_type = ?', search_types[search_type]) unless search_type == :all + result = result.group(:term) + .order('COUNT(DISTINCT ip_address) DESC, COUNT(*) DESC') .limit(100).to_a end diff --git a/app/serializers/search_logs_serializer.rb b/app/serializers/search_logs_serializer.rb index 434ef5dec9c..f059c33cb61 100644 --- a/app/serializers/search_logs_serializer.rb +++ b/app/serializers/search_logs_serializer.rb @@ -2,16 +2,5 @@ class SearchLogsSerializer < ApplicationSerializer attributes :term, :searches, :click_through, - :clicked_topic_id, - :topic_title, - :topic_url, :unique - - def topic_title - object&.topic&.title - end - - def topic_url - object&.topic&.url - end end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 0721af6522c..e491bc1f762 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -3209,9 +3209,12 @@ en: term: "Term" searches: "Searches" click_through: "Click Through" - most_viewed_topic: "Most Viewed Topic" unique: "Unique" unique_title: "unique users performing the search" + types: + all_search_types: "All search types" + header: "Header" + full_page: "Full Page" logster: title: "Error Logs" diff --git a/db/migrate/20171128172835_rename_clicked_topic_id_to_search_result_id.rb b/db/migrate/20171128172835_rename_clicked_topic_id_to_search_result_id.rb new file mode 100644 index 00000000000..998dc7b95e5 --- /dev/null +++ b/db/migrate/20171128172835_rename_clicked_topic_id_to_search_result_id.rb @@ -0,0 +1,13 @@ +class RenameClickedTopicIdToSearchResultId < ActiveRecord::Migration[5.1] + def up + rename_column :search_logs, :clicked_topic_id, :search_result_id + add_column :search_logs, :search_result_type, :integer, null: true + + execute "UPDATE search_logs SET search_result_type = 1 WHERE search_result_id is NOT NULL" + end + + def down + rename_column :search_logs, :search_result_id, :clicked_topic_id + remove_column :search_logs, :search_result_type + end +end diff --git a/spec/controllers/search_controller_spec.rb b/spec/controllers/search_controller_spec.rb index 1cc46050392..d6cec92d0b3 100644 --- a/spec/controllers/search_controller_spec.rb +++ b/spec/controllers/search_controller_spec.rb @@ -187,14 +187,14 @@ describe SearchController do } expect(response).to be_success - expect(SearchLog.find(search_log_id).clicked_topic_id).to be_blank + expect(SearchLog.find(search_log_id).search_result_id).to be_blank end it "records the click for a logged in user" do user = log_in(:user) _, search_log_id = SearchLog.log( - term: 'kitty', + term: 'foobar', search_type: :header, user_id: user.id, ip_address: '127.0.0.1' @@ -203,11 +203,12 @@ describe SearchController do post :click, params: { search_log_id: search_log_id, search_result_id: 12345, - search_result_type: 'topic' + search_result_type: 'user' }, format: :json expect(response).to be_success - expect(SearchLog.find(search_log_id).clicked_topic_id).to eq(12345) + expect(SearchLog.find(search_log_id).search_result_id).to eq(12345) + expect(SearchLog.find(search_log_id).search_result_type).to eq(SearchLog.search_result_types[:user]) end it "records the click for an anonymous user" do @@ -226,7 +227,8 @@ describe SearchController do }, format: :json expect(response).to be_success - expect(SearchLog.find(search_log_id).clicked_topic_id).to eq(22222) + expect(SearchLog.find(search_log_id).search_result_id).to eq(22222) + expect(SearchLog.find(search_log_id).search_result_type).to eq(SearchLog.search_result_types[:topic]) end it "doesn't record the click for a different IP" do @@ -245,7 +247,48 @@ describe SearchController do } expect(response).to be_success - expect(SearchLog.find(search_log_id).clicked_topic_id).to be_blank + expect(SearchLog.find(search_log_id).search_result_id).to be_blank + end + + it "records the click for search result type category" do + request.remote_addr = '192.168.0.1'; + + _, search_log_id = SearchLog.log( + term: 'dev', + search_type: :header, + ip_address: '192.168.0.1' + ) + + post :click, params: { + search_log_id: search_log_id, + search_result_id: 23456, + search_result_type: 'category' + }, format: :json + + expect(response).to be_success + expect(SearchLog.find(search_log_id).search_result_id).to eq(23456) + expect(SearchLog.find(search_log_id).search_result_type).to eq(SearchLog.search_result_types[:category]) + end + + it "records the click for search result type tag" do + request.remote_addr = '192.168.0.1'; + tag = Fabricate(:tag, name: 'test') + + _, search_log_id = SearchLog.log( + term: 'test', + search_type: :header, + ip_address: '192.168.0.1' + ) + + post :click, params: { + search_log_id: search_log_id, + search_result_id: tag.name, + search_result_type: 'tag' + }, format: :json + + expect(response).to be_success + expect(SearchLog.find(search_log_id).search_result_id).to eq(tag.id) + expect(SearchLog.find(search_log_id).search_result_type).to eq(SearchLog.search_result_types[:tag]) end end end diff --git a/spec/models/search_log_spec.rb b/spec/models/search_log_spec.rb index 3bc2f39106b..509d9517aaf 100644 --- a/spec/models/search_log_spec.rb +++ b/spec/models/search_log_spec.rb @@ -179,15 +179,11 @@ RSpec.describe SearchLog, type: :model do expect(top_trending.searches).to eq(3) expect(top_trending.unique).to eq(2) expect(top_trending.click_through).to eq(0) - expect(top_trending.clicked_topic_id).to eq(nil) - popular_topic = Fabricate(:topic) - not_so_popular_topic = Fabricate(:topic) - SearchLog.where(term: 'ruby', ip_address: '127.0.0.1').update_all(clicked_topic_id: popular_topic.id) - SearchLog.where(term: 'ruby', ip_address: '127.0.0.2').update_all(clicked_topic_id: not_so_popular_topic.id) + SearchLog.where(term: 'ruby', ip_address: '127.0.0.1').update_all(search_result_id: 12) + SearchLog.where(term: 'ruby', ip_address: '127.0.0.2').update_all(search_result_id: 24) top_trending = SearchLog.trending.first expect(top_trending.click_through).to eq(3) - expect(top_trending.clicked_topic_id).to eq(popular_topic.id) end end diff --git a/test/javascripts/helpers/create-pretender.js.es6 b/test/javascripts/helpers/create-pretender.js.es6 index 81539e88dfb..56c119ad85c 100644 --- a/test/javascripts/helpers/create-pretender.js.es6 +++ b/test/javascripts/helpers/create-pretender.js.es6 @@ -393,7 +393,7 @@ export default function() { this.get('/admin/logs/search_logs.json', () => { return response(200, [ - {"term":"foobar","searches":35,"click_through":6,"clicked_topic_id":1550,"topic_title":"Foo Bar Topic Title","topic_url":"http://discourse.example.com/t/foo-bar-topic-title/1550","unique":16} + {"term":"foobar","searches":35,"click_through":6,"unique":16} ]); });