Merge branch '239166-recent-items-autocomplete' into 'master'

Recent issues search autocomplete

Closes #239166

See merge request gitlab-org/gitlab!40669

app/controllers/projects/issues_controller.rb

@@ -26,6 +26,7 @@ class Projects::IssuesController < Projects::ApplicationController
   before_action :whitelist_query_limiting, only: [:create, :create_merge_request, :move, :bulk_update]
   before_action :check_issues_available!
   before_action :issue, unless: ->(c) { c.issue_except_actions.include?(c.action_name.to_sym) }
+  after_action :log_issue_show, unless: ->(c) { c.issue_except_actions.include?(c.action_name.to_sym) }
 
   before_action :set_issuables_index, if: ->(c) { c.set_issuables_index_only_actions.include?(c.action_name.to_sym) }
 
@@ -249,6 +250,13 @@ class Projects::IssuesController < Projects::ApplicationController
     @issue
   end
   # rubocop: enable CodeReuse/ActiveRecord
+
+  def log_issue_show
+    return unless current_user && @issue
+
+    ::Gitlab::Search::RecentIssues.new(user: current_user).log_view(@issue)
+  end
+
   alias_method :subscribable_resource, :issue
   alias_method :issuable, :issue
   alias_method :awardable, :issue

app/helpers/search_helper.rb

@@ -7,6 +7,7 @@ module SearchHelper
     return unless current_user
 
     resources_results = [
+      recent_issues_autocomplete(term),
       groups_autocomplete(term),
       projects_autocomplete(term)
     ].flatten
@@ -178,6 +179,20 @@ module SearchHelper
       }
     end
   end
+
+  def recent_issues_autocomplete(term, limit = 5)
+    return [] unless current_user
+
+    ::Gitlab::Search::RecentIssues.new(user: current_user).search(term).limit(limit).map do |i|
+      {
+        category: "Recent issues",
+        id: i.id,
+        label: search_result_sanitize(i.title),
+        url: issue_path(i),
+        avatar_url: i.project.avatar_url || ''
+      }
+    end
+  end
   # rubocop: enable CodeReuse/ActiveRecord
 
   def search_result_sanitize(str)

app/models/concerns/id_in_ordered.rb

+# frozen_string_literal: true
+
+module IdInOrdered
+  extend ActiveSupport::Concern
+
+  included do
+    scope :id_in_ordered, -> (ids) do
+      raise ArgumentError, "ids must be an array of integers" unless ids.is_a?(Enumerable) && ids.all? { |id| id.is_a?(Integer) }
+
+      # No need to sort if no more than 1 and the sorting code doesn't work
+      # with an empty array
+      return id_in(ids) unless ids.count > 1
+
+      id_attribute = arel_table[:id]
+      id_in(ids)
+        .order(
+          Arel.sql("array_position(ARRAY[#{ids.join(',')}], #{id_attribute.relation.name}.#{id_attribute.name})"))
+    end
+  end
+end

app/models/issue.rb

@@ -18,6 +18,7 @@ class Issue < ApplicationRecord
   include MilestoneEventable
   include WhereComposite
   include StateEventable
+  include IdInOrdered
 
   DueDateStruct                   = Struct.new(:title, :name).freeze
   NoDueDate                       = DueDateStruct.new('No Due Date', '0').freeze

config/feature_flags/development/recent_items_search.yml

+---
+name: recent_items_search
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/40669
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/244277
+group: group::global search
+type: development
+default_enabled: false
\ No newline at end of file

lib/gitlab/search/recent_issues.rb

+# frozen_string_literal: true
+
+module Gitlab
+  module Search
+    class RecentIssues
+      ITEMS_LIMIT = 100
+      EXPIRES_AFTER = 7.days
+
+      def initialize(user:, items_limit: ITEMS_LIMIT, expires_after: EXPIRES_AFTER)
+        @user = user
+        @items_limit = items_limit
+        @expires_after = expires_after
+      end
+
+      def log_view(issue)
+        return unless recent_items_enabled?
+
+        with_redis do |redis|
+          redis.zadd(key, Time.now.to_f, issue.id)
+          redis.expire(key, @expires_after)
+
+          # There is a race condition here where we could end up removing an
+          # item from 2 places concurrently but this is fine since worst case
+          # scenario we remove an extra item from the end of the list.
+          if redis.zcard(key) > @items_limit
+            redis.zremrangebyrank(key, 0, 0) # Remove least recent
+          end
+        end
+      end
+
+      def search(term)
+        return Issue.none unless recent_items_enabled?
+
+        ids = with_redis do |redis|
+          redis.zrevrange(key, 0, @items_limit - 1)
+        end.map(&:to_i)
+
+        IssuesFinder.new(@user, search: term, in: 'title').execute.reorder(nil).id_in_ordered(ids) # rubocop: disable CodeReuse/ActiveRecord
+      end
+
+      private
+
+      def with_redis(&blk)
+        Gitlab::Redis::SharedState.with(&blk) # rubocop: disable CodeReuse/ActiveRecord
+      end
+
+      def key
+        "recent_items:#{type.name.downcase}:#{@user.id}"
+      end
+
+      def type
+        Issue
+      end
+
+      def recent_items_enabled?
+        Feature.enabled?(:recent_items_search, @user)
+      end
+    end
+  end
+end

spec/controllers/projects/issues_controller_spec.rb

@@ -1011,6 +1011,24 @@ RSpec.describe Projects::IssuesController do
           end
         end
       end
+
+      it 'logs the view with Gitlab::Search::RecentIssues' do
+        sign_in(user)
+        recent_issues_double = instance_double(::Gitlab::Search::RecentIssues, log_view: nil)
+        expect(::Gitlab::Search::RecentIssues).to receive(:new).with(user: user).and_return(recent_issues_double)
+
+        go(id: issue.to_param)
+
+        expect(recent_issues_double).to have_received(:log_view)
+      end
+
+      context 'when not logged in' do
+        it 'does not log the view with Gitlab::Search::RecentIssues' do
+          expect(::Gitlab::Search::RecentIssues).not_to receive(:new)
+
+          go(id: issue.to_param)
+        end
+      end
     end
 
     describe 'GET #realtime_changes' do

spec/helpers/search_helper_spec.rb

@@ -73,6 +73,39 @@ RSpec.describe SearchHelper do
         expect(result.keys).to match_array(%i[category id label url avatar_url])
       end
 
+      it 'includes the first 5 of the users recent issues' do
+        recent_issues = instance_double(::Gitlab::Search::RecentIssues)
+        expect(::Gitlab::Search::RecentIssues).to receive(:new).with(user: user).and_return(recent_issues)
+        project1 = create(:project, :with_avatar, namespace: user.namespace)
+        project2 = create(:project, namespace: user.namespace)
+        issue1 = create(:issue, title: 'issue 1', project: project1)
+        issue2 = create(:issue, title: 'issue 2', project: project2)
+
+        other_issues = create_list(:issue, 5)
+
+        expect(recent_issues).to receive(:search).with('the search term').and_return(Issue.id_in_ordered([issue1.id, issue2.id, *other_issues.map(&:id)]))
+
+        results = search_autocomplete_opts("the search term")
+
+        expect(results.count).to eq(5)
+
+        expect(results[0]).to include({
+          category: 'Recent issues',
+          id: issue1.id,
+          label: 'issue 1',
+          url: Gitlab::Routing.url_helpers.project_issue_path(issue1.project, issue1),
+          avatar_url: project1.avatar_url
+        })
+
+        expect(results[1]).to include({
+          category: 'Recent issues',
+          id: issue2.id,
+          label: 'issue 2',
+          url: Gitlab::Routing.url_helpers.project_issue_path(issue2.project, issue2),
+          avatar_url: '' # This project didn't have an avatar so set this to ''
+        })
+      end
+
       it "does not include the public group" do
         group = create(:group)
         expect(search_autocomplete_opts(group.name).size).to eq(0)

spec/lib/gitlab/search/recent_issues_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe ::Gitlab::Search::RecentIssues, :clean_gitlab_redis_shared_state do
+  let(:user) { create(:user) }
+  let(:issue) { create(:issue, title: 'hello world 1', project: project) }
+  let(:recent_issues) { described_class.new(user: user, items_limit: 5) }
+  let(:project) { create(:project, :public) }
+
+  before do
+    stub_feature_flags(recent_items_search: true)
+  end
+
+  describe '#log_viewing' do
+    it 'adds the item to the recent items' do
+      recent_issues.log_view(issue)
+
+      results = recent_issues.search('hello')
+
+      expect(results).to eq([issue])
+    end
+
+    it 'removes an item when it exceeds the size items_limit' do
+      (1..6).each do |i|
+        recent_issues.log_view(create(:issue, title: "issue #{i}", project: project))
+      end
+
+      results = recent_issues.search('issue')
+
+      expect(results.map(&:title)).to contain_exactly('issue 6', 'issue 5', 'issue 4', 'issue 3', 'issue 2')
+    end
+
+    it 'expires the items after expires_after' do
+      recent_issues = described_class.new(user: user, expires_after: 0)
+
+      recent_issues.log_view(issue)
+
+      results = recent_issues.search('hello')
+
+      expect(results).to be_empty
+    end
+
+    it 'does not include results logged for another user' do
+      another_user = create(:user)
+      another_issue = create(:issue, title: 'hello world 2', project: project)
+      described_class.new(user: another_user).log_view(another_issue)
+      recent_issues.log_view(issue)
+
+      results = recent_issues.search('hello')
+
+      expect(results).to eq([issue])
+    end
+
+    context 'when recent_items_search feature flag is disabled' do
+      before do
+        stub_feature_flags(recent_items_search: false)
+      end
+
+      it 'does not store anything' do
+        recent_issues.log_view(issue)
+
+        # Re-enable before searching to prove that the `log_view` call did
+        # not persist it
+        stub_feature_flags(recent_items_search: true)
+
+        results = recent_issues.search('hello')
+
+        expect(results).to be_empty
+      end
+    end
+  end
+
+  describe '#search' do
+    let(:issue1) { create(:issue, title: "matching issue 1", project: project) }
+    let(:issue2) { create(:issue, title: "matching issue 2", project: project) }
+    let(:issue3) { create(:issue, title: "matching issue 3", project: project) }
+    let(:non_matching_issue) { create(:issue, title: "different issue", project: project) }
+    let!(:non_viewed_issued) { create(:issue, title: "matching but not viewed issue", project: project) }
+
+    before do
+      recent_issues.log_view(issue1)
+      recent_issues.log_view(issue2)
+      recent_issues.log_view(issue3)
+      recent_issues.log_view(non_matching_issue)
+    end
+
+    it 'matches partial text in the issue title' do
+      expect(recent_issues.search('matching')).to contain_exactly(issue1, issue2, issue3)
+    end
+
+    it 'returns results sorted by recently viewed' do
+      recent_issues.log_view(issue2)
+
+      expect(recent_issues.search('matching')).to eq([issue2, issue3, issue1])
+    end
+
+    it 'does not leak issues you no longer have access to' do
+      private_project = create(:project, :public, namespace: create(:group))
+      private_issue = create(:issue, project: private_project, title: 'matching issue title')
+
+      recent_issues.log_view(private_issue)
+
+      private_project.update!(visibility_level: Project::PRIVATE)
+
+      expect(recent_issues.search('matching')).not_to include(private_issue)
+    end
+
+    context 'when recent_items_search feature flag is disabled' do
+      it 'does not return anything' do
+        recent_issues.log_view(issue)
+
+        # Disable after persisting to prove that the `search` is not searching
+        # anything
+        stub_feature_flags(recent_items_search: false)
+
+        results = recent_issues.search('hello')
+
+        expect(results).to be_empty
+      end
+    end
+  end
+end

spec/models/concerns/id_in_ordered_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe IdInOrdered do
+  describe 'Issue' do
+    describe '.id_in_ordered' do
+      it 'returns issues matching the ids in the same order as the ids' do
+        issue1 = create(:issue)
+        issue2 = create(:issue)
+        issue3 = create(:issue)
+        issue4 = create(:issue)
+        issue5 = create(:issue)
+
+        expect(Issue.id_in_ordered([issue3.id, issue1.id, issue4.id, issue5.id, issue2.id])).to eq([
+          issue3, issue1, issue4, issue5, issue2
+])
+      end
+
+      context 'when the ids are not an array of integers' do
+        it 'raises ArgumentError' do
+          expect { Issue.id_in_ordered([1, 'no SQL injection']) }.to raise_error(ArgumentError, "ids must be an array of integers")
+        end
+      end
+
+      context 'when an empty array is given' do
+        it 'does not raise error' do
+          expect(Issue.id_in_ordered([]).to_a).to be_empty
+        end
+      end
+    end
+  end
+end