Merge branch 'ce-to-ee-2018-02-19' into 'master'

CE upstream - 2018-02-19 18:23 UTC

Closes #4984, #4864, gitlab-qa#190, and gitlab-ce#40854

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

app/assets/stylesheets/framework/stacked_progress_bar.scss

@@ -10,7 +10,7 @@
   .status-neutral,
   .status-red, {
     height: 100%;
-    min-width: 25px;
+    min-width: 30px;
     padding: 0 5px;
     font-size: $tooltip-font-size;
     font-weight: normal;

app/mailers/emails/members.rb

@@ -7,18 +7,11 @@ module Emails
       helper_method :member_source, :member
     end
 
-    def member_access_requested_email(member_source_type, member_id)
+    def member_access_requested_email(member_source_type, member_id, recipient_notification_email)
       @member_source_type = member_source_type
       @member_id = member_id
 
-      admins = member_source.members.owners_and_masters.pluck(:notification_email)
-      # A project in a group can have no explicit owners/masters, in that case
-      # we fallbacks to the group's owners/masters.
-      if admins.empty? && member_source.respond_to?(:group) && member_source.group
-        admins = member_source.group.members.owners_and_masters.pluck(:notification_email)
-      end
-
-      mail(to: admins,
+      mail(to: recipient_notification_email,
            subject: subject("Request to join the #{member_source.human_name} #{member_source.model_name.singular}"))
     end
 

app/services/notification_service.rb

@@ -231,7 +231,12 @@ class NotificationService
   def new_access_request(member)
     return true unless member.notifiable?(:subscription)
 
-    mailer.member_access_requested_email(member.real_source_type, member.id).deliver_later
+    recipients = member.source.members.owners_and_masters
+    if fallback_to_group_owners_masters?(recipients, member)
+      recipients = member.source.group.members.owners_and_masters
+    end
+
+    recipients.each { |recipient| deliver_access_request_email(recipient, member) }
   end
 
   def decline_access_request(member)
@@ -482,4 +487,14 @@ class NotificationService
   def notifiable_users(*args)
     NotificationRecipientService.notifiable_users(*args)
   end
+
+  def deliver_access_request_email(recipient, member)
+    mailer.member_access_requested_email(member.real_source_type, member.id, recipient.user.notification_email).deliver_later
+  end
+
+  def fallback_to_group_owners_masters?(recipients, member)
+    return false if recipients.present?
+
+    member.source.respond_to?(:group) && member.source.group
+  end
 end

changelogs/unreleased/42274-group-request-membership-long-too.yml

+---
+title: Fix long list of recipients on group request membership email
+merge_request: 17121
+author: Jacopo Beschi @jacopo-beschi
+type: fixed

changelogs/unreleased/4826-github-import-wiki-fix-1.yml

+---
+title: "[GitHub Import] Create an empty wiki if wiki import failed"
+merge_request:
+author:
+type: fixed

changelogs/unreleased/flipper-caching.yml

+---
+title: Increase feature flag cache TTL to one hour
+merge_request:
+author:
+type: performance

changelogs/unreleased/kp-fix-stacked-bar-progress-value-clipping.yml

+---
+title: Fix single digit value clipping for stacked progress bar
+merge_request: 17217
+author:
+type: fixed

config/initializers/flipper.rb

@@ -8,7 +8,7 @@ Flipper.configure do |config|
     cached_adapter = Flipper::Adapters::ActiveSupportCacheStore.new(
       adapter,
       Rails.cache,
-      expires_in: 10.seconds)
+      expires_in: 1.hour)
 
     Flipper.new(cached_adapter)
   end

lib/gitlab/github_import/importer/repository_importer.rb

@@ -63,6 +63,7 @@ module Gitlab
           true
         rescue Gitlab::Shell::Error => e
           if e.message !~ /repository not exported/
+            project.create_wiki
             fail_import("Failed to import the wiki: #{e.message}")
           else
             true

qa/qa/factory/resource/secret_variable.rb

@@ -4,18 +4,6 @@ module QA
       class SecretVariable < Factory::Base
         attr_accessor :key, :value
 
-        product :key do
-          Page::Project::Settings::CICD.act do
-            expand_secret_variables(&:variable_key)
-          end
-        end
-
-        product :value do
-          Page::Project::Settings::CICD.act do
-            expand_secret_variables(&:variable_value)
-          end
-        end
-
         dependency Factory::Resource::Project, as: :project do |project|
           project.name = 'project-with-secret-variables'
           project.description = 'project for adding secret variable test'

qa/qa/page/base.rb

@@ -98,6 +98,10 @@ module QA
         views.map(&:errors).flatten
       end
 
+      def self.elements
+        views.map(&:elements).flatten
+      end
+
       class DSL
         attr_reader :views
 

qa/qa/page/project/settings/secret_variables.rb

@@ -6,39 +6,37 @@ module QA
           include Common
 
           view 'app/views/ci/variables/_variable_row.html.haml' do
+            element :variable_row, '.ci-variable-row-body'
             element :variable_key, '.js-ci-variable-input-key'
             element :variable_value, '.js-ci-variable-input-value'
+            element :key_placeholder, 'Input variable key'
+            element :value_placeholder, 'Input variable value'
           end
 
           view 'app/views/ci/variables/_index.html.haml' do
             element :save_variables, '.js-secret-variables-save-button'
+            element :reveal_values, '.js-secret-value-reveal-button'
           end
 
           def fill_variable_key(key)
-            page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
-              page.find('.js-ci-variable-input-key').set(key)
-            end
+            fill_in('Input variable key', with: key, match: :first)
           end
 
           def fill_variable_value(value)
-            page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
-              page.find('.js-ci-variable-input-value').set(value)
-            end
+            fill_in('Input variable value', with: value, match: :first)
           end
 
           def save_variables
-            click_button('Save variables')
+            find('.js-secret-variables-save-button').click
           end
 
-          def variable_key
-            page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
-              page.find('.js-ci-variable-input-key').value
-            end
+          def reveal_variables
+            find('.js-secret-value-reveal-button').click
           end
 
-          def variable_value
-            page.within('.js-ci-variable-list-section .js-row:nth-child(1)') do
-              page.find('.js-ci-variable-input-value').value
+          def variable_value(key)
+            within('.ci-variable-row-body', text: key) do
+              find('.js-ci-variable-input-value').value
             end
           end
         end

qa/qa/specs/features/project/add_secret_variable_spec.rb

@@ -4,16 +4,21 @@ module QA
       Runtime::Browser.visit(:gitlab, Page::Main::Login)
       Page::Main::Login.act { sign_in_using_credentials }
 
-      variable_key = 'VARIABLE_KEY'
-      variable_value = 'variable value'
-
-      variable = Factory::Resource::SecretVariable.fabricate! do |resource|
-        resource.key = variable_key
-        resource.value = variable_value
+      Factory::Resource::SecretVariable.fabricate! do |resource|
+        resource.key = 'VARIABLE_KEY'
+        resource.value = 'some secret variable'
       end
 
-      expect(variable.key).to eq(variable_key)
-      expect(variable.value).to eq(variable_value)
+      Page::Project::Settings::CICD.perform do |settings|
+        settings.expand_secret_variables do |page|
+          expect(page).to have_field(with: 'VARIABLE_KEY')
+          expect(page).not_to have_field(with: 'some secret variable')
+
+          page.reveal_variables
+
+          expect(page).to have_field(with: 'some secret variable')
+        end
+      end
     end
   end
 end

qa/spec/page/base_spec.rb

@@ -14,7 +14,7 @@ describe QA::Page::Base do
         end
 
         view 'path/to/some/_partial.html.haml' do
-          element :something, 'string pattern'
+          element :another_element, 'string pattern'
         end
       end
     end
@@ -25,11 +25,10 @@ describe QA::Page::Base do
     end
 
     it 'populates views objects with data about elements' do
-      subject.views.first.elements.tap do |elements|
-        expect(elements.size).to eq 2
-        expect(elements).to all(be_an_instance_of QA::Page::Element)
-        expect(elements.map(&:name)).to eq [:something, :something_else]
-      end
+      expect(subject.elements.size).to eq 3
+      expect(subject.elements).to all(be_an_instance_of QA::Page::Element)
+      expect(subject.elements.map(&:name))
+        .to eq [:something, :something_else, :another_element]
     end
   end
 

spec/lib/gitlab/checks/change_access_spec.rb

@@ -221,7 +221,7 @@ describe Gitlab::Checks::ChangeAccess do
 
       context 'with LFS not enabled' do
         it 'skips the validation' do
-          expect_any_instance_of(described_class).not_to receive(:lfs_file_locks_validation)
+          expect_any_instance_of(Gitlab::Checks::CommitCheck).not_to receive(:validate)
 
           subject.exec
         end
@@ -238,7 +238,7 @@ describe Gitlab::Checks::ChangeAccess do
           end
         end
 
-        context 'when change is sent by the author od the lock' do
+        context 'when change is sent by the author of the lock' do
           let(:user) { owner }
 
           it "doesn't raise any error" do

spec/lib/gitlab/github_import/importer/repository_importer_spec.rb

@@ -11,7 +11,8 @@ describe Gitlab::GithubImport::Importer::RepositoryImporter do
       import_source: 'foo/bar',
       repository_storage_path: 'foo',
       disk_path: 'foo',
-      repository: repository
+      repository: repository,
+      create_wiki: true
     )
   end
 
@@ -192,7 +193,7 @@ describe Gitlab::GithubImport::Importer::RepositoryImporter do
       expect(importer.import_wiki_repository).to eq(true)
     end
 
-    it 'marks the import as failed if an error was raised' do
+    it 'marks the import as failed and creates an empty repo if an error was raised' do
       expect(importer.gitlab_shell)
         .to receive(:import_repository)
         .and_raise(Gitlab::Shell::Error)
@@ -201,6 +202,9 @@ describe Gitlab::GithubImport::Importer::RepositoryImporter do
         .to receive(:fail_import)
         .and_return(false)
 
+      expect(project)
+        .to receive(:create_wiki)
+
       expect(importer.import_wiki_repository).to eq(false)
     end
   end

spec/mailers/notify_spec.rb

@@ -578,59 +578,30 @@ describe Notify do
     end
 
     describe 'project access requested' do
-      context 'for a project in a user namespace' do
-        let(:project) do
-          create(:project, :public, :access_requestable) do |project|
-            project.add_master(project.owner, current_user: project.owner)
-          end
-        end
-
-        let(:project_member) do
-          project.request_access(user)
-          project.requesters.find_by(user_id: user.id)
-        end
-        subject { described_class.member_access_requested_email('project', project_member.id) }
-
-        it_behaves_like 'an email sent from GitLab'
-        it_behaves_like 'it should not have Gmail Actions links'
-        it_behaves_like "a user cannot unsubscribe through footer link"
-
-        it 'contains all the useful information' do
-          to_emails = subject.header[:to].addrs
-          expect(to_emails.size).to eq(1)
-          expect(to_emails[0].address).to eq(project.members.owners_and_masters.first.user.notification_email)
-
-          is_expected.to have_subject "Request to join the #{project.name_with_namespace} project"
-          is_expected.to have_html_escaped_body_text project.name_with_namespace
-          is_expected.to have_body_text project_project_members_url(project)
-          is_expected.to have_body_text project_member.human_access
+      let(:project) do
+        create(:project, :public, :access_requestable) do |project|
+          project.add_master(project.owner)
         end
       end
 
-      context 'for a project in a group' do
-        let(:group_owner) { create(:user) }
-        let(:group) { create(:group).tap { |g| g.add_owner(group_owner) } }
-        let(:project) { create(:project, :public, :access_requestable, namespace: group) }
-        let(:project_member) do
-          project.request_access(user)
-          project.requesters.find_by(user_id: user.id)
-        end
-        subject { described_class.member_access_requested_email('project', project_member.id) }
+      let(:project_member) do
+        project.request_access(user)
+        project.requesters.find_by(user_id: user.id)
+      end
+      subject { described_class.member_access_requested_email('project', project_member.id, recipient.notification_email) }
 
-        it_behaves_like 'an email sent from GitLab'
-        it_behaves_like 'it should not have Gmail Actions links'
-        it_behaves_like "a user cannot unsubscribe through footer link"
+      it_behaves_like 'an email sent from GitLab'
+      it_behaves_like 'it should not have Gmail Actions links'
+      it_behaves_like "a user cannot unsubscribe through footer link"
 
-        it 'contains all the useful information' do
-          to_emails = subject.header[:to].addrs
-          expect(to_emails.size).to eq(1)
-          expect(to_emails[0].address).to eq(group.members.owners_and_masters.first.user.notification_email)
+      it 'contains all the useful information' do
+        to_emails = subject.header[:to].addrs.map(&:address)
+        expect(to_emails).to eq([recipient.notification_email])
 
-          is_expected.to have_subject "Request to join the #{project.name_with_namespace} project"
-          is_expected.to have_html_escaped_body_text project.name_with_namespace
-          is_expected.to have_body_text project_project_members_url(project)
-          is_expected.to have_body_text project_member.human_access
-        end
+        is_expected.to have_subject "Request to join the #{project.name_with_namespace} project"
+        is_expected.to have_html_escaped_body_text project.name_with_namespace
+        is_expected.to have_body_text project_project_members_url(project)
+        is_expected.to have_body_text project_member.human_access
       end
     end
 
@@ -1074,13 +1045,16 @@ describe Notify do
         group.request_access(user)
         group.requesters.find_by(user_id: user.id)
       end
-      subject { described_class.member_access_requested_email('group', group_member.id) }
+      subject { described_class.member_access_requested_email('group', group_member.id, recipient.notification_email) }
 
       it_behaves_like 'an email sent from GitLab'
       it_behaves_like 'it should not have Gmail Actions links'
       it_behaves_like "a user cannot unsubscribe through footer link"
 
       it 'contains all the useful information' do
+        to_emails = subject.header[:to].addrs.map(&:address)
+        expect(to_emails).to eq([recipient.notification_email])
+
         is_expected.to have_subject "Request to join the #{group.name} group"
         is_expected.to have_html_escaped_body_text group.name
         is_expected.to have_body_text group_group_members_url(group)

spec/services/notification_service_spec.rb

@@ -1349,6 +1349,33 @@ describe NotificationService, :mailer do
   end
 
   describe 'GroupMember' do
+    let(:added_user) { create(:user) }
+
+    describe '#new_access_request' do
+      let(:master) { create(:user) }
+      let(:owner) { create(:user) }
+      let(:developer) { create(:user) }
+      let!(:group) do
+        create(:group, :public, :access_requestable) do |group|
+          group.add_owner(owner)
+          group.add_master(master)
+          group.add_developer(developer)
+        end
+      end
+
+      before do
+        reset_delivered_emails!
+      end
+
+      it 'sends notification to group owners_and_masters' do
+        group.request_access(added_user)
+
+        should_email(owner)
+        should_email(master)
+        should_not_email(developer)
+      end
+    end
+
     describe '#decline_group_invite' do
       let(:creator) { create(:user) }
       let(:group) { create(:group) }
@@ -1370,18 +1397,9 @@ describe NotificationService, :mailer do
 
     describe '#new_group_member' do
       let(:group) { create(:group) }
-      let(:added_user) { create(:user) }
-
-      def create_member!
-        GroupMember.create(
-          group: group,
-          user: added_user,
-          access_level: Gitlab::Access::GUEST
-        )
-      end
 
       it 'sends a notification' do
-        create_member!
+        group.add_guest(added_user)
         should_only_email(added_user)
       end
 
@@ -1391,7 +1409,7 @@ describe NotificationService, :mailer do
         end
 
         it 'does not send a notification' do
-          create_member!
+          group.add_guest(added_user)
           should_not_email_anyone
         end
       end
@@ -1399,8 +1417,42 @@ describe NotificationService, :mailer do
   end
 
   describe 'ProjectMember' do
+    let(:project) { create(:project) }
+    set(:added_user) { create(:user) }
+
+    describe '#new_access_request' do
+      context 'for a project in a user namespace' do
+        let(:project) do
+          create(:project, :public, :access_requestable) do |project|
+            project.add_master(project.owner)
+          end
+        end
+
+        it 'sends notification to project owners_and_masters' do
+          project.request_access(added_user)
+
+          should_only_email(project.owner)
+        end
+      end
+
+      context 'for a project in a group' do
+        let(:group_owner) { create(:user) }
+        let(:group) { create(:group).tap { |g| g.add_owner(group_owner) } }
+        let!(:project) { create(:project, :public, :access_requestable, namespace: group) }
+
+        before do
+          reset_delivered_emails!
+        end
+
+        it 'sends notification to group owners_and_masters' do
+          project.request_access(added_user)
+
+          should_only_email(group_owner)
+        end
+      end
+    end
+
     describe '#decline_group_invite' do
-      let(:project) { create(:project) }
       let(:member) { create(:user) }
 
       before do
@@ -1417,19 +1469,12 @@ describe NotificationService, :mailer do
     end
 
     describe '#new_project_member' do
-      let(:project) { create(:project) }
-      let(:added_user) { create(:user) }
-
-      def create_member!
-        create(:project_member, user: added_user, project: project)
-      end
-
       it do
         create_member!
         should_only_email(added_user)
       end
 
-      describe 'when notifications are disabled' do
+      context 'when notifications are disabled' do
         before do
           create_global_setting_for(added_user, :disabled)
         end
@@ -1440,6 +1485,10 @@ describe NotificationService, :mailer do
         end
       end
     end
+
+    def create_member!
+      create(:project_member, user: added_user, project: project)
+    end
   end
 
   context 'guest user in private project' do