From dd3b738d5b3eb70217d7ac7f9fe441498d2e8e7e Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 16 Aug 2016 13:34:56 +0530
Subject: Fix failing tests relating to backporting ee!581.

1. `GitPushService` was still using `{merge,push}_access_level_attributes` instead
   of `{merge,push}_access_levels_attributes`.

2. The branches API creates access levels regardless of the state of the
   `developers_can_{push,merge}` parameters. This is in line with the UI,
   where Master access is the default for a new protected branch.

3. Use `after(:build)` to create access levels in the
   `protected_branches` factory, so that `factories_spec` passes. It
   only builds records, so we need to create access levels on `build` as
   well.
---
 app/services/git_push_service.rb     |  8 ++++----
 lib/api/branches.rb                  | 29 +++++++++++++++++------------
 spec/factories/protected_branches.rb |  2 +-
 spec/models/project_spec.rb          |  6 +++---
 4 files changed, 25 insertions(+), 20 deletions(-)

diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index 6f521462cf3..d5fb2018d24 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -91,12 +91,12 @@ class GitPushService < BaseService
 
       params = {
         name: @project.default_branch,
-        push_access_level_attributes: {
+        push_access_levels_attributes: [{
           access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-        },
-        merge_access_level_attributes: {
+        }],
+        merge_access_levels_attributes: [{
           access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-        }
+        }]
       }
 
       ProtectedBranches::CreateService.new(@project, current_user, params).execute
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index a77afe634f6..b615703df93 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -61,22 +61,27 @@ module API
           name: @branch.name
         }
 
-        unless developers_can_merge.nil?
-          protected_branch_params.merge!({
-            merge_access_level_attributes: {
-              access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-            }
-          })
+        # If `developers_can_merge` is switched off, _all_ `DEVELOPER`
+        # merge_access_levels need to be deleted.
+        if developers_can_merge == false
+          protected_branch.merge_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all
         end
 
-        unless developers_can_push.nil?
-          protected_branch_params.merge!({
-            push_access_level_attributes: {
-              access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
-            }
-          })
+        # If `developers_can_push` is switched off, _all_ `DEVELOPER`
+        # push_access_levels need to be deleted.
+        if developers_can_push == false
+          protected_branch.push_access_levels.where(access_level: Gitlab::Access::DEVELOPER).destroy_all
         end
 
+        protected_branch_params.merge!(
+          merge_access_levels_attributes: [{
+            access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+          }],
+          push_access_levels_attributes: [{
+            access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+          }]
+        )
+
         if protected_branch
           service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params)
           service.execute(protected_branch)
diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb
index 42853cac112..b2695e0482a 100644
--- a/spec/factories/protected_branches.rb
+++ b/spec/factories/protected_branches.rb
@@ -3,7 +3,7 @@ FactoryGirl.define do
     name
     project
 
-    before(:create) do |protected_branch|
+    after(:build) do |protected_branch|
       protected_branch.push_access_levels.new(access_level: Gitlab::Access::MASTER)
       protected_branch.merge_access_levels.new(access_level: Gitlab::Access::MASTER)
     end
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 9c3b4712cab..0a32a486703 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1089,13 +1089,13 @@ describe Project, models: true do
       let(:project) { create(:project) }
 
       it 'returns true when the branch matches a protected branch via direct match' do
-        project.protected_branches.create!(name: 'foo')
+        create(:protected_branch, project: project, name: "foo")
 
         expect(project.protected_branch?('foo')).to eq(true)
       end
 
       it 'returns true when the branch matches a protected branch via wildcard match' do
-        project.protected_branches.create!(name: 'production/*')
+        create(:protected_branch, project: project, name: "production/*")
 
         expect(project.protected_branch?('production/some-branch')).to eq(true)
       end
@@ -1105,7 +1105,7 @@ describe Project, models: true do
       end
 
       it 'returns false when the branch does not match a protected branch via wildcard match' do
-        project.protected_branches.create!(name: 'production/*')
+        create(:protected_branch, project: project, name: "production/*")
 
         expect(project.protected_branch?('staging/some-branch')).to eq(false)
       end
-- 
cgit v1.2.1