Merge branch 'ce-to-ee-2018-01-29' into 'master'

CE upstream - 2018-01-29 15:59 UTC

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

app/assets/javascripts/awards_handler.js

 /* eslint-disable class-methods-use-this */
 import _ from 'underscore';
 import Cookies from 'js-cookie';
+import { s__ } from './locale';
 import { isInIssuePage, updateTooltipTitle } from './lib/utils/common_utils';
-import Flash from './flash';
+import flash from './flash';
+import axios from './lib/utils/axios_utils';
 
 const animationEndEventString = 'animationend webkitAnimationEnd MSAnimationEnd oAnimationEnd';
 const transitionEndEventString = 'transitionend webkitTransitionEnd oTransitionEnd MSTransitionEnd';
@@ -441,13 +443,15 @@ class AwardsHandler {
     if (this.isUserAuthored($emojiButton)) {
       this.userAuthored($emojiButton);
     } else {
-      $.post(awardUrl, {
+      axios.post(awardUrl, {
         name: emoji,
-      }, (data) => {
+      })
+      .then(({ data }) => {
         if (data.ok) {
           callback();
         }
-      }).fail(() => new Flash('Something went wrong on our end.'));
+      })
+      .catch(() => flash(s__('Something went wrong on our end.')));
     }
   }
 

app/assets/javascripts/sidebar/components/assignees/assignees.js

@@ -84,7 +84,7 @@ export default {
       return !this.showLess || (index < this.defaultRenderCount && this.showLess);
     },
     avatarUrl(user) {
-      return user.avatar || user.avatar_url;
+      return user.avatar || user.avatar_url || gon.default_avatar_url;
     },
     assigneeUrl(user) {
       return `${this.rootPath}${user.username}`;

app/assets/javascripts/users_select.js

@@ -492,7 +492,7 @@ function UsersSelect(currentUser, els, options = {}) {
         renderRow: function(user) {
           var avatar, img, listClosingTags, listWithName, listWithUserName, username;
           username = user.username ? "@" + user.username : "";
-          avatar = user.avatar_url ? user.avatar_url : false;
+          avatar = user.avatar_url ? user.avatar_url : gon.default_avatar_url;
 
           let selected = false;
 
@@ -513,10 +513,8 @@ function UsersSelect(currentUser, els, options = {}) {
           if (user.beforeDivider != null) {
             `<li><a href='#' class='${selected === true ? 'is-active' : ''}'>${_.escape(user.name)}</a></li>`;
           } else {
-            if (avatar) {
             img = "<img src='" + avatar + "' class='avatar avatar-inline' width='32' />";
           }
-          }
 
           return `
             <li data-user-id=${user.id}>

app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.js

-import Flash from '../../../flash';
-import mrWidgetAuthorTime from '../../components/mr_widget_author_time';
-import tooltip from '../../../vue_shared/directives/tooltip';
-import loadingIcon from '../../../vue_shared/components/loading_icon.vue';
-import statusIcon from '../mr_widget_status_icon.vue';
-import eventHub from '../../event_hub';
-
-export default {
-  name: 'MRWidgetMerged',
-  props: {
-    mr: { type: Object, required: true },
-    service: { type: Object, required: true },
-  },
-  data() {
-    return {
-      isMakingRequest: false,
-    };
-  },
-  directives: {
-    tooltip,
-  },
-  components: {
-    'mr-widget-author-and-time': mrWidgetAuthorTime,
-    loadingIcon,
-    statusIcon,
-  },
-  computed: {
-    shouldShowRemoveSourceBranch() {
-      const { sourceBranchRemoved, isRemovingSourceBranch, canRemoveSourceBranch } = this.mr;
-
-      return !sourceBranchRemoved && canRemoveSourceBranch &&
-        !this.isMakingRequest && !isRemovingSourceBranch;
-    },
-    shouldShowSourceBranchRemoving() {
-      const { sourceBranchRemoved, isRemovingSourceBranch } = this.mr;
-      return !sourceBranchRemoved && (isRemovingSourceBranch || this.isMakingRequest);
-    },
-    shouldShowMergedButtons() {
-      const { canRevertInCurrentMR, canCherryPickInCurrentMR, revertInForkPath,
-        cherryPickInForkPath } = this.mr;
-
-      return canRevertInCurrentMR || canCherryPickInCurrentMR ||
-        revertInForkPath || cherryPickInForkPath;
-    },
-  },
-  methods: {
-    removeSourceBranch() {
-      this.isMakingRequest = true;
-      this.service.removeSourceBranch()
-        .then(res => res.data)
-        .then((data) => {
-          if (data.message === 'Branch was removed') {
-            eventHub.$emit('MRWidgetUpdateRequested', () => {
-              this.isMakingRequest = false;
-            });
-          }
-        })
-        .catch(() => {
-          this.isMakingRequest = false;
-          new Flash('Something went wrong. Please try again.'); // eslint-disable-line
-        });
-    },
-  },
-  template: `
-    <div class="mr-widget-body media">
-      <status-icon status="success" />
-      <div class="media-body">
-        <div class="space-children">
-          <mr-widget-author-and-time
-            actionText="Merged by"
-            :author="mr.metrics.mergedBy"
-            :date-title="mr.metrics.mergedAt"
-            :date-readable="mr.metrics.readableMergedAt" />
-          <a
-            v-if="mr.canRevertInCurrentMR"
-            v-tooltip
-            class="btn btn-close btn-xs"
-            href="#modal-revert-commit"
-            data-toggle="modal"
-            data-container="body"
-            title="Revert this merge request in a new merge request">
-            Revert
-          </a>
-          <a
-            v-else-if="mr.revertInForkPath"
-            v-tooltip
-            class="btn btn-close btn-xs"
-            data-method="post"
-            :href="mr.revertInForkPath"
-            title="Revert this merge request in a new merge request">
-            Revert
-          </a>
-          <a
-            v-if="mr.canCherryPickInCurrentMR"
-            v-tooltip
-            class="btn btn-default btn-xs"
-            href="#modal-cherry-pick-commit"
-            data-toggle="modal"
-            data-container="body"
-            title="Cherry-pick this merge request in a new merge request">
-            Cherry-pick
-          </a>
-          <a
-            v-else-if="mr.cherryPickInForkPath"
-            v-tooltip
-            class="btn btn-default btn-xs"
-            data-method="post"
-            :href="mr.cherryPickInForkPath"
-            title="Cherry-pick this merge request in a new merge request">
-            Cherry-pick
-          </a>
-        </div>
-        <section class="mr-info-list">
-          <p>
-            The changes were merged into
-            <span class="label-branch">
-              <a :href="mr.targetBranchPath">{{mr.targetBranch}}</a>
-            </span>
-          </p>
-          <p v-if="mr.sourceBranchRemoved">The source branch has been removed</p>
-          <p v-if="shouldShowRemoveSourceBranch" class="space-children">
-            <span>You can remove source branch now</span>
-            <button
-              @click="removeSourceBranch"
-              :disabled="isMakingRequest"
-              type="button"
-              class="btn btn-xs btn-default js-remove-branch-button">
-              Remove Source Branch
-            </button>
-          </p>
-          <p v-if="shouldShowSourceBranchRemoving">
-            <loading-icon inline />
-            <span>The source branch is being removed</span>
-          </p>
-        </section>
-      </div>
-    </div>
-  `,
-};

app/assets/javascripts/vue_merge_request_widget/components/states/mr_widget_merged.vue

+<script>
+  import Flash from '~/flash';
+  import tooltip from '~/vue_shared/directives/tooltip';
+  import loadingIcon from '~/vue_shared/components/loading_icon.vue';
+  import { s__, __ } from '~/locale';
+  import mrWidgetAuthorTime from '../../components/mr_widget_author_time';
+  import statusIcon from '../mr_widget_status_icon.vue';
+  import eventHub from '../../event_hub';
+
+  export default {
+    name: 'MRWidgetMerged',
+    directives: {
+      tooltip,
+    },
+    components: {
+      mrWidgetAuthorTime,
+      loadingIcon,
+      statusIcon,
+    },
+    props: {
+      mr: {
+        type: Object,
+        required: true,
+        default: () => ({}),
+      },
+      service: {
+        type: Object,
+        required: true,
+        default: () => ({}),
+      },
+    },
+    data() {
+      return {
+        isMakingRequest: false,
+      };
+    },
+    computed: {
+      shouldShowRemoveSourceBranch() {
+        const {
+          sourceBranchRemoved,
+          isRemovingSourceBranch,
+          canRemoveSourceBranch,
+        } = this.mr;
+
+        return !sourceBranchRemoved &&
+          canRemoveSourceBranch &&
+          !this.isMakingRequest &&
+          !isRemovingSourceBranch;
+      },
+      shouldShowSourceBranchRemoving() {
+        const {
+          sourceBranchRemoved,
+          isRemovingSourceBranch,
+        } = this.mr;
+        return !sourceBranchRemoved &&
+          (isRemovingSourceBranch || this.isMakingRequest);
+      },
+      shouldShowMergedButtons() {
+        const {
+          canRevertInCurrentMR,
+          canCherryPickInCurrentMR,
+          revertInForkPath,
+          cherryPickInForkPath,
+        } = this.mr;
+
+        return canRevertInCurrentMR ||
+          canCherryPickInCurrentMR ||
+          revertInForkPath ||
+          cherryPickInForkPath;
+      },
+      revertTitle() {
+        return s__('mrWidget|Revert this merge request in a new merge request');
+      },
+      cherryPickTitle() {
+        return s__('mrWidget|Cherry-pick this merge request in a new merge request');
+      },
+      revertLabel() {
+        return s__('mrWidget|Revert');
+      },
+      cherryPickLabel() {
+        return s__('mrWidget|Cherry-pick');
+      },
+    },
+    methods: {
+      removeSourceBranch() {
+        this.isMakingRequest = true;
+
+        this.service.removeSourceBranch()
+          .then(res => res.data)
+          .then((data) => {
+            if (data.message === 'Branch was removed') {
+              eventHub.$emit('MRWidgetUpdateRequested', () => {
+                this.isMakingRequest = false;
+              });
+            }
+          })
+          .catch(() => {
+            this.isMakingRequest = false;
+            Flash(__('Something went wrong. Please try again.'));
+          });
+      },
+    },
+  };
+</script>
+<template>
+  <div class="mr-widget-body media">
+    <status-icon status="success" />
+    <div class="media-body">
+      <div class="space-children">
+        <mr-widget-author-time
+          :action-text="s__('mrWidget|Merged by')"
+          :author="mr.metrics.mergedBy"
+          :date-title="mr.metrics.mergedAt"
+          :date-readable="mr.metrics.readableMergedAt"
+        />
+        <a
+          v-if="mr.canRevertInCurrentMR"
+          v-tooltip
+          class="btn btn-close btn-xs"
+          href="#modal-revert-commit"
+          data-toggle="modal"
+          data-container="body"
+          :title="revertTitle"
+        >
+          {{ revertLabel }}
+        </a>
+        <a
+          v-else-if="mr.revertInForkPath"
+          v-tooltip
+          class="btn btn-close btn-xs"
+          data-method="post"
+          :href="mr.revertInForkPath"
+          :title="revertTitle"
+        >
+          {{ revertLabel }}
+        </a>
+        <a
+          v-if="mr.canCherryPickInCurrentMR"
+          v-tooltip
+          class="btn btn-default btn-xs"
+          href="#modal-cherry-pick-commit"
+          data-toggle="modal"
+          data-container="body"
+          :title="cherryPickTitle"
+        >
+          {{ cherryPickLabel }}
+        </a>
+        <a
+          v-else-if="mr.cherryPickInForkPath"
+          v-tooltip
+          class="btn btn-default btn-xs"
+          data-method="post"
+          :href="mr.cherryPickInForkPath"
+          :title="cherryPickTitle"
+        >
+          {{ cherryPickLabel }}
+        </a>
+      </div>
+      <section class="mr-info-list">
+        <p>
+          {{ s__("mrWidget|The changes were merged into") }}
+          <span class="label-branch">
+            <a :href="mr.targetBranchPath">{{ mr.targetBranch }}</a>
+          </span>
+        </p>
+        <p v-if="mr.sourceBranchRemoved">
+          {{ s__("mrWidget|The source branch has been removed") }}
+        </p>
+        <p
+          v-if="shouldShowRemoveSourceBranch"
+          class="space-children"
+        >
+          <span>{{ s__("mrWidget|You can remove source branch now") }}</span>
+          <button
+            @click="removeSourceBranch"
+            :disabled="isMakingRequest"
+            type="button"
+            class="btn btn-xs btn-default js-remove-branch-button"
+          >
+            {{ s__("mrWidget|Remove Source Branch") }}
+          </button>
+        </p>
+        <p v-if="shouldShowSourceBranchRemoving">
+          <loading-icon :inline="true" />
+          <span>
+            {{ s__("mrWidget|The source branch is being removed") }}
+          </span>
+        </p>
+      </section>
+    </div>
+  </div>
+</template>

app/assets/javascripts/vue_merge_request_widget/dependencies.js

@@ -16,7 +16,7 @@ export { default as WidgetMergeHelp } from './components/mr_widget_merge_help';
 export { default as WidgetPipeline } from './components/mr_widget_pipeline.vue';
 export { default as WidgetDeployment } from './components/mr_widget_deployment';
 export { default as WidgetRelatedLinks } from './components/mr_widget_related_links';
-export { default as MergedState } from './components/states/mr_widget_merged';
+export { default as MergedState } from './components/states/mr_widget_merged.vue';
 export { default as FailedToMerge } from './components/states/mr_widget_failed_to_merge.vue';
 export { default as ClosedState } from './components/states/mr_widget_closed.vue';
 export { default as MergingState } from './components/states/mr_widget_merging.vue';

app/helpers/application_helper.rb

@@ -89,7 +89,7 @@ module ApplicationHelper
   end
 
   def default_avatar
-    'no_avatar.png'
+    asset_path('no_avatar.png')
   end
 
   def last_commit(project)

app/services/merge_requests/refresh_service.rb

@@ -9,7 +9,8 @@ module MergeRequests
       Gitlab::GitalyClient.allow_n_plus_1_calls(&method(:find_new_commits))
       # Be sure to close outstanding MRs before reloading them to avoid generating an
       # empty diff during a manual merge
-      close_merge_requests
+      close_upon_missing_source_branch_ref
+      post_merge_manually_merged
       reload_merge_requests
       reset_merge_when_pipeline_succeeds
       mark_pending_todos_done
@@ -30,11 +31,22 @@ module MergeRequests
 
     private
 
+    def close_upon_missing_source_branch_ref
+      # MergeRequest#reload_diff ignores not opened MRs. This means it won't
+      # create an `empty` diff for `closed` MRs without a source branch, keeping
+      # the latest diff state as the last _valid_ one.
+      merge_requests_for_source_branch.reject(&:source_branch_exists?).each do |mr|
+        MergeRequests::CloseService
+          .new(mr.target_project, @current_user)
+          .execute(mr)
+      end
+    end
+
     # Collect open merge requests that target same branch we push into
     # and close if push to master include last commit from merge request
     # We need this to close(as merged) merge requests that were merged into
     # target branch manually
-    def close_merge_requests
+    def post_merge_manually_merged
       commit_ids = @commits.map(&:id)
       merge_requests = @project.merge_requests.preload(:latest_merge_request_diff).opened.where(target_branch: @branch_name).to_a
       merge_requests = merge_requests.select(&:diff_head_commit)

app/views/projects/_last_push.html.haml

@@ -14,5 +14,5 @@
         #{time_ago_with_tooltip(event.created_at)}
 
       .pull-right
-        = link_to new_mr_path_from_push_event(event), title: _("New merge request"), class: "btn btn-info btn-sm" do
+        = link_to new_mr_path_from_push_event(event), title: _("New merge request"), class: "btn btn-info btn-sm qa-create-merge-request" do
           #{ _('Create merge request') }

app/views/shared/form_elements/_description.html.haml

@@ -15,7 +15,7 @@
 
     = render layout: 'projects/md_preview', locals: { url: preview_url, referenced_users: true } do
       = render 'projects/zen', f: form, attr: :description,
-                               classes: 'note-textarea',
+                               classes: 'note-textarea qa-issuable-form-description',
                                placeholder: "Write a comment or drag your files here...",
                                supports_quick_actions: supports_quick_actions
       = render 'shared/notes/hints', supports_quick_actions: supports_quick_actions

app/views/shared/issuable/_form.html.haml

@@ -67,7 +67,7 @@
 
   %span.append-right-10
     - if issuable.new_record?
-      = form.submit "Submit #{issuable.class.model_name.human.downcase}", class: 'btn btn-create'
+      = form.submit "Submit #{issuable.class.model_name.human.downcase}", class: 'btn btn-create qa-issuable-create-button'
     - else
       = form.submit 'Save changes', class: 'btn btn-save'
 

app/views/shared/issuable/form/_title.html.haml

@@ -6,7 +6,7 @@
 
 %div{ class: div_class }
   = form.text_field :title, required: true, maxlength: 255, autofocus: true,
-      autocomplete: 'off', class: 'form-control pad'
+      autocomplete: 'off', class: 'form-control pad qa-issuable-form-title'
 
   - if issuable.respond_to?(:work_in_progress?)
     %p.help-block

changelogs/unreleased/34733-fix-default-avatar-when-gravatar-disabled.yml

+---
+title: Fix default avatar icon missing when Gravatar is disabled
+merge_request: 16681
+author: Felix Geyer
+type: fixed

changelogs/unreleased/osw-fix-lost-diffs-when-source-branch-deleted.yml

+---
+title: Close and do not reload MR diffs when source branch is deleted
+merge_request:
+author:
+type: fixed

doc/raketasks/backup_restore.md

@@ -496,7 +496,7 @@ more of the following options:
 
 - `BACKUP=timestamp_of_backup` - Required if more than one backup exists.
   Read what the [backup timestamp is about](#backup-timestamp).
-- `force=yes` - Do not ask if the authorized_keys file should get regenerated.
+- `force=yes` - Does not ask if the authorized_keys file should get regenerated and assumes 'yes' for warning that database tables will be removed.
 
 ### Restore for installation from source
 

lib/gitlab/git/repository.rb

@@ -468,9 +468,13 @@ module Gitlab
         }
 
         options = default_options.merge(options)
-        options[:limit] ||= 0
         options[:offset] ||= 0
 
+        limit = options[:limit]
+        if limit == 0 || !limit.is_a?(Integer)
+          raise ArgumentError.new("invalid Repository#log limit: #{limit.inspect}")
+        end
+
         gitaly_migrate(:find_commits) do |is_enabled|
           if is_enabled
             gitaly_commit_client.find_commits(options)

qa/qa.rb

@@ -28,6 +28,7 @@ module QA
       autoload :Sandbox, 'qa/factory/resource/sandbox'
       autoload :Group, 'qa/factory/resource/group'
       autoload :Project, 'qa/factory/resource/project'
+      autoload :MergeRequest, 'qa/factory/resource/merge_request'
       autoload :DeployKey, 'qa/factory/resource/deploy_key'
       autoload :SecretVariable, 'qa/factory/resource/secret_variable'
       autoload :Runner, 'qa/factory/resource/runner'
@@ -135,6 +136,10 @@ module QA
       autoload :PersonalAccessTokens, 'qa/page/profile/personal_access_tokens'
     end
 
+    module MergeRequest
+      autoload :New, 'qa/page/merge_request/new'
+    end
+
     module Admin
       autoload :Settings, 'qa/page/admin/settings'
     end

qa/qa/factory/dependency.rb

@@ -16,20 +16,21 @@ module QA
       def build!
         return if overridden?
 
-        Builder.new(@signature).fabricate!.tap do |product|
+        Builder.new(@signature, @factory).fabricate!.tap do |product|
           @factory.public_send("#{@name}=", product)
         end
       end
 
       class Builder
-        def initialize(signature)
+        def initialize(signature, caller_factory)
           @factory = signature.factory
           @block = signature.block
+          @caller_factory = caller_factory
         end
 
         def fabricate!
           @factory.fabricate! do |factory|
-            @block&.call(factory)
+            @block&.call(factory, @caller_factory)
           end
         end
       end

qa/qa/factory/resource/merge_request.rb

+require 'securerandom'
+
+module QA
+  module Factory
+    module Resource
+      class MergeRequest < Factory::Base
+        attr_accessor :title,
+                      :description,
+                      :source_branch,
+                      :target_branch
+
+        dependency Factory::Resource::Project, as: :project do |project|
+          project.name = 'project-with-merge-request'
+        end
+
+        dependency Factory::Repository::Push, as: :target do |push, factory|
+          push.project = factory.project
+          push.branch_name = "master:#{factory.target_branch}"
+        end
+
+        dependency Factory::Repository::Push, as: :source do |push, factory|
+          push.project = factory.project
+          push.branch_name = "#{factory.target_branch}:#{factory.source_branch}"
+          push.file_name = "added_file.txt"
+          push.file_content = "File Added"
+        end
+
+        def initialize
+          @title = 'QA test - merge request'
+          @description = 'This is a test merge request'
+          @source_branch = "qa-test-feature-#{SecureRandom.hex(8)}"
+          @target_branch = "master"
+        end
+
+        def fabricate!
+          project.visit!
+
+          Page::Project::Show.act { new_merge_request }
+
+          Page::MergeRequest::New.perform do |page|
+            page.fill_title(@title)
+            page.fill_description(@description)
+            page.create_merge_request
+          end
+        end
+      end
+    end
+  end
+end

qa/qa/page/base.rb

@@ -42,12 +42,16 @@ module QA
         page.within(selector) { yield } if block_given?
       end
 
+      def find_element(name)
+        find(element_selector_css(name))
+      end
+
       def click_element(name)
         find_element(name).click
       end
 
-      def find_element(name)
-        find(element_selector_css(name))
+      def fill_element(name, content)
+        find_element(name).set(content)
       end
 
       def within_element(name)

qa/qa/page/merge_request/new.rb

+module QA
+  module Page
+    module MergeRequest
+      class New < Page::Base
+        view 'app/views/shared/issuable/_form.html.haml' do
+          element :issuable_create_button
+        end
+
+        view 'app/views/shared/issuable/form/_title.html.haml' do
+          element :issuable_form_title
+        end
+
+        view 'app/views/shared/form_elements/_description.html.haml' do
+          element :issuable_form_description
+        end
+
+        def create_merge_request
+          click_element :issuable_create_button
+        end
+
+        def fill_title(title)
+          fill_element :issuable_form_title, title
+        end
+
+        def fill_description(description)
+          fill_element :issuable_form_description, description
+        end
+      end
+    end
+  end
+end

qa/qa/page/project/show.rb

@@ -9,6 +9,10 @@ module QA
           element :project_repository_location, 'text_field_tag :project_clone'
         end
 
+        view 'app/views/projects/_last_push.html.haml' do
+          element :create_merge_request
+        end
+
         view 'app/views/projects/_home_panel.html.haml' do
           element :project_name
         end
@@ -34,6 +38,10 @@ module QA
           find('.qa-project-name').text
         end
 
+        def new_merge_request
+          click_element :create_merge_request
+        end
+
         def wait_for_push
           sleep 5
           refresh

qa/qa/specs/features/merge_request/create_spec.rb

+module QA
+  feature 'creates a merge request', :core do
+    scenario 'user creates a new merge request'  do
+      Runtime::Browser.visit(:gitlab, Page::Main::Login)
+      Page::Main::Login.act { sign_in_using_credentials }
+
+      Factory::Resource::MergeRequest.fabricate! do |merge_request|
+        merge_request.title = 'This is a merge request'
+        merge_request.description = 'Great feature'
+      end
+
+      expect(page).to have_content('This is a merge request')
+      expect(page).to have_content('Great feature')
+      expect(page).to have_content('Opened less than a minute ago')
+    end
+  end
+end

qa/spec/factory/dependency_spec.rb

@@ -54,6 +54,19 @@ describe QA::Factory::Dependency do
 
         expect(factory).to have_received(:mydep=).with(dependency)
       end
+
+      context 'when receives a caller factory as block argument' do
+        let(:dependency) { QA::Factory::Base }
+
+        it 'calls given block with dependency factory and caller factory' do
+          allow_any_instance_of(QA::Factory::Base).to receive(:fabricate!).and_return(factory)
+          allow(QA::Factory::Product).to receive(:populate!).and_return(spy('any'))
+
+          subject.build!
+
+          expect(block).to have_received(:call).with(an_instance_of(QA::Factory::Base), factory)
+        end
+      end
     end
   end
 end

spec/features/commits_spec.rb

@@ -194,7 +194,7 @@ describe 'Commits' do
     end
 
     it 'includes the committed_date for each commit' do
-      commits = project.repository.commits(branch_name)
+      commits = project.repository.commits(branch_name, limit: 40)
 
       commits.each do |commit|
         expect(page).to have_content("authored #{commit.authored_date.strftime("%b %d, %Y")}")

spec/helpers/application_helper_spec.rb

@@ -100,7 +100,7 @@ describe ApplicationHelper do
       end
 
       it 'returns a generic avatar' do
-        expect(helper.gravatar_icon(user_email)).to match('no_avatar.png')
+        expect(helper.gravatar_icon(user_email)).to match_asset_path('no_avatar.png')
       end
     end
 
@@ -110,7 +110,7 @@ describe ApplicationHelper do
       end
 
       it 'returns a generic avatar when email is blank' do
-        expect(helper.gravatar_icon('')).to match('no_avatar.png')
+        expect(helper.gravatar_icon('')).to match_asset_path('no_avatar.png')
       end
 
       it 'returns a valid Gravatar URL' do

spec/javascripts/vue_mr_widget/components/states/mr_widget_merged_spec.js

 import Vue from 'vue';
-import mergedComponent from '~/vue_merge_request_widget/components/states/mr_widget_merged';
+import mergedComponent from '~/vue_merge_request_widget/components/states/mr_widget_merged.vue';
 import eventHub from '~/vue_merge_request_widget/event_hub';
+import mountComponent from '../../../helpers/vue_mount_component_helper';
 
-const targetBranch = 'foo';
+describe('MRWidgetMerged', () => {
+  let vm;
+  const targetBranch = 'foo';
 
-const createComponent = () => {
+  beforeEach(() => {
     const Component = Vue.extend(mergedComponent);
     const mr = {
       isRemovingSourceBranch: false,
@@ -15,14 +18,19 @@ const createComponent = () => {
       canRemoveSourceBranch: true,
       sourceBranchRemoved: true,
       metrics: {
-      mergedBy: {},
-      mergedAt: 'mergedUpdatedAt',
+        mergedBy: {
+          name: 'Administrator',
+          username: 'root',
+          webUrl: 'http://localhost:3000/root',
+          avatarUrl: 'http://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon',
+        },
+        mergedAt: 'Jan 24, 2018 1:02pm GMT+0000',
         readableMergedAt: '',
         closedBy: {},
-      closedAt: 'mergedUpdatedAt',
+        closedAt: 'Jan 24, 2018 1:02pm GMT+0000',
         readableClosedAt: '',
       },
-    updatedAt: 'mrUpdatedAt',
+      updatedAt: 'mergedUpdatedAt',
       targetBranch,
     };
 
@@ -30,79 +38,62 @@ const createComponent = () => {
       removeSourceBranch() {},
     };
 
-  return new Component({
-    el: document.createElement('div'),
-    propsData: { mr, service },
-  });
-};
-
-describe('MRWidgetMerged', () => {
-  describe('props', () => {
-    it('should have props', () => {
-      const { mr, service } = mergedComponent.props;
-
-      expect(mr.type instanceof Object).toBeTruthy();
-      expect(mr.required).toBeTruthy();
-
-      expect(service.type instanceof Object).toBeTruthy();
-      expect(service.required).toBeTruthy();
-    });
-  });
+    spyOn(eventHub, '$emit');
 
-  describe('components', () => {
-    it('should have components added', () => {
-      expect(mergedComponent.components['mr-widget-author-and-time']).toBeDefined();
-    });
+    vm = mountComponent(Component, { mr, service });
   });
 
-  describe('data', () => {
-    it('should have default data', () => {
-      const data = mergedComponent.data();
-
-      expect(data.isMakingRequest).toBeFalsy();
-    });
+  afterEach(() => {
+    vm.$destroy();
   });
 
   describe('computed', () => {
     describe('shouldShowRemoveSourceBranch', () => {
-      it('should correct value when fields changed', () => {
-        const vm = createComponent();
+      it('returns true when sourceBranchRemoved is false', () => {
         vm.mr.sourceBranchRemoved = false;
-        expect(vm.shouldShowRemoveSourceBranch).toBeTruthy();
+        expect(vm.shouldShowRemoveSourceBranch).toEqual(true);
+      });
 
+      it('returns false wehn sourceBranchRemoved is true', () => {
         vm.mr.sourceBranchRemoved = true;
-        expect(vm.shouldShowRemoveSourceBranch).toBeFalsy();
+        expect(vm.shouldShowRemoveSourceBranch).toEqual(false);
+      });
 
+      it('returns false when canRemoveSourceBranch is false', () => {
         vm.mr.sourceBranchRemoved = false;
         vm.mr.canRemoveSourceBranch = false;
-        expect(vm.shouldShowRemoveSourceBranch).toBeFalsy();
+        expect(vm.shouldShowRemoveSourceBranch).toEqual(false);
+      });
 
+      it('returns false when is making request', () => {
         vm.mr.canRemoveSourceBranch = true;
         vm.isMakingRequest = true;
-        expect(vm.shouldShowRemoveSourceBranch).toBeFalsy();
+        expect(vm.shouldShowRemoveSourceBranch).toEqual(false);
+      });
 
+      it('returns true when all are true', () => {
         vm.mr.isRemovingSourceBranch = true;
         vm.mr.canRemoveSourceBranch = true;
         vm.isMakingRequest = true;
-        expect(vm.shouldShowRemoveSourceBranch).toBeFalsy();
+        expect(vm.shouldShowRemoveSourceBranch).toEqual(false);
       });
     });
+
     describe('shouldShowSourceBranchRemoving', () => {
       it('should correct value when fields changed', () => {
-        const vm = createComponent();
         vm.mr.sourceBranchRemoved = false;
-        expect(vm.shouldShowSourceBranchRemoving).toBeFalsy();
+        expect(vm.shouldShowSourceBranchRemoving).toEqual(false);
 
         vm.mr.sourceBranchRemoved = true;
-        expect(vm.shouldShowRemoveSourceBranch).toBeFalsy();
+        expect(vm.shouldShowRemoveSourceBranch).toEqual(false);
 
         vm.mr.sourceBranchRemoved = false;
         vm.isMakingRequest = true;
-        expect(vm.shouldShowSourceBranchRemoving).toBeTruthy();
+        expect(vm.shouldShowSourceBranchRemoving).toEqual(true);
 
         vm.isMakingRequest = false;
         vm.mr.isRemovingSourceBranch = true;
-        expect(vm.shouldShowSourceBranchRemoving).toBeTruthy();
+        expect(vm.shouldShowSourceBranchRemoving).toEqual(true);
       });
     });
   });
@@ -110,8 +101,6 @@ describe('MRWidgetMerged', () => {
   describe('methods', () => {
     describe('removeSourceBranch', () => {
       it('should set flag and call service then request main component to update the widget', (done) => {
-        const vm = createComponent();
-        spyOn(eventHub, '$emit');
         spyOn(vm.service, 'removeSourceBranch').and.returnValue(new Promise((resolve) => {
           resolve({
             data: {
@@ -123,7 +112,7 @@ describe('MRWidgetMerged', () => {
         vm.removeSourceBranch();
         setTimeout(() => {
           const args = eventHub.$emit.calls.argsFor(0);
-          expect(vm.isMakingRequest).toBeTruthy();
+          expect(vm.isMakingRequest).toEqual(true);
           expect(args[0]).toEqual('MRWidgetUpdateRequested');
           expect(args[1]).not.toThrow();
           done();
@@ -132,33 +121,31 @@ describe('MRWidgetMerged', () => {
     });
   });
 
-  describe('template', () => {
-    let vm;
-    let el;
+  it('has merged by information', () => {
+    expect(vm.$el.textContent).toContain('Merged by');
+    expect(vm.$el.textContent).toContain('Administrator');
+  });
 
-    beforeEach(() => {
-      vm = createComponent();
-      el = vm.$el;
+  it('renders branch information', () => {
+    expect(vm.$el.textContent).toContain('The changes were merged into');
+    expect(vm.$el.textContent).toContain(targetBranch);
   });
 
-    it('should have correct elements', () => {
-      expect(el.classList.contains('mr-widget-body')).toBeTruthy();
-      expect(el.querySelector('.js-mr-widget-author')).toBeDefined();
-      expect(el.innerText).toContain('The changes were merged into');
-      expect(el.innerText).toContain(targetBranch);
-      expect(el.innerText).toContain('The source branch has been removed');
-      expect(el.innerText).toContain('Revert');
-      expect(el.innerText).toContain('Cherry-pick');
-      expect(el.innerText).not.toContain('You can remove source branch now');
-      expect(el.innerText).not.toContain('The source branch is being removed');
+  it('renders information about branch being removed', () => {
+    expect(vm.$el.textContent).toContain('The source branch has been removed');
+  });
+
+  it('shows revert and cherry-pick buttons', () => {
+    expect(vm.$el.textContent).toContain('Revert');
+    expect(vm.$el.textContent).toContain('Cherry-pick');
   });
 
   it('should not show source branch removed text', (done) => {
     vm.mr.sourceBranchRemoved = false;
 
     Vue.nextTick(() => {
-        expect(el.innerText).toContain('You can remove source branch now');
-        expect(el.innerText).not.toContain('The source branch has been removed');
+      expect(vm.$el.innerText).toContain('You can remove source branch now');
+      expect(vm.$el.innerText).not.toContain('The source branch has been removed');
       done();
     });
   });
@@ -168,17 +155,16 @@ describe('MRWidgetMerged', () => {
     vm.mr.sourceBranchRemoved = false;
 
     Vue.nextTick(() => {
-        expect(el.innerText).toContain('The source branch is being removed');
-        expect(el.innerText).not.toContain('You can remove source branch now');
-        expect(el.innerText).not.toContain('The source branch has been removed');
+      expect(vm.$el.innerText).toContain('The source branch is being removed');
+      expect(vm.$el.innerText).not.toContain('You can remove source branch now');
+      expect(vm.$el.innerText).not.toContain('The source branch has been removed');
       done();
     });
   });
 
-    it('should use mergedEvent updatedAt as tooltip title', () => {
+  it('should use mergedEvent mergedAt as tooltip title', () => {
     expect(
-        el.querySelector('time').getAttribute('title'),
-      ).toBe('mergedUpdatedAt');
-    });
+      vm.$el.querySelector('time').getAttribute('title'),
+    ).toBe('Jan 24, 2018 1:02pm GMT+0000');
   });
 });

spec/lib/gitlab/git/repository_spec.rb

@@ -981,6 +981,16 @@ describe Gitlab::Git::Repository, seed_helper: true do
         end
       end
     end
+
+    context 'limit validation' do
+      where(:limit) do
+        [0, nil, '', 'foo']
+      end
+
+      with_them do
+        it { expect { repository.log(limit: limit) }.to raise_error(ArgumentError) }
+      end
+    end
   end
 
   describe "#rugged_commits_between" do

spec/models/merge_request_spec.rb

@@ -1768,7 +1768,7 @@ describe MergeRequest do
       expect { subject.reload_diff }.to change { subject.merge_request_diffs.count }.by(1)
     end
 
-    it "executs diff cache service" do
+    it "executes diff cache service" do
       expect_any_instance_of(MergeRequests::MergeRequestDiffCacheService).to receive(:execute).with(subject)
 
       subject.reload_diff

spec/models/repository_spec.rb

@@ -222,20 +222,20 @@ describe Repository do
     it 'sets follow when path is a single path' do
       expect(Gitlab::Git::Commit).to receive(:where).with(a_hash_including(follow: true)).and_call_original.twice
 
-      repository.commits('master', path: 'README.md')
-      repository.commits('master', path: ['README.md'])
+      repository.commits('master', limit: 1, path: 'README.md')
+      repository.commits('master', limit: 1, path: ['README.md'])
     end
 
     it 'does not set follow when path is multiple paths' do
       expect(Gitlab::Git::Commit).to receive(:where).with(a_hash_including(follow: false)).and_call_original
 
-      repository.commits('master', path: ['README.md', 'CHANGELOG'])
+      repository.commits('master', limit: 1, path: ['README.md', 'CHANGELOG'])
     end
 
     it 'does not set follow when there are no paths' do
       expect(Gitlab::Git::Commit).to receive(:where).with(a_hash_including(follow: false)).and_call_original
 
-      repository.commits('master')
+      repository.commits('master', limit: 1)
     end
   end
 
@@ -455,7 +455,7 @@ describe Repository do
       expect do
         repository.create_dir(user, 'newdir',
           message: 'Create newdir', branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       newdir = repository.tree('master', 'newdir')
       expect(newdir.path).to eq('newdir')
@@ -469,7 +469,7 @@ describe Repository do
           repository.create_dir(user, 'newdir',
             message: 'Create newdir', branch_name: 'patch',
             start_branch_name: 'master', start_project: forked_project)
-        end.to change { repository.commits('master').count }.by(0)
+        end.to change { repository.count_commits(ref: 'master') }.by(0)
 
         expect(repository.branch_exists?('patch')).to be_truthy
         expect(forked_project.repository.branch_exists?('patch')).to be_falsy
@@ -486,7 +486,7 @@ describe Repository do
             message: 'Add newdir',
             branch_name: 'master',
             author_email: author_email, author_name: author_name)
-        end.to change { repository.commits('master').count }.by(1)
+        end.to change { repository.count_commits(ref: 'master') }.by(1)
 
         last_commit = repository.commit
 
@@ -502,7 +502,7 @@ describe Repository do
         repository.create_file(user, 'NEWCHANGELOG', 'Changelog!',
                                message: 'Create changelog',
                                branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       blob = repository.blob_at('master', 'NEWCHANGELOG')
 
@@ -514,7 +514,7 @@ describe Repository do
         repository.create_file(user, 'new_dir/new_file.txt', 'File!',
                                message: 'Create new_file with new_dir',
                                branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       expect(repository.tree('master', 'new_dir').path).to eq('new_dir')
       expect(repository.blob_at('master', 'new_dir/new_file.txt').data).to eq('File!')
@@ -538,7 +538,7 @@ describe Repository do
                                  branch_name: 'master',
                                  author_email: author_email,
                                  author_name: author_name)
-        end.to change { repository.commits('master').count }.by(1)
+        end.to change { repository.count_commits(ref: 'master') }.by(1)
 
         last_commit = repository.commit
 
@@ -554,7 +554,7 @@ describe Repository do
         repository.update_file(user, 'CHANGELOG', 'Changelog!',
                                message: 'Update changelog',
                                branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       blob = repository.blob_at('master', 'CHANGELOG')
 
@@ -567,7 +567,7 @@ describe Repository do
                                      branch_name: 'master',
                                      previous_path: 'LICENSE',
                                      message: 'Changes filename')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       files = repository.ls_files('master')
 
@@ -584,7 +584,7 @@ describe Repository do
                                  message: 'Update README',
                                  author_email: author_email,
                                  author_name: author_name)
-        end.to change { repository.commits('master').count }.by(1)
+        end.to change { repository.count_commits(ref: 'master') }.by(1)
 
         last_commit = repository.commit
 
@@ -599,7 +599,7 @@ describe Repository do
       expect do
         repository.delete_file(user, 'README',
           message: 'Remove README', branch_name: 'master')
-      end.to change { repository.commits('master').count }.by(1)
+      end.to change { repository.count_commits(ref: 'master') }.by(1)
 
       expect(repository.blob_at('master', 'README')).to be_nil
     end
@@ -610,7 +610,7 @@ describe Repository do
           repository.delete_file(user, 'README',
             message: 'Remove README', branch_name: 'master',
             author_email: author_email, author_name: author_name)
-        end.to change { repository.commits('master').count }.by(1)
+        end.to change { repository.count_commits(ref: 'master') }.by(1)
 
         last_commit = repository.commit
 

spec/requests/api/commits_spec.rb

@@ -62,7 +62,7 @@ describe API::Commits do
 
       context "since optional parameter" do
         it "returns project commits since provided parameter" do
-          commits = project.repository.commits("master")
+          commits = project.repository.commits("master", limit: 2)
           after = commits.second.created_at
 
           get api("/projects/#{project_id}/repository/commits?since=#{after.utc.iso8601}", user)
@@ -73,7 +73,7 @@ describe API::Commits do
         end
 
         it 'include correct pagination headers' do
-          commits = project.repository.commits("master")
+          commits = project.repository.commits("master", limit: 2)
           after = commits.second.created_at
           commit_count = project.repository.count_commits(ref: 'master', after: after).to_s
 
@@ -87,12 +87,12 @@ describe API::Commits do
 
       context "until optional parameter" do
         it "returns project commits until provided parameter" do
-          commits = project.repository.commits("master")
+          commits = project.repository.commits("master", limit: 20)
           before = commits.second.created_at
 
           get api("/projects/#{project_id}/repository/commits?until=#{before.utc.iso8601}", user)
 
-          if commits.size >= 20
+          if commits.size == 20
             expect(json_response.size).to eq(20)
           else
             expect(json_response.size).to eq(commits.size - 1)
@@ -103,7 +103,7 @@ describe API::Commits do
         end
 
         it 'include correct pagination headers' do
-          commits = project.repository.commits("master")
+          commits = project.repository.commits("master", limit: 2)
           before = commits.second.created_at
           commit_count = project.repository.count_commits(ref: 'master', before: before).to_s
 
@@ -181,7 +181,7 @@ describe API::Commits do
           let(:page) { 3 }
 
           it 'returns the third 5 commits' do
-            commit = project.repository.commits('HEAD', offset: (page - 1) * per_page).first
+            commit = project.repository.commits('HEAD', limit: per_page, offset: (page - 1) * per_page).first
 
             expect(json_response.size).to eq(per_page)
             expect(json_response.first['id']).to eq(commit.id)

spec/requests/api/v3/commits_spec.rb

@@ -36,7 +36,7 @@ describe API::V3::Commits do
 
     context "since optional parameter" do
       it "returns project commits since provided parameter" do
-        commits = project.repository.commits("master")
+        commits = project.repository.commits("master", limit: 2)
         since = commits.second.created_at
 
         get v3_api("/projects/#{project.id}/repository/commits?since=#{since.utc.iso8601}", user)
@@ -49,12 +49,12 @@ describe API::V3::Commits do
 
     context "until optional parameter" do
       it "returns project commits until provided parameter" do
-        commits = project.repository.commits("master")
+        commits = project.repository.commits("master", limit: 20)
         before = commits.second.created_at
 
         get v3_api("/projects/#{project.id}/repository/commits?until=#{before.utc.iso8601}", user)
 
-        if commits.size >= 20
+        if commits.size == 20
           expect(json_response.size).to eq(20)
         else
           expect(json_response.size).to eq(commits.size - 1)

spec/services/merge_requests/refresh_service_spec.rb

@@ -58,11 +58,12 @@ describe MergeRequests::RefreshService do
 
       before do
         allow(refresh_service).to receive(:execute_hooks)
-        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
-        reload_mrs
       end
 
       it 'executes hooks with update action' do
+        refresh_service.execute(@oldrev, @newrev, 'refs/heads/master')
+        reload_mrs
+
         expect(refresh_service).to have_received(:execute_hooks)
           .with(@merge_request, 'update', old_rev: @oldrev)
 
@@ -78,6 +79,26 @@ describe MergeRequests::RefreshService do
         expect(@merge_request.approvals).to be_empty
         expect(@fork_merge_request.approvals).not_to be_empty
       end
+
+      context 'when source branch ref does not exists' do
+        before do
+          DeleteBranchService.new(@project, @user).execute(@merge_request.source_branch)
+        end
+
+        it 'closes MRs without source branch ref' do
+          expect { refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') }
+            .to change { @merge_request.reload.state }
+            .from('opened')
+            .to('closed')
+
+          expect(@fork_merge_request.reload).to be_open
+        end
+
+        it 'does not change the merge request diff' do
+          expect { refresh_service.execute(@oldrev, @newrev, 'refs/heads/master') }
+            .not_to change { @merge_request.reload.merge_request_diff }
+        end
+      end
     end
 
     context 'when pipeline exists for the source branch' do