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.

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')}
+                    ]
 });

app/assets/javascripts/admin/routes/admin-logs-search-logs.js.es6

@@ -6,20 +6,19 @@ export default Discourse.Route.extend({
   },
 
   queryParams: {
-    period: {
+    period: { refreshModel: true },
-      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 });
   }
 });

app/assets/javascripts/admin/templates/logs/search-logs.hbs

@@ -1,5 +1,6 @@
 <p>
   {{period-chooser period=period}}
+  {{combo-box content=searchTypeOptions value=searchType class='search-logs-filter'}}
 </p>
 <br>
 
@@ -11,7 +12,6 @@
         <div class="col heading term">{{i18n 'admin.logs.search_logs.term'}}</div>
         <div class="col heading">{{i18n 'admin.logs.search_logs.searches'}}</div>
         <div class="col heading">{{i18n 'admin.logs.search_logs.click_through'}}</div>
-        <div class="col heading topic">{{i18n 'admin.logs.search_logs.most_viewed_topic'}}</div>
         <div class="col heading" title="{{i18n 'admin.logs.search_logs.unique_title'}}">{{i18n 'admin.logs.search_logs.unique'}}</div>
       </div>
 
@@ -20,11 +20,6 @@
           <div class="col term">{{item.term}}</div>
           <div class="col">{{item.searches}}</div>
           <div class="col">{{item.click_through}}</div>
-          <div class="col topic">
-            {{#if item.clicked_topic_id}}
-              <a href='{{unbound item.topic_url}}'>{{item.topic_title}}</a>
-            {{/if}}
-          </div>
           <div class="col">{{item.unique}}</div>
         </div>
       {{/each}}

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),

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 {

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

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
 

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)
+  def self.trending(period = :all, search_type = :all)
-    SearchLog.select("term,
+    result = SearchLog.select("term,
-                       COUNT(*) AS searches,
+       COUNT(*) AS searches,
-                       SUM(CASE
+       SUM(CASE
-                               WHEN clicked_topic_id IS NOT NULL THEN 1
+               WHEN search_result_id IS NOT NULL THEN 1
-                               ELSE 0
+               ELSE 0
-                           END) AS click_through,
+           END) AS click_through,
-                       MODE() WITHIN GROUP (ORDER BY clicked_topic_id) AS clicked_topic_id,
+       COUNT(DISTINCT ip_address) AS unique")
-                       COUNT(DISTINCT ip_address) AS unique")
-      .includes(:topic)
       .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
 

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

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"
 

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

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

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)
+      SearchLog.where(term: 'ruby', ip_address: '127.0.0.1').update_all(search_result_id: 12)
-      not_so_popular_topic = Fabricate(:topic)
+      SearchLog.where(term: 'ruby', ip_address: '127.0.0.2').update_all(search_result_id: 24)
-      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)
       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
 

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}
       ]);
     });