Merge branch '263365-add-sort-by-popularity' into 'master'

Add sort by popularity to issues

See merge request gitlab-org/gitlab!65452

app/helpers/search_helper.rb

@@ -131,7 +131,7 @@ module SearchHelper
   end
 
   def search_sort_options
-    [
+    options = [
       {
         title: _('Created date'),
         sortable: true,
@@ -149,6 +149,19 @@ module SearchHelper
         }
       }
     ]
+
+    if search_service.scope == 'issues' && Feature.enabled?(:search_sort_issues_by_popularity)
+      options << {
+        title: _('Popularity'),
+        sortable: true,
+        sortParam: {
+          asc: 'popularity_asc',
+          desc: 'popularity_desc'
+        }
+      }
+    end
+
+    options
   end
 
   private

app/models/award_emoji.rb

@@ -27,9 +27,6 @@ class AwardEmoji < ApplicationRecord
   after_save :expire_cache
   after_destroy :expire_cache
 
-  after_save :update_awardable_upvotes_count
-  after_destroy :update_awardable_upvotes_count
-
   class << self
     def votes_for_collection(ids, type)
       select('name', 'awardable_id', 'COUNT(*) as count')
@@ -66,15 +63,6 @@ class AwardEmoji < ApplicationRecord
   def expire_cache
     awardable.try(:bump_updated_at)
     awardable.try(:expire_etag_cache)
-  end
-
-  private
-
-  def update_awardable_upvotes_count
-    return unless upvote? && awardable.has_attribute?(:upvotes_count)
-
-    awardable.update_column(:upvotes_count, awardable.upvotes)
+    awardable.try(:update_upvotes_count) if upvote?
   end
 end
-
-AwardEmoji.prepend_mod_with('AwardEmoji')

app/models/issue.rb

@@ -520,6 +520,11 @@ class Issue < ApplicationRecord
     issue_assignees.pluck(:user_id)
   end
 
+  def update_upvotes_count
+    self.lock!
+    self.update_column(:upvotes_count, self.upvotes)
+  end
+
   private
 
   def spammable_attribute_changed?

app/views/search/results/_issuable.html.haml

-%div{ class: 'search-result-row gl-pb-3! gl-mt-5 gl-mb-0!' }
+%div{ class: 'search-result-row gl-display-flex gl-sm-flex-direction-row gl-flex-direction-column gl-align-items-center gl-pb-3! gl-mt-5 gl-mb-0!' }
+  .col-sm-9
     %span.gl-display-flex.gl-align-items-center
       %span.badge.badge-pill.gl-badge.sm{ class: "badge-#{issuable_state_to_badge_class(issuable)}" }= issuable_state_text(issuable)
       = sprite_icon('eye-slash', css_class: 'gl-text-gray-500 gl-ml-2') if issuable.respond_to?(:confidential?) && issuable.confidential?
@@ -8,7 +9,11 @@
       = issuable_project_reference(issuable)
       ·
       = sprintf(s_('created %{issuable_created} by %{author}'), { issuable_created: time_ago_with_tooltip(issuable.created_at, placement: 'bottom'), author: link_to_member(@project, issuable.author, avatar: false) }).html_safe
-    ·
-    = sprintf(s_('updated %{time_ago}'), { time_ago: time_ago_with_tooltip(issuable.updated_at, placement: 'bottom') }).html_safe
-  .description.term.col-sm-10.gl-px-0
+    .description.term.gl-px-0
       = highlight_and_truncate_issuable(issuable, @search_term, @search_highlight)
+  .col-sm-3.gl-mt-3.gl-sm-mt-0.gl-text-right
+    - if Feature.enabled?(:search_sort_issues_by_popularity) && issuable.respond_to?(:upvotes_count) && issuable.upvotes_count > 0
+      %li.issuable-upvotes.gl-list-style-none.has-tooltip{ title: _('Upvotes') }
+        = sprite_icon('thumb-up', css_class: "gl-vertical-align-middle")
+        = issuable.upvotes_count
+    %span.gl-text-gray-500= sprintf(s_('updated %{time_ago}'), { time_ago: time_ago_with_tooltip(issuable.updated_at, placement: 'bottom') }).html_safe

config/feature_flags/development/search_sort_issues_by_popularity.yml

+---
+name: search_sort_issues_by_popularity
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/65231
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/334974
+milestone: '14.1'
+type: development
+group: group::global search
+default_enabled: false

db/post_migrate/20210706115312_add_upvotes_count_index_to_issues.rb

+# frozen_string_literal: true
+
+class AddUpvotesCountIndexToIssues < ActiveRecord::Migration[6.1]
+  include Gitlab::Database::MigrationHelpers
+
+  disable_ddl_transaction!
+
+  INDEX_NAME = 'index_issues_on_project_id_and_upvotes_count'
+
+  def up
+    add_concurrent_index :issues, [:project_id, :upvotes_count], name: INDEX_NAME
+  end
+
+  def down
+    remove_concurrent_index :issues, [:project_id, :upvotes_count], name: INDEX_NAME
+  end
+end

db/schema_migrations/20210706115312

+ac150e706b115849aa3802ae7b8e07d983e89eb637c48582c64948cbc7d7163d
\ No newline at end of file

db/structure.sql

@@ -23870,6 +23870,8 @@ CREATE UNIQUE INDEX index_issues_on_project_id_and_external_key ON issues USING
 
 CREATE UNIQUE INDEX index_issues_on_project_id_and_iid ON issues USING btree (project_id, iid);
 
+CREATE INDEX index_issues_on_project_id_and_upvotes_count ON issues USING btree (project_id, upvotes_count);
+
 CREATE INDEX index_issues_on_promoted_to_epic_id ON issues USING btree (promoted_to_epic_id) WHERE (promoted_to_epic_id IS NOT NULL);
 
 CREATE INDEX index_issues_on_sprint_id ON issues USING btree (sprint_id);

ee/app/helpers/ee/search_helper.rb

@@ -118,16 +118,25 @@ module EE
 
     override :search_sort_options
     def search_sort_options
+      original_options = super
+
       options = []
+
       if search_service.use_elasticsearch?
         options << {
           title: _('Most relevant'),
           sortable: false,
           sortParam: 'relevant'
         }
+
+        unless Elastic::DataMigrationService.migration_has_finished?(:add_upvotes_to_issues)
+          original_options.delete_if do |option|
+            option[:title] == _('Popularity')
+          end
+        end
       end
 
-      options + super
+      options + original_options
     end
 
     private

ee/app/models/ee/award_emoji.rb

-# frozen_string_literal: true
-
-module EE
-  module AwardEmoji
-    extend ActiveSupport::Concern
-
-    prepended do
-      UPDATE_ELASTIC_ASSOCIATIONS_FOR = [::Issue].freeze
-
-      after_commit :update_elastic_associations, on: [:create, :destroy]
-
-      def update_elastic_associations
-        return unless UPDATE_ELASTIC_ASSOCIATIONS_FOR.any? { |model| awardable.is_a?(model) }
-        return unless awardable.maintaining_elasticsearch?
-
-        awardable.maintain_elasticsearch_update
-      end
-    end
-  end
-end

ee/app/models/ee/issue.rb

@@ -242,6 +242,13 @@ module EE
       issue_type_supports?(:epics) && project.group.present?
     end
 
+    override :update_upvotes_count
+    def update_upvotes_count
+      maintain_elasticsearch_update if maintaining_elasticsearch?
+
+      super
+    end
+
     private
 
     def blocking_issues_ids

ee/lib/elastic/latest/issue_class_proxy.rb

@@ -30,12 +30,32 @@ module Elastic
 
       # rubocop: disable CodeReuse/ActiveRecord
       def preload_indexing_data(relation)
-        relation.includes(:issue_assignees, :award_emoji, project: [:project_feature])
+        relation.includes(:issue_assignees, project: [:project_feature])
       end
       # rubocop: enable CodeReuse/ActiveRecord
 
       private
 
+      # override
+      def apply_sort(query_hash, options)
+        case ::Gitlab::Search::SortOptions.sort_and_direction(options[:order_by], options[:sort])
+        when :popularity_asc
+          query_hash.merge(sort: {
+            upvotes: {
+              order: 'asc'
+            }
+          })
+        when :popularity_desc
+          query_hash.merge(sort: {
+            upvotes: {
+              order: 'desc'
+            }
+          })
+        else
+          super
+        end
+      end
+
       # Builds an elasticsearch query that will select documents from a
       # set of projects for Group and Project searches, taking user access
       # rules for issues into account. Relies upon super for Global searches

ee/lib/elastic/latest/issue_instance_proxy.rb

@@ -20,7 +20,7 @@ module Elastic
         data['visibility_level'] = target.project.visibility_level
         data['issues_access_level'] = safely_read_project_feature_for_elasticsearch(:issues)
 
-        data['upvotes'] = count_emoji
+        data['upvotes'] = target.upvotes_count
 
         data.merge(generic_attributes)
       end
@@ -30,12 +30,6 @@ module Elastic
       def generic_attributes
         super.except('join_field')
       end
-
-      def count_emoji
-        target.award_emoji.count do |c|
-          c.name == AwardEmoji::UPVOTE_NAME
-        end
-      end
     end
   end
 end

ee/spec/lib/gitlab/elastic/search_results_spec.rb

@@ -310,6 +310,10 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :clean_gitlab_redis_sha
       let!(:new_updated) { create(:issue, project: project, title: 'updated recent', updated_at: 1.day.ago) }
       let!(:very_old_updated) { create(:issue, project: project, title: 'updated very old', updated_at: 1.year.ago) }
 
+      let!(:less_popular_result) { create(:issue, project: project, title: 'less popular', upvotes_count: 10) }
+      let!(:popular_result) { create(:issue, project: project, title: 'popular', upvotes_count: 100) }
+      let!(:non_popular_result) { create(:issue, project: project, title: 'non popular', upvotes_count: 1) }
+
       before do
         ensure_elasticsearch_index!
       end
@@ -318,6 +322,10 @@ RSpec.describe Gitlab::Elastic::SearchResults, :elastic, :clean_gitlab_redis_sha
         let(:results_created) { described_class.new(user, 'sorted', [project.id], sort: sort) }
         let(:results_updated) { described_class.new(user, 'updated', [project.id], sort: sort) }
       end
+
+      include_examples 'search results sorted by popularity' do
+        let(:results_popular) { described_class.new(user, 'popular', [project.id], sort: sort) }
+      end
     end
   end
 

ee/spec/models/award_emoji_spec.rb

-# frozen_string_literal: true
-
-require 'spec_helper'
-
-RSpec.describe AwardEmoji do
-  describe '#update_elastic_associations' do
-    let_it_be(:issue) { create(:issue) }
-    let_it_be(:merge_request) { create(:merge_request) }
-
-    context 'maintaining_elasticsearch is true' do
-      before do
-        allow(issue).to receive(:maintaining_elasticsearch?).and_return(true)
-        allow(merge_request).to receive(:maintaining_elasticsearch?).and_return(true)
-      end
-
-      it 'calls maintain_elasticsearch_update on create' do
-        expect(issue).to receive(:maintain_elasticsearch_update)
-
-        create(:award_emoji, :upvote, awardable: issue)
-      end
-
-      it 'calls maintain_elasticsearch_update on destroy' do
-        award_emoji = create(:award_emoji, :upvote, awardable: issue)
-
-        expect(issue).to receive(:maintain_elasticsearch_update)
-
-        award_emoji.destroy!
-      end
-
-      it 'does nothing for other awardable_type' do
-        expect(merge_request).not_to receive(:maintain_elasticsearch_update)
-
-        create(:award_emoji, :upvote, awardable: merge_request)
-      end
-    end
-
-    context 'maintaining_elasticsearch is false' do
-      it 'does not call maintain_elasticsearch_update' do
-        expect(issue).not_to receive(:maintain_elasticsearch_update)
-
-        award_emoji = create(:award_emoji, :upvote, awardable: issue)
-
-        expect(issue).not_to receive(:maintain_elasticsearch_update)
-
-        award_emoji.destroy!
-      end
-    end
-  end
-end

ee/spec/models/issue_spec.rb

@@ -528,6 +528,16 @@ RSpec.describe Issue do
           issue.update!(title: 'the new title')
         end
       end
+
+      context 'when changing upvotes' do
+        it 'calls maintain_elasticsearch_update' do
+          expect(issue).to receive(:maintain_elasticsearch_update).twice
+
+          award_emoji = create(:award_emoji, :upvote, awardable: issue)
+
+          award_emoji.destroy
+        end
+      end
     end
   end
 

lib/gitlab/search/sort_options.rb

@@ -15,6 +15,10 @@ module Gitlab
           :updated_at_asc
         when %w[updated_at desc], [nil, 'updated_desc']
           :updated_at_desc
+        when %w[popularity asc], [nil, 'popularity_asc']
+          :popularity_asc
+        when %w[popularity desc], [nil, 'popularity_desc']
+          :popularity_desc
         else
           :unknown
         end

lib/gitlab/search_results.rb

@@ -7,6 +7,11 @@ module Gitlab
     DEFAULT_PAGE = 1
     DEFAULT_PER_PAGE = 20
 
+    SCOPE_ONLY_SORT = {
+      popularity_asc: %w[issues],
+      popularity_desc: %w[issues]
+    }.freeze
+
     attr_reader :current_user, :query, :order_by, :sort, :filters
 
     # Limit search results by passed projects
@@ -128,20 +133,29 @@ module Gitlab
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
-    def apply_sort(scope)
+    def apply_sort(results, scope: nil)
       # Due to different uses of sort param we prefer order_by when
       # present
-      case ::Gitlab::Search::SortOptions.sort_and_direction(order_by, sort)
+      sort_by = ::Gitlab::Search::SortOptions.sort_and_direction(order_by, sort)
+
+      # Reset sort to default if the chosen one is not supported by scope
+      sort_by = nil if SCOPE_ONLY_SORT[sort_by] && !SCOPE_ONLY_SORT[sort_by].include?(scope)
+
+      case sort_by
       when :created_at_asc
-        scope.reorder('created_at ASC')
+        results.reorder('created_at ASC')
       when :created_at_desc
-        scope.reorder('created_at DESC')
+        results.reorder('created_at DESC')
       when :updated_at_asc
-        scope.reorder('updated_at ASC')
+        results.reorder('updated_at ASC')
       when :updated_at_desc
-        scope.reorder('updated_at DESC')
+        results.reorder('updated_at DESC')
+      when :popularity_asc
+        results.reorder('upvotes_count ASC')
+      when :popularity_desc
+        results.reorder('upvotes_count DESC')
       else
-        scope.reorder('created_at DESC')
+        results.reorder('created_at DESC')
       end
     end
     # rubocop: enable CodeReuse/ActiveRecord
@@ -157,7 +171,7 @@ module Gitlab
         issues = issues.where(project_id: project_ids_relation) # rubocop: disable CodeReuse/ActiveRecord
       end
 
-      apply_sort(issues)
+      apply_sort(issues, scope: 'issues')
     end
 
     # rubocop: disable CodeReuse/ActiveRecord
@@ -177,7 +191,7 @@ module Gitlab
         merge_requests = merge_requests.in_projects(project_ids_relation)
       end
 
-      apply_sort(merge_requests)
+      apply_sort(merge_requests, scope: 'merge_requests')
     end
 
     def default_scope

spec/lib/gitlab/search_results_spec.rb

@@ -229,10 +229,18 @@ RSpec.describe Gitlab::SearchResults do
         let!(:new_updated) { create(:issue, project: project, title: 'updated recent', updated_at: 1.day.ago) }
         let!(:very_old_updated) { create(:issue, project: project, title: 'updated very old', updated_at: 1.year.ago) }
 
+        let!(:less_popular_result) { create(:issue, project: project, title: 'less popular', upvotes_count: 10) }
+        let!(:popular_result) { create(:issue, project: project, title: 'popular', upvotes_count: 100) }
+        let!(:non_popular_result) { create(:issue, project: project, title: 'non popular', upvotes_count: 1) }
+
         include_examples 'search results sorted' do
           let(:results_created) { described_class.new(user, 'sorted', Project.order(:id), sort: sort, filters: filters) }
           let(:results_updated) { described_class.new(user, 'updated', Project.order(:id), sort: sort, filters: filters) }
         end
+
+        include_examples 'search results sorted by popularity' do
+          let(:results_popular) { described_class.new(user, 'popular', Project.order(:id), sort: sort, filters: filters) }
+        end
       end
     end
 

spec/migrations/add_upvotes_count_index_to_issues_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+require_migration!
+
+RSpec.describe AddUpvotesCountIndexToIssues do
+  let(:migration_instance) { described_class.new }
+
+  describe '#up' do
+    it 'adds index' do
+      expect { migrate! }.to change { migration_instance.index_exists?(:issues, [:project_id, :upvotes_count], name: described_class::INDEX_NAME) }.from(false).to(true)
+    end
+  end
+
+  describe '#down' do
+    it 'removes index' do
+      migrate!
+
+      expect { schema_migrate_down! }.to change { migration_instance.index_exists?(:issues, [:project_id, :upvotes_count], name: described_class::INDEX_NAME) }.from(true).to(false)
+    end
+  end
+end

spec/support/shared_examples/lib/gitlab/search_results_sorted_shared_examples.rb

@@ -33,3 +33,21 @@ RSpec.shared_examples 'search results sorted' do
     end
   end
 end
+
+RSpec.shared_examples 'search results sorted by popularity' do
+  context 'sort: popularity_desc' do
+    let(:sort) { 'popularity_desc' }
+
+    it 'sorts results by upvotes' do
+      expect(results_popular.objects(scope).map(&:id)).to eq([popular_result.id, less_popular_result.id, non_popular_result.id])
+    end
+  end
+
+  context 'sort: popularity_asc' do
+    let(:sort) { 'popularity_asc' }
+
+    it 'sorts results by created_at' do
+      expect(results_popular.objects(scope).map(&:id)).to eq([non_popular_result.id, less_popular_result.id, popular_result.id])
+    end
+  end
+end