From 6a10ed3ff2ddd0e13fe22dbd43c3dcd5e3083910 Mon Sep 17 00:00:00 2001
From: Jose <jvargas@gitlab.com>
Date: Wed, 9 May 2018 16:43:46 -0500
Subject: [PATCH 01/25] Add dot to separate system notes content

---
 app/assets/javascripts/notes/components/note_header.vue      | 5 ++++-
 .../javascripts/vue_shared/components/time_ago_tooltip.vue   | 4 ++--
 app/assets/stylesheets/pages/notes.scss                      | 4 ++++
 app/views/shared/notes/_note.html.haml                       | 5 +++--
 changelogs/unreleased/jivl-add-dot-system-notes.yml          | 5 +++++
 5 files changed, 18 insertions(+), 5 deletions(-)
 create mode 100644 changelogs/unreleased/jivl-add-dot-system-notes.yml

diff --git a/app/assets/javascripts/notes/components/note_header.vue b/app/assets/javascripts/notes/components/note_header.vue
index c3d1ef1fcc6..76028f7f3f7 100644
--- a/app/assets/javascripts/notes/components/note_header.vue
+++ b/app/assets/javascripts/notes/components/note_header.vue
@@ -78,10 +78,13 @@ export default {
           v-html="actionTextHtml"
           class="system-note-message">
         </span>
+        <span class="system-note-separator">
+          &middot;
+        </span>
         <a
           :href="noteTimestampLink"
           @click="updateTargetNoteHash"
-          class="note-timestamp">
+          class="note-timestamp system-note-separator">
           <time-ago-tooltip
             :time="createdAt"
             tooltip-placement="bottom"
diff --git a/app/assets/javascripts/vue_shared/components/time_ago_tooltip.vue b/app/assets/javascripts/vue_shared/components/time_ago_tooltip.vue
index bec4e7c99b6..368eeb6c453 100644
--- a/app/assets/javascripts/vue_shared/components/time_ago_tooltip.vue
+++ b/app/assets/javascripts/vue_shared/components/time_ago_tooltip.vue
@@ -40,7 +40,7 @@ export default {
     :class="cssClass"
     :title="tooltipTitle(time)"
     :data-placement="tooltipPlacement"
-    data-container="body">
-    {{ timeFormated(time) }}
+    data-container="body"
+    v-text="timeFormated(time)">
   </time>
 </template>
diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index 6d5c6cb136f..a8fd3aa6412 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -459,6 +459,10 @@ ul.notes {
     white-space: normal;
   }
 
+  .system-note-separator {
+    color: $gl-text-color-disabled;
+  }
+
   a:hover {
     text-decoration: underline;
   }
diff --git a/app/views/shared/notes/_note.html.haml b/app/views/shared/notes/_note.html.haml
index 893a7f26ebd..d4e67b5e7e3 100644
--- a/app/views/shared/notes/_note.html.haml
+++ b/app/views/shared/notes/_note.html.haml
@@ -41,8 +41,9 @@
               - if note.system
                 %span.system-note-message
                   = markdown_field(note, :note)
-              %a{ href: "##{dom_id(note)}" }
-                = time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago')
+              %span.system-note-separator
+                &middot;
+              %a.system-note-separator{ href: "##{dom_id(note)}" }= time_ago_with_tooltip(note.created_at, placement: 'bottom', html_class: 'note-created-ago')
         - unless note.system?
           .note-actions
             - if note.for_personal_snippet?
diff --git a/changelogs/unreleased/jivl-add-dot-system-notes.yml b/changelogs/unreleased/jivl-add-dot-system-notes.yml
new file mode 100644
index 00000000000..2246bab1464
--- /dev/null
+++ b/changelogs/unreleased/jivl-add-dot-system-notes.yml
@@ -0,0 +1,5 @@
+---
+title: Add dot to separate system notes content
+merge_request: 18864
+author:
+type: changed

From 6a20cf91229bed0935b77a7a1886c80c78e5e7a2 Mon Sep 17 00:00:00 2001
From: Paul Vorbach <paul@vorba.ch>
Date: Sat, 28 Apr 2018 22:49:31 +0200
Subject: [PATCH 02/25] Fix width of contributors graphs

The contributors graphs were sized with the full .content width in mind,
but at some point GitLab changed to a more narrow design, so graphs
would be too wide for their container divs. This change resizes
contributors graphs relative to their correct container widths.
---
 .../graphs/show/stat_graph_contributors_graph.js         | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js b/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
index a99ce0f1c36..d7f4cae971b 100644
--- a/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
+++ b/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
@@ -109,7 +109,7 @@ export const ContributorsMasterGraph = (function(superClass) {
 
     this.data = data1;
     this.update_content = this.update_content.bind(this);
-    this.width = $('.content').width() - parentPadding - (this.MARGIN.left + this.MARGIN.right);
+    this.width = $('.stat-graph').width() - parentPadding - (this.MARGIN.left + this.MARGIN.right);
     this.height = 200;
     this.x = null;
     this.y = null;
@@ -218,12 +218,15 @@ export const ContributorsAuthorGraph = (function(superClass) {
   extend(ContributorsAuthorGraph, superClass);
 
   function ContributorsAuthorGraph(data1) {
+    const $parentElements = $('.person');
+    const parentPadding = parseFloat($parentElements.css('padding-left')) + parseFloat($parentElements.css('padding-right'));
+
     this.data = data1;
     // Don't split graph size in half for mobile devices.
     if ($(window).width() < 768) {
-      this.width = $('.content').width() - 80;
+      this.width = $('.stat-graph').width() - parentPadding - (this.MARGIN.left + this.MARGIN.right);
     } else {
-      this.width = ($('.content').width() / 2) - 100;
+      this.width = ($('.stat-graph').width() / 2) - parentPadding - (this.MARGIN.left + this.MARGIN.right);
     }
     this.height = 200;
     this.x = null;

From 2b1321288a911d8c1336aff9fd712cef2592bfb5 Mon Sep 17 00:00:00 2001
From: Paul Vorbach <paul@vorba.ch>
Date: Sat, 28 Apr 2018 22:53:02 +0200
Subject: [PATCH 03/25] Use ES6 `const` more consistently

---
 .../graphs/show/stat_graph_contributors_graph.js  | 15 ++++++---------
 1 file changed, 6 insertions(+), 9 deletions(-)

diff --git a/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js b/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
index d7f4cae971b..9f73e099066 100644
--- a/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
+++ b/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
@@ -1,4 +1,4 @@
-/* eslint-disable func-names, space-before-function-paren, no-var, prefer-rest-params, max-len, no-restricted-syntax, vars-on-top, no-use-before-define, no-param-reassign, new-cap, no-underscore-dangle, wrap-iife, comma-dangle, no-return-assign, prefer-arrow-callback, quotes, prefer-template, newline-per-chained-call, no-else-return, no-shadow */
+/* eslint-disable func-names, space-before-function-paren, prefer-rest-params, max-len, no-restricted-syntax, vars-on-top, no-use-before-define, no-param-reassign, new-cap, no-underscore-dangle, wrap-iife, comma-dangle, no-return-assign, prefer-arrow-callback, quotes, prefer-template, newline-per-chained-call, no-else-return, no-shadow */
 
 import $ from 'jquery';
 import _ from 'underscore';
@@ -13,7 +13,7 @@ import { dateTickFormat } from '~/lib/utils/tick_formats';
 
 const d3 = { extent, max, select, scaleTime, scaleLinear, axisLeft, axisBottom, area, brushX, timeParse };
 
-const extend = function(child, parent) { for (var key in parent) { if (hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; };
+const extend = function(child, parent) { for (const key in parent) { if (hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; };
 const hasProp = {}.hasOwnProperty;
 
 export const ContributorsGraph = (function() {
@@ -122,8 +122,7 @@ export const ContributorsMasterGraph = (function(superClass) {
   }
 
   ContributorsMasterGraph.prototype.process_dates = function(data) {
-    var dates;
-    dates = this.get_dates(data);
+    const dates = this.get_dates(data);
     this.parse_dates(data);
     return ContributorsGraph.set_dates(dates);
   };
@@ -133,8 +132,7 @@ export const ContributorsMasterGraph = (function(superClass) {
   };
 
   ContributorsMasterGraph.prototype.parse_dates = function(data) {
-    var parseDate;
-    parseDate = d3.timeParse("%Y-%m-%d");
+    const parseDate = d3.timeParse("%Y-%m-%d");
     return data.forEach(function(d) {
       return d.date = parseDate(d.date);
     });
@@ -252,8 +250,7 @@ export const ContributorsAuthorGraph = (function(superClass) {
 
   ContributorsAuthorGraph.prototype.create_area = function(x, y) {
     return this.area = d3.area().x(function(d) {
-      var parseDate;
-      parseDate = d3.timeParse("%Y-%m-%d");
+      const parseDate = d3.timeParse("%Y-%m-%d");
       return x(parseDate(d));
     }).y0(this.height).y1((function(_this) {
       return function(d) {
@@ -267,7 +264,7 @@ export const ContributorsAuthorGraph = (function(superClass) {
   };
 
   ContributorsAuthorGraph.prototype.create_svg = function() {
-    var persons = document.querySelectorAll('.person');
+    const persons = document.querySelectorAll('.person');
     this.list_item = persons[persons.length - 1];
     return this.svg = d3.select(this.list_item).append("svg").attr("width", this.width + this.MARGIN.left + this.MARGIN.right).attr("height", this.height + this.MARGIN.top + this.MARGIN.bottom).attr("class", "spark").append("g").attr("transform", "translate(" + this.MARGIN.left + "," + this.MARGIN.top + ")");
   };

From 2fc6c0d93fcfec1ae2bff504bfd2f8181e5fe3f6 Mon Sep 17 00:00:00 2001
From: Paul Vorbach <paul@vorba.ch>
Date: Sat, 28 Apr 2018 23:13:05 +0200
Subject: [PATCH 04/25] Add changelog entry for contrib graphs width fix

---
 changelogs/unreleased/22647-width-contributors-graphs.yml | 5 +++++
 1 file changed, 5 insertions(+)
 create mode 100644 changelogs/unreleased/22647-width-contributors-graphs.yml

diff --git a/changelogs/unreleased/22647-width-contributors-graphs.yml b/changelogs/unreleased/22647-width-contributors-graphs.yml
new file mode 100644
index 00000000000..87be3a25d8a
--- /dev/null
+++ b/changelogs/unreleased/22647-width-contributors-graphs.yml
@@ -0,0 +1,5 @@
+---
+title: Fix width of contributors graphs
+merge_request: 18639
+author: Paul Vorbach
+type: fixed

From e9fcaed3fed3e1dddae8a0f70503f49f4084c270 Mon Sep 17 00:00:00 2001
From: Paul Vorbach <paul@vorba.ch>
Date: Sun, 29 Apr 2018 16:07:15 +0200
Subject: [PATCH 05/25] Refactor duplicate code

---
 .../graphs/show/stat_graph_contributors_graph.js | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js b/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
index 9f73e099066..f3e6c371ead 100644
--- a/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
+++ b/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
@@ -13,8 +13,8 @@ import { dateTickFormat } from '~/lib/utils/tick_formats';
 
 const d3 = { extent, max, select, scaleTime, scaleLinear, axisLeft, axisBottom, area, brushX, timeParse };
 
-const extend = function(child, parent) { for (const key in parent) { if (hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; };
 const hasProp = {}.hasOwnProperty;
+const extend = function(child, parent) { for (const key in parent) { if (hasProp.call(parent, key)) child[key] = parent[key]; } function ctor() { this.constructor = child; } ctor.prototype = parent.prototype; child.prototype = new ctor(); child.__super__ = parent.prototype; return child; };
 
 export const ContributorsGraph = (function() {
   function ContributorsGraph() {}
@@ -32,6 +32,12 @@ export const ContributorsGraph = (function() {
 
   ContributorsGraph.prototype.dates = [];
 
+  ContributorsGraph.prototype.determine_width = function(baseWidth, $parentElement) {
+    const parentPaddingWidth = parseFloat($parentElement.css('padding-left')) + parseFloat($parentElement.css('padding-right'));
+    const marginWidth = this.MARGIN.left + this.MARGIN.right;
+    return baseWidth - parentPaddingWidth - marginWidth;
+  };
+
   ContributorsGraph.set_x_domain = function(data) {
     return ContributorsGraph.prototype.x_domain = data;
   };
@@ -105,11 +111,10 @@ export const ContributorsMasterGraph = (function(superClass) {
 
   function ContributorsMasterGraph(data1) {
     const $parentElement = $('#contributors-master');
-    const parentPadding = parseFloat($parentElement.css('padding-left')) + parseFloat($parentElement.css('padding-right'));
 
     this.data = data1;
     this.update_content = this.update_content.bind(this);
-    this.width = $('.stat-graph').width() - parentPadding - (this.MARGIN.left + this.MARGIN.right);
+    this.width = this.determine_width($('.stat-graph').width(), $parentElement);
     this.height = 200;
     this.x = null;
     this.y = null;
@@ -217,14 +222,13 @@ export const ContributorsAuthorGraph = (function(superClass) {
 
   function ContributorsAuthorGraph(data1) {
     const $parentElements = $('.person');
-    const parentPadding = parseFloat($parentElements.css('padding-left')) + parseFloat($parentElements.css('padding-right'));
 
     this.data = data1;
     // Don't split graph size in half for mobile devices.
     if ($(window).width() < 768) {
-      this.width = $('.stat-graph').width() - parentPadding - (this.MARGIN.left + this.MARGIN.right);
+      this.width = this.determine_width($('.stat-graph').width(), $parentElements);
     } else {
-      this.width = ($('.stat-graph').width() / 2) - parentPadding - (this.MARGIN.left + this.MARGIN.right);
+      this.width = this.determine_width($('.stat-graph').width() / 2, $parentElements);
     }
     this.height = 200;
     this.x = null;

From 8b36485ce7217e2b10f765576ec4e816e556263b Mon Sep 17 00:00:00 2001
From: Paul Vorbach <paul@vorba.ch>
Date: Tue, 15 May 2018 22:53:55 +0200
Subject: [PATCH 06/25] Use correct base width

---
 .../show/stat_graph_contributors_graph.js     | 30 ++++++++++++++-----
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js b/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
index f3e6c371ead..5316d3e9f3c 100644
--- a/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
+++ b/app/assets/javascripts/pages/projects/graphs/show/stat_graph_contributors_graph.js
@@ -21,9 +21,9 @@ export const ContributorsGraph = (function() {
 
   ContributorsGraph.prototype.MARGIN = {
     top: 20,
-    right: 20,
+    right: 10,
     bottom: 30,
-    left: 50
+    left: 40
   };
 
   ContributorsGraph.prototype.x_domain = null;
@@ -114,7 +114,7 @@ export const ContributorsMasterGraph = (function(superClass) {
 
     this.data = data1;
     this.update_content = this.update_content.bind(this);
-    this.width = this.determine_width($('.stat-graph').width(), $parentElement);
+    this.width = this.determine_width($('.js-graphs-show').width(), $parentElement);
     this.height = 200;
     this.x = null;
     this.y = null;
@@ -155,7 +155,14 @@ export const ContributorsMasterGraph = (function(superClass) {
   };
 
   ContributorsMasterGraph.prototype.create_svg = function() {
-    return this.svg = d3.select("#contributors-master").append("svg").attr("width", this.width + this.MARGIN.left + this.MARGIN.right).attr("height", this.height + this.MARGIN.top + this.MARGIN.bottom).attr("class", "tint-box").append("g").attr("transform", "translate(" + this.MARGIN.left + "," + this.MARGIN.top + ")");
+    this.svg = d3.select("#contributors-master")
+      .append("svg")
+      .attr("width", this.width + this.MARGIN.left + this.MARGIN.right)
+      .attr("height", this.height + this.MARGIN.top + this.MARGIN.bottom)
+      .attr("class", "tint-box")
+      .append("g")
+      .attr("transform", "translate(" + this.MARGIN.left + "," + this.MARGIN.top + ")");
+    return this.svg;
   };
 
   ContributorsMasterGraph.prototype.create_area = function(x, y) {
@@ -225,10 +232,10 @@ export const ContributorsAuthorGraph = (function(superClass) {
 
     this.data = data1;
     // Don't split graph size in half for mobile devices.
-    if ($(window).width() < 768) {
-      this.width = this.determine_width($('.stat-graph').width(), $parentElements);
+    if ($(window).width() < 790) {
+      this.width = this.determine_width($('.js-graphs-show').width(), $parentElements);
     } else {
-      this.width = this.determine_width($('.stat-graph').width() / 2, $parentElements);
+      this.width = this.determine_width($('.js-graphs-show').width() / 2, $parentElements);
     }
     this.height = 200;
     this.x = null;
@@ -270,7 +277,14 @@ export const ContributorsAuthorGraph = (function(superClass) {
   ContributorsAuthorGraph.prototype.create_svg = function() {
     const persons = document.querySelectorAll('.person');
     this.list_item = persons[persons.length - 1];
-    return this.svg = d3.select(this.list_item).append("svg").attr("width", this.width + this.MARGIN.left + this.MARGIN.right).attr("height", this.height + this.MARGIN.top + this.MARGIN.bottom).attr("class", "spark").append("g").attr("transform", "translate(" + this.MARGIN.left + "," + this.MARGIN.top + ")");
+    this.svg = d3.select(this.list_item)
+      .append("svg")
+      .attr("width", this.width + this.MARGIN.left + this.MARGIN.right)
+      .attr("height", this.height + this.MARGIN.top + this.MARGIN.bottom)
+      .attr("class", "spark")
+      .append("g")
+      .attr("transform", "translate(" + this.MARGIN.left + "," + this.MARGIN.top + ")");
+    return this.svg;
   };
 
   ContributorsAuthorGraph.prototype.draw_path = function(data) {

From 7da3b2cdd09078984416aa03da108ad0a4a4e477 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Wed, 2 May 2018 18:21:42 +0200
Subject: [PATCH 07/25] Delete remote uploads

ObjectStore uploader requires presence of associated `uploads` record
when deleting the upload file (through the carrierwave's after_commit
hook) because we keep info whether file is LOCAL or REMOTE in `upload`
object.

For this reason we can not destroy uploads as "dependent: :destroy" hook
because these would be deleted too soon. Instead we rely on
carrierwave's hook to destroy `uploads` in after_commit hook.

But in before_destroy hook we still have to delete not-mounted uploads
(which don't use carrierwave's destroy hook). This has to be done in
before_Destroy instead of after_commit because `FileUpload` requires
existence of model's object on destroy action.

This is not ideal state of things, in a next step we should investigate
how to unify model dependencies so we can use same workflow for all
uploads.

Related to #45425
---
 app/models/appearance.rb                      |  3 +-
 app/models/concerns/with_uploads.rb           | 37 +++++++++++++++++++
 app/models/group.rb                           |  3 +-
 app/models/project.rb                         |  3 +-
 app/models/user.rb                            |  2 +-
 .../jprovazn-remote-upload-destroy.yml        |  5 +++
 spec/models/appearance_spec.rb                | 10 ++++-
 spec/models/group_spec.rb                     | 10 ++++-
 spec/models/project_spec.rb                   | 10 ++++-
 spec/models/user_spec.rb                      | 10 ++++-
 .../models/with_uploads_shared_examples.rb    | 23 ++++++++++++
 11 files changed, 105 insertions(+), 11 deletions(-)
 create mode 100644 app/models/concerns/with_uploads.rb
 create mode 100644 changelogs/unreleased/jprovazn-remote-upload-destroy.yml
 create mode 100644 spec/support/shared_examples/models/with_uploads_shared_examples.rb

diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index fb66dd0b766..f3cfc8ccd1e 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -1,4 +1,5 @@
 class Appearance < ActiveRecord::Base
+  include WithUploads
   include CacheMarkdownField
   include AfterCommitQueue
   include ObjectStorage::BackgroundMove
@@ -14,8 +15,6 @@ class Appearance < ActiveRecord::Base
   mount_uploader :logo,         AttachmentUploader
   mount_uploader :header_logo,  AttachmentUploader
 
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-
   CACHE_KEY = "current_appearance:#{Gitlab::VERSION}".freeze
 
   after_commit :flush_redis_cache
diff --git a/app/models/concerns/with_uploads.rb b/app/models/concerns/with_uploads.rb
new file mode 100644
index 00000000000..101d09f161d
--- /dev/null
+++ b/app/models/concerns/with_uploads.rb
@@ -0,0 +1,37 @@
+# Mounted uploaders are destroyed by carrierwave's after_commit
+# hook. This hook fetches upload location (local vs remote) from
+# Upload model. So it's neccessary to make sure that during that
+# after_commit hook model's associated uploads are not deleted yet.
+# IOW we can not use denepdent => :destroy :
+# has_many :uploads, as: :model, dependent: :destroy
+#
+# And because not-mounted uploads require presence of upload's
+# object model when destroying them (FileUploader's `build_upload` method
+# references `model` on delete), we can not use after_commit hook for these
+# uploads.
+#
+# Instead FileUploads are destroyed in before_destroy hook and remaining uploads
+# are destroyed by the carrierwave's after_commit hook.
+
+module WithUploads
+  extend ActiveSupport::Concern
+
+  # Currently there is no simple way how to select only not-mounted
+  # uploads, it should be all FileUploaders so we select them by
+  # `uploader` class
+  FILE_UPLOADERS = %w(PersonalFileUploader NamespaceFileUploader FileUploader).freeze
+
+  included do
+    has_many :uploads, as: :model
+
+    before_destroy :destroy_file_uploads
+  end
+
+  # mounted uploads are deleted in carrierwave's after_commit hook,
+  # but FileUploaders which are not mounted must be deleted explicitly and
+  # it can not be done in after_commit because FileUploader requires loads
+  # associated model on destroy (which is already deleted in after_commit)
+  def destroy_file_uploads
+    self.uploads.where(uploader: FILE_UPLOADERS).destroy_all
+  end
+end
diff --git a/app/models/group.rb b/app/models/group.rb
index cefca316399..107711e3cc5 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -2,6 +2,7 @@ require 'carrierwave/orm/activerecord'
 
 class Group < Namespace
   include Gitlab::ConfigHelper
+  include WithUploads
   include AfterCommitQueue
   include AccessRequestable
   include Avatarable
@@ -30,8 +31,6 @@ class Group < Namespace
   has_many :variables, class_name: 'Ci::GroupVariable'
   has_many :custom_attributes, class_name: 'GroupCustomAttribute'
 
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-
   has_many :boards
   has_many :badges, class_name: 'GroupBadge'
 
diff --git a/app/models/project.rb b/app/models/project.rb
index 534a0e630af..0b0d653c4af 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -4,6 +4,7 @@ class Project < ActiveRecord::Base
   include Gitlab::ConfigHelper
   include Gitlab::ShellAdapter
   include Gitlab::VisibilityLevel
+  include WithUploads
   include AccessRequestable
   include Avatarable
   include CacheMarkdownField
@@ -301,8 +302,6 @@ class Project < ActiveRecord::Base
     inclusion: { in: ->(_object) { Gitlab.config.repositories.storages.keys } }
   validates :variables, variable_duplicates: { scope: :environment_scope }
 
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
-
   # Scopes
   scope :pending_delete, -> { where(pending_delete: true) }
   scope :without_deleted, -> { where(pending_delete: false) }
diff --git a/app/models/user.rb b/app/models/user.rb
index 173ab38e20c..082ec76e86a 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -3,6 +3,7 @@ require 'carrierwave/orm/activerecord'
 class User < ActiveRecord::Base
   extend Gitlab::ConfigHelper
 
+  include WithUploads
   include Gitlab::ConfigHelper
   include Gitlab::SQL::Pattern
   include AfterCommitQueue
@@ -137,7 +138,6 @@ class User < ActiveRecord::Base
 
   has_many :custom_attributes, class_name: 'UserCustomAttribute'
   has_many :callouts, class_name: 'UserCallout'
-  has_many :uploads, as: :model, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent
   has_many :term_agreements
   belongs_to :accepted_term, class_name: 'ApplicationSetting::Term'
 
diff --git a/changelogs/unreleased/jprovazn-remote-upload-destroy.yml b/changelogs/unreleased/jprovazn-remote-upload-destroy.yml
new file mode 100644
index 00000000000..22e55920fa3
--- /dev/null
+++ b/changelogs/unreleased/jprovazn-remote-upload-destroy.yml
@@ -0,0 +1,5 @@
+---
+title: Fix deletion of Object Store uploads
+merge_request:
+author:
+type: fixed
diff --git a/spec/models/appearance_spec.rb b/spec/models/appearance_spec.rb
index 56b5d616284..5489c17bd82 100644
--- a/spec/models/appearance_spec.rb
+++ b/spec/models/appearance_spec.rb
@@ -5,7 +5,7 @@ describe Appearance do
 
   it { is_expected.to be_valid }
 
-  it { is_expected.to have_many(:uploads).dependent(:destroy) }
+  it { is_expected.to have_many(:uploads) }
 
   describe '.current', :use_clean_rails_memory_store_caching do
     let!(:appearance) { create(:appearance) }
@@ -41,4 +41,12 @@ describe Appearance do
       expect(new_row.valid?).to eq(false)
     end
   end
+
+  context 'with uploads' do
+    it_behaves_like 'model with mounted uploader', false do
+      let(:model_object) { create(:appearance, :with_logo) }
+      let(:upload_attribute) { :logo }
+      let(:uploader_class) { AttachmentUploader }
+    end
+  end
 end
diff --git a/spec/models/group_spec.rb b/spec/models/group_spec.rb
index 0907d28d33b..f83b52e8975 100644
--- a/spec/models/group_spec.rb
+++ b/spec/models/group_spec.rb
@@ -15,7 +15,7 @@ describe Group do
     it { is_expected.to have_many(:notification_settings).dependent(:destroy) }
     it { is_expected.to have_many(:labels).class_name('GroupLabel') }
     it { is_expected.to have_many(:variables).class_name('Ci::GroupVariable') }
-    it { is_expected.to have_many(:uploads).dependent(:destroy) }
+    it { is_expected.to have_many(:uploads) }
     it { is_expected.to have_one(:chat_team) }
     it { is_expected.to have_many(:custom_attributes).class_name('GroupCustomAttribute') }
     it { is_expected.to have_many(:badges).class_name('GroupBadge') }
@@ -691,4 +691,12 @@ describe Group do
       end
     end
   end
+
+  context 'with uploads' do
+    it_behaves_like 'model with mounted uploader', true do
+      let(:model_object) { create(:group, :with_avatar) }
+      let(:upload_attribute) { :avatar }
+      let(:uploader_class) { AttachmentUploader }
+    end
+  end
 end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 5b452f17979..39625b559eb 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -76,7 +76,7 @@ describe Project do
     it { is_expected.to have_many(:project_group_links) }
     it { is_expected.to have_many(:notification_settings).dependent(:delete_all) }
     it { is_expected.to have_many(:forks).through(:forked_project_links) }
-    it { is_expected.to have_many(:uploads).dependent(:destroy) }
+    it { is_expected.to have_many(:uploads) }
     it { is_expected.to have_many(:pipeline_schedules) }
     it { is_expected.to have_many(:members_and_requesters) }
     it { is_expected.to have_many(:clusters) }
@@ -3739,4 +3739,12 @@ describe Project do
       it { is_expected.to be_nil }
     end
   end
+
+  context 'with uploads' do
+    it_behaves_like 'model with mounted uploader', true do
+      let(:model_object) { create(:project, :with_avatar) }
+      let(:upload_attribute) { :avatar }
+      let(:uploader_class) { AttachmentUploader }
+    end
+  end
 end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index bb5308221f0..9dff6173f94 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -39,7 +39,7 @@ describe User do
     it { is_expected.to have_many(:builds).dependent(:nullify) }
     it { is_expected.to have_many(:pipelines).dependent(:nullify) }
     it { is_expected.to have_many(:chat_names).dependent(:destroy) }
-    it { is_expected.to have_many(:uploads).dependent(:destroy) }
+    it { is_expected.to have_many(:uploads) }
     it { is_expected.to have_many(:reported_abuse_reports).dependent(:destroy).class_name('AbuseReport') }
     it { is_expected.to have_many(:custom_attributes).class_name('UserCustomAttribute') }
 
@@ -2769,4 +2769,12 @@ describe User do
       expect { user.increment_failed_attempts! }.not_to change(user, :failed_attempts)
     end
   end
+
+  context 'with uploads' do
+    it_behaves_like 'model with mounted uploader', false do
+      let(:model_object) { create(:user, :with_avatar) }
+      let(:upload_attribute) { :avatar }
+      let(:uploader_class) { AttachmentUploader }
+    end
+  end
 end
diff --git a/spec/support/shared_examples/models/with_uploads_shared_examples.rb b/spec/support/shared_examples/models/with_uploads_shared_examples.rb
new file mode 100644
index 00000000000..47ad0c6345d
--- /dev/null
+++ b/spec/support/shared_examples/models/with_uploads_shared_examples.rb
@@ -0,0 +1,23 @@
+require 'spec_helper'
+
+shared_examples_for 'model with mounted uploader' do |supports_fileuploads|
+  describe '.destroy' do
+    before do
+      stub_uploads_object_storage(uploader_class)
+
+      model_object.public_send(upload_attribute).migrate!(ObjectStorage::Store::REMOTE)
+    end
+
+    it 'deletes remote uploads' do
+      expect_any_instance_of(CarrierWave::Storage::Fog::File).to receive(:delete).and_call_original
+
+      expect { model_object.destroy }.to change { Upload.count }.by(-1)
+    end
+
+    it 'deletes any FileUploader uploads which are not mounted', skip: !supports_fileuploads do
+      create(:upload, uploader: FileUploader, model: model_object)
+
+      expect { model_object.destroy }.to change { Upload.count }.by(-2)
+    end
+  end
+end

From c81a37c1d3f864cf0a00386dab29da78f222e3a5 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Fri, 4 May 2018 19:52:43 +0200
Subject: [PATCH 08/25] Use find_in_batches instead of destroy_all

destroy_all loads all records at once
---
 app/models/concerns/with_uploads.rb | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/app/models/concerns/with_uploads.rb b/app/models/concerns/with_uploads.rb
index 101d09f161d..3d99889c131 100644
--- a/app/models/concerns/with_uploads.rb
+++ b/app/models/concerns/with_uploads.rb
@@ -32,6 +32,8 @@ module WithUploads
   # it can not be done in after_commit because FileUploader requires loads
   # associated model on destroy (which is already deleted in after_commit)
   def destroy_file_uploads
-    self.uploads.where(uploader: FILE_UPLOADERS).destroy_all
+    self.uploads.where(uploader: FILE_UPLOADERS).find_each do |upload|
+      upload.destroy
+    end
   end
 end

From 32e22468300e3a52c82a855a01fc3983473107e0 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Mon, 7 May 2018 15:49:32 +0200
Subject: [PATCH 09/25] Changed order of include

---
 app/models/appearance.rb | 2 +-
 app/models/group.rb      | 2 +-
 app/models/project.rb    | 2 +-
 app/models/user.rb       | 2 +-
 4 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/app/models/appearance.rb b/app/models/appearance.rb
index f3cfc8ccd1e..f8713138a93 100644
--- a/app/models/appearance.rb
+++ b/app/models/appearance.rb
@@ -1,8 +1,8 @@
 class Appearance < ActiveRecord::Base
-  include WithUploads
   include CacheMarkdownField
   include AfterCommitQueue
   include ObjectStorage::BackgroundMove
+  include WithUploads
 
   cache_markdown_field :description
   cache_markdown_field :new_project_guidelines
diff --git a/app/models/group.rb b/app/models/group.rb
index 107711e3cc5..8fb77a7869d 100644
--- a/app/models/group.rb
+++ b/app/models/group.rb
@@ -2,7 +2,6 @@ require 'carrierwave/orm/activerecord'
 
 class Group < Namespace
   include Gitlab::ConfigHelper
-  include WithUploads
   include AfterCommitQueue
   include AccessRequestable
   include Avatarable
@@ -11,6 +10,7 @@ class Group < Namespace
   include LoadedInGroupList
   include GroupDescendant
   include TokenAuthenticatable
+  include WithUploads
 
   has_many :group_members, -> { where(requested_at: nil) }, dependent: :destroy, as: :source # rubocop:disable Cop/ActiveRecordDependent
   alias_method :members, :group_members
diff --git a/app/models/project.rb b/app/models/project.rb
index 0b0d653c4af..0975e64e995 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -4,7 +4,6 @@ class Project < ActiveRecord::Base
   include Gitlab::ConfigHelper
   include Gitlab::ShellAdapter
   include Gitlab::VisibilityLevel
-  include WithUploads
   include AccessRequestable
   include Avatarable
   include CacheMarkdownField
@@ -24,6 +23,7 @@ class Project < ActiveRecord::Base
   include ::Gitlab::Utils::StrongMemoize
   include ChronicDurationAttribute
   include FastDestroyAll::Helpers
+  include WithUploads
 
   extend Gitlab::ConfigHelper
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 082ec76e86a..b90f5471071 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -3,7 +3,6 @@ require 'carrierwave/orm/activerecord'
 class User < ActiveRecord::Base
   extend Gitlab::ConfigHelper
 
-  include WithUploads
   include Gitlab::ConfigHelper
   include Gitlab::SQL::Pattern
   include AfterCommitQueue
@@ -18,6 +17,7 @@ class User < ActiveRecord::Base
   include IgnorableColumn
   include BulkMemberAccessLoad
   include BlocksJsonSerialization
+  include WithUploads
 
   DEFAULT_NOTIFICATION_LEVEL = :participating
 

From 8c0166f2b96e233cbf8ec84e4314094521dc0316 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Wed, 9 May 2018 21:43:23 +0200
Subject: [PATCH 10/25] Fixed typo

---
 app/models/concerns/with_uploads.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/models/concerns/with_uploads.rb b/app/models/concerns/with_uploads.rb
index 3d99889c131..e7cfffb775b 100644
--- a/app/models/concerns/with_uploads.rb
+++ b/app/models/concerns/with_uploads.rb
@@ -2,7 +2,7 @@
 # hook. This hook fetches upload location (local vs remote) from
 # Upload model. So it's neccessary to make sure that during that
 # after_commit hook model's associated uploads are not deleted yet.
-# IOW we can not use denepdent => :destroy :
+# IOW we can not use dependent: :destroy :
 # has_many :uploads, as: :model, dependent: :destroy
 #
 # And because not-mounted uploads require presence of upload's

From 2060533f91b5527b568321f63a1aa7f4ef0081d5 Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Fri, 11 May 2018 14:45:18 +0200
Subject: [PATCH 11/25] Whitelisted query limits for group destroy API

---
 lib/api/groups.rb    | 1 +
 lib/api/v3/groups.rb | 1 +
 2 files changed, 2 insertions(+)

diff --git a/lib/api/groups.rb b/lib/api/groups.rb
index 92e3d5cc10a..0d125cd7831 100644
--- a/lib/api/groups.rb
+++ b/lib/api/groups.rb
@@ -165,6 +165,7 @@ module API
         group = find_group!(params[:id])
         authorize! :admin_group, group
 
+        Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/46285')
         destroy_conditionally!(group) do |group|
           ::Groups::DestroyService.new(group, current_user).execute
         end
diff --git a/lib/api/v3/groups.rb b/lib/api/v3/groups.rb
index 2c52d21fa1c..3844fd4810d 100644
--- a/lib/api/v3/groups.rb
+++ b/lib/api/v3/groups.rb
@@ -131,6 +131,7 @@ module API
         delete ":id" do
           group = find_group!(params[:id])
           authorize! :admin_group, group
+          Gitlab::QueryLimiting.whitelist('https://gitlab.com/gitlab-org/gitlab-ce/issues/46285')
           present ::Groups::DestroyService.new(group, current_user).execute, with: Entities::GroupDetail, current_user: current_user
         end
 

From 846f73b53b8a6d3bc1f18607630d7a7853cb9d13 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Wed, 9 May 2018 14:46:34 +0200
Subject: [PATCH 12/25] Allow group runners to be viewed/edited in API

---
 app/models/ci/runner.rb                       |  2 +-
 app/models/user.rb                            | 16 ++++-
 lib/api/runners.rb                            |  1 +
 .../settings/ci_cd_controller_spec.rb         |  6 +-
 spec/models/ci/runner_spec.rb                 | 64 ++++--------------
 spec/models/user_spec.rb                      | 66 +++++++++++++++----
 spec/requests/api/runners_spec.rb             |  4 +-
 7 files changed, 88 insertions(+), 71 deletions(-)

diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb
index bda69f85a78..e6f1ed519be 100644
--- a/app/models/ci/runner.rb
+++ b/app/models/ci/runner.rb
@@ -52,7 +52,7 @@ module Ci
       #        Without that, placeholders would miss one and couldn't match.
       where(locked: false)
         .where.not("ci_runners.id IN (#{project.runners.select(:id).to_sql})")
-        .specific
+        .project_type
     end
 
     validate :tag_constraints
diff --git a/app/models/user.rb b/app/models/user.rb
index 173ab38e20c..2afe9ea77f9 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1001,10 +1001,17 @@ class User < ActiveRecord::Base
 
   def ci_authorized_runners
     @ci_authorized_runners ||= begin
-      runner_ids = Ci::RunnerProject
+      project_runner_ids = Ci::RunnerProject
         .where(project: authorized_projects(Gitlab::Access::MASTER))
         .select(:runner_id)
-      Ci::Runner.specific.where(id: runner_ids)
+
+      group_runner_ids = Ci::RunnerNamespace
+        .where(namespace_id: owned_or_masters_groups.select(:id))
+        .select(:runner_id)
+
+      union = Gitlab::SQL::Union.new([project_runner_ids, group_runner_ids])
+
+      Ci::Runner.specific.where("ci_runners.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection
     end
   end
 
@@ -1205,6 +1212,11 @@ class User < ActiveRecord::Base
       !terms_accepted?
   end
 
+  def owned_or_masters_groups
+    union = Gitlab::SQL::Union.new([owned_groups, masters_groups])
+    Group.from("(#{union.to_sql}) namespaces")
+  end
+
   protected
 
   # override, from Devise::Validatable
diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index 5f2a9567605..1b528a8490c 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -205,6 +205,7 @@ module API
       def authenticate_enable_runner!(runner)
         forbidden!("Runner is shared") if runner.is_shared?
         forbidden!("Runner is locked") if runner.locked?
+        forbidden!("Runner is a group runner") if runner.group_type?
         return if current_user.admin?
 
         forbidden!("No access granted") unless user_can_access_runner?(runner)
diff --git a/spec/controllers/projects/settings/ci_cd_controller_spec.rb b/spec/controllers/projects/settings/ci_cd_controller_spec.rb
index a91c868cbaf..f1810763d2d 100644
--- a/spec/controllers/projects/settings/ci_cd_controller_spec.rb
+++ b/spec/controllers/projects/settings/ci_cd_controller_spec.rb
@@ -19,11 +19,11 @@ describe Projects::Settings::CiCdController do
     end
 
     context 'with group runners' do
-      let(:group_runner) { create(:ci_runner) }
+      let(:group_runner) { create(:ci_runner, runner_type: :group_type) }
       let(:parent_group) { create(:group) }
       let(:group) { create(:group, runners: [group_runner], parent: parent_group) }
       let(:other_project) { create(:project, group: group) }
-      let!(:project_runner) { create(:ci_runner, projects: [other_project]) }
+      let!(:project_runner) { create(:ci_runner, projects: [other_project], runner_type: :project_type) }
       let!(:shared_runner) { create(:ci_runner, :shared) }
 
       it 'sets assignable project runners only' do
@@ -31,7 +31,7 @@ describe Projects::Settings::CiCdController do
 
         get :show, namespace_id: project.namespace, project_id: project
 
-        expect(assigns(:assignable_runners)).to eq [project_runner]
+        expect(assigns(:assignable_runners)).to contain_exactly(project_runner)
       end
     end
   end
diff --git a/spec/models/ci/runner_spec.rb b/spec/models/ci/runner_spec.rb
index e2b212f4f4c..0fbc934f669 100644
--- a/spec/models/ci/runner_spec.rb
+++ b/spec/models/ci/runner_spec.rb
@@ -626,62 +626,26 @@ describe Ci::Runner do
   end
 
   describe '.assignable_for' do
-    let(:runner) { create(:ci_runner) }
+    let!(:unlocked_project_runner) { create(:ci_runner, runner_type: :project_type, projects: [project]) }
+    let!(:locked_project_runner) { create(:ci_runner, runner_type: :project_type, locked: true, projects: [project]) }
+    let!(:group_runner) { create(:ci_runner, runner_type: :group_type) }
+    let!(:instance_runner) { create(:ci_runner, :shared) }
     let(:project) { create(:project) }
     let(:another_project) { create(:project) }
 
-    before do
-      project.runners << runner
+    context 'with already assigned project' do
+      subject { described_class.assignable_for(project) }
+
+      it { is_expected.to be_empty }
     end
 
-    context 'with shared runners' do
-      before do
-        runner.update(is_shared: true)
-      end
+    context 'with a different project' do
+      subject { described_class.assignable_for(another_project) }
 
-      context 'does not give owned runner' do
-        subject { described_class.assignable_for(project) }
-
-        it { is_expected.to be_empty }
-      end
-
-      context 'does not give shared runner' do
-        subject { described_class.assignable_for(another_project) }
-
-        it { is_expected.to be_empty }
-      end
-    end
-
-    context 'with unlocked runner' do
-      context 'does not give owned runner' do
-        subject { described_class.assignable_for(project) }
-
-        it { is_expected.to be_empty }
-      end
-
-      context 'does give a specific runner' do
-        subject { described_class.assignable_for(another_project) }
-
-        it { is_expected.to contain_exactly(runner) }
-      end
-    end
-
-    context 'with locked runner' do
-      before do
-        runner.update(locked: true)
-      end
-
-      context 'does not give owned runner' do
-        subject { described_class.assignable_for(project) }
-
-        it { is_expected.to be_empty }
-      end
-
-      context 'does not give a locked runner' do
-        subject { described_class.assignable_for(another_project) }
-
-        it { is_expected.to be_empty }
-      end
+      it { is_expected.to include(unlocked_project_runner) }
+      it { is_expected.not_to include(group_runner) }
+      it { is_expected.not_to include(locked_project_runner) }
+      it { is_expected.not_to include(instance_runner) }
     end
   end
 
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index bb5308221f0..52d25d20442 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1788,14 +1788,12 @@ describe User do
 
   describe '#ci_authorized_runners' do
     let(:user) { create(:user) }
-    let(:runner) { create(:ci_runner) }
+    let(:runner_1) { create(:ci_runner) }
+    let(:runner_2) { create(:ci_runner) }
 
-    before do
-      project.runners << runner
-    end
-
-    context 'without any projects' do
-      let(:project) { create(:project) }
+    context 'without any projects nor groups' do
+      let!(:project) { create(:project, runners: [runner_1]) }
+      let!(:group) { create(:group) }
 
       it 'does not load' do
         expect(user.ci_authorized_runners).to be_empty
@@ -1804,10 +1802,38 @@ describe User do
 
     context 'with personal projects runners' do
       let(:namespace) { create(:namespace, owner: user) }
-      let(:project) { create(:project, namespace: namespace) }
+      let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) }
 
       it 'loads' do
-        expect(user.ci_authorized_runners).to contain_exactly(runner)
+        expect(user.ci_authorized_runners).to contain_exactly(runner_1)
+      end
+    end
+
+    context 'with personal group runner' do
+      let!(:project) { create(:project, runners: [runner_1]) }
+      let!(:group) do
+        create(:group, runners: [runner_2]).tap do |group|
+          group.add_owner(user)
+        end
+      end
+
+      it 'loads' do
+        expect(user.ci_authorized_runners).to contain_exactly(runner_2)
+      end
+    end
+
+    context 'with personal project and group runner' do
+      let(:namespace) { create(:namespace, owner: user) }
+      let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) }
+
+      let!(:group) do
+        create(:group, runners: [runner_2]).tap do |group|
+          group.add_owner(user)
+        end
+      end
+
+      it 'loads' do
+        expect(user.ci_authorized_runners).to contain_exactly(runner_1, runner_2)
       end
     end
 
@@ -1818,7 +1844,7 @@ describe User do
         end
 
         it 'loads' do
-          expect(user.ci_authorized_runners).to contain_exactly(runner)
+          expect(user.ci_authorized_runners).to contain_exactly(runner_1)
         end
       end
 
@@ -1835,7 +1861,21 @@ describe User do
 
     context 'with groups projects runners' do
       let(:group) { create(:group) }
-      let(:project) { create(:project, group: group) }
+      let!(:project) { create(:project, group: group, runners: [runner_1]) }
+
+      def add_user(access)
+        group.add_user(user, access)
+      end
+
+      it_behaves_like :member
+    end
+
+    context 'with groups runners' do
+      let!(:group) do
+        create(:group, runners: [runner_1]).tap do |group|
+          group.add_owner(user)
+        end
+      end
 
       def add_user(access)
         group.add_user(user, access)
@@ -1845,7 +1885,7 @@ describe User do
     end
 
     context 'with other projects runners' do
-      let(:project) { create(:project) }
+      let!(:project) { create(:project, runners: [runner_1]) }
 
       def add_user(access)
         project.add_role(user, access)
@@ -1858,7 +1898,7 @@ describe User do
       let(:group) { create(:group) }
       let(:another_user) { create(:user) }
       let(:subgroup) { create(:group, parent: group) }
-      let(:project) { create(:project, group: subgroup) }
+      let!(:project) { create(:project, group: subgroup, runners: [runner_1]) }
 
       def add_user(access)
         group.add_user(user, access)
diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb
index 981ac768e3a..44eac4b2073 100644
--- a/spec/requests/api/runners_spec.rb
+++ b/spec/requests/api/runners_spec.rb
@@ -27,7 +27,7 @@ describe API::Runners do
     end
   end
 
-  let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group]) }
+  let!(:group_runner) { create(:ci_runner, description: 'Group runner', groups: [group], runner_type: :group_type) }
 
   before do
     # Set project access for users
@@ -48,7 +48,7 @@ describe API::Runners do
         expect(json_response).to be_an Array
         expect(json_response[0]).to have_key('ip_address')
         expect(descriptions).to contain_exactly(
-          'Project runner', 'Two projects runner'
+          'Project runner', 'Two projects runner', 'Group runner'
         )
         expect(shared).to be_falsey
       end

From 7320684c00ada153c0a9b102f8cf2db38367129a Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Wed, 9 May 2018 16:41:15 +0200
Subject: [PATCH 13/25] Use can? policies for lib/api/runners.rb

---
 app/policies/ci/runner_policy.rb |  8 ++++----
 lib/api/runners.rb               | 14 +++++---------
 2 files changed, 9 insertions(+), 13 deletions(-)

diff --git a/app/policies/ci/runner_policy.rb b/app/policies/ci/runner_policy.rb
index 7dff8470e23..2908989b154 100644
--- a/app/policies/ci/runner_policy.rb
+++ b/app/policies/ci/runner_policy.rb
@@ -1,8 +1,5 @@
 module Ci
   class RunnerPolicy < BasePolicy
-    with_options scope: :subject, score: 0
-    condition(:shared) { @subject.is_shared? }
-
     with_options scope: :subject, score: 0
     condition(:locked, scope: :subject) { @subject.locked? }
 
@@ -10,7 +7,10 @@ module Ci
 
     rule { anonymous }.prevent_all
     rule { admin | authorized_runner }.enable :assign_runner
-    rule { ~admin & shared }.prevent :assign_runner
+    rule { admin | authorized_runner }.enable :read_runner
+    rule { admin | authorized_runner }.enable :update_runner
+    rule { admin | authorized_runner }.enable :delete_runner
+    rule { admin | authorized_runner }.enable :list_runner_jobs
     rule { ~admin & locked }.prevent :assign_runner
   end
 end
diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index 1b528a8490c..db9cff80cf9 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -184,14 +184,14 @@ module API
       def authenticate_show_runner!(runner)
         return if runner.is_shared || current_user.admin?
 
-        forbidden!("No access granted") unless user_can_access_runner?(runner)
+        forbidden!("No access granted") unless can?(current_user, :read_runner, runner)
       end
 
       def authenticate_update_runner!(runner)
         return if current_user.admin?
 
         forbidden!("Runner is shared") if runner.is_shared?
-        forbidden!("No access granted") unless user_can_access_runner?(runner)
+        forbidden!("No access granted") unless can?(current_user, :update_runner, runner)
       end
 
       def authenticate_delete_runner!(runner)
@@ -199,7 +199,7 @@ module API
 
         forbidden!("Runner is shared") if runner.is_shared?
         forbidden!("Runner associated with more than one project") if runner.projects.count > 1
-        forbidden!("No access granted") unless user_can_access_runner?(runner)
+        forbidden!("No access granted") unless can?(current_user, :delete_runner, runner)
       end
 
       def authenticate_enable_runner!(runner)
@@ -208,17 +208,13 @@ module API
         forbidden!("Runner is a group runner") if runner.group_type?
         return if current_user.admin?
 
-        forbidden!("No access granted") unless user_can_access_runner?(runner)
+        forbidden!("No access granted") unless can?(current_user, :assign_runner, runner)
       end
 
       def authenticate_list_runners_jobs!(runner)
         return if current_user.admin?
 
-        forbidden!("No access granted") unless user_can_access_runner?(runner)
-      end
-
-      def user_can_access_runner?(runner)
-        current_user.ci_authorized_runners.exists?(runner.id)
+        forbidden!("No access granted") unless can?(current_user, :list_runner_jobs, runner)
       end
     end
   end

From 18821b157dbf3a73637ab741e8154b5133ce0e72 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Wed, 9 May 2018 16:59:59 +0200
Subject: [PATCH 14/25] Improve efficiency of authorized_runner policy query

---
 app/policies/ci/runner_policy.rb | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/app/policies/ci/runner_policy.rb b/app/policies/ci/runner_policy.rb
index 2908989b154..82d8e86ae05 100644
--- a/app/policies/ci/runner_policy.rb
+++ b/app/policies/ci/runner_policy.rb
@@ -3,7 +3,7 @@ module Ci
     with_options scope: :subject, score: 0
     condition(:locked, scope: :subject) { @subject.locked? }
 
-    condition(:authorized_runner) { @user.ci_authorized_runners.include?(@subject) }
+    condition(:authorized_runner) { @user.ci_authorized_runners.exists?(@subject.id) }
 
     rule { anonymous }.prevent_all
     rule { admin | authorized_runner }.enable :assign_runner

From c3f9d80a6e0950361e056ded4107015d3923f56d Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Thu, 10 May 2018 14:42:55 +0200
Subject: [PATCH 15/25] Rename User#ci_authorized_runners -> ci_owned_runners

---
 .../projects/settings/ci_cd_controller.rb          |  2 +-
 app/models/user.rb                                 |  4 ++--
 app/policies/ci/runner_policy.rb                   | 12 ++++++------
 lib/api/runners.rb                                 |  2 +-
 lib/api/v3/runners.rb                              |  2 +-
 spec/models/user_spec.rb                           | 14 +++++++-------
 6 files changed, 18 insertions(+), 18 deletions(-)

diff --git a/app/controllers/projects/settings/ci_cd_controller.rb b/app/controllers/projects/settings/ci_cd_controller.rb
index 177c8a54099..1d850baf012 100644
--- a/app/controllers/projects/settings/ci_cd_controller.rb
+++ b/app/controllers/projects/settings/ci_cd_controller.rb
@@ -69,7 +69,7 @@ module Projects
         @project_runners = @project.runners.ordered
 
         @assignable_runners = current_user
-          .ci_authorized_runners
+          .ci_owned_runners
           .assignable_for(project)
           .ordered
           .page(params[:page]).per(20)
diff --git a/app/models/user.rb b/app/models/user.rb
index 2afe9ea77f9..226a4489261 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -999,8 +999,8 @@ class User < ActiveRecord::Base
     !solo_owned_groups.present?
   end
 
-  def ci_authorized_runners
-    @ci_authorized_runners ||= begin
+  def ci_owned_runners
+    @ci_owned_runners ||= begin
       project_runner_ids = Ci::RunnerProject
         .where(project: authorized_projects(Gitlab::Access::MASTER))
         .select(:runner_id)
diff --git a/app/policies/ci/runner_policy.rb b/app/policies/ci/runner_policy.rb
index 82d8e86ae05..61912696e88 100644
--- a/app/policies/ci/runner_policy.rb
+++ b/app/policies/ci/runner_policy.rb
@@ -3,14 +3,14 @@ module Ci
     with_options scope: :subject, score: 0
     condition(:locked, scope: :subject) { @subject.locked? }
 
-    condition(:authorized_runner) { @user.ci_authorized_runners.exists?(@subject.id) }
+    condition(:owned_runner) { @user.ci_owned_runners.exists?(@subject.id) }
 
     rule { anonymous }.prevent_all
-    rule { admin | authorized_runner }.enable :assign_runner
-    rule { admin | authorized_runner }.enable :read_runner
-    rule { admin | authorized_runner }.enable :update_runner
-    rule { admin | authorized_runner }.enable :delete_runner
-    rule { admin | authorized_runner }.enable :list_runner_jobs
+    rule { admin | owned_runner }.enable :assign_runner
+    rule { admin | owned_runner }.enable :read_runner
+    rule { admin | owned_runner }.enable :update_runner
+    rule { admin | owned_runner }.enable :delete_runner
+    rule { admin | owned_runner  }.enable :list_runner_jobs
     rule { ~admin & locked }.prevent :assign_runner
   end
 end
diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index db9cff80cf9..4f12aeac1fd 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -14,7 +14,7 @@ module API
         use :pagination
       end
       get do
-        runners = filter_runners(current_user.ci_authorized_runners, params[:scope], without: %w(specific shared))
+        runners = filter_runners(current_user.ci_owned_runners, params[:scope], without: %w(specific shared))
         present paginate(runners), with: Entities::Runner
       end
 
diff --git a/lib/api/v3/runners.rb b/lib/api/v3/runners.rb
index c6d9957d452..8a5c46805bd 100644
--- a/lib/api/v3/runners.rb
+++ b/lib/api/v3/runners.rb
@@ -58,7 +58,7 @@ module API
         end
 
         def user_can_access_runner?(runner)
-          current_user.ci_authorized_runners.exists?(runner.id)
+          current_user.ci_owned_runners.exists?(runner.id)
         end
       end
     end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 52d25d20442..de15e0e62aa 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1786,7 +1786,7 @@ describe User do
     end
   end
 
-  describe '#ci_authorized_runners' do
+  describe '#ci_owned_runners' do
     let(:user) { create(:user) }
     let(:runner_1) { create(:ci_runner) }
     let(:runner_2) { create(:ci_runner) }
@@ -1796,7 +1796,7 @@ describe User do
       let!(:group) { create(:group) }
 
       it 'does not load' do
-        expect(user.ci_authorized_runners).to be_empty
+        expect(user.ci_owned_runners).to be_empty
       end
     end
 
@@ -1805,7 +1805,7 @@ describe User do
       let!(:project) { create(:project, namespace: namespace, runners: [runner_1]) }
 
       it 'loads' do
-        expect(user.ci_authorized_runners).to contain_exactly(runner_1)
+        expect(user.ci_owned_runners).to contain_exactly(runner_1)
       end
     end
 
@@ -1818,7 +1818,7 @@ describe User do
       end
 
       it 'loads' do
-        expect(user.ci_authorized_runners).to contain_exactly(runner_2)
+        expect(user.ci_owned_runners).to contain_exactly(runner_2)
       end
     end
 
@@ -1833,7 +1833,7 @@ describe User do
       end
 
       it 'loads' do
-        expect(user.ci_authorized_runners).to contain_exactly(runner_1, runner_2)
+        expect(user.ci_owned_runners).to contain_exactly(runner_1, runner_2)
       end
     end
 
@@ -1844,7 +1844,7 @@ describe User do
         end
 
         it 'loads' do
-          expect(user.ci_authorized_runners).to contain_exactly(runner_1)
+          expect(user.ci_owned_runners).to contain_exactly(runner_1)
         end
       end
 
@@ -1854,7 +1854,7 @@ describe User do
         end
 
         it 'does not load' do
-          expect(user.ci_authorized_runners).to be_empty
+          expect(user.ci_owned_runners).to be_empty
         end
       end
     end

From 8583e4a1478ffe94dfd75c51c8480b323cada6df Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Thu, 10 May 2018 14:53:24 +0200
Subject: [PATCH 16/25] Change policy list_runner_jobs -> read_runner

---
 app/policies/ci/runner_policy.rb | 1 -
 lib/api/runners.rb               | 2 +-
 2 files changed, 1 insertion(+), 2 deletions(-)

diff --git a/app/policies/ci/runner_policy.rb b/app/policies/ci/runner_policy.rb
index 61912696e88..4649dc645ba 100644
--- a/app/policies/ci/runner_policy.rb
+++ b/app/policies/ci/runner_policy.rb
@@ -10,7 +10,6 @@ module Ci
     rule { admin | owned_runner }.enable :read_runner
     rule { admin | owned_runner }.enable :update_runner
     rule { admin | owned_runner }.enable :delete_runner
-    rule { admin | owned_runner  }.enable :list_runner_jobs
     rule { ~admin & locked }.prevent :assign_runner
   end
 end
diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index 4f12aeac1fd..c6dc40ae789 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -214,7 +214,7 @@ module API
       def authenticate_list_runners_jobs!(runner)
         return if current_user.admin?
 
-        forbidden!("No access granted") unless can?(current_user, :list_runner_jobs, runner)
+        forbidden!("No access granted") unless can?(current_user, :read_runner, runner)
       end
     end
   end

From b35d16a77f6a63db756a94eeff3788e819e7ed04 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Fri, 11 May 2018 10:10:22 +0200
Subject: [PATCH 17/25] Allow admin to assign shared runner to project through
 API

---
 lib/api/runners.rb                | 4 ++--
 spec/requests/api/runners_spec.rb | 9 +++++++++
 2 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index c6dc40ae789..0fb125a6944 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -203,11 +203,11 @@ module API
       end
 
       def authenticate_enable_runner!(runner)
-        forbidden!("Runner is shared") if runner.is_shared?
-        forbidden!("Runner is locked") if runner.locked?
         forbidden!("Runner is a group runner") if runner.group_type?
+
         return if current_user.admin?
 
+        forbidden!("Runner is locked") if runner.locked?
         forbidden!("No access granted") unless can?(current_user, :assign_runner, runner)
       end
 
diff --git a/spec/requests/api/runners_spec.rb b/spec/requests/api/runners_spec.rb
index 44eac4b2073..c7587c877fc 100644
--- a/spec/requests/api/runners_spec.rb
+++ b/spec/requests/api/runners_spec.rb
@@ -592,6 +592,15 @@ describe API::Runners do
           end.to change { project.runners.count }.by(+1)
           expect(response).to have_gitlab_http_status(201)
         end
+
+        it 'enables a shared runner' do
+          expect do
+            post api("/projects/#{project.id}/runners", admin), runner_id: shared_runner.id
+          end.to change { project.runners.count }.by(1)
+
+          expect(shared_runner.reload).not_to be_shared
+          expect(response).to have_gitlab_http_status(201)
+        end
       end
 
       context 'user is not admin' do

From 983bc6b175ab47fafefe2866dd19c3fdb98c2b84 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Fri, 11 May 2018 10:56:02 +0200
Subject: [PATCH 18/25] Remove unnecessary runner.is_shared? checks in api
 because they are handled by policy

---
 lib/api/runners.rb | 2 --
 1 file changed, 2 deletions(-)

diff --git a/lib/api/runners.rb b/lib/api/runners.rb
index 0fb125a6944..5cb96d467c0 100644
--- a/lib/api/runners.rb
+++ b/lib/api/runners.rb
@@ -190,14 +190,12 @@ module API
       def authenticate_update_runner!(runner)
         return if current_user.admin?
 
-        forbidden!("Runner is shared") if runner.is_shared?
         forbidden!("No access granted") unless can?(current_user, :update_runner, runner)
       end
 
       def authenticate_delete_runner!(runner)
         return if current_user.admin?
 
-        forbidden!("Runner is shared") if runner.is_shared?
         forbidden!("Runner associated with more than one project") if runner.projects.count > 1
         forbidden!("No access granted") unless can?(current_user, :delete_runner, runner)
       end

From 1cfa5ed07065f04531900fe0931deaaaef3e69d2 Mon Sep 17 00:00:00 2001
From: Dylan Griffith <dyl.griffith@gmail.com>
Date: Wed, 16 May 2018 09:56:28 +0200
Subject: [PATCH 19/25] Refactor out duplication in runner_policy.rb

---
 app/policies/ci/runner_policy.rb | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/app/policies/ci/runner_policy.rb b/app/policies/ci/runner_policy.rb
index 4649dc645ba..895abe87d86 100644
--- a/app/policies/ci/runner_policy.rb
+++ b/app/policies/ci/runner_policy.rb
@@ -6,10 +6,14 @@ module Ci
     condition(:owned_runner) { @user.ci_owned_runners.exists?(@subject.id) }
 
     rule { anonymous }.prevent_all
-    rule { admin | owned_runner }.enable :assign_runner
-    rule { admin | owned_runner }.enable :read_runner
-    rule { admin | owned_runner }.enable :update_runner
-    rule { admin | owned_runner }.enable :delete_runner
+
+    rule { admin | owned_runner }.policy do
+      enable :assign_runner
+      enable :read_runner
+      enable :update_runner
+      enable :delete_runner
+    end
+
     rule { ~admin & locked }.prevent :assign_runner
   end
 end

From dfdd88151092fde3e32729dd466ad9ba44e158c6 Mon Sep 17 00:00:00 2001
From: Zeger-Jan van de Weg <git@zjvandeweg.nl>
Date: Tue, 15 May 2018 17:05:19 +0200
Subject: [PATCH 20/25] Workhorse to send raw diff and patch for commits

Prior to this change, this was done through unicorn. In theory this
could time out. Workhorse has been sending these raw patches and diffs
for a long time and is stable in doing so.

Added bonus is the fact that `Commit#to_patch` can be removed.

`Commit#to_diff` too, which closes
https://gitlab.com/gitlab-org/gitaly/issues/324

Closes https://gitlab.com/gitlab-org/gitaly/issues/1196
---
 app/controllers/projects/commit_controller.rb |  8 +++--
 .../zj-workhorse-commit-patch-diff.yml        |  5 +++
 lib/gitlab/git/commit.rb                      | 25 --------------
 .../projects/commit_controller_spec.rb        | 33 +++----------------
 spec/lib/gitlab/git/commit_spec.rb            | 14 --------
 spec/models/commit_spec.rb                    |  1 -
 6 files changed, 16 insertions(+), 70 deletions(-)
 create mode 100644 changelogs/unreleased/zj-workhorse-commit-patch-diff.yml

diff --git a/app/controllers/projects/commit_controller.rb b/app/controllers/projects/commit_controller.rb
index b7f548e0e63..1d1184d46d1 100644
--- a/app/controllers/projects/commit_controller.rb
+++ b/app/controllers/projects/commit_controller.rb
@@ -23,8 +23,12 @@ class Projects::CommitController < Projects::ApplicationController
 
     respond_to do |format|
       format.html  { render }
-      format.diff  { render text: @commit.to_diff }
-      format.patch { render text: @commit.to_patch }
+      format.diff  do
+        send_git_diff(@project.repository, @commit.diff_refs)
+      end
+      format.patch do
+        send_git_patch(@project.repository, @commit.diff_refs)
+      end
     end
   end
 
diff --git a/changelogs/unreleased/zj-workhorse-commit-patch-diff.yml b/changelogs/unreleased/zj-workhorse-commit-patch-diff.yml
new file mode 100644
index 00000000000..bce68692d98
--- /dev/null
+++ b/changelogs/unreleased/zj-workhorse-commit-patch-diff.yml
@@ -0,0 +1,5 @@
+---
+title: Workhorse to send raw diff and patch for commits
+merge_request:
+author:
+type: other
diff --git a/lib/gitlab/git/commit.rb b/lib/gitlab/git/commit.rb
index fabcd46c8e9..d79a4dbeee4 100644
--- a/lib/gitlab/git/commit.rb
+++ b/lib/gitlab/git/commit.rb
@@ -342,21 +342,6 @@ module Gitlab
         parent_ids.first
       end
 
-      # Shows the diff between the commit's parent and the commit.
-      #
-      # Cuts out the header and stats from #to_patch and returns only the diff.
-      #
-      # Gitaly migration: https://gitlab.com/gitlab-org/gitaly/issues/324
-      def to_diff
-        Gitlab::GitalyClient.migrate(:commit_patch, status: Gitlab::GitalyClient::MigrationStatus::OPT_OUT) do |is_enabled|
-          if is_enabled
-            @repository.gitaly_commit_client.patch(id)
-          else
-            rugged_diff_from_parent.patch
-          end
-        end
-      end
-
       # Returns a diff object for the changes from this commit's first parent.
       # If there is no parent, then the diff is between this commit and an
       # empty repo. See Repository#diff for keys allowed in the +options+
@@ -432,16 +417,6 @@ module Gitlab
         Gitlab::Git::CommitStats.new(@repository, self)
       end
 
-      def to_patch(options = {})
-        begin
-          rugged_commit.to_mbox(options)
-        rescue Rugged::InvalidError => ex
-          if ex.message =~ /commit \w+ is a merge commit/i
-            'Patch format is not currently supported for merge commits.'
-          end
-        end
-      end
-
       # Get ref names collection
       #
       # Ex.
diff --git a/spec/controllers/projects/commit_controller_spec.rb b/spec/controllers/projects/commit_controller_spec.rb
index 694c64ae1ad..003fec8ac68 100644
--- a/spec/controllers/projects/commit_controller_spec.rb
+++ b/spec/controllers/projects/commit_controller_spec.rb
@@ -79,41 +79,18 @@ describe Projects::CommitController do
     end
 
     describe "as diff" do
-      include_examples "export as", :diff
-      let(:format) { :diff }
+      it "triggers workhorse to serve the request" do
+        go(id: commit.id, format: :diff)
 
-      it "should really only be a git diff" do
-        go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format)
-
-        expect(response.body).to start_with("diff --git")
-      end
-
-      it "is only be a git diff without whitespace changes" do
-        go(id: '66eceea0db202bb39c4e445e8ca28689645366c5', format: format, w: 1)
-
-        expect(response.body).to start_with("diff --git")
-
-        # without whitespace option, there are more than 2 diff_splits for other formats
-        diff_splits = assigns(:diffs).diff_files.first.diff.diff.split("\n")
-        expect(diff_splits.length).to be <= 2
+        expect(response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-diff:")
       end
     end
 
     describe "as patch" do
-      include_examples "export as", :patch
-      let(:format) { :patch }
-      let(:commit2) { project.commit('498214de67004b1da3d820901307bed2a68a8ef6') }
-
-      it "is a git email patch" do
-        go(id: commit2.id, format: format)
-
-        expect(response.body).to start_with("From #{commit2.id}")
-      end
-
       it "contains a git diff" do
-        go(id: commit2.id, format: format)
+        go(id: commit.id, format: :patch)
 
-        expect(response.body).to match(/^diff --git/)
+        expect(response.headers[Gitlab::Workhorse::SEND_DATA_HEADER]).to start_with("git-format-patch:")
       end
     end
 
diff --git a/spec/lib/gitlab/git/commit_spec.rb b/spec/lib/gitlab/git/commit_spec.rb
index a05feaac1ca..2e068584c2e 100644
--- a/spec/lib/gitlab/git/commit_spec.rb
+++ b/spec/lib/gitlab/git/commit_spec.rb
@@ -554,24 +554,10 @@ describe Gitlab::Git::Commit, seed_helper: true do
     it_should_behave_like '#stats'
   end
 
-  describe '#to_diff' do
-    subject { commit.to_diff }
-
-    it { is_expected.not_to include "From #{SeedRepo::Commit::ID}" }
-    it { is_expected.to include 'diff --git a/files/ruby/popen.rb b/files/ruby/popen.rb'}
-  end
-
   describe '#has_zero_stats?' do
     it { expect(commit.has_zero_stats?).to eq(false) }
   end
 
-  describe '#to_patch' do
-    subject { commit.to_patch }
-
-    it { is_expected.to include "From #{SeedRepo::Commit::ID}" }
-    it { is_expected.to include 'diff --git a/files/ruby/popen.rb b/files/ruby/popen.rb'}
-  end
-
   describe '#to_hash' do
     let(:hash) { commit.to_hash }
     subject { hash }
diff --git a/spec/models/commit_spec.rb b/spec/models/commit_spec.rb
index 4e6b037a720..448b813c2e1 100644
--- a/spec/models/commit_spec.rb
+++ b/spec/models/commit_spec.rb
@@ -182,7 +182,6 @@ eos
     it { is_expected.to respond_to(:date) }
     it { is_expected.to respond_to(:diffs) }
     it { is_expected.to respond_to(:id) }
-    it { is_expected.to respond_to(:to_patch) }
   end
 
   describe '#closes_issues' do

From cdcde75bb782951b27ab9db0d54a71db7c94d7cb Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Fri, 11 May 2018 23:06:08 +0800
Subject: [PATCH 21/25] Only setup db in the first checkout!

---
 .gitlab-ci.yml           | 3 ++-
 scripts/prepare_build.sh | 6 +-----
 scripts/utils.sh         | 8 ++++++++
 3 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index b1445feee58..690e26711be 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -234,6 +234,7 @@ stages:
   <<: *dedicated-no-docs-and-no-qa-pull-cache-job
   variables:
     CREATE_DB_USER: "true"
+    SETUP_DB: "false"
   script:
     - git fetch https://gitlab.com/gitlab-org/gitlab-ce.git v9.3.0
     - git checkout -f FETCH_HEAD
@@ -242,7 +243,7 @@ stages:
     - cp config/gitlab.yml.example config/gitlab.yml
     - bundle exec rake db:drop db:create db:schema:load db:seed_fu
     - date
-    - git checkout $CI_COMMIT_SHA
+    - git checkout -f $CI_COMMIT_SHA
     - bundle install $BUNDLE_INSTALL_FLAGS
     - date
     - . scripts/prepare_build.sh
diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh
index 206d62dbc78..01cb01ed812 100644
--- a/scripts/prepare_build.sh
+++ b/scripts/prepare_build.sh
@@ -60,9 +60,5 @@ if [ "$CREATE_DB_USER" != "false" ]; then
 fi
 
 if [ "$SETUP_DB" != "false" ]; then
-    bundle exec rake db:drop db:create db:schema:load db:migrate
-
-    if [ "$GITLAB_DATABASE" = "mysql" ]; then
-        bundle exec rake add_limits_mysql
-    fi
+    setup_db
 fi
diff --git a/scripts/utils.sh b/scripts/utils.sh
index 6faa701f0ce..08c33f3f67e 100644
--- a/scripts/utils.sh
+++ b/scripts/utils.sh
@@ -12,3 +12,11 @@ retry() {
     done
     return 1
 }
+
+setup_db() {
+    bundle exec rake db:drop db:create db:schema:load db:migrate
+
+    if [ "$GITLAB_DATABASE" = "mysql" ]; then
+        bundle exec rake add_limits_mysql
+    fi
+}

From 0ab6469187285368d9f64f9ec67dbbcfa3e5a901 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 15 May 2018 01:49:46 +0800
Subject: [PATCH 22/25] Grant privileges after database is created

Never drop the database when granting privileges
---
 .gitlab-ci.yml                  |  3 +--
 scripts/create_mysql_user.sh    |  1 -
 scripts/create_postgres_user.sh |  4 +---
 scripts/prepare_build.sh        | 12 ++----------
 scripts/utils.sh                | 10 ++++++++++
 5 files changed, 14 insertions(+), 16 deletions(-)

diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 690e26711be..84d8e69b84e 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -189,7 +189,7 @@ stages:
   <<: *dedicated-no-docs-and-no-qa-pull-cache-job
   <<: *use-pg
   variables:
-    CREATE_DB_USER: "true"
+    SETUP_DB: "false"
   script:
     # Manually clone gitlab-test and only seed this project in
     # db/fixtures/development/04_project.rb thanks to SIZE=1 below
@@ -233,7 +233,6 @@ stages:
 .migration-paths: &migration-paths
   <<: *dedicated-no-docs-and-no-qa-pull-cache-job
   variables:
-    CREATE_DB_USER: "true"
     SETUP_DB: "false"
   script:
     - git fetch https://gitlab.com/gitlab-org/gitlab-ce.git v9.3.0
diff --git a/scripts/create_mysql_user.sh b/scripts/create_mysql_user.sh
index 286b1325f1d..35f68c581f3 100644
--- a/scripts/create_mysql_user.sh
+++ b/scripts/create_mysql_user.sh
@@ -1,7 +1,6 @@
 #!/bin/bash
 
 mysql --user=root --host=mysql <<EOF
-CREATE DATABASE IF NOT EXISTS gitlabhq_test DEFAULT CHARACTER SET utf8mb4 COLLATE utf8mb4_general_ci;
 CREATE USER IF NOT EXISTS 'gitlab'@'%';
 GRANT ALL PRIVILEGES ON gitlabhq_test.* TO 'gitlab'@'%';
 FLUSH PRIVILEGES;
diff --git a/scripts/create_postgres_user.sh b/scripts/create_postgres_user.sh
index 8a744df3226..8a049bcd7fb 100644
--- a/scripts/create_postgres_user.sh
+++ b/scripts/create_postgres_user.sh
@@ -1,8 +1,6 @@
 #!/bin/bash
 
 psql -h postgres -U postgres postgres <<EOF
-DROP DATABASE IF EXISTS gitlabhq_test;
-CREATE DATABASE gitlabhq_test;
 CREATE USER gitlab;
-GRANT ALL PRIVILEGES ON DATABASE gitlabhq_test TO gitlab;
+GRANT ALL PRIVILEGES ON ALL TABLES IN SCHEMA public TO gitlab;
 EOF
diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh
index 01cb01ed812..d8bcc9f8191 100644
--- a/scripts/prepare_build.sh
+++ b/scripts/prepare_build.sh
@@ -49,16 +49,8 @@ sed -i 's/localhost/redis/g' config/redis.queues.yml
 cp config/redis.shared_state.yml.example config/redis.shared_state.yml
 sed -i 's/localhost/redis/g' config/redis.shared_state.yml
 
-# Some tasks (e.g. db:seed_fu) need to have a properly-configured database
-# user but not necessarily a full schema loaded
-if [ "$CREATE_DB_USER" != "false" ]; then
-    if [ "$GITLAB_DATABASE" = 'postgresql' ]; then
-        . scripts/create_postgres_user.sh
-    else
-        . scripts/create_mysql_user.sh
-    fi
-fi
-
 if [ "$SETUP_DB" != "false" ]; then
     setup_db
+elif getent hosts postgres || getent hosts mysql; then
+    setup_db_user_only
 fi
diff --git a/scripts/utils.sh b/scripts/utils.sh
index 08c33f3f67e..2d2ba115563 100644
--- a/scripts/utils.sh
+++ b/scripts/utils.sh
@@ -13,7 +13,17 @@ retry() {
     return 1
 }
 
+setup_db_user_only() {
+    if [ "$GITLAB_DATABASE" = "postgresql" ]; then
+        . scripts/create_postgres_user.sh
+    else
+        . scripts/create_mysql_user.sh
+    fi
+}
+
 setup_db() {
+    setup_db_user_only
+
     bundle exec rake db:drop db:create db:schema:load db:migrate
 
     if [ "$GITLAB_DATABASE" = "mysql" ]; then

From e38938b332ca751dfc5e784f242d620016e8ca43 Mon Sep 17 00:00:00 2001
From: Stan Hu <stanhu@gmail.com>
Date: Tue, 15 May 2018 23:06:55 -0700
Subject: [PATCH 23/25] Fix Error 500 viewing admin page due to statement
 timeouts

Uses PostgreSQL tuple estimates to provide a much faster yet approximate
count. See https://wiki.postgresql.org/wiki/Slow_Counting for more details.
We only use this fast method if the table has been analyzed or vacuumed
within the last hour.

Closes #46255
---
 app/controllers/admin/dashboard_controller.rb |  2 +
 app/helpers/count_helper.rb                   |  5 ++
 app/views/admin/dashboard/index.html.haml     | 20 +++---
 lib/gitlab/database/count.rb                  | 48 ++++++++++++++
 spec/lib/gitlab/database/count_spec.rb        | 62 +++++++++++++++++++
 5 files changed, 127 insertions(+), 10 deletions(-)
 create mode 100644 app/helpers/count_helper.rb
 create mode 100644 lib/gitlab/database/count.rb
 create mode 100644 spec/lib/gitlab/database/count_spec.rb

diff --git a/app/controllers/admin/dashboard_controller.rb b/app/controllers/admin/dashboard_controller.rb
index e85cdcb8db7..d6a6bc7d4a1 100644
--- a/app/controllers/admin/dashboard_controller.rb
+++ b/app/controllers/admin/dashboard_controller.rb
@@ -1,4 +1,6 @@
 class Admin::DashboardController < Admin::ApplicationController
+  include CountHelper
+
   def index
     @projects = Project.order_id_desc.without_deleted.with_route.limit(10)
     @users = User.order_id_desc.limit(10)
diff --git a/app/helpers/count_helper.rb b/app/helpers/count_helper.rb
new file mode 100644
index 00000000000..24ee62e68ba
--- /dev/null
+++ b/app/helpers/count_helper.rb
@@ -0,0 +1,5 @@
+module CountHelper
+  def approximate_count_with_delimiters(model)
+    number_with_delimiter(Gitlab::Database::Count.approximate_count(model))
+  end
+end
diff --git a/app/views/admin/dashboard/index.html.haml b/app/views/admin/dashboard/index.html.haml
index bbf0e0fb95c..41ef646fc0e 100644
--- a/app/views/admin/dashboard/index.html.haml
+++ b/app/views/admin/dashboard/index.html.haml
@@ -10,7 +10,7 @@
             = link_to admin_projects_path do
               %h3.text-center
                 Projects:
-                = number_with_delimiter(Project.cached_count)
+                = approximate_count_with_delimiters(Project)
             %hr
             = link_to('New project', new_project_path, class: "btn btn-new")
       .col-sm-4
@@ -19,7 +19,7 @@
             = link_to admin_users_path do
               %h3.text-center
                 Users:
-                = number_with_delimiter(User.count)
+                = approximate_count_with_delimiters(User)
             %hr
             = link_to 'New user', new_admin_user_path, class: "btn btn-new"
       .col-sm-4
@@ -28,7 +28,7 @@
             = link_to admin_groups_path do
               %h3.text-center
                 Groups:
-                = number_with_delimiter(Group.count)
+                = approximate_count_with_delimiters(Group)
             %hr
             = link_to 'New group', new_admin_group_path, class: "btn btn-new"
     .row
@@ -39,31 +39,31 @@
             %p
               Forks
               %span.light.pull-right
-                = number_with_delimiter(ForkedProjectLink.count)
+                = approximate_count_with_delimiters(ForkedProjectLink)
             %p
               Issues
               %span.light.pull-right
-                = number_with_delimiter(Issue.count)
+                = approximate_count_with_delimiters(Issue)
             %p
               Merge Requests
               %span.light.pull-right
-                = number_with_delimiter(MergeRequest.count)
+                = approximate_count_with_delimiters(MergeRequest)
             %p
               Notes
               %span.light.pull-right
-                = number_with_delimiter(Note.count)
+                = approximate_count_with_delimiters(Note)
             %p
               Snippets
               %span.light.pull-right
-                = number_with_delimiter(Snippet.count)
+                = approximate_count_with_delimiters(Snippet)
             %p
               SSH Keys
               %span.light.pull-right
-                = number_with_delimiter(Key.count)
+                = approximate_count_with_delimiters(Key)
             %p
               Milestones
               %span.light.pull-right
-                = number_with_delimiter(Milestone.count)
+                = approximate_count_with_delimiters(Milestone)
             %p
               Active Users
               %span.light.pull-right
diff --git a/lib/gitlab/database/count.rb b/lib/gitlab/database/count.rb
new file mode 100644
index 00000000000..3374203960e
--- /dev/null
+++ b/lib/gitlab/database/count.rb
@@ -0,0 +1,48 @@
+# For large tables, PostgreSQL can take a long time to count rows due to MVCC.
+# We can optimize this by using the reltuples count as described in https://wiki.postgresql.org/wiki/Slow_Counting.
+module Gitlab
+  module Database
+    module Count
+      CONNECTION_ERRORS =
+        if defined?(PG)
+          [
+            ActionView::Template::Error,
+            ActiveRecord::StatementInvalid,
+            PG::Error
+          ].freeze
+        else
+          [
+            ActionView::Template::Error,
+            ActiveRecord::StatementInvalid
+          ].freeze
+        end
+
+      def self.approximate_count(model)
+        return model.count unless Gitlab::Database.postgresql?
+
+        execute_estimate_if_updated_recently(model) || model.count
+      end
+
+      def self.execute_estimate_if_updated_recently(model)
+        ActiveRecord::Base.connection.select_value(postgresql_estimate_query(model)).to_i if reltuples_updated_recently?(model)
+      rescue *CONNECTION_ERRORS
+      end
+
+      def self.reltuples_updated_recently?(model)
+        time = "to_timestamp(#{1.hour.ago.to_i})"
+        query = <<~SQL
+          SELECT 1 FROM pg_stat_user_tables WHERE relname = '#{model.table_name}' AND
+          (last_vacuum > #{time} OR last_autovacuum > #{time} OR last_analyze > #{time} OR last_autoanalyze > #{time})
+        SQL
+
+        ActiveRecord::Base.connection.select_all(query).count > 0
+      rescue *CONNECTION_ERRORS
+        false
+      end
+
+      def self.postgresql_estimate_query(model)
+        "SELECT reltuples::bigint AS estimate FROM pg_class where relname = '#{model.table_name}'"
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/database/count_spec.rb b/spec/lib/gitlab/database/count_spec.rb
new file mode 100644
index 00000000000..9d9caaabe16
--- /dev/null
+++ b/spec/lib/gitlab/database/count_spec.rb
@@ -0,0 +1,62 @@
+require 'spec_helper'
+
+describe Gitlab::Database::Count do
+  before do
+    create_list(:project, 3)
+  end
+
+  describe '.execute_estimate_if_updated_recently', :postgresql do
+    context 'when reltuples have not been updated' do
+      before do
+        expect(described_class).to receive(:reltuples_updated_recently?).and_return(false)
+      end
+
+      it 'returns nil' do
+        expect(described_class.execute_estimate_if_updated_recently(Project)).to be nil
+      end
+    end
+
+    context 'when reltuples have been updated' do
+      before do
+        ActiveRecord::Base.connection.execute('ANALYZE projects')
+      end
+
+      it 'calls postgresql_estimate_query' do
+        expect(described_class).to receive(:postgresql_estimate_query).with(Project).and_call_original
+        expect(described_class.execute_estimate_if_updated_recently(Project)).to eq(3)
+      end
+    end
+  end
+
+  describe '.approximate_count' do
+    context 'when reltuples have not been updated' do
+      it 'counts all projects the normal way' do
+        allow(described_class).to receive(:reltuples_updated_recently?).and_return(false)
+
+        expect(Project).to receive(:count).and_call_original
+        expect(described_class.approximate_count(Project)).to eq(3)
+      end
+    end
+
+    context 'no permission' do
+      it 'falls back to standard query' do
+        allow(described_class).to receive(:reltuples_updated_recently?).and_raise(PG::InsufficientPrivilege)
+
+        expect(Project).to receive(:count).and_call_original
+        expect(described_class.approximate_count(Project)).to eq(3)
+      end
+    end
+
+    describe 'when reltuples have been updated', :postgresql do
+      before do
+        ActiveRecord::Base.connection.execute('ANALYZE projects')
+      end
+
+      it 'counts all projects in the fast way' do
+        expect(described_class).to receive(:postgresql_estimate_query).with(Project).and_call_original
+
+        expect(described_class.approximate_count(Project)).to eq(3)
+      end
+    end
+  end
+end

From 539042a81e91e641fc93d80b1126e492f67b073f Mon Sep 17 00:00:00 2001
From: Annabel Dunstone Gray <annabel.dunstone@gmail.com>
Date: Wed, 16 May 2018 10:54:05 -0700
Subject: [PATCH 24/25] Remove unneccessary imports

---
 app/assets/stylesheets/pages/boards.scss | 2 --
 app/assets/stylesheets/pages/issues.scss | 2 --
 2 files changed, 4 deletions(-)

diff --git a/app/assets/stylesheets/pages/boards.scss b/app/assets/stylesheets/pages/boards.scss
index 011d38532b4..6bb40bae9ed 100644
--- a/app/assets/stylesheets/pages/boards.scss
+++ b/app/assets/stylesheets/pages/boards.scss
@@ -1,5 +1,3 @@
-@import './issues/issue_count_badge';
-
 [v-cloak] {
   display: none;
 }
diff --git a/app/assets/stylesheets/pages/issues.scss b/app/assets/stylesheets/pages/issues.scss
index b9390450477..0d17b9bae7e 100644
--- a/app/assets/stylesheets/pages/issues.scss
+++ b/app/assets/stylesheets/pages/issues.scss
@@ -1,5 +1,3 @@
-@import "./issues/issue_count_badge";
-
 .issues-list {
   .issue {
     padding: 10px 0 10px $gl-padding;

From 01275667e323d4702cc396f6f756305b06cba726 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=F0=9F=99=88=20=20jacopo=20beschi=20=F0=9F=99=89?=
 <intrip@gmail.com>
Date: Thu, 17 May 2018 09:19:47 +0000
Subject: [PATCH 25/25] Resolve "Opening Project with invite but without
 accepting leads to 404 error page"

---
 .../concerns/accepts_pending_invitations.rb   |  15 +++
 app/controllers/confirmations_controller.rb   |   4 +
 app/controllers/registrations_controller.rb   |   4 +-
 app/models/user.rb                            |  10 ++
 .../notify/member_invited_email.html.haml     |   2 +-
 .../unreleased/42531-open-invite-404.yml      |   5 +
 spec/features/invites_spec.rb                 | 112 ++++++++++++++++--
 spec/mailers/notify_spec.rb                   |   2 +-
 spec/models/user_spec.rb                      |  18 +++
 9 files changed, 157 insertions(+), 15 deletions(-)
 create mode 100644 app/controllers/concerns/accepts_pending_invitations.rb
 create mode 100644 changelogs/unreleased/42531-open-invite-404.yml

diff --git a/app/controllers/concerns/accepts_pending_invitations.rb b/app/controllers/concerns/accepts_pending_invitations.rb
new file mode 100644
index 00000000000..6e8aef52b52
--- /dev/null
+++ b/app/controllers/concerns/accepts_pending_invitations.rb
@@ -0,0 +1,15 @@
+module AcceptsPendingInvitations
+  extend ActiveSupport::Concern
+
+  def accept_pending_invitations
+    return unless resource.active_for_authentication?
+
+    clear_stored_location_for_resource if resource.accept_pending_invitations!.any?
+  end
+
+  def clear_stored_location_for_resource
+    session_key = stored_location_key_for(resource)
+
+    session.delete(session_key)
+  end
+end
diff --git a/app/controllers/confirmations_controller.rb b/app/controllers/confirmations_controller.rb
index 6d9c38d9581..7bc46a6ccc0 100644
--- a/app/controllers/confirmations_controller.rb
+++ b/app/controllers/confirmations_controller.rb
@@ -1,4 +1,6 @@
 class ConfirmationsController < Devise::ConfirmationsController
+  include AcceptsPendingInvitations
+
   def almost_there
     flash[:notice] = nil
     render layout: "devise_empty"
@@ -11,6 +13,8 @@ class ConfirmationsController < Devise::ConfirmationsController
   end
 
   def after_confirmation_path_for(resource_name, resource)
+    accept_pending_invitations
+
     # incoming resource can either be a :user or an :email
     if signed_in?(:user)
       after_sign_in(resource)
diff --git a/app/controllers/registrations_controller.rb b/app/controllers/registrations_controller.rb
index 1848c806c41..f5a222b3a48 100644
--- a/app/controllers/registrations_controller.rb
+++ b/app/controllers/registrations_controller.rb
@@ -1,5 +1,6 @@
 class RegistrationsController < Devise::RegistrationsController
   include Recaptcha::Verify
+  include AcceptsPendingInvitations
 
   before_action :whitelist_query_limiting, only: [:destroy]
 
@@ -16,6 +17,7 @@ class RegistrationsController < Devise::RegistrationsController
     end
 
     if !Gitlab::Recaptcha.load_configurations! || verify_recaptcha
+      accept_pending_invitations
       super
     else
       flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.'
@@ -60,7 +62,7 @@ class RegistrationsController < Devise::RegistrationsController
 
   def after_sign_up_path_for(user)
     Gitlab::AppLogger.info("User Created: username=#{user.username} email=#{user.email} ip=#{request.remote_ip} confirmed:#{user.confirmed?}")
-    user.confirmed? ? dashboard_projects_path : users_almost_there_path
+    user.confirmed? ? stored_location_for(user) || dashboard_projects_path : users_almost_there_path
   end
 
   def after_inactive_sign_up_path_for(resource)
diff --git a/app/models/user.rb b/app/models/user.rb
index 474fde36c02..8ef3c3ceff0 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -860,6 +860,16 @@ class User < ActiveRecord::Base
     confirmed? && !temp_oauth_email?
   end
 
+  def accept_pending_invitations!
+    pending_invitations.select do |member|
+      member.accept_invite!(self)
+    end
+  end
+
+  def pending_invitations
+    Member.where(invite_email: verified_emails).invite
+  end
+
   def all_emails
     all_emails = []
     all_emails << email unless temp_oauth_email?
diff --git a/app/views/notify/member_invited_email.html.haml b/app/views/notify/member_invited_email.html.haml
index b8b75da3f2f..6730172242b 100644
--- a/app/views/notify/member_invited_email.html.haml
+++ b/app/views/notify/member_invited_email.html.haml
@@ -4,7 +4,7 @@
     by
     = link_to member.created_by.name, user_url(member.created_by)
   to join the
-  = link_to member_source.human_name, member_source.web_url
+  = link_to member_source.human_name, member_source.public? ? member_source.web_url : invite_url(@token)
   #{member_source.model_name.singular} as #{member.human_access}.
 
 %p
diff --git a/changelogs/unreleased/42531-open-invite-404.yml b/changelogs/unreleased/42531-open-invite-404.yml
new file mode 100644
index 00000000000..73729f4a929
--- /dev/null
+++ b/changelogs/unreleased/42531-open-invite-404.yml
@@ -0,0 +1,5 @@
+---
+title: Automatically accepts project/group invite by email after user signup
+merge_request: 17634
+author: Jacopo Beschi @jacopo-beschi
+type: changed
diff --git a/spec/features/invites_spec.rb b/spec/features/invites_spec.rb
index e4be6193b8b..a986ddc4abc 100644
--- a/spec/features/invites_spec.rb
+++ b/spec/features/invites_spec.rb
@@ -5,18 +5,41 @@ describe 'Invites' do
   let(:owner) { create(:user, name: 'John Doe') }
   let(:group) { create(:group, name: 'Owned') }
   let(:project) { create(:project, :repository, namespace: group) }
-  let(:invite) { group.group_members.invite.last }
+  let(:group_invite) { group.group_members.invite.last }
 
   before do
     project.add_master(owner)
     group.add_user(owner, Gitlab::Access::OWNER)
     group.add_developer('user@example.com', owner)
-    invite.generate_invite_token!
+    group_invite.generate_invite_token!
+  end
+
+  def confirm_email_and_sign_in(new_user)
+    new_user_token = User.find_by_email(new_user.email).confirmation_token
+
+    visit user_confirmation_path(confirmation_token: new_user_token)
+    fill_in_sign_in_form(new_user)
+  end
+
+  def fill_in_sign_up_form(new_user)
+    fill_in 'new_user_name',                with: new_user.name
+    fill_in 'new_user_username',            with: new_user.username
+    fill_in 'new_user_email',               with: new_user.email
+    fill_in 'new_user_email_confirmation',  with: new_user.email
+    fill_in 'new_user_password',            with: new_user.password
+    click_button "Register"
+  end
+
+  def fill_in_sign_in_form(user)
+    fill_in 'user_login', with: user.email
+    fill_in 'user_password', with: user.password
+    check 'user_remember_me'
+    click_button 'Sign in'
   end
 
   context 'when signed out' do
     before do
-      visit invite_path(invite.raw_invite_token)
+      visit invite_path(group_invite.raw_invite_token)
     end
 
     it 'renders sign in page with sign in notice' do
@@ -25,12 +48,9 @@ describe 'Invites' do
     end
 
     it 'sign in and redirects to invitation page' do
-      fill_in 'user_login', with: user.email
-      fill_in 'user_password', with: user.password
-      check 'user_remember_me'
-      click_button 'Sign in'
+      fill_in_sign_in_form(user)
 
-      expect(current_path).to eq(invite_path(invite.raw_invite_token))
+      expect(current_path).to eq(invite_path(group_invite.raw_invite_token))
       expect(page).to have_content(
         'You have been invited by John Doe to join group Owned as Developer.'
       )
@@ -45,7 +65,7 @@ describe 'Invites' do
     end
 
     it 'shows message user already a member' do
-      visit invite_path(invite.raw_invite_token)
+      visit invite_path(group_invite.raw_invite_token)
       expect(page).to have_content('However, you are already a member of this group.')
     end
   end
@@ -53,7 +73,7 @@ describe 'Invites' do
   describe 'accepting the invitation' do
     before do
       sign_in(user)
-      visit invite_path(invite.raw_invite_token)
+      visit invite_path(group_invite.raw_invite_token)
     end
 
     it 'grants access and redirects to group page' do
@@ -69,7 +89,7 @@ describe 'Invites' do
     context 'when signed in' do
       before do
         sign_in(user)
-        visit invite_path(invite.raw_invite_token)
+        visit invite_path(group_invite.raw_invite_token)
       end
 
       it 'declines application and redirects to dashboard' do
@@ -83,7 +103,7 @@ describe 'Invites' do
 
     context 'when signed out' do
       before do
-        visit decline_invite_path(invite.raw_invite_token)
+        visit decline_invite_path(group_invite.raw_invite_token)
       end
 
       it 'declines application and redirects to sign in page' do
@@ -94,4 +114,72 @@ describe 'Invites' do
       end
     end
   end
+
+  describe 'invite an user using their email address' do
+    let(:new_user) { build_stubbed(:user) }
+    let(:invite_email) { new_user.email }
+    let(:group_invite) { create(:group_member, :invited, group: group, invite_email: invite_email) }
+    let!(:project_invite) { create(:project_member, :invited, project: project, invite_email: invite_email) }
+
+    before do
+      stub_application_setting(send_user_confirmation_email: send_email_confirmation)
+      visit invite_path(group_invite.raw_invite_token)
+    end
+
+    context 'email confirmation disabled' do
+      let(:send_email_confirmation) { false }
+
+      it 'signs up and redirects to the dashboard page with all the projects/groups invitations automatically accepted' do
+        fill_in_sign_up_form(new_user)
+
+        expect(current_path).to eq(dashboard_projects_path)
+        expect(page).to have_content(project.full_name)
+        visit group_path(group)
+        expect(page).to have_content(group.full_name)
+      end
+
+      context 'the user sign-up using a different email address' do
+        let(:invite_email) { build_stubbed(:user).email }
+
+        it 'signs up and redirects to the invitation page' do
+          fill_in_sign_up_form(new_user)
+
+          expect(current_path).to eq(invite_path(group_invite.raw_invite_token))
+        end
+      end
+    end
+
+    context 'email confirmation enabled' do
+      let(:send_email_confirmation) { true }
+
+      it 'signs up and redirects to root page with all the project/groups invitation automatically accepted' do
+        fill_in_sign_up_form(new_user)
+        confirm_email_and_sign_in(new_user)
+
+        expect(current_path).to eq(root_path)
+        expect(page).to have_content(project.full_name)
+        visit group_path(group)
+        expect(page).to have_content(group.full_name)
+      end
+
+      it "doesn't accept invitations until the user confirm his email" do
+        fill_in_sign_up_form(new_user)
+        sign_in(owner)
+
+        visit project_project_members_path(project)
+        expect(page).to have_content 'Invited'
+      end
+
+      context 'the user sign-up using a different email address' do
+        let(:invite_email) { build_stubbed(:user).email }
+
+        it 'signs up and redirects to the invitation page' do
+          fill_in_sign_up_form(new_user)
+          confirm_email_and_sign_in(new_user)
+
+          expect(current_path).to eq(invite_path(group_invite.raw_invite_token))
+        end
+      end
+    end
+  end
 end
diff --git a/spec/mailers/notify_spec.rb b/spec/mailers/notify_spec.rb
index 84ddbbbf2ee..8a52c151cc4 100644
--- a/spec/mailers/notify_spec.rb
+++ b/spec/mailers/notify_spec.rb
@@ -594,7 +594,7 @@ describe Notify do
       it 'contains all the useful information' do
         is_expected.to have_subject "Invitation to join the #{project.full_name} project"
         is_expected.to have_html_escaped_body_text project.full_name
-        is_expected.to have_body_text project.web_url
+        is_expected.to have_body_text project.full_name
         is_expected.to have_body_text project_member.human_access
         is_expected.to have_body_text project_member.invite_token
       end
diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb
index 8d3ddd1f87d..684fa030baf 100644
--- a/spec/models/user_spec.rb
+++ b/spec/models/user_spec.rb
@@ -1223,6 +1223,24 @@ describe User do
     end
   end
 
+  describe '#accept_pending_invitations!' do
+    let(:user) { create(:user, email: 'user@email.com') }
+    let!(:project_member_invite) { create(:project_member, :invited, invite_email: user.email) }
+    let!(:group_member_invite) { create(:group_member, :invited, invite_email: user.email) }
+    let!(:external_project_member_invite) { create(:project_member, :invited, invite_email: 'external@email.com') }
+    let!(:external_group_member_invite) { create(:group_member, :invited, invite_email: 'external@email.com') }
+
+    it 'accepts all the user members pending invitations and returns the accepted_members' do
+      accepted_members = user.accept_pending_invitations!
+
+      expect(accepted_members).to match_array([project_member_invite, group_member_invite])
+      expect(group_member_invite.reload).not_to be_invite
+      expect(project_member_invite.reload).not_to be_invite
+      expect(external_project_member_invite.reload).to be_invite
+      expect(external_group_member_invite.reload).to be_invite
+    end
+  end
+
   describe '#all_emails' do
     let(:user) { create(:user) }