Merge branch 'master' into 29326-update-documentation

* master: Using guard clause and added more specs Changelog Fix GitHub Import for open PRs from a fork Add frequently used emojis back to awards menu
2017-03-10 23:11:50 +00:00 · 2017-03-10 23:11:50 +00:00 · 6b1b616ea9
parent e8d525d07c e78aed49cd
commit 6b1b616ea9
8 changed files with 166 additions and 56 deletions
--- a/app/assets/javascripts/awards_handler.js
+++ b/app/assets/javascripts/awards_handler.js
@ -45,12 +45,12 @@ function buildCategoryMap() {
  });
 }

-function renderCategory(name, emojiList) {
+function renderCategory(name, emojiList, opts = {}) {
  return `
    <h5 class="emoji-menu-title">
      ${name}
    </h5>
-    <ul class="clearfix emoji-menu-list">
+    <ul class="clearfix emoji-menu-list ${opts.menuListClass}">
      ${emojiList.map(emojiName => `
        <li class="emoji-menu-list-item">
          <button class="emoji-menu-btn text-center js-emoji-btn" type="button">
@ -140,9 +140,6 @@ AwardsHandler.prototype.showEmojiMenu = function showEmojiMenu($addBtn) {
      const $createdMenu = $('.emoji-menu');
      $addBtn.removeClass('is-loading');
      this.positionMenu($createdMenu, $addBtn);
-      if (!this.frequentEmojiBlockRendered) {
-        this.renderFrequentlyUsedBlock();
-      }
      return setTimeout(() => {
        $createdMenu.addClass('is-visible');
        $('#emoji_search').focus();
@ -165,11 +162,21 @@ AwardsHandler.prototype.createEmojiMenu = function createEmojiMenu(callback) {
  const emojisInCategory = categoryMap[categoryNameKey];
  const firstCategory = renderCategory(categoryLabelMap[categoryNameKey], emojisInCategory);

+  // Render the frequently used
+  const frequentlyUsedEmojis = this.getFrequentlyUsedEmojis();
+  let frequentlyUsedCatgegory = '';
+  if (frequentlyUsedEmojis.length > 0) {
+    frequentlyUsedCatgegory = renderCategory('Frequently used', frequentlyUsedEmojis, {
+      menuListClass: 'frequent-emojis',
+    });
+  }
+
  const emojiMenuMarkup = `
    <div class="emoji-menu">
      <input type="text" name="emoji_search" id="emoji_search" value="" class="emoji-search search-input form-control" placeholder="Search emoji" />

      <div class="emoji-menu-content">
+        ${frequentlyUsedCatgegory}
        ${firstCategory}
      </div>
    </div>
@ -457,19 +464,6 @@ AwardsHandler.prototype.getFrequentlyUsedEmojis = function getFrequentlyUsedEmoj
  return _.compact(_.uniq(frequentlyUsedEmojis));
 };

-AwardsHandler.prototype.renderFrequentlyUsedBlock = function renderFrequentlyUsedBlock() {
-  if (Cookies.get('frequently_used_emojis')) {
-    const frequentlyUsedEmojis = this.getFrequentlyUsedEmojis();
-    const ul = $('<ul class="clearfix emoji-menu-list frequent-emojis">');
-    for (let i = 0, len = frequentlyUsedEmojis.length; i < len; i += 1) {
-      const emoji = frequentlyUsedEmojis[i];
-      $(`.emoji-menu-content [data-name="${emoji}"]`).closest('li').clone().appendTo(ul);
-    }
-    $('.emoji-menu-content').prepend(ul).prepend($('<h5>').text('Frequently used'));
-  }
-  this.frequentEmojiBlockRendered = true;
-};
-
 AwardsHandler.prototype.setupSearch = function setupSearch() {
  this.registerEventListener('on', $('input.emoji-search'), 'input', (e) => {
    const term = $(e.target).val().trim();
--- a/changelogs/unreleased/29046-fix-github-importer-open-prs.yml
+++ b/changelogs/unreleased/29046-fix-github-importer-open-prs.yml
@ -0,0 +1,4 @@
+---
+title: Fix GitHub Import deleting branches for open PRs from a fork
+merge_request: 9758
+author:
--- a/changelogs/unreleased/add-frequently-used-emojis-back-to-menu.yml
+++ b/changelogs/unreleased/add-frequently-used-emojis-back-to-menu.yml
@ -0,0 +1,4 @@
+---
+title: Add frequently used emojis back to awards menu
+merge_request:
+author:
--- a/lib/gitlab/github_import/importer.rb
+++ b/lib/gitlab/github_import/importer.rb
@ -171,6 +171,8 @@ module Gitlab
      end

      def clean_up_restored_branches(pull_request)
+        return if pull_request.opened?
+
        remove_branch(pull_request.source_branch_name) unless pull_request.source_branch_exists?
        remove_branch(pull_request.target_branch_name) unless pull_request.target_branch_exists?
      end
--- a/lib/gitlab/github_import/pull_request_formatter.rb
+++ b/lib/gitlab/github_import/pull_request_formatter.rb
@ -60,6 +60,10 @@ module Gitlab
        source_branch.repo.id != target_branch.repo.id
      end

+      def opened?
+        state == 'opened'
+      end
+
      private

      def state
--- a/spec/javascripts/awards_handler_spec.js
+++ b/spec/javascripts/awards_handler_spec.js
@ -1,6 +1,7 @@
 /* eslint-disable space-before-function-paren, no-var, one-var, one-var-declaration-per-line, no-unused-expressions, comma-dangle, new-parens, no-unused-vars, quotes, jasmine/no-spec-dupes, prefer-template, max-len */

 import promisePolyfill from 'es6-promise';
+import Cookies from 'js-cookie';
 import AwardsHandler from '~/awards_handler';

 promisePolyfill.polyfill();
@ -208,8 +209,8 @@ promisePolyfill.polyfill();
            expect($('[data-name=alien]').is(':visible')).toBe(true);
          })
          .then(done)
-          .catch(() => {
-            done.fail('Failed to open and build emoji menu');
+          .catch((err) => {
+            done.fail(`Failed to open and build emoji menu: ${err.message}`);
          });
      });
    });
@ -232,8 +233,8 @@ promisePolyfill.polyfill();
      it('should add selected emoji to awards block', function(done) {
        return openEmojiMenuAndAddEmoji()
          .then(done)
-          .catch(() => {
-            done.fail('Failed to open and build emoji menu');
+          .catch((err) => {
+            done.fail(`Failed to open and build emoji menu: ${err.message}`);
          });
      });
      it('should remove already selected emoji', function(done) {
@ -247,7 +248,46 @@ promisePolyfill.polyfill();
          })
          .then(done)
          .catch((err) => {
-            done.fail('Failed to open and build emoji menu');
+            done.fail(`Failed to open and build emoji menu: ${err.message}`);
+          });
+      });
+    });
+
+    describe('frequently used emojis', function() {
+      beforeEach(() => {
+        // Clear it out
+        Cookies.set('frequently_used_emojis', '');
+      });
+
+      it('shouldn\'t have any "Frequently used" heading if no frequently used emojis', function(done) {
+        return openAndWaitForEmojiMenu()
+          .then(() => {
+            const emojiMenu = document.querySelector('.emoji-menu');
+            Array.prototype.forEach.call(emojiMenu.querySelectorAll('.emoji-menu-title'), (title) => {
+              expect(title.textContent.trim().toLowerCase()).not.toBe('frequently used');
+            });
+          })
+          .then(done)
+          .catch((err) => {
+            done.fail(`Failed to open and build emoji menu: ${err.message}`);
+          });
+      });
+
+      it('should have any frequently used section when there are frequently used emojis', function(done) {
+        awardsHandler.addEmojiToFrequentlyUsedList('8ball');
+
+        return openAndWaitForEmojiMenu()
+          .then(() => {
+            const emojiMenu = document.querySelector('.emoji-menu');
+            const hasFrequentlyUsedHeading = Array.prototype.some.call(emojiMenu.querySelectorAll('.emoji-menu-title'), title =>
+              title.textContent.trim().toLowerCase() === 'frequently used'
+            );
+
+            expect(hasFrequentlyUsedHeading).toBe(true);
+          })
+          .then(done)
+          .catch((err) => {
+            done.fail(`Failed to open and build emoji menu: ${err.message}`);
          });
      });
    });
--- a/spec/lib/gitlab/github_import/importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer_spec.rb
@ -55,9 +55,6 @@ describe Gitlab::GithubImport::Importer, lib: true do
      allow_any_instance_of(Octokit::Client).to receive(:releases).and_return([release1, release2])
    end

-    let(:octocat) { double(id: 123456, login: 'octocat', email: 'octocat@example.com') }
-    let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
-    let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
    let(:label1) do
      double(
        name: 'Bug',
@ -127,32 +124,6 @@ describe Gitlab::GithubImport::Importer, lib: true do
      )
    end

-    let!(:user) { create(:user, email: octocat.email) }
-    let(:repository) { double(id: 1, fork: false) }
-    let(:source_sha) { create(:commit, project: project).id }
-    let(:source_branch) { double(ref: 'branch-merged', repo: repository, sha: source_sha) }
-    let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id }
-    let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha) }
-    let(:pull_request) do
-      double(
-        number: 1347,
-        milestone: nil,
-        state: 'open',
-        title: 'New feature',
-        body: 'Please pull these awesome changes',
-        head: source_branch,
-        base: target_branch,
-        assignee: nil,
-        user: octocat,
-        created_at: created_at,
-        updated_at: updated_at,
-        closed_at: nil,
-        merged_at: nil,
-        url: "#{api_root}/repos/octocat/Hello-World/pulls/1347",
-        labels: [double(name: 'Label #2')]
-      )
-    end
-
    let(:release1) do
      double(
        tag_name: 'v1.0.0',
@ -177,12 +148,14 @@ describe Gitlab::GithubImport::Importer, lib: true do
      )
    end

+    subject { described_class.new(project) }
+
    it 'returns true' do
-      expect(described_class.new(project).execute).to eq true
+      expect(subject.execute).to eq true
    end

    it 'does not raise an error' do
-      expect { described_class.new(project).execute }.not_to raise_error
+      expect { subject.execute }.not_to raise_error
    end

    it 'stores error messages' do
@ -205,15 +178,93 @@ describe Gitlab::GithubImport::Importer, lib: true do
    end
  end

+  shared_examples 'Gitlab::GithubImport unit-testing' do
+    describe '#clean_up_restored_branches' do
+      subject { described_class.new(project) }
+
+      before do
+        allow(gh_pull_request).to receive(:source_branch_exists?).at_least(:once) { false }
+        allow(gh_pull_request).to receive(:target_branch_exists?).at_least(:once) { false }
+      end
+
+      context 'when pull request stills open' do
+        let(:gh_pull_request) { Gitlab::GithubImport::PullRequestFormatter.new(project, pull_request) }
+
+        it 'does not remove branches' do
+          expect(subject).not_to receive(:remove_branch)
+          subject.send(:clean_up_restored_branches, gh_pull_request)
+        end
+      end
+
+      context 'when pull request is closed' do
+        let(:gh_pull_request) { Gitlab::GithubImport::PullRequestFormatter.new(project, closed_pull_request) }
+
+        it 'does remove branches' do
+          expect(subject).to receive(:remove_branch).at_least(2).times
+          subject.send(:clean_up_restored_branches, gh_pull_request)
+        end
+      end
+    end
+  end
+
  let(:project) { create(:project, :wiki_disabled, import_url: "#{repo_root}/octocat/Hello-World.git") }
+  let(:octocat) { double(id: 123456, login: 'octocat', email: 'octocat@example.com') }
  let(:credentials) { { user: 'joe' } }

+  let(:created_at) { DateTime.strptime('2011-01-26T19:01:12Z') }
+  let(:updated_at) { DateTime.strptime('2011-01-27T19:01:12Z') }
+  let(:repository) { double(id: 1, fork: false) }
+  let(:source_sha) { create(:commit, project: project).id }
+  let(:source_branch) { double(ref: 'branch-merged', repo: repository, sha: source_sha) }
+  let(:target_sha) { create(:commit, project: project, git_commit: RepoHelpers.another_sample_commit).id }
+  let(:target_branch) { double(ref: 'master', repo: repository, sha: target_sha) }
+  let(:pull_request) do
+    double(
+      number: 1347,
+      milestone: nil,
+      state: 'open',
+      title: 'New feature',
+      body: 'Please pull these awesome changes',
+      head: source_branch,
+      base: target_branch,
+      assignee: nil,
+      user: octocat,
+      created_at: created_at,
+      updated_at: updated_at,
+      closed_at: nil,
+      merged_at: nil,
+      url: "#{api_root}/repos/octocat/Hello-World/pulls/1347",
+      labels: [double(name: 'Label #2')]
+    )
+  end
+  let(:closed_pull_request) do
+    double(
+      number: 1347,
+      milestone: nil,
+      state: 'closed',
+      title: 'New feature',
+      body: 'Please pull these awesome changes',
+      head: source_branch,
+      base: target_branch,
+      assignee: nil,
+      user: octocat,
+      created_at: created_at,
+      updated_at: updated_at,
+      closed_at: updated_at,
+      merged_at: nil,
+      url: "#{api_root}/repos/octocat/Hello-World/pulls/1347",
+      labels: [double(name: 'Label #2')]
+    )
+  end
+
  context 'when importing a GitHub project' do
    let(:api_root) { 'https://api.github.com' }
    let(:repo_root) { 'https://github.com' }
+    subject { described_class.new(project) }

    it_behaves_like 'Gitlab::GithubImport::Importer#execute'
    it_behaves_like 'Gitlab::GithubImport::Importer#execute an error occurs'
+    it_behaves_like 'Gitlab::GithubImport unit-testing'

    describe '#client' do
      it 'instantiates a Client' do
@ -223,7 +274,7 @@ describe Gitlab::GithubImport::Importer, lib: true do
          {}
        )

-        described_class.new(project).client
+        subject.client
      end
    end
  end
@ -231,6 +282,8 @@ describe Gitlab::GithubImport::Importer, lib: true do
  context 'when importing a Gitea project' do
    let(:api_root) { 'https://try.gitea.io/api/v1' }
    let(:repo_root) { 'https://try.gitea.io' }
+    subject { described_class.new(project) }
+
    before do
      project.update(import_type: 'gitea', import_url: "#{repo_root}/foo/group/project.git")
    end
@ -239,6 +292,7 @@ describe Gitlab::GithubImport::Importer, lib: true do
      let(:expected_not_called) { [:import_releases] }
    end
    it_behaves_like 'Gitlab::GithubImport::Importer#execute an error occurs'
+    it_behaves_like 'Gitlab::GithubImport unit-testing'

    describe '#client' do
      it 'instantiates a Client' do
@ -248,7 +302,7 @@ describe Gitlab::GithubImport::Importer, lib: true do
          { host: "#{repo_root}:443/foo", api_version: 'v1' }
        )

-        described_class.new(project).client
+        subject.client
      end
    end
  end
--- a/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
+++ b/spec/lib/gitlab/github_import/pull_request_formatter_spec.rb
@ -306,4 +306,12 @@ describe Gitlab::GithubImport::PullRequestFormatter, lib: true do
      expect(pull_request.url).to eq 'https://api.github.com/repos/octocat/Hello-World/pulls/1347'
    end
  end
+
+  describe '#opened?' do
+    let(:raw_data) { double(base_data.merge(state: 'open')) }
+
+    it 'returns true when state is "open"' do
+      expect(pull_request.opened?).to be_truthy
+    end
+  end
 end