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 000000000000..512d99e48234
--- /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 000000000000..9f82c0a8aa36
--- /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 000000000000..20ca9c3a4884
--- /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 000000000000..498fb393d614
--- /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 000000000000..1e9977cfa6e9
--- /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 000000000000..43d02fbaed69
--- /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 15cee55a7bfe..7a5eded8e024 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -867,13 +867,29 @@
   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 @@
   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
-- 
GitLab


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 b7011d7afdfc..a411cb417e29 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 000000000000..78cec5bf5666
--- /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 000000000000..d53c4c391e34
--- /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
-- 
GitLab


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 10dca47fdede..fdbe0044d3cd 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 @@ def show
   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 @@ def load_protected_branch
   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 a411cb417e29..b0fde6c6c1b2 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 78cec5bf5666..cfaa9c166fe1 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 d53c4c391e34..4345dc4ede4a 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 000000000000..d4be8698a5fc
--- /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 000000000000..743f7bd2ce1a
--- /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 000000000000..ed59b06b79a1
--- /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
-- 
GitLab


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 000000000000..103c7f9445c0
--- /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
-- 
GitLab


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 000000000000..b472ff7ec277
--- /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 cc3aef5199ec..c6e73dd1f397 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 fdbe0044d3cd..126358bfe778 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -56,7 +56,7 @@ def load_protected_branch
   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 4345dc4ede4a..8861632c055d 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 d4be8698a5fc..3a7c35327fef 100644
--- a/app/services/protected_branches/base_service.rb
+++ b/app/services/protected_branches/base_service.rb
@@ -1,17 +1,21 @@
 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'
+      case params[:allowed_to_merge]
+      when 'masters'
         @protected_branch.merge_access_level.masters!
-      elsif params[:developers_can_merge] == '1'
+      when 'developers'
         @protected_branch.merge_access_level.developers!
       end
+
+      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
 end
diff --git a/app/services/protected_branches/create_service.rb b/app/services/protected_branches/create_service.rb
index 743f7bd2ce1a..ab462f3054eb 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!
-    end
+        set_access_levels!
+      end
 
-    true
-  rescue ActiveRecord::RecordInvalid
-    false
+      true
+    rescue ActiveRecord::RecordInvalid
+      false
+    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 720d67dff7c4..5f9f2992dcae 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 7fda7f960473..ceae182e82cc 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"
-- 
GitLab


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            | 78 +++++++++++--------
 spec/lib/gitlab/user_access_spec.rb           |  4 +-
 6 files changed, 89 insertions(+), 40 deletions(-)

diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb
index cfaa9c166fe1..2d13d8c83812 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 8861632c055d..5a4a33556ce3 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 c0f85e9b3a85..3a69027368fd 100644
--- a/lib/gitlab/user_access.rb
+++ b/lib/gitlab/user_access.rb
@@ -29,8 +29,9 @@ def allowed?
     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 @@ def can_push_to_branch?(ref)
     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 28ed8078157f..24a9b78f0c2d 100644
--- a/spec/factories/protected_branches.rb
+++ b/spec/factories/protected_branches.rb
@@ -2,5 +2,22 @@
   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 ae064a878b03..324bb5000257 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -217,29 +217,32 @@ def self.run_permission_checks(permissions_matrix)
         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 }))
-          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 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)
+              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
@@ -249,44 +252,51 @@ def self.run_permission_checks(permissions_matrix)
         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 }
 
-          it { expect(subject).not_to be_allowed }
-        end
+    context 'push code' do
+      subject { access.check('git-receive-pack') }
 
-        context 'when unauthorized' do
-          context 'to public project' do
-            let(:project) { create(:project, :public) }
+      context 'when project is authorized' do
+        before { key.projects << project }
 
-            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 'when unauthorized' do
+        context 'to public project' do
+          let(:project) { create(:project, :public) }
 
-            it { expect(subject).not_to be_allowed }
-          end
+          it { expect(subject).not_to be_allowed }
+        end
 
-          context 'to private project' do
-            let(:project) { create(:project, :internal) }
+        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 private project' do
+          let(:project) { create(:project, :internal) }
+
+          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 aa9ec243498b..5bb095366fa1 100644
--- a/spec/lib/gitlab/user_access_spec.rb
+++ b/spec/lib/gitlab/user_access_spec.rb
@@ -44,7 +44,7 @@
 
     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 '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
-- 
GitLab


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 b472ff7ec277..7c6f2f9f38e1 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 c6e73dd1f397..4409477916f4 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 b0fde6c6c1b2..c0bee72b4d70 100644
--- a/app/models/protected_branch.rb
+++ b/app/models/protected_branch.rb
@@ -12,12 +12,12 @@ def commit
     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 5f9f2992dcae..9240f1cf92df 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 ceae182e82cc..96533b141af4 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 950df740bbca..cd87f978fb27 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);
-- 
GitLab


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 7c6f2f9f38e1..6df11146ba98 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 96533b141af4..89d606d9e203 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 d94dee0c7976..087e3677169e 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -81,4 +81,79 @@ def set_protected_branch_name(branch_name)
       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
-- 
GitLab


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 e02b50ff9a2d..604737e69340 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -88,9 +88,11 @@ def process_default_branch
 
     # 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 ab462f3054eb..212c21346387 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 ed59b06b79a1..4a2b1be9c938 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 47c0580e0f04..663c270d61f5 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -224,8 +224,10 @@
       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 @@
 
         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 @@
 
         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 @@
 
         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
-- 
GitLab


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 dc44a757b4bc..7aecd7860c51 100644
--- a/app/models/project.rb
+++ b/app/models/project.rb
@@ -874,14 +874,6 @@ def protected_branch?(branch_name)
     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 2d13d8c83812..3d2a69717022 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -8,7 +8,7 @@ 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))
+      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 5a4a33556ce3..d446c1a03f02 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -8,7 +8,7 @@ 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))
+      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 0a42931147dc..4bfb7e92e996 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 324bb5000257..8d7497f76f57 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -230,7 +230,7 @@ def self.run_permission_checks(permissions_matrix)
           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 72b8a4e25bdd..e365e4e98b2b 100644
--- a/spec/models/project_spec.rb
+++ b/spec/models/project_spec.rb
@@ -1095,46 +1095,6 @@
     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') }
 
-- 
GitLab


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 6df11146ba98..2c29513ae610 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 126358bfe778..ddf1824ccb9c 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 3d2a69717022..d536f8163174 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 @@ def check_access(user)
       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 d446c1a03f02..bb46b39b7145 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 @@ def check_access(user)
       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 89d606d9e203..e27dea8145d5 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 087e3677169e..553d1c704617 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -83,11 +83,7 @@ def set_protected_branch_name(branch_name)
   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 @@ def set_protected_branch_name(branch_name)
       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')
-- 
GitLab


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 2c29513ae610..a4d9b6eb616b 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 553d1c704617..d72b62a4962c 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -97,8 +97,6 @@ def set_protected_branch_name(branch_name)
         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 @@ def set_protected_branch_name(branch_name)
         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')
-- 
GitLab


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 fdfaf0527301..436d5bd2948f 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -118,6 +118,14 @@ def master?(user)
     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 d536f8163174..632e47b028fc 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -13,9 +13,9 @@ def self.human_access_levels
 
   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 bb46b39b7145..35d4ad93231c 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -14,9 +14,9 @@ def self.human_access_levels
 
   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 8d7497f76f57..c6f03525aab2 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -151,7 +151,13 @@ def merge_into_protected_branch
     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 @@ def self.run_permission_checks(permissions_matrix)
     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 @@ def self.run_permission_checks(permissions_matrix)
 
         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
 
-- 
GitLab


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 d555d23860dd..c791776a8918 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
-- 
GitLab


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 9240f1cf92df..a6956c8e69f9 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 e27dea8145d5..2fc6081e4485 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 cd87f978fb27..69caed7d9792 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]" }})
 
-- 
GitLab


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 c791776a8918..4f1da451df04 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 db21a19964dc..000000000000
--- 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 a4d9b6eb616b..000000000000
--- 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 000000000000..93b7d7755a7b
--- /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 436d5bd2948f..fdfaf0527301 100644
--- a/app/models/project_team.rb
+++ b/app/models/project_team.rb
@@ -118,14 +118,6 @@ def master?(user)
     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 632e47b028fc..17a3a86c3e1e 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -12,11 +12,15 @@ def self.human_access_levels
   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 35d4ad93231c..22096b133007 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -13,13 +13,16 @@ def self.human_access_levels
   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 3a7c35327fef..f8741fcb3d53 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 512d99e48234..3031574fe2a1 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 9f82c0a8aa36..cf1cdb8b3b62 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 20ca9c3a4884..c2b278ce6739 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 498fb393d614..5bc70283f609 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 1e9977cfa6e9..ad6ad43686d7 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 43d02fbaed69..084914e423af 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
-- 
GitLab


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 c6f03525aab2..8447305a316d 100644
--- a/spec/lib/gitlab/git_access_spec.rb
+++ b/spec/lib/gitlab/git_access_spec.rb
@@ -250,23 +250,21 @@ def self.run_permission_checks(permissions_matrix)
                                      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
-- 
GitLab


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 69caed7d9792..75c2063027a8 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 d72b62a4962c..dac2bcf9efde 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 @@ def set_protected_branch_name(branch_name)
           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 @@ def set_protected_branch_name(branch_name)
           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
-- 
GitLab


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                           | 29 +++++++++-------
 lib/api/entities.rb                           |  6 ++--
 spec/requests/api/branches_spec.rb            |  2 +-
 4 files changed, 54 insertions(+), 17 deletions(-)

diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb
index f8741fcb3d53..a5896587ded9 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 @@ def set_access_levels!
     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 @@ def set_merge_access_levels!
     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 @@ def set_push_access_levels!
         @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 66b853eb342c..4133a1f7a6b6 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -35,6 +35,10 @@ class Branches < Grape::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 @@ class Branches < Grape::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])
-
-        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
+        protected_branch_params = {
+          name: @branch.name,
+          developers_can_push: params[:developers_can_push],
+          developers_can_merge: params[:developers_can_merge]
+        }
+
+        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 e76e7304674d..e51bee5c846d 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -126,11 +126,13 @@ class RepoBranch < Grape::Entity
       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 719da27f9198..e8fd697965f3 100644
--- a/spec/requests/api/branches_spec.rb
+++ b/spec/requests/api/branches_spec.rb
@@ -112,7 +112,7 @@
 
       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
-- 
GitLab


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 a5896587ded9..bdd175e8552b 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 @@ def set_access_levels!
       set_push_access_levels!
     end
 
-    protected
+    private
 
     def set_merge_access_levels!
       case @allowed_to_merge
@@ -56,5 +54,14 @@ def translate_api_params!
           '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 212c21346387..360199064162 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -3,6 +3,8 @@ class CreateService < ProtectedBranches::BaseService
     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 4a2b1be9c938..58f2f774baed 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -4,12 +4,13 @@ class UpdateService < ProtectedBranches::BaseService
 
     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
 
-- 
GitLab


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 663c270d61f5..621eced83f6c 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -7,6 +7,7 @@
   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 "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
 
-- 
GitLab


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 93b7d7755a7b..e98312bbf37d 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 ddf1824ccb9c..d28ec6e2eacc 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 @@ def show
   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 @@ def load_protected_branch
   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 c0bee72b4d70..226b3f543421 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 17a3a86c3e1e..25a6ca6a8ee3 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 22096b133007..1999316aa26d 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 604737e69340..3f6a177bf3af 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -88,10 +88,17 @@ def process_default_branch
 
     # 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 bdd175e8552b..000000000000
--- 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 360199064162..50f79f491ce4 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 58f2f774baed..da4c96b3e5f0 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 a6956c8e69f9..2498b57afb4e 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 2fc6081e4485..498e412235e4 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 75c2063027a8..8270da6cd278 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 3031574fe2a1..5c14d449e71d 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 cf1cdb8b3b62..789e3e042206 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 7a5eded8e024..2d2ae5fd8405 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -868,19 +868,19 @@
   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 4133a1f7a6b6..a77afe634f62 100644
--- a/lib/api/branches.rb
+++ b/lib/api/branches.rb
@@ -53,19 +53,37 @@ class Branches < Grape::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 e51bee5c846d..4eb95d8a2151 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -127,12 +127,12 @@ class RepoBranch < Grape::Entity
 
       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 24a9b78f0c2d..5575852c2d77 100644
--- a/spec/factories/protected_branches.rb
+++ b/spec/factories/protected_branches.rb
@@ -4,20 +4,26 @@
     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 dac2bcf9efde..57734b33a44a 100644
--- a/spec/features/protected_branches_spec.rb
+++ b/spec/features/protected_branches_spec.rb
@@ -91,12 +91,12 @@ def set_protected_branch_name(branch_name)
         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 @@ def set_protected_branch_name(branch_name)
         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 @@ def set_protected_branch_name(branch_name)
         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 @@ def set_protected_branch_name(branch_name)
         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 621eced83f6c..ffa998dffc3e 100644
--- a/spec/services/git_push_service_spec.rb
+++ b/spec/services/git_push_service_spec.rb
@@ -227,8 +227,8 @@
         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 @@
         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 @@
         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
-- 
GitLab


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 4f1da451df04..2b04c15b8274 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 d212d66da1b5..9e6901962c6b 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 25a6ca6a8ee3..b1112ee737df 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -14,6 +14,7 @@ def self.human_access_levels
 
   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 1999316aa26d..6a5e49cf4539 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -17,6 +17,7 @@ def self.human_access_levels
   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 50f79f491ce4..6150a2a83c93 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -3,7 +3,7 @@ class CreateService < BaseService
     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 da4c96b3e5f0..89d8ba601345 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -3,7 +3,7 @@ class UpdateService < BaseService
     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 2498b57afb4e..0603a014008c 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 8270da6cd278..4efe44c72335 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 5c14d449e71d..f27295524e1b 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 789e3e042206..32adfa266cd7 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 c2b278ce6739..fa93936ced76 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 5bc70283f609..56f6159d1d8e 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 ad6ad43686d7..f563f660ddf3 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 084914e423af..aa71e06d36e8 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
-- 
GitLab