diff --git a/app/controllers/search_controller.rb b/app/controllers/search_controller.rb index 048fa7921859d64a95b28be8b56917a5d04fcbb2..0f149c24a59a2ce3437e91d31d67f33b06f40c1c 100644 --- a/app/controllers/search_controller.rb +++ b/app/controllers/search_controller.rb @@ -9,6 +9,7 @@ class SearchController < ApplicationController SCOPE_PRELOAD_METHOD = { projects: :with_web_entity_associations, issues: :with_web_entity_associations, + merge_requests: :with_web_entity_associations, epics: :with_web_entity_associations }.freeze diff --git a/app/helpers/search_helper.rb b/app/helpers/search_helper.rb index f951c8e22cfe1d58f5d9a3d6ce9392ffb30f1e11..de1e0e4e05eb1d8eb5ac01f1c06b383c35a439cc 100644 --- a/app/helpers/search_helper.rb +++ b/app/helpers/search_helper.rb @@ -367,10 +367,10 @@ def simple_search_highlight_and_truncate(text, phrase, options = {}) end # _search_highlight is used in EE override - def highlight_and_truncate_issue(issue, search_term, _search_highlight) - return unless issue.description.present? + def highlight_and_truncate_issuable(issuable, search_term, _search_highlight) + return unless issuable.description.present? - simple_search_highlight_and_truncate(issue.description, search_term, highlighter: '<span class="gl-text-black-normal gl-font-weight-bold">\1</span>') + simple_search_highlight_and_truncate(issuable.description, search_term, highlighter: '<span class="gl-text-black-normal gl-font-weight-bold">\1</span>') end def show_user_search_tab? @@ -380,6 +380,36 @@ def show_user_search_tab? can?(current_user, :read_users_list) end end + + def issuable_state_to_badge_class(issuable) + # Closed is considered "danger" for MR so we need to handle separately + if issuable.is_a?(::MergeRequest) + if issuable.merged? + :primary + elsif issuable.closed? + :danger + else + :success + end + else + if issuable.closed? + :info + else + :success + end + end + end + + def issuable_state_text(issuable) + case issuable.state + when 'merged' + _("Merged") + when 'closed' + _("Closed") + else + _("Open") + end + end end SearchHelper.prepend_if_ee('EE::SearchHelper') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index bf7a73f24e4163e95f41c33274c75283051e45b8..45eb2361cf469d96eec359ac35d50b62ee2dda03 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -294,6 +294,7 @@ def public_merge_status scope :preload_author, -> { preload(:author) } scope :preload_approved_by_users, -> { preload(:approved_by_users) } scope :preload_metrics, -> (relation) { preload(metrics: relation) } + scope :with_web_entity_associations, -> { preload(:author, :target_project) } scope :with_auto_merge_enabled, -> do with_state(:opened).where(auto_merge_enabled: true) diff --git a/app/views/search/results/_issuable.html.haml b/app/views/search/results/_issuable.html.haml new file mode 100644 index 0000000000000000000000000000000000000000..288ac53a954b85ebeef09b640908279d1a947834 --- /dev/null +++ b/app/views/search/results/_issuable.html.haml @@ -0,0 +1,10 @@ +%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 + = sprintf(s_(' %{project_name}#%{issuable_iid} · opened %{issuable_created} by %{author}'), { project_name: issuable.project.full_name, issuable_iid: issuable.iid, issuable_created: time_ago_with_tooltip(issuable.created_at, placement: 'bottom'), author: link_to_member(@project, issuable.author, avatar: false) }).html_safe + .description.term.col-sm-10.gl-px-0 + = highlight_and_truncate_issuable(issuable, @search_term, @search_highlight) diff --git a/app/views/search/results/_issue.html.haml b/app/views/search/results/_issue.html.haml index a101e60f297c2284489cacedb9e8aaf76bc8b9e5..6fb463b75fc4a6ba7d5638911cd6ff4dda710ce3 100644 --- a/app/views/search/results/_issue.html.haml +++ b/app/views/search/results/_issue.html.haml @@ -1,13 +1 @@ -%div{ class: 'search-result-row gl-pb-3! gl-mt-5 gl-mb-0!' } - %span.gl-display-flex.gl-align-items-center - - if issue.closed? - %span.badge.badge-info.badge-pill.gl-badge.sm= _("Closed") - - else - %span.badge.badge-success.badge-pill.gl-badge.sm= _("Open") - = sprite_icon('eye-slash', css_class: 'gl-text-gray-500 gl-ml-2') if issue.confidential? - = link_to project_issue_path(issue.project, issue), data: { track_event: 'click_text', track_label: 'issue_title', track_property: 'search_result' }, class: 'gl-w-full' do - %span.term.str-truncated.gl-font-weight-bold.gl-ml-2= issue.title - .gl-text-gray-500.gl-my-3 - = sprintf(s_(' %{project_name}#%{issue_iid} · opened %{issue_created} by %{author}'), { project_name: issue.project.full_name, issue_iid: issue.iid, issue_created: time_ago_with_tooltip(issue.created_at, placement: 'bottom'), author: link_to_member(@project, issue.author, avatar: false) }).html_safe - .description.term.col-sm-10.gl-px-0 - = highlight_and_truncate_issue(issue, @search_term, @search_highlight) += render partial: 'search/results/issuable', object: issue diff --git a/app/views/search/results/_merge_request.html.haml b/app/views/search/results/_merge_request.html.haml index 3135ab9a17e0e9c93ce5d3a9963b8180b77e0988..b2b067bcf68f7689f3a0c0503ac19a44f7b223f8 100644 --- a/app/views/search/results/_merge_request.html.haml +++ b/app/views/search/results/_merge_request.html.haml @@ -1,14 +1 @@ -.search-result-row - %h4 - = link_to project_merge_request_path(merge_request.target_project, merge_request), data: {track_event: 'click_text', track_label: 'merge_request_title', track_property: 'search_result'} do - %span.term.str-truncated= merge_request.title - - if merge_request.merged? - %span.badge.badge-primary.gl-ml-2= _("Merged") - - elsif merge_request.closed? - %span.badge.badge-danger.gl-ml-2= _("Closed") - .float-right= merge_request.to_reference - - if merge_request.description.present? - .description.term - = search_md_sanitize(merge_request.description) - %span.light - #{merge_request.project.full_name} += render partial: 'search/results/issuable', object: merge_request diff --git a/changelogs/unreleased/275899-share-view-code-for-mr-issue-search-results.yml b/changelogs/unreleased/275899-share-view-code-for-mr-issue-search-results.yml new file mode 100644 index 0000000000000000000000000000000000000000..c9e6a0a97fef7968889b2fe953d2d01f3e8621e1 --- /dev/null +++ b/changelogs/unreleased/275899-share-view-code-for-mr-issue-search-results.yml @@ -0,0 +1,5 @@ +--- +title: Update merge request search results design +merge_request: 46944 +author: +type: changed diff --git a/ee/app/helpers/ee/search_helper.rb b/ee/app/helpers/ee/search_helper.rb index c61aeafddbe93cec72c596bfecfe66e499d59775..967d9de650f832ce47d61eac47aba1437e8fda16 100644 --- a/ee/app/helpers/ee/search_helper.rb +++ b/ee/app/helpers/ee/search_helper.rb @@ -69,14 +69,14 @@ def search_entries_info_template(collection) end end - override :highlight_and_truncate_issue - def highlight_and_truncate_issue(issue, search_term, search_highlight) - return super unless search_service.use_elasticsearch? && search_highlight[issue.id]&.description.present? + override :highlight_and_truncate_issuable + def highlight_and_truncate_issuable(issuable, search_term, search_highlight) + return super unless search_service.use_elasticsearch? && search_highlight[issuable.id]&.description.present? # We use Elasticsearch highlighting for results from Elasticsearch. Sanitize the description, replace the # pre/post tags from Elasticsearch with highlighting, truncate, and mark as html_safe. HTML tags are not # counted towards the character limit. - text = sanitize(search_highlight[issue.id].description.first) + text = sanitize(search_highlight[issuable.id].description.first) text.gsub!(::Elastic::Latest::GitClassProxy::HIGHLIGHT_START_TAG, '<span class="gl-text-black-normal gl-font-weight-bold">') text.gsub!(::Elastic::Latest::GitClassProxy::HIGHLIGHT_END_TAG, '</span>') Truncato.truncate(text, count_tags: false, count_tail: false, max_length: 200).html_safe diff --git a/ee/spec/helpers/search_helper_spec.rb b/ee/spec/helpers/search_helper_spec.rb index 127866a322dbd327841b98a60b526ccbeb318715..e9ff96d4483b77855e0452c70258bc8ecb2c5fa3 100644 --- a/ee/spec/helpers/search_helper_spec.rb +++ b/ee/spec/helpers/search_helper_spec.rb @@ -209,7 +209,7 @@ end end - describe '#highlight_and_truncate_issue' do + describe '#highlight_and_truncate_issuable' do let(:description) { 'hello world' } let(:issue) { create(:issue, description: description) } let(:user) { create(:user) } @@ -221,7 +221,7 @@ end # Elasticsearch returns Elasticsearch::Model::HashWrapper class for the highlighting - subject { highlight_and_truncate_issue(issue, 'test', Elasticsearch::Model::HashWrapper.new(search_highlight)) } + subject { highlight_and_truncate_issuable(issue, 'test', Elasticsearch::Model::HashWrapper.new(search_highlight)) } context 'when description is not present' do let(:description) { nil } diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 3e0fc4509b1a5d98a8161ff38abf8238152e6d97..2a3d8c23e89d735de5518dd2ea63faf84fae7e23 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -16,7 +16,7 @@ msgstr "" "Content-Transfer-Encoding: 8bit\n" "Plural-Forms: nplurals=INTEGER; plural=EXPRESSION;\n" -msgid " %{project_name}#%{issue_iid} · opened %{issue_created} by %{author}" +msgid " %{project_name}#%{issuable_iid} · opened %{issuable_created} by %{author}" msgstr "" msgid " %{start} to %{end}" diff --git a/spec/helpers/search_helper_spec.rb b/spec/helpers/search_helper_spec.rb index c31ef1aba286f579075d230ebe8bfc5e692013bb..34af3ce7e5e85603f006b6802108fd2516c055f3 100644 --- a/spec/helpers/search_helper_spec.rb +++ b/spec/helpers/search_helper_spec.rb @@ -477,7 +477,7 @@ def simple_sanitize(str) end end - describe '#highlight_and_truncate_issue' do + describe '#highlight_and_truncate_issuable' do let(:description) { 'hello world' } let(:issue) { create(:issue, description: description) } let(:user) { create(:user) } @@ -486,7 +486,7 @@ def simple_sanitize(str) allow(self).to receive(:current_user).and_return(user) end - subject { highlight_and_truncate_issue(issue, 'test', {}) } + subject { highlight_and_truncate_issuable(issue, 'test', {}) } context 'when description is not present' do let(:description) { nil } @@ -545,4 +545,38 @@ def simple_sanitize(str) end end end + + describe '#issuable_state_to_badge_class' do + context 'with merge request' do + it 'returns correct badge based on status' do + expect(issuable_state_to_badge_class(build(:merge_request, :merged))).to eq(:primary) + expect(issuable_state_to_badge_class(build(:merge_request, :closed))).to eq(:danger) + expect(issuable_state_to_badge_class(build(:merge_request, :opened))).to eq(:success) + end + end + + context 'with an issue' do + it 'returns correct badge based on status' do + expect(issuable_state_to_badge_class(build(:issue, :closed))).to eq(:info) + expect(issuable_state_to_badge_class(build(:issue, :opened))).to eq(:success) + end + end + end + + describe '#issuable_state_text' do + context 'with merge request' do + it 'returns correct badge based on status' do + expect(issuable_state_text(build(:merge_request, :merged))).to eq(_('Merged')) + expect(issuable_state_text(build(:merge_request, :closed))).to eq(_('Closed')) + expect(issuable_state_text(build(:merge_request, :opened))).to eq(_('Open')) + end + end + + context 'with an issue' do + it 'returns correct badge based on status' do + expect(issuable_state_text(build(:issue, :closed))).to eq(_('Closed')) + expect(issuable_state_text(build(:issue, :opened))).to eq(_('Open')) + end + end + end end diff --git a/spec/requests/search_controller_spec.rb b/spec/requests/search_controller_spec.rb index 700cdd96ba865755cff7e8a4298803e4cd7d7057..93a2fecab749e4c6196e602524d1348760287c07 100644 --- a/spec/requests/search_controller_spec.rb +++ b/spec/requests/search_controller_spec.rb @@ -45,14 +45,9 @@ def send_search_request(params) let(:object) { :merge_request } let(:creation_args) { { source_project: project, title: 'bar' } } let(:params) { { search: 'bar', scope: 'merge_requests' } } - # some N+1 queries exist - # each merge request require 4 extra queries for: - # - one for projects - # - one for namespaces - # - two for routes - # plus 4 additional queries run for the logged in user: + # there are 4 additional queries run for the logged in user: # - (1) geo_nodes, (1) users, (2) broadcast_messages - let(:threshold) { 16 } + let(:threshold) { 4 } it_behaves_like 'an efficient database result' end