Commit 5cc63c5d authored by Dmitry Gruzd's avatar Dmitry Gruzd

Add sort by popularity to issues

Changelog: changed
parent 3ee41a90
......@@ -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
......
......@@ -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')
......@@ -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?
......
%div{ class: 'search-result-row gl-pb-3! gl-mt-5 gl-mb-0!' }
%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?
= link_to issuable_path(issuable), data: { track_event: 'click_text', track_label: "#{issuable.class.name.downcase}_title", track_property: 'search_result' }, class: 'gl-w-full' do
%span.term.str-truncated.gl-font-weight-bold.gl-ml-2= issuable.title
.gl-text-gray-500.gl-my-3
= issuable_project_reference(issuable)
&middot;
= 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
&middot;
= 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
= highlight_and_truncate_issuable(issuable, @search_term, @search_highlight)
%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?
= link_to issuable_path(issuable), data: { track_event: 'click_text', track_label: "#{issuable.class.name.downcase}_title", track_property: 'search_result' }, class: 'gl-w-full' do
%span.term.str-truncated.gl-font-weight-bold.gl-ml-2= issuable.title
.gl-text-gray-500.gl-my-3
= issuable_project_reference(issuable)
&middot;
= 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
.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
---
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
# 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
ac150e706b115849aa3802ae7b8e07d983e89eb637c48582c64948cbc7d7163d
\ No newline at end of file
......@@ -23865,6 +23865,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);
......@@ -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
......
# 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
......@@ -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
......
......@@ -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
......
......@@ -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
......@@ -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
......
# 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
......@@ -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
......
......@@ -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
......
......@@ -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
......
......@@ -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
......
# 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
......@@ -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
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment