Implemented issue #2073 - Allow sort_by and order_by for tickets, users and organisations search REST API.

app/controllers/organizations_controller.rb

@@ -246,6 +246,8 @@ curl http://localhost/api/v1/organization/{id} -v -u #{login}:#{password} -H "Co
       query: query,
       limit: per_page,
       offset: offset,
+      sort_by: params[:sort_by],
+      order_by: params[:order_by],
       current_user: current_user,
     }
     if params[:role_ids].present?

app/controllers/tickets_controller.rb

@@ -445,6 +445,8 @@ class TicketsController < ApplicationController
       condition: params[:condition].to_h,
       limit: per_page,
       offset: offset,
+      order_by: params[:order_by],
+      sort_by: params[:sort_by],
       current_user: current_user,
     )
 

app/controllers/users_controller.rb

@@ -405,6 +405,8 @@ class UsersController < ApplicationController
       query: query,
       limit: per_page,
       offset: offset,
+      sort_by: params[:sort_by],
+      order_by: params[:order_by],
       current_user: current_user,
     }
     %i[role_ids permissions].each do |key|

app/models/concerns/has_search_sortable.rb

@@ -0,0 +1,157 @@
+# Copyright (C) 2012-2016 Zammad Foundation, http://zammad-foundation.org/
+module HasSearchSortable
+  extend ActiveSupport::Concern
+
+  # methods defined here are going to extend the class, not the instance of it
+  class_methods do
+
+=begin
+
+This function will check the params for the "sort_by" attribute
+and validate its values.
+
+  sort_by = search_get_sort_by(params, default)
+
+returns
+
+  sort_by = [
+    'created_at',
+    'updated_at',
+  ]
+
+=end
+
+    def search_get_sort_by(params, default)
+      sort_by = []
+      if params[:sort_by].present? && params[:sort_by].is_a?(String)
+        params[:sort_by] = [params[:sort_by]]
+      elsif params[:sort_by].blank?
+        params[:sort_by] = []
+      end
+
+      # check order
+      params[:sort_by].each do |value|
+
+        # only accept values which are set for the db schema
+        raise "Found invalid column '#{value}' for sorting." if columns_hash[value].blank?
+
+        sort_by.push(value)
+      end
+
+      if sort_by.blank?
+        sort_by.push(default)
+      end
+
+      sort_by
+    end
+
+=begin
+
+This function will check the params for the "order_by" attribute
+and validate its values.
+
+  order_by = search_get_order_by(params, default)
+
+returns
+
+  order_by = [
+    'asc',
+    'desc',
+  ]
+
+=end
+
+    def search_get_order_by(params, default)
+      order_by = []
+      if params[:order_by].present? && params[:order_by].is_a?(String)
+        params[:order_by] = [ params[:order_by] ]
+      elsif params[:order_by].blank?
+        params[:order_by] = []
+      end
+
+      # check order
+      params[:order_by].each do |value|
+        raise "Found invalid order by value #{value}. Please use 'asc' or 'desc'." if value !~ /\A(asc|desc)\z/i
+        order_by.push(value.downcase)
+      end
+
+      if order_by.blank?
+        order_by.push(default)
+      end
+
+      order_by
+    end
+
+=begin
+
+This function will use the evaluated values for sort_by and
+order_by to generate the ORDER-SELECT sql statement for the sorting
+of the result.
+
+  sort_by  = [ 'created_at', 'updated_at' ]
+  order_by = [ 'asc', 'desc' ]
+  default  = 'tickets.created_at'
+
+  sql = search_get_order_select_sql(sort_by, order_by, default)
+
+returns
+
+  sql = 'tickets.created_at, tickets.updated_at'
+
+=end
+
+    def search_get_order_select_sql(sort_by, order_by, default)
+      sql = []
+
+      sort_by.each_with_index do |value, index|
+        next if value.blank?
+        next if order_by[index].blank?
+
+        sql.push( "#{ActiveRecord::Base.connection.quote_table_name(table_name)}.#{ActiveRecord::Base.connection.quote_column_name(value)}" )
+      end
+
+      if sql.blank?
+        sql.push("#{ActiveRecord::Base.connection.quote_table_name(table_name)}.#{ActiveRecord::Base.connection.quote_column_name(default)}")
+      end
+
+      sql.join(', ')
+    end
+
+=begin
+
+This function will use the evaluated values for sort_by and
+order_by to generate the ORDER- sql statement for the sorting
+of the result.
+
+  sort_by  = [ 'created_at', 'updated_at' ]
+  order_by = [ 'asc', 'desc' ]
+  default  = 'tickets.created_at DESC'
+
+  sql = search_get_order_sql(sort_by, order_by, default)
+
+returns
+
+  sql = 'tickets.created_at ASC, tickets.updated_at DESC'
+
+=end
+
+    def search_get_order_sql(sort_by, order_by, default)
+      sql = []
+
+      sort_by.each_with_index do |value, index|
+        next if value.blank?
+        next if order_by[index].blank?
+
+        sql.push( "#{ActiveRecord::Base.connection.quote_table_name(table_name)}.#{ActiveRecord::Base.connection.quote_column_name(value)} #{order_by[index]}" )
+      end
+
+      if sql.blank?
+        sql.push("#{ActiveRecord::Base.connection.quote_table_name(table_name)}.#{ActiveRecord::Base.connection.quote_column_name(default)}")
+      end
+
+      sql.join(', ')
+    end
+
+  end
+
+end

app/models/organization/search.rb

@@ -4,6 +4,10 @@ class Organization
   module Search
     extend ActiveSupport::Concern
 
+    included do
+      include HasSearchSortable
+    end
+
     # methods defined here are going to extend the class, not the instance of it
     class_methods do
 
@@ -43,6 +47,14 @@ search organizations
     query: 'search something',
     limit: 15,
     offset: 100,
+
+    # sort single column
+    sort_by: 'created_at',
+    order_by: 'asc',
+
+    # sort multiple columns
+    sort_by: [ 'created_at', 'updated_at' ],
+    order_by: [ 'asc', 'desc' ],
   )
 
 returns
@@ -59,12 +71,18 @@ returns
         offset = params[:offset] || 0
         current_user = params[:current_user]
 
+        # check sort
+        sort_by = search_get_sort_by(params, 'name')
+
+        # check order
+        order_by = search_get_order_by(params, 'asc')
+
         # enable search only for agents and admins
         return [] if !search_preferences(current_user)
 
         # try search index backend
         if SearchIndexBackend.enabled?
-          items = SearchIndexBackend.search(query, limit, 'Organization', {}, offset)
+          items = SearchIndexBackend.search(query, limit, 'Organization', {}, offset, sort_by, order_by)
           organizations = []
           items.each do |item|
             organization = Organization.lookup(id: item[:id])
@@ -74,11 +92,14 @@ returns
           return organizations
         end
 
+        order_select_sql = search_get_order_select_sql(sort_by, order_by, 'organizations.name')
+        order_sql        = search_get_order_sql(sort_by, order_by, 'organizations.name ASC')
+
         # fallback do sql query
         # - stip out * we already search for *query* -
         query.delete! '*'
         organizations = Organization.where_or_cis(%i[name note], "%#{query}%")
-                                    .order('name')
+                                    .order(order_sql)
                                     .offset(offset)
                                     .limit(limit)
                                     .to_a
@@ -89,10 +110,10 @@ returns
         return organizations if organizations.length > 3
 
         # if only a few organizations are found, search for names of users
-        organizations_by_user = Organization.select('DISTINCT(organizations.id), organizations.name')
+        organizations_by_user = Organization.select('DISTINCT(organizations.id), ' + order_select_sql)
                                             .joins('LEFT OUTER JOIN users ON users.organization_id = organizations.id')
                                             .where(User.or_cis(%i[firstname lastname email], "%#{query}%"))
-                                            .order('organizations.name')
+                                            .order(order_sql)
                                             .limit(limit)
 
         organizations_by_user.each do |organization_by_user|

app/models/ticket/search.rb

@@ -2,6 +2,10 @@
 module Ticket::Search
   extend ActiveSupport::Concern
 
+  included do
+    include HasSearchSortable
+  end
+
   # methods defined here are going to extend the class, not the instance of it
   class_methods do
 
@@ -84,6 +88,15 @@ search tickets via database
     },
     limit: 15,
     offset: 100,
+
+    # sort single column
+    sort_by: 'created_at',
+    order_by: 'asc',
+
+    # sort multiple columns
+    sort_by: [ 'created_at', 'updated_at' ],
+    order_by: [ 'asc', 'desc' ],
+
     full: false,
   )
 
@@ -106,6 +119,12 @@ returns
         full = true
       end
 
+      # check sort
+      sort_by = search_get_sort_by(params, 'created_at')
+
+      # check order
+      order_by = search_get_order_by(params, 'desc')
+
       # try search index backend
       if condition.blank? && SearchIndexBackend.enabled?
         query_extention = {}
@@ -135,7 +154,7 @@ returns
 
         query_extention['bool']['must'].push access_condition
 
-        items = SearchIndexBackend.search(query, limit, 'Ticket', query_extention, offset)
+        items = SearchIndexBackend.search(query, limit, 'Ticket', query_extention, offset, sort_by, order_by)
         if !full
           ids = []
           items.each do |item|
@@ -157,22 +176,25 @@ returns
 
       # do query
       # - stip out * we already search for *query* -
+
+      order_select_sql = search_get_order_select_sql(sort_by, order_by, 'tickets.created_at')
+      order_sql        = search_get_order_sql(sort_by, order_by, 'tickets.created_at DESC')
       if query
         query.delete! '*'
-        tickets_all = Ticket.select('DISTINCT(tickets.id), tickets.created_at')
+        tickets_all = Ticket.select('DISTINCT(tickets.id), ' + order_select_sql)
                             .where(access_condition)
                             .where('(tickets.title LIKE ? OR tickets.number LIKE ? OR ticket_articles.body LIKE ? OR ticket_articles.from LIKE ? OR ticket_articles.to LIKE ? OR ticket_articles.subject LIKE ?)', "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%" )
                             .joins(:articles)
-                            .order('tickets.created_at DESC')
+                            .order(order_sql)
                             .offset(offset)
                             .limit(limit)
       else
         query_condition, bind_condition, tables = selector2sql(condition)
-        tickets_all = Ticket.select('DISTINCT(tickets.id), tickets.created_at')
+        tickets_all = Ticket.select('DISTINCT(tickets.id), ' + order_select_sql)
                             .joins(tables)
                             .where(access_condition)
                             .where(query_condition, *bind_condition)
-                            .order('tickets.created_at DESC')
+                            .order(order_sql)
                             .offset(offset)
                             .limit(limit)
       end
@@ -193,4 +215,5 @@ returns
       tickets
     end
   end
+
 end

app/models/user/search.rb

@@ -4,6 +4,10 @@ class User
   module Search
     extend ActiveSupport::Concern
 
+    included do
+      include HasSearchSortable
+    end
+
     # methods defined here are going to extend the class, not the instance of it
     class_methods do
 
@@ -54,6 +58,14 @@ or with certain role_ids | permissions
     current_user: user_model,
     role_ids: [1,2,3],
     permissions: ['ticket.agent']
+
+    # sort single column
+    sort_by: 'created_at',
+    order_by: 'asc',
+
+    # sort multiple columns
+    sort_by: [ 'created_at', 'updated_at' ],
+    order_by: [ 'asc', 'desc' ],
   )
 
 returns
@@ -70,6 +82,12 @@ returns
         offset = params[:offset] || 0
         current_user = params[:current_user]
 
+        # check sort
+        sort_by = search_get_sort_by(params, 'updated_at')
+
+        # check order
+        order_by = search_get_order_by(params, 'desc')
+
         # enable search only for agents and admins
         return [] if !search_preferences(current_user)
 
@@ -94,7 +112,7 @@ returns
             }
             query_extention['bool']['must'].push access_condition
           end
-          items = SearchIndexBackend.search(query, limit, 'User', query_extention, offset)
+          items = SearchIndexBackend.search(query, limit, 'User', query_extention, offset, sort_by, order_by)
           users = []
           items.each do |item|
             user = User.lookup(id: item[:id])
@@ -104,17 +122,19 @@ returns
           return users
         end
 
+        order_sql = search_get_order_sql(sort_by, order_by, 'users.updated_at DESC')
+
         # fallback do sql query
         # - stip out * we already search for *query* -
         query.delete! '*'
         users = if params[:role_ids]
                   User.joins(:roles).where('roles.id' => params[:role_ids]).where(
                     '(users.firstname LIKE ? OR users.lastname LIKE ? OR users.email LIKE ? OR users.login LIKE ?) AND users.id != 1', "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%"
-                  ).order('updated_at DESC').offset(offset).limit(limit)
+                  ).order(order_sql).offset(offset).limit(limit)
                 else
                   User.where(
                     '(firstname LIKE ? OR lastname LIKE ? OR email LIKE ? OR login LIKE ?) AND id != 1', "%#{query}%", "%#{query}%", "%#{query}%", "%#{query}%"
-                  ).order('updated_at DESC').offset(offset).limit(limit)
+                  ).order(order_sql).offset(offset).limit(limit)
                 end
         users
       end

lib/search_index_backend.rb

@@ -277,6 +277,8 @@ return search result
 
   result = SearchIndexBackend.search('search query', limit, 'User')
 
+  result = SearchIndexBackend.search('search query', limit, 'User', ['updated_at'], ['desc'])
+
   result = [
     {
       :id   => 123,
@@ -294,20 +296,24 @@ return search result
 
 =end
 
-  def self.search(query, limit = 10, index = nil, query_extention = {}, from = 0)
+  # rubocop:disable Metrics/ParameterLists
+  def self.search(query, limit = 10, index = nil, query_extention = {}, from = 0, sort_by = [], order_by = [])
+    # rubocop:enable Metrics/ParameterLists
     return [] if query.blank?
     if index.class == Array
       ids = []
       index.each do |local_index|
-        local_ids = search_by_index(query, limit, local_index, query_extention, from)
+        local_ids = search_by_index(query, limit, local_index, query_extention, from, sort_by, order_by )
         ids = ids.concat(local_ids)
       end
       return ids
     end
-    search_by_index(query, limit, index, query_extention, from)
+    search_by_index(query, limit, index, query_extention, from, sort_by, order_by)
   end
 
-  def self.search_by_index(query, limit = 10, index = nil, query_extention = {}, from)
+  # rubocop:disable Metrics/ParameterLists
+  def self.search_by_index(query, limit = 10, index = nil, query_extention = {}, from = 0, sort_by = [], order_by = [])
+    # rubocop:enable Metrics/ParameterLists
     return [] if query.blank?
 
     url = build_url
@@ -324,15 +330,8 @@ return search result
     data = {}
     data['from'] = from
     data['size'] = limit
-    data['sort'] =
-      [
-        {
-          updated_at: {
-            order: 'desc'
-          }
-        },
-        '_score'
-      ]
+
+    data['sort'] = search_by_index_sort(sort_by, order_by)
 
     data['query'] = query_extention || {}
     data['query']['bool'] ||= {}
@@ -389,6 +388,36 @@ return search result
     ids
   end
 
+  def self.search_by_index_sort(sort_by = [], order_by = [])
+    result = []
+
+    sort_by.each_with_index do |value, index|
+      next if value.blank?
+      next if order_by[index].blank?
+
+      if value !~ /\./ && value !~ /_(time|date|till|id|ids|at)$/
+        value += '.raw'
+      end
+      result.push(
+        value => {
+          order: order_by[index],
+        },
+      )
+    end
+
+    if result.blank?
+      result.push(
+        created_at: {
+          order: 'desc',
+        },
+      )
+    end
+
+    result.push('_score')
+
+    result
+  end
+
 =begin
 
 get count of tickets and tickets which match on selector

lib/tasks/search_index_es.rake

@@ -23,50 +23,26 @@ namespace :searchindex do
     if info.present?
       number = info['version']['number'].to_s
     end
-    if number.blank? || number =~ /^[2-4]\./ || number =~ /^5\.[0-5]\./
 
-      # create indexes
-      SearchIndexBackend.index(
-        action: 'create',
-        data: {
-          mappings: {
-            Ticket: {
-              _source: { excludes: [ 'article.attachment' ] },
-              properties: {
-                article: {
-                  type: 'nested',
-                  include_in_parent: true,
-                  properties: {
-                    attachment: {
-                      type: 'attachment',
-                    }
-                  }
-                }
-              }
-            }
-          }
-        }
-      )
-      puts 'done'
-      Setting.set('es_pipeline', '')
+    mapping = {}
+    Models.searchable.each do |local_object|
+      mapping.merge!(get_mapping_properties_object(local_object))
+    end
 
-    # es with ingest-attachment plugin
-    else
+    # create indexes
+    SearchIndexBackend.index(
+      action: 'create',
+      data: {
+        mappings: mapping
+      }
+    )
 
-      # create indexes
-      SearchIndexBackend.index(
-        action: 'create',
-        data: {
-          mappings: {
-            Ticket: {
-              _source: { excludes: [ 'article.attachment' ] },
-            }
-          }
-        }
-      )
-      puts 'done'
+    if number.blank? || number =~ /^[2-4]\./ || number =~ /^5\.[0-5]\./
+      Setting.set('es_pipeline', '')
     end
 
+    puts 'done'
+
     Rake::Task['searchindex:create_pipeline'].execute
   end
 
@@ -168,3 +144,107 @@ namespace :searchindex do
 
   end
 end
+
+=begin
+
+This function will return a index mapping based on the
+attributes of the database table of the existing object.
+
+mapping = get_mapping_properties_object(Ticket)
+
+Returns:
+
+mapping = {
+  User: {
+    properties: {
+      firstname: {
+        type: 'keyword',
+      },
+    }
+  }
+}
+
+=end
+
+def get_mapping_properties_object(object)
+  result = {
+    object.name => {
+      properties: {}
+    }
+  }
+
+  store_columns = %w[preferences data]
+
+  object.columns_hash.each do |key, value|
+    if value.type == :string && value.limit && value.limit <= 5000 && store_columns.exclude?(key)
+      result[object.name][:properties][key] = {
+        type: 'text',
+        fields: {
+          raw: { 'type': 'string', 'index': 'not_analyzed' }
+        }
+      }
+    elsif value.type == :integer
+      result[object.name][:properties][key] = {
+        type: 'integer',
+      }
+    elsif value.type == :datetime
+      result[object.name][:properties][key] = {
+        type: 'date',
+      }
+    elsif value.type == :boolean
+      result[object.name][:properties][key] = {
+        type: 'boolean',
+        fields: {
+          raw: { 'type': 'boolean', 'index': 'not_analyzed' }
+        }
+      }
+    elsif value.type == :binary
+      result[object.name][:properties][key] = {
+        type: 'binary',
+      }
+    elsif value.type == :bigint
+      result[object.name][:properties][key] = {
+        type: 'long',
+      }
+    elsif value.type == :decimal
+      result[object.name][:properties][key] = {
+        type: 'float',
+      }
+    elsif value.type == :date
+      result[object.name][:properties][key] = {
+        type: 'date',
+      }
+    end
+  end
+
+  # es with mapper-attachments plugin
+  info = SearchIndexBackend.info
+  number = nil
+  if info.present?
+    number = info['version']['number'].to_s
+  end
+
+  if object.name == 'Ticket'
+
+    result[object.name][:_source] = {
+      excludes: ['article.attachment']
+    }
+
+    if number.blank? || number =~ /^[2-4]\./ || number =~ /^5\.[0-5]\./
+      result[object.name][:_source] = {
+        excludes: ['article.attachment']
+      }
+      result[object.name][:properties][:article] = {
+        type: 'nested',
+        include_in_parent: true,
+        properties: {
+          attachment: {
+            type: 'attachment',
+          }
+        }
+      }
+    end
+  end
+
+  result
+end

test/controllers/tickets_controller_test.rb

@@ -2126,4 +2126,93 @@ AAAFCAYAAACNbyblAAAAHElEQVQI12P4//8/w38GIAXDIBKE0DHxgljNBAAO
     assert_equal(ticket2.id, result['master_ticket']['id'])
   end
 
+  test '08.01 ticket search sorted' do
+    title = "ticket pagination #{rand(999_999_999)}"
+    tickets = []
+
+    ticket1 = Ticket.create!(
+      title: "#{title} A",
+      group: Group.lookup(name: 'Users'),
+      customer_id: @customer_without_org.id,
+      state: Ticket::State.lookup(name: 'new'),
+      priority: Ticket::Priority.lookup(name: '2 normal'),
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+    Ticket::Article.create!(
+      type: Ticket::Article::Type.lookup(name: 'note'),
+      sender: Ticket::Article::Sender.lookup(name: 'Customer'),
+      from: 'sender',
+      subject: 'subject',
+      body: 'some body',
+      ticket_id: ticket1.id,
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+
+    travel 2.seconds
+
+    ticket2 = Ticket.create!(
+      title: "#{title} B",
+      group: Group.lookup(name: 'Users'),
+      customer_id: @customer_without_org.id,
+      state: Ticket::State.lookup(name: 'new'),
+      priority: Ticket::Priority.lookup(name: '3 hoch'),
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+    Ticket::Article.create!(
+      type: Ticket::Article::Type.lookup(name: 'note'),
+      sender: Ticket::Article::Sender.lookup(name: 'Customer'),
+      from: 'sender',
+      subject: 'subject',
+      body: 'some body',
+      ticket_id: ticket2.id,
+      updated_by_id: 1,
+      created_by_id: 1,
+    )
+
+    credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin', 'adminpw')
+    get "/api/v1/tickets/search?query=#{CGI.escape(title)}&limit=40", params: {}, headers: @headers.merge('Authorization' => credentials)
+    assert_response(200)
+    result = JSON.parse(@response.body)
+    assert_equal(Hash, result.class)
+    assert_equal([ticket2.id, ticket1.id], result['tickets'])
+
+    credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin', 'adminpw')
+    get "/api/v1/tickets/search?query=#{CGI.escape(title)}&limit=40", params: { sort_by: 'created_at', order_by: 'asc' }, headers: @headers.merge('Authorization' => credentials)
+    assert_response(200)
+    result = JSON.parse(@response.body)
+    assert_equal(Hash, result.class)
+    assert_equal([ticket1.id, ticket2.id], result['tickets'])
+
+    credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin', 'adminpw')
+    get "/api/v1/tickets/search?query=#{CGI.escape(title)}&limit=40", params: { sort_by: 'title', order_by: 'asc' }, headers: @headers.merge('Authorization' => credentials)
+    assert_response(200)
+    result = JSON.parse(@response.body)
+    assert_equal(Hash, result.class)
+    assert_equal([ticket1.id, ticket2.id], result['tickets'])
+
+    credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin', 'adminpw')
+    get "/api/v1/tickets/search?query=#{CGI.escape(title)}&limit=40", params: { sort_by: 'title', order_by: 'desc' }, headers: @headers.merge('Authorization' => credentials)
+    assert_response(200)
+    result = JSON.parse(@response.body)
+    assert_equal(Hash, result.class)
+    assert_equal([ticket2.id, ticket1.id], result['tickets'])
+
+    credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin', 'adminpw')
+    get "/api/v1/tickets/search?query=#{CGI.escape(title)}&limit=40", params: { sort_by: %w[created_at updated_at], order_by: %w[asc asc] }, headers: @headers.merge('Authorization' => credentials)
+    assert_response(200)
+    result = JSON.parse(@response.body)
+    assert_equal(Hash, result.class)
+    assert_equal([ticket1.id, ticket2.id], result['tickets'])
+
+    credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin', 'adminpw')
+    get "/api/v1/tickets/search?query=#{CGI.escape(title)}&limit=40", params: { sort_by: %w[created_at updated_at], order_by: %w[desc asc]  }, headers: @headers.merge('Authorization' => credentials)
+    assert_response(200)
+    result = JSON.parse(@response.body)
+    assert_equal(Hash, result.class)
+    assert_equal([ticket2.id, ticket1.id], result['tickets'])
+  end
+
 end