Merge remote-tracking branch 'origin/master' into rc/ce-to-ee-friday

Signed-off-by: Rémy Coutable <remy@rymai.me>

.rubocop.yml

@@ -956,6 +956,10 @@ RSpec/DescribeClass:
 RSpec/DescribeMethod:
   Enabled: false
 
+# Avoid describing symbols.
+RSpec/DescribeSymbol:
+  Enabled: true
+
 # Checks that the second argument to top level describe is the tested method
 # name.
 RSpec/DescribedClass:

Gemfile

@@ -315,7 +315,7 @@ group :development, :test do
   gem 'spring-commands-spinach', '~> 1.1.0'
 
   gem 'rubocop', '~> 0.47.1', require: false
-  gem 'rubocop-rspec', '~> 1.12.0', require: false
+  gem 'rubocop-rspec', '~> 1.15.0', require: false
   gem 'scss_lint', '~> 0.47.0', require: false
   gem 'haml_lint', '~> 0.21.0', require: false
   gem 'simplecov', '~> 0.14.0', require: false

Gemfile.lock

@@ -704,7 +704,7 @@ GEM
       rainbow (>= 1.99.1, < 3.0)
       ruby-progressbar (~> 1.7)
       unicode-display_width (~> 1.0, >= 1.0.1)
-    rubocop-rspec (1.12.0)
+    rubocop-rspec (1.15.0)
       rubocop (>= 0.42.0)
     ruby-fogbugz (0.2.1)
       crack (~> 0.4)
@@ -1029,7 +1029,7 @@ DEPENDENCIES
   rspec-retry (~> 0.4.5)
   rspec_profiling (~> 0.0.5)
   rubocop (~> 0.47.1)
-  rubocop-rspec (~> 1.12.0)
+  rubocop-rspec (~> 1.15.0)
   ruby-fogbugz (~> 0.2.1)
   ruby-prof (~> 0.16.2)
   rufus-scheduler (~> 3.1.10)

app/assets/javascripts/build.js

@@ -82,6 +82,7 @@ window.Build = (function () {
 
   Build.prototype.getBuildTrace = function () {
     return $.ajax({
+<<<<<<< HEAD
       url: `${this.pageUrl}/trace.json`,
       dataType: 'json',
       data: {
@@ -92,6 +93,15 @@ window.Build = (function () {
 
         if (log.state) {
           this.state = log.state;
+=======
+      url: this.pageUrl + "/trace.json",
+      dataType: 'json',
+      success: function(buildData) {
+        $('.js-build-output').html(buildData.html);
+        gl.utils.setCiStatusFavicon(`${this.pageUrl}/status.json`);
+        if (window.location.hash === DOWN_BUILD_TRACE) {
+          $("html,body").scrollTop(this.$buildTrace.height());
+>>>>>>> origin/master
         }
 
         if (log.append) {

app/assets/javascripts/comment_type_toggle.js

+import DropLab from './droplab/drop_lab';
+import InputSetter from './droplab/plugins/input_setter';
+
+class CommentTypeToggle {
+  constructor(opts = {}) {
+    this.dropdownTrigger = opts.dropdownTrigger;
+    this.dropdownList = opts.dropdownList;
+    this.noteTypeInput = opts.noteTypeInput;
+    this.submitButton = opts.submitButton;
+    this.closeButton = opts.closeButton;
+    this.reopenButton = opts.reopenButton;
+  }
+
+  initDroplab() {
+    this.droplab = new DropLab();
+
+    const config = this.setConfig();
+
+    this.droplab.init(this.dropdownTrigger, this.dropdownList, [InputSetter], config);
+  }
+
+  setConfig() {
+    const config = {
+      InputSetter: [{
+        input: this.noteTypeInput,
+        valueAttribute: 'data-value',
+      },
+      {
+        input: this.submitButton,
+        valueAttribute: 'data-submit-text',
+      }],
+    };
+
+    if (this.closeButton) {
+      config.InputSetter.push({
+        input: this.closeButton,
+        valueAttribute: 'data-close-text',
+      }, {
+        input: this.closeButton,
+        valueAttribute: 'data-close-text',
+        inputAttribute: 'data-alternative-text',
+      });
+    }
+
+    if (this.reopenButton) {
+      config.InputSetter.push({
+        input: this.reopenButton,
+        valueAttribute: 'data-reopen-text',
+      }, {
+        input: this.reopenButton,
+        valueAttribute: 'data-reopen-text',
+        inputAttribute: 'data-alternative-text',
+      });
+    }
+
+    return config;
+  }
+}
+
+export default CommentTypeToggle;

app/assets/javascripts/diff_notes/components/comment_resolve_btn.js

@@ -42,10 +42,14 @@ import Vue from 'vue';
       }
     },
     created() {
+      if (this.discussionId) {
         this.discussion = CommentsStore.state[this.discussionId];
+      }
     },
     mounted: function () {
-      const $textarea = $(`#new-discussion-note-form-${this.discussionId} .note-textarea`);
+      if (!this.discussionId) return;
+
+      const $textarea = $(`.js-discussion-note-form[data-discussion-id=${this.discussionId}] .note-textarea`);
       this.textareaIsEmpty = $textarea.val() === '';
 
       $textarea.on('input.comment-and-resolve-btn', () => {
@@ -53,7 +57,9 @@ import Vue from 'vue';
       });
     },
     destroyed: function () {
-      $(`#new-discussion-note-form-${this.discussionId} .note-textarea`).off('input.comment-and-resolve-btn');
+      if (!this.discussionId) return;
+
+      $(`.js-discussion-note-form[data-discussion-id=${this.discussionId}] .note-textarea`).off('input.comment-and-resolve-btn');
     }
   });
 

app/assets/javascripts/dispatcher.js

@@ -385,6 +385,7 @@ const ShortcutsBlob = require('./shortcuts_blob');
           new Admin();
           switch (path[1]) {
             case 'application_settings':
+            case 'cohorts':
               new gl.ApplicationSettings();
               break;
             case 'groups':

app/assets/javascripts/droplab/drop_down.js

@@ -35,6 +35,11 @@ Object.assign(DropDown.prototype, {
   },
 
   clickEvent: function(e) {
+<<<<<<< HEAD
+=======
+    if (e.target.tagName === 'UL') return;
+
+>>>>>>> origin/master
     var selected = utils.closest(e.target, 'LI');
     if (!selected) return;
 

app/assets/javascripts/droplab/plugins/input_setter.js

@@ -35,8 +35,11 @@ const InputSetter = {
     const newValue = selectedItem.getAttribute(config.valueAttribute);
     const inputAttribute = config.inputAttribute;
 
+<<<<<<< HEAD
     if (!newValue) return;
 
+=======
+>>>>>>> origin/master
     if (input.hasAttribute(inputAttribute)) return input.setAttribute(inputAttribute, newValue);
     if (input.tagName === 'INPUT') return input.value = newValue;
     return input.textContent = newValue;

app/assets/javascripts/environments/stores/environments_store.js

@@ -64,7 +64,7 @@ export default class EnvironmentsStore {
       if (filtered.size === 1 && filtered.rollout_status_path) {
         filtered = Object.assign({}, filtered, {
           hasDeployBoard: true,
-          isDeployBoardVisible: false,
+          isDeployBoardVisible: true,
           deployBoardData: {},
         });
       }

app/assets/javascripts/files_comment_button.js

@@ -55,14 +55,19 @@ window.FilesCommentButton = (function() {
 
     textFileElement = this.getTextFileElement($currentTarget);
     buttonParentElement.append(this.buildButton({
+      discussionID: lineContentElement.attr('data-discussion-id'),
+      lineType: lineContentElement.attr('data-line-type'),
+
       noteableType: textFileElement.attr('data-noteable-type'),
       noteableID: textFileElement.attr('data-noteable-id'),
       commitID: textFileElement.attr('data-commit-id'),
       noteType: lineContentElement.attr('data-note-type'),
-      position: lineContentElement.attr('data-position'),
-      lineType: lineContentElement.attr('data-line-type'),
-      discussionID: lineContentElement.attr('data-discussion-id'),
-      lineCode: lineContentElement.attr('data-line-code')
+
+      // LegacyDiffNote
+      lineCode: lineContentElement.attr('data-line-code'),
+
+      // DiffNote
+      position: lineContentElement.attr('data-position')
     }));
   };
 
@@ -76,14 +81,19 @@ window.FilesCommentButton = (function() {
 
   FilesCommentButton.prototype.buildButton = function(buttonAttributes) {
     return $commentButtonTemplate.clone().attr({
+      'data-discussion-id': buttonAttributes.discussionID,
+      'data-line-type': buttonAttributes.lineType,
+
       'data-noteable-type': buttonAttributes.noteableType,
       'data-noteable-id': buttonAttributes.noteableID,
       'data-commit-id': buttonAttributes.commitID,
       'data-note-type': buttonAttributes.noteType,
+
+      // LegacyDiffNote
       'data-line-code': buttonAttributes.lineCode,
-      'data-position': buttonAttributes.position,
-      'data-discussion-id': buttonAttributes.discussionID,
-      'data-line-type': buttonAttributes.lineType
+
+      // DiffNote
+      'data-position': buttonAttributes.position
     });
   };
 
@@ -121,7 +131,7 @@ window.FilesCommentButton = (function() {
   };
 
   FilesCommentButton.prototype.validateLineContent = function(lineContentElement) {
-    return lineContentElement.attr('data-discussion-id') && lineContentElement.attr('data-discussion-id') !== '';
+    return lineContentElement.attr('data-note-type') && lineContentElement.attr('data-note-type') !== '';
   };
 
   return FilesCommentButton;

app/assets/javascripts/gl_form.js

@@ -29,7 +29,8 @@ GLForm.prototype.setupForm = function() {
     this.form.find('.div-dropzone').remove();
     this.form.addClass('gfm-form');
     // remove notify commit author checkbox for non-commit notes
-    gl.utils.disableButtonIfEmptyField(this.form.find('.js-note-text'), this.form.find('.js-comment-button'));
+    gl.utils.disableButtonIfEmptyField(this.form.find('.js-note-text'), this.form.find('.js-comment-button, .js-note-new-discussion'));
+
     gl.GfmAutoComplete.setup(this.form.find('.js-gfm-input'));
     new DropzoneInput(this.form);
     autosize(this.textarea);

app/assets/javascripts/main.js

@@ -287,7 +287,7 @@ $(function () {
   // Disable form buttons while a form is submitting
   $body.on('ajax:complete, ajax:beforeSend, submit', 'form', function (e) {
     var buttons;
-    buttons = $('[type="submit"]', this);
+    buttons = $('[type="submit"], .js-disable-on-submit', this);
     switch (e.type) {
       case 'ajax:beforeSend':
       case 'submit':

app/assets/javascripts/projects/settings_service_desk/components/service_desk_setting.js

@@ -18,6 +18,11 @@ export default {
       required: false,
       default: null,
     },
+    isInstanceAdmin: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
   },
 
   methods: {
@@ -32,15 +37,23 @@ export default {
       <div class="checkbox">
         <label for="service-desk-enabled-checkbox">
           <input
+            ref="enabled-checkbox"
             type="checkbox"
             id="service-desk-enabled-checkbox"
+            :disabled="!isInstanceAdmin"
             :checked="isEnabled"
             @change="onCheckboxToggle($event)">
           <span class="descr">
-            Activate service desk
+            Activate Service Desk
           </span>
         </label>
       </div>
+      <p
+        ref="only-instance-admin-activate-message"
+        v-if="!isInstanceAdmin"
+        class="settings-message">
+        Only instance admins can activate/deactivate Service Desk
+      </p>
       <template v-if="isEnabled">
         <div
           class="panel-slim panel-default">
@@ -55,7 +68,8 @@ export default {
               An error occurred while fetching the incoming email
             </template>
             <template v-else-if="incomingEmail">
-              <span ref="service-desk-incoming-email">
+              <span
+                ref="service-desk-incoming-email">
                 {{ incomingEmail }}
               </span>
               <button
@@ -74,7 +88,9 @@ export default {
             </template>
           </div>
         </div>
-        <p class="settings-message">
+        <p
+          ref="recommend-protect-email-from-spam-message"
+          class="settings-message">
           We recommend you protect the external support email address.
           Unblocked email spam would result in many spam issues being created,
           and may disrupt your GitLab service.

app/assets/javascripts/projects/settings_service_desk/service_desk_root.js

@@ -8,14 +8,17 @@ import eventHub from './event_hub';
 class ServiceDeskRoot {
   constructor(wrapperElement) {
     this.wrapperElement = wrapperElement;
-    const isEnabled = this.wrapperElement.dataset.enabled !== undefined &&
+    const isEnabled = typeof this.wrapperElement.dataset.enabled !== 'undefined' &&
       this.wrapperElement.dataset.enabled !== 'false';
     const incomingEmail = this.wrapperElement.dataset.incomingEmail;
     const endpoint = this.wrapperElement.dataset.endpoint;
+    const isInstanceAdmin = typeof this.wrapperElement.dataset.isInstanceAdmin !== 'undefined' &&
+      this.wrapperElement.dataset.isInstanceAdmin !== 'false';
 
     this.store = new ServiceDeskStore({
       isEnabled,
       incomingEmail,
+      isInstanceAdmin,
     });
     this.service = new ServiceDeskService(endpoint);
   }
@@ -37,7 +40,7 @@ class ServiceDeskRoot {
   }
 
   unbindEvents() {
-    eventHub.$on('serviceDeskEnabledCheckboxToggled', this.onEnableToggledWrapper);
+    eventHub.$off('serviceDeskEnabledCheckboxToggled', this.onEnableToggledWrapper);
   }
 
   render() {
@@ -48,7 +51,8 @@ class ServiceDeskRoot {
         <service-desk-setting
           :isEnabled="isEnabled"
           :incomingEmail="incomingEmail"
-          :fetchError="fetchError" />
+          :fetchError="fetchError"
+          :isInstanceAdmin="isInstanceAdmin" />
       `,
       components: {
         'service-desk-setting': ServiceDeskSetting,

app/assets/javascripts/projects/settings_service_desk/stores/service_desk_store.js

@@ -4,6 +4,7 @@ class ServiceDeskStore {
       isEnabled: false,
       incomingEmail: '',
       fetchError: null,
+      isInstanceAdmin: false,
     }, initialState);
   }
 
@@ -18,6 +19,10 @@ class ServiceDeskStore {
   setFetchError(value) {
     this.state.fetchError = value;
   }
+
+  setIsInstanceAdmin(value) {
+    this.state.isInstanceAdmin = value;
+  }
 }
 
 export default ServiceDeskStore;

app/assets/javascripts/render_gfm.js

@@ -8,6 +8,7 @@
   $.fn.renderGFM = function() {
     this.find('.js-syntax-highlight').syntaxHighlight();
     this.find('.js-render-math').renderMath();
+    return this;
   };
 
   $(document).on('ready load', function() {

app/assets/stylesheets/framework/filters.scss

@@ -446,8 +446,15 @@
   }
 }
 
+<<<<<<< HEAD
 .filter-dropdown-item.droplab-item-active .btn {
   @extend %filter-dropdown-item-btn-hover;
+=======
+.filter-dropdown-item.droplab-item-active {
+  .btn {
+    @extend %filter-dropdown-item-btn-hover;
+  }
+>>>>>>> origin/master
 }
 
 .filter-dropdown-loading {

app/assets/stylesheets/pages/note_form.scss

@@ -310,3 +310,94 @@
     margin-bottom: 10px;
   }
 }
+
+.comment-type-dropdown {
+  .comment-btn {
+    width: auto;
+  }
+
+  .dropdown-toggle {
+    float: right;
+
+    .toggle-icon {
+      color: $white-light;
+      padding-right: 2px;
+      margin-top: 2px;
+      pointer-events: none;
+    }
+  }
+
+  .dropdown-menu {
+    top: initial;
+    bottom: 40px;
+    width: 298px;
+  }
+
+  .description {
+    display: inline-block;
+    white-space: normal;
+    margin-left: 8px;
+    padding-right: 33px;
+  }
+
+  li {
+    padding-top: 6px;
+
+    & > a {
+      margin: 0;
+      padding: 0;
+      color: inherit;
+      border-radius: 0;
+      text-overflow: inherit;
+
+      &:hover,
+      &:focus {
+        background-color: inherit;
+        color: inherit;
+      }
+    }
+
+    &:hover,
+    &:focus {
+      background-color: $dropdown-hover-color;
+      color: $white-light;
+    }
+
+    &.droplab-item-selected i {
+      visibility: visible;
+    }
+
+    i {
+      visibility: hidden;
+    }
+  }
+
+  i {
+    display: inline-block;
+    vertical-align: top;
+    padding-top: 2px;
+  }
+
+  .divider {
+    margin: 0 8px;
+    padding: 0;
+    border-top: $gray-darkest;
+  }
+
+  @media (max-width: $screen-xs-max) {
+    display: flex;
+    width: 100%;
+
+    .comment-btn {
+      flex-grow: 1;
+      flex-shrink: 0;
+      width: auto;
+    }
+
+    .dropdown-toggle {
+      flex-grow: 0;
+      flex-shrink: 1;
+      width: auto;
+    }
+  }
+}

app/assets/stylesheets/pages/notes.scss

@@ -294,6 +294,18 @@ ul.notes {
         border-width: 1px;
       }
 
+      .discussion-notes {
+        &:not(:first-child) {
+          border-top: 1px solid $white-normal;
+          margin-top: 20px;
+        }
+
+        &:not(:last-child) {
+          border-bottom: 1px solid $white-normal;
+          margin-bottom: 20px;
+        }
+      }
+
       .notes {
         background-color: $white-light;
       }

app/controllers/admin/application_controller.rb

@@ -7,7 +7,7 @@ class Admin::ApplicationController < ApplicationController
   layout 'admin'
 
   def authenticate_admin!
-    render_404 unless current_user.is_admin?
+    render_404 unless current_user.admin?
   end
 
   def display_geo_information

app/controllers/admin/application_settings_controller.rb

@@ -19,7 +19,12 @@ class Admin::ApplicationSettingsController < Admin::ApplicationController
 
   def usage_data
     respond_to do |format|
-      format.html { render html: Gitlab::Highlight.highlight('payload.json', Gitlab::UsageData.to_json) }
+      format.html do
+        usage_data = Gitlab::UsageData.data
+        usage_data_json = params[:pretty] ? JSON.pretty_generate(usage_data) : usage_data.to_json
+
+        render html: Gitlab::Highlight.highlight('payload.json', usage_data_json)
+      end
       format.json { render json: Gitlab::UsageData.to_json }
     end
   end

app/controllers/admin/cohorts_controller.rb

+class Admin::CohortsController < Admin::ApplicationController
+  def index
+    if current_application_settings.usage_ping_enabled
+      cohorts_results = Rails.cache.fetch('cohorts', expires_in: 1.day) do
+        CohortsService.new.execute
+      end
+
+      @cohorts = CohortsSerializer.new.represent(cohorts_results)
+    end
+  end
+end

app/controllers/admin/impersonations_controller.rb

@@ -21,6 +21,6 @@ class Admin::ImpersonationsController < Admin::ApplicationController
   end
 
   def authenticate_impersonator!
-    render_404 unless impersonator && impersonator.is_admin? && !impersonator.blocked?
+    render_404 unless impersonator && impersonator.admin? && !impersonator.blocked?
   end
 end

app/controllers/concerns/renders_notes.rb

+module RendersNotes
+  def prepare_notes_for_rendering(notes)
+    preload_noteable_for_regular_notes(notes)
+    preload_max_access_for_authors(notes, @project)
+    Banzai::NoteRenderer.render(notes, @project, current_user)
+
+    notes
+  end
+
+  private
+
+  def preload_max_access_for_authors(notes, project)
+    user_ids = notes.map(&:author_id)
+    project.team.max_member_access_for_user_ids(user_ids)
+  end
+
+  def preload_noteable_for_regular_notes(notes)
+    ActiveRecord::Associations::Preloader.new.preload(notes.reject(&:for_commit?), :noteable)
+  end
+end

app/controllers/projects/commit_controller.rb

@@ -2,6 +2,7 @@
 #
 # Not to be confused with CommitsController, plural.
 class Projects::CommitController < Projects::ApplicationController
+  include RendersNotes
   include CreatesCommit
   include DiffForPath
   include DiffHelper
@@ -113,22 +114,19 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   def define_note_vars
-    @grouped_diff_discussions = commit.notes.grouped_diff_discussions
-    @notes = commit.notes.non_diff_notes.fresh
-
-    Banzai::NoteRenderer.render(
-      @grouped_diff_discussions.values.flat_map(&:notes) + @notes,
-      @project,
-      current_user,
-    )
-
+    @noteable = @commit
     @note = @project.build_commit_note(commit)
 
-    @noteable = @commit
-    @comments_target = {
+    @new_diff_note_attrs = {
       noteable_type: 'Commit',
       commit_id: @commit.id
     }
+
+    @grouped_diff_discussions = commit.grouped_diff_discussions
+    @discussions = commit.discussions
+
+    @notes = (@grouped_diff_discussions.values.flatten + @discussions).flat_map(&:notes)
+    @notes = prepare_notes_for_rendering(@notes)
   end
 
   def assign_change_commit_vars

app/controllers/projects/discussions_controller.rb

@@ -28,7 +28,7 @@ class Projects::DiscussionsController < Projects::ApplicationController
   end
 
   def discussion
-    @discussion ||= @merge_request.find_diff_discussion(params[:id]) || render_404
+    @discussion ||= @merge_request.find_discussion(params[:id]) || render_404
   end
 
   def authorize_resolve_discussion!

app/controllers/projects/issues_controller.rb

 class Projects::IssuesController < Projects::ApplicationController
-  include NotesHelper
+  include RendersNotes
   include ToggleSubscriptionAction
   include IssuableActions
   include ToggleAwardEmoji
@@ -85,15 +85,11 @@ class Projects::IssuesController < Projects::ApplicationController
   end
 
   def show
-    raw_notes = @issue.notes.inc_relations_for_view.fresh
-
-    @notes = Banzai::NoteRenderer.
-      render(raw_notes, @project, current_user, @path, @project_wiki, @ref)
-
-    @note     = @project.notes.new(noteable: @issue)
     @noteable = @issue
+    @note     = @project.notes.new(noteable: @issue)
 
-    preload_max_access_for_authors(@notes, @project)
+    @discussions = @issue.discussions
+    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
 
     respond_to do |format|
       format.html

app/controllers/projects/merge_requests_controller.rb

@@ -3,7 +3,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   include DiffForPath
   include DiffHelper
   include IssuableActions
-  include NotesHelper
+  include RendersNotes
   include ToggleAwardEmoji
   include IssuableCollections
 
@@ -635,20 +635,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController
     @note = @project.notes.new(noteable: @merge_request)
 
     @discussions = @merge_request.discussions
-
-    preload_noteable_for_regular_notes(@discussions.flat_map(&:notes))
-
-    # This is not executed lazily
-    @notes = Banzai::NoteRenderer.render(
-      @discussions.flat_map(&:notes),
-      @project,
-      current_user,
-      @path,
-      @project_wiki,
-      @ref
-    )
-
-    preload_max_access_for_authors(@notes, @project)
+    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
   end
 
   def define_widget_vars
@@ -661,22 +648,15 @@ class Projects::MergeRequestsController < Projects::ApplicationController
   end
 
   def define_diff_comment_vars
-    @comments_target = {
+    @new_diff_note_attrs = {
       noteable_type: 'MergeRequest',
       noteable_id: @merge_request.id
     }
 
     @use_legacy_diff_notes = !@merge_request.has_complete_diff_refs?
-    @grouped_diff_discussions = @merge_request.notes.inc_relations_for_view.grouped_diff_discussions
-
-    Banzai::NoteRenderer.render(
-      @grouped_diff_discussions.values.flat_map(&:notes),
-      @project,
-      current_user,
-      @path,
-      @project_wiki,
-      @ref
-    )
+
+    @grouped_diff_discussions = @merge_request.grouped_diff_discussions
+    @notes = prepare_notes_for_rendering(@grouped_diff_discussions.values.flatten.flat_map(&:notes))
   end
 
   def define_pipelines_vars

app/controllers/projects/notes_controller.rb

 class Projects::NotesController < Projects::ApplicationController
+  include RendersNotes
   include ToggleAwardEmoji
 
   # Authorize
@@ -6,13 +7,15 @@ class Projects::NotesController < Projects::ApplicationController
   before_action :authorize_create_note!, only: [:create]
   before_action :authorize_admin_note!, only: [:update, :destroy]
   before_action :authorize_resolve_note!, only: [:resolve, :unresolve]
-  before_action :find_current_user_notes, only: [:index]
 
   def index
     current_fetched_at = Time.now.to_i
 
     notes_json = { notes: [], last_fetched_at: current_fetched_at }
 
+    @notes = notes_finder.execute.inc_relations_for_view
+    @notes = prepare_notes_for_rendering(@notes)
+
     @notes.each do |note|
       next if note.cross_reference_not_visible_for?(current_user)
 
@@ -23,7 +26,10 @@ class Projects::NotesController < Projects::ApplicationController
   end
 
   def create
-    create_params = note_params.merge(merge_request_diff_head_sha: params[:merge_request_diff_head_sha])
+    create_params = note_params.merge(
+      merge_request_diff_head_sha: params[:merge_request_diff_head_sha],
+      in_reply_to_discussion_id: params[:in_reply_to_discussion_id]
+    )
     @note = Notes::CreateService.new(project, current_user, create_params).execute
 
     if @note.is_a?(Note)
@@ -111,6 +117,17 @@ class Projects::NotesController < Projects::ApplicationController
     )
   end
 
+  def discussion_html(discussion)
+    return if discussion.individual_note?
+
+    render_to_string(
+      "discussions/_discussion",
+      layout: false,
+      formats: [:html],
+      locals: { discussion: discussion }
+    )
+  end
+
   def diff_discussion_html(discussion)
     return unless discussion.diff_discussion?
 
@@ -118,13 +135,13 @@ class Projects::NotesController < Projects::ApplicationController
       template = "discussions/_parallel_diff_discussion"
       locals =
         if params[:line_type] == 'old'
-          { discussion_left: discussion, discussion_right: nil }
+          { discussions_left: [discussion], discussions_right: nil }
         else
-          { discussion_left: nil, discussion_right: discussion }
+          { discussions_left: nil, discussions_right: [discussion] }
         end
     else
       template = "discussions/_diff_discussion"
-      locals = { discussion: discussion }
+      locals = { discussions: [discussion] }
     end
 
     render_to_string(
@@ -135,54 +152,28 @@ class Projects::NotesController < Projects::ApplicationController
     )
   end
 
-  def discussion_html(discussion)
-    return unless discussion.diff_discussion?
-
-    render_to_string(
-      "discussions/_discussion",
-      layout: false,
-      formats: [:html],
-      locals: { discussion: discussion }
-    )
-  end
-
   def note_json(note)
     attrs = {
-      id: note.id
+      commands_changes: note.commands_changes
     }
 
     if note.persisted?
-      Banzai::NoteRenderer.render([note], @project, current_user)
-
       attrs.merge!(
         valid: true,
-        discussion_id: note.discussion_id,
+        id: note.id,
+        discussion_id: note.discussion_id(noteable),
         html: note_html(note),
         note: note.note
       )
 
-      if note.diff_note?
-        discussion = note.to_discussion
-
+      discussion = note.to_discussion(noteable)
+      unless discussion.individual_note?
         attrs.merge!(
+          discussion_resolvable: discussion.resolvable?,
+
           diff_discussion_html: diff_discussion_html(discussion),
           discussion_html: discussion_html(discussion)
         )
-
-        # The discussion_id is used to add the comment to the correct discussion
-        # element on the merge request page. Among other things, the discussion_id
-        # contains the sha of head commit of the merge request.
-        # When new commits are pushed into the merge request after the initial
-        # load of the merge request page, the discussion elements will still have
-        # the old discussion_ids, with the old head commit sha. The new comment,
-        # however, will have the new discussion_id with the new commit sha.
-        # To ensure that these new comments will still end up in the correct
-        # discussion element, we also send the original discussion_id, with the
-        # old commit sha, along, and fall back on this value when no discussion
-        # element with the new discussion_id could be found.
-        if note.new_diff_note? && note.position != note.original_position
-          attrs[:original_discussion_id] = note.original_discussion_id
-        end
       end
     else
       attrs.merge!(
@@ -191,7 +182,6 @@ class Projects::NotesController < Projects::ApplicationController
       )
     end
 
-    attrs[:commands_changes] = note.commands_changes
     attrs
   end
 
@@ -205,14 +195,30 @@ class Projects::NotesController < Projects::ApplicationController
 
   def note_params
     params.require(:note).permit(
-      :note, :noteable, :noteable_id, :noteable_type, :project_id,
-      :attachment, :line_code, :commit_id, :type, :position
+      :project_id,
+      :noteable_type,
+      :noteable_id,
+      :commit_id,
+      :noteable,
+      :type,
+
+      :note,
+      :attachment,
+
+      # LegacyDiffNote
+      :line_code,
+
+      # DiffNote
+      :position
     )
   end
 
-  def find_current_user_notes
-    @notes = NotesFinder.new(project, current_user, params.merge(last_fetched_at: last_fetched_at))
-      .execute.inc_author
+  def notes_finder
+    @notes_finder ||= NotesFinder.new(project, current_user, params.merge(last_fetched_at: last_fetched_at))
+  end
+
+  def noteable
+    @noteable ||= notes_finder.target
   end
 
   def last_fetched_at

app/controllers/projects/service_desk_controller.rb

@@ -24,6 +24,6 @@ class Projects::ServiceDeskController < Projects::ApplicationController
   end
 
   def authorize_admin_instance!
-    return render_404 unless current_user.is_admin?
+    return render_404 unless current_user.admin?
   end
 end

app/controllers/projects/snippets_controller.rb

 class Projects::SnippetsController < Projects::ApplicationController
+  include RendersNotes
   include ToggleAwardEmoji
   include SpammableActions
   include SnippetsActions
@@ -55,8 +56,10 @@ class Projects::SnippetsController < Projects::ApplicationController
 
   def show
     @note = @project.notes.new(noteable: @snippet)
-    @notes = Banzai::NoteRenderer.render(@snippet.notes.fresh, @project, current_user)
     @noteable = @snippet
+
+    @discussions = @snippet.discussions
+    @notes = prepare_notes_for_rendering(@discussions.flat_map(&:notes))
   end
 
   def destroy

app/finders/notes_finder.rb

@@ -17,20 +17,37 @@ class NotesFinder
     @project = project
     @current_user = current_user
     @params = params
-    init_collection
   end
 
   def execute
-    @notes = since_fetch_at(@params[:last_fetched_at]) if @params[:last_fetched_at]
-    @notes
+    notes = init_collection
+    notes = since_fetch_at(notes)
+    notes.fresh
+  end
+
+  def target
+    return @target if defined?(@target)
+
+    target_type = @params[:target_type]
+    target_id   = @params[:target_id]
+
+    return @target = nil unless target_type && target_id
+
+    @target =
+      if target_type == "commit"
+        if Ability.allowed?(@current_user, :download_code, @project)
+          @project.commit(target_id)
+        end
+      else
+        noteables_for_type(target_type).find(target_id)
+      end
   end
 
   private
 
   def init_collection
-    @notes =
-      if @params[:target_id]
-        on_target(@params[:target_type], @params[:target_id])
+    if target
+      notes_on_target
     else
       notes_of_any_type
     end
@@ -39,7 +56,7 @@ class NotesFinder
   def notes_of_any_type
     types = %w(commit issue merge_request snippet)
     note_relations = types.map { |t| notes_for_type(t) }
-    note_relations.map!{ |notes| search(@params[:search], notes) } if @params[:search]
+    note_relations.map! { |notes| search(notes) }
     UnionFinder.new.find_union(note_relations, Note)
   end
 
@@ -69,35 +86,33 @@ class NotesFinder
     end
   end
 
-  def on_target(target_type, target_id)
-    if target_type == "commit"
-      notes_for_type('commit').for_commit_id(target_id)
-    else
-      target = noteables_for_type(target_type).find(target_id)
-
+  def notes_on_target
     if target.respond_to?(:related_notes)
       target.related_notes
     else
       target.notes
     end
   end
-  end
 
   # Searches for notes matching the given query.
   #
   # This method uses ILIKE on PostgreSQL and LIKE on MySQL.
   #
-  def search(query, notes_relation = @notes)
+  def search(notes)
+    query = @params[:search]
+    return notes unless query
+
     pattern = "%#{query}%"
-    notes_relation.where(Note.arel_table[:note].matches(pattern))
+    notes.where(Note.arel_table[:note].matches(pattern))
   end
 
   # Notes changed since last fetch
   # Uses overlapping intervals to avoid worrying about race conditions
-  def since_fetch_at(fetch_time)
+  def since_fetch_at(notes)
+    return notes unless @params[:last_fetched_at]
+
     # Default to 0 to remain compatible with old clients
     last_fetched_at = Time.at(@params.fetch(:last_fetched_at, 0).to_i)
-
-    @notes.where('updated_at > ?', last_fetched_at - FETCH_OVERLAP).fresh
+    notes.updated_after(last_fetched_at - FETCH_OVERLAP)
   end
 end

app/helpers/diff_helper.rb

@@ -62,19 +62,19 @@ module DiffHelper
   end
 
   def parallel_diff_discussions(left, right, diff_file)
-    discussion_left = discussion_right = nil
+    discussions_left = discussions_right = nil
 
     if left && (left.unchanged? || left.removed?)
       line_code = diff_file.line_code(left)
-      discussion_left = @grouped_diff_discussions[line_code]
+      discussions_left = @grouped_diff_discussions[line_code]
     end
 
     if right && right.added?
       line_code = diff_file.line_code(right)
-      discussion_right = @grouped_diff_discussions[line_code]
+      discussions_right = @grouped_diff_discussions[line_code]
     end
 
-    [discussion_left, discussion_right]
+    [discussions_left, discussions_right]
   end
 
   def inline_diff_btn

app/helpers/license_helper.rb

@@ -15,7 +15,7 @@ module LicenseHelper
   # in_html is set to false from an initializer, which shouldn't try to render
   # HTML links.
   #
-  def license_message(signed_in: signed_in?, is_admin: (current_user && current_user.is_admin?), in_html: true)
+  def license_message(signed_in: signed_in?, is_admin: (current_user && current_user.admin?), in_html: true)
     @license_message =
       if License.current
         yes_license_message(signed_in, is_admin)

app/helpers/notes_helper.rb

@@ -24,57 +24,24 @@ module NotesHelper
   end
 
   def diff_view_data
-    return {} unless @comments_target
+    return {} unless @new_diff_note_attrs
 
-    @comments_target.slice(:noteable_id, :noteable_type, :commit_id)
+    @new_diff_note_attrs.slice(:noteable_id, :noteable_type, :commit_id)
   end
 
   def diff_view_line_data(line_code, position, line_type)
     return if @diff_notes_disabled
 
-    use_legacy_diff_note = @use_legacy_diff_notes
-    # If the controller doesn't force the use of legacy diff notes, we
-    # determine this on a line-by-line basis by seeing if there already exist
-    # active legacy diff notes at this line, in which case newly created notes
-    # will use the legacy technology as well.
-    # We do this because the discussion_id values of legacy and "new" diff
-    # notes, which are used to group notes on the merge request discussion tab,
-    # are incompatible.
-    # If we didn't, diff notes that would show for the same line on the changes
-    # tab, would show in different discussions on the discussion tab.
-    use_legacy_diff_note ||= begin
-      discussion = @grouped_diff_discussions[line_code]
-      discussion && discussion.legacy_diff_discussion?
-    end
-
     data = {
       line_code: line_code,
       line_type: line_type,
     }
 
-    if use_legacy_diff_note
-      discussion_id = LegacyDiffNote.discussion_id(
-        @comments_target[:noteable_type],
-        @comments_target[:noteable_id] || @comments_target[:commit_id],
-        line_code
-      )
-
-      data.merge!(
-        note_type: LegacyDiffNote.name,
-        discussion_id: discussion_id
-      )
+    if @use_legacy_diff_notes
+      data[:note_type] = LegacyDiffNote.name
     else
-      discussion_id = DiffNote.discussion_id(
-        @comments_target[:noteable_type],
-        @comments_target[:noteable_id] || @comments_target[:commit_id],
-        position
-      )
-
-      data.merge!(
-        position: position.to_json,
-        note_type: DiffNote.name,
-        discussion_id: discussion_id
-      )
+      data[:note_type] = DiffNote.name
+      data[:position] = position.to_json
     end
 
     data
@@ -83,21 +50,12 @@ module NotesHelper
   def link_to_reply_discussion(discussion, line_type = nil)
     return unless current_user
 
-    data = discussion.reply_attributes.merge(line_type: line_type)
+    data = { discussion_id: discussion.id, line_type: line_type }
 
     button_tag 'Reply...', class: 'btn btn-text-field js-discussion-reply-button',
                            data: data, title: 'Add a reply'
   end
 
-  def preload_max_access_for_authors(notes, project)
-    user_ids = notes.map(&:author_id)
-    project.team.max_member_access_for_user_ids(user_ids)
-  end
-
-  def preload_noteable_for_regular_notes(notes)
-    ActiveRecord::Associations::Preloader.new.preload(notes.select { |note| !note.for_commit? }, :noteable)
-  end
-
   def note_max_access_for_user(note)
     note.project.team.human_max_access(note.author_id)
   end

app/helpers/visibility_level_helper.rb

@@ -85,7 +85,7 @@ module VisibilityLevelHelper
   end
 
   def restricted_visibility_levels(show_all = false)
-    return [] if current_user.is_admin? && !show_all
+    return [] if current_user.admin? && !show_all
     current_application_settings.restricted_visibility_levels || []
   end
 

app/mailers/emails/ee/service_desk.rb

 module Emails
   module EE
     module ServiceDesk
+      extend ActiveSupport::Concern
+
+      included do
+        layout 'service_desk', only: [:service_desk_thank_you_email, :service_desk_new_note_email]
+      end
+
       def service_desk_thank_you_email(issue_id)
         setup_service_desk_mail(issue_id)
 

app/mailers/emails/notes.rb

@@ -4,13 +4,8 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @commit = @note.noteable
-      @discussion = @note.to_discussion if @note.diff_note?
       @target_url = namespace_project_commit_url(*note_target_url_options)
-
-      mail_answer_thread(@commit,
-                         from: sender(@note.author_id),
-                         to: recipient(recipient_id),
-                         subject: subject("#{@commit.title} (#{@commit.short_id})"))
+      mail_answer_thread(@commit, note_thread_options(recipient_id))
     end
 
     def note_issue_email(recipient_id, note_id)
@@ -25,7 +20,6 @@ module Emails
       setup_note_mail(note_id, recipient_id)
 
       @merge_request = @note.noteable
-      @discussion = @note.to_discussion if @note.diff_note?
       @target_url = namespace_project_merge_request_url(*note_target_url_options)
       mail_answer_thread(@merge_request, note_thread_options(recipient_id))
     end
@@ -56,15 +50,18 @@ module Emails
       {
         from: sender(@note.author_id),
         to: recipient(recipient_id),
-        subject: subject("#{@note.noteable.title} (#{@note.noteable.to_reference})")
+        subject: subject("#{@note.noteable.title} (#{@note.noteable.reference_link_text})")
       }
     end
 
     def setup_note_mail(note_id, recipient_id)
-      @note = Note.find(note_id)
+      # `note_id` is a `Note` when originating in `NotifyPreview`
+      @note = note_id.is_a?(Note) ? note_id : Note.find(note_id)
       @project = @note.project
 
+      if @project && @note.persisted?
         @sent_notification = SentNotification.record_note(@note, recipient_id, reply_key)
       end
     end
+  end
 end

app/mailers/notify.rb

@@ -115,7 +115,7 @@ class Notify < BaseMailer
     headers["X-GitLab-#{model.class.name}-ID"] = model.id
     headers['X-GitLab-Reply-Key'] = reply_key
 
-    if Gitlab::IncomingEmail.enabled?
+    if Gitlab::IncomingEmail.enabled? && @sent_notification
       address = Mail::Address.new(Gitlab::IncomingEmail.reply_address(reply_key))
       address.display_name = @project.name_with_namespace
 
@@ -180,6 +180,6 @@ class Notify < BaseMailer
     end
 
     headers['List-Unsubscribe'] = list_unsubscribe_methods.map { |e| "<#{e}>" }.join(',')
-    @sent_notification_url = unsubscribe_sent_notification_url(@sent_notification)
+    @unsubscribe_url = unsubscribe_sent_notification_url(@sent_notification)
   end
 end

app/models/ci/build.rb

@@ -236,6 +236,7 @@ module Ci
 
     def has_trace?
       trace.exist?
+<<<<<<< HEAD
     end
 
     def trace=(data)
@@ -246,6 +247,18 @@ module Ci
       read_attribute(:trace)
     end
 
+=======
+    end
+
+    def trace=(data)
+      raise NotImplementedError
+    end
+
+    def old_trace
+      read_attribute(:trace)
+    end
+
+>>>>>>> origin/master
     def erase_old_trace!
       write_attribute(:trace, nil)
       save

app/models/commit.rb

@@ -2,6 +2,7 @@ class Commit
   extend ActiveModel::Naming
 
   include ActiveModel::Conversion
+  include Noteable
   include Participable
   include Mentionable
   include Referable
@@ -203,6 +204,10 @@ class Commit
     project.notes.for_commit_id(self.id)
   end
 
+  def discussion_notes
+    notes.non_diff_notes
+  end
+
   def notes_with_associations
     notes.includes(:author)
   end

app/models/concerns/discussion_on_diff.rb

+# Contains functionality shared between `DiffDiscussion` and `LegacyDiffDiscussion`.
+module DiscussionOnDiff
+  extend ActiveSupport::Concern
+
+  included do
+    NUMBER_OF_TRUNCATED_DIFF_LINES = 16
+
+    memoized_values << :active
+
+    delegate  :line_code,
+              :original_line_code,
+              :diff_file,
+              :diff_line,
+              :for_line?,
+              :active?,
+
+              to: :first_note
+
+    delegate  :file_path,
+              :blob,
+              :highlighted_diff_lines,
+              :diff_lines,
+
+              to: :diff_file,
+              allow_nil: true
+  end
+
+  def diff_discussion?
+    true
+  end
+
+  def active?
+    return @active if @active.present?
+
+    @active = first_note.active?
+  end
+
+  # Returns an array of at most 16 highlighted lines above a diff note
+  def truncated_diff_lines(highlight: true)
+    lines = highlight ? highlighted_diff_lines : diff_lines
+    prev_lines = []
+
+    lines.each do |line|
+      if line.meta?
+        prev_lines.clear
+      else
+        prev_lines << line
+
+        break if for_line?(line)
+
+        prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
+      end
+    end
+
+    prev_lines
+  end
+end

app/models/concerns/ignorable_column.rb

+# Module that can be included into a model to make it easier to ignore database
+# columns.
+#
+# Example:
+#
+#     class User < ActiveRecord::Base
+#       include IgnorableColumn
+#
+#       ignore_column :updated_at
+#     end
+#
+module IgnorableColumn
+  extend ActiveSupport::Concern
+
+  module ClassMethods
+    def columns
+      super.reject { |column| ignored_columns.include?(column.name) }
+    end
+
+    def ignored_columns
+      @ignored_columns ||= Set.new
+    end
+
+    def ignore_column(name)
+      ignored_columns << name.to_s
+    end
+  end
+end

app/models/concerns/issuable.rb

@@ -303,17 +303,6 @@ module Issuable
     self.class.to_ability_name
   end
 
-  # Convert this Issuable class name to a format usable by notifications.
-  #
-  # Examples:
-  #
-  #   issuable.class           # => MergeRequest
-  #   issuable.human_class_name # => "merge request"
-
-  def human_class_name
-    @human_class_name ||= self.class.name.titleize.downcase
-  end
-
   # Returns a Hash of attributes to be used for Twitter card metadata
   def card_attributes
     {

app/models/concerns/note_on_diff.rb

+# Contains functionality shared between `DiffNote` and `LegacyDiffNote`.
 module NoteOnDiff
   extend ActiveSupport::Concern
 
@@ -24,12 +25,4 @@ module NoteOnDiff
   def diff_attributes
     raise NotImplementedError
   end
-
-  def can_be_award_emoji?
-    false
-  end
-
-  def to_discussion
-    Discussion.new([self])
-  end
 end

app/models/concerns/noteable.rb

+module Noteable
+  # Names of all implementers of `Noteable` that support resolvable notes.
+  RESOLVABLE_TYPES = %w(MergeRequest).freeze
+
+  def base_class_name
+    self.class.base_class.name
+  end
+
+  # Convert this Noteable class name to a format usable by notifications.
+  #
+  # Examples:
+  #
+  #   noteable.class           # => MergeRequest
+  #   noteable.human_class_name # => "merge request"
+  def human_class_name
+    @human_class_name ||= base_class_name.titleize.downcase
+  end
+
+  def supports_resolvable_notes?
+    RESOLVABLE_TYPES.include?(base_class_name)
+  end
+
+  def supports_discussions?
+    DiscussionNote::NOTEABLE_TYPES.include?(base_class_name)
+  end
+
+  def discussion_notes
+    notes
+  end
+
+  delegate :find_discussion, to: :discussion_notes
+
+  def discussions
+    @discussions ||= discussion_notes
+      .inc_relations_for_view
+      .discussions(self)
+  end
+
+  def grouped_diff_discussions
+    # Doesn't use `discussion_notes`, because this may include commit diff notes
+    # besides MR diff notes, that we do no want to display on the MR Changes tab.
+    notes.inc_relations_for_view.grouped_diff_discussions
+  end
+
+  def resolvable_discussions
+    @resolvable_discussions ||= discussion_notes.resolvable.discussions(self)
+  end
+
+  def discussions_resolvable?
+    resolvable_discussions.any?(&:resolvable?)
+  end
+
+  def discussions_resolved?
+    discussions_resolvable? && resolvable_discussions.none?(&:to_be_resolved?)
+  end
+
+  def discussions_to_be_resolved?
+    discussions_resolvable? && !discussions_resolved?
+  end
+
+  def discussions_to_be_resolved
+    @discussions_to_be_resolved ||= resolvable_discussions.select(&:to_be_resolved?)
+  end
+
+  def discussions_can_be_resolved_by?(user)
+    discussions_to_be_resolved.all? { |discussion| discussion.can_resolve?(user) }
+  end
+end

app/models/concerns/resolvable_discussion.rb

+module ResolvableDiscussion
+  extend ActiveSupport::Concern
+
+  included do
+    # A number of properties of this `Discussion`, like `first_note` and `resolvable?`, are memoized.
+    # When this discussion is resolved or unresolved, the values of these properties potentially change.
+    # To make sure all memoized values are reset when this happens, `update` resets all instance variables with names in
+    # `memoized_variables`. If you add a memoized method in `ResolvableDiscussion` or any `Discussion` subclass,
+    # please make sure the instance variable name is added to `memoized_values`, like below.
+    cattr_accessor :memoized_values, instance_accessor: false do
+      []
+    end
+
+    memoized_values.push(
+      :resolvable,
+      :resolved,
+      :first_note,
+      :first_note_to_resolve,
+      :last_resolved_note,
+      :last_note
+    )
+
+    delegate :potentially_resolvable?, to: :first_note
+
+    delegate  :resolved_at,
+              :resolved_by,
+
+              to: :last_resolved_note,
+              allow_nil: true
+  end
+
+  def resolvable?
+    return @resolvable if @resolvable.present?
+
+    @resolvable = potentially_resolvable? && notes.any?(&:resolvable?)
+  end
+
+  def resolved?
+    return @resolved if @resolved.present?
+
+    @resolved = resolvable? && notes.none?(&:to_be_resolved?)
+  end
+
+  def first_note
+    @first_note ||= notes.first
+  end
+
+  def first_note_to_resolve
+    return unless resolvable?
+
+    @first_note_to_resolve ||= notes.find(&:to_be_resolved?)
+  end
+
+  def last_resolved_note
+    return unless resolved?
+
+    @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last
+  end
+
+  def resolved_notes
+    notes.select(&:resolved?)
+  end
+
+  def to_be_resolved?
+    resolvable? && !resolved?
+  end
+
+  def can_resolve?(current_user)
+    return false unless current_user
+    return false unless resolvable?
+
+    current_user == self.noteable.author ||
+      current_user.can?(:resolve_note, self.project)
+  end
+
+  def resolve!(current_user)
+    return unless resolvable?
+
+    update { |notes| notes.resolve!(current_user) }
+  end
+
+  def unresolve!
+    return unless resolvable?
+
+    update { |notes| notes.unresolve! }
+  end
+
+  private
+
+  def update
+    # Do not select `Note.resolvable`, so that system notes remain in the collection
+    notes_relation = Note.where(id: notes.map(&:id))
+
+    yield(notes_relation)
+
+    # Set the notes array to the updated notes
+    @notes = notes_relation.fresh.to_a
+
+    self.class.memoized_values.each do |var|
+      instance_variable_set(:"@#{var}", nil)
+    end
+  end
+end

app/models/concerns/resolvable_note.rb

+module ResolvableNote
+  extend ActiveSupport::Concern
+
+  # Names of all subclasses of `Note` that can be resolvable.
+  RESOLVABLE_TYPES = %w(DiffNote DiscussionNote).freeze
+
+  included do
+    belongs_to :resolved_by, class_name: "User"
+
+    validates :resolved_by, presence: true, if: :resolved?
+
+    # Keep this scope in sync with `#potentially_resolvable?`
+    scope :potentially_resolvable, -> { where(type: RESOLVABLE_TYPES).where(noteable_type: Noteable::RESOLVABLE_TYPES) }
+    # Keep this scope in sync with `#resolvable?`
+    scope :resolvable, -> { potentially_resolvable.user }
+
+    scope :resolved, -> { resolvable.where.not(resolved_at: nil) }
+    scope :unresolved, -> { resolvable.where(resolved_at: nil) }
+  end
+
+  module ClassMethods
+    # This method must be kept in sync with `#resolve!`
+    def resolve!(current_user)
+      unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)
+    end
+
+    # This method must be kept in sync with `#unresolve!`
+    def unresolve!
+      resolved.update_all(resolved_at: nil, resolved_by_id: nil)
+    end
+  end
+
+  # Keep this method in sync with the `potentially_resolvable` scope
+  def potentially_resolvable?
+    RESOLVABLE_TYPES.include?(self.class.name) && noteable.supports_resolvable_notes?
+  end
+
+  # Keep this method in sync with the `resolvable` scope
+  def resolvable?
+    potentially_resolvable? && !system?
+  end
+
+  def resolved?
+    return false unless resolvable?
+
+    self.resolved_at.present?
+  end
+
+  def to_be_resolved?
+    resolvable? && !resolved?
+  end
+
+  # If you update this method remember to also update `.resolve!`
+  def resolve!(current_user)
+    return unless resolvable?
+    return if resolved?
+
+    self.resolved_at = Time.now
+    self.resolved_by = current_user
+    save!
+  end
+
+  # If you update this method remember to also update `.unresolve!`
+  def unresolve!
+    return unless resolvable?
+    return unless resolved?
+
+    self.resolved_at = nil
+    self.resolved_by = nil
+    save!
+  end
+end

app/models/diff_discussion.rb

+# A discussion on merge request or commit diffs consisting of `DiffNote` notes.
+#
+# A discussion of this type can be resolvable.
+class DiffDiscussion < Discussion
+  include DiscussionOnDiff
+
+  def self.note_class
+    DiffNote
+  end
+
+  delegate  :position,
+            :original_position,
+
+            to: :first_note
+
+  def legacy_diff_discussion?
+    false
+  end
+
+  def reply_attributes
+    super.merge(
+      original_position: original_position.to_json,
+      position: position.to_json,
+    )
+  end
+end

app/models/diff_note.rb

+# A note on merge request or commit diffs
+#
+# A note of this type can be resolvable.
 class DiffNote < Note
   # Elastic search configuration (it does not support STI properly)
   document_type 'note'
@@ -6,6 +9,8 @@ class DiffNote < Note
 
   include NoteOnDiff
 
+  NOTEABLE_TYPES = %w(MergeRequest Commit).freeze
+
   serialize :original_position, Gitlab::Diff::Position
   serialize :position, Gitlab::Diff::Position
 
@@ -13,49 +18,20 @@ class DiffNote < Note
   validates :position, presence: true
   validates :diff_line, presence: true
   validates :line_code, presence: true, line_code: true
-  validates :noteable_type, inclusion: { in: %w(Commit MergeRequest) }
-  validates :resolved_by, presence: true, if: :resolved?
+  validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
   validate :positions_complete
   validate :verify_supported
 
-  # Keep this scope in sync with the logic in `#resolvable?`
-  scope :resolvable, -> { user.where(noteable_type: 'MergeRequest') }
-  scope :resolved, -> { resolvable.where.not(resolved_at: nil) }
-  scope :unresolved, -> { resolvable.where(resolved_at: nil) }
-
-  after_initialize :ensure_original_discussion_id
   before_validation :set_original_position, :update_position, on: :create
-  before_validation :set_line_code, :set_original_discussion_id
-  # We need to do this again, because it's already in `Note`, but is affected by
-  # `update_position` and needs to run after that.
-  before_validation :set_discussion_id
+  before_validation :set_line_code
   after_save :keep_around_commits
 
-  class << self
-    def build_discussion_id(noteable_type, noteable_id, position)
-      [super(noteable_type, noteable_id), *position.key].join("-")
-    end
-
-    # This method must be kept in sync with `#resolve!`
-    def resolve!(current_user)
-      unresolved.update_all(resolved_at: Time.now, resolved_by_id: current_user.id)
-    end
-
-    # This method must be kept in sync with `#unresolve!`
-    def unresolve!
-      resolved.update_all(resolved_at: nil, resolved_by_id: nil)
-    end
-  end
-
-  def new_diff_note?
-    true
+  def discussion_class(*)
+    DiffDiscussion
   end
 
-  def diff_attributes
-    { position: position.to_json }
-  end
-
-  def position=(new_position)
+  %i(original_position position).each do |meth|
+    define_method "#{meth}=" do |new_position|
       if new_position.is_a?(String)
         new_position = JSON.parse(new_position) rescue nil
       end
@@ -67,6 +43,7 @@ class DiffNote < Note
 
       super(new_position)
     end
+  end
 
   def diff_file
     @diff_file ||= self.original_position.diff_file(self.project.repository)
@@ -93,43 +70,6 @@ class DiffNote < Note
     self.position.diff_refs == diff_refs
   end
 
-  # If you update this method remember to also update the scope `resolvable`
-  def resolvable?
-    !system? && for_merge_request?
-  end
-
-  def resolved?
-    return false unless resolvable?
-
-    self.resolved_at.present?
-  end
-
-  # If you update this method remember to also update `.resolve!`
-  def resolve!(current_user)
-    return unless resolvable?
-    return if resolved?
-
-    self.resolved_at = Time.now
-    self.resolved_by = current_user
-    save!
-  end
-
-  # If you update this method remember to also update `.unresolve!`
-  def unresolve!
-    return unless resolvable?
-    return unless resolved?
-
-    self.resolved_at = nil
-    self.resolved_by = nil
-    save!
-  end
-
-  def discussion
-    return unless resolvable?
-
-    self.noteable.find_diff_discussion(self.discussion_id)
-  end
-
   private
 
   def supported?
@@ -145,33 +85,13 @@ class DiffNote < Note
   end
 
   def set_original_position
-    self.original_position = self.position.dup
+    self.original_position = self.position.dup unless self.original_position&.complete?
   end
 
   def set_line_code
     self.line_code = self.position.line_code(self.project.repository)
   end
 
-  def ensure_original_discussion_id
-    return unless self.persisted?
-    return if self.original_discussion_id
-
-    set_original_discussion_id
-    update_column(:original_discussion_id, self.original_discussion_id)
-  end
-
-  def set_original_discussion_id
-    self.original_discussion_id = Digest::SHA1.hexdigest(build_original_discussion_id)
-  end
-
-  def build_discussion_id
-    self.class.build_discussion_id(noteable_type, noteable_id || commit_id, position)
-  end
-
-  def build_original_discussion_id
-    self.class.build_discussion_id(noteable_type, noteable_id || commit_id, original_position)
-  end
-
   def update_position
     return unless supported?
     return if for_commit?

app/models/discussion.rb

+# A non-diff discussion on an issue, merge request, commit, or snippet, consisting of `DiscussionNote` notes.
+#
+# A discussion of this type can be resolvable.
 class Discussion
-  NUMBER_OF_TRUNCATED_DIFF_LINES = 16
+  include ResolvableDiscussion
 
-  attr_reader :notes
+  attr_reader :notes, :context_noteable
 
   delegate  :created_at,
             :project,
@@ -11,43 +14,62 @@ class Discussion
             :for_commit?,
             :for_merge_request?,
 
-            :line_code,
-            :original_line_code,
-            :diff_file,
-            :for_line?,
-            :active?,
-
             to: :first_note
 
-  delegate  :resolved_at,
-            :resolved_by,
+  def self.build(notes, context_noteable = nil)
+    notes.first.discussion_class(context_noteable).new(notes, context_noteable)
+  end
 
-            to: :last_resolved_note,
-            allow_nil: true
+  def self.build_collection(notes, context_noteable = nil)
+    notes.group_by { |n| n.discussion_id(context_noteable) }.values.map { |notes| build(notes, context_noteable) }
+  end
 
-  delegate  :blob,
-            :highlighted_diff_lines,
-            :diff_lines,
+  # Returns an alphanumeric discussion ID based on `build_discussion_id`
+  def self.discussion_id(note)
+    Digest::SHA1.hexdigest(build_discussion_id(note).join("-"))
+  end
 
-            to: :diff_file,
-            allow_nil: true
+  # Returns an array of discussion ID components
+  def self.build_discussion_id(note)
+    [*base_discussion_id(note), SecureRandom.hex]
+  end
 
-  def self.for_notes(notes)
-    notes.group_by(&:discussion_id).values.map { |notes| new(notes) }
+  def self.base_discussion_id(note)
+    noteable_id = note.noteable_id || note.commit_id
+    [:discussion, note.noteable_type.try(:underscore), noteable_id]
   end
 
-  def self.for_diff_notes(notes)
-    notes.group_by(&:line_code).values.map { |notes| new(notes) }
+  # When notes on a commit are displayed in context of a merge request that contains that commit,
+  # these notes are to be displayed as if they were part of one discussion, even though they were actually
+  # individual notes on the commit with different discussion IDs, so that it's clear that these are not
+  # notes on the merge request itself.
+  #
+  # To turn a list of notes into a list of discussions, they are grouped by discussion ID, so to
+  # get these out-of-context notes to end up in the same discussion, we need to get them to return the same
+  # `discussion_id` when this grouping happens. To enable this, `Note#discussion_id` calls out
+  # to the `override_discussion_id` method on the appropriate `Discussion` subclass, as determined by
+  # the `discussion_class` method on `Note` or a subclass of `Note`.
+  #
+  # If no override is necessary, return `nil`.
+  # For the case described above, see `OutOfContextDiscussion.override_discussion_id`.
+  def self.override_discussion_id(note)
+    nil
   end
 
-  def initialize(notes)
-    @notes = notes
+  def self.note_class
+    DiscussionNote
   end
 
-  def last_resolved_note
-    return unless resolved?
+  def initialize(notes, context_noteable = nil)
+    @notes = notes
+    @context_noteable = context_noteable
+  end
 
-    @last_resolved_note ||= resolved_notes.sort_by(&:resolved_at).last
+  def ==(other)
+    other.class == self.class &&
+      other.context_noteable == self.context_noteable &&
+      other.id == self.id &&
+      other.notes == self.notes
   end
 
   def last_updated_at
@@ -59,91 +81,29 @@ class Discussion
   end
 
   def id
-    first_note.discussion_id
+    first_note.discussion_id(context_noteable)
   end
 
   alias_method :to_param, :id
 
   def diff_discussion?
-    first_note.diff_note?
-  end
-
-  def legacy_diff_discussion?
-    notes.any?(&:legacy_diff_note?)
+    false
   end
 
-  def resolvable?
-    return @resolvable if @resolvable.present?
-
-    @resolvable = diff_discussion? && notes.any?(&:resolvable?)
-  end
-
-  def resolved?
-    return @resolved if @resolved.present?
-
-    @resolved = resolvable? && notes.none?(&:to_be_resolved?)
+  def individual_note?
+    false
   end
 
-  def first_note
-    @first_note ||= @notes.first
-  end
-
-  def first_note_to_resolve
-    @first_note_to_resolve ||= notes.detect(&:to_be_resolved?)
+  def new_discussion?
+    notes.length == 1
   end
 
   def last_note
-    @last_note ||= @notes.last
-  end
-
-  def resolved_notes
-    notes.select(&:resolved?)
-  end
-
-  def to_be_resolved?
-    resolvable? && !resolved?
-  end
-
-  def can_resolve?(current_user)
-    return false unless current_user
-    return false unless resolvable?
-
-    current_user == self.noteable.author ||
-      current_user.can?(:resolve_note, self.project)
-  end
-
-  def resolve!(current_user)
-    return unless resolvable?
-
-    update { |notes| notes.resolve!(current_user) }
-  end
-
-  def unresolve!
-    return unless resolvable?
-
-    update { |notes| notes.unresolve! }
-  end
-
-  def for_target?(target)
-    self.noteable == target && !diff_discussion?
-  end
-
-  def active?
-    return @active if @active.present?
-
-    @active = first_note.active?
+    @last_note ||= notes.last
   end
 
   def collapsed?
-    return false unless diff_discussion?
-
-    if resolvable?
-      # New diff discussions only disappear once they are marked resolved
     resolved?
-    else
-      # Old diff discussions disappear once they become outdated
-      !active?
-    end
   end
 
   def expanded?
@@ -151,52 +111,6 @@ class Discussion
   end
 
   def reply_attributes
-    data = {
-      noteable_type: first_note.noteable_type,
-      noteable_id:   first_note.noteable_id,
-      commit_id:     first_note.commit_id,
-      discussion_id: self.id,
-    }
-
-    if diff_discussion?
-      data[:note_type] = first_note.type
-
-      data.merge!(first_note.diff_attributes)
-    end
-
-    data
-  end
-
-  # Returns an array of at most 16 highlighted lines above a diff note
-  def truncated_diff_lines(highlight: true)
-    lines = highlight ? highlighted_diff_lines : diff_lines
-    prev_lines = []
-
-    lines.each do |line|
-      if line.meta?
-        prev_lines.clear
-      else
-        prev_lines << line
-
-        break if for_line?(line)
-
-        prev_lines.shift if prev_lines.length >= NUMBER_OF_TRUNCATED_DIFF_LINES
-      end
-    end
-
-    prev_lines
-  end
-
-  private
-
-  def update
-    notes_relation = DiffNote.where(id: notes.map(&:id)).fresh
-    yield(notes_relation)
-
-    # Set the notes array to the updated notes
-    @notes = notes_relation.to_a
-
-    # Reset the memoized values
-    @last_resolved_note = @resolvable = @resolved = @first_note = @last_note = nil
+    first_note.slice(:type, :noteable_type, :noteable_id, :commit_id, :discussion_id)
   end
 end

app/models/discussion_note.rb

+# A note in a non-diff discussion on an issue, merge request, commit, or snippet.
+#
+# A note of this type can be resolvable.
+class DiscussionNote < Note
+  # Names of all implementers of `Noteable` that support discussions.
+  NOTEABLE_TYPES = %w(MergeRequest Issue Commit Snippet).freeze
+
+  validates :noteable_type, inclusion: { in: NOTEABLE_TYPES }
+
+  def discussion_class(*)
+    Discussion
+  end
+end

app/models/ee/protected_branch_access.rb

@@ -45,7 +45,7 @@ module EE
     end
 
     def check_access(user)
-      return true if user.is_admin?
+      return true if user.admin?
       return user.id == self.user_id if self.user.present?
       return group.users.exists?(user.id) if self.group.present?
 

app/models/individual_note_discussion.rb

+# A discussion to wrap a single `Note` note on the root of an issue, merge request,
+# commit, or snippet, that is not displayed as a discussion.
+#
+# A discussion of this type is never resolvable.
+class IndividualNoteDiscussion < Discussion
+  def self.note_class
+    Note
+  end
+
+  def individual_note?
+    true
+  end
+end

app/models/issue.rb

@@ -5,6 +5,7 @@ class Issue < ActiveRecord::Base
 
   include InternalId
   include Issuable
+  include Noteable
   include Referable
   include Sortable
   include Spammable

app/models/legacy_diff_discussion.rb

+# A discussion on merge request or commit diffs consisting of `LegacyDiffNote` notes.
+#
+# All new diff discussions are of the type `DiffDiscussion`, but any diff discussions created
+# before the introduction of the new implementation still use `LegacyDiffDiscussion`.
+#
+# A discussion of this type is never resolvable.
+class LegacyDiffDiscussion < Discussion
+  include DiscussionOnDiff
+
+  def legacy_diff_discussion?
+    true
+  end
+
+  def self.note_class
+    LegacyDiffNote
+  end
+
+  def collapsed?
+    !active?
+  end
+
+  def reply_attributes
+    super.merge(line_code: line_code)
+  end
+end

app/models/legacy_diff_note.rb

+# A note on merge request or commit diffs, using the legacy implementation.
+#
+# All new diff notes are of the type `DiffNote`, but any diff notes created
+# before the introduction of the new implementation still use `LegacyDiffNote`.
+#
+# A note of this type is never resolvable.
 class LegacyDiffNote < Note
   # Elastic search configuration (it does not support STI properly)
   document_type 'note'
@@ -12,18 +18,8 @@ class LegacyDiffNote < Note
 
   before_create :set_diff
 
-  class << self
-    def build_discussion_id(noteable_type, noteable_id, line_code)
-      [super(noteable_type, noteable_id), line_code].join("-")
-    end
-  end
-
-  def legacy_diff_note?
-    true
-  end
-
-  def diff_attributes
-    { line_code: line_code }
+  def discussion_class(*)
+    LegacyDiffDiscussion
   end
 
   def project_repository
@@ -124,8 +120,4 @@ class LegacyDiffNote < Note
     diffs = noteable.raw_diffs(Commit.max_diff_options)
     diffs.find { |d| d.new_path == self.diff.new_path }
   end
-
-  def build_discussion_id
-    self.class.build_discussion_id(noteable_type, noteable_id || commit_id, line_code)
-  end
 end

app/models/merge_request.rb

 class MergeRequest < ActiveRecord::Base
   include InternalId
   include Issuable
+  include Noteable
   include Referable
   include Sortable
   include Elastic::MergeRequestsSearch
@@ -498,43 +499,7 @@ class MergeRequest < ActiveRecord::Base
     )
   end
 
-  def discussions
-    @discussions ||= self.related_notes.
-      inc_relations_for_view.
-      fresh.
-      discussions
-  end
-
-  def diff_discussions
-    @diff_discussions ||= self.notes.diff_notes.discussions
-  end
-
-  def resolvable_discussions
-    @resolvable_discussions ||= diff_discussions.select(&:to_be_resolved?)
-  end
-
-  def discussions_can_be_resolved_by?(user)
-    resolvable_discussions.all? { |discussion| discussion.can_resolve?(user) }
-  end
-
-  def find_diff_discussion(discussion_id)
-    notes = self.notes.diff_notes.where(discussion_id: discussion_id).fresh.to_a
-    return if notes.empty?
-
-    Discussion.new(notes)
-  end
-
-  def discussions_resolvable?
-    diff_discussions.any?(&:resolvable?)
-  end
-
-  def discussions_resolved?
-    discussions_resolvable? && diff_discussions.none?(&:to_be_resolved?)
-  end
-
-  def discussions_to_be_resolved?
-    discussions_resolvable? && !discussions_resolved?
-  end
+  alias_method :discussion_notes, :related_notes
 
   def mergeable_discussions_state?
     return true unless project.only_allow_merge_if_all_discussions_are_resolved?
@@ -924,8 +889,8 @@ class MergeRequest < ActiveRecord::Base
     return unless has_complete_diff_refs?
     return if new_diff_refs == old_diff_refs
 
-    active_diff_notes = self.notes.diff_notes.select do |note|
-      note.new_diff_note? && note.active?(old_diff_refs)
+    active_diff_notes = self.notes.new_diff_notes.select do |note|
+      note.active?(old_diff_refs)
     end
 
     return if active_diff_notes.empty?

app/models/note.rb

+# A note on the root of an issue, merge request, commit, or snippet.
+#
+# A note of this type is never resolvable.
 class Note < ActiveRecord::Base
   extend ActiveModel::Naming
   include Gitlab::CurrentSettings
@@ -9,6 +12,10 @@ class Note < ActiveRecord::Base
   include FasterCacheKeys
   include CacheMarkdownField
   include AfterCommitQueue
+  include ResolvableNote
+  include IgnorableColumn
+
+  ignore_column :original_discussion_id
 
   cache_markdown_field :note, pipeline: :note
 
@@ -33,9 +40,6 @@ class Note < ActiveRecord::Base
   belongs_to :author, class_name: "User"
   belongs_to :updated_by, class_name: "User"
 
-  # Only used by DiffNote, but defined here so that it can be used in `Note.includes`
-  belongs_to :resolved_by, class_name: "User"
-
   has_many :todos, dependent: :destroy
   has_many :events, as: :target, dependent: :destroy
   has_one :system_note_metadata
@@ -55,10 +59,11 @@ class Note < ActiveRecord::Base
   validates :noteable_id, presence: true, unless: [:for_commit?, :importing?]
   validates :commit_id, presence: true, if: :for_commit?
   validates :author, presence: true
+  validates :discussion_id, presence: true, format: { with: /\A\h{40}\z/ }
 
   validate unless: [:for_commit?, :importing?, :for_personal_snippet?] do |note|
     unless note.noteable.try(:project) == note.project
-      errors.add(:invalid_project, 'Note and noteable project mismatch')
+      errors.add(:project, 'does not match noteable project')
     end
   end
 
@@ -71,6 +76,7 @@ class Note < ActiveRecord::Base
   scope :user, ->{ where(system: false) }
   scope :common, ->{ where(noteable_type: ["", nil]) }
   scope :fresh, ->{ order(created_at: :asc, id: :asc) }
+  scope :updated_after, ->(time){ where('updated_at > ?', time) }
   scope :inc_author_project, ->{ includes(:project, :author) }
   scope :inc_author, ->{ includes(:author) }
   scope :inc_relations_for_view, -> do
@@ -78,7 +84,8 @@ class Note < ActiveRecord::Base
   end
 
   scope :diff_notes, ->{ where(type: %w(LegacyDiffNote DiffNote)) }
-  scope :non_diff_notes, ->{ where(type: ['Note', nil]) }
+  scope :new_diff_notes, ->{ where(type: 'DiffNote') }
+  scope :non_diff_notes, ->{ where(type: ['Note', 'DiscussionNote', nil]) }
 
   scope :with_associations, -> do
     # FYI noteable cannot be loaded for LegacyDiffNote for commits
@@ -88,7 +95,7 @@ class Note < ActiveRecord::Base
 
   after_initialize :ensure_discussion_id
   before_validation :nullify_blank_type, :nullify_blank_line_code
-  before_validation :set_discussion_id
+  before_validation :set_discussion_id, on: :create
   after_save :keep_around_commit, unless: :for_personal_snippet?
   after_save :expire_etag_cache
 
@@ -97,22 +104,23 @@ class Note < ActiveRecord::Base
       ActiveModel::Name.new(self, nil, 'note')
     end
 
-    def build_discussion_id(noteable_type, noteable_id)
-      [:discussion, noteable_type.try(:underscore), noteable_id].join("-")
+    def discussions(context_noteable = nil)
+      Discussion.build_collection(fresh, context_noteable)
     end
 
-    def discussion_id(*args)
-      Digest::SHA1.hexdigest(build_discussion_id(*args))
-    end
+    def find_discussion(discussion_id)
+      notes = where(discussion_id: discussion_id).fresh.to_a
+      return if notes.empty?
 
-    def discussions
-      Discussion.for_notes(fresh)
+      Discussion.build(notes)
     end
 
     def grouped_diff_discussions
-      active_notes = diff_notes.fresh.select(&:active?)
-      Discussion.for_diff_notes(active_notes).
-        map { |d| [d.line_code, d] }.to_h
+      diff_notes.
+        fresh.
+        discussions.
+        select(&:active?).
+        group_by(&:line_code)
     end
 
     def count_for_collection(ids, type)
@@ -127,37 +135,17 @@ class Note < ActiveRecord::Base
   end
 
   def cross_reference?
-    system && SystemNoteService.cross_reference?(note)
+    system? && SystemNoteService.cross_reference?(note)
   end
 
   def diff_note?
     false
   end
 
-  def legacy_diff_note?
-    false
-  end
-
-  def new_diff_note?
-    false
-  end
-
   def active?
     true
   end
 
-  def resolvable?
-    false
-  end
-
-  def resolved?
-    false
-  end
-
-  def to_be_resolved?
-    resolvable? && !resolved?
-  end
-
   def max_attachment_size
     current_application_settings.max_attachment_size.megabytes.to_i
   end
@@ -234,7 +222,7 @@ class Note < ActiveRecord::Base
   end
 
   def can_be_award_emoji?
-    noteable.is_a?(Awardable)
+    noteable.is_a?(Awardable) && !part_of_discussion?
   end
 
   def contains_emoji_only?
@@ -245,6 +233,63 @@ class Note < ActiveRecord::Base
     for_personal_snippet? ? 'personal_snippet' : noteable_type.underscore
   end
 
+  def can_be_discussion_note?
+    self.noteable.supports_discussions? && !part_of_discussion?
+  end
+
+  def discussion_class(noteable = nil)
+    # When commit notes are rendered on an MR's Discussion page, they are
+    # displayed in one discussion instead of individually.
+    # See also `#discussion_id` and `Discussion.override_discussion_id`.
+    if noteable && noteable != self.noteable
+      OutOfContextDiscussion
+    else
+      IndividualNoteDiscussion
+    end
+  end
+
+  # See `Discussion.override_discussion_id` for details.
+  def discussion_id(noteable = nil)
+    discussion_class(noteable).override_discussion_id(self) || super()
+  end
+
+  # Returns a discussion containing just this note.
+  # This method exists as an alternative to `#discussion` to use when the methods
+  # we intend to call on the Discussion object don't require it to have all of its notes,
+  # and just depend on the first note or the type of discussion. This saves us a DB query.
+  def to_discussion(noteable = nil)
+    Discussion.build([self], noteable)
+  end
+
+  # Returns the entire discussion this note is part of.
+  # Consider using `#to_discussion` if we do not need to render the discussion
+  # and all its notes and if we don't care about the discussion's resolvability status.
+  def discussion
+    full_discussion = self.noteable.notes.find_discussion(self.discussion_id) if part_of_discussion?
+    full_discussion || to_discussion
+  end
+
+  def part_of_discussion?
+    !to_discussion.individual_note?
+  end
+
+  def in_reply_to?(other)
+    case other
+    when Note
+      if part_of_discussion?
+        in_reply_to?(other.noteable) && in_reply_to?(other.to_discussion)
+      else
+        in_reply_to?(other.noteable)
+      end
+    when Discussion
+      self.discussion_id == other.id
+    when Noteable
+      self.noteable == other
+    else
+      false
+    end
+  end
+
   private
 
   def keep_around_commit
@@ -270,17 +315,7 @@ class Note < ActiveRecord::Base
   end
 
   def set_discussion_id
-    self.discussion_id = Digest::SHA1.hexdigest(build_discussion_id)
-  end
-
-  def build_discussion_id
-    if for_merge_request?
-      # Notes on merge requests are always in a discussion of their own,
-      # so we generate a unique discussion ID.
-      [:discussion, :note, SecureRandom.hex].join("-")
-    else
-      self.class.build_discussion_id(noteable_type, noteable_id || commit_id)
-    end
+    self.discussion_id ||= discussion_class.discussion_id(self)
   end
 
   def expire_etag_cache

app/models/out_of_context_discussion.rb

+# When notes on a commit are displayed in the context of a merge request that
+# contains that commit, they are displayed as if they were a discussion.
+#
+# This represents one of those discussions, consisting of `Note` notes.
+#
+# A discussion of this type is never resolvable.
+class OutOfContextDiscussion < Discussion
+  # Returns an array of discussion ID components
+  def self.build_discussion_id(note)
+    base_discussion_id(note)
+  end
+
+  # To make sure all out-of-context notes end up grouped as one discussion,
+  # we override the discussion ID to be a newly generated but consistent ID.
+  def self.override_discussion_id(note)
+    discussion_id(note)
+  end
+  
+  def self.note_class
+    Note
+  end
+end

app/models/project.rb

@@ -1498,7 +1498,7 @@ class Project < ActiveRecord::Base
   end
 
   def repository_and_lfs_size
-    statistics.storage_size + statistics.lfs_objects_size
+    statistics.total_repository_size
   end
 
   def above_size_limit?

app/models/project_services/mock_deployment_service.rb

@@ -28,6 +28,7 @@ class MockDeploymentService < DeploymentService
   private
 
   def rollout_status_instances
-    JSON.parse(Rails.root.join('spec', 'fixtures', 'rollout_status_instances.json'))
+    data = File.read(Rails.root.join('spec', 'fixtures', 'rollout_status_instances.json'))
+    JSON.parse(data)
   end
 end

app/models/project_services/mock_monitoring_service.rb

@@ -12,6 +12,7 @@ class MockMonitoringService < MonitoringService
   end
 
   def metrics(environment)
-    JSON.parse(Rails.root.join('spec', 'fixtures', 'metrics.json'))
+    data = File.read(Rails.root.join('spec', 'fixtures', 'metrics.json'))
+    JSON.parse(data)
   end
 end

app/models/sent_notification.rb

@@ -5,10 +5,11 @@ class SentNotification < ActiveRecord::Base
   belongs_to :noteable, polymorphic: true
   belongs_to :recipient, class_name: "User"
 
-  validates :project, :recipient, :reply_key, presence: true
-  validates :reply_key, uniqueness: true
+  validates :project, :recipient, presence: true
+  validates :reply_key, presence: true, uniqueness: true
   validates :noteable_id, presence: true, unless: :for_commit?
   validates :commit_id, presence: true, if: :for_commit?
+  validates :in_reply_to_discussion_id, format: { with: /\A\h{40}\z/, allow_nil: true }
   validate :note_valid
 
   after_save :keep_around_commit
@@ -22,9 +23,7 @@ class SentNotification < ActiveRecord::Base
       find_by(reply_key: reply_key)
     end
 
-    def record(noteable, recipient_id, reply_key, attrs = {})
-      return unless reply_key
-
+    def record(noteable, recipient_id, reply_key = self.reply_key, attrs = {})
       noteable_id = nil
       commit_id = nil
       if noteable.is_a?(Commit)
@@ -35,22 +34,19 @@ class SentNotification < ActiveRecord::Base
 
       attrs.reverse_merge!(
         project: noteable.project,
+        recipient_id: recipient_id,
+        reply_key: reply_key,
+
         noteable_type: noteable.class.name,
         noteable_id: noteable_id,
         commit_id: commit_id,
-        recipient_id:   recipient_id,
-        reply_key:      reply_key
       )
 
       create(attrs)
     end
 
-    def record_note(note, recipient_id, reply_key, attrs = {})
-      if note.diff_note?
-        attrs[:note_type] = note.type
-
-        attrs.merge!(note.diff_attributes)
-      end
+    def record_note(note, recipient_id, reply_key = self.reply_key, attrs = {})
+      attrs[:in_reply_to_discussion_id] = note.discussion_id
 
       record(note.noteable, recipient_id, reply_key, attrs)
     end
@@ -89,31 +85,45 @@ class SentNotification < ActiveRecord::Base
     self.reply_key
   end
 
-  def note_attributes
-    {
-      project:        self.project,
-      author:         self.recipient,
-      type:           self.note_type,
+  def create_reply(message, dryrun: false)
+    klass = dryrun ? Notes::BuildService : Notes::CreateService
+    klass.new(self.project, self.recipient, reply_params.merge(note: message)).execute
+  end
+
+  private
+
+  def reply_params
+    attrs = {
       noteable_type: self.noteable_type,
       noteable_id: self.noteable_id,
-      commit_id:      self.commit_id,
+      commit_id: self.commit_id
+    }
+
+    if self.in_reply_to_discussion_id.present?
+      attrs[:in_reply_to_discussion_id] = self.in_reply_to_discussion_id
+    else
+      # Remove in GitLab 10.0, when we will not support replying to SentNotifications
+      # that don't have `in_reply_to_discussion_id` anymore.
+      attrs.merge!(
+        type: self.note_type,
+
+        # LegacyDiffNote
         line_code: self.line_code,
+
+        # DiffNote
         position: self.position.to_json
-    }
+      )
     end
 
-  def create_note(note)
-    Notes::CreateService.new(
-      self.project,
-      self.recipient,
-      self.note_attributes.merge(note: note)
-    ).execute
+    attrs
   end
 
-  private
-
   def note_valid
-    Note.new(note_attributes.merge(note: "Test")).valid?
+    note = create_reply('Test', dryrun: true)
+
+    unless note.valid?
+      self.errors.add(:base, "Note parameters are invalid: #{note.errors.full_messages.to_sentence}")
+    end
   end
 
   def keep_around_commit

app/models/snippet.rb

@@ -2,6 +2,7 @@ class Snippet < ActiveRecord::Base
   include Gitlab::VisibilityLevel
   include Linguist::BlobHelper
   include CacheMarkdownField
+  include Noteable
   include Participable
   include Referable
   include Sortable

app/models/user.rb

@@ -581,10 +581,6 @@ class User < ActiveRecord::Base
     authorized_projects(Gitlab::Access::REPORTER).non_archived.with_issues_enabled
   end
 
-  def is_admin?
-    admin
-  end
-
   def require_ssh_key?
     keys.count == 0 && Gitlab::ProtocolAccess.allowed?('ssh')
   end
@@ -979,10 +975,6 @@ class User < ActiveRecord::Base
     end
   end
 
-  def record_activity
-    Gitlab::UserActivities::ActivitySet.record(self)
-  end
-
   def access_level
     if admin?
       :admin

app/policies/ci/runner_policy.rb

@@ -3,7 +3,7 @@ module Ci
     def rules
       return unless @user
 
-      can! :assign_runner if @user.is_admin?
+      can! :assign_runner if @user.admin?
 
       return if @subject.is_shared? || @subject.locked?
 

app/policies/project_policy.rb

@@ -74,7 +74,7 @@ class ProjectPolicy < BasePolicy
     can! :read_deployment
     can! :read_merge_request
 
-    if License.current&.add_on?('GitLab_DeployBoard')
+    if License.current&.add_on?('GitLab_DeployBoard') || Rails.env.development?
       can! :read_deploy_board
     end
   end

app/serializers/cohort_activity_month_entity.rb

+class CohortActivityMonthEntity < Grape::Entity
+  include ActionView::Helpers::NumberHelper
+
+  expose :total do |cohort_activity_month|
+    number_with_delimiter(cohort_activity_month[:total])
+  end
+
+  expose :percentage do |cohort_activity_month|
+    number_to_percentage(cohort_activity_month[:percentage], precision: 0)
+  end
+end

app/serializers/cohort_entity.rb

+class CohortEntity < Grape::Entity
+  include ActionView::Helpers::NumberHelper
+
+  expose :registration_month do |cohort|
+    cohort[:registration_month].strftime('%b %Y')
+  end
+
+  expose :total do |cohort|
+    number_with_delimiter(cohort[:total])
+  end
+
+  expose :inactive do |cohort|
+    number_with_delimiter(cohort[:inactive])
+  end
+
+  expose :activity_months, using: CohortActivityMonthEntity
+end

app/serializers/cohorts_entity.rb

+class CohortsEntity < Grape::Entity
+  expose :months_included
+  expose :cohorts, using: CohortEntity
+end

app/serializers/cohorts_serializer.rb

+class CohortsSerializer < AnalyticsGenericSerializer
+  entity CohortsEntity
+end

app/services/cohorts_service.rb

+class CohortsService
+  MONTHS_INCLUDED = 12
+
+  def execute
+    {
+      months_included: MONTHS_INCLUDED,
+      cohorts: cohorts
+    }
+  end
+
+  # Get an array of hashes that looks like:
+  #
+  #     [
+  #       {
+  #         registration_month: Date.new(2017, 3),
+  #         activity_months: [3, 2, 1],
+  #         total: 3
+  #         inactive: 0
+  #      },
+  #      etc.
+  #
+  # The `months` array is always from oldest to newest, so it's always
+  # non-strictly decreasing from left to right.
+  def cohorts
+    months = Array.new(MONTHS_INCLUDED) { |i| i.months.ago.beginning_of_month.to_date }
+
+    Array.new(MONTHS_INCLUDED) do
+      registration_month = months.last
+      activity_months = running_totals(months, registration_month)
+
+      # Even if no users registered in this month, we always want to have a
+      # value to fill in the table.
+      inactive = counts_by_month[[registration_month, nil]].to_i
+
+      months.pop
+
+      {
+        registration_month: registration_month,
+        activity_months: activity_months,
+        total: activity_months.first[:total],
+        inactive: inactive
+      }
+    end
+  end
+
+  private
+
+  # Calculate a running sum of active users, so users active in later months
+  # count as active in this month, too. Start with the most recent month first,
+  # for calculating the running totals, and then reverse for displaying in the
+  # table.
+  #
+  # Each month has a total, and a percentage of the overall total, as keys.
+  def running_totals(all_months, registration_month)
+    month_totals =
+      all_months
+        .map { |activity_month| counts_by_month[[registration_month, activity_month]] }
+        .reduce([]) { |result, total| result << result.last.to_i + total.to_i }
+        .reverse
+
+    overall_total = month_totals.first
+
+    month_totals.map do |total|
+      { total: total, percentage: total.zero? ? 0 : 100 * total / overall_total }
+    end
+  end
+
+  # Get a hash that looks like:
+  #
+  #     {
+  #       [created_at_month, last_activity_on_month] => count,
+  #       [created_at_month, last_activity_on_month_2] => count_2,
+  #       # etc.
+  #     }
+  #
+  # created_at_month can never be nil, but last_activity_on_month can (when a
+  # user has never logged in, just been created). This covers the last
+  # MONTHS_INCLUDED months.
+  def counts_by_month
+    @counts_by_month ||=
+      begin
+        created_at_month = column_to_date('created_at')
+        last_activity_on_month = column_to_date('last_activity_on')
+
+        User
+          .where('created_at > ?', MONTHS_INCLUDED.months.ago.end_of_month)
+          .group(created_at_month, last_activity_on_month)
+          .reorder("#{created_at_month} ASC", "#{last_activity_on_month} ASC")
+          .count
+      end
+  end
+
+  def column_to_date(column)
+    if Gitlab::Database.postgresql?
+      "CAST(DATE_TRUNC('month', #{column}) AS date)"
+    else
+      "STR_TO_DATE(DATE_FORMAT(#{column}, '%Y-%m-01'), '%Y-%m-%d')"
+    end
+  end
+end

app/services/concerns/issues/resolve_discussions.rb

@@ -21,11 +21,11 @@ module Issues
       @discussions_to_resolve ||=
         if discussion_to_resolve_id
           discussion_or_nil = merge_request_to_resolve_discussions_of
-                                .find_diff_discussion(discussion_to_resolve_id)
+                                .find_discussion(discussion_to_resolve_id)
           Array(discussion_or_nil)
         else
           merge_request_to_resolve_discussions_of
-            .resolvable_discussions
+            .discussions_to_be_resolved
         end
     end
   end

app/services/issues/build_service.rb

@@ -39,14 +39,19 @@ module Issues
     end
 
     def item_for_discussion(discussion)
-      first_note = discussion.first_note_to_resolve || discussion.first_note
+      first_note_to_resolve = discussion.first_note_to_resolve || discussion.first_note
+
+      is_very_first_note = first_note_to_resolve == discussion.first_note
+      action = is_very_first_note ? "started" : "commented on"
+
+      note_url = Gitlab::UrlBuilder.build(first_note_to_resolve)
+
       other_note_count = discussion.notes.size - 1
-      note_url = Gitlab::UrlBuilder.build(first_note)
 
-      discussion_info = "- [ ] #{first_note.author.to_reference} commented on a [discussion](#{note_url}): "
+      discussion_info = "- [ ] #{first_note_to_resolve.author.to_reference} #{action} a [discussion](#{note_url}): "
       discussion_info << " (+#{other_note_count} #{'comment'.pluralize(other_note_count)})" if other_note_count > 0
 
-      note_without_block_quotes = Banzai::Filter::BlockquoteFenceFilter.new(first_note.note).call
+      note_without_block_quotes = Banzai::Filter::BlockquoteFenceFilter.new(first_note_to_resolve.note).call
       spaces = ' ' * 4
       quote = note_without_block_quotes.lines.map { |line| "#{spaces}> #{line}" }.join
 

app/services/notes/build_service.rb

+module Notes
+  class BuildService < ::BaseService
+    def execute
+      in_reply_to_discussion_id = params.delete(:in_reply_to_discussion_id)
+
+      if project && in_reply_to_discussion_id.present?
+        discussion = project.notes.find_discussion(in_reply_to_discussion_id)
+
+        unless discussion
+          note = Note.new
+          note.errors.add(:base, 'Discussion to reply to cannot be found')
+          return note
+        end
+
+        params.merge!(discussion.reply_attributes)
+      end
+
+      note = Note.new(params)
+      note.project = project
+      note.author = current_user
+
+      note
+    end
+  end
+end

app/services/notes/create_service.rb

 module Notes
-  class CreateService < BaseService
+  class CreateService < ::BaseService
     def execute
       merge_request_diff_head_sha = params.delete(:merge_request_diff_head_sha)
 
-      note = Note.new(params)
-      note.project = project
-      note.author  = current_user
-      note.system  = false
+      note = Notes::BuildService.new(project, current_user, params).execute
+      return note unless note.valid?
 
       # We execute commands (extracted from `params[:note]`) on the noteable
       # **before** we save the note because if the note consists of commands

app/services/system_note_service.rb

@@ -228,12 +228,10 @@ module SystemNoteService
 
   def discussion_continued_in_issue(discussion, project, author, issue)
     body = "created #{issue.to_reference} to continue this discussion"
+    note_attributes = discussion.reply_attributes.merge(project: project, author: author, note: body)
 
-    note_params = discussion.reply_attributes.merge(project: project, author: author, note: body)
-    note_params[:type] = note_params.delete(:note_type)
-
-    note = Note.create(note_params.merge(system: true))
-    note.system_note_metadata = SystemNoteMetadata.new({ action: 'discussion' })
+    note = Note.create(note_attributes.merge(system: true))
+    note.system_note_metadata = SystemNoteMetadata.new(action: 'discussion')
 
     note
   end

app/services/users/activity_service.rb

@@ -14,7 +14,7 @@ module Users
     private
 
     def record_activity
-      @author.record_activity unless Gitlab::Geo.secondary?
+      Gitlab::UserActivities.record(@author.id) unless Gitlab::Geo.secondary?
 
       Rails.logger.debug("Recorded activity: #{@activity} for User ID: #{@author.id} (username: #{@author.username}")
     end

app/services/users/create_service.rb

@@ -11,7 +11,7 @@ module Users
 
       user = User.new(build_user_params)
 
-      if current_user&.is_admin?
+      if current_user&.admin?
         if params[:reset_password]
           @reset_token = user.generate_reset_token
           params[:force_random_password] = true
@@ -47,7 +47,7 @@ module Users
     private
 
     def can_create_user?
-      (current_user.nil? && current_application_settings.signup_enabled?) || current_user&.is_admin?
+      (current_user.nil? && current_application_settings.signup_enabled?) || current_user&.admin?
     end
 
     # Allowed params for creating a user (admins only)
@@ -94,7 +94,7 @@ module Users
     end
 
     def build_user_params
-      if current_user&.is_admin?
+      if current_user&.admin?
         user_params = params.slice(*admin_create_params)
         user_params[:created_by_id] = current_user&.id
 

app/views/admin/application_settings/_form.html.haml

@@ -556,7 +556,7 @@
           diagrams in Asciidoc documents using an external PlantUML service.
 
   %fieldset
-    %legend Usage statistics
+    %legend#usage-statistics Usage statistics
     .form-group
       .col-sm-offset-2.col-sm-10
         .checkbox

app/views/admin/cohorts/_cohorts_table.html.haml

+.bs-callout.clearfix
+  %p
+    User cohorts are shown for the last #{@cohorts[:months_included]}
+    months. Only users with activity are counted in the cohort total; inactive
+    users are counted separately.
+    = link_to icon('question-circle'), help_page_path('user/admin_area/user_cohorts', anchor: 'cohorts'), title: 'About this feature', target: '_blank'
+
+.table-holder
+  %table.table
+    %thead
+      %tr
+        %th Registration month
+        %th Inactive users
+        %th Cohort total
+        - @cohorts[:months_included].times do |i|
+          %th Month #{i}
+    %tbody
+      - @cohorts[:cohorts].each do |cohort|
+        %tr
+          %td= cohort[:registration_month]
+          %td= cohort[:inactive]
+          %td= cohort[:total]
+          - cohort[:activity_months].each do |activity_month|
+            %td
+              - next if cohort[:total] == '0'
+              = activity_month[:percentage]
+              %br
+              = activity_month[:total]

app/views/admin/cohorts/_usage_ping.html.haml

+%h2 Usage ping
+
+.bs-callout.clearfix
+  %p
+    User cohorts are shown because the usage ping is enabled. The data sent with
+    this is shown below. To disable this, visit
+    = succeed '.' do
+      = link_to 'application settings', admin_application_settings_path(anchor: 'usage-statistics')
+
+%pre.usage-data.js-syntax-highlight.code.highlight{ data: { endpoint: usage_data_admin_application_settings_path(format: :html, pretty: true) } }

app/views/admin/cohorts/index.html.haml

+- @no_container = true
+= render "admin/dashboard/head"
+
+%div{ class: container_class }
+  - if @cohorts
+    = render 'cohorts_table'
+    = render 'usage_ping'
+  - else
+    .bs-callout.bs-callout-warning.clearfix
+      %p
+        User cohorts are only shown when the
+        = link_to 'usage ping', help_page_path('user/admin_area/usage_statistics'), target: '_blank'
+        is enabled. To enable it and see user cohorts,
+        visit
+        = succeed '.' do
+          = link_to 'application settings', admin_application_settings_path(anchor: 'usage-statistics')

app/views/admin/dashboard/_head.html.haml

@@ -27,3 +27,7 @@
           = link_to admin_runners_path, title: 'Runners' do
             %span
               Runners
+        = nav_link path: 'cohorts#index' do
+          = link_to admin_cohorts_path, title: 'Cohorts' do
+            %span
+              Cohorts

app/views/discussions/_diff_discussion.html.haml

@@ -3,4 +3,4 @@
   %td.notes_line{ colspan: 2 }
   %td.notes_content
     .content{ class: ('hide' unless expanded) }
-      = render "discussions/notes", discussion: discussion
+      = render partial: "discussions/notes", collection: discussions, as: :discussion

app/views/discussions/_diff_with_notes.html.haml

@@ -7,7 +7,7 @@
 
   .diff-content.code.js-syntax-highlight
     %table
-      - discussions = { discussion.original_line_code => discussion }
+      - discussions = { discussion.original_line_code => [discussion] }
       = render partial: "projects/diffs/line",
         collection: discussion.truncated_diff_lines,
         as: :line,

app/views/discussions/_discussion.html.haml

@@ -5,7 +5,7 @@
       = link_to user_path(discussion.author) do
         = image_tag avatar_icon(discussion.author), class: "avatar s40"
     .timeline-content
-      .discussion.js-toggle-container{ class: discussion.id, data: { discussion_id: discussion.id } }
+      .discussion.js-toggle-container{ data: { discussion_id: discussion.id } }
         .discussion-header
           .discussion-actions
             %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button" }
@@ -18,19 +18,21 @@
 
           .inline.discussion-headline-light
             = discussion.author.to_reference
-            started a discussion on
+            started a discussion
 
-            - if discussion.for_commit?
+            - if discussion.for_commit? && @noteable != discussion.noteable
+              on
               - commit = discussion.noteable
               - if commit
                 commit
-                = link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code), class: 'monospace'
+                - anchor = discussion.line_code if discussion.diff_discussion?
+                = link_to commit.short_id, namespace_project_commit_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: anchor), class: 'monospace'
               - else
                 a deleted commit
-            - else
+            - elsif discussion.diff_discussion?
+              on
               - if discussion.active?
-                = link_to diffs_namespace_project_merge_request_path(discussion.project.namespace, discussion.project, discussion.noteable, anchor: discussion.line_code) do
-                  the diff
+                = link_to 'the diff', discussion_diff_path(discussion)
               - else
                 an outdated diff
 

app/views/discussions/_notes.html.haml

-%ul.notes{ data: { discussion_id: discussion.id } }
+.discussion-notes
+  %ul.notes{ data: { discussion_id: discussion.id } }
     = render partial: "projects/notes/note", collection: discussion.notes, as: :note
 
-- if current_user
+  - if current_user
     .discussion-reply-holder
-    - if discussion.diff_discussion?
+      - if discussion.potentially_resolvable?
         - line_type = local_assigns.fetch(:line_type, nil)
 
         .btn-group-justified.discussion-with-resolve-btn{ role: "group" }
           .btn-group{ role: "group" }
             = link_to_reply_discussion(discussion, line_type)
+
           = render "discussions/resolve_all", discussion: discussion
-        - if discussion.for_merge_request?
+
           .btn-group.discussion-actions
             = render "discussions/new_issue_for_discussion", discussion: discussion, merge_request: discussion.noteable
             = render "discussions/jump_to_next", discussion: discussion

app/views/discussions/_parallel_diff_discussion.html.haml

-- expanded = discussion_left.try(:expanded?) || discussion_right.try(:expanded?)
+- expanded = [*discussions_left, *discussions_right].any?(&:expanded?)
 %tr.notes_holder{ class: ('hide' unless expanded) }
-  - if discussion_left
+  - if discussions_left
     %td.notes_line.old
     %td.notes_content.parallel.old
-      .content{ class: ('hide' unless discussion_left.expanded?) }
-        = render "discussions/notes", discussion: discussion_left, line_type: 'old'
+      .content{ class: ('hide' unless discussions_left.any?(&:expanded?)) }
+        = render partial: "discussions/notes", collection: discussions_left, as: :discussion, line_type: 'old'
   - else
     %td.notes_line.old= ("")
     %td.notes_content.parallel.old
       .content
 
-  - if discussion_right
+  - if discussions_right
     %td.notes_line.new
     %td.notes_content.parallel.new
-      .content{ class: ('hide' unless discussion_right.expanded?) }
-        = render "discussions/notes", discussion: discussion_right, line_type: 'new'
+      .content{ class: ('hide' unless discussions_right.any?(&:expanded?)) }
+        = render partial: "discussions/notes", collection: discussions_right, as: :discussion, line_type: 'new'
   - else
     %td.notes_line.new= ("")
     %td.notes_content.parallel.new

app/views/discussions/_resolve_all.html.haml

-- if discussion.for_merge_request?
-  %resolve-discussion-btn{ ":discussion-id" => "'#{discussion.id}'",
+%resolve-discussion-btn{ ":discussion-id" => "'#{discussion.id}'",
     ":merge-request-id" => discussion.noteable.iid,
     ":can-resolve" => discussion.can_resolve?(current_user),
     "inline-template" => true }

app/views/layouts/header/_default.html.haml

@@ -31,7 +31,7 @@
               %li.impersonation
                 = link_to admin_impersonation_path, method: :delete, title: "Stop impersonation", aria: { label: 'Stop impersonation' }, data: { toggle: 'tooltip', placement: 'bottom', container: 'body' } do
                   = icon('user-secret fw')
-            - if current_user.is_admin?
+            - if current_user.admin?
               %li
                 = link_to admin_root_path, title: 'Admin area', aria: { label: "Admin area" }, data: {toggle: 'tooltip', placement: 'bottom', container: 'body'} do
                   = icon('wrench fw')

app/views/layouts/mailer.text.erb

+<%= yield -%>
+
+---
+You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>.

app/views/layouts/mailer.text.haml

-= yield
-
-You're receiving this email because of your account on #{Gitlab.config.gitlab.host}.
-Manage all notifications: #{profile_notifications_url}
-Help: #{help_url}

app/views/layouts/notify.html.haml

@@ -25,8 +25,8 @@
           - if @labels_url
             adjust your #{link_to 'label subscriptions', @labels_url}.
           - else
-            - if @sent_notification_url
-              = link_to "unsubscribe", @sent_notification_url
+            - if @unsubscribe_url
+              = link_to "unsubscribe", @unsubscribe_url
               from this thread or
             adjust your notification settings.
 

app/views/layouts/notify.text.erb

+<%= yield -%>
+
+---
+<%  if @target_url -%>
+<%    if @reply_by_email -%>
+<%=     "Reply to this email directly or view it on GitLab: #{@target_url}" -%>
+<%    else -%>
+<%=     "View it on GitLab: #{@target_url}" -%>
+<%    end -%>
+<%  end -%>
+
+You're receiving this email because of your account on <%= Gitlab.config.gitlab.host %>.

app/views/layouts/service_desk.html.haml

+%html{ lang: "en" }
+  %head
+    %meta{ content: "text/html; charset=utf-8", "http-equiv" => "Content-Type" }
+    %title
+      GitLab
+    = stylesheet_link_tag 'notify'
+    = yield :head
+  %body
+    .content
+      = yield
+    .footer{ style: "margin-top: 10px;" }
+    %p
+      —
+      %br
+      = link_to "Unsubscribe", @sent_notification_url