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: 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.
---
 .../projects/protected_branches_controller.rb      | 31 +++++++++++++---------
 1 file changed, 19 insertions(+), 12 deletions(-)

(limited to 'app/controllers/projects/protected_branches_controller.rb')

diff --git a/app/controllers/projects/protected_branches_controller.rb b/app/controllers/projects/protected_branches_controller.rb
index ddf1824ccb9..d28ec6e2eac 100644
--- a/app/controllers/projects/protected_branches_controller.rb
+++ b/app/controllers/projects/protected_branches_controller.rb
@@ -3,23 +3,22 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   before_action :require_non_empty_project
   before_action :authorize_admin_project!
   before_action :load_protected_branch, only: [:show, :update, :destroy]
-  before_action :load_protected_branches, only: [:index, :create]
+  before_action :load_protected_branches, only: [:index]
 
   layout "project_settings"
 
   def index
     @protected_branch = @project.protected_branches.new
-    gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } },
-               push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
-               merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } })
+    load_protected_branches_gon_variables
   end
 
   def create
-    service = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params)
-    if service.execute
+    @protected_branch = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params).execute
+    if @protected_branch.persisted?
       redirect_to namespace_project_protected_branches_path(@project.namespace, @project)
     else
-      @protected_branch = service.protected_branch
+      load_protected_branches
+      load_protected_branches_gon_variables
       render :index
     end
   end
@@ -29,15 +28,15 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def update
-    service = ProtectedBranches::UpdateService.new(@project, current_user, params[:id], protected_branch_params)
+    @protected_branch = ProtectedBranches::UpdateService.new(@project, current_user, protected_branch_params).execute(@protected_branch)
 
-    if service.execute
+    if @protected_branch.valid?
       respond_to do |format|
-        format.json { render json: service.protected_branch, status: :ok }
+        format.json { render json: @protected_branch, status: :ok }
       end
     else
       respond_to do |format|
-        format.json { render json: service.protected_branch.errors, status: :unprocessable_entity }
+        format.json { render json: @protected_branch.errors, status: :unprocessable_entity }
       end
     end
   end
@@ -58,10 +57,18 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
   end
 
   def protected_branch_params
-    params.require(:protected_branch).permit(:name, :allowed_to_push, :allowed_to_merge)
+    params.require(:protected_branch).permit(:name,
+                                             merge_access_level_attributes: [:access_level],
+                                             push_access_level_attributes: [:access_level])
   end
 
   def load_protected_branches
     @protected_branches = @project.protected_branches.order(:name).page(params[:page])
   end
+
+  def load_protected_branches_gon_variables
+    gon.push({ open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } },
+               push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
+               merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } } })
+  end
 end
-- 
cgit v1.2.3