Merge branch 'ce-to-ee-2018-08-24' into 'master'

CE upstream - 2018-08-24 15:22 UTC

See merge request gitlab-org/gitlab-ee!6979

app/assets/javascripts/awards_handler.js

@@ -109,8 +109,6 @@ export class AwardsHandler {
     }
 
     const $menu = $(`.${this.menuClass}`);
-    const $thumbsBtn = $menu.find('[data-name="thumbsup"], [data-name="thumbsdown"]').parent();
-    const $userAuthored = this.isUserAuthored($addBtn);
     if ($menu.length) {
       if ($menu.is('.is-visible')) {
         $addBtn.removeClass('is-active');
@@ -134,9 +132,6 @@ export class AwardsHandler {
         }, 200);
       });
     }
-
-    $thumbsBtn.toggleClass('disabled', $userAuthored);
-    $thumbsBtn.prop('disabled', $userAuthored);
   }
 
   // Create the emoji menu with the first category of emojis.
@@ -364,10 +359,6 @@ export class AwardsHandler {
     return $emojiButton.hasClass('active');
   }
 
-  isUserAuthored($button) {
-    return $button.hasClass('js-user-authored');
-  }
-
   decrementCounter($emojiButton, emoji) {
     const counter = $('.js-counter', $emojiButton);
     const counterNumber = parseInt(counter.text(), 10);
@@ -474,20 +465,16 @@ export class AwardsHandler {
   }
 
   postEmoji($emojiButton, awardUrl, emoji, callback) {
-    if (this.isUserAuthored($emojiButton)) {
-      this.userAuthored($emojiButton);
-    } else {
-      axios
-        .post(awardUrl, {
-          name: emoji,
-        })
-        .then(({ data }) => {
-          if (data.ok) {
-            callback();
-          }
-        })
-        .catch(() => flash(__('Something went wrong on our end.')));
-    }
+    axios
+      .post(awardUrl, {
+        name: emoji,
+      })
+      .then(({ data }) => {
+        if (data.ok) {
+          callback();
+        }
+      })
+      .catch(() => flash(__('Something went wrong on our end.')));
   }
 
   findEmojiIcon(votesBlock, emoji) {

app/assets/javascripts/notes/components/note_awards_list.vue

@@ -82,29 +82,17 @@ export default {
     getAwardHTML(name) {
       return glEmojiTag(name);
     },
-    getAwardClassBindings(awardList, awardName) {
+    getAwardClassBindings(awardList) {
       return {
         active: this.hasReactionByCurrentUser(awardList),
-        disabled: !this.canInteractWithEmoji(awardList, awardName),
+        disabled: !this.canInteractWithEmoji(),
       };
     },
-    canInteractWithEmoji(awardList, awardName) {
-      let isAllowed = true;
-      const restrictedEmojis = ['thumbsup', 'thumbsdown'];
-
-      // Users can not add :+1: and :-1: to their own notes
-      if (
-        this.getUserData.id === this.noteAuthorId &&
-        restrictedEmojis.indexOf(awardName) > -1
-      ) {
-        isAllowed = false;
-      }
-
-      return this.getUserData.id && isAllowed;
+    canInteractWithEmoji() {
+      return this.getUserData.id;
     },
     hasReactionByCurrentUser(awardList) {
-      return awardList.filter(award => award.user.id === this.getUserData.id)
-        .length;
+      return awardList.filter(award => award.user.id === this.getUserData.id).length;
     },
     awardTitle(awardsList) {
       const hasReactionByCurrentUser = this.hasReactionByCurrentUser(
@@ -197,7 +185,7 @@ export default {
         v-tooltip
         v-for="(awardList, awardName, index) in groupedAwards"
         :key="index"
-        :class="getAwardClassBindings(awardList, awardName)"
+        :class="getAwardClassBindings(awardList)"
         :title="awardTitle(awardList)"
         class="btn award-control"
         data-boundary="viewport"

app/assets/stylesheets/framework/variables.scss

@@ -673,7 +673,6 @@ $dropdown-animation-timing: cubic-bezier(0.23, 1, 0.32, 1);
 /*
 GitLab Plans
 */
-
 $gl-gold-plan: #d4af37;
 $gl-silver-plan: #91a1ab;
 $gl-bronze-plan: #cd7f32;
@@ -681,7 +680,6 @@ $gl-bronze-plan: #cd7f32;
 /*
 Cross-project Pipelines
  */
-
 $linked-project-column-margin: 60px;
 
 /*

app/controllers/concerns/toggle_award_emoji.rb

@@ -5,7 +5,7 @@ module ToggleAwardEmoji
     authenticate_user!
     name = params.require(:name)
 
-    if awardable.user_can_award?(current_user, name)
+    if awardable.user_can_award?(current_user)
       awardable.toggle_award_emoji(name, current_user)
 
       todoable = to_todoable(awardable)

app/controllers/projects/issues_controller.rb

@@ -117,7 +117,7 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def referenced_merge_requests
-    @merge_requests, @closed_by_merge_requests = ::Issues::FetchReferencedMergeRequestsService.new(project, current_user).execute(issue)
+    @merge_requests, @closed_by_merge_requests = ::Issues::ReferencedMergeRequestsService.new(project, current_user).execute(issue)
 
     respond_to do |format|
       format.json do

app/helpers/issues_helper.rb

@@ -94,14 +94,6 @@ module IssuesHelper
     end
   end
 
-  def award_user_authored_class(award)
-    if award == 'thumbsdown' || award == 'thumbsup'
-      'user-authored js-user-authored'
-    else
-      ''
-    end
-  end
-
   def awards_sort(awards)
     awards.sort_by do |award, award_emojis|
       if award == "thumbsup"

app/models/concerns/awardable.rb

@@ -76,12 +76,8 @@ module Awardable
     true
   end
 
-  def awardable_votes?(name)
-    AwardEmoji::UPVOTE_NAME == name || AwardEmoji::DOWNVOTE_NAME == name
-  end
-
-  def user_can_award?(current_user, name)
-    awardable_by_user?(current_user, name) && Ability.allowed?(current_user, :award_emoji, self)
+  def user_can_award?(current_user)
+    Ability.allowed?(current_user, :award_emoji, self)
   end
 
   def user_authored?(current_user)
@@ -117,12 +113,4 @@ module Awardable
   def normalize_name(name)
     Gitlab::Emoji.normalize_emoji_name(name)
   end
-
-  def awardable_by_user?(current_user, name)
-    if user_authored?(current_user)
-      !awardable_votes?(normalize_name(name))
-    else
-      true
-    end
-  end
 end

app/models/issue.rb

@@ -177,27 +177,6 @@ class Issue < ActiveRecord::Base
     "#{project.to_reference(from, full: full)}#{reference}"
   end
 
-  def referenced_merge_requests(current_user = nil)
-    ext = all_references(current_user)
-
-    notes_with_associations.each do |object|
-      object.all_references(current_user, extractor: ext)
-    end
-
-    merge_requests = ext.merge_requests.sort_by(&:iid)
-
-    cross_project_filter = -> (merge_requests) do
-      merge_requests.select { |mr| mr.target_project == project }
-    end
-
-    Ability.merge_requests_readable_by_user(
-      merge_requests, current_user,
-      filters: {
-        read_cross_project: cross_project_filter
-      }
-    )
-  end
-
   # All branches containing the current issue's ID, except for
   # those with a merge request open referencing the current issue.
   def related_branches(current_user)
@@ -205,7 +184,11 @@ class Issue < ActiveRecord::Base
       branch =~ /\A#{iid}-(?!\d+-stable)/i
     end
 
-    branches_with_merge_request = self.referenced_merge_requests(current_user).map(&:source_branch)
+    branches_with_merge_request =
+      Issues::ReferencedMergeRequestsService
+        .new(project, current_user)
+        .referenced_merge_requests(self)
+        .map(&:source_branch)
 
     branches_with_iid - branches_with_merge_request
   end
@@ -249,26 +232,6 @@ class Issue < ActiveRecord::Base
     project
   end
 
-  # From all notes on this issue, we'll select the system notes about linked
-  # merge requests. Of those, the MRs closing `self` are returned.
-  def closed_by_merge_requests(current_user = nil)
-    return [] unless open?
-
-    ext = all_references(current_user)
-
-    notes.system.each do |note|
-      note.all_references(current_user, extractor: ext)
-    end
-
-    merge_requests = ext.merge_requests.select(&:open?)
-    if merge_requests.any?
-      ids = MergeRequestsClosingIssues.where(merge_request_id: merge_requests.map(&:id), issue_id: id).pluck(:merge_request_id)
-      merge_requests.select { |mr| mr.id.in?(ids) }
-    else
-      []
-    end
-  end
-
   def moved?
     !moved_to.nil?
   end

app/models/remote_mirror.rb

@@ -152,6 +152,15 @@ class RemoteMirror < ActiveRecord::Base
     result.to_s
   end
 
+  def ensure_remote!
+    return unless project
+    return unless remote_name && url
+
+    # If this fails or the remote already exists, we won't know due to
+    # https://gitlab.com/gitlab-org/gitaly/issues/1317
+    project.repository.add_remote(remote_name, url)
+  end
+
   private
 
   def raw

app/services/issues/fetch_referenced_merge_requests_service.rb

-# frozen_string_literal: true
-
-module Issues
-  class FetchReferencedMergeRequestsService < Issues::BaseService
-    def execute(issue)
-      referenced_merge_requests = issue.referenced_merge_requests(current_user)
-      referenced_merge_requests = Gitlab::IssuableSorter.sort(project, referenced_merge_requests) { |i| i.iid.to_s }
-      closed_by_merge_requests = issue.closed_by_merge_requests(current_user)
-      closed_by_merge_requests = Gitlab::IssuableSorter.sort(project, closed_by_merge_requests) { |i| i.iid.to_s }
-
-      [referenced_merge_requests, closed_by_merge_requests]
-    end
-  end
-end

app/services/issues/referenced_merge_requests_service.rb

+# frozen_string_literal: true
+
+module Issues
+  class ReferencedMergeRequestsService < Issues::BaseService
+    def execute(issue)
+      referenced = referenced_merge_requests(issue)
+      closed_by = closed_by_merge_requests(issue)
+      preloader = ActiveRecord::Associations::Preloader.new
+
+      preloader.preload(referenced + closed_by,
+                        head_pipeline: { project: [:route, { namespace: :route }] })
+
+      [sort_by_iid(referenced), sort_by_iid(closed_by)]
+    end
+
+    def referenced_merge_requests(issue)
+      merge_requests = extract_merge_requests(issue)
+
+      cross_project_filter = -> (merge_requests) do
+        merge_requests.select { |mr| mr.target_project == project }
+      end
+
+      Ability.merge_requests_readable_by_user(
+        merge_requests,
+        current_user,
+        filters: {
+          read_cross_project: cross_project_filter
+        }
+      )
+    end
+
+    def closed_by_merge_requests(issue)
+      return [] unless issue.open?
+
+      merge_requests = extract_merge_requests(issue, filter: :system).select(&:open?)
+
+      return [] if merge_requests.empty?
+
+      ids = MergeRequestsClosingIssues.where(merge_request_id: merge_requests.map(&:id), issue_id: issue.id).pluck(:merge_request_id)
+      merge_requests.select { |mr| mr.id.in?(ids) }
+    end
+
+    private
+
+    def extract_merge_requests(issue, filter: nil)
+      ext = issue.all_references(current_user)
+      notes = issue_notes(issue)
+      notes = notes.select(&filter) if filter
+
+      notes.each do |note|
+        note.all_references(current_user, extractor: ext)
+      end
+
+      ext.merge_requests
+    end
+
+    def issue_notes(issue)
+      @issue_notes ||= {}
+      @issue_notes[issue] ||= issue.notes.includes(:author)
+    end
+
+    def sort_by_iid(merge_requests)
+      Gitlab::IssuableSorter.sort(project, merge_requests) { |mr| mr.iid.to_s }
+    end
+  end
+end

app/services/projects/update_remote_mirror_service.rb

@@ -10,6 +10,7 @@ module Projects
       return success unless remote_mirror.enabled?
 
       begin
+        remote_mirror.ensure_remote!
         repository.fetch_remote(remote_mirror.remote_name, no_tags: true)
 
         opts = {}

app/services/quick_actions/interpret_service.rb

@@ -403,7 +403,7 @@ module QuickActions
       match[1] if match
     end
     command :award do |name|
-      if name && issuable.user_can_award?(current_user, name)
+      if name && issuable.user_can_award?(current_user)
         @updates[:emoji_award] = name
       end
     end

app/views/award_emoji/_awards_block.html.haml

@@ -3,7 +3,7 @@
 .awards.js-awards-block{ class: ("hidden" if !inline && grouped_emojis.empty?), data: { award_url: toggle_award_url(awardable) } }
   - awards_sort(grouped_emojis).each do |emoji, awards|
     %button.btn.award-control.js-emoji-btn.has-tooltip{ type: "button",
-      class: [(award_state_class(awardable, awards, current_user)), (award_user_authored_class(emoji) if user_authored)],
+      class: [(award_state_class(awardable, awards, current_user))],
       data: { placement: "bottom", title: award_user_list(awards, current_user) } }
       = emoji_icon(emoji)
       %span.award-control-text.js-counter
@@ -13,7 +13,6 @@
     .award-menu-holder.js-award-holder
       %button.btn.award-control.has-tooltip.js-add-award{ type: 'button',
         'aria-label': _('Add reaction'),
-        class: ("js-user-authored" if user_authored),
         data: { title: _('Add reaction'), placement: "bottom" } }
         %span{ class: "award-control-icon award-control-icon-neutral" }= custom_icon('emoji_slightly_smiling_face')
         %span{ class: "award-control-icon award-control-icon-positive" }= custom_icon('emoji_smiley')

app/views/projects/notes/_actions.html.haml

@@ -40,7 +40,7 @@
   - if note.emoji_awardable?
     - user_authored = note.user_authored?(current_user)
     .note-actions-item
-      = button_tag title: 'Add reaction', class: "note-action-button note-emoji-button js-add-award js-note-emoji #{'js-user-authored' if user_authored} has-tooltip btn btn-transparent", data: { position: 'right', container: 'body' } do
+      = button_tag title: 'Add reaction', class: "note-action-button note-emoji-button js-add-award js-note-emoji} has-tooltip btn btn-transparent", data: { position: 'right', container: 'body' } do
         = icon('spinner spin')
         %span{ class: 'link-highlight award-control-icon-neutral' }= custom_icon('emoji_slightly_smiling_face')
         %span{ class: 'link-highlight award-control-icon-positive' }= custom_icon('emoji_smiley')

app/views/snippets/notes/_actions.html.haml

@@ -2,7 +2,7 @@
   - if note.emoji_awardable?
     - user_authored = note.user_authored?(current_user)
     .note-actions-item
-      = link_to '#', title: 'Add reaction', class: "note-action-button note-emoji-button js-add-award js-note-emoji #{'js-user-authored' if user_authored} has-tooltip", data: { position: 'right' } do
+      = link_to '#', title: 'Add reaction', class: "note-action-button note-emoji-button js-add-award js-note-emoji has-tooltip", data: { position: 'right' } do
         = icon('spinner spin')
         %span{ class: 'link-highlight award-control-icon-neutral' }= custom_icon('emoji_slightly_smiling_face')
         %span{ class: 'link-highlight award-control-icon-positive' }= custom_icon('emoji_smiley')

changelogs/unreleased/43096-controller-projects-issuescontroller-referenced_merge_requests-json-executes-more-than-100-sql-queries.yml

+---
+title: Improve performance when fetching related merge requests for an issue
+merge_request: 21237
+author:
+type: performance

changelogs/unreleased/ccr-43283_allow_author_upvote.yml

+---
+title: Allow author to vote on their own issue and MRs
+merge_request: 21203
+author:
+type: changed

changelogs/unreleased/sh-fix-issue-50562.yml

+---
+title: Fix remote mirrors failing if Git remotes have not been added
+merge_request: 21351
+author:
+type: fixed

doc/development/README.md

@@ -56,6 +56,7 @@ description: 'Learn how to contribute to GitLab.'
 - [Performance guidelines](performance.md)
 - [Merge request performance guidelines](merge_request_performance_guidelines.md)
   for ensuring merge requests do not negatively impact GitLab performance
+- [Profiling](profiling.md) for profiling a URL
 
 ## Database guides
 

doc/development/architecture.md

@@ -70,7 +70,7 @@ The add-on component gitlab-shell serves repositories over SSH. It manages the S
 
 Gitaly executes git operations from gitlab-shell and the GitLab web app, and provides an API to the GitLab web app to get attributes from git (e.g. title, branches, tags, other meta data), and to get blobs (e.g. diffs, commits, files).
 
-You may also be interested in the [production architecture of GitLab.com](https://about.gitlab.com/handbook/infrastructure/production-architecture/).
+You may also be interested in the [production architecture of GitLab.com](https://about.gitlab.com/handbook/engineering/infrastructure/production-architecture/).
 
 ### Installation Folder Summary
 

doc/install/installation.md

@@ -457,11 +457,35 @@ GitLab-Pages uses [GNU Make](https://www.gnu.org/software/make/). This step is o
     sudo -u git -H git checkout v$(</home/git/gitlab/GITLAB_PAGES_VERSION)
     sudo -u git -H make
 
+### Install Gitaly
+
+    # Fetch Gitaly source with Git and compile with Go
+    sudo -u git -H bundle exec rake "gitlab:gitaly:install[/home/git/gitaly]" RAILS_ENV=production
+
+You can specify a different Git repository by providing it as an extra parameter:
+
+    sudo -u git -H bundle exec rake "gitlab:gitaly:install[/home/git/gitaly,https://example.com/gitaly.git]" RAILS_ENV=production
+
+Next, make sure gitaly configured:
+
+    # Restrict Gitaly socket access
+    sudo chmod 0700 /home/git/gitlab/tmp/sockets/private
+    sudo chown git /home/git/gitlab/tmp/sockets/private
+
+    # If you are using non-default settings you need to update config.toml
+    cd /home/git/gitaly
+    sudo -u git -H editor config.toml
+
+For more information about configuring Gitaly see
+[doc/administration/gitaly](../administration/gitaly).
+
 ### Initialize Database and Activate Advanced Features
 
     sudo -u git -H bundle exec rake gitlab:setup RAILS_ENV=production
-
     # Type 'yes' to create the database tables.
+    
+    # or you can skip the question by adding force=yes
+    sudo -u git -H bundle exec rake gitlab:setup RAILS_ENV=production force=yes
 
     # When done you see 'Administrator account created:'
 
@@ -491,28 +515,6 @@ Make GitLab start on boot:
 
     sudo update-rc.d gitlab defaults 21
 
-### Install Gitaly
-
-    # Fetch Gitaly source with Git and compile with Go
-    sudo -u git -H bundle exec rake "gitlab:gitaly:install[/home/git/gitaly,/home/git/repositories]" RAILS_ENV=production
-
-You can specify a different Git repository by providing it as an extra parameter:
-
-    sudo -u git -H bundle exec rake "gitlab:gitaly:install[/home/git/gitaly,/home/git/repositories,https://example.com/gitaly.git]" RAILS_ENV=production
-
-Next, make sure gitaly configured:
-
-    # Restrict Gitaly socket access
-    sudo chmod 0700 /home/git/gitlab/tmp/sockets/private
-    sudo chown git /home/git/gitlab/tmp/sockets/private
-
-    # If you are using non-default settings you need to update config.toml
-    cd /home/git/gitaly
-    sudo -u git -H editor config.toml
-
-For more information about configuring Gitaly see
-[doc/administration/gitaly](../administration/gitaly).
-
 ### Setup Logrotate
 
     sudo cp lib/support/logrotate/gitlab /etc/logrotate.d/gitlab

lib/api/award_emoji.rb

@@ -100,7 +100,7 @@ module API
       end
 
       def can_award_awardable?
-        awardable.user_can_award?(current_user, params[:name])
+        awardable.user_can_award?(current_user)
       end
 
       def awardable

lib/banzai/reference_parser/merge_request_parser.rb

@@ -14,11 +14,12 @@ module Banzai
               # Eager loading these ensures we don't end up running dozens of
               # queries in this process.
               target_project: [
-                { namespace: :owner },
+                { namespace: [:owner, :route] },
                 { group: [:owners, :group_members] },
                 :invited_groups,
                 :project_members,
-                :project_feature
+                :project_feature,
+                :route
               ]
             }),
           self.class.data_attribute

spec/javascripts/badges/dummy_badge.js

+import _ from 'underscore';
 import { PROJECT_BADGE } from '~/badges/constants';
 import { DUMMY_IMAGE_URL, TEST_HOST } from 'spec/test_constants';
 
 export const createDummyBadge = () => {
-  const id = Math.floor(1000 * Math.random());
+  const id = _.uniqueId();
   return {
     id,
     imageUrl: `${TEST_HOST}/badges/${id}/image/url`,

spec/models/concerns/awardable_spec.rb

@@ -53,21 +53,14 @@ describe Awardable do
       issue.project.add_guest(user)
     end
 
-    it 'does not allow upvoting or downvoting your own issue' do
-      issue.update!(author: user)
-
-      expect(issue.user_can_award?(user, AwardEmoji::DOWNVOTE_NAME)).to be_falsy
-      expect(issue.user_can_award?(user, AwardEmoji::UPVOTE_NAME)).to be_falsy
-    end
-
     it 'is truthy when the user is allowed to award emoji' do
-      expect(issue.user_can_award?(user, AwardEmoji::UPVOTE_NAME)).to be_truthy
+      expect(issue.user_can_award?(user)).to be_truthy
     end
 
     it 'is falsy when the project is archived' do
       issue.project.update!(archived: true)
 
-      expect(issue.user_can_award?(user, AwardEmoji::UPVOTE_NAME)).to be_falsy
+      expect(issue.user_can_award?(user)).to be_falsy
     end
   end
 

spec/models/issue_spec.rb

@@ -188,98 +188,6 @@ describe Issue do
     end
   end
 
-  describe '#closed_by_merge_requests' do
-    let(:project) { create(:project, :repository) }
-    let(:issue) { create(:issue, project: project)}
-    let(:closed_issue) { build(:issue, :closed, project: project)}
-
-    let(:mr) do
-      opts = {
-        title: 'Awesome merge_request',
-        description: "Fixes #{issue.to_reference}",
-        source_branch: 'feature',
-        target_branch: 'master'
-      }
-      MergeRequests::CreateService.new(project, project.owner, opts).execute
-    end
-
-    let(:closed_mr) do
-      opts = {
-        title: 'Awesome merge_request 2',
-        description: "Fixes #{issue.to_reference}",
-        source_branch: 'feature',
-        target_branch: 'master',
-        state: 'closed'
-      }
-      MergeRequests::CreateService.new(project, project.owner, opts).execute
-    end
-
-    it 'returns the merge request to close this issue' do
-      expect(issue.closed_by_merge_requests(mr.author)).to eq([mr])
-    end
-
-    it "returns an empty array when the merge request is closed already" do
-      expect(issue.closed_by_merge_requests(closed_mr.author)).to eq([])
-    end
-
-    it "returns an empty array when the current issue is closed already" do
-      expect(closed_issue.closed_by_merge_requests(closed_issue.author)).to eq([])
-    end
-  end
-
-  describe '#referenced_merge_requests' do
-    let(:project) { create(:project, :public) }
-    let(:issue) do
-      create(:issue, description: merge_request.to_reference, project: project)
-    end
-    let!(:merge_request) do
-      create(:merge_request,
-             source_project: project,
-             source_branch:  'master',
-             target_branch:  'feature')
-    end
-
-    it 'returns the referenced merge requests' do
-      mr2 = create(:merge_request,
-                   source_project: project,
-                   source_branch:  'feature',
-                   target_branch:  'master')
-
-      create(:note_on_issue,
-             noteable:   issue,
-             note:       mr2.to_reference,
-             project_id: project.id)
-
-      expect(issue.referenced_merge_requests).to eq([merge_request, mr2])
-    end
-
-    it 'returns cross project referenced merge requests' do
-      other_project = create(:project, :public)
-      cross_project_merge_request = create(:merge_request, source_project: other_project)
-      create(:note_on_issue,
-             noteable:   issue,
-             note:       cross_project_merge_request.to_reference(issue.project),
-             project_id: issue.project.id)
-
-      expect(issue.referenced_merge_requests).to eq([merge_request, cross_project_merge_request])
-    end
-
-    it 'excludes cross project references if the user cannot read cross project' do
-      user = create(:user)
-      allow(Ability).to receive(:allowed?).and_call_original
-      expect(Ability).to receive(:allowed?).with(user, :read_cross_project) { false }
-
-      other_project = create(:project, :public)
-      cross_project_merge_request = create(:merge_request, source_project: other_project)
-      create(:note_on_issue,
-             noteable:   issue,
-             note:       cross_project_merge_request.to_reference(issue.project),
-             project_id: issue.project.id)
-
-      expect(issue.referenced_merge_requests(user)).to eq([merge_request])
-    end
-  end
-
   describe '#can_move?' do
     let(:user) { create(:user) }
     let(:issue) { create(:issue) }
@@ -365,7 +273,12 @@ describe Issue do
                                                source_project: subject.project,
                                                source_branch: "#{subject.iid}-branch" })
       merge_request.create_cross_references!(user)
-      expect(subject.referenced_merge_requests(user)).not_to be_empty
+
+      referenced_merge_requests = Issues::ReferencedMergeRequestsService
+                                    .new(subject.project, user)
+                                    .referenced_merge_requests(subject)
+
+      expect(referenced_merge_requests).not_to be_empty
       expect(subject.related_branches(user)).to eq([subject.to_branch_name])
     end
 

spec/models/remote_mirror_spec.rb

@@ -220,6 +220,18 @@ describe RemoteMirror do
     end
   end
 
+  context '#ensure_remote!' do
+    let(:remote_mirror) { create(:project, :repository, :remote_mirror).remote_mirrors.first }
+
+    it 'adds a remote multiple times with no errors' do
+      expect(remote_mirror.project.repository).to receive(:add_remote).with(remote_mirror.remote_name, remote_mirror.url).twice.and_call_original
+
+      2.times do
+        remote_mirror.ensure_remote!
+      end
+    end
+  end
+
   context '#updated_since?' do
     let(:remote_mirror) { create(:project, :repository, :remote_mirror).remote_mirrors.first }
     let(:timestamp) { Time.now - 5.minutes }

spec/requests/api/award_emoji_spec.rb

@@ -167,12 +167,6 @@ describe API::AwardEmoji do
         expect(response).to have_gitlab_http_status(401)
       end
 
-      it "returns a 404 error if the user authored issue" do
-        post api("/projects/#{project.id}/issues/#{issue2.id}/award_emoji", user), name: 'thumbsup'
-
-        expect(response).to have_gitlab_http_status(404)
-      end
-
       it "normalizes +1 as thumbsup award" do
         post api("/projects/#{project.id}/issues/#{issue.iid}/award_emoji", user), name: '+1'
 
@@ -215,12 +209,6 @@ describe API::AwardEmoji do
       expect(json_response['user']['username']).to eq(user.username)
     end
 
-    it "it returns 404 error when user authored note" do
-      post api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{note2.id}/award_emoji", user), name: 'thumbsup'
-
-      expect(response).to have_gitlab_http_status(404)
-    end
-
     it "normalizes +1 as thumbsup award" do
       post api("/projects/#{project.id}/issues/#{issue.iid}/notes/#{note.id}/award_emoji", user), name: '+1'
 

spec/services/issues/fetch_referenced_merge_requests_service_spec.rb

-require 'spec_helper.rb'
-
-describe Issues::FetchReferencedMergeRequestsService do
-  let(:project) { create(:project) }
-  let(:issue) { create(:issue, project: project) }
-  let(:other_project) { create(:project) }
-
-  let(:mr) { create(:merge_request, source_project: project, target_project: project, id: 2)}
-  let(:other_mr) { create(:merge_request, source_project: other_project, target_project: other_project, id: 1)}
-
-  let(:user) { create(:user) }
-  let(:service) { described_class.new(project, user) }
-
-  context 'with mentioned merge requests' do
-    it 'returns a list of sorted merge requests' do
-      allow(issue).to receive(:referenced_merge_requests).with(user).and_return([other_mr, mr])
-
-      mrs, closed_by_mrs = service.execute(issue)
-
-      expect(mrs).to match_array([mr, other_mr])
-      expect(closed_by_mrs).to match_array([])
-    end
-  end
-
-  context 'with closed-by merge requests' do
-    it 'returns a list of sorted merge requests' do
-      allow(issue).to receive(:closed_by_merge_requests).with(user).and_return([other_mr, mr])
-
-      mrs, closed_by_mrs = service.execute(issue)
-
-      expect(mrs).to match_array([])
-      expect(closed_by_mrs).to match_array([mr, other_mr])
-    end
-  end
-end

spec/services/issues/referenced_merge_requests_service_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper.rb'
+
+describe Issues::ReferencedMergeRequestsService do
+  def create_referencing_mr(attributes = {})
+    create(:merge_request, attributes).tap do |merge_request|
+      create(:note, :system, project: project, noteable: issue, author: user, note: merge_request.to_reference(full: true))
+    end
+  end
+
+  def create_closing_mr(attributes = {})
+    create_referencing_mr(attributes).tap do |merge_request|
+      create(:merge_requests_closing_issues, issue: issue, merge_request: merge_request)
+    end
+  end
+
+  set(:user) { create(:user) }
+  set(:project) { create(:project, :public, :repository) }
+  set(:other_project) { create(:project, :public, :repository) }
+  set(:issue) { create(:issue, author: user, project: project) }
+
+  set(:closing_mr) { create_closing_mr(source_project: project) }
+  set(:closing_mr_other_project) { create_closing_mr(source_project: other_project) }
+
+  set(:referencing_mr) { create_referencing_mr(source_project: project, source_branch: 'csv') }
+  set(:referencing_mr_other_project) { create_referencing_mr(source_project: other_project, source_branch: 'csv') }
+
+  let(:service) { described_class.new(project, user) }
+
+  describe '#execute' do
+    it 'returns a list of sorted merge requests' do
+      mrs, closed_by_mrs = service.execute(issue)
+
+      expect(mrs).to eq([closing_mr, referencing_mr, closing_mr_other_project, referencing_mr_other_project])
+      expect(closed_by_mrs).to eq([closing_mr, closing_mr_other_project])
+    end
+
+    context 'performance' do
+      it 'does not run extra queries when extra namespaces are included', :use_clean_rails_memory_store_caching do
+        service.execute(issue) # warm cache
+        control_count = ActiveRecord::QueryRecorder.new { service.execute(issue) }.count
+
+        third_project = create(:project, :public)
+        create_closing_mr(source_project: third_project)
+        service.execute(issue) # warm cache
+
+        expect { service.execute(issue) }.not_to exceed_query_limit(control_count)
+      end
+
+      it 'preloads the head pipeline for each merge request, and its routes' do
+        # Hack to ensure no data is preserved on issue before starting the spec,
+        # to avoid false negatives
+        reloaded_issue = Issue.find(issue.id)
+
+        pipeline_routes = lambda do |merge_requests|
+          merge_requests.map { |mr| mr.head_pipeline&.project&.full_path }
+        end
+
+        closing_mr_other_project.update!(head_pipeline: create(:ci_pipeline))
+        control_count = ActiveRecord::QueryRecorder.new { service.execute(reloaded_issue).each(&pipeline_routes) }
+
+        closing_mr.update!(head_pipeline: create(:ci_pipeline))
+
+        expect { service.execute(issue).each(&pipeline_routes) }
+          .not_to exceed_query_limit(control_count)
+      end
+
+      it 'only loads issue notes once' do
+        expect(issue).to receive(:notes).once.and_call_original
+
+        service.execute(issue)
+      end
+    end
+  end
+
+  describe '#referenced_merge_requests' do
+    it 'returns the referenced merge requests' do
+      expect(service.referenced_merge_requests(issue)).to match_array([
+                                                                        closing_mr,
+                                                                        closing_mr_other_project,
+                                                                        referencing_mr,
+                                                                        referencing_mr_other_project
+                                                                      ])
+    end
+
+    it 'excludes cross project references if the user cannot read cross project' do
+      allow(Ability).to receive(:allowed?).and_call_original
+      expect(Ability).to receive(:allowed?).with(user, :read_cross_project).at_least(:once).and_return(false)
+
+      expect(service.referenced_merge_requests(issue)).not_to include(closing_mr_other_project)
+      expect(service.referenced_merge_requests(issue)).not_to include(referencing_mr_other_project)
+    end
+
+    context 'performance' do
+      it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do
+        service.referenced_merge_requests(issue) # warm cache
+        control_count = ActiveRecord::QueryRecorder.new { service.referenced_merge_requests(issue) }.count
+
+        create(:note, project: project, noteable: issue, author: create(:user))
+        service.referenced_merge_requests(issue) # warm cache
+
+        expect { service.referenced_merge_requests(issue) }.not_to exceed_query_limit(control_count)
+      end
+    end
+  end
+
+  describe '#closed_by_merge_requests' do
+    let(:closed_issue) { build(:issue, :closed, project: project)}
+
+    it 'returns the open merge requests that close this issue' do
+      create_closing_mr(source_project: project, state: 'closed')
+
+      expect(service.closed_by_merge_requests(issue)).to match_array([closing_mr, closing_mr_other_project])
+    end
+
+    it 'returns an empty array when the current issue is closed already' do
+      expect(service.closed_by_merge_requests(closed_issue)).to eq([])
+    end
+
+    context 'performance' do
+      it 'does not run a query for each note author', :use_clean_rails_memory_store_caching do
+        service.closed_by_merge_requests(issue) # warm cache
+        control_count = ActiveRecord::QueryRecorder.new { service.closed_by_merge_requests(issue) }.count
+
+        create(:note, :system, project: project, noteable: issue, author: create(:user))
+        service.closed_by_merge_requests(issue) # warm cache
+
+        expect { service.closed_by_merge_requests(issue) }.not_to exceed_query_limit(control_count)
+      end
+    end
+  end
+end

spec/services/projects/update_remote_mirror_service_spec.rb

@@ -18,6 +18,7 @@ describe Projects::UpdateRemoteMirrorService do
     end
 
     it "fetches the remote repository" do
+      expect(remote_mirror).to receive(:ensure_remote!).and_call_original
       expect(repository).to receive(:fetch_remote).with(remote_mirror.remote_name, no_tags: true) do
         sync_remote(repository, remote_mirror.remote_name, local_branch_names)
       end

spec/support/helpers/cycle_analytics_helpers.rb

@@ -65,7 +65,9 @@ module CycleAnalyticsHelpers
   end
 
   def merge_merge_requests_closing_issue(user, project, issue)
-    merge_requests = issue.closed_by_merge_requests(user)
+    merge_requests = Issues::ReferencedMergeRequestsService
+                       .new(project, user)
+                       .closed_by_merge_requests(issue)
 
     merge_requests.each { |merge_request| MergeRequests::MergeService.new(project, user).execute(merge_request) }
   end