Merge branch 'master' into ldap-sync-group-members

CHANGELOG

@@ -12,6 +12,8 @@ v 7.14.0 (unreleased)
   - Add support for destroying project milestones (Stan Hu)
   - Add fetch command to the MR page.
   - Fix bug causing "Remove source-branch" option not to work for merge requests from the same project.
+  - Fix approvals for forks. Now you can change approvals settings on the new merge request form
+  - Suggested approvers are shown on the new merge request form
 
 v 7.13.1
   - Revert issue caching

CHANGELOG-EE

 v 7.14
   - Disable adding, updating and removing members from a group that is synced with LDAP
+  - Don't send "Added to group" notifications when group is LDAP synched
 
 v 7.13.2
   - Fix group web hook
+  - Don't resend admin email to everyone if one delivery fails
 
 v 7.13.1
   - Merge community edition changes for version 7.13.1

app/models/group.rb

@@ -64,18 +64,18 @@ class Group < Namespace
     @owners ||= group_members.owners.map(&:user)
   end
 
-  def add_users(user_ids, access_level, current_user = nil)
+  def add_users(user_ids, access_level, current_user = nil, skip_notification: false)
     user_ids.each do |user_id|
-      Member.add_user(self.group_members, user_id, access_level, current_user)
+      Member.add_user(self.group_members, user_id, access_level, current_user, skip_notification: skip_notification)
     end
   end
 
-  def add_user(user, access_level, current_user = nil)
-    add_users([user], access_level, current_user)
+  def add_user(user, access_level, current_user = nil, skip_notification: false)
+    add_users([user], access_level, current_user, skip_notification: skip_notification)
   end
 
-  def add_owner(user, current_user = nil)
-    self.add_user(user, Gitlab::Access::OWNER, current_user)
+  def add_owner(user, current_user = nil, skip_notification: false)
+    self.add_user(user, Gitlab::Access::OWNER, current_user, skip_notification: skip_notification)
   end
 
   def has_owner?(user)

app/models/member.rb

@@ -23,6 +23,7 @@ class Member < ActiveRecord::Base
   include Gitlab::Access
 
   attr_accessor :raw_invite_token
+  attr_accessor :skip_notification
 
   belongs_to :created_by, class_name: "User"
   belongs_to :user
@@ -71,7 +72,7 @@ class Member < ActiveRecord::Base
       user
     end
 
-    def add_user(members, user_id, access_level, current_user = nil)
+    def add_user(members, user_id, access_level, current_user = nil, skip_notification: false)
       user = user_for_id(user_id)
       
       # `user` can be either a User object or an email to be invited
@@ -85,6 +86,8 @@ class Member < ActiveRecord::Base
       member.created_by ||= current_user
       member.access_level = access_level
 
+      member.skip_notification = skip_notification
+
       member.save
     end
   end

app/models/members/group_member.rb

@@ -47,33 +47,33 @@ class GroupMember < Member
   private
 
   def send_invite
-    notification_service.invite_group_member(self, @raw_invite_token)
+    notification_service.invite_group_member(self, @raw_invite_token) unless @skip_notification
 
     super
   end
 
   def post_create_hook
-    notification_service.new_group_member(self)
+    notification_service.new_group_member(self) unless @skip_notification
 
     super
   end
 
   def post_update_hook
     if access_level_changed?
-      notification_service.update_group_member(self)
+      notification_service.update_group_member(self) unless @skip_notification
     end
 
     super
   end
 
   def after_accept_invite
-    notification_service.accept_group_invite(self)
+    notification_service.accept_group_invite(self) unless @skip_notification
 
     super
   end
 
   def after_decline_invite
-    notification_service.decline_group_invite(self)
+    notification_service.decline_group_invite(self) unless @skip_notification
 
     super
   end

app/models/members/project_member.rb

@@ -123,7 +123,7 @@ class ProjectMember < Member
   private
 
   def send_invite
-    notification_service.invite_project_member(self, @raw_invite_token)
+    notification_service.invite_project_member(self, @raw_invite_token) unless @skip_notification
 
     super
   end
@@ -131,7 +131,7 @@ class ProjectMember < Member
   def post_create_hook
     unless owner?
       event_service.join_project(self.project, self.user)
-      notification_service.new_project_member(self)
+      notification_service.new_project_member(self) unless @skip_notification
     end
     
     super
@@ -139,7 +139,7 @@ class ProjectMember < Member
 
   def post_update_hook
     if access_level_changed?
-      notification_service.update_project_member(self) 
+      notification_service.update_project_member(self)  unless @skip_notification
     end
 
     super
@@ -152,13 +152,13 @@ class ProjectMember < Member
   end
 
   def after_accept_invite
-    notification_service.accept_project_invite(self)
+    notification_service.accept_project_invite(self) unless @skip_notification
 
     super
   end
 
   def after_decline_invite
-    notification_service.decline_project_invite(self)
+    notification_service.decline_project_invite(self) unless @skip_notification
 
     super
   end

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

@@ -81,7 +81,7 @@
         = link_to 'Create new label', new_namespace_project_label_path(issuable.project.namespace, issuable.project), target: :blank
 
 - if issuable.is_a?(MergeRequest)
-  - if @project.approvals_before_merge.nonzero?
+  - if @merge_request.requires_approve?
     .form-group
       = f.label :approver_ids, class: 'control-label' do
         Approvers
@@ -89,16 +89,26 @@
         = users_select_tag("merge_request[approver_ids]", multiple: true, class: 'input-large', scope: :all, email_user: true)
         .help-block
           Merge Request should be approved by these users. 
-          - if @project.approvers.any?
-            Suggested approver(s): #{render_items_list(@project.approvers.map{ |approver| approver.user.name})}.
           You can override the project settings by setting your own list of approvers.
         
         .panel.panel-default.prepend-top-10
           .panel-heading
             Approvers
-            %small
-              (#{@merge_request.approvers.count(:all)})
+            %small#approvers-counter
+              - approvers = @merge_request.new_record? ? @merge_request.target_project.approvers : @merge_request.approvers
+              (#{approvers.count(:all)})
           %ul.well-list
+            - if @merge_request.new_record?
+              - @merge_request.target_project.approvers.each do |approver|
+                %li.project-approvers{id: dom_id(approver.user)}
+                  = link_to approver.user.name, approver.user
+                  .pull-right
+                    = link_to "#", data: { confirm: "Are you sure you want to remove approver #{approver.user.name}"}, class: "btn-xs btn btn-remove", title: 'Remove approver' do
+                      = icon("sign-out")
+                      Remove
+              - if @merge_request.target_project.approvers.empty?
+                %li There are no approvers
+            - else
               - @merge_request.approvers.each do |approver|
                 %li
                   = link_to approver.user.name, approver.user
@@ -141,3 +151,18 @@
   - else
     - cancel_project = issuable.project
   = link_to 'Cancel', [cancel_project.namespace.becomes(Namespace), cancel_project, issuable], class: 'btn btn-cancel'
+
+:coffeescript
+  $(".project-approvers .btn-remove").click ->
+    approvers_list = $(this).closest("ul")
+    $(this).closest("li").remove()
+    approvers_count = approvers_list.find("li").size()
+    $("#approvers-counter").text("(" + approvers_count + ")")
+    return false
+  $("form#new_merge_request").submit ->
+    approver_ids = $.map $("li.project-approvers"), (li, i) ->
+      li.id.replace("user_", "")
+    approvers_input = $(this).find("input#merge_request_approver_ids")
+    approver_ids = approver_ids.concat(approvers_input.val().split(","))
+    approvers_input.val(approver_ids.join(","))
+

app/workers/admin_emails_worker.rb

@@ -3,7 +3,7 @@ class AdminEmailsWorker
 
   def perform(recipient_id, subject, body)
     recipient_list(recipient_id).pluck(:id).each do |user_id|
-      Notify.send_admin_notification(user_id, subject, body).deliver
+      Notify.delay.send_admin_notification(user_id, subject, body)
     end
   end
 

features/project/merge_requests.feature

@@ -255,6 +255,13 @@ Feature: Project Merge Requests
     When I should not see Approve button
     And I should see message that MR require an approval
 
+  Scenario: I see suggested approvers on new merge request form
+    Given project settings contain list of approvers
+    When I click link "New Merge Request"
+    And I select "fix" as source
+    Then I see suggested approver
+    
+
   Scenario: I should see rebase checkbox
     Given project "Shop" have "Bug NS-05" open merge request with diffs inside
     And rebase before merge enabled

features/steps/project/merge_requests.rb

@@ -338,6 +338,17 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     expect(page).to have_content 'Target branch changed from master to feature'
   end
 
+  step 'project settings contain list of approvers' do
+    project.update(approvals_before_merge: 1)
+    project.approvers.create(user_id: current_user.id)
+  end
+
+  step 'I see suggested approver' do
+    page.within '.project-approvers' do
+      expect(page).to have_content(current_user.name)
+    end
+  end
+
   step 'merge request \'Bug NS-04\' must be approved' do
     merge_request = MergeRequest.find_by!(title: "Bug NS-04")
     project = merge_request.target_project
@@ -350,7 +361,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     merge_request = MergeRequest.find_by!(title: "Bug NS-04")
     project = merge_request.target_project
     project.approvals_before_merge = 1
-    project.approvers.create(user_id: current_user.id)
+    merge_request.approvers.create(user_id: current_user.id)
     project.ensure_satellite_exists
     project.save!
   end
@@ -359,7 +370,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
     merge_request = MergeRequest.find_by!(title: "Bug NS-04")
     project = merge_request.target_project
     project.approvals_before_merge = 1
-    project.approvers.create(user_id: create(:user).id)
+    merge_request.approvers.create(user_id: create(:user).id)
     project.ensure_satellite_exists
     project.save!
   end

lib/gitlab/ldap/access.rb

@@ -140,7 +140,7 @@ module Gitlab
           active_group_links = group.ldap_group_links.where(cn: cns_with_access)
 
           if active_group_links.any?
-            group.add_users([user.id], fetch_group_access(group, user, active_group_links))
+            group.add_users([user.id], fetch_group_access(group, user, active_group_links), skip_notification: true)
           elsif group.last_owner?(user)
             Rails.logger.warn "#{self.class.name}: LDAP group sync cannot remove #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
           else

spec/lib/gitlab/ldap/access_spec.rb

@@ -258,6 +258,11 @@ objectclass: posixGroup
         access.update_ldap_group_links
         expect( gitlab_group_1.has_master?(user) ).to be_truthy
       end
+
+      it "doesn't send a notification email" do
+        expect { access.update_ldap_group_links }.not_to \
+          change { ActionMailer::Base.deliveries }
+      end
     end
 
     context "existing access as guest for group-1, allowed via ldap-group1 as DEVELOPER" do
@@ -271,6 +276,11 @@ objectclass: posixGroup
         expect { access.update_ldap_group_links }.to \
           change{ gitlab_group_1.has_master?(user) }.from(false).to(true)
       end
+
+      it "doesn't send a notification email" do
+        expect { access.update_ldap_group_links }.not_to \
+          change { ActionMailer::Base.deliveries }
+      end
     end
 
     context "existing access as MASTER for group-1, allowed via ldap-group1 as DEVELOPER" do
@@ -284,6 +294,11 @@ objectclass: posixGroup
         expect { access.update_ldap_group_links }.not_to \
           change{ gitlab_group_1.has_master?(user) }
       end
+
+      it "doesn't send a notification email" do
+        expect { access.update_ldap_group_links }.not_to \
+          change { ActionMailer::Base.deliveries }
+      end
     end
 
     context "existing access as master for group-1, not allowed" do

spec/models/merge_request_spec.rb

@@ -181,8 +181,8 @@ describe MergeRequest do
     it "returns correct value" do
       user = create(:user)
       user1 = create(:user)
-      merge_request.target_project.approvers.create(user_id: user.id)
-      merge_request.target_project.approvers.create(user_id: user1.id)
+      merge_request.approvers.create(user_id: user.id)
+      merge_request.approvers.create(user_id: user1.id)
       merge_request.approvals.create(user_id: user1.id)
 
       expect(merge_request.approvers_left).to eq [user]