Merge branch 'live-trace-v2' into live-trace-v2-efficient-destroy-all

CHANGELOG.md

@@ -2,6 +2,14 @@
 documentation](doc/development/changelog.md) for instructions on adding your own
 entry.
 
+## 10.7.2 (2018-04-25)
+
+### Security (2 changes)
+
+- Serve archive requests with the correct file in all cases.
+- Sanitizes user name to avoid XSS attacks.
+
+
 ## 10.7.1 (2018-04-23)
 
 ### Fixed (11 changes)
@@ -237,6 +245,13 @@ entry.
 - Upgrade Gitaly to upgrade its charlock_holmes.
 
 
+## 10.6.5 (2018-04-24)
+
+### Security (1 change)
+
+- Sanitizes user name to avoid XSS attacks.
+
+
 ## 10.6.4 (2018-04-09)
 
 ### Fixed (8 changes, 1 of them is from the community)
@@ -478,6 +493,13 @@ entry.
 - Use host URL to build JIRA remote link icon.
 
 
+## 10.5.8 (2018-04-24)
+
+### Security (1 change)
+
+- Sanitizes user name to avoid XSS attacks.
+
+
 ## 10.5.7 (2018-04-03)
 
 ### Security (2 changes)

GITALY_SERVER_VERSION

-0.96.1
+0.96.2

GITLAB_WORKHORSE_VERSION

-4.1.0
+4.2.0

app/assets/javascripts/vue_shared/components/identicon.vue

@@ -17,7 +17,7 @@ export default {
   },
   computed: {
     /**
-     * This method is based on app/helpers/application_helper.rb#project_identicon
+     * This method is based on app/helpers/avatars_helper.rb#project_identicon
      */
     identiconStyles() {
       const allowedColors = [

app/controllers/omniauth_callbacks_controller.rb

@@ -8,8 +8,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     omniauth_flow(Gitlab::Auth::OAuth)
   end
 
-  Gitlab.config.omniauth.providers.each do |provider|
-    alias_method provider['name'], :handle_omniauth
+  AuthHelper.providers_for_base_controller.each do |provider|
+    alias_method provider, :handle_omniauth
   end
 
   # Extend the standard implementation to also increment

app/controllers/projects/notes_controller.rb

@@ -33,9 +33,7 @@ class Projects::NotesController < Projects::ApplicationController
   def resolve
     return render_404 unless note.resolvable?
 
-    note.resolve!(current_user)
-
-    MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
+    Notes::ResolveService.new(project, current_user).execute(note)
 
     discussion = note.discussion
 

app/finders/groups_finder.rb

@@ -39,7 +39,7 @@ class GroupsFinder < UnionFinder
 
   def all_groups
     return [owned_groups] if params[:owned]
-    return [Group.all] if current_user&.full_private_access?
+    return [Group.all] if current_user&.full_private_access? && all_available?
 
     groups = []
     groups << Gitlab::GroupHierarchy.new(groups_for_ancestors, groups_for_descendants).all_groups if current_user
@@ -67,6 +67,10 @@ class GroupsFinder < UnionFinder
   end
 
   def include_public_groups?
-    current_user.nil? || params.fetch(:all_available, true)
+    current_user.nil? || all_available?
+  end
+
+  def all_available?
+    params.fetch(:all_available, true)
   end
 end

app/helpers/application_helper.rb

@@ -32,80 +32,6 @@ module ApplicationHelper
     args.any? { |v| v.to_s.downcase == action_name }
   end
 
-  def project_icon(project_id, options = {})
-    project =
-      if project_id.respond_to?(:avatar_url)
-        project_id
-      else
-        Project.find_by_full_path(project_id)
-      end
-
-    if project.avatar_url
-      image_tag project.avatar_url, options
-    else # generated icon
-      project_identicon(project, options)
-    end
-  end
-
-  def project_identicon(project, options = {})
-    allowed_colors = {
-      red: 'FFEBEE',
-      purple: 'F3E5F5',
-      indigo: 'E8EAF6',
-      blue: 'E3F2FD',
-      teal: 'E0F2F1',
-      orange: 'FBE9E7',
-      gray: 'EEEEEE'
-    }
-
-    options[:class] ||= ''
-    options[:class] << ' identicon'
-    bg_key = project.id % 7
-    style = "background-color: ##{allowed_colors.values[bg_key]}; color: #555"
-
-    content_tag(:div, class: options[:class], style: style) do
-      project.name[0, 1].upcase
-    end
-  end
-
-  # Takes both user and email and returns the avatar_icon by
-  # user (preferred) or email.
-  def avatar_icon_for(user = nil, email = nil, size = nil, scale = 2, only_path: true)
-    if user
-      avatar_icon_for_user(user, size, scale, only_path: only_path)
-    elsif email
-      avatar_icon_for_email(email, size, scale, only_path: only_path)
-    else
-      default_avatar
-    end
-  end
-
-  def avatar_icon_for_email(email = nil, size = nil, scale = 2, only_path: true)
-    user = User.find_by_any_email(email.try(:downcase))
-    if user
-      avatar_icon_for_user(user, size, scale, only_path: only_path)
-    else
-      gravatar_icon(email, size, scale)
-    end
-  end
-
-  def avatar_icon_for_user(user = nil, size = nil, scale = 2, only_path: true)
-    if user
-      user.avatar_url(size: size, only_path: only_path) || default_avatar
-    else
-      gravatar_icon(nil, size, scale)
-    end
-  end
-
-  def gravatar_icon(user_email = '', size = nil, scale = 2)
-    GravatarService.new.execute(user_email, size, scale) ||
-      default_avatar
-  end
-
-  def default_avatar
-    asset_path('no_avatar.png')
-  end
-
   def last_commit(project)
     if project.repo_exists?
       time_ago_with_tooltip(project.repository.commit.committed_date)

app/helpers/auth_helper.rb

 module AuthHelper
   PROVIDERS_WITH_ICONS = %w(twitter github gitlab bitbucket google_oauth2 facebook azure_oauth2 authentiq).freeze
-  FORM_BASED_PROVIDERS = [/\Aldap/, 'crowd'].freeze
+  LDAP_PROVIDER = /\Aldap/
 
   def ldap_enabled?
     Gitlab::Auth::LDAP::Config.enabled?
@@ -23,7 +23,7 @@ module AuthHelper
   end
 
   def form_based_provider?(name)
-    FORM_BASED_PROVIDERS.any? { |pattern| pattern === name.to_s }
+    [LDAP_PROVIDER, 'crowd'].any? { |pattern| pattern === name.to_s }
   end
 
   def form_based_providers
@@ -38,6 +38,10 @@ module AuthHelper
     auth_providers.reject { |provider| form_based_provider?(provider) }
   end
 
+  def providers_for_base_controller
+    auth_providers.reject { |provider| LDAP_PROVIDER === provider }
+  end
+
   def enabled_button_based_providers
     disabled_providers = Gitlab::CurrentSettings.disabled_oauth_sign_in_sources || []
 

app/helpers/avatars_helper.rb

 module AvatarsHelper
+  def project_icon(project_id, options = {})
+    project =
+      if project_id.respond_to?(:avatar_url)
+        project_id
+      else
+        Project.find_by_full_path(project_id)
+      end
+
+    if project.avatar_url
+      image_tag project.avatar_url, options
+    else # generated icon
+      project_identicon(project, options)
+    end
+  end
+
+  def project_identicon(project, options = {})
+    allowed_colors = {
+      red: 'FFEBEE',
+      purple: 'F3E5F5',
+      indigo: 'E8EAF6',
+      blue: 'E3F2FD',
+      teal: 'E0F2F1',
+      orange: 'FBE9E7',
+      gray: 'EEEEEE'
+    }
+
+    options[:class] ||= ''
+    options[:class] << ' identicon'
+    bg_key = project.id % 7
+    style = "background-color: ##{allowed_colors.values[bg_key]}; color: #555"
+
+    content_tag(:div, class: options[:class], style: style) do
+      project.name[0, 1].upcase
+    end
+  end
+
+  # Takes both user and email and returns the avatar_icon by
+  # user (preferred) or email.
+  def avatar_icon_for(user = nil, email = nil, size = nil, scale = 2, only_path: true)
+    if user
+      avatar_icon_for_user(user, size, scale, only_path: only_path)
+    elsif email
+      avatar_icon_for_email(email, size, scale, only_path: only_path)
+    else
+      default_avatar
+    end
+  end
+
+  def avatar_icon_for_email(email = nil, size = nil, scale = 2, only_path: true)
+    user = User.find_by_any_email(email.try(:downcase))
+    if user
+      avatar_icon_for_user(user, size, scale, only_path: only_path)
+    else
+      gravatar_icon(email, size, scale)
+    end
+  end
+
+  def avatar_icon_for_user(user = nil, size = nil, scale = 2, only_path: true)
+    if user
+      user.avatar_url(size: size, only_path: only_path) || default_avatar
+    else
+      gravatar_icon(nil, size, scale)
+    end
+  end
+
+  def gravatar_icon(user_email = '', size = nil, scale = 2)
+    GravatarService.new.execute(user_email, size, scale) ||
+      default_avatar
+  end
+
+  def default_avatar
+    ActionController::Base.helpers.image_path('no_avatar.png')
+  end
+
   def author_avatar(commit_or_event, options = {})
     user_avatar(options.merge({
       user: commit_or_event.author,

app/helpers/system_note_helper.rb

 module SystemNoteHelper
   ICON_NAMES_BY_ACTION = {
     'commit' => 'commit',
-    'description' => 'pencil',
+    'description' => 'pencil-square',
     'merge' => 'git-merge',
     'merged' => 'git-merge',
     'opened' => 'issue-open',
     'closed' => 'issue-close',
     'time_tracking' => 'timer',
     'assignee' => 'user',
-    'title' => 'pencil',
+    'title' => 'pencil-square',
     'task' => 'task-done',
     'label' => 'label',
     'cross_reference' => 'comment-dots',
@@ -18,7 +18,7 @@ module SystemNoteHelper
     'milestone' => 'clock',
     'discussion' => 'comment',
     'moved' => 'arrow-right',
-    'outdated' => 'pencil',
+    'outdated' => 'pencil-square',
     'duplicate' => 'issue-duplicate',
     'locked' => 'lock',
     'unlocked' => 'lock-open'

app/mailers/notify.rb

@@ -16,6 +16,7 @@ class Notify < BaseMailer
   helper BlobHelper
   helper EmailsHelper
   helper MembersHelper
+  helper AvatarsHelper
   helper GitlabRoutingHelper
 
   def test_email(recipient_email, subject, body)

app/models/commit.rb

@@ -105,6 +105,10 @@ class Commit
         end
       end
     end
+
+    def parent_class
+      ::Project
+    end
   end
 
   attr_accessor :raw

app/models/diff_note.rb

@@ -54,7 +54,20 @@ class DiffNote < Note
   end
 
   def diff_file
-    @diff_file ||= self.original_position.diff_file(self.project.repository)
+    @diff_file ||=
+      begin
+        if created_at_diff?(noteable.diff_refs)
+          # We're able to use the already persisted diffs (Postgres) if we're
+          # presenting a "current version" of the MR discussion diff.
+          # So no need to make an extra Gitaly diff request for it.
+          # As an extra benefit, the returned `diff_file` already
+          # has `highlighted_diff_lines` data set from Redis on
+          # `Diff::FileCollection::MergeRequestDiff`.
+          noteable.diffs(paths: original_position.paths, expanded: true).diff_files.first
+        else
+          original_position.diff_file(self.project.repository)
+        end
+      end
   end
 
   def diff_line

app/services/notes/resolve_service.rb

+module Notes
+  class ResolveService < ::BaseService
+    def execute(note)
+      note.resolve!(current_user)
+
+      ::MergeRequests::ResolvedDiscussionNotificationService.new(project, current_user).execute(note.noteable)
+    end
+  end
+end

changelogs/unreleased/dm-commit-trailer-without-gravatar.yml

+---
+title: Fix commit trailer rendering when Gravatar is disabled
+merge_request:
+author:
+type: fixed

changelogs/unreleased/feature-show-only-groups-user-is-member-of-in-dashboard.yml

+---
+title: For group dashboard, we no longer show groups which the visitor is not a member of (this applies to admins and auditors)
+merge_request: 17884
+author: Roger Rüttimann
+type: changed

changelogs/unreleased/helm-add-alpine-mirrors.yml

+---
+title: Increase cluster applications installer availability using alpine linux mirrors
+merge_request:
+author:
+type: performance

changelogs/unreleased/jprovazn-commit-notes-api.yml

+---
+title: Add discussion API for merge requests and commits
+merge_request:
+author:
+type: added

changelogs/unreleased/osw-use-cached-highlighted-content-for-discussions.yml

+---
+title: Use persisted diff data instead fetching Git on discussions
+merge_request:
+author:
+type: performance

changelogs/unreleased/update-timeline-icon-for-description-edit.yml

+---
+title: Update timeline icon for description edit
+merge_request: 18633
+author: George Tsiolis
+type: changed

config/karma.config.js

-var path = require('path');
-var webpack = require('webpack');
-var argumentsParser = require('commander');
-var webpackConfig = require('./webpack.config.js');
-var ROOT_PATH = path.resolve(__dirname, '..');
+const path = require('path');
+const glob = require('glob');
+const chalk = require('chalk');
+const webpack = require('webpack');
+const argumentsParser = require('commander');
+const webpackConfig = require('./webpack.config.js');
+
+const ROOT_PATH = path.resolve(__dirname, '..');
+
+function fatalError(message) {
+  console.error(chalk.red(`\nError: ${message}\n`));
+  process.exit(1);
+}
 
 // remove problematic plugins
 if (webpackConfig.plugins) {
@@ -15,33 +23,70 @@ if (webpackConfig.plugins) {
   });
 }
 
-var testFiles = argumentsParser
+const specFilters = argumentsParser
   .option(
     '-f, --filter-spec [filter]',
     'Filter run spec files by path. Multiple filters are like a logical OR.',
-    (val, memo) => {
-      memo.push(val);
+    (filter, memo) => {
+      memo.push(filter, filter.replace(/\/?$/, '/**/*.js'));
       return memo;
     },
     []
   )
   .parse(process.argv).filterSpec;
 
-webpackConfig.plugins.push(
-  new webpack.DefinePlugin({
-    'process.env.TEST_FILES': JSON.stringify(testFiles),
-  })
-);
+if (specFilters.length) {
+  const specsPath = /^(?:\.[\\\/])?spec[\\\/]javascripts[\\\/]/;
+
+  // resolve filters
+  let filteredSpecFiles = specFilters.map(filter =>
+    glob
+      .sync(filter, {
+        root: ROOT_PATH,
+        matchBase: true,
+      })
+      .filter(path => path.endsWith('spec.js'))
+  );
+
+  // flatten
+  filteredSpecFiles = Array.prototype.concat.apply([], filteredSpecFiles);
+
+  // remove duplicates
+  filteredSpecFiles = [...new Set(filteredSpecFiles)];
+
+  if (filteredSpecFiles.length < 1) {
+    fatalError('Your filter did not match any test files.');
+  }
+
+  if (!filteredSpecFiles.every(file => specsPath.test(file))) {
+    fatalError('Test files must be located within /spec/javascripts.');
+  }
+
+  const newContext = filteredSpecFiles.reduce((context, file) => {
+    const relativePath = file.replace(specsPath, '');
+    context[file] = `./${relativePath}`;
+    return context;
+  }, {});
+
+  webpackConfig.plugins.push(
+    new webpack.ContextReplacementPlugin(
+      /spec[\\\/]javascripts$/,
+      path.join(ROOT_PATH, 'spec/javascripts'),
+      newContext
+    )
+  );
+}
 
-webpackConfig.devtool = process.env.BABEL_ENV !== 'coverage' && 'cheap-inline-source-map';
+webpackConfig.entry = undefined;
+webpackConfig.devtool = 'cheap-inline-source-map';
 
 // Karma configuration
 module.exports = function(config) {
   process.env.TZ = 'Etc/UTC';
 
-  var progressReporter = process.env.CI ? 'mocha' : 'progress';
+  const progressReporter = process.env.CI ? 'mocha' : 'progress';
 
-  var karmaConfig = {
+  const karmaConfig = {
     basePath: ROOT_PATH,
     browsers: ['ChromeHeadlessCustom'],
     customLaunchers: {

config/webpack.config.js

@@ -69,6 +69,9 @@ const config = {
         test: /\.js$/,
         exclude: /(node_modules|vendor\/assets)/,
         loader: 'babel-loader',
+        options: {
+          cacheDirectory: path.join(ROOT_PATH, 'tmp/cache/babel-loader'),
+        },
       },
       {
         test: /\.vue$/,

doc/api/groups.md

@@ -10,7 +10,7 @@ Parameters:
 | Attribute | Type | Required | Description |
 | --------- | ---- | -------- | ----------- |
 | `skip_groups` | array of integers | no | Skip the group IDs passed |
-| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users) |
+| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) |
 | `search` | string | no | Return the list of authorized groups matching the search criteria |
 | `order_by` | string | no | Order groups by `name` or `path`. Default is `name` |
 | `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |
@@ -94,7 +94,7 @@ Parameters:
 | --------- | ---- | -------- | ----------- |
 | `id` | integer/string | yes | The ID or [URL-encoded path of the group](README.md#namespaced-path-encoding) of the parent group |
 | `skip_groups` | array of integers | no | Skip the group IDs passed |
-| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users) |
+| `all_available` | boolean | no | Show all the groups you have access to (defaults to `false` for authenticated users, `true` for admin) |
 | `search` | string | no | Return the list of authorized groups matching the search criteria |
 | `order_by` | string | no | Order groups by `name` or `path`. Default is `name` |
 | `sort` | string | no | Order groups in `asc` or `desc` order. Default is `asc` |

doc/api/notes.md

@@ -39,7 +39,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at
     "system": true,
     "noteable_id": 377,
     "noteable_type": "Issue",
-    "noteable_iid": 377
+    "noteable_iid": 377,
+    "resolvable": false
   },
   {
     "id": 305,
@@ -58,7 +59,8 @@ GET /projects/:id/issues/:issue_iid/notes?sort=asc&order_by=updated_at
     "system": true,
     "noteable_id": 121,
     "noteable_type": "Issue",
-    "noteable_iid": 121
+    "noteable_iid": 121,
+    "resolvable": false
   }
 ]
 ```
@@ -314,7 +316,8 @@ Parameters:
   "system": false,
   "noteable_id": 2,
   "noteable_type": "MergeRequest",
-  "noteable_iid": 2
+  "noteable_iid": 2,
+  "resolvable": false
 }
 ```
 

doc/development/testing_guide/frontend_testing.md

@@ -62,6 +62,7 @@ describe('.methodName', () => {
   });
 });
 ```
+
 #### Testing promises
 
 When testing Promises you should always make sure that the test is asynchronous and rejections are handled.
@@ -69,9 +70,9 @@ Your Promise chain should therefore end with a call of the `done` callback and `
 
 ```javascript
 // Good
-it('tests a promise', (done) => {
+it('tests a promise', done => {
   promise
-    .then((data) => {
+    .then(data => {
       expect(data).toBe(asExpected);
     })
     .then(done)
@@ -79,10 +80,10 @@ it('tests a promise', (done) => {
 });
 
 // Good
-it('tests a promise rejection', (done) => {
+it('tests a promise rejection', done => {
   promise
     .then(done.fail)
-    .catch((error) => {
+    .catch(error => {
       expect(error).toBe(expectedError);
     })
     .then(done)
@@ -91,38 +92,37 @@ it('tests a promise rejection', (done) => {
 
 // Bad (missing done callback)
 it('tests a promise', () => {
-  promise
-    .then((data) => {
-      expect(data).toBe(asExpected);
-    })
+  promise.then(data => {
+    expect(data).toBe(asExpected);
+  });
 });
 
 // Bad (missing catch)
-it('tests a promise', (done) => {
+it('tests a promise', done => {
   promise
-    .then((data) => {
+    .then(data => {
       expect(data).toBe(asExpected);
     })
-    .then(done)
+    .then(done);
 });
 
 // Bad (use done.fail in asynchronous tests)
-it('tests a promise', (done) => {
+it('tests a promise', done => {
   promise
-    .then((data) => {
+    .then(data => {
       expect(data).toBe(asExpected);
     })
     .then(done)
-    .catch(fail)
+    .catch(fail);
 });
 
 // Bad (missing catch)
-it('tests a promise rejection', (done) => {
+it('tests a promise rejection', done => {
   promise
-    .catch((error) => {
+    .catch(error => {
       expect(error).toBe(expectedError);
     })
-    .then(done)
+    .then(done);
 });
 ```
 
@@ -139,7 +139,7 @@ documentation for these methods can be found in the [jasmine introduction page](
 Sometimes you may need to spy on a method that is directly imported by another
 module. GitLab has a custom `spyOnDependency` method which utilizes
 [babel-plugin-rewire](https://github.com/speedskater/babel-plugin-rewire) to
-achieve this.  It can be used like so:
+achieve this. It can be used like so:
 
 ```javascript
 // my_module.js
@@ -181,8 +181,8 @@ See this [section][vue-test].
 `rake karma` runs the frontend-only (JavaScript) tests.
 It consists of two subtasks:
 
-- `rake karma:fixtures` (re-)generates fixtures
-- `rake karma:tests` actually executes the tests
+* `rake karma:fixtures` (re-)generates fixtures
+* `rake karma:tests` actually executes the tests
 
 As long as the fixtures don't change, `rake karma:tests` (or `yarn karma`)
 is sufficient (and saves you some time).
@@ -217,6 +217,14 @@ yarn karma-start --filter-spec profile/account/components/
 yarn karma-start -f vue_shared -f vue_mr_widget
 ```
 
+You can also use glob syntax to match files. Remember to put quotes around the
+glob otherwise your shell may split it into multiple arguments:
+
+```bash
+# Run all specs named `file_spec` within the IDE subdirectory
+yarn karma -f 'spec/javascripts/ide/**/file_spec.js'
+```
+
 ## RSpec feature integration tests
 
 Information on setting up and running RSpec integration tests with
@@ -231,14 +239,14 @@ supported by the PhantomJS test runner which is used for both Karma and RSpec
 tests. We polyfill some JavaScript objects for older browsers, but some
 features are still unavailable:
 
-- Array.from
-- Array.first
-- Async functions
-- Generators
-- Array destructuring
-- For..Of
-- Symbol/Symbol.iterator
-- Spread
+* Array.from
+* Array.first
+* Async functions
+* Generators
+* Array destructuring
+* For..Of
+* Symbol/Symbol.iterator
+* Spread
 
 Until these are polyfilled appropriately, they should not be used. Please
 update this list with additional unsupported features.
@@ -295,11 +303,11 @@ Scenario: Developer can approve merge request
 [jasmine-focus]: https://jasmine.github.io/2.5/focused_specs.html
 [jasmine-jquery]: https://github.com/velesin/jasmine-jquery
 [karma]: http://karma-runner.github.io/
-[vue-test]:https://docs.gitlab.com/ce/development/fe_guide/vue.html#testing-vue-components
-[RSpec]: https://github.com/rspec/rspec-rails#feature-specs
-[Capybara]: https://github.com/teamcapybara/capybara
-[Karma]: http://karma-runner.github.io/
-[Jasmine]: https://jasmine.github.io/
+[vue-test]: https://docs.gitlab.com/ce/development/fe_guide/vue.html#testing-vue-components
+[rspec]: https://github.com/rspec/rspec-rails#feature-specs
+[capybara]: https://github.com/teamcapybara/capybara
+[karma]: http://karma-runner.github.io/
+[jasmine]: https://jasmine.github.io/
 
 ---
 

lib/api/discussions.rb

@@ -5,11 +5,12 @@ module API
 
     before { authenticate! }
 
-    NOTEABLE_TYPES = [Issue, Snippet].freeze
+    NOTEABLE_TYPES = [Issue, Snippet, MergeRequest, Commit].freeze
 
     NOTEABLE_TYPES.each do |noteable_type|
       parent_type = noteable_type.parent_class.to_s.underscore
       noteables_str = noteable_type.to_s.underscore.pluralize
+      noteables_path = noteable_type == Commit ? "repository/#{noteables_str}" : noteables_str
 
       params do
         requires :id, type: String, desc: "The ID of a #{parent_type}"
@@ -19,14 +20,12 @@ module API
           success Entities::Discussion
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           use :pagination
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions" do
+        get ":id/#{noteables_path}/:noteable_id/discussions" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          break not_found!("Discussions") unless can?(current_user, noteable_read_ability_name(noteable), noteable)
-
           notes = noteable.notes
             .inc_relations_for_view
             .includes(:noteable)
@@ -43,13 +42,13 @@ module API
         end
         params do
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id" do
+        get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
-          if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
+          if notes.empty?
             break not_found!("Discussion")
           end
 
@@ -62,19 +61,36 @@ module API
           success Entities::Discussion
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :body, type: String, desc: 'The content of a note'
           optional :created_at, type: String, desc: 'The creation date of the note'
+          optional :position, type: Hash do
+            requires :base_sha, type: String, desc: 'Base commit SHA in the source branch'
+            requires :start_sha, type: String, desc: 'SHA referencing commit in target branch'
+            requires :head_sha, type: String, desc: 'SHA referencing HEAD of this merge request'
+            requires :position_type, type: String, desc: 'Type of the position reference', values: %w(text image)
+            optional :new_path, type: String, desc: 'File path after change'
+            optional :new_line, type: Integer, desc: 'Line number after change'
+            optional :old_path, type: String, desc: 'File path before change'
+            optional :old_line, type: Integer, desc: 'Line number before change'
+            optional :width, type: Integer, desc: 'Width of the image'
+            optional :height, type: Integer, desc: 'Height of the image'
+            optional :x, type: Integer, desc: 'X coordinate in the image'
+            optional :y, type: Integer, desc: 'Y coordinate in the image'
+          end
         end
-        post ":id/#{noteables_str}/:noteable_id/discussions" do
+        post ":id/#{noteables_path}/:noteable_id/discussions" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
+          type = params[:position] ? 'DiffNote' : 'DiscussionNote'
+          id_key = noteable.is_a?(Commit) ? :commit_id : :noteable_id
 
           opts = {
             note: params[:body],
             created_at: params[:created_at],
-            type: 'DiscussionNote',
+            type: type,
             noteable_type: noteables_str.classify,
-            noteable_id: noteable.id
+            position: params[:position],
+            id_key => noteable.id
           }
 
           note = create_note(noteable, opts)
@@ -91,13 +107,13 @@ module API
         end
         params do
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes" do
+        get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
-          if notes.empty? || !can?(current_user, noteable_read_ability_name(noteable), noteable)
+          if notes.empty?
             break not_found!("Notes")
           end
 
@@ -108,12 +124,12 @@ module API
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :body, type: String, desc: 'The content of a note'
           optional :created_at, type: String, desc: 'The creation date of the note'
         end
-        post ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes" do
+        post ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
           notes = readable_discussion_notes(noteable, params[:discussion_id])
 
@@ -139,11 +155,11 @@ module API
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :note_id, type: Integer, desc: 'The ID of a note'
         end
-        get ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+        get ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
           get_note(noteable, params[:note_id])
@@ -153,30 +169,52 @@ module API
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :note_id, type: Integer, desc: 'The ID of a note'
-          requires :body, type: String, desc: 'The content of a note'
+          optional :body, type: String, desc: 'The content of a note'
+          optional :resolved, type: Boolean, desc: 'Mark note resolved/unresolved'
+          exactly_one_of :body, :resolved
         end
-        put ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+        put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          update_note(noteable, params[:note_id])
+          if params[:resolved].nil?
+            update_note(noteable, params[:note_id])
+          else
+            resolve_note(noteable, params[:note_id], params[:resolved])
+          end
         end
 
         desc "Delete a comment in a #{noteable_type.to_s.downcase} discussion" do
           success Entities::Note
         end
         params do
-          requires :noteable_id, type: Integer, desc: 'The ID of the noteable'
+          requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
           requires :discussion_id, type: String, desc: 'The ID of a discussion'
           requires :note_id, type: Integer, desc: 'The ID of a note'
         end
-        delete ":id/#{noteables_str}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+        delete ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
           delete_note(noteable, params[:note_id])
         end
+
+        if Noteable::RESOLVABLE_TYPES.include?(noteable_type.to_s)
+          desc "Resolve/unresolve an existing #{noteable_type.to_s.downcase} discussion" do
+            success Entities::Discussion
+          end
+          params do
+            requires :noteable_id, types: [Integer, String], desc: 'The ID of the noteable'
+            requires :discussion_id, type: String, desc: 'The ID of a discussion'
+            requires :resolved, type: Boolean, desc: 'Mark discussion resolved/unresolved'
+          end
+          put ":id/#{noteables_path}/:noteable_id/discussions/:discussion_id" do
+            noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
+
+            resolve_discussion(noteable, params[:discussion_id], params[:resolved])
+          end
+        end
       end
     end
 

lib/api/entities.rb

@@ -286,6 +286,10 @@ module API
       end
     end
 
+    class DiffRefs < Grape::Entity
+      expose :base_sha, :head_sha, :start_sha
+    end
+
     class Commit < Grape::Entity
       expose :id, :short_id, :title, :created_at
       expose :parent_ids
@@ -601,6 +605,8 @@ module API
         merge_request.metrics&.pipeline
       end
 
+      expose :diff_refs, using: Entities::DiffRefs
+
       def build_available?(options)
         options[:project]&.feature_available?(:builds, options[:current_user])
       end
@@ -642,6 +648,11 @@ module API
       expose :id, :key, :created_at
     end
 
+    class DiffPosition < Grape::Entity
+      expose :base_sha, :start_sha, :head_sha, :old_path, :new_path,
+        :position_type
+    end
+
     class Note < Grape::Entity
       # Only Issue and MergeRequest have iid
       NOTEABLE_TYPES_WITH_IID = %w(Issue MergeRequest).freeze
@@ -655,6 +666,14 @@ module API
       expose :system?, as: :system
       expose :noteable_id, :noteable_type
 
+      expose :position, if: ->(note, options) { note.diff_note? } do |note|
+        note.position.to_h
+      end
+
+      expose :resolvable?, as: :resolvable
+      expose :resolved?, as: :resolved, if: ->(note, options) { note.resolvable? }
+      expose :resolved_by, using: Entities::UserBasic, if: ->(note, options) { note.resolvable? }
+
       # Avoid N+1 queries as much as possible
       expose(:noteable_iid) { |note| note.noteable.iid if NOTEABLE_TYPES_WITH_IID.include?(note.noteable_type) }
     end

lib/api/groups.rb

@@ -37,13 +37,11 @@ module API
         use :pagination
       end
 
-      def find_groups(params)
-        find_params = {
-          all_available: params[:all_available],
-          custom_attributes: params[:custom_attributes],
-          owned: params[:owned]
-        }
-        find_params[:parent] = find_group!(params[:id]) if params[:id]
+      def find_groups(params, parent_id = nil)
+        find_params = params.slice(:all_available, :custom_attributes, :owned)
+        find_params[:parent] = find_group!(parent_id) if parent_id
+        find_params[:all_available] =
+          find_params.fetch(:all_available, current_user&.full_private_access?)
 
         groups = GroupsFinder.new(current_user, find_params).execute
         groups = groups.search(params[:search]) if params[:search].present?
@@ -85,7 +83,7 @@ module API
         use :with_custom_attributes
       end
       get do
-        groups = find_groups(params)
+        groups = find_groups(declared_params(include_missing: false), params[:id])
         present_groups params, groups
       end
 
@@ -213,7 +211,7 @@ module API
         use :with_custom_attributes
       end
       get ":id/subgroups" do
-        groups = find_groups(params)
+        groups = find_groups(declared_params(include_missing: false), params[:id])
         present_groups params, groups
       end
 

lib/api/helpers.rb

@@ -171,6 +171,10 @@ module API
       MergeRequestsFinder.new(current_user, project_id: user_project.id).find_by!(iid: iid)
     end
 
+    def find_project_commit(id)
+      user_project.commit_by(oid: id)
+    end
+
     def find_project_snippet(id)
       finder_params = { project: user_project }
       SnippetsFinder.new(current_user, finder_params).find(id)

lib/api/helpers/custom_attributes.rb

@@ -7,6 +7,9 @@ module API
         helpers do
           params :with_custom_attributes do
             optional :with_custom_attributes, type: Boolean, default: false, desc: 'Include custom attributes in the response'
+
+            optional :custom_attributes, type: Hash,
+                                         desc: 'Filter with custom attributes'
           end
 
           def with_custom_attributes(collection_or_resource, options = {})

lib/api/helpers/notes_helpers.rb

@@ -21,6 +21,23 @@ module API
         end
       end
 
+      def resolve_note(noteable, note_id, resolved)
+        note = noteable.notes.find(note_id)
+
+        authorize! :resolve_note, note
+
+        bad_request!("Note is not resolvable") unless note.resolvable?
+
+        if resolved
+          parent = noteable_parent(noteable)
+          ::Notes::ResolveService.new(parent, current_user).execute(note)
+        else
+          note.unresolve!
+        end
+
+        present note, with: Entities::Note
+      end
+
       def delete_note(noteable, note_id)
         note = noteable.notes.find(note_id)
 
@@ -35,7 +52,7 @@ module API
 
       def get_note(noteable, note_id)
         note = noteable.notes.with_metadata.find(params[:note_id])
-        can_read_note = can?(current_user, noteable_read_ability_name(noteable), noteable) && !note.cross_reference_not_visible_for?(current_user)
+        can_read_note = !note.cross_reference_not_visible_for?(current_user)
 
         if can_read_note
           present note, with: Entities::Note
@@ -49,7 +66,20 @@ module API
       end
 
       def find_noteable(parent, noteables_str, noteable_id)
-        public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
+        noteable = public_send("find_#{parent}_#{noteables_str.singularize}", noteable_id) # rubocop:disable GitlabSecurity/PublicSend
+
+        readable =
+          if noteable.is_a?(Commit)
+            # for commits there is not :read_commit policy, check if user
+            # has :read_note permission on the commit's project
+            can?(current_user, :read_note, user_project)
+          else
+            can?(current_user, noteable_read_ability_name(noteable), noteable)
+          end
+
+        return not_found!(noteables_str) unless readable
+
+        noteable
       end
 
       def noteable_parent(noteable)
@@ -57,11 +87,8 @@ module API
       end
 
       def create_note(noteable, opts)
-        noteables_str = noteable.model_name.to_s.underscore.pluralize
-
-        return not_found!(noteables_str) unless can?(current_user, noteable_read_ability_name(noteable), noteable)
-
-        authorize! :create_note, noteable
+        policy_object = noteable.is_a?(Commit) ? user_project : noteable
+        authorize!(:create_note, policy_object)
 
         parent = noteable_parent(noteable)
 
@@ -73,6 +100,21 @@ module API
         project = parent if parent.is_a?(Project)
         ::Notes::CreateService.new(project, current_user, opts).execute
       end
+
+      def resolve_discussion(noteable, discussion_id, resolved)
+        discussion = noteable.find_discussion(discussion_id)
+
+        forbidden! unless discussion.can_resolve?(current_user)
+
+        if resolved
+          parent = noteable_parent(noteable)
+          ::Discussions::ResolveService.new(parent, current_user, merge_request: noteable).execute(discussion)
+        else
+          discussion.unresolve!
+        end
+
+        present discussion, with: Entities::Discussion
+      end
     end
   end
 end

lib/api/notes.rb

@@ -31,23 +31,19 @@ module API
         get ":id/#{noteables_str}/:noteable_id/notes" do
           noteable = find_noteable(parent_type, noteables_str, params[:noteable_id])
 
-          if can?(current_user, noteable_read_ability_name(noteable), noteable)
-            # We exclude notes that are cross-references and that cannot be viewed
-            # by the current user. By doing this exclusion at this level and not
-            # at the DB query level (which we cannot in that case), the current
-            # page can have less elements than :per_page even if
-            # there's more than one page.
-            raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
-            notes =
-              # paginate() only works with a relation. This could lead to a
-              # mismatch between the pagination headers info and the actual notes
-              # array returned, but this is really a edge-case.
-              paginate(raw_notes)
-              .reject { |n| n.cross_reference_not_visible_for?(current_user) }
-            present notes, with: Entities::Note
-          else
-            not_found!("Notes")
-          end
+          # We exclude notes that are cross-references and that cannot be viewed
+          # by the current user. By doing this exclusion at this level and not
+          # at the DB query level (which we cannot in that case), the current
+          # page can have less elements than :per_page even if
+          # there's more than one page.
+          raw_notes = noteable.notes.with_metadata.reorder(params[:order_by] => params[:sort])
+          notes =
+            # paginate() only works with a relation. This could lead to a
+            # mismatch between the pagination headers info and the actual notes
+            # array returned, but this is really a edge-case.
+            paginate(raw_notes)
+            .reject { |n| n.cross_reference_not_visible_for?(current_user) }
+          present notes, with: Entities::Note
         end
 
         desc "Get a single #{noteable_type.to_s.downcase} note" do

lib/banzai/filter/commit_trailers_filter.rb

@@ -13,7 +13,6 @@ module Banzai
     # * https://git.wiki.kernel.org/index.php/CommitMessageConventions
     class CommitTrailersFilter < HTML::Pipeline::Filter
       include ActionView::Helpers::TagHelper
-      include ApplicationHelper
       include AvatarsHelper
 
       TRAILER_REGEXP = /(?<label>[[:alpha:]-]+-by:)/i.freeze

lib/gitlab/ci/trace/chunked_io.rb

@@ -134,7 +134,8 @@ module Gitlab
         end
 
         def truncate(offset)
-          return unless offset < size && offset >= 0
+          raise ArgumentError, 'Outside of file' if offset > size || offset < 0
+          return if offset == size # Skip the following process as it doesn't affect anything
 
           @tell = offset
           @size = offset

lib/gitlab/diff/file_collection/base.rb

@@ -36,6 +36,8 @@ module Gitlab
         private
 
         def decorate_diff!(diff)
+          return diff if diff.is_a?(File)
+
           Gitlab::Diff::File.new(diff, repository: project.repository, diff_refs: diff_refs, fallback_diff_refs: fallback_diff_refs)
         end
       end

lib/gitlab/diff/position.rb

@@ -12,6 +12,10 @@ module Gitlab
                :head_sha,
                :old_line,
                :new_line,
+               :width,
+               :height,
+               :x,
+               :y,
                :position_type, to: :formatter
 
       # A position can belong to a text line or to an image coordinate

lib/gitlab/git/raw_diff_change.rb

@@ -38,7 +38,9 @@ module Gitlab
       end
 
       def extract_operation
-        case @raw_operation&.first(1)
+        return :unknown unless @raw_operation
+
+        case @raw_operation[0]
         when 'A'
           :added
         when 'C'

lib/gitlab/kubernetes/helm/base_command.rb

@@ -15,6 +15,9 @@ module Gitlab
         def generate_script
           <<~HEREDOC
             set -eo pipefail
+            ALPINE_VERSION=$(cat /etc/alpine-release | cut -d '.' -f 1,2)
+            echo http://mirror.clarkson.edu/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories
+            echo http://mirror1.hs-esslingen.de/pub/Mirrors/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories
             apk add -U ca-certificates openssl >/dev/null
             wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v#{Gitlab::Kubernetes::Helm::HELM_VERSION}-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
             mv /tmp/linux-amd64/helm /usr/bin/

package.json

@@ -98,10 +98,11 @@
   "devDependencies": {
     "axios-mock-adapter": "^1.10.0",
     "babel-eslint": "^8.0.2",
-    "babel-plugin-istanbul": "^4.1.5",
+    "babel-plugin-istanbul": "^4.1.6",
     "babel-plugin-rewire": "^1.1.0",
     "babel-template": "^6.26.0",
     "babel-types": "^6.26.0",
+    "chalk": "^2.4.1",
     "commander": "^2.15.1",
     "eslint": "^3.18.0",
     "eslint-config-airbnb-base": "^10.0.1",
@@ -116,13 +117,13 @@
     "istanbul": "^0.4.5",
     "jasmine-core": "^2.9.0",
     "jasmine-jquery": "^2.1.1",
-    "karma": "^2.0.0",
+    "karma": "^2.0.2",
     "karma-chrome-launcher": "^2.2.0",
-    "karma-coverage-istanbul-reporter": "^1.4.1",
+    "karma-coverage-istanbul-reporter": "^1.4.2",
     "karma-jasmine": "^1.1.1",
     "karma-mocha-reporter": "^2.2.5",
     "karma-sourcemap-loader": "^0.3.7",
-    "karma-webpack": "2.0.7",
+    "karma-webpack": "3.0.0",
     "nodemon": "^1.15.1",
     "prettier": "1.11.1",
     "webpack-dev-server": "^2.11.2"

qa/Gemfile.lock

@@ -47,7 +47,7 @@ GEM
     mini_portile2 (2.3.0)
     minitest (5.11.1)
     netrc (0.11.0)
-    nokogiri (1.8.1)
+    nokogiri (1.8.2)
       mini_portile2 (~> 2.3.0)
     pry (0.11.3)
       coderay (~> 1.1.0)

spec/features/dashboard/groups_list_spec.rb

@@ -40,7 +40,7 @@ feature 'Dashboard Groups page', :js do
     expect(page).to have_content(nested_group.name)
   end
 
-  describe 'when filtering groups', :nested_groups do
+  context 'when filtering groups', :nested_groups do
     before do
       group.add_owner(user)
       nested_group.add_owner(user)
@@ -75,7 +75,7 @@ feature 'Dashboard Groups page', :js do
     end
   end
 
-  describe 'group with subgroups', :nested_groups do
+  context 'with subgroups', :nested_groups do
     let!(:subgroup) { create(:group, :public, parent: group) }
 
     before do
@@ -106,7 +106,7 @@ feature 'Dashboard Groups page', :js do
     end
   end
 
-  describe 'when using pagination' do
+  context 'when using pagination' do
     let(:group)  { create(:group, created_at: 5.days.ago) }
     let(:group2) { create(:group, created_at: 2.days.ago) }
 
@@ -141,4 +141,20 @@ feature 'Dashboard Groups page', :js do
       expect(page).not_to have_selector("#group-#{group2.id}")
     end
   end
+
+  context 'when signed in as admin' do
+    let(:admin) { create(:admin) }
+
+    it 'shows only groups admin is member of' do
+      group.add_owner(admin)
+      expect(another_group).to be_persisted
+
+      sign_in(admin)
+      visit dashboard_groups_path
+      wait_for_requests
+
+      expect(page).to have_content(group.name)
+      expect(page).not_to have_content(another_group.name)
+    end
+  end
 end

spec/features/projects/issues/user_toggles_subscription_spec.rb

 require "spec_helper"
 
 describe "User toggles subscription", :js do
-  set(:project) { create(:project_empty_repo, :public) }
-  set(:user) { create(:user) }
-  set(:issue) { create(:issue, project: project, author: user) }
+  let(:project) { create(:project_empty_repo, :public) }
+  let(:user) { create(:user) }
+  let(:issue) { create(:issue, project: project, author: user) }
 
   before do
     project.add_developer(user)
@@ -12,7 +12,7 @@ describe "User toggles subscription", :js do
     visit(project_issue_path(project, issue))
   end
 
-  it "unsibscribes from issue" do
+  it "unsubscribes from issue" do
     subscription_button = find(".js-issuable-subscribe-button")
 
     # Check we're subscribed.

spec/finders/groups_finder_spec.rb

@@ -2,43 +2,71 @@ require 'spec_helper'
 
 describe GroupsFinder do
   describe '#execute' do
-    let(:user)            { create(:user) }
-
-    context 'root level groups' do
-      let!(:private_group)  { create(:group, :private) }
-      let!(:internal_group) { create(:group, :internal) }
-      let!(:public_group)   { create(:group, :public) }
-
-      context 'without a user' do
-        subject { described_class.new.execute }
-
-        it { is_expected.to eq([public_group]) }
+    let(:user) { create(:user) }
+
+    describe 'root level groups' do
+      using RSpec::Parameterized::TableSyntax
+
+      where(:user_type, :params, :results) do
+        nil | { all_available: true  } | %i(public_group user_public_group)
+        nil | { all_available: false  } | %i(public_group user_public_group)
+        nil | {} | %i(public_group user_public_group)
+
+        :regular | { all_available: true  } | %i(public_group internal_group user_public_group user_internal_group
+                                                 user_private_group)
+        :regular | { all_available: false  } | %i(user_public_group user_internal_group user_private_group)
+        :regular | {} | %i(public_group internal_group user_public_group user_internal_group user_private_group)
+
+        :external | { all_available: true  } | %i(public_group user_public_group user_internal_group user_private_group)
+        :external | { all_available: false  } | %i(user_public_group user_internal_group user_private_group)
+        :external | {} | %i(public_group user_public_group user_internal_group user_private_group)
+
+        :admin | { all_available: true  } | %i(public_group internal_group private_group user_public_group
+                                               user_internal_group user_private_group)
+        :admin | { all_available: false  } | %i(user_public_group user_internal_group user_private_group)
+        :admin | {} | %i(public_group internal_group private_group user_public_group user_internal_group
+                         user_private_group)
       end
 
-      context 'with a user' do
-        subject { described_class.new(user).execute }
-
-        context 'normal user' do
-          it { is_expected.to contain_exactly(public_group, internal_group) }
-        end
-
-        context 'external user' do
-          let(:user) { create(:user, external: true) }
-
-          it { is_expected.to contain_exactly(public_group) }
+      with_them do
+        before do
+          # Fixme: Because of an issue: https://github.com/tomykaira/rspec-parameterized/issues/8#issuecomment-381888428
+          # The groups need to be created here, not with let syntax, and also compared by name and not ids
+
+          @groups = {
+            private_group: create(:group, :private, name: 'private_group'),
+            internal_group: create(:group, :internal, name: 'internal_group'),
+            public_group: create(:group, :public, name: 'public_group'),
+
+            user_private_group: create(:group, :private, name: 'user_private_group'),
+            user_internal_group: create(:group, :internal, name: 'user_internal_group'),
+            user_public_group: create(:group, :public, name: 'user_public_group')
+          }
+
+          if user_type
+            user =
+              case user_type
+              when :regular
+                create(:user)
+              when :external
+                create(:user, external: true)
+              when :admin
+                create(:user, :admin)
+              end
+            @groups.values_at(:user_private_group, :user_internal_group, :user_public_group).each do |group|
+              group.add_developer(user)
+            end
+          end
         end
 
-        context 'user is member of the private group' do
-          before do
-            private_group.add_guest(user)
-          end
+        subject { described_class.new(User.last, params).execute.to_a }
 
-          it { is_expected.to contain_exactly(public_group, internal_group, private_group) }
-        end
+        it { is_expected.to match_array(@groups.values_at(*results)) }
       end
     end
 
     context 'subgroups', :nested_groups do
+      let(:user) { create(:user) }
       let!(:parent_group) { create(:group, :public) }
       let!(:public_subgroup) { create(:group, :public, parent: parent_group) }
       let!(:internal_subgroup) { create(:group, :internal, parent: parent_group) }

spec/fixtures/api/schemas/public_api/v4/notes.json

@@ -24,7 +24,10 @@
       "system": { "type": "boolean" },
       "noteable_id": { "type": "integer" },
       "noteable_iid": { "type": "integer" },
-      "noteable_type": { "type": "string" }
+      "noteable_type": { "type": "string" },
+      "resolved": { "type": "boolean" },
+      "resolvable": { "type": "boolean" },
+      "resolved_by": { "type": ["string", "null"] }
     },
     "required": [
       "id", "body", "attachment", "author", "created_at", "updated_at",

spec/helpers/application_helper_spec.rb

@@ -2,8 +2,6 @@
 require 'spec_helper'
 
 describe ApplicationHelper do
-  include UploadHelpers
-
   describe 'current_controller?' do
     it 'returns true when controller matches argument' do
       stub_controller_name('foo')
@@ -54,143 +52,6 @@ describe ApplicationHelper do
     end
   end
 
-  describe 'project_icon' do
-    it 'returns an url for the avatar' do
-      project = create(:project, :public, avatar: File.open(uploaded_image_temp_path))
-
-      expect(helper.project_icon(project.full_path).to_s)
-        .to eq "<img data-src=\"#{project.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
-    end
-  end
-
-  describe 'avatar_icon_for' do
-    let!(:user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: 'bar@example.com') }
-    let(:email) { 'foo@example.com' }
-    let!(:another_user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: email) }
-
-    it 'prefers the user to retrieve the avatar_url' do
-      expect(helper.avatar_icon_for(user, email).to_s)
-        .to eq(user.avatar.url)
-    end
-
-    it 'falls back to email lookup if no user given' do
-      expect(helper.avatar_icon_for(nil, email).to_s)
-        .to eq(another_user.avatar.url)
-    end
-  end
-
-  describe 'avatar_icon_for_email' do
-    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
-
-    context 'using an email' do
-      context 'when there is a matching user' do
-        it 'returns a relative URL for the avatar' do
-          expect(helper.avatar_icon_for_email(user.email).to_s)
-            .to eq(user.avatar.url)
-        end
-      end
-
-      context 'when no user exists for the email' do
-        it 'calls gravatar_icon' do
-          expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
-
-          helper.avatar_icon_for_email('foo@example.com', 20, 2)
-        end
-      end
-
-      context 'without an email passed' do
-        it 'calls gravatar_icon' do
-          expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
-
-          helper.avatar_icon_for_email(nil, 20, 2)
-        end
-      end
-    end
-  end
-
-  describe 'avatar_icon_for_user' do
-    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
-
-    context 'with a user object passed' do
-      it 'returns a relative URL for the avatar' do
-        expect(helper.avatar_icon_for_user(user).to_s)
-          .to eq(user.avatar.url)
-      end
-    end
-
-    context 'without a user object passed' do
-      it 'calls gravatar_icon' do
-        expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
-
-        helper.avatar_icon_for_user(nil, 20, 2)
-      end
-    end
-  end
-
-  describe 'gravatar_icon' do
-    let(:user_email) { 'user@email.com' }
-
-    context 'with Gravatar disabled' do
-      before do
-        stub_application_setting(gravatar_enabled?: false)
-      end
-
-      it 'returns a generic avatar' do
-        expect(helper.gravatar_icon(user_email)).to match_asset_path('no_avatar.png')
-      end
-    end
-
-    context 'with Gravatar enabled' do
-      before do
-        stub_application_setting(gravatar_enabled?: true)
-      end
-
-      it 'returns a generic avatar when email is blank' do
-        expect(helper.gravatar_icon('')).to match_asset_path('no_avatar.png')
-      end
-
-      it 'returns a valid Gravatar URL' do
-        stub_config_setting(https: false)
-
-        expect(helper.gravatar_icon(user_email))
-          .to match('https://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118')
-      end
-
-      it 'uses HTTPs when configured' do
-        stub_config_setting(https: true)
-
-        expect(helper.gravatar_icon(user_email))
-          .to match('https://secure.gravatar.com')
-      end
-
-      it 'returns custom gravatar path when gravatar_url is set' do
-        stub_gravatar_setting(plain_url: 'http://example.local/?s=%{size}&hash=%{hash}')
-
-        expect(gravatar_icon(user_email, 20))
-          .to eq('http://example.local/?s=40&hash=b58c6f14d292556214bd64909bcdb118')
-      end
-
-      it 'accepts a custom size argument' do
-        expect(helper.gravatar_icon(user_email, 64)).to include '?s=128'
-      end
-
-      it 'defaults size to 40@2x when given an invalid size' do
-        expect(helper.gravatar_icon(user_email, nil)).to include '?s=80'
-      end
-
-      it 'accepts a scaling factor' do
-        expect(helper.gravatar_icon(user_email, 40, 3)).to include '?s=120'
-      end
-
-      it 'ignores case and surrounding whitespace' do
-        normal = helper.gravatar_icon('foo@example.com')
-        upcase = helper.gravatar_icon(' FOO@EXAMPLE.COM ')
-
-        expect(normal).to eq upcase
-      end
-    end
-  end
-
   describe 'simple_sanitize' do
     let(:a_tag) { '<a href="#">Foo</a>' }
 

spec/helpers/auth_helper_spec.rb

@@ -18,6 +18,30 @@ describe AuthHelper do
     end
   end
 
+  describe "providers_for_base_controller" do
+    it 'returns all enabled providers from devise' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :github] }
+      expect(helper.providers_for_base_controller).to include(*[:twitter, :github])
+    end
+
+    it 'excludes ldap providers' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :ldapmain] }
+      expect(helper.providers_for_base_controller).not_to include(:ldapmain)
+    end
+  end
+
+  describe "form_based_providers" do
+    it 'includes LDAP providers' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :ldapmain] }
+      expect(helper.form_based_providers).to eq %i(ldapmain)
+    end
+
+    it 'includes crowd provider' do
+      allow(helper).to receive(:auth_providers) { [:twitter, :crowd] }
+      expect(helper.form_based_providers).to eq %i(crowd)
+    end
+  end
+
   describe 'enabled_button_based_providers' do
     before do
       allow(helper).to receive(:auth_providers) { [:twitter, :github] }

spec/helpers/avatars_helper_spec.rb

 require 'rails_helper'
 
 describe AvatarsHelper do
-  include ApplicationHelper
+  include UploadHelpers
 
   let(:user) { create(:user) }
 
+  describe '#project_icon' do
+    it 'returns an url for the avatar' do
+      project = create(:project, :public, avatar: File.open(uploaded_image_temp_path))
+
+      expect(helper.project_icon(project.full_path).to_s)
+        .to eq "<img data-src=\"#{project.avatar.url}\" class=\" lazy\" src=\"#{LazyImageTagHelper.placeholder_image}\" />"
+    end
+  end
+
+  describe '#avatar_icon_for' do
+    let!(:user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: 'bar@example.com') }
+    let(:email) { 'foo@example.com' }
+    let!(:another_user) { create(:user, avatar: File.open(uploaded_image_temp_path), email: email) }
+
+    it 'prefers the user to retrieve the avatar_url' do
+      expect(helper.avatar_icon_for(user, email).to_s)
+        .to eq(user.avatar.url)
+    end
+
+    it 'falls back to email lookup if no user given' do
+      expect(helper.avatar_icon_for(nil, email).to_s)
+        .to eq(another_user.avatar.url)
+    end
+  end
+
+  describe '#avatar_icon_for_email' do
+    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
+
+    context 'using an email' do
+      context 'when there is a matching user' do
+        it 'returns a relative URL for the avatar' do
+          expect(helper.avatar_icon_for_email(user.email).to_s)
+            .to eq(user.avatar.url)
+        end
+      end
+
+      context 'when no user exists for the email' do
+        it 'calls gravatar_icon' do
+          expect(helper).to receive(:gravatar_icon).with('foo@example.com', 20, 2)
+
+          helper.avatar_icon_for_email('foo@example.com', 20, 2)
+        end
+      end
+
+      context 'without an email passed' do
+        it 'calls gravatar_icon' do
+          expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
+
+          helper.avatar_icon_for_email(nil, 20, 2)
+        end
+      end
+    end
+  end
+
+  describe '#avatar_icon_for_user' do
+    let(:user) { create(:user, avatar: File.open(uploaded_image_temp_path)) }
+
+    context 'with a user object passed' do
+      it 'returns a relative URL for the avatar' do
+        expect(helper.avatar_icon_for_user(user).to_s)
+          .to eq(user.avatar.url)
+      end
+    end
+
+    context 'without a user object passed' do
+      it 'calls gravatar_icon' do
+        expect(helper).to receive(:gravatar_icon).with(nil, 20, 2)
+
+        helper.avatar_icon_for_user(nil, 20, 2)
+      end
+    end
+  end
+
+  describe '#gravatar_icon' do
+    let(:user_email) { 'user@email.com' }
+
+    context 'with Gravatar disabled' do
+      before do
+        stub_application_setting(gravatar_enabled?: false)
+      end
+
+      it 'returns a generic avatar' do
+        expect(helper.gravatar_icon(user_email)).to match_asset_path('no_avatar.png')
+      end
+    end
+
+    context 'with Gravatar enabled' do
+      before do
+        stub_application_setting(gravatar_enabled?: true)
+      end
+
+      it 'returns a generic avatar when email is blank' do
+        expect(helper.gravatar_icon('')).to match_asset_path('no_avatar.png')
+      end
+
+      it 'returns a valid Gravatar URL' do
+        stub_config_setting(https: false)
+
+        expect(helper.gravatar_icon(user_email))
+          .to match('https://www.gravatar.com/avatar/b58c6f14d292556214bd64909bcdb118')
+      end
+
+      it 'uses HTTPs when configured' do
+        stub_config_setting(https: true)
+
+        expect(helper.gravatar_icon(user_email))
+          .to match('https://secure.gravatar.com')
+      end
+
+      it 'returns custom gravatar path when gravatar_url is set' do
+        stub_gravatar_setting(plain_url: 'http://example.local/?s=%{size}&hash=%{hash}')
+
+        expect(gravatar_icon(user_email, 20))
+          .to eq('http://example.local/?s=40&hash=b58c6f14d292556214bd64909bcdb118')
+      end
+
+      it 'accepts a custom size argument' do
+        expect(helper.gravatar_icon(user_email, 64)).to include '?s=128'
+      end
+
+      it 'defaults size to 40@2x when given an invalid size' do
+        expect(helper.gravatar_icon(user_email, nil)).to include '?s=80'
+      end
+
+      it 'accepts a scaling factor' do
+        expect(helper.gravatar_icon(user_email, 40, 3)).to include '?s=120'
+      end
+
+      it 'ignores case and surrounding whitespace' do
+        normal = helper.gravatar_icon('foo@example.com')
+        upcase = helper.gravatar_icon(' FOO@EXAMPLE.COM ')
+
+        expect(normal).to eq upcase
+      end
+    end
+  end
+
   describe '#user_avatar' do
     subject { helper.user_avatar(user: user) }
 

spec/javascripts/test_bundle.js

@@ -84,21 +84,11 @@ beforeEach(() => {
 
 const axiosDefaultAdapter = getDefaultAdapter();
 
-let testFiles = process.env.TEST_FILES || [];
-if (testFiles.length > 0) {
-  testFiles = testFiles.map(path => path.replace(/^spec\/javascripts\//, '').replace(/\.js$/, ''));
-  console.log(`Running only tests matching: ${testFiles}`);
-} else {
-  console.log('Running all tests');
-}
-
 // render all of our tests
 const testsContext = require.context('.', true, /_spec$/);
 testsContext.keys().forEach(function(path) {
   try {
-    if (testFiles.length === 0 || testFiles.some(p => path.includes(p))) {
-      testsContext(path);
-    }
+    testsContext(path);
   } catch (err) {
     console.error('[ERROR] Unable to load spec: ', path);
     describe('Test bundle', function() {

spec/lib/banzai/filter/commit_trailers_filter_spec.rb

@@ -47,16 +47,36 @@ describe Banzai::Filter::CommitTrailersFilter do
       )
     end
 
-    it 'non GitLab users and replaces them with mailto links' do
-      _, message_html = build_commit_message(
-        trailer: trailer,
-        name: FFaker::Name.name,
-        email: email
-      )
+    context 'non GitLab users' do
+      shared_examples 'mailto links' do
+        it 'replaces them with mailto links' do
+          _, message_html = build_commit_message(
+            trailer: trailer,
+            name: FFaker::Name.name,
+            email: email
+          )
 
-      doc = filter(message_html)
+          doc = filter(message_html)
 
-      expect_to_have_mailto_link(doc, email: email, trailer: trailer)
+          expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: trailer)
+        end
+      end
+
+      context 'when Gravatar is disabled' do
+        before do
+          stub_application_setting(gravatar_enabled: false)
+        end
+
+        it_behaves_like 'mailto links'
+      end
+
+      context 'when Gravatar is enabled' do
+        before do
+          stub_application_setting(gravatar_enabled: true)
+        end
+
+        it_behaves_like 'mailto links'
+      end
     end
 
     it 'multiple trailers in the same message' do
@@ -69,7 +89,7 @@ describe Banzai::Filter::CommitTrailersFilter do
       doc = filter(message)
 
       expect_to_have_user_link_with_avatar(doc, user: user, trailer: trailer)
-      expect_to_have_mailto_link(doc, email: email, trailer: different_trailer)
+      expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: different_trailer)
     end
 
     context 'special names' do
@@ -90,7 +110,7 @@ describe Banzai::Filter::CommitTrailersFilter do
 
           doc = filter(message_html)
 
-          expect_to_have_mailto_link(doc, email: email, trailer: trailer)
+          expect_to_have_mailto_link_with_avatar(doc, email: email, trailer: trailer)
           expect(doc.text).to match Regexp.escape(message)
         end
       end

spec/lib/gitlab/kubernetes/helm/base_command_spec.rb

@@ -4,22 +4,10 @@ describe Gitlab::Kubernetes::Helm::BaseCommand do
   let(:application) { create(:clusters_applications_helm) }
   let(:base_command) { described_class.new(application.name) }
 
-  describe '#generate_script' do
-    let(:helm_version) { Gitlab::Kubernetes::Helm::HELM_VERSION }
-    let(:command) do
-      <<~HEREDOC
-         set -eo pipefail
-         apk add -U ca-certificates openssl >/dev/null
-         wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v#{helm_version}-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
-         mv /tmp/linux-amd64/helm /usr/bin/
-      HEREDOC
-    end
-
-    subject { base_command.generate_script }
+  subject { base_command }
 
-    it 'should return a command that prepares the environment for helm-cli' do
-      expect(subject).to eq(command)
-    end
+  it_behaves_like 'helm commands' do
+    let(:commands) { '' }
   end
 
   describe '#pod_resource' do

spec/lib/gitlab/kubernetes/helm/init_command_spec.rb

@@ -2,23 +2,9 @@ require 'spec_helper'
 
 describe Gitlab::Kubernetes::Helm::InitCommand do
   let(:application) { create(:clusters_applications_helm) }
-  let(:init_command) { described_class.new(application.name) }
+  let(:commands) { 'helm init >/dev/null' }
 
-  describe '#generate_script' do
-    let(:command) do
-      <<~MSG.chomp
-        set -eo pipefail
-        apk add -U ca-certificates openssl >/dev/null
-        wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
-        mv /tmp/linux-amd64/helm /usr/bin/
-        helm init >/dev/null
-      MSG
-    end
+  subject { described_class.new(application.name) }
 
-    subject { init_command.generate_script }
-
-    it 'should return the appropriate command' do
-      is_expected.to eq(command)
-    end
-  end
+  it_behaves_like 'helm commands'
 end

spec/lib/gitlab/kubernetes/helm/install_command_spec.rb

@@ -12,50 +12,36 @@ describe Gitlab::Kubernetes::Helm::InstallCommand do
     )
   end
 
-  describe '#generate_script' do
-    let(:command) do
-      <<~MSG
-      set -eo pipefail
-      apk add -U ca-certificates openssl >/dev/null
-      wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
-      mv /tmp/linux-amd64/helm /usr/bin/
-      helm init --client-only >/dev/null
-      helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null
-      MSG
-    end
-
-    subject { install_command.generate_script }
+  subject { install_command }
 
-    it 'should return appropriate command' do
-      is_expected.to eq(command)
+  it_behaves_like 'helm commands' do
+    let(:commands) do
+      <<~EOS
+         helm init --client-only >/dev/null
+         helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null
+      EOS
     end
+  end
 
-    context 'with an application with a repository' do
-      let(:ci_runner) { create(:ci_runner) }
-      let(:application) { create(:clusters_applications_runner, runner: ci_runner) }
-      let(:install_command) do
-        described_class.new(
-          application.name,
-          chart: application.chart,
-          values: application.values,
-          repository: application.repository
-        )
-      end
-
-      let(:command) do
-        <<~MSG
-        set -eo pipefail
-        apk add -U ca-certificates openssl >/dev/null
-        wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
-        mv /tmp/linux-amd64/helm /usr/bin/
-        helm init --client-only >/dev/null
-        helm repo add #{application.name} #{application.repository}
-        helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null
-        MSG
-      end
+  context 'with an application with a repository' do
+    let(:ci_runner) { create(:ci_runner) }
+    let(:application) { create(:clusters_applications_runner, runner: ci_runner) }
+    let(:install_command) do
+      described_class.new(
+        application.name,
+        chart: application.chart,
+        values: application.values,
+        repository: application.repository
+      )
+    end
 
-      it 'should return appropriate command' do
-        is_expected.to eq(command)
+    it_behaves_like 'helm commands' do
+      let(:commands) do
+        <<~EOS
+           helm init --client-only >/dev/null
+           helm repo add #{application.name} #{application.repository}
+           helm install #{application.chart} --name #{application.name} --namespace #{namespace} -f /data/helm/#{application.name}/config/values.yaml >/dev/null
+        EOS
       end
     end
   end

spec/models/diff_note_spec.rb

@@ -85,12 +85,35 @@ describe DiffNote do
   end
 
   describe "#diff_file" do
-    it "returns the correct diff file" do
-      diff_file = subject.diff_file
+    context 'when the discussion was created in the diff' do
+      it 'returns correct diff file' do
+        diff_file = subject.diff_file
 
-      expect(diff_file.old_path).to eq(position.old_path)
-      expect(diff_file.new_path).to eq(position.new_path)
-      expect(diff_file.diff_refs).to eq(position.diff_refs)
+        expect(diff_file.old_path).to eq(position.old_path)
+        expect(diff_file.new_path).to eq(position.new_path)
+        expect(diff_file.diff_refs).to eq(position.diff_refs)
+      end
+    end
+
+    context 'when discussion is outdated or not created in the diff' do
+      let(:diff_refs) { project.commit(sample_commit.id).diff_refs }
+      let(:position) do
+        Gitlab::Diff::Position.new(
+          old_path: "files/ruby/popen.rb",
+          new_path: "files/ruby/popen.rb",
+          old_line: nil,
+          new_line: 14,
+          diff_refs: diff_refs
+        )
+      end
+
+      it 'returns the correct diff file' do
+        diff_file = subject.diff_file
+
+        expect(diff_file.old_path).to eq(position.old_path)
+        expect(diff_file.new_path).to eq(position.new_path)
+        expect(diff_file.diff_refs).to eq(position.diff_refs)
+      end
     end
   end
 

spec/requests/api/discussions_spec.rb

@@ -2,32 +2,53 @@ require 'spec_helper'
 
 describe API::Discussions do
   let(:user) { create(:user) }
-  let!(:project) { create(:project, :public, namespace: user.namespace) }
+  let!(:project) { create(:project, :public, :repository, namespace: user.namespace) }
   let(:private_user)    { create(:user) }
 
   before do
-    project.add_reporter(user)
+    project.add_developer(user)
   end
 
-  context "when noteable is an Issue" do
+  context 'when noteable is an Issue' do
     let!(:issue) { create(:issue, project: project, author: user) }
     let!(:issue_note) { create(:discussion_note_on_issue, noteable: issue, project: project, author: user) }
 
-    it_behaves_like "discussions API", 'projects', 'issues', 'iid' do
+    it_behaves_like 'discussions API', 'projects', 'issues', 'iid' do
       let(:parent) { project }
       let(:noteable) { issue }
       let(:note) { issue_note }
     end
   end
 
-  context "when noteable is a Snippet" do
+  context 'when noteable is a Snippet' do
     let!(:snippet) { create(:project_snippet, project: project, author: user) }
     let!(:snippet_note) { create(:discussion_note_on_snippet, noteable: snippet, project: project, author: user) }
 
-    it_behaves_like "discussions API", 'projects', 'snippets', 'id' do
+    it_behaves_like 'discussions API', 'projects', 'snippets', 'id' do
       let(:parent) { project }
       let(:noteable) { snippet }
       let(:note) { snippet_note }
     end
   end
+
+  context 'when noteable is a Merge Request' do
+    let!(:noteable) { create(:merge_request_with_diffs, source_project: project, target_project: project, author: user) }
+    let!(:note) { create(:discussion_note_on_merge_request, noteable: noteable, project: project, author: user) }
+    let!(:diff_note) { create(:diff_note_on_merge_request, noteable: noteable, project: project, author: user) }
+    let(:parent) { project }
+
+    it_behaves_like 'discussions API', 'projects', 'merge_requests', 'iid'
+    it_behaves_like 'diff discussions API', 'projects', 'merge_requests', 'iid'
+    it_behaves_like 'resolvable discussions API', 'projects', 'merge_requests', 'iid'
+  end
+
+  context 'when noteable is a Commit' do
+    let!(:noteable) { create(:commit, project: project, author: user) }
+    let!(:note) { create(:discussion_note_on_commit, commit_id: noteable.id, project: project, author: user) }
+    let!(:diff_note) { create(:diff_note_on_commit, commit_id: noteable.id, project: project, author: user) }
+    let(:parent) { project }
+
+    it_behaves_like 'discussions API', 'projects', 'repository/commits', 'id'
+    it_behaves_like 'diff discussions API', 'projects', 'repository/commits', 'id'
+  end
 end

spec/services/notes/resolve_service_spec.rb

+require 'spec_helper'
+
+describe Notes::ResolveService do
+  let(:merge_request) { create(:merge_request) }
+  let(:note) { create(:diff_note_on_merge_request, noteable: merge_request, project: merge_request.project) }
+  let(:user) { merge_request.author }
+
+  describe '#execute' do
+    it "resolves the note" do
+      described_class.new(merge_request.project, user).execute(note)
+      note.reload
+
+      expect(note.resolved?).to be true
+      expect(note.resolved_by).to eq(user)
+    end
+
+    it "sends notifications if all discussions are resolved" do
+      expect_any_instance_of(MergeRequests::ResolvedDiscussionNotificationService).to receive(:execute).with(merge_request)
+
+      described_class.new(merge_request.project, user).execute(note)
+    end
+  end
+end

spec/support/commit_trailers_spec_helper.rb

@@ -8,7 +8,7 @@ module CommitTrailersSpecHelper
     expect(wrapper.attribute('data-user').value).to eq user.id.to_s
   end
 
-  def expect_to_have_mailto_link(doc, email:, trailer:)
+  def expect_to_have_mailto_link_with_avatar(doc, email:, trailer:)
     wrapper = find_user_wrapper(doc, trailer)
 
     expect_to_have_links_with_url_and_avatar(wrapper, "mailto:#{CGI.escape_html(email)}", email)

spec/support/shared_examples/helm_generated_script.rb

+shared_examples 'helm commands' do
+  describe '#generate_script' do
+    let(:helm_setup) do
+      <<~EOS
+         set -eo pipefail
+         ALPINE_VERSION=$(cat /etc/alpine-release | cut -d '.' -f 1,2)
+         echo http://mirror.clarkson.edu/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories
+         echo http://mirror1.hs-esslingen.de/pub/Mirrors/alpine/v$ALPINE_VERSION/main >> /etc/apk/repositories
+         apk add -U ca-certificates openssl >/dev/null
+         wget -q -O - https://kubernetes-helm.storage.googleapis.com/helm-v2.7.0-linux-amd64.tar.gz | tar zxC /tmp >/dev/null
+         mv /tmp/linux-amd64/helm /usr/bin/
+      EOS
+    end
+
+    it 'should return appropriate command' do
+      expect(subject.generate_script).to eq(helm_setup + commands)
+    end
+  end
+end

spec/support/shared_examples/requests/api/diff_discussions.rb

+shared_examples 'diff discussions API' do |parent_type, noteable_type, id_name|
+  describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do
+    it "includes diff discussions" do
+      get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user)
+
+      discussion = json_response.find { |record| record['id'] == diff_note.discussion_id }
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(discussion).not_to be_nil
+      expect(discussion['individual_note']).to eq(false)
+      expect(discussion['notes'].first['body']).to eq(diff_note.note)
+    end
+  end
+
+  describe "GET /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do
+    it "returns a discussion by id" do
+      get api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions/#{diff_note.discussion_id}", user)
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['id']).to eq(diff_note.discussion_id)
+      expect(json_response['notes'].first['body']).to eq(diff_note.note)
+      expect(json_response['notes'].first['position']).to eq(diff_note.position.to_h.stringify_keys)
+    end
+  end
+
+  describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions" do
+    it "creates a new diff note" do
+      position = diff_note.position.to_h
+
+      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position
+
+      expect(response).to have_gitlab_http_status(201)
+      expect(json_response['notes'].first['body']).to eq('hi!')
+      expect(json_response['notes'].first['type']).to eq('DiffNote')
+      expect(json_response['notes'].first['position']).to eq(position.stringify_keys)
+    end
+
+    it "returns a 400 bad request error when position is invalid" do
+      position = diff_note.position.to_h.merge(new_line: '100000')
+
+      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/discussions", user), body: 'hi!', position: position
+
+      expect(response).to have_gitlab_http_status(400)
+    end
+  end
+
+  describe "POST /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes" do
+    it 'adds a new note to the diff discussion' do
+      post api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+               "discussions/#{diff_note.discussion_id}/notes", user), body: 'hi!'
+
+      expect(response).to have_gitlab_http_status(201)
+      expect(json_response['body']).to eq('hi!')
+      expect(json_response['type']).to eq('DiffNote')
+    end
+  end
+end

spec/support/shared_examples/requests/api/resolvable_discussions.rb

+shared_examples 'resolvable discussions API' do |parent_type, noteable_type, id_name|
+  describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id" do
+    it "resolves discussion if resolved is true" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}", user), resolved: true
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['notes'].size).to eq(1)
+      expect(json_response['notes'][0]['resolved']).to eq(true)
+    end
+
+    it "unresolves discussion if resolved is false" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}", user), resolved: false
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['notes'].size).to eq(1)
+      expect(json_response['notes'][0]['resolved']).to eq(false)
+    end
+
+    it "returns a 400 bad request error if resolved parameter is not passed" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}", user)
+
+      expect(response).to have_gitlab_http_status(400)
+    end
+
+    it "returns a 401 unauthorized error if user is not authenticated" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}"), resolved: true
+
+      expect(response).to have_gitlab_http_status(401)
+    end
+
+    it "returns a 403 error if user resolves discussion of someone else" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}", private_user), resolved: true
+
+      expect(response).to have_gitlab_http_status(403)
+    end
+
+    context 'when user does not have access to read the discussion' do
+      before do
+        parent.update!(visibility_level: Gitlab::VisibilityLevel::PRIVATE)
+      end
+
+      it 'responds with 404' do
+        put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+                "discussions/#{note.discussion_id}", private_user), resolved: true
+
+        expect(response).to have_gitlab_http_status(404)
+      end
+    end
+  end
+
+  describe "PUT /#{parent_type}/:id/#{noteable_type}/:noteable_id/discussions/:discussion_id/notes/:note_id" do
+    it 'returns resolved note when resolved parameter is true' do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}/notes/#{note.id}", user), resolved: true
+
+      expect(response).to have_gitlab_http_status(200)
+      expect(json_response['resolved']).to eq(true)
+    end
+
+    it 'returns a 404 error when note id not found' do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}/notes/12345", user),
+              body: 'Hello!'
+
+      expect(response).to have_gitlab_http_status(404)
+    end
+
+    it 'returns a 400 bad request error if neither body nor resolved parameter is given' do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}/notes/#{note.id}", user)
+
+      expect(response).to have_gitlab_http_status(400)
+    end
+
+    it "returns a 403 error if user resolves note of someone else" do
+      put api("/#{parent_type}/#{parent.id}/#{noteable_type}/#{noteable[id_name]}/"\
+              "discussions/#{note.discussion_id}/notes/#{note.id}", private_user), resolved: true
+
+      expect(response).to have_gitlab_http_status(403)
+    end
+  end
+end