Merge pull request #5385 from techAPJ/search-logs-improvements

FEATURE: support search click through tracking for user, category and tags

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: {
-      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 });
   }
 });

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

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

@@ -3219,9 +3219,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)
-      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
 

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