From f1e46d1e63faf63f1dc9960c5f28d5260dfc84db Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 5 Jul 2016 12:29:15 +0530
Subject: [PATCH 01/23] Add a series of migrations changing the model-level
 design of protected branch access levels.

1. Remove the `developers_can_push` and `developers_can_merge` boolean
   columns.

2. Add two new tables, `protected_branches_push_access`, and
   `protected_branches_merge_access`. Each row of these 'access' tables is
   linked to a protected branch, and uses a `access_level` column to
   figure out settings for the protected branch.

3. The `access_level` column is intended to be used with rails' `enum`,
   with `:masters` at index 0 and `:developers` at index 1.

4. Doing it this way has a few advantages:

   - Cleaner path to planned EE features where a protected branch is
     accessible only by certain users or groups.

   - Rails' `enum` doesn't allow a declaration like this due to the
     duplicates. This approach doesn't have this problem.

       enum can_be_pushed_by: [:masters, :developers]
       enum can_be_merged_by: [:masters, :developers]
---
 ...4938_add_protected_branches_push_access.rb | 15 +++++++++
 ...952_add_protected_branches_merge_access.rb | 15 +++++++++
 ...erge_to_protected_branches_merge_access.rb | 33 +++++++++++++++++++
 ..._push_to_protected_branches_push_access.rb | 33 +++++++++++++++++++
 ...lopers_can_push_from_protected_branches.rb | 21 ++++++++++++
 ...opers_can_merge_from_protected_branches.rb | 21 ++++++++++++
 db/schema.rb                                  | 26 ++++++++++++---
 7 files changed, 160 insertions(+), 4 deletions(-)
 create mode 100644 db/migrate/20160705054938_add_protected_branches_push_access.rb
 create mode 100644 db/migrate/20160705054952_add_protected_branches_merge_access.rb
 create mode 100644 db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
 create mode 100644 db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
 create mode 100644 db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
 create mode 100644 db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb

diff --git a/db/migrate/20160705054938_add_protected_branches_push_access.rb b/db/migrate/20160705054938_add_protected_branches_push_access.rb
new file mode 100644
index 00000000000..512d99e4823
--- /dev/null
+++ b/db/migrate/20160705054938_add_protected_branches_push_access.rb
@@ -0,0 +1,15 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddProtectedBranchesPushAccess < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  def change
+    create_table :protected_branch_push_access_levels do |t|
+      t.references :protected_branch, index: { name: "index_protected_branch_push_access" }, foreign_key: true, null: false
+      t.integer :access_level, default: 0, null: false
+
+      t.timestamps null: false
+    end
+  end
+end
diff --git a/db/migrate/20160705054952_add_protected_branches_merge_access.rb b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
new file mode 100644
index 00000000000..9f82c0a8aa3
--- /dev/null
+++ b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
@@ -0,0 +1,15 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddProtectedBranchesMergeAccess < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  def change
+    create_table :protected_branch_merge_access_levels do |t|
+      t.references :protected_branch, index: { name: "index_protected_branch_merge_access" }, foreign_key: true, null: false
+      t.integer :access_level, default: 0, null: false
+
+      t.timestamps null: false
+    end
+  end
+end
diff --git a/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
new file mode 100644
index 00000000000..20ca9c3a488
--- /dev/null
+++ b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
@@ -0,0 +1,33 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MoveFromDevelopersCanMergeToProtectedBranchesMergeAccess < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def up
+    execute <<-HEREDOC
+      INSERT into protected_branch_merge_access_levels (protected_branch_id, access_level, created_at, updated_at)
+        SELECT id, (CASE WHEN developers_can_merge THEN 1 ELSE 0 END), now(), now()
+          FROM protected_branches
+    HEREDOC
+  end
+
+  def down
+    execute <<-HEREDOC
+      UPDATE protected_branches SET developers_can_merge = TRUE
+        WHERE id IN (SELECT protected_branch_id FROM protected_branch_merge_access_levels
+                       WHERE access_level = 1);
+    HEREDOC
+  end
+end
diff --git a/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
new file mode 100644
index 00000000000..498fb393d61
--- /dev/null
+++ b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
@@ -0,0 +1,33 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class MoveFromDevelopersCanPushToProtectedBranchesPushAccess < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def up
+    execute <<-HEREDOC
+      INSERT into protected_branch_push_access_levels (protected_branch_id, access_level, created_at, updated_at)
+        SELECT id, (CASE WHEN developers_can_push THEN 1 ELSE 0 END), now(), now()
+          FROM protected_branches
+    HEREDOC
+  end
+
+  def down
+    execute <<-HEREDOC
+      UPDATE protected_branches SET developers_can_push = TRUE
+        WHERE id IN (SELECT protected_branch_id FROM protected_branch_push_access_levels
+                       WHERE access_level = 1);
+    HEREDOC
+  end
+end
diff --git a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
new file mode 100644
index 00000000000..1e9977cfa6e
--- /dev/null
+++ b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
@@ -0,0 +1,21 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveDevelopersCanPushFromProtectedBranches < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    remove_column :protected_branches, :developers_can_push, :boolean
+  end
+end
diff --git a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
new file mode 100644
index 00000000000..43d02fbaed6
--- /dev/null
+++ b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
@@ -0,0 +1,21 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class RemoveDevelopersCanMergeFromProtectedBranches < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # When using the methods "add_concurrent_index" or "add_column_with_default"
+  # you must disable the use of transactions as these methods can not run in an
+  # existing transaction. When using "add_concurrent_index" make sure that this
+  # method is the _only_ method called in the migration, any other changes
+  # should go in a separate migration. This ensures that upon failure _only_ the
+  # index creation fails and can be retried or reverted easily.
+  #
+  # To disable transactions uncomment the following line and remove these
+  # comments:
+  # disable_ddl_transaction!
+
+  def change
+    remove_column :protected_branches, :developers_can_merge, :boolean
+  end
+end
diff --git a/db/schema.rb b/db/schema.rb
index 15cee55a7bf..7a5eded8e02 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -867,13 +867,29 @@ ActiveRecord::Schema.define(version: 20160722221922) do
   add_index "projects", ["star_count"], name: "index_projects_on_star_count", using: :btree
   add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
 
+  create_table "protected_branch_merge_access_levels", force: :cascade do |t|
+    t.integer  "protected_branch_id",             null: false
+    t.integer  "access_level",        default: 0, null: false
+    t.datetime "created_at",                      null: false
+    t.datetime "updated_at",                      null: false
+  end
+
+  add_index "protected_branch_merge_access_levels", ["protected_branch_id"], name: "index_protected_branch_merge_access", using: :btree
+
+  create_table "protected_branch_push_access_levels", force: :cascade do |t|
+    t.integer  "protected_branch_id",             null: false
+    t.integer  "access_level",        default: 0, null: false
+    t.datetime "created_at",                      null: false
+    t.datetime "updated_at",                      null: false
+  end
+
+  add_index "protected_branch_push_access_levels", ["protected_branch_id"], name: "index_protected_branch_push_access", using: :btree
+
   create_table "protected_branches", force: :cascade do |t|
-    t.integer  "project_id",                           null: false
-    t.string   "name",                                 null: false
+    t.integer  "project_id", null: false
+    t.string   "name",       null: false
     t.datetime "created_at"
     t.datetime "updated_at"
-    t.boolean  "developers_can_push",  default: false, null: false
-    t.boolean  "developers_can_merge", default: false, null: false
   end
 
   add_index "protected_branches", ["project_id"], name: "index_protected_branches_on_project_id", using: :btree
@@ -1136,5 +1152,7 @@ ActiveRecord::Schema.define(version: 20160722221922) do
   add_index "web_hooks", ["project_id"], name: "index_web_hooks_on_project_id", using: :btree
 
   add_foreign_key "personal_access_tokens", "users"
+  add_foreign_key "protected_branch_merge_access_levels", "protected_branches"
+  add_foreign_key "protected_branch_push_access_levels", "protected_branches"
   add_foreign_key "u2f_registrations", "users"
 end

From 21bece443d5f871680a3d7649c2d16861035196d Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 5 Jul 2016 13:10:42 +0530
Subject: [PATCH 02/23] Add models for the protected branch access levels.

- And hook up their associations.
---
 app/models/protected_branch.rb                    | 3 +++
 app/models/protected_branch/merge_access_level.rb | 3 +++
 app/models/protected_branch/push_access_level.rb  | 3 +++
 3 files changed, 9 insertions(+)
 create mode 100644 app/models/protected_branch/merge_access_level.rb
 create mode 100644 app/models/protected_branch/push_access_level.rb

diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index b7011d7afdf..a411cb417e2 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -5,6 +5,9 @@ class ProtectedBranch < ActiveRecord::Base
   validates :name, presence: true
   validates :project, presence: true
 
+  has_one :merge_access_level
+  has_one :push_access_level
+
   def commit
     project.commit(self.name)
   end
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
new file mode 100644
index 00000000000..78cec5bf566
--- /dev/null
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -0,0 +1,3 @@
+class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
+  belongs_to :protected_branch
+end
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
new file mode 100644
index 00000000000..d53c4c391e3
--- /dev/null
+++ b/app/models/protected_branch/push_access_level.rb
@@ -0,0 +1,3 @@
+class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
+  belongs_to :protected_branch
+end

From 134fe5af83167f95205a080f7932452de7d77496 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 7 Jul 2016 13:06:28 +0530
Subject: [PATCH 03/23] Use the `{Push,Merge}AccessLevel` models in the UI.

1. Improve error handling while creating protected branches.

2. Modify coffeescript code so that the "Developers can *" checkboxes
   send a '1' or '0' even when using AJAX. This lets us keep the backend
   code simpler.

3. Use services for both creating and updating protected branches.
   Destruction is taken care of with `dependent: :destroy`
---
 .../projects/protected_branches_controller.rb | 24 +++++++++++++------
 app/models/protected_branch.rb                | 12 ++++++++--
 .../protected_branch/merge_access_level.rb    |  2 ++
 .../protected_branch/push_access_level.rb     |  2 ++
 .../protected_branches/base_service.rb        | 17 +++++++++++++
 .../protected_branches/create_service.rb      | 19 +++++++++++++++
 .../protected_branches/update_service.rb      | 21 ++++++++++++++++
 7 files changed, 88 insertions(+), 9 deletions(-)
 create mode 100644 app/services/protected_branches/base_service.rb
 create mode 100644 app/services/protected_branches/create_service.rb
 create mode 100644 app/services/protected_branches/update_service.rb

diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index 10dca47fded..fdbe0044d3c 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -3,19 +3,23 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   before_action :require_non_empty_project
   before_action :authorize_admin_project!
   before_action :load_protected_branch, only: [:show, :update, :destroy]
+  before_action :load_protected_branches, only: [:index, :create]
 
   layout "project_settings"
 
   def index
-    @protected_branches = @project.protected_branches.order(:name).page(params[:page])
     @protected_branch = @project.protected_branches.new
     gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } })
   end
 
   def create
-    @project.protected_branches.create(protected_branch_params)
-    redirect_to namespace_project_protected_branches_path(@project.namespace,
-                                                          @project)
+    service = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params)
+    if service.execute
+      redirect_to namespace_project_protected_branches_path(@project.namespace, @project)
+    else
+      @protected_branch = service.protected_branch
+      render :index
+    end
   end
 
   def show
@@ -23,13 +27,15 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def update
-    if @protected_branch && @protected_branch.update_attributes(protected_branch_params)
+    service = ProtectedBranches::UpdateService.new(@project, current_user, params[:id], protected_branch_params)
+
+    if service.execute
       respond_to do |format|
-        format.json { render json: @protected_branch, status: :ok }
+        format.json { render json: service.protected_branch, status: :ok }
       end
     else
       respond_to do |format|
-        format.json { render json: @protected_branch.errors, status: :unprocessable_entity }
+        format.json { render json: service.protected_branch.errors, status: :unprocessable_entity }
       end
     end
   end
@@ -52,4 +58,8 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   def protected_branch_params
     params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge)
   end
+
+  def load_protected_branches
+    @protected_branches = @project.protected_branches.order(:name).page(params[:page])
+  end
 end
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index a411cb417e2..b0fde6c6c1b 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -5,13 +5,21 @@ class ProtectedBranch < ActiveRecord::Base
   validates :name, presence: true
   validates :project, presence: true
 
-  has_one :merge_access_level
-  has_one :push_access_level
+  has_one :merge_access_level, dependent: :destroy
+  has_one :push_access_level, dependent: :destroy
 
   def commit
     project.commit(self.name)
   end
 
+  def developers_can_push
+    self.push_access_level && self.push_access_level.developers?
+  end
+
+  def developers_can_merge
+    self.merge_access_level && self.merge_access_level.developers?
+  end
+
   # Returns all protected branches that match the given branch name.
   # This realizes all records from the scope built up so far, and does
   # _not_ return a relation.
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index 78cec5bf566..cfaa9c166fe 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -1,3 +1,5 @@
 class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
+
+  enum access_level: [:masters, :developers]
 end
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index d53c4c391e3..4345dc4ede4 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -1,3 +1,5 @@
 class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
+
+  enum access_level: [:masters, :developers]
 end
diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb
new file mode 100644
index 00000000000..d4be8698a5f
--- /dev/null
+++ b/app/services/protected_branches/base_service.rb
@@ -0,0 +1,17 @@
+module ProtectedBranches
+  class BaseService < ::BaseService
+    def set_access_levels!
+      if params[:developers_can_push] == '0'
+        @protected_branch.push_access_level.masters!
+      elsif params[:developers_can_push] == '1'
+        @protected_branch.push_access_level.developers!
+      end
+
+      if params[:developers_can_merge] == '0'
+        @protected_branch.merge_access_level.masters!
+      elsif params[:developers_can_merge] == '1'
+        @protected_branch.merge_access_level.developers!
+      end
+    end
+  end
+end
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
new file mode 100644
index 00000000000..743f7bd2ce1
--- /dev/null
+++ b/app/services/protected_branches/create_service.rb
@@ -0,0 +1,19 @@
+class ProtectedBranches::CreateService < BaseService
+  attr_reader :protected_branch
+
+  def execute
+    ProtectedBranch.transaction do
+      @protected_branch = project.protected_branches.new(name: params[:name])
+      @protected_branch.save!
+
+      @protected_branch.create_push_access_level!
+      @protected_branch.create_merge_access_level!
+
+      set_access_levels!
+    end
+
+    true
+  rescue ActiveRecord::RecordInvalid
+    false
+  end
+end
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
new file mode 100644
index 00000000000..ed59b06b79a
--- /dev/null
+++ b/app/services/protected_branches/update_service.rb
@@ -0,0 +1,21 @@
+module ProtectedBranches
+  class UpdateService < BaseService
+    attr_reader :protected_branch
+
+    def initialize(project, current_user, id, params = {})
+      super(project, current_user, params)
+      @id = id
+    end
+
+    def execute
+      ProtectedBranch.transaction do
+        @protected_branch = ProtectedBranch.find(@id)
+        set_access_levels!
+      end
+
+      true
+    rescue ActiveRecord::RecordInvalid
+      false
+    end
+  end
+end

From f8a04e15371815ad39e2c66056db4ab0439555f4 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 7 Jul 2016 13:21:13 +0530
Subject: [PATCH 04/23] Add seeds for protected branches.

---
 db/fixtures/development/16_protected_branches.rb | 12 ++++++++++++
 1 file changed, 12 insertions(+)
 create mode 100644 db/fixtures/development/16_protected_branches.rb

diff --git a/db/fixtures/development/16_protected_branches.rb b/db/fixtures/development/16_protected_branches.rb
new file mode 100644
index 00000000000..103c7f9445c
--- /dev/null
+++ b/db/fixtures/development/16_protected_branches.rb
@@ -0,0 +1,12 @@
+Gitlab::Seeder.quiet do
+  admin_user = User.find(1)
+
+  Project.all.each do |project|
+    params = {
+      name: 'master'
+    }
+
+    ProtectedBranches::CreateService.new(project, admin_user, params).execute
+    print '.'
+  end
+end

From ab6096c17261605d835a4a8edae21f31d90026df Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 7 Jul 2016 16:18:07 +0530
Subject: [PATCH 05/23] Add "No One Can Push" to the protected branches UI.

1. Move to dropdowns instead of checkboxes. One each for "Allowed to
   Push" and "Allowed to Merge"

2. Refactor the `ProtectedBranches` coffeescript class into
   `ProtectedBranchesAccessSelect`.

3. Modify the backend to accept the new parameters.
---
 ...protected_branches_access_select.js.coffee | 39 +++++++++++++++++++
 app/assets/stylesheets/pages/projects.scss    |  5 ++-
 .../projects/protected_branches_controller.rb |  2 +-
 .../protected_branch/push_access_level.rb     |  2 +-
 .../protected_branches/base_service.rb        | 20 ++++++----
 .../protected_branches/create_service.rb      | 28 ++++++-------
 .../_branches_list.html.haml                  | 37 +++++++++---------
 .../_protected_branch.html.haml               |  6 ++-
 8 files changed, 95 insertions(+), 44 deletions(-)
 create mode 100644 app/assets/javascripts/protected_branches_access_select.js.coffee

diff --git a/app/assets/javascripts/protected_branches_access_select.js.coffee b/app/assets/javascripts/protected_branches_access_select.js.coffee
new file mode 100644
index 00000000000..b472ff7ec27
--- /dev/null
+++ b/app/assets/javascripts/protected_branches_access_select.js.coffee
@@ -0,0 +1,39 @@
+class @ProtectedBranchesAccessSelect
+  constructor: () ->
+    $(".allowed-to-merge").each (i, element) =>
+      fieldName = $(element).data('field-name')
+      $(element).glDropdown
+        data: [{id: 'developers', text: 'Developers'}, {id: 'masters', text: 'Masters'}]
+        selectable: true
+        fieldName: fieldName
+        clicked: _.partial(@onSelect, element)
+
+    $(".allowed-to-push").each (i, element) =>
+      fieldName = $(element).data('field-name')
+      $(element).glDropdown
+        data: [{id: 'no_one', text: 'No one'},
+               {id: 'developers', text: 'Developers'},
+               {id: 'masters', text: 'Masters'}]
+        selectable: true
+        fieldName: fieldName
+        clicked: _.partial(@onSelect, element)
+
+
+  onSelect: (dropdown, selected, element, e) =>
+    $(dropdown).find('.dropdown-toggle-text').text(selected.text)
+    $.ajax
+      type: "PATCH"
+      url: $(dropdown).data('url')
+      dataType: "json"
+      data:
+        id: $(dropdown).data('id')
+        protected_branch:
+          "#{$(dropdown).data('type')}": selected.id
+
+      success: ->
+        row = $(e.target)
+        row.closest('tr').effect('highlight')
+
+      error: ->
+        new Flash("Failed to update branch!", "alert")
+
diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss
index cc3aef5199e..c6e73dd1f39 100644
--- a/app/assets/stylesheets/pages/projects.scss
+++ b/app/assets/stylesheets/pages/projects.scss
@@ -664,11 +664,14 @@ pre.light-well {
 .protected-branches-list {
   a {
     color: $gl-gray;
-    font-weight: 600;
 
     &:hover {
       color: $gl-link-color;
     }
+
+    &.is-active {
+      font-weight: 600;
+    }
   }
 }
 
diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index fdbe0044d3c..126358bfe77 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -56,7 +56,7 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def protected_branch_params
-    params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge)
+    params.require(:protected_branch).permit(:name, :allowed_to_push, :allowed_to_merge)
   end
 
   def load_protected_branches
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index 4345dc4ede4..8861632c055 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -1,5 +1,5 @@
 class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
 
-  enum access_level: [:masters, :developers]
+  enum access_level: [:masters, :developers, :no_one]
 end
diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb
index d4be8698a5f..3a7c35327fe 100644
--- a/app/services/protected_branches/base_service.rb
+++ b/app/services/protected_branches/base_service.rb
@@ -1,16 +1,20 @@
 module ProtectedBranches
   class BaseService < ::BaseService
     def set_access_levels!
-      if params[:developers_can_push] == '0'
-        @protected_branch.push_access_level.masters!
-      elsif params[:developers_can_push] == '1'
-        @protected_branch.push_access_level.developers!
+      case params[:allowed_to_merge]
+      when 'masters'
+        @protected_branch.merge_access_level.masters!
+      when 'developers'
+        @protected_branch.merge_access_level.developers!
       end
 
-      if params[:developers_can_merge] == '0'
-        @protected_branch.merge_access_level.masters!
-      elsif params[:developers_can_merge] == '1'
-        @protected_branch.merge_access_level.developers!
+      case params[:allowed_to_push]
+      when 'masters'
+        @protected_branch.push_access_level.masters!
+      when 'developers'
+        @protected_branch.push_access_level.developers!
+      when 'no_one'
+        @protected_branch.push_access_level.no_one!
       end
     end
   end
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
index 743f7bd2ce1..ab462f3054e 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -1,19 +1,21 @@
-class ProtectedBranches::CreateService < BaseService
-  attr_reader :protected_branch
+module ProtectedBranches
+  class CreateService < BaseService
+    attr_reader :protected_branch
 
-  def execute
-    ProtectedBranch.transaction do
-      @protected_branch = project.protected_branches.new(name: params[:name])
-      @protected_branch.save!
+    def execute
+      ProtectedBranch.transaction do
+        @protected_branch = project.protected_branches.new(name: params[:name])
+        @protected_branch.save!
 
-      @protected_branch.create_push_access_level!
-      @protected_branch.create_merge_access_level!
+        @protected_branch.create_push_access_level!
+        @protected_branch.create_merge_access_level!
 
-      set_access_levels!
+        set_access_levels!
+      end
+
+      true
+    rescue ActiveRecord::RecordInvalid
+      false
     end
-
-    true
-  rescue ActiveRecord::RecordInvalid
-    false
   end
 end
diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml
index 720d67dff7c..5f9f2992dca 100644
--- a/app/views/projects/protected_branches/_branches_list.html.haml
+++ b/app/views/projects/protected_branches/_branches_list.html.haml
@@ -5,24 +5,25 @@
     No branches are protected, protect a branch with the form above.
 - else
   - can_admin_project = can?(current_user, :admin_project, @project)
-  .table-responsive
-    %table.table.protected-branches-list
-      %colgroup
-        %col{ width: "20%" }
-        %col{ width: "30%" }
-        %col{ width: "25%" }
-        %col{ width: "25%" }
+
+  %table.table.protected-branches-list
+    %colgroup
+      %col{ width: "20%" }
+      %col{ width: "30%" }
+      %col{ width: "25%" }
+      %col{ width: "25%" }
+    %thead
+      %tr
+        %th Branch
+        %th Last commit
+        %th Allowed to Merge
+        %th Allowed to Push
         - if can_admin_project
-          %col
-      %thead
-        %tr
-          %th Protected Branch
-          %th Commit
-          %th Developers Can Push
-          %th Developers Can Merge
-          - if can_admin_project
-            %th
-      %tbody
-        = render partial: @protected_branches, locals: { can_admin_project: can_admin_project }
+          %th
+    %tbody
+      = render partial: @protected_branches, locals: { can_admin_project: can_admin_project }
 
   = paginate @protected_branches, theme: 'gitlab'
+
+:javascript
+  new ProtectedBranchesAccessSelect();
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
index 7fda7f96047..ceae182e82c 100644
--- a/app/views/projects/protected_branches/_protected_branch.html.haml
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -15,9 +15,11 @@
       - else
         (branch was removed from repository)
   %td
-    = check_box_tag("developers_can_push", protected_branch.id, protected_branch.developers_can_push, data: { url: url })
+    = hidden_field_tag "allowed_to_merge_#{branch.id}", branch.merge_access_level.access_level
+    = dropdown_tag(branch.merge_access_level.access_level.humanize, options: { title: "Allowed To Merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "allowed_to_merge_#{branch.id}", url: @url, id: branch.id, type: "allowed_to_merge" }})
   %td
-    = check_box_tag("developers_can_merge", protected_branch.id, protected_branch.developers_can_merge, data: { url: url })
+    = hidden_field_tag "allowed_to_push_#{branch.id}", branch.push_access_level.access_level
+    = dropdown_tag(branch.push_access_level.access_level.humanize, options: { title: "Allowed To Push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "allowed_to_push_#{branch.id}", url: @url, id: branch.id, type: "allowed_to_push" }})
   - if can_admin_project
     %td
       = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right"

From 828f6eb6e50e6193fad9dbdd95d9dd56506e4064 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 8 Jul 2016 11:45:02 +0530
Subject: [PATCH 06/23] Enforce "No One Can Push" during git operations.

1. The crux of this change is in `UserAccess`, which looks through all
   the access levels, asking each if the user has access to push/merge
   for the current project.

2. Update the `protected_branches` factory to create access levels as
   necessary.

3. Fix and augment `user_access` and `git_access` specs.
---
 .../protected_branch/merge_access_level.rb    |  9 +++
 .../protected_branch/push_access_level.rb     | 11 +++
 lib/gitlab/user_access.rb                     | 10 ++-
 spec/factories/protected_branches.rb          | 17 ++++
 spec/lib/gitlab/git_access_spec.rb            | 80 +++++++++++--------
 spec/lib/gitlab/user_access_spec.rb           |  4 +-
 6 files changed, 90 insertions(+), 41 deletions(-)

diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index cfaa9c166fe..2d13d8c8381 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -1,5 +1,14 @@
 class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
+  delegate :project, to: :protected_branch
 
   enum access_level: [:masters, :developers]
+
+  def check_access(user)
+    if masters?
+      user.can?(:push_code, project) if project.team.master?(user)
+    elsif developers?
+      user.can?(:push_code, project) if (project.team.master?(user) || project.team.developer?(user))
+    end
+  end
 end
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index 8861632c055..5a4a33556ce 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -1,5 +1,16 @@
 class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
+  delegate :project, to: :protected_branch
 
   enum access_level: [:masters, :developers, :no_one]
+
+  def check_access(user)
+    if masters?
+      user.can?(:push_code, project) if project.team.master?(user)
+    elsif developers?
+      user.can?(:push_code, project) if (project.team.master?(user) || project.team.developer?(user))
+    elsif no_one?
+      false
+    end
+  end
 end
diff --git a/lib/gitlab/user_access.rb b/lib/gitlab/user_access.rb
index c0f85e9b3a8..3a69027368f 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -29,8 +29,9 @@ module Gitlab
     def can_push_to_branch?(ref)
       return false unless user
 
-      if project.protected_branch?(ref) && !project.developers_can_push_to_protected_branch?(ref)
-        user.can?(:push_code_to_protected_branches, project)
+      if project.protected_branch?(ref)
+        access_levels = project.protected_branches.matching(ref).map(&:push_access_level)
+        access_levels.any? { |access_level| access_level.check_access(user) }
       else
         user.can?(:push_code, project)
       end
@@ -39,8 +40,9 @@ module Gitlab
     def can_merge_to_branch?(ref)
       return false unless user
 
-      if project.protected_branch?(ref) && !project.developers_can_merge_to_protected_branch?(ref)
-        user.can?(:push_code_to_protected_branches, project)
+      if project.protected_branch?(ref)
+        access_levels = project.protected_branches.matching(ref).map(&:merge_access_level)
+        access_levels.any? { |access_level| access_level.check_access(user) }
       else
         user.can?(:push_code, project)
       end
diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb
index 28ed8078157..24a9b78f0c2 100644
--- a/spec/factories/protected_branches.rb
+++ b/spec/factories/protected_branches.rb
@@ -2,5 +2,22 @@ FactoryGirl.define do
   factory :protected_branch do
     name
     project
+
+    after(:create) do |protected_branch|
+      protected_branch.create_push_access_level!(access_level: :masters)
+      protected_branch.create_merge_access_level!(access_level: :masters)
+    end
+
+    trait :developers_can_push do
+      after(:create) { |protected_branch| protected_branch.push_access_level.developers! }
+    end
+
+    trait :developers_can_merge do
+      after(:create) { |protected_branch| protected_branch.merge_access_level.developers! }
+    end
+
+    trait :no_one_can_push do
+      after(:create) { |protected_branch| protected_branch.push_access_level.no_one! }
+    end
   end
 end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index ae064a878b0..324bb500025 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -217,29 +217,32 @@ describe Gitlab::GitAccess, lib: true do
         run_permission_checks(permissions_matrix)
       end
 
-      context "when 'developers can push' is turned on for the #{protected_branch_type} protected branch" do
-        before { create(:protected_branch, name: protected_branch_name, developers_can_push: true, project: project) }
+      context "when developers are allowed to push into the #{protected_branch_type} protected branch" do
+        before { create(:protected_branch, :developers_can_push, name: protected_branch_name, project: project) }
 
         run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
       end
 
-      context "when 'developers can merge' is turned on for the #{protected_branch_type} protected branch" do
-        before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, project: project) }
+      context "developers are allowed to merge into the #{protected_branch_type} protected branch" do
+        before { create(:protected_branch, :developers_can_merge, name: protected_branch_name, project: project) }
 
         context "when a merge request exists for the given source/target branch" do
           context "when the merge request is in progress" do
             before do
-              create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
+              create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature',
+                     state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
             end
 
-            run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
+            context "when the merge request is not in progress" do
+              before do
+                create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', in_progress_merge_commit_sha: nil)
+              end
+
+              run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
+            end
           end
 
-          context "when the merge request is not in progress" do
-            before do
-              create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', in_progress_merge_commit_sha: nil)
-            end
-
+          context "when a merge request does not exist for the given source/target branch" do
             run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
           end
         end
@@ -249,44 +252,51 @@ describe Gitlab::GitAccess, lib: true do
         end
       end
 
-      context "when 'developers can merge' and 'developers can push' are turned on for the #{protected_branch_type} protected branch" do
-        before { create(:protected_branch, name: protected_branch_name, developers_can_merge: true, developers_can_push: true, project: project) }
+      context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do
+        before { create(:protected_branch, :developers_can_merge, :developers_can_push, name: protected_branch_name, project: project) }
 
         run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
       end
     end
 
-    describe 'deploy key permissions' do
-      let(:key) { create(:deploy_key) }
-      let(:actor) { key }
+    context "when no one is allowed to push to the #{protected_branch_name} protected branch" do
+      before { create(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
 
-      context 'push code' do
-        subject { access.check('git-receive-pack') }
+      run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
+                                                          master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
+    end
+  end
 
-        context 'when project is authorized' do
-          before { key.projects << project }
+  describe 'deploy key permissions' do
+    let(:key) { create(:deploy_key) }
+    let(:actor) { key }
+
+    context 'push code' do
+      subject { access.check('git-receive-pack') }
+
+      context 'when project is authorized' do
+        before { key.projects << project }
+
+        it { expect(subject).not_to be_allowed }
+      end
+
+      context 'when unauthorized' do
+        context 'to public project' do
+          let(:project) { create(:project, :public) }
 
           it { expect(subject).not_to be_allowed }
         end
 
-        context 'when unauthorized' do
-          context 'to public project' do
-            let(:project) { create(:project, :public) }
+        context 'to internal project' do
+          let(:project) { create(:project, :internal) }
 
-            it { expect(subject).not_to be_allowed }
-          end
+          it { expect(subject).not_to be_allowed }
+        end
 
-          context 'to internal project' do
-            let(:project) { create(:project, :internal) }
+        context 'to private project' do
+          let(:project) { create(:project, :internal) }
 
-            it { expect(subject).not_to be_allowed }
-          end
-
-          context 'to private project' do
-            let(:project) { create(:project, :internal) }
-
-            it { expect(subject).not_to be_allowed }
-          end
+          it { expect(subject).not_to be_allowed }
         end
       end
     end
diff --git a/spec/lib/gitlab/user_access_spec.rb b/spec/lib/gitlab/user_access_spec.rb
index aa9ec243498..5bb095366fa 100644
--- a/spec/lib/gitlab/user_access_spec.rb
+++ b/spec/lib/gitlab/user_access_spec.rb
@@ -44,7 +44,7 @@ describe Gitlab::UserAccess, lib: true do
 
     describe 'push to protected branch if allowed for developers' do
       before do
-        @branch = create :protected_branch, project: project, developers_can_push: true
+        @branch = create :protected_branch, :developers_can_push, project: project
       end
 
       it 'returns true if user is a master' do
@@ -65,7 +65,7 @@ describe Gitlab::UserAccess, lib: true do
 
     describe 'merge to protected branch if allowed for developers' do
       before do
-        @branch = create :protected_branch, project: project, developers_can_merge: true
+        @branch = create :protected_branch, :developers_can_merge, project: project
       end
 
       it 'returns true if user is a master' do

From 12387b4d2c6abbe1de2fc6b0776207d9135c29f0 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 8 Jul 2016 13:00:38 +0530
Subject: [PATCH 07/23] Allow setting "Allowed To Push/Merge" while creating a
 protected branch.

1. Reuse the same dropdown component that we used for updating these
   settings (`ProtectedBranchesAccessSelect`). Have it accept options
   for the parent container (so we can control the elements it sees) and
   whether or not to save changes via AJAX (we need this for update, but
   not create).

2. Change the "Developers" option to "Developers + Masters", which is
   clearer.

3. Remove `developers_can_push` and `developers_can_merge` from the
   model, since they're not needed anymore.
---
 ...protected_branches_access_select.js.coffee | 37 ++++++++++---------
 app/assets/stylesheets/pages/projects.scss    | 11 ++++++
 app/models/protected_branch.rb                |  8 ++--
 .../_branches_list.html.haml                  |  2 +-
 .../_protected_branch.html.haml               |  8 ++--
 .../protected_branches/index.html.haml        | 29 ++++++++++-----
 6 files changed, 58 insertions(+), 37 deletions(-)

diff --git a/app/assets/javascripts/protected_branches_access_select.js.coffee b/app/assets/javascripts/protected_branches_access_select.js.coffee
index b472ff7ec27..7c6f2f9f38e 100644
--- a/app/assets/javascripts/protected_branches_access_select.js.coffee
+++ b/app/assets/javascripts/protected_branches_access_select.js.coffee
@@ -1,18 +1,18 @@
 class @ProtectedBranchesAccessSelect
-  constructor: () ->
-    $(".allowed-to-merge").each (i, element) =>
+  constructor: (@container, @saveOnSelect) ->
+    @container.find(".allowed-to-merge").each (i, element) =>
       fieldName = $(element).data('field-name')
       $(element).glDropdown
-        data: [{id: 'developers', text: 'Developers'}, {id: 'masters', text: 'Masters'}]
+        data: [{id: 'developers', text: 'Developers + Masters'}, {id: 'masters', text: 'Masters'}]
         selectable: true
         fieldName: fieldName
         clicked: _.partial(@onSelect, element)
 
-    $(".allowed-to-push").each (i, element) =>
+    @container.find(".allowed-to-push").each (i, element) =>
       fieldName = $(element).data('field-name')
       $(element).glDropdown
         data: [{id: 'no_one', text: 'No one'},
-               {id: 'developers', text: 'Developers'},
+               {id: 'developers', text: 'Developers + Masters'},
                {id: 'masters', text: 'Masters'}]
         selectable: true
         fieldName: fieldName
@@ -21,19 +21,20 @@ class @ProtectedBranchesAccessSelect
 
   onSelect: (dropdown, selected, element, e) =>
     $(dropdown).find('.dropdown-toggle-text').text(selected.text)
-    $.ajax
-      type: "PATCH"
-      url: $(dropdown).data('url')
-      dataType: "json"
-      data:
-        id: $(dropdown).data('id')
-        protected_branch:
-          "#{$(dropdown).data('type')}": selected.id
+    if @saveOnSelect
+      $.ajax
+        type: "PATCH"
+        url: $(dropdown).data('url')
+        dataType: "json"
+        data:
+          id: $(dropdown).data('id')
+          protected_branch:
+            "#{$(dropdown).data('type')}": selected.id
 
-      success: ->
-        row = $(e.target)
-        row.closest('tr').effect('highlight')
+        success: ->
+          row = $(e.target)
+          row.closest('tr').effect('highlight')
 
-      error: ->
-        new Flash("Failed to update branch!", "alert")
+        error: ->
+          new Flash("Failed to update branch!", "alert")
 
diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss
index c6e73dd1f39..4409477916f 100644
--- a/app/assets/stylesheets/pages/projects.scss
+++ b/app/assets/stylesheets/pages/projects.scss
@@ -661,6 +661,17 @@ pre.light-well {
   }
 }
 
+.new_protected_branch {
+  .dropdown {
+    display: inline;
+    margin-left: 15px;
+  }
+
+  label {
+    min-width: 120px;
+  }
+}
+
 .protected-branches-list {
   a {
     color: $gl-gray;
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index b0fde6c6c1b..c0bee72b4d7 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -12,12 +12,12 @@ class ProtectedBranch < ActiveRecord::Base
     project.commit(self.name)
   end
 
-  def developers_can_push
-    self.push_access_level && self.push_access_level.developers?
+  def allowed_to_push
+    self.push_access_level && self.push_access_level.access_level
   end
 
-  def developers_can_merge
-    self.merge_access_level && self.merge_access_level.developers?
+  def allowed_to_merge
+    self.merge_access_level && self.merge_access_level.access_level
   end
 
   # Returns all protected branches that match the given branch name.
diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml
index 5f9f2992dca..9240f1cf92d 100644
--- a/app/views/projects/protected_branches/_branches_list.html.haml
+++ b/app/views/projects/protected_branches/_branches_list.html.haml
@@ -26,4 +26,4 @@
   = paginate @protected_branches, theme: 'gitlab'
 
 :javascript
-  new ProtectedBranchesAccessSelect();
+  new ProtectedBranchesAccessSelect($(".protected-branches-list"), true);
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
index ceae182e82c..96533b141af 100644
--- a/app/views/projects/protected_branches/_protected_branch.html.haml
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -15,11 +15,11 @@
       - else
         (branch was removed from repository)
   %td
-    = hidden_field_tag "allowed_to_merge_#{branch.id}", branch.merge_access_level.access_level
-    = dropdown_tag(branch.merge_access_level.access_level.humanize, options: { title: "Allowed To Merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "allowed_to_merge_#{branch.id}", url: @url, id: branch.id, type: "allowed_to_merge" }})
+    = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level
+    = dropdown_tag(protected_branch.merge_access_level.access_level.humanize, options: { title: "Allowed To Merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_merge" }})
   %td
-    = hidden_field_tag "allowed_to_push_#{branch.id}", branch.push_access_level.access_level
-    = dropdown_tag(branch.push_access_level.access_level.humanize, options: { title: "Allowed To Push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "allowed_to_push_#{branch.id}", url: @url, id: branch.id, type: "allowed_to_push" }})
+    = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level
+    = dropdown_tag(protected_branch.push_access_level.access_level.humanize, options: { title: "Allowed To Push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_push" }})
   - if can_admin_project
     %td
       = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right"
diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml
index 950df740bbc..cd87f978fb2 100644
--- a/app/views/projects/protected_branches/index.html.haml
+++ b/app/views/projects/protected_branches/index.html.haml
@@ -32,19 +32,28 @@
             are supported.
 
         .form-group
-          = f.check_box :developers_can_push, class: "pull-left"
-          .prepend-left-20
-            = f.label :developers_can_push, "Developers can push", class: "label-light append-bottom-0"
-            %p.light.append-bottom-0
-              Allow developers to push to this branch
+          .prepend-left-10
+            = f.hidden_field :allowed_to_merge
+            = f.label :allowed_to_merge, "Allowed to Merge: ", class: "label-light append-bottom-0"
+            = dropdown_tag("<Make a selection>",
+                           options: { title: "Allowed To Merge", toggle_class: 'allowed-to-merge',
+                                      dropdown_class: 'dropdown-menu-selectable',
+                                      data: { field_name: "protected_branch[allowed_to_merge]" }})
 
         .form-group
-          = f.check_box :developers_can_merge, class: "pull-left"
-          .prepend-left-20
-            = f.label :developers_can_merge, "Developers can merge", class: "label-light append-bottom-0"
-            %p.light.append-bottom-0
-              Allow developers to accept merge requests to this branch
+          .prepend-left-10
+            = f.hidden_field :allowed_to_push
+            = f.label :allowed_to_push, "Allowed to Push: ", class: "label-light append-bottom-0"
+            = dropdown_tag("<Make a selection>",
+                           options: { title: "Allowed To Push", toggle_class: 'allowed-to-push',
+                                      dropdown_class: 'dropdown-menu-selectable',
+                                      data: { field_name: "protected_branch[allowed_to_push]" }})
+
+
         = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true
 
     %hr
     = render "branches_list"
+
+:javascript
+  new ProtectedBranchesAccessSelect($(".new_protected_branch"), false);

From 9fa661472e5e1e2edc91032a6093a3516974e27e Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 8 Jul 2016 14:18:50 +0530
Subject: [PATCH 08/23] Update protected branches spec to work with the
 `select`s.

1. Get the existing spec passing.

2. Add specs for all the access control options, both while creating and
   updating protected branches.

3. Show a flash notice when updating protected branches, primarily so
   the spec knows when the update is done.
---
 ...protected_branches_access_select.js.coffee |  1 +
 .../_protected_branch.html.haml               |  8 +-
 spec/features/protected_branches_spec.rb      | 75 +++++++++++++++++++
 3 files changed, 82 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/protected_branches_access_select.js.coffee b/app/assets/javascripts/protected_branches_access_select.js.coffee
index 7c6f2f9f38e..6df11146ba9 100644
--- a/app/assets/javascripts/protected_branches_access_select.js.coffee
+++ b/app/assets/javascripts/protected_branches_access_select.js.coffee
@@ -34,6 +34,7 @@ class @ProtectedBranchesAccessSelect
         success: ->
           row = $(e.target)
           row.closest('tr').effect('highlight')
+          new Flash("Updated protected branch!", "notice")
 
         error: ->
           new Flash("Failed to update branch!", "alert")
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
index 96533b141af..89d606d9e20 100644
--- a/app/views/projects/protected_branches/_protected_branch.html.haml
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -16,10 +16,14 @@
         (branch was removed from repository)
   %td
     = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level
-    = dropdown_tag(protected_branch.merge_access_level.access_level.humanize, options: { title: "Allowed To Merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_merge" }})
+    = dropdown_tag(protected_branch.merge_access_level.access_level.humanize,
+                   options: { title: "Allowed To Merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable merge',
+                   data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_merge" }})
   %td
     = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level
-    = dropdown_tag(protected_branch.push_access_level.access_level.humanize, options: { title: "Allowed To Push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable', data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_push" }})
+    = dropdown_tag(protected_branch.push_access_level.access_level.humanize,
+                   options: { title: "Allowed To Push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable push',
+                   data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_push" }})
   - if can_admin_project
     %td
       = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right"
diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb
index d94dee0c797..087e3677169 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -81,4 +81,79 @@ feature 'Projected Branches', feature: true, js: true do
       end
     end
   end
+
+  describe "access control" do
+    [
+      ['developers', 'Developers + Masters'],
+      ['masters', 'Masters'],
+      ['no_one', 'No one']
+    ].each do |access_type_id, access_type_name|
+      it "allows creating protected branches that #{access_type_name} can push to" do
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('master')
+        within('.new_protected_branch') do
+          find(".allowed-to-push").click
+          click_on access_type_name
+        end
+        click_on "Protect"
+
+        expect(ProtectedBranch.count).to eq(1)
+        expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
+      end
+
+      # This spec fails on PhantomJS versions below 2.0, which don't support `PATCH` requests.
+      # https://github.com/ariya/phantomjs/issues/11384
+      it "allows updating protected branches so that #{access_type_name} can push to them" do
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('master')
+        click_on "Protect"
+
+        expect(ProtectedBranch.count).to eq(1)
+
+        within(".protected-branches-list") do
+          find(".allowed-to-push").click
+          within('.dropdown-menu.push') { click_on access_type_name }
+        end
+
+        expect(page).to have_content "Updated protected branch"
+        expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
+      end
+    end
+
+    [
+      ['developers', 'Developers + Masters'],
+      ['masters', 'Masters']
+    ].each do |access_type_id, access_type_name|
+      it "allows creating protected branches that #{access_type_name} can merge to" do
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('master')
+        within('.new_protected_branch') do
+          find(".allowed-to-merge").click
+          click_on access_type_name
+        end
+        click_on "Protect"
+
+        expect(ProtectedBranch.count).to eq(1)
+        expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
+      end
+
+      # This spec fails on PhantomJS versions below 2.0, which don't support `PATCH` requests.
+      # https://github.com/ariya/phantomjs/issues/11384
+      it "allows updating protected branches so that #{access_type_name} can merge to them" do
+        visit namespace_project_protected_branches_path(project.namespace, project)
+        set_protected_branch_name('master')
+        click_on "Protect"
+
+        expect(ProtectedBranch.count).to eq(1)
+
+        within(".protected-branches-list") do
+          find(".allowed-to-merge").click
+          within('.dropdown-menu.merge') { click_on access_type_name }
+        end
+
+        expect(page).to have_content "Updated protected branch"
+        expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
+      end
+    end
+  end
 end

From a9958ddc7c9d4c455d4c5459b7b83da1fab9ccb4 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 8 Jul 2016 14:45:29 +0530
Subject: [PATCH 09/23] Fix default branch protection.

1. So it works with the new data model for protected branch access levels.
---
 app/services/git_push_service.rb                |  8 +++++---
 .../protected_branches/create_service.rb        |  2 +-
 .../protected_branches/update_service.rb        |  2 +-
 spec/services/git_push_service_spec.rb          | 17 ++++++++++++-----
 4 files changed, 19 insertions(+), 10 deletions(-)

diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index e02b50ff9a2..604737e6934 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -88,9 +88,11 @@ class GitPushService < BaseService
 
     # Set protection on the default branch if configured
     if current_application_settings.default_branch_protection != PROTECTION_NONE
-      developers_can_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? true : false
-      developers_can_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? true : false
-      @project.protected_branches.create({ name: @project.default_branch, developers_can_push: developers_can_push, developers_can_merge: developers_can_merge })
+      allowed_to_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? 'developers' : 'masters'
+      allowed_to_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? 'developers' : 'masters'
+
+      params = { name: @project.default_branch, allowed_to_push: allowed_to_push, allowed_to_merge: allowed_to_merge }
+      ProtectedBranches::CreateService.new(@project, current_user, params).execute
     end
   end
 
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
index ab462f3054e..212c2134638 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -1,5 +1,5 @@
 module ProtectedBranches
-  class CreateService < BaseService
+  class CreateService < ProtectedBranches::BaseService
     attr_reader :protected_branch
 
     def execute
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
index ed59b06b79a..4a2b1be9c93 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -1,5 +1,5 @@
 module ProtectedBranches
-  class UpdateService < BaseService
+  class UpdateService < ProtectedBranches::BaseService
     attr_reader :protected_branch
 
     def initialize(project, current_user, id, params = {})
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 47c0580e0f0..663c270d61f 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -224,8 +224,10 @@ describe GitPushService, services: true do
       it "when pushing a branch for the first time" do
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: false })
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        expect(project.protected_branches).not_to be_empty
+        expect(project.protected_branches.first.allowed_to_push).to eq('masters')
+        expect(project.protected_branches.first.allowed_to_merge).to eq('masters')
       end
 
       it "when pushing a branch for the first time with default branch protection disabled" do
@@ -233,8 +235,8 @@ describe GitPushService, services: true do
 
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        expect(project.protected_branches).not_to receive(:create)
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        expect(project.protected_branches).to be_empty
       end
 
       it "when pushing a branch for the first time with default branch protection set to 'developers can push'" do
@@ -242,9 +244,12 @@ describe GitPushService, services: true do
 
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: true, developers_can_merge: false })
 
-        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master')
+        execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+
+        expect(project.protected_branches).not_to be_empty
+        expect(project.protected_branches.last.allowed_to_push).to eq('developers')
+        expect(project.protected_branches.last.allowed_to_merge).to eq('masters')
       end
 
       it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do
@@ -252,8 +257,10 @@ describe GitPushService, services: true do
 
         expect(project).to receive(:execute_hooks)
         expect(project.default_branch).to eq("master")
-        expect(project.protected_branches).to receive(:create).with({ name: "master", developers_can_push: false, developers_can_merge: true })
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
+        expect(project.protected_branches).not_to be_empty
+        expect(project.protected_branches.first.allowed_to_push).to eq('masters')
+        expect(project.protected_branches.first.allowed_to_merge).to eq('developers')
       end
 
       it "when pushing new commits to existing branch" do

From c647540c1010fd1e51bced1db90947aa00c83fa8 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 8 Jul 2016 14:46:13 +0530
Subject: [PATCH 10/23] Fix all specs related to changes in !5081.

1. Remove `Project#developers_can_push_to_protected_branch?` since it
   isn't used anymore.

2. Remove `Project#developers_can_merge_to_protected_branch?` since it
   isn't used anymore.
---
 app/models/project.rb                         |  8 ----
 .../protected_branch/merge_access_level.rb    |  2 +-
 .../protected_branch/push_access_level.rb     |  2 +-
 features/steps/project/commits/branches.rb    |  2 +-
 spec/lib/gitlab/git_access_spec.rb            |  2 +-
 spec/models/project_spec.rb                   | 40 -------------------
 6 files changed, 4 insertions(+), 52 deletions(-)

diff --git a/app/models/project.rb b/app/models/project.rb
index dc44a757b4b..7aecd7860c5 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -874,14 +874,6 @@ class Project < ActiveRecord::Base
     ProtectedBranch.matching(branch_name, protected_branches: @protected_branches).present?
   end
 
-  def developers_can_push_to_protected_branch?(branch_name)
-    protected_branches.matching(branch_name).any?(&:developers_can_push)
-  end
-
-  def developers_can_merge_to_protected_branch?(branch_name)
-    protected_branches.matching(branch_name).any?(&:developers_can_merge)
-  end
-
   def forked?
     !(forked_project_link.nil? || forked_project_link.forked_from_project.nil?)
   end
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index 2d13d8c8381..3d2a6971702 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -8,7 +8,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
     if masters?
       user.can?(:push_code, project) if project.team.master?(user)
     elsif developers?
-      user.can?(:push_code, project) if (project.team.master?(user) || project.team.developer?(user))
+      user.can?(:push_code, project) if project.team.master?(user) || project.team.developer?(user)
     end
   end
 end
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index 5a4a33556ce..d446c1a03f0 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -8,7 +8,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
     if masters?
       user.can?(:push_code, project) if project.team.master?(user)
     elsif developers?
-      user.can?(:push_code, project) if (project.team.master?(user) || project.team.developer?(user))
+      user.can?(:push_code, project) if project.team.master?(user) || project.team.developer?(user)
     elsif no_one?
       false
     end
diff --git a/features/steps/project/commits/branches.rb b/features/steps/project/commits/branches.rb
index 0a42931147d..4bfb7e92e99 100644
--- a/features/steps/project/commits/branches.rb
+++ b/features/steps/project/commits/branches.rb
@@ -25,7 +25,7 @@ class Spinach::Features::ProjectCommitsBranches < Spinach::FeatureSteps
 
   step 'project "Shop" has protected branches' do
     project = Project.find_by(name: "Shop")
-    project.protected_branches.create(name: "stable")
+    create(:protected_branch, project: project, name: "stable")
   end
 
   step 'I click new branch link' do
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 324bb500025..8d7497f76f5 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -230,7 +230,7 @@ describe Gitlab::GitAccess, lib: true do
           context "when the merge request is in progress" do
             before do
               create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature',
-                     state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
+                                     state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
             end
 
             context "when the merge request is not in progress" do
diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb
index 72b8a4e25bd..e365e4e98b2 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1095,46 +1095,6 @@ describe Project, models: true do
     end
   end
 
-  describe "#developers_can_push_to_protected_branch?" do
-    let(:project) { create(:empty_project) }
-
-    context "when the branch matches a protected branch via direct match" do
-      it "returns true if 'Developers can Push' is turned on" do
-        create(:protected_branch, name: "production", project: project, developers_can_push: true)
-
-        expect(project.developers_can_push_to_protected_branch?('production')).to be true
-      end
-
-      it "returns false if 'Developers can Push' is turned off" do
-        create(:protected_branch, name: "production", project: project, developers_can_push: false)
-
-        expect(project.developers_can_push_to_protected_branch?('production')).to be false
-      end
-    end
-
-    context "when the branch matches a protected branch via wilcard match" do
-      it "returns true if 'Developers can Push' is turned on" do
-        create(:protected_branch, name: "production/*", project: project, developers_can_push: true)
-
-        expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be true
-      end
-
-      it "returns false if 'Developers can Push' is turned off" do
-        create(:protected_branch, name: "production/*", project: project, developers_can_push: false)
-
-        expect(project.developers_can_push_to_protected_branch?('production/some-branch')).to be false
-      end
-    end
-
-    context "when the branch does not match a protected branch" do
-      it "returns false" do
-        create(:protected_branch, name: "production/*", project: project, developers_can_push: true)
-
-        expect(project.developers_can_push_to_protected_branch?('staging/some-branch')).to be false
-      end
-    end
-  end
-
   describe '#container_registry_path_with_namespace' do
     let(:project) { create(:empty_project, path: 'PROJECT') }
 

From f2df2966aabc601dd1d6a6f9e75ead84db8a2765 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 14 Jul 2016 09:12:16 +0530
Subject: [PATCH 11/23] Humanize protected branches' access levels at one
 location.

1. The model now contains this humanization data, which is the once
   source of truth.

2. Previously, this was being listed out in the dropdown component as well.
---
 .../protected_branches_access_select.js.coffee       |  6 ++----
 .../projects/protected_branches_controller.rb        |  4 +++-
 app/models/protected_branch/merge_access_level.rb    | 11 +++++++++++
 app/models/protected_branch/push_access_level.rb     | 12 ++++++++++++
 .../protected_branches/_protected_branch.html.haml   |  4 ++--
 spec/features/protected_branches_spec.rb             | 11 ++---------
 6 files changed, 32 insertions(+), 16 deletions(-)

diff --git a/app/assets/javascripts/protected_branches_access_select.js.coffee b/app/assets/javascripts/protected_branches_access_select.js.coffee
index 6df11146ba9..2c29513ae61 100644
--- a/app/assets/javascripts/protected_branches_access_select.js.coffee
+++ b/app/assets/javascripts/protected_branches_access_select.js.coffee
@@ -3,7 +3,7 @@ class @ProtectedBranchesAccessSelect
     @container.find(".allowed-to-merge").each (i, element) =>
       fieldName = $(element).data('field-name')
       $(element).glDropdown
-        data: [{id: 'developers', text: 'Developers + Masters'}, {id: 'masters', text: 'Masters'}]
+        data: gon.merge_access_levels
         selectable: true
         fieldName: fieldName
         clicked: _.partial(@onSelect, element)
@@ -11,9 +11,7 @@ class @ProtectedBranchesAccessSelect
     @container.find(".allowed-to-push").each (i, element) =>
       fieldName = $(element).data('field-name')
       $(element).glDropdown
-        data: [{id: 'no_one', text: 'No one'},
-               {id: 'developers', text: 'Developers + Masters'},
-               {id: 'masters', text: 'Masters'}]
+        data: gon.push_access_levels
         selectable: true
         fieldName: fieldName
         clicked: _.partial(@onSelect, element)
diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index 126358bfe77..ddf1824ccb9 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -9,7 +9,9 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
 
   def index
     @protected_branch = @project.protected_branches.new
-    gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } })
+    gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } },
+               push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
+               merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } })
   end
 
   def create
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index 3d2a6971702..d536f816317 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -4,6 +4,13 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
 
   enum access_level: [:masters, :developers]
 
+  def self.human_access_levels
+    {
+      "masters" => "Masters",
+      "developers" => "Developers + Masters"
+    }.with_indifferent_access
+  end
+
   def check_access(user)
     if masters?
       user.can?(:push_code, project) if project.team.master?(user)
@@ -11,4 +18,8 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
       user.can?(:push_code, project) if project.team.master?(user) || project.team.developer?(user)
     end
   end
+
+  def humanize
+    self.class.human_access_levels[self.access_level]
+  end
 end
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index d446c1a03f0..bb46b39b714 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -4,6 +4,14 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
 
   enum access_level: [:masters, :developers, :no_one]
 
+  def self.human_access_levels
+    {
+      "masters" => "Masters",
+      "developers" => "Developers + Masters",
+      "no_one" => "No one"
+    }.with_indifferent_access
+  end
+
   def check_access(user)
     if masters?
       user.can?(:push_code, project) if project.team.master?(user)
@@ -13,4 +21,8 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
       false
     end
   end
+
+  def humanize
+    self.class.human_access_levels[self.access_level]
+  end
 end
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
index 89d606d9e20..e27dea8145d 100644
--- a/app/views/projects/protected_branches/_protected_branch.html.haml
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -16,12 +16,12 @@
         (branch was removed from repository)
   %td
     = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level
-    = dropdown_tag(protected_branch.merge_access_level.access_level.humanize,
+    = dropdown_tag(protected_branch.merge_access_level.humanize,
                    options: { title: "Allowed To Merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable merge',
                    data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_merge" }})
   %td
     = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level
-    = dropdown_tag(protected_branch.push_access_level.access_level.humanize,
+    = dropdown_tag(protected_branch.push_access_level.humanize,
                    options: { title: "Allowed To Push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable push',
                    data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_push" }})
   - if can_admin_project
diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb
index 087e3677169..553d1c70461 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -83,11 +83,7 @@ feature 'Projected Branches', feature: true, js: true do
   end
 
   describe "access control" do
-    [
-      ['developers', 'Developers + Masters'],
-      ['masters', 'Masters'],
-      ['no_one', 'No one']
-    ].each do |access_type_id, access_type_name|
+    ProtectedBranch::PushAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
       it "allows creating protected branches that #{access_type_name} can push to" do
         visit namespace_project_protected_branches_path(project.namespace, project)
         set_protected_branch_name('master')
@@ -120,10 +116,7 @@ feature 'Projected Branches', feature: true, js: true do
       end
     end
 
-    [
-      ['developers', 'Developers + Masters'],
-      ['masters', 'Masters']
-    ].each do |access_type_id, access_type_name|
+    ProtectedBranch::MergeAccessLevel.human_access_levels.each do |(access_type_id, access_type_name)|
       it "allows creating protected branches that #{access_type_name} can merge to" do
         visit namespace_project_protected_branches_path(project.namespace, project)
         set_protected_branch_name('master')

From 4d6dadc8f8af986a0792fb388775a174e76b0b7d Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 14 Jul 2016 09:24:59 +0530
Subject: [PATCH 12/23] Make specs compatible with PhantomJS versions < 2.

1. These versions of PhantomJS don't support `PATCH` requests, so we use
   a `POST` with `_method` set to `PATCH`.
---
 .../javascripts/protected_branches_access_select.js.coffee    | 3 ++-
 spec/features/protected_branches_spec.rb                      | 4 ----
 2 files changed, 2 insertions(+), 5 deletions(-)

diff --git a/app/assets/javascripts/protected_branches_access_select.js.coffee b/app/assets/javascripts/protected_branches_access_select.js.coffee
index 2c29513ae61..a4d9b6eb616 100644
--- a/app/assets/javascripts/protected_branches_access_select.js.coffee
+++ b/app/assets/javascripts/protected_branches_access_select.js.coffee
@@ -21,10 +21,11 @@ class @ProtectedBranchesAccessSelect
     $(dropdown).find('.dropdown-toggle-text').text(selected.text)
     if @saveOnSelect
       $.ajax
-        type: "PATCH"
+        type: "POST"
         url: $(dropdown).data('url')
         dataType: "json"
         data:
+          _method: 'PATCH'
           id: $(dropdown).data('id')
           protected_branch:
             "#{$(dropdown).data('type')}": selected.id
diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb
index 553d1c70461..d72b62a4962 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -97,8 +97,6 @@ feature 'Projected Branches', feature: true, js: true do
         expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
       end
 
-      # This spec fails on PhantomJS versions below 2.0, which don't support `PATCH` requests.
-      # https://github.com/ariya/phantomjs/issues/11384
       it "allows updating protected branches so that #{access_type_name} can push to them" do
         visit namespace_project_protected_branches_path(project.namespace, project)
         set_protected_branch_name('master')
@@ -130,8 +128,6 @@ feature 'Projected Branches', feature: true, js: true do
         expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
       end
 
-      # This spec fails on PhantomJS versions below 2.0, which don't support `PATCH` requests.
-      # https://github.com/ariya/phantomjs/issues/11384
       it "allows updating protected branches so that #{access_type_name} can merge to them" do
         visit namespace_project_protected_branches_path(project.namespace, project)
         set_protected_branch_name('master')

From cc1cebdcc536244d97bdf6c767c2f1875c71cdf5 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 14 Jul 2016 11:46:13 +0530
Subject: [PATCH 13/23] Admins count as masters too.

1. In the context of protected branches.

2. Test this behaviour.
---
 app/models/project_team.rb                    |  8 +++++
 .../protected_branch/merge_access_level.rb    |  4 +--
 .../protected_branch/push_access_level.rb     |  4 +--
 spec/lib/gitlab/git_access_spec.rb            | 30 +++++++++++++++----
 4 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index fdfaf052730..436d5bd2948 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -118,6 +118,14 @@ class ProjectTeam
     max_member_access(user.id) == Gitlab::Access::MASTER
   end
 
+  def master_or_greater?(user)
+    master?(user) || user.is_admin?
+  end
+
+  def developer_or_greater?(user)
+    master_or_greater?(user) || developer?(user)
+  end
+
   def member?(user, min_member_access = nil)
     member = !!find_member(user.id)
 
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index d536f816317..632e47b028f 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -13,9 +13,9 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
 
   def check_access(user)
     if masters?
-      user.can?(:push_code, project) if project.team.master?(user)
+      user.can?(:push_code, project) if project.team.master_or_greater?(user)
     elsif developers?
-      user.can?(:push_code, project) if project.team.master?(user) || project.team.developer?(user)
+      user.can?(:push_code, project) if project.team.developer_or_greater?(user)
     end
   end
 
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index bb46b39b714..35d4ad93231 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -14,9 +14,9 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
 
   def check_access(user)
     if masters?
-      user.can?(:push_code, project) if project.team.master?(user)
+      user.can?(:push_code, project) if project.team.master_or_greater?(user)
     elsif developers?
-      user.can?(:push_code, project) if project.team.master?(user) || project.team.developer?(user)
+      user.can?(:push_code, project) if project.team.developer_or_greater?(user)
     elsif no_one?
       false
     end
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 8d7497f76f5..c6f03525aab 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -151,7 +151,13 @@ describe Gitlab::GitAccess, lib: true do
     def self.run_permission_checks(permissions_matrix)
       permissions_matrix.keys.each do |role|
         describe "#{role} access" do
-          before { project.team << [user, role] }
+          before do
+            if role == :admin
+              user.update_attribute(:admin, true)
+            else
+              project.team << [user, role]
+            end
+          end
 
           permissions_matrix[role].each do |action, allowed|
             context action do
@@ -165,6 +171,17 @@ describe Gitlab::GitAccess, lib: true do
     end
 
     permissions_matrix = {
+      admin: {
+        push_new_branch: true,
+        push_master: true,
+        push_protected_branch: true,
+        push_remove_protected_branch: false,
+        push_tag: true,
+        push_new_tag: true,
+        push_all: true,
+        merge_into_protected_branch: true
+      },
+
       master: {
         push_new_branch: true,
         push_master: true,
@@ -257,13 +274,14 @@ describe Gitlab::GitAccess, lib: true do
 
         run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: true, push_all: true, merge_into_protected_branch: true }))
       end
-    end
 
-    context "when no one is allowed to push to the #{protected_branch_name} protected branch" do
-      before { create(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
+      context "when no one is allowed to push to the #{protected_branch_name} protected branch" do
+        before { create(:protected_branch, :no_one_can_push, name: protected_branch_name, project: project) }
 
-      run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
-                                                          master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
+        run_permission_checks(permissions_matrix.deep_merge(developer: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
+                                                            master: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false },
+                                                            admin: { push_protected_branch: false, push_all: false, merge_into_protected_branch: false }))
+      end
     end
   end
 

From 8e25ddc529e41d8244c9e7b4adcf54e071b8c318 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Thu, 14 Jul 2016 11:46:58 +0530
Subject: [PATCH 14/23] Add changelog entry.

---
 CHANGELOG | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/CHANGELOG b/CHANGELOG
index d555d23860d..c791776a891 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -8,6 +8,7 @@ v 8.11.0 (unreleased)
   - Fix of 'Commits being passed to custom hooks are already reachable when using the UI'
   - Add support for using RequestStore within Sidekiq tasks via SIDEKIQ_REQUEST_STORE env variable
   - Optimize maximum user access level lookup in loading of notes
+  - Add "No one can push" as an option for protected branches. !5081
   - Limit git rev-list output count to one in forced push check
   - Clean up unused routes (Josef Strzibny)
   - Add green outline to New Branch button. !5447 (winniehell)
@@ -126,6 +127,8 @@ v 8.10.0
   - Allow to define manual actions/builds on Pipelines and Environments
   - Fix pagination when sorting by columns with lots of ties (like priority)
   - The Markdown reference parsers now re-use query results to prevent running the same queries multiple times. !5020
+  - The Markdown reference parsers now re-use query results to prevent running the same queries multiple times !5020
+  - Add "No one can push" as an option for protected branches. !5081
   - Updated project header design
   - Issuable collapsed assignee tooltip is now the users name
   - Fix compare view not changing code view rendering style

From b3a29b3180c4edda33d82fc3564bd4991831e06c Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Tue, 19 Jul 2016 17:46:57 +0530
Subject: [PATCH 15/23] Favor labels like `Allowed to push` over `Allowed To
 Push`.

- Based on feedback from @axil
- http://docs.gitlab.com/ce/development/ui_guide.html#buttons
---
 .../projects/protected_branches/_branches_list.html.haml  | 4 ++--
 .../protected_branches/_protected_branch.html.haml        | 4 ++--
 app/views/projects/protected_branches/index.html.haml     | 8 ++++----
 3 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml
index 9240f1cf92d..a6956c8e69f 100644
--- a/app/views/projects/protected_branches/_branches_list.html.haml
+++ b/app/views/projects/protected_branches/_branches_list.html.haml
@@ -16,8 +16,8 @@
       %tr
         %th Branch
         %th Last commit
-        %th Allowed to Merge
-        %th Allowed to Push
+        %th Allowed to merge
+        %th Allowed to push
         - if can_admin_project
           %th
     %tbody
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
index e27dea8145d..2fc6081e448 100644
--- a/app/views/projects/protected_branches/_protected_branch.html.haml
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -17,12 +17,12 @@
   %td
     = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level
     = dropdown_tag(protected_branch.merge_access_level.humanize,
-                   options: { title: "Allowed To Merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable merge',
+                   options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable merge',
                    data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_merge" }})
   %td
     = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level
     = dropdown_tag(protected_branch.push_access_level.humanize,
-                   options: { title: "Allowed To Push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable push',
+                   options: { title: "Allowed to push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable push',
                    data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_push" }})
   - if can_admin_project
     %td
diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml
index cd87f978fb2..69caed7d979 100644
--- a/app/views/projects/protected_branches/index.html.haml
+++ b/app/views/projects/protected_branches/index.html.haml
@@ -34,18 +34,18 @@
         .form-group
           .prepend-left-10
             = f.hidden_field :allowed_to_merge
-            = f.label :allowed_to_merge, "Allowed to Merge: ", class: "label-light append-bottom-0"
+            = f.label :allowed_to_merge, "Allowed to merge: ", class: "label-light append-bottom-0"
             = dropdown_tag("<Make a selection>",
-                           options: { title: "Allowed To Merge", toggle_class: 'allowed-to-merge',
+                           options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge',
                                       dropdown_class: 'dropdown-menu-selectable',
                                       data: { field_name: "protected_branch[allowed_to_merge]" }})
 
         .form-group
           .prepend-left-10
             = f.hidden_field :allowed_to_push
-            = f.label :allowed_to_push, "Allowed to Push: ", class: "label-light append-bottom-0"
+            = f.label :allowed_to_push, "Allowed to push: ", class: "label-light append-bottom-0"
             = dropdown_tag("<Make a selection>",
-                           options: { title: "Allowed To Push", toggle_class: 'allowed-to-push',
+                           options: { title: "Allowed to push", toggle_class: 'allowed-to-push',
                                       dropdown_class: 'dropdown-menu-selectable',
                                       data: { field_name: "protected_branch[allowed_to_push]" }})
 

From 7b2ad2d5b99d84fc2d2c11a654085afc02a05bb1 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 25 Jul 2016 14:42:52 +0530
Subject: [PATCH 16/23] Implement review comments from @dbalexandre.

1. Remove `master_or_greater?` and `developer_or_greater?` in favor of
   `max_member_access`, which is a lot nicer.

2. Remove a number of instances of `include Gitlab::Database::MigrationHelpers`
   in migrations that don't need this module. Also remove comments where
   not necessary.

3. Remove duplicate entry in CHANGELOG.

4. Move `ProtectedBranchAccessSelect` from Coffeescript to ES6.

5. Split the `set_access_levels!` method in two - one each for `merge` and
   `push` access levels.
---
 CHANGELOG                                     |  1 -
 app/assets/javascripts/protected_branches.js  | 35 ------------
 ...protected_branches_access_select.js.coffee | 40 --------------
 .../protected_branches_access_select.js.es6   | 53 +++++++++++++++++++
 app/models/project_team.rb                    |  8 ---
 .../protected_branch/merge_access_level.rb    | 14 +++--
 .../protected_branch/push_access_level.rb     | 17 +++---
 .../protected_branches/base_service.rb        |  9 ++++
 ...4938_add_protected_branches_push_access.rb |  2 -
 ...952_add_protected_branches_merge_access.rb |  2 -
 ...erge_to_protected_branches_merge_access.rb | 13 -----
 ..._push_to_protected_branches_push_access.rb | 13 -----
 ...lopers_can_push_from_protected_branches.rb | 13 -----
 ...opers_can_merge_from_protected_branches.rb | 13 -----
 14 files changed, 81 insertions(+), 152 deletions(-)
 delete mode 100644 app/assets/javascripts/protected_branches.js
 delete mode 100644 app/assets/javascripts/protected_branches_access_select.js.coffee
 create mode 100644 app/assets/javascripts/protected_branches_access_select.js.es6

diff --git a/CHANGELOG b/CHANGELOG
index c791776a891..4f1da451df0 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -127,7 +127,6 @@ v 8.10.0
   - Allow to define manual actions/builds on Pipelines and Environments
   - Fix pagination when sorting by columns with lots of ties (like priority)
   - The Markdown reference parsers now re-use query results to prevent running the same queries multiple times. !5020
-  - The Markdown reference parsers now re-use query results to prevent running the same queries multiple times !5020
   - Add "No one can push" as an option for protected branches. !5081
   - Updated project header design
   - Issuable collapsed assignee tooltip is now the users name
diff --git a/app/assets/javascripts/protected_branches.js b/app/assets/javascripts/protected_branches.js
deleted file mode 100644
index db21a19964d..00000000000
--- a/app/assets/javascripts/protected_branches.js
+++ /dev/null
@@ -1,35 +0,0 @@
-(function() {
-  $(function() {
-    return $(".protected-branches-list :checkbox").change(function(e) {
-      var can_push, id, name, obj, url;
-      name = $(this).attr("name");
-      if (name === "developers_can_push" || name === "developers_can_merge") {
-        id = $(this).val();
-        can_push = $(this).is(":checked");
-        url = $(this).data("url");
-        return $.ajax({
-          type: "PATCH",
-          url: url,
-          dataType: "json",
-          data: {
-            id: id,
-            protected_branch: (
-              obj = {},
-              obj["" + name] = can_push,
-              obj
-            )
-          },
-          success: function() {
-            var row;
-            row = $(e.target);
-            return row.closest('tr').effect('highlight');
-          },
-          error: function() {
-            return new Flash("Failed to update branch!", "alert");
-          }
-        });
-      }
-    });
-  });
-
-}).call(this);
diff --git a/app/assets/javascripts/protected_branches_access_select.js.coffee b/app/assets/javascripts/protected_branches_access_select.js.coffee
deleted file mode 100644
index a4d9b6eb616..00000000000
--- a/app/assets/javascripts/protected_branches_access_select.js.coffee
+++ /dev/null
@@ -1,40 +0,0 @@
-class @ProtectedBranchesAccessSelect
-  constructor: (@container, @saveOnSelect) ->
-    @container.find(".allowed-to-merge").each (i, element) =>
-      fieldName = $(element).data('field-name')
-      $(element).glDropdown
-        data: gon.merge_access_levels
-        selectable: true
-        fieldName: fieldName
-        clicked: _.partial(@onSelect, element)
-
-    @container.find(".allowed-to-push").each (i, element) =>
-      fieldName = $(element).data('field-name')
-      $(element).glDropdown
-        data: gon.push_access_levels
-        selectable: true
-        fieldName: fieldName
-        clicked: _.partial(@onSelect, element)
-
-
-  onSelect: (dropdown, selected, element, e) =>
-    $(dropdown).find('.dropdown-toggle-text').text(selected.text)
-    if @saveOnSelect
-      $.ajax
-        type: "POST"
-        url: $(dropdown).data('url')
-        dataType: "json"
-        data:
-          _method: 'PATCH'
-          id: $(dropdown).data('id')
-          protected_branch:
-            "#{$(dropdown).data('type')}": selected.id
-
-        success: ->
-          row = $(e.target)
-          row.closest('tr').effect('highlight')
-          new Flash("Updated protected branch!", "notice")
-
-        error: ->
-          new Flash("Failed to update branch!", "alert")
-
diff --git a/app/assets/javascripts/protected_branches_access_select.js.es6 b/app/assets/javascripts/protected_branches_access_select.js.es6
new file mode 100644
index 00000000000..93b7d7755a7
--- /dev/null
+++ b/app/assets/javascripts/protected_branches_access_select.js.es6
@@ -0,0 +1,53 @@
+class ProtectedBranchesAccessSelect {
+  constructor(container, saveOnSelect) {
+    this.container = container;
+    this.saveOnSelect = saveOnSelect;
+
+    this.container.find(".allowed-to-merge").each((i, element) => {
+      var fieldName = $(element).data('field-name');
+      return $(element).glDropdown({
+        data: gon.merge_access_levels,
+        selectable: true,
+        fieldName: fieldName,
+        clicked: _.chain(this.onSelect).partial(element).bind(this).value()
+      });
+    });
+
+
+    this.container.find(".allowed-to-push").each((i, element) => {
+      var fieldName = $(element).data('field-name');
+      return $(element).glDropdown({
+        data: gon.push_access_levels,
+        selectable: true,
+        fieldName: fieldName,
+        clicked: _.chain(this.onSelect).partial(element).bind(this).value()
+      });
+    });
+  }
+
+  onSelect(dropdown, selected, element, e) {
+    $(dropdown).find('.dropdown-toggle-text').text(selected.text);
+    if (this.saveOnSelect) {
+      return $.ajax({
+        type: "POST",
+        url: $(dropdown).data('url'),
+        dataType: "json",
+        data: {
+          _method: 'PATCH',
+          id: $(dropdown).data('id'),
+          protected_branch: {
+            ["" + ($(dropdown).data('type'))]: selected.id
+          }
+        },
+        success: function() {
+          var row;
+          row = $(e.target);
+          return row.closest('tr').effect('highlight');
+        },
+        error: function() {
+          return new Flash("Failed to update branch!", "alert");
+        }
+      });
+    }
+  }
+}
diff --git a/app/models/project_team.rb b/app/models/project_team.rb
index 436d5bd2948..fdfaf052730 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -118,14 +118,6 @@ class ProjectTeam
     max_member_access(user.id) == Gitlab::Access::MASTER
   end
 
-  def master_or_greater?(user)
-    master?(user) || user.is_admin?
-  end
-
-  def developer_or_greater?(user)
-    master_or_greater?(user) || developer?(user)
-  end
-
   def member?(user, min_member_access = nil)
     member = !!find_member(user.id)
 
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index 632e47b028f..17a3a86c3e1 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -12,11 +12,15 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   end
 
   def check_access(user)
-    if masters?
-      user.can?(:push_code, project) if project.team.master_or_greater?(user)
-    elsif developers?
-      user.can?(:push_code, project) if project.team.developer_or_greater?(user)
-    end
+    return true if user.is_admin?
+
+    min_member_access = if masters?
+                          Gitlab::Access::MASTER
+                        elsif developers?
+                          Gitlab::Access::DEVELOPER
+                        end
+
+    project.team.max_member_access(user.id) >= min_member_access
   end
 
   def humanize
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index 35d4ad93231..22096b13300 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -13,13 +13,16 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   end
 
   def check_access(user)
-    if masters?
-      user.can?(:push_code, project) if project.team.master_or_greater?(user)
-    elsif developers?
-      user.can?(:push_code, project) if project.team.developer_or_greater?(user)
-    elsif no_one?
-      false
-    end
+    return false if no_one?
+    return true if user.is_admin?
+
+    min_member_access = if masters?
+                          Gitlab::Access::MASTER
+                        elsif developers?
+                          Gitlab::Access::DEVELOPER
+                        end
+
+    project.team.max_member_access(user.id) >= min_member_access
   end
 
   def humanize
diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb
index 3a7c35327fe..f8741fcb3d5 100644
--- a/app/services/protected_branches/base_service.rb
+++ b/app/services/protected_branches/base_service.rb
@@ -1,13 +1,22 @@
 module ProtectedBranches
   class BaseService < ::BaseService
     def set_access_levels!
+      set_merge_access_levels!
+      set_push_access_levels!
+    end
+
+    protected
+
+    def set_merge_access_levels!
       case params[:allowed_to_merge]
       when 'masters'
         @protected_branch.merge_access_level.masters!
       when 'developers'
         @protected_branch.merge_access_level.developers!
       end
+    end
 
+    def set_push_access_levels!
       case params[:allowed_to_push]
       when 'masters'
         @protected_branch.push_access_level.masters!
diff --git a/db/migrate/20160705054938_add_protected_branches_push_access.rb b/db/migrate/20160705054938_add_protected_branches_push_access.rb
index 512d99e4823..3031574fe2a 100644
--- a/db/migrate/20160705054938_add_protected_branches_push_access.rb
+++ b/db/migrate/20160705054938_add_protected_branches_push_access.rb
@@ -2,8 +2,6 @@
 # for more information on how to write migrations for GitLab.
 
 class AddProtectedBranchesPushAccess < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
   def change
     create_table :protected_branch_push_access_levels do |t|
       t.references :protected_branch, index: { name: "index_protected_branch_push_access" }, foreign_key: true, null: false
diff --git a/db/migrate/20160705054952_add_protected_branches_merge_access.rb b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
index 9f82c0a8aa3..cf1cdb8b3b6 100644
--- a/db/migrate/20160705054952_add_protected_branches_merge_access.rb
+++ b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
@@ -2,8 +2,6 @@
 # for more information on how to write migrations for GitLab.
 
 class AddProtectedBranchesMergeAccess < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
   def change
     create_table :protected_branch_merge_access_levels do |t|
       t.references :protected_branch, index: { name: "index_protected_branch_merge_access" }, foreign_key: true, null: false
diff --git a/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
index 20ca9c3a488..c2b278ce673 100644
--- a/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
+++ b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
@@ -2,19 +2,6 @@
 # for more information on how to write migrations for GitLab.
 
 class MoveFromDevelopersCanMergeToProtectedBranchesMergeAccess < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  # When using the methods "add_concurrent_index" or "add_column_with_default"
-  # you must disable the use of transactions as these methods can not run in an
-  # existing transaction. When using "add_concurrent_index" make sure that this
-  # method is the _only_ method called in the migration, any other changes
-  # should go in a separate migration. This ensures that upon failure _only_ the
-  # index creation fails and can be retried or reverted easily.
-  #
-  # To disable transactions uncomment the following line and remove these
-  # comments:
-  # disable_ddl_transaction!
-
   def up
     execute <<-HEREDOC
       INSERT into protected_branch_merge_access_levels (protected_branch_id, access_level, created_at, updated_at)
diff --git a/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
index 498fb393d61..5bc70283f60 100644
--- a/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
+++ b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
@@ -2,19 +2,6 @@
 # for more information on how to write migrations for GitLab.
 
 class MoveFromDevelopersCanPushToProtectedBranchesPushAccess < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  # When using the methods "add_concurrent_index" or "add_column_with_default"
-  # you must disable the use of transactions as these methods can not run in an
-  # existing transaction. When using "add_concurrent_index" make sure that this
-  # method is the _only_ method called in the migration, any other changes
-  # should go in a separate migration. This ensures that upon failure _only_ the
-  # index creation fails and can be retried or reverted easily.
-  #
-  # To disable transactions uncomment the following line and remove these
-  # comments:
-  # disable_ddl_transaction!
-
   def up
     execute <<-HEREDOC
       INSERT into protected_branch_push_access_levels (protected_branch_id, access_level, created_at, updated_at)
diff --git a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
index 1e9977cfa6e..ad6ad43686d 100644
--- a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
+++ b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
@@ -2,19 +2,6 @@
 # for more information on how to write migrations for GitLab.
 
 class RemoveDevelopersCanPushFromProtectedBranches < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  # When using the methods "add_concurrent_index" or "add_column_with_default"
-  # you must disable the use of transactions as these methods can not run in an
-  # existing transaction. When using "add_concurrent_index" make sure that this
-  # method is the _only_ method called in the migration, any other changes
-  # should go in a separate migration. This ensures that upon failure _only_ the
-  # index creation fails and can be retried or reverted easily.
-  #
-  # To disable transactions uncomment the following line and remove these
-  # comments:
-  # disable_ddl_transaction!
-
   def change
     remove_column :protected_branches, :developers_can_push, :boolean
   end
diff --git a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
index 43d02fbaed6..084914e423a 100644
--- a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
+++ b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
@@ -2,19 +2,6 @@
 # for more information on how to write migrations for GitLab.
 
 class RemoveDevelopersCanMergeFromProtectedBranches < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  # When using the methods "add_concurrent_index" or "add_column_with_default"
-  # you must disable the use of transactions as these methods can not run in an
-  # existing transaction. When using "add_concurrent_index" make sure that this
-  # method is the _only_ method called in the migration, any other changes
-  # should go in a separate migration. This ensures that upon failure _only_ the
-  # index creation fails and can be retried or reverted easily.
-  #
-  # To disable transactions uncomment the following line and remove these
-  # comments:
-  # disable_ddl_transaction!
-
   def change
     remove_column :protected_branches, :developers_can_merge, :boolean
   end

From a72d4491903ad4f6730565df6391667e8ba8b71f Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 25 Jul 2016 14:59:05 +0530
Subject: [PATCH 17/23] Remove duplicate specs from `git_access_spec`

- Likely introduced during an improper conflict resolution.
---
 spec/lib/gitlab/git_access_spec.rb | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index c6f03525aab..8447305a316 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -250,23 +250,21 @@ describe Gitlab::GitAccess, lib: true do
                                      state: 'locked', in_progress_merge_commit_sha: merge_into_protected_branch)
             end
 
-            context "when the merge request is not in progress" do
-              before do
-                create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', in_progress_merge_commit_sha: nil)
-              end
+            run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: true }))
+          end
 
-              run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
+          context "when the merge request is not in progress" do
+            before do
+              create(:merge_request, source_project: project, source_branch: unprotected_branch, target_branch: 'feature', in_progress_merge_commit_sha: nil)
             end
+
+            run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
           end
 
           context "when a merge request does not exist for the given source/target branch" do
             run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
           end
         end
-
-        context "when a merge request does not exist for the given source/target branch" do
-          run_permission_checks(permissions_matrix.deep_merge(developer: { merge_into_protected_branch: false }))
-        end
       end
 
       context "when developers are allowed to push and merge into the #{protected_branch_type} protected branch" do

From 88fd401d599f94bc4ea572cd47afa7d12c484db2 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 25 Jul 2016 15:39:15 +0530
Subject: [PATCH 18/23] Implement review comments from @axil.

1. Align "Allowed to Merge" and "Allowed to Push" dropdowns.

2. Don't display a flash every time a protected branch is updated.
   Previously, we were using this so the test has something to hook onto
   before the assertion. Now we're using `wait_for_ajax` instead.
---
 .../protected_branches/index.html.haml        | 26 +++++++++----------
 spec/features/protected_branches_spec.rb      |  6 +++--
 2 files changed, 16 insertions(+), 16 deletions(-)

diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml
index 69caed7d979..75c2063027a 100644
--- a/app/views/projects/protected_branches/index.html.haml
+++ b/app/views/projects/protected_branches/index.html.haml
@@ -32,22 +32,20 @@
             are supported.
 
         .form-group
-          .prepend-left-10
-            = f.hidden_field :allowed_to_merge
-            = f.label :allowed_to_merge, "Allowed to merge: ", class: "label-light append-bottom-0"
-            = dropdown_tag("<Make a selection>",
-                           options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge',
-                                      dropdown_class: 'dropdown-menu-selectable',
-                                      data: { field_name: "protected_branch[allowed_to_merge]" }})
+          = f.hidden_field :allowed_to_merge
+          = f.label :allowed_to_merge, "Allowed to merge: ", class: "label-light append-bottom-0"
+          = dropdown_tag("<Make a selection>",
+                         options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge',
+                                    dropdown_class: 'dropdown-menu-selectable',
+                                    data: { field_name: "protected_branch[allowed_to_merge]" }})
 
         .form-group
-          .prepend-left-10
-            = f.hidden_field :allowed_to_push
-            = f.label :allowed_to_push, "Allowed to push: ", class: "label-light append-bottom-0"
-            = dropdown_tag("<Make a selection>",
-                           options: { title: "Allowed to push", toggle_class: 'allowed-to-push',
-                                      dropdown_class: 'dropdown-menu-selectable',
-                                      data: { field_name: "protected_branch[allowed_to_push]" }})
+          = f.hidden_field :allowed_to_push
+          = f.label :allowed_to_push, "Allowed to push: ", class: "label-light append-bottom-0"
+          = dropdown_tag("<Make a selection>",
+                         options: { title: "Allowed to push", toggle_class: 'allowed-to-push',
+                                    dropdown_class: 'dropdown-menu-selectable',
+                                    data: { field_name: "protected_branch[allowed_to_push]" }})
 
 
         = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true
diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb
index d72b62a4962..dac2bcf9efd 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -1,6 +1,8 @@
 require 'spec_helper'
 
 feature 'Projected Branches', feature: true, js: true do
+  include WaitForAjax
+
   let(:user) { create(:user, :admin) }
   let(:project) { create(:project) }
 
@@ -109,7 +111,7 @@ feature 'Projected Branches', feature: true, js: true do
           within('.dropdown-menu.push') { click_on access_type_name }
         end
 
-        expect(page).to have_content "Updated protected branch"
+        wait_for_ajax
         expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
       end
     end
@@ -140,7 +142,7 @@ feature 'Projected Branches', feature: true, js: true do
           within('.dropdown-menu.merge') { click_on access_type_name }
         end
 
-        expect(page).to have_content "Updated protected branch"
+        wait_for_ajax
         expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
       end
     end

From 01d190a84ad9b8e4a40cbdec8a55946bac38ab76 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Mon, 25 Jul 2016 20:14:53 +0530
Subject: [PATCH 19/23] Have the `branches` API work with the new protected
 branches data model.

1. The new data model moves from `developers_can_{push,merge}` to
   `allowed_to_{push,merge}`.

2. The API interface has not been changed. It still accepts
   `developers_can_push` and `developers_can_merge` as options. These
   attributes are inferred from the new data model.

3. Modify the protected branch create/update services to translate from
   the API interface to our current data model.
---
 .../protected_branches/base_service.rb        | 34 +++++++++++++++++--
 lib/api/branches.rb                           | 27 +++++++++------
 lib/api/entities.rb                           |  6 ++--
 spec/requests/api/branches_spec.rb            |  2 +-
 4 files changed, 53 insertions(+), 16 deletions(-)

diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb
index f8741fcb3d5..a5896587ded 100644
--- a/app/services/protected_branches/base_service.rb
+++ b/app/services/protected_branches/base_service.rb
@@ -1,6 +1,15 @@
 module ProtectedBranches
   class BaseService < ::BaseService
+    include API::Helpers
+
+    def initialize(project, current_user, params = {})
+      super(project, current_user, params)
+      @allowed_to_push = params[:allowed_to_push]
+      @allowed_to_merge = params[:allowed_to_merge]
+    end
+
     def set_access_levels!
+      translate_api_params!
       set_merge_access_levels!
       set_push_access_levels!
     end
@@ -8,7 +17,7 @@ module ProtectedBranches
     protected
 
     def set_merge_access_levels!
-      case params[:allowed_to_merge]
+      case @allowed_to_merge
       when 'masters'
         @protected_branch.merge_access_level.masters!
       when 'developers'
@@ -17,7 +26,7 @@ module ProtectedBranches
     end
 
     def set_push_access_levels!
-      case params[:allowed_to_push]
+      case @allowed_to_push
       when 'masters'
         @protected_branch.push_access_level.masters!
       when 'developers'
@@ -26,5 +35,26 @@ module ProtectedBranches
         @protected_branch.push_access_level.no_one!
       end
     end
+
+    # The `branches` API still uses `developers_can_push` and `developers_can_merge`,
+    # which need to be translated to `allowed_to_push` and `allowed_to_merge`,
+    # respectively.
+    def translate_api_params!
+      @allowed_to_push ||=
+        case to_boolean(params[:developers_can_push])
+        when true
+          'developers'
+        when false
+          'masters'
+        end
+
+      @allowed_to_merge ||=
+        case to_boolean(params[:developers_can_merge])
+        when true
+          'developers'
+        when false
+          'masters'
+        end
+    end
   end
 end
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index 66b853eb342..4133a1f7a6b 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -35,6 +35,10 @@ module API
 
       # Protect a single branch
       #
+      # Note: The internal data model moved from `developers_can_{merge,push}` to `allowed_to_{merge,push}`
+      # in `gitlab-org/gitlab-ce!5081`. The API interface has not been changed (to maintain compatibility),
+      # but it works with the changed data model to infer `developers_can_merge` and `developers_can_push`.
+      #
       # Parameters:
       #   id (required) - The ID of a project
       #   branch (required) - The name of the branch
@@ -49,18 +53,19 @@ module API
         @branch = user_project.repository.find_branch(params[:branch])
         not_found!('Branch') unless @branch
         protected_branch = user_project.protected_branches.find_by(name: @branch.name)
-        developers_can_push = to_boolean(params[:developers_can_push])
-        developers_can_merge = to_boolean(params[:developers_can_merge])
+        protected_branch_params = {
+          name: @branch.name,
+          developers_can_push: params[:developers_can_push],
+          developers_can_merge: params[:developers_can_merge]
+        }
 
-        if protected_branch
-          protected_branch.developers_can_push = developers_can_push unless developers_can_push.nil?
-          protected_branch.developers_can_merge = developers_can_merge unless developers_can_merge.nil?
-          protected_branch.save
-        else
-          user_project.protected_branches.create(name: @branch.name,
-                                                 developers_can_push: developers_can_push || false,
-                                                 developers_can_merge: developers_can_merge || false)
-        end
+        service = if protected_branch
+                    ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch.id, protected_branch_params)
+                  else
+                    ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
+                  end
+
+        service.execute
 
         present @branch, with: Entities::RepoBranch, project: user_project
       end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index e76e7304674..e51bee5c846 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -126,11 +126,13 @@ module API
       end
 
       expose :developers_can_push do |repo_branch, options|
-        options[:project].developers_can_push_to_protected_branch? repo_branch.name
+        project = options[:project]
+        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_level.developers? }
       end
 
       expose :developers_can_merge do |repo_branch, options|
-        options[:project].developers_can_merge_to_protected_branch? repo_branch.name
+        project = options[:project]
+        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.developers? }
       end
     end
 
diff --git a/spec/requests/api/branches_spec.rb b/spec/requests/api/branches_spec.rb
index 719da27f919..e8fd697965f 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -112,7 +112,7 @@ describe API::API, api: true  do
 
       before do
         project.repository.add_branch(user, protected_branch, 'master')
-        create(:protected_branch, project: project, name: protected_branch, developers_can_push: true, developers_can_merge: true)
+        create(:protected_branch, :developers_can_push, :developers_can_merge, project: project, name: protected_branch)
       end
 
       it 'updates that a developer can push' do

From 6d841eaadcbccfa4527bd892bf86fc8dbba19455 Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 27 Jul 2016 10:14:38 +0530
Subject: [PATCH 20/23] Authorize user before creating/updating a protected
 branch.

1. This is a third line of defence (first in the view, second in the
   controller).

2. Duplicate the `API::Helpers.to_boolean` method in `BaseService`. The
   other alternative is to `include API::Helpers`, but this brings with it
   a number of other methods that might cause conflicts.

3. Return a 403 if authorization fails.
---
 app/services/protected_branches/base_service.rb   | 13 ++++++++++---
 app/services/protected_branches/create_service.rb |  2 ++
 app/services/protected_branches/update_service.rb |  5 +++--
 3 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb
index a5896587ded..bdd175e8552 100644
--- a/app/services/protected_branches/base_service.rb
+++ b/app/services/protected_branches/base_service.rb
@@ -1,7 +1,5 @@
 module ProtectedBranches
   class BaseService < ::BaseService
-    include API::Helpers
-
     def initialize(project, current_user, params = {})
       super(project, current_user, params)
       @allowed_to_push = params[:allowed_to_push]
@@ -14,7 +12,7 @@ module ProtectedBranches
       set_push_access_levels!
     end
 
-    protected
+    private
 
     def set_merge_access_levels!
       case @allowed_to_merge
@@ -56,5 +54,14 @@ module ProtectedBranches
           'masters'
         end
     end
+
+    protected
+
+    def to_boolean(value)
+      return true if value =~ /^(true|t|yes|y|1|on)$/i
+      return false if value =~ /^(false|f|no|n|0|off)$/i
+
+      nil
+    end
   end
 end
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
index 212c2134638..36019906416 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -3,6 +3,8 @@ module ProtectedBranches
     attr_reader :protected_branch
 
     def execute
+      raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
+
       ProtectedBranch.transaction do
         @protected_branch = project.protected_branches.new(name: params[:name])
         @protected_branch.save!
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
index 4a2b1be9c93..58f2f774bae 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -4,12 +4,13 @@ module ProtectedBranches
 
     def initialize(project, current_user, id, params = {})
       super(project, current_user, params)
-      @id = id
+      @protected_branch = ProtectedBranch.find(id)
     end
 
     def execute
+      raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
+
       ProtectedBranch.transaction do
-        @protected_branch = ProtectedBranch.find(@id)
         set_access_levels!
       end
 

From c93a895abc434b9b78aa7cf4d285ce309cfd868a Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Wed, 27 Jul 2016 12:33:50 +0530
Subject: [PATCH 21/23] Fix `git_push_service_spec`

1. Caused by incorrect test setup. The user wasn't added to the project,
   so protected branch creation failed authorization.

2. Change setup for a different test (`Event.last` to
   `Event.find_by_action`) because our `project.team << ...` addition
   was causing a conflict.
---
 spec/services/git_push_service_spec.rb | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 663c270d61f..621eced83f6 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -7,6 +7,7 @@ describe GitPushService, services: true do
   let(:project)       { create :project }
 
   before do
+    project.team << [user, :master]
     @blankrev = Gitlab::Git::BLANK_SHA
     @oldrev = sample_commit.parent_id
     @newrev = sample_commit.id
@@ -172,7 +173,7 @@ describe GitPushService, services: true do
   describe "Push Event" do
     before do
       service = execute_service(project, user, @oldrev, @newrev, @ref )
-      @event = Event.last
+      @event = Event.find_by_action(Event::PUSHED)
       @push_data = service.push_data
     end
 

From 0a8aeb46dc187cc309ddbe23d8624f5d24b6218c Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 29 Jul 2016 11:43:07 +0530
Subject: [PATCH 22/23] Use `Gitlab::Access` to protected branch access levels.

1. It makes sense to reuse these constants since we had them duplicated
   in the previous enum implementation. This also simplifies our
   `check_access` implementation, because we can use
   `project.team.max_member_access` directly.

2. Use `accepts_nested_attributes_for` to create push/merge access
   levels. This was a bit fiddly to set up, but this simplifies our code
   by quite a large amount. We can even get rid of
   `ProtectedBranches::BaseService`.

3. Move API handling back into the API (previously in
   `ProtectedBranches::BaseService#translate_api_params`.

4. The protected branch services now return a `ProtectedBranch` rather
   than `true/false`.

5. Run `load_protected_branches` on-demand in the `create` action, to
   prevent it being called unneccessarily.

6. "Masters" is pre-selected as the default option for "Allowed to Push"
   and "Allowed to Merge".

7. These changes were based on a review from @rymai in !5081.
---
 .../protected_branches_access_select.js.es6   | 18 +++--
 .../projects/protected_branches_controller.rb | 31 +++++----
 app/models/protected_branch.rb                | 11 +--
 .../protected_branch/merge_access_level.rb    | 16 ++---
 .../protected_branch/push_access_level.rb     | 21 +++---
 app/services/git_push_service.rb              | 13 +++-
 .../protected_branches/base_service.rb        | 67 -------------------
 .../protected_branches/create_service.rb      | 20 +++---
 .../protected_branches/update_service.rb      | 19 ++----
 .../_branches_list.html.haml                  |  2 +-
 .../_protected_branch.html.haml               |  4 +-
 .../protected_branches/index.html.haml        | 14 ++--
 ...4938_add_protected_branches_push_access.rb |  4 +-
 ...952_add_protected_branches_merge_access.rb |  4 +-
 db/schema.rb                                  | 16 ++---
 lib/api/branches.rb                           | 36 +++++++---
 lib/api/entities.rb                           |  4 +-
 spec/factories/protected_branches.rb          | 16 +++--
 spec/features/protected_branches_spec.rb      | 12 ++--
 spec/services/git_push_service_spec.rb        | 12 ++--
 20 files changed, 152 insertions(+), 188 deletions(-)
 delete mode 100644 app/services/protected_branches/base_service.rb

diff --git a/app/assets/javascripts/protected_branches_access_select.js.es6 b/app/assets/javascripts/protected_branches_access_select.js.es6
index 93b7d7755a7..e98312bbf37 100644
--- a/app/assets/javascripts/protected_branches_access_select.js.es6
+++ b/app/assets/javascripts/protected_branches_access_select.js.es6
@@ -1,27 +1,35 @@
 class ProtectedBranchesAccessSelect {
-  constructor(container, saveOnSelect) {
+  constructor(container, saveOnSelect, selectDefault) {
     this.container = container;
     this.saveOnSelect = saveOnSelect;
 
     this.container.find(".allowed-to-merge").each((i, element) => {
       var fieldName = $(element).data('field-name');
-      return $(element).glDropdown({
+      var dropdown = $(element).glDropdown({
         data: gon.merge_access_levels,
         selectable: true,
         fieldName: fieldName,
         clicked: _.chain(this.onSelect).partial(element).bind(this).value()
       });
+
+      if (selectDefault) {
+        dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0);
+      }
     });
 
 
     this.container.find(".allowed-to-push").each((i, element) => {
       var fieldName = $(element).data('field-name');
-      return $(element).glDropdown({
+      var dropdown = $(element).glDropdown({
         data: gon.push_access_levels,
         selectable: true,
         fieldName: fieldName,
         clicked: _.chain(this.onSelect).partial(element).bind(this).value()
       });
+
+      if (selectDefault) {
+        dropdown.data('glDropdown').selectRowAtIndex(document.createEvent("Event"), 0);
+      }
     });
   }
 
@@ -36,7 +44,9 @@ class ProtectedBranchesAccessSelect {
           _method: 'PATCH',
           id: $(dropdown).data('id'),
           protected_branch: {
-            ["" + ($(dropdown).data('type'))]: selected.id
+            ["" + ($(dropdown).data('type')) + "_attributes"]: {
+              "access_level": selected.id
+            }
           }
         },
         success: function() {
diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index ddf1824ccb9..d28ec6e2eac 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -3,23 +3,22 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   before_action :require_non_empty_project
   before_action :authorize_admin_project!
   before_action :load_protected_branch, only: [:show, :update, :destroy]
-  before_action :load_protected_branches, only: [:index, :create]
+  before_action :load_protected_branches, only: [:index]
 
   layout "project_settings"
 
   def index
     @protected_branch = @project.protected_branches.new
-    gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } },
-               push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
-               merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } })
+    load_protected_branches_gon_variables
   end
 
   def create
-    service = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params)
-    if service.execute
+    @protected_branch = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute
+    if @protected_branch.persisted?
       redirect_to namespace_project_protected_branches_path(@project.namespace, @project)
     else
-      @protected_branch = service.protected_branch
+      load_protected_branches
+      load_protected_branches_gon_variables
       render :index
     end
   end
@@ -29,15 +28,15 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def update
-    service = ProtectedBranches::UpdateService.new(@project, current_user, params[:id], protected_branch_params)
+    @protected_branch = ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch)
 
-    if service.execute
+    if @protected_branch.valid?
       respond_to do |format|
-        format.json { render json: service.protected_branch, status: :ok }
+        format.json { render json: @protected_branch, status: :ok }
       end
     else
       respond_to do |format|
-        format.json { render json: service.protected_branch.errors, status: :unprocessable_entity }
+        format.json { render json: @protected_branch.errors, status: :unprocessable_entity }
       end
     end
   end
@@ -58,10 +57,18 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def protected_branch_params
-    params.require(:protected_branch).permit(:name, :allowed_to_push, :allowed_to_merge)
+    params.require(:protected_branch).permit(:name,
+                                             merge_access_level_attributes: [:access_level],
+                                             push_access_level_attributes: [:access_level])
   end
 
   def load_protected_branches
     @protected_branches = @project.protected_branches.order(:name).page(params[:page])
   end
+
+  def load_protected_branches_gon_variables
+    gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } },
+               push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
+               merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } })
+  end
 end
diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb
index c0bee72b4d7..226b3f54342 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -8,18 +8,13 @@ class ProtectedBranch < ActiveRecord::Base
   has_one :merge_access_level, dependent: :destroy
   has_one :push_access_level, dependent: :destroy
 
+  accepts_nested_attributes_for :push_access_level
+  accepts_nested_attributes_for :merge_access_level
+
   def commit
     project.commit(self.name)
   end
 
-  def allowed_to_push
-    self.push_access_level && self.push_access_level.access_level
-  end
-
-  def allowed_to_merge
-    self.merge_access_level && self.merge_access_level.access_level
-  end
-
   # Returns all protected branches that match the given branch name.
   # This realizes all records from the scope built up so far, and does
   # _not_ return a relation.
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index 17a3a86c3e1..25a6ca6a8ee 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -2,25 +2,19 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
   delegate :project, to: :protected_branch
 
-  enum access_level: [:masters, :developers]
+  validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
+                                                             Gitlab::Access::DEVELOPER] }
 
   def self.human_access_levels
     {
-      "masters" => "Masters",
-      "developers" => "Developers + Masters"
+      Gitlab::Access::MASTER => "Masters",
+      Gitlab::Access::DEVELOPER => "Developers + Masters"
     }.with_indifferent_access
   end
 
   def check_access(user)
     return true if user.is_admin?
-
-    min_member_access = if masters?
-                          Gitlab::Access::MASTER
-                        elsif developers?
-                          Gitlab::Access::DEVELOPER
-                        end
-
-    project.team.max_member_access(user.id) >= min_member_access
+    project.team.max_member_access(user.id) >= access_level
   end
 
   def humanize
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index 22096b13300..1999316aa26 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -2,27 +2,22 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   belongs_to :protected_branch
   delegate :project, to: :protected_branch
 
-  enum access_level: [:masters, :developers, :no_one]
+  validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
+                                                             Gitlab::Access::DEVELOPER,
+                                                             Gitlab::Access::NO_ACCESS] }
 
   def self.human_access_levels
     {
-      "masters" => "Masters",
-      "developers" => "Developers + Masters",
-      "no_one" => "No one"
+      Gitlab::Access::MASTER => "Masters",
+      Gitlab::Access::DEVELOPER => "Developers + Masters",
+      Gitlab::Access::NO_ACCESS => "No one"
     }.with_indifferent_access
   end
 
   def check_access(user)
-    return false if no_one?
+    return false if access_level == Gitlab::Access::NO_ACCESS
     return true if user.is_admin?
-
-    min_member_access = if masters?
-                          Gitlab::Access::MASTER
-                        elsif developers?
-                          Gitlab::Access::DEVELOPER
-                        end
-
-    project.team.max_member_access(user.id) >= min_member_access
+    project.team.max_member_access(user.id) >= access_level
   end
 
   def humanize
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index 604737e6934..3f6a177bf3a 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -88,10 +88,17 @@ class GitPushService < BaseService
 
     # Set protection on the default branch if configured
     if current_application_settings.default_branch_protection != PROTECTION_NONE
-      allowed_to_push = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? 'developers' : 'masters'
-      allowed_to_merge = current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_MERGE ? 'developers' : 'masters'
 
-      params = { name: @project.default_branch, allowed_to_push: allowed_to_push, allowed_to_merge: allowed_to_merge }
+      params = {
+        name: @project.default_branch,
+        push_access_level_attributes: {
+          access_level: current_application_settings.default_branch_protection == PROTECTION_DEV_CAN_PUSH ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+        },
+        merge_access_level_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
     end
   end
diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb
deleted file mode 100644
index bdd175e8552..00000000000
--- a/app/services/protected_branches/base_service.rb
+++ /dev/null
@@ -1,67 +0,0 @@
-module ProtectedBranches
-  class BaseService < ::BaseService
-    def initialize(project, current_user, params = {})
-      super(project, current_user, params)
-      @allowed_to_push = params[:allowed_to_push]
-      @allowed_to_merge = params[:allowed_to_merge]
-    end
-
-    def set_access_levels!
-      translate_api_params!
-      set_merge_access_levels!
-      set_push_access_levels!
-    end
-
-    private
-
-    def set_merge_access_levels!
-      case @allowed_to_merge
-      when 'masters'
-        @protected_branch.merge_access_level.masters!
-      when 'developers'
-        @protected_branch.merge_access_level.developers!
-      end
-    end
-
-    def set_push_access_levels!
-      case @allowed_to_push
-      when 'masters'
-        @protected_branch.push_access_level.masters!
-      when 'developers'
-        @protected_branch.push_access_level.developers!
-      when 'no_one'
-        @protected_branch.push_access_level.no_one!
-      end
-    end
-
-    # The `branches` API still uses `developers_can_push` and `developers_can_merge`,
-    # which need to be translated to `allowed_to_push` and `allowed_to_merge`,
-    # respectively.
-    def translate_api_params!
-      @allowed_to_push ||=
-        case to_boolean(params[:developers_can_push])
-        when true
-          'developers'
-        when false
-          'masters'
-        end
-
-      @allowed_to_merge ||=
-        case to_boolean(params[:developers_can_merge])
-        when true
-          'developers'
-        when false
-          'masters'
-        end
-    end
-
-    protected
-
-    def to_boolean(value)
-      return true if value =~ /^(true|t|yes|y|1|on)$/i
-      return false if value =~ /^(false|f|no|n|0|off)$/i
-
-      nil
-    end
-  end
-end
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
index 36019906416..50f79f491ce 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -1,23 +1,27 @@
 module ProtectedBranches
-  class CreateService < ProtectedBranches::BaseService
+  class CreateService < BaseService
     attr_reader :protected_branch
 
     def execute
       raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
 
+      protected_branch = project.protected_branches.new(params)
+
       ProtectedBranch.transaction do
-        @protected_branch = project.protected_branches.new(name: params[:name])
-        @protected_branch.save!
+        protected_branch.save!
 
-        @protected_branch.create_push_access_level!
-        @protected_branch.create_merge_access_level!
+        if protected_branch.push_access_level.blank?
+          protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
+        end
 
-        set_access_levels!
+        if protected_branch.merge_access_level.blank?
+          protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
+        end
       end
 
-      true
+      protected_branch
     rescue ActiveRecord::RecordInvalid
-      false
+      protected_branch
     end
   end
 end
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
index 58f2f774bae..da4c96b3e5f 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -1,22 +1,13 @@
 module ProtectedBranches
-  class UpdateService < ProtectedBranches::BaseService
+  class UpdateService < BaseService
     attr_reader :protected_branch
 
-    def initialize(project, current_user, id, params = {})
-      super(project, current_user, params)
-      @protected_branch = ProtectedBranch.find(id)
-    end
-
-    def execute
+    def execute(protected_branch)
       raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
 
-      ProtectedBranch.transaction do
-        set_access_levels!
-      end
-
-      true
-    rescue ActiveRecord::RecordInvalid
-      false
+      @protected_branch = protected_branch
+      @protected_branch.update(params)
+      @protected_branch
     end
   end
 end
diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml
index a6956c8e69f..2498b57afb4 100644
--- a/app/views/projects/protected_branches/_branches_list.html.haml
+++ b/app/views/projects/protected_branches/_branches_list.html.haml
@@ -26,4 +26,4 @@
   = paginate @protected_branches, theme: 'gitlab'
 
 :javascript
-  new ProtectedBranchesAccessSelect($(".protected-branches-list"), true);
+  new ProtectedBranchesAccessSelect($(".protected-branches-list"), true, false);
diff --git a/app/views/projects/protected_branches/_protected_branch.html.haml b/app/views/projects/protected_branches/_protected_branch.html.haml
index 2fc6081e448..498e412235e 100644
--- a/app/views/projects/protected_branches/_protected_branch.html.haml
+++ b/app/views/projects/protected_branches/_protected_branch.html.haml
@@ -18,12 +18,12 @@
     = hidden_field_tag "allowed_to_merge_#{protected_branch.id}", protected_branch.merge_access_level.access_level
     = dropdown_tag(protected_branch.merge_access_level.humanize,
                    options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge', dropdown_class: 'dropdown-menu-selectable merge',
-                   data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_merge" }})
+                   data: { field_name: "allowed_to_merge_#{protected_branch.id}", url: url, id: protected_branch.id, type: "merge_access_level" }})
   %td
     = hidden_field_tag "allowed_to_push_#{protected_branch.id}", protected_branch.push_access_level.access_level
     = dropdown_tag(protected_branch.push_access_level.humanize,
                    options: { title: "Allowed to push", toggle_class: 'allowed-to-push', dropdown_class: 'dropdown-menu-selectable push',
-                   data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "allowed_to_push" }})
+                   data: { field_name: "allowed_to_push_#{protected_branch.id}", url: url, id: protected_branch.id, type: "push_access_level" }})
   - if can_admin_project
     %td
       = link_to 'Unprotect', [@project.namespace.becomes(Namespace), @project, protected_branch], data: { confirm: 'Branch will be writable for developers. Are you sure?' }, method: :delete, class: "btn btn-warning btn-sm pull-right"
diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml
index 75c2063027a..8270da6cd27 100644
--- a/app/views/projects/protected_branches/index.html.haml
+++ b/app/views/projects/protected_branches/index.html.haml
@@ -32,20 +32,20 @@
             are supported.
 
         .form-group
-          = f.hidden_field :allowed_to_merge
-          = f.label :allowed_to_merge, "Allowed to merge: ", class: "label-light append-bottom-0"
+          = hidden_field_tag 'protected_branch[merge_access_level_attributes][access_level]'
+          = label_tag "Allowed to merge: ", nil, class: "label-light append-bottom-0"
           = dropdown_tag("<Make a selection>",
                          options: { title: "Allowed to merge", toggle_class: 'allowed-to-merge',
                                     dropdown_class: 'dropdown-menu-selectable',
-                                    data: { field_name: "protected_branch[allowed_to_merge]" }})
+                                    data: { field_name: "protected_branch[merge_access_level_attributes][access_level]" }})
 
         .form-group
-          = f.hidden_field :allowed_to_push
-          = f.label :allowed_to_push, "Allowed to push: ", class: "label-light append-bottom-0"
+          = hidden_field_tag 'protected_branch[push_access_level_attributes][access_level]'
+          = label_tag "Allowed to push: ", nil, class: "label-light append-bottom-0"
           = dropdown_tag("<Make a selection>",
                          options: { title: "Allowed to push", toggle_class: 'allowed-to-push',
                                     dropdown_class: 'dropdown-menu-selectable',
-                                    data: { field_name: "protected_branch[allowed_to_push]" }})
+                                    data: { field_name: "protected_branch[push_access_level_attributes][access_level]" }})
 
 
         = f.submit "Protect", class: "btn-create btn protect-branch-btn", disabled: true
@@ -54,4 +54,4 @@
     = render "branches_list"
 
 :javascript
-  new ProtectedBranchesAccessSelect($(".new_protected_branch"), false);
+  new ProtectedBranchesAccessSelect($(".new_protected_branch"), false, true);
diff --git a/db/migrate/20160705054938_add_protected_branches_push_access.rb b/db/migrate/20160705054938_add_protected_branches_push_access.rb
index 3031574fe2a..5c14d449e71 100644
--- a/db/migrate/20160705054938_add_protected_branches_push_access.rb
+++ b/db/migrate/20160705054938_add_protected_branches_push_access.rb
@@ -5,7 +5,9 @@ class AddProtectedBranchesPushAccess < ActiveRecord::Migration
   def change
     create_table :protected_branch_push_access_levels do |t|
       t.references :protected_branch, index: { name: "index_protected_branch_push_access" }, foreign_key: true, null: false
-      t.integer :access_level, default: 0, null: false
+
+      # Gitlab::Access::MASTER == 40
+      t.integer :access_level, default: 40, null: false
 
       t.timestamps null: false
     end
diff --git a/db/migrate/20160705054952_add_protected_branches_merge_access.rb b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
index cf1cdb8b3b6..789e3e04220 100644
--- a/db/migrate/20160705054952_add_protected_branches_merge_access.rb
+++ b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
@@ -5,7 +5,9 @@ class AddProtectedBranchesMergeAccess < ActiveRecord::Migration
   def change
     create_table :protected_branch_merge_access_levels do |t|
       t.references :protected_branch, index: { name: "index_protected_branch_merge_access" }, foreign_key: true, null: false
-      t.integer :access_level, default: 0, null: false
+
+      # Gitlab::Access::MASTER == 40
+      t.integer :access_level, default: 40, null: false
 
       t.timestamps null: false
     end
diff --git a/db/schema.rb b/db/schema.rb
index 7a5eded8e02..2d2ae5fd840 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -868,19 +868,19 @@ ActiveRecord::Schema.define(version: 20160722221922) do
   add_index "projects", ["visibility_level"], name: "index_projects_on_visibility_level", using: :btree
 
   create_table "protected_branch_merge_access_levels", force: :cascade do |t|
-    t.integer  "protected_branch_id",             null: false
-    t.integer  "access_level",        default: 0, null: false
-    t.datetime "created_at",                      null: false
-    t.datetime "updated_at",                      null: false
+    t.integer  "protected_branch_id",              null: false
+    t.integer  "access_level",        default: 40, null: false
+    t.datetime "created_at",                       null: false
+    t.datetime "updated_at",                       null: false
   end
 
   add_index "protected_branch_merge_access_levels", ["protected_branch_id"], name: "index_protected_branch_merge_access", using: :btree
 
   create_table "protected_branch_push_access_levels", force: :cascade do |t|
-    t.integer  "protected_branch_id",             null: false
-    t.integer  "access_level",        default: 0, null: false
-    t.datetime "created_at",                      null: false
-    t.datetime "updated_at",                      null: false
+    t.integer  "protected_branch_id",              null: false
+    t.integer  "access_level",        default: 40, null: false
+    t.datetime "created_at",                       null: false
+    t.datetime "updated_at",                       null: false
   end
 
   add_index "protected_branch_push_access_levels", ["protected_branch_id"], name: "index_protected_branch_push_access", using: :btree
diff --git a/lib/api/branches.rb b/lib/api/branches.rb
index 4133a1f7a6b..a77afe634f6 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -53,19 +53,37 @@ module API
         @branch = user_project.repository.find_branch(params[:branch])
         not_found!('Branch') unless @branch
         protected_branch = user_project.protected_branches.find_by(name: @branch.name)
+
+        developers_can_merge = to_boolean(params[:developers_can_merge])
+        developers_can_push = to_boolean(params[:developers_can_push])
+
         protected_branch_params = {
-          name: @branch.name,
-          developers_can_push: params[:developers_can_push],
-          developers_can_merge: params[:developers_can_merge]
+          name: @branch.name
         }
 
-        service = if protected_branch
-                    ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch.id, protected_branch_params)
-                  else
-                    ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
-                  end
+        unless developers_can_merge.nil?
+          protected_branch_params.merge!({
+            merge_access_level_attributes: {
+              access_level: developers_can_merge ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+            }
+          })
+        end
 
-        service.execute
+        unless developers_can_push.nil?
+          protected_branch_params.merge!({
+            push_access_level_attributes: {
+              access_level: developers_can_push ? Gitlab::Access::DEVELOPER : Gitlab::Access::MASTER
+            }
+          })
+        end
+
+        if protected_branch
+          service = ProtectedBranches::UpdateService.new(user_project, current_user, protected_branch_params)
+          service.execute(protected_branch)
+        else
+          service = ProtectedBranches::CreateService.new(user_project, current_user, protected_branch_params)
+          service.execute
+        end
 
         present @branch, with: Entities::RepoBranch, project: user_project
       end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index e51bee5c846..4eb95d8a215 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -127,12 +127,12 @@ module API
 
       expose :developers_can_push do |repo_branch, options|
         project = options[:project]
-        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_level.developers? }
+        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.push_access_level.access_level == Gitlab::Access::DEVELOPER }
       end
 
       expose :developers_can_merge do |repo_branch, options|
         project = options[:project]
-        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.developers? }
+        project.protected_branches.matching(repo_branch.name).any? { |protected_branch| protected_branch.merge_access_level.access_level == Gitlab::Access::DEVELOPER }
       end
     end
 
diff --git a/spec/factories/protected_branches.rb b/spec/factories/protected_branches.rb
index 24a9b78f0c2..5575852c2d7 100644
--- a/spec/factories/protected_branches.rb
+++ b/spec/factories/protected_branches.rb
@@ -4,20 +4,26 @@ FactoryGirl.define do
     project
 
     after(:create) do |protected_branch|
-      protected_branch.create_push_access_level!(access_level: :masters)
-      protected_branch.create_merge_access_level!(access_level: :masters)
+      protected_branch.create_push_access_level!(access_level: Gitlab::Access::MASTER)
+      protected_branch.create_merge_access_level!(access_level: Gitlab::Access::MASTER)
     end
 
     trait :developers_can_push do
-      after(:create) { |protected_branch| protected_branch.push_access_level.developers! }
+      after(:create) do |protected_branch|
+        protected_branch.push_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
+      end
     end
 
     trait :developers_can_merge do
-      after(:create) { |protected_branch| protected_branch.merge_access_level.developers! }
+      after(:create) do |protected_branch|
+        protected_branch.merge_access_level.update!(access_level: Gitlab::Access::DEVELOPER)
+      end
     end
 
     trait :no_one_can_push do
-      after(:create) { |protected_branch| protected_branch.push_access_level.no_one! }
+      after(:create) do |protected_branch|
+        protected_branch.push_access_level.update!(access_level: Gitlab::Access::NO_ACCESS)
+      end
     end
   end
 end
diff --git a/spec/features/protected_branches_spec.rb b/spec/features/protected_branches_spec.rb
index dac2bcf9efd..57734b33a44 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -91,12 +91,12 @@ feature 'Projected Branches', feature: true, js: true do
         set_protected_branch_name('master')
         within('.new_protected_branch') do
           find(".allowed-to-push").click
-          click_on access_type_name
+          within(".dropdown.open .dropdown-menu") { click_on access_type_name }
         end
         click_on "Protect"
 
         expect(ProtectedBranch.count).to eq(1)
-        expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
+        expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
       end
 
       it "allows updating protected branches so that #{access_type_name} can push to them" do
@@ -112,7 +112,7 @@ feature 'Projected Branches', feature: true, js: true do
         end
 
         wait_for_ajax
-        expect(ProtectedBranch.last.allowed_to_push).to eq(access_type_id)
+        expect(ProtectedBranch.last.push_access_level.access_level).to eq(access_type_id)
       end
     end
 
@@ -122,12 +122,12 @@ feature 'Projected Branches', feature: true, js: true do
         set_protected_branch_name('master')
         within('.new_protected_branch') do
           find(".allowed-to-merge").click
-          click_on access_type_name
+          within(".dropdown.open .dropdown-menu") { click_on access_type_name }
         end
         click_on "Protect"
 
         expect(ProtectedBranch.count).to eq(1)
-        expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
+        expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
       end
 
       it "allows updating protected branches so that #{access_type_name} can merge to them" do
@@ -143,7 +143,7 @@ feature 'Projected Branches', feature: true, js: true do
         end
 
         wait_for_ajax
-        expect(ProtectedBranch.last.allowed_to_merge).to eq(access_type_id)
+        expect(ProtectedBranch.last.merge_access_level.access_level).to eq(access_type_id)
       end
     end
   end
diff --git a/spec/services/git_push_service_spec.rb b/spec/services/git_push_service_spec.rb
index 621eced83f6..ffa998dffc3 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -227,8 +227,8 @@ describe GitPushService, services: true do
         expect(project.default_branch).to eq("master")
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
         expect(project.protected_branches).not_to be_empty
-        expect(project.protected_branches.first.allowed_to_push).to eq('masters')
-        expect(project.protected_branches.first.allowed_to_merge).to eq('masters')
+        expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
+        expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
       end
 
       it "when pushing a branch for the first time with default branch protection disabled" do
@@ -249,8 +249,8 @@ describe GitPushService, services: true do
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
 
         expect(project.protected_branches).not_to be_empty
-        expect(project.protected_branches.last.allowed_to_push).to eq('developers')
-        expect(project.protected_branches.last.allowed_to_merge).to eq('masters')
+        expect(project.protected_branches.last.push_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
+        expect(project.protected_branches.last.merge_access_level.access_level).to eq(Gitlab::Access::MASTER)
       end
 
       it "when pushing a branch for the first time with default branch protection set to 'developers can merge'" do
@@ -260,8 +260,8 @@ describe GitPushService, services: true do
         expect(project.default_branch).to eq("master")
         execute_service(project, user, @blankrev, 'newrev', 'refs/heads/master' )
         expect(project.protected_branches).not_to be_empty
-        expect(project.protected_branches.first.allowed_to_push).to eq('masters')
-        expect(project.protected_branches.first.allowed_to_merge).to eq('developers')
+        expect(project.protected_branches.first.push_access_level.access_level).to eq(Gitlab::Access::MASTER)
+        expect(project.protected_branches.first.merge_access_level.access_level).to eq(Gitlab::Access::DEVELOPER)
       end
 
       it "when pushing new commits to existing branch" do

From cebcc417eda08711ad17a433d6d9b4f49830c04c Mon Sep 17 00:00:00 2001
From: Timothy Andrew <mail@timothyandrew.net>
Date: Fri, 29 Jul 2016 12:31:15 +0530
Subject: [PATCH 23/23] Implement final review comments from @rymai.

1. Instantiate `ProtectedBranchesAccessSelect` from `dispatcher`

2. Use `can?(user, ...)` instead of `user.can?(...)`

3. Add `DOWNTIME` notes to all migrations added in !5081.

4. Add an explicit `down` method for migrations removing the
   `developers_can_push` and `developers_can_merge` columns, ensuring that
   the columns created (on rollback) have the appropriate defaults.

5. Remove duplicate CHANGELOG entries.

6. Blank lines after guard clauses.
---
 CHANGELOG                                           |  1 -
 app/assets/javascripts/dispatcher.js                |  5 +++++
 app/models/protected_branch/merge_access_level.rb   |  1 +
 app/models/protected_branch/push_access_level.rb    |  1 +
 app/services/protected_branches/create_service.rb   |  2 +-
 app/services/protected_branches/update_service.rb   |  2 +-
 .../protected_branches/_branches_list.html.haml     |  3 ---
 .../projects/protected_branches/index.html.haml     |  3 ---
 ...0705054938_add_protected_branches_push_access.rb |  2 ++
 ...705054952_add_protected_branches_merge_access.rb |  2 ++
 ..._can_merge_to_protected_branches_merge_access.rb |  9 +++++++++
 ...rs_can_push_to_protected_branches_push_access.rb |  9 +++++++++
 ...e_developers_can_push_from_protected_branches.rb | 13 ++++++++++++-
 ..._developers_can_merge_from_protected_branches.rb | 13 ++++++++++++-
 14 files changed, 55 insertions(+), 11 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 4f1da451df0..2b04c15b827 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -127,7 +127,6 @@ v 8.10.0
   - Allow to define manual actions/builds on Pipelines and Environments
   - Fix pagination when sorting by columns with lots of ties (like priority)
   - The Markdown reference parsers now re-use query results to prevent running the same queries multiple times. !5020
-  - Add "No one can push" as an option for protected branches. !5081
   - Updated project header design
   - Issuable collapsed assignee tooltip is now the users name
   - Fix compare view not changing code view rendering style
diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js
index d212d66da1b..9e6901962c6 100644
--- a/app/assets/javascripts/dispatcher.js
+++ b/app/assets/javascripts/dispatcher.js
@@ -171,6 +171,11 @@
           break;
         case 'search:show':
           new Search();
+          break;
+        case 'projects:protected_branches:index':
+          new ProtectedBranchesAccessSelect($(".new_protected_branch"), false, true);
+          new ProtectedBranchesAccessSelect($(".protected-branches-list"), true, false);
+          break;
       }
       switch (path.first()) {
         case 'admin':
diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index 25a6ca6a8ee..b1112ee737d 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -14,6 +14,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
 
   def check_access(user)
     return true if user.is_admin?
+
     project.team.max_member_access(user.id) >= access_level
   end
 
diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb
index 1999316aa26..6a5e49cf453 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -17,6 +17,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
   def check_access(user)
     return false if access_level == Gitlab::Access::NO_ACCESS
     return true if user.is_admin?
+
     project.team.max_member_access(user.id) >= access_level
   end
 
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
index 50f79f491ce..6150a2a83c9 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -3,7 +3,7 @@ module ProtectedBranches
     attr_reader :protected_branch
 
     def execute
-      raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
+      raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
 
       protected_branch = project.protected_branches.new(params)
 
diff --git a/app/services/protected_branches/update_service.rb b/app/services/protected_branches/update_service.rb
index da4c96b3e5f..89d8ba60134 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -3,7 +3,7 @@ module ProtectedBranches
     attr_reader :protected_branch
 
     def execute(protected_branch)
-      raise Gitlab::Access::AccessDeniedError unless current_user.can?(:admin_project, project)
+      raise Gitlab::Access::AccessDeniedError unless can?(current_user, :admin_project, project)
 
       @protected_branch = protected_branch
       @protected_branch.update(params)
diff --git a/app/views/projects/protected_branches/_branches_list.html.haml b/app/views/projects/protected_branches/_branches_list.html.haml
index 2498b57afb4..0603a014008 100644
--- a/app/views/projects/protected_branches/_branches_list.html.haml
+++ b/app/views/projects/protected_branches/_branches_list.html.haml
@@ -24,6 +24,3 @@
       = render partial: @protected_branches, locals: { can_admin_project: can_admin_project }
 
   = paginate @protected_branches, theme: 'gitlab'
-
-:javascript
-  new ProtectedBranchesAccessSelect($(".protected-branches-list"), true, false);
diff --git a/app/views/projects/protected_branches/index.html.haml b/app/views/projects/protected_branches/index.html.haml
index 8270da6cd27..4efe44c7233 100644
--- a/app/views/projects/protected_branches/index.html.haml
+++ b/app/views/projects/protected_branches/index.html.haml
@@ -52,6 +52,3 @@
 
     %hr
     = render "branches_list"
-
-:javascript
-  new ProtectedBranchesAccessSelect($(".new_protected_branch"), false, true);
diff --git a/db/migrate/20160705054938_add_protected_branches_push_access.rb b/db/migrate/20160705054938_add_protected_branches_push_access.rb
index 5c14d449e71..f27295524e1 100644
--- a/db/migrate/20160705054938_add_protected_branches_push_access.rb
+++ b/db/migrate/20160705054938_add_protected_branches_push_access.rb
@@ -2,6 +2,8 @@
 # for more information on how to write migrations for GitLab.
 
 class AddProtectedBranchesPushAccess < ActiveRecord::Migration
+  DOWNTIME = false
+
   def change
     create_table :protected_branch_push_access_levels do |t|
       t.references :protected_branch, index: { name: "index_protected_branch_push_access" }, foreign_key: true, null: false
diff --git a/db/migrate/20160705054952_add_protected_branches_merge_access.rb b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
index 789e3e04220..32adfa266cd 100644
--- a/db/migrate/20160705054952_add_protected_branches_merge_access.rb
+++ b/db/migrate/20160705054952_add_protected_branches_merge_access.rb
@@ -2,6 +2,8 @@
 # for more information on how to write migrations for GitLab.
 
 class AddProtectedBranchesMergeAccess < ActiveRecord::Migration
+  DOWNTIME = false
+
   def change
     create_table :protected_branch_merge_access_levels do |t|
       t.references :protected_branch, index: { name: "index_protected_branch_merge_access" }, foreign_key: true, null: false
diff --git a/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
index c2b278ce673..fa93936ced7 100644
--- a/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
+++ b/db/migrate/20160705055254_move_from_developers_can_merge_to_protected_branches_merge_access.rb
@@ -2,6 +2,15 @@
 # for more information on how to write migrations for GitLab.
 
 class MoveFromDevelopersCanMergeToProtectedBranchesMergeAccess < ActiveRecord::Migration
+  DOWNTIME = true
+  DOWNTIME_REASON = <<-HEREDOC
+    We're creating a `merge_access_level` for each `protected_branch`. If a user creates a `protected_branch` while this
+    is running, we might be left with a `protected_branch` _without_ an associated `merge_access_level`. The `protected_branches`
+    table must not change while this is running, so downtime is required.
+
+    https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5081#note_13247410
+  HEREDOC
+
   def up
     execute <<-HEREDOC
       INSERT into protected_branch_merge_access_levels (protected_branch_id, access_level, created_at, updated_at)
diff --git a/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
index 5bc70283f60..56f6159d1d8 100644
--- a/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
+++ b/db/migrate/20160705055308_move_from_developers_can_push_to_protected_branches_push_access.rb
@@ -2,6 +2,15 @@
 # for more information on how to write migrations for GitLab.
 
 class MoveFromDevelopersCanPushToProtectedBranchesPushAccess < ActiveRecord::Migration
+  DOWNTIME = true
+  DOWNTIME_REASON = <<-HEREDOC
+    We're creating a `push_access_level` for each `protected_branch`. If a user creates a `protected_branch` while this
+    is running, we might be left with a `protected_branch` _without_ an associated `push_access_level`. The `protected_branches`
+    table must not change while this is running, so downtime is required.
+
+    https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/5081#note_13247410
+  HEREDOC
+
   def up
     execute <<-HEREDOC
       INSERT into protected_branch_push_access_levels (protected_branch_id, access_level, created_at, updated_at)
diff --git a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
index ad6ad43686d..f563f660ddf 100644
--- a/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
+++ b/db/migrate/20160705055809_remove_developers_can_push_from_protected_branches.rb
@@ -2,7 +2,18 @@
 # for more information on how to write migrations for GitLab.
 
 class RemoveDevelopersCanPushFromProtectedBranches < ActiveRecord::Migration
-  def change
+  include Gitlab::Database::MigrationHelpers
+
+  # This is only required for `#down`
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  def up
     remove_column :protected_branches, :developers_can_push, :boolean
   end
+
+  def down
+    add_column_with_default(:protected_branches, :developers_can_push, :boolean, default: false, null: false)
+  end
 end
diff --git a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
index 084914e423a..aa71e06d36e 100644
--- a/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
+++ b/db/migrate/20160705055813_remove_developers_can_merge_from_protected_branches.rb
@@ -2,7 +2,18 @@
 # for more information on how to write migrations for GitLab.
 
 class RemoveDevelopersCanMergeFromProtectedBranches < ActiveRecord::Migration
-  def change
+  include Gitlab::Database::MigrationHelpers
+
+  # This is only required for `#down`
+  disable_ddl_transaction!
+
+  DOWNTIME = false
+
+  def up
     remove_column :protected_branches, :developers_can_merge, :boolean
   end
+
+  def down
+    add_column_with_default(:protected_branches, :developers_can_merge, :boolean, default: false, null: false)
+  end
 end