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`
This commit is contained in:
		
							parent
							
								
									21bece443d
								
							
						
					
					
						commit
						134fe5af83
					
				|  | @ -3,19 +3,23 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController | ||||||
|   before_action :require_non_empty_project |   before_action :require_non_empty_project | ||||||
|   before_action :authorize_admin_project! |   before_action :authorize_admin_project! | ||||||
|   before_action :load_protected_branch, only: [:show, :update, :destroy] |   before_action :load_protected_branch, only: [:show, :update, :destroy] | ||||||
|  |   before_action :load_protected_branches, only: [:index, :create] | ||||||
| 
 | 
 | ||||||
|   layout "project_settings" |   layout "project_settings" | ||||||
| 
 | 
 | ||||||
|   def index |   def index | ||||||
|     @protected_branches = @project.protected_branches.order(:name).page(params[:page]) |  | ||||||
|     @protected_branch = @project.protected_branches.new |     @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 } } }) | ||||||
|   end |   end | ||||||
| 
 | 
 | ||||||
|   def create |   def create | ||||||
|     @project.protected_branches.create(protected_branch_params) |     service = ProtectedBranches::CreateService.new(@project, current_user, protected_branch_params) | ||||||
|     redirect_to namespace_project_protected_branches_path(@project.namespace, |     if service.execute | ||||||
|                                                           @project) |       redirect_to namespace_project_protected_branches_path(@project.namespace, @project) | ||||||
|  |     else | ||||||
|  |       @protected_branch = service.protected_branch | ||||||
|  |       render :index | ||||||
|  |     end | ||||||
|   end |   end | ||||||
| 
 | 
 | ||||||
|   def show |   def show | ||||||
|  | @ -23,13 +27,15 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController | ||||||
|   end |   end | ||||||
| 
 | 
 | ||||||
|   def update |   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| |       respond_to do |format| | ||||||
|         format.json { render json: @protected_branch, status: :ok } |         format.json { render json: service.protected_branch, status: :ok } | ||||||
|       end |       end | ||||||
|     else |     else | ||||||
|       respond_to do |format| |       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 |     end | ||||||
|   end |   end | ||||||
|  | @ -52,4 +58,8 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController | ||||||
|   def protected_branch_params |   def protected_branch_params | ||||||
|     params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge) |     params.require(:protected_branch).permit(:name, :developers_can_push, :developers_can_merge) | ||||||
|   end |   end | ||||||
|  | 
 | ||||||
|  |   def load_protected_branches | ||||||
|  |     @protected_branches = @project.protected_branches.order(:name).page(params[:page]) | ||||||
|  |   end | ||||||
| end | end | ||||||
|  |  | ||||||
|  | @ -5,13 +5,21 @@ class ProtectedBranch < ActiveRecord::Base | ||||||
|   validates :name, presence: true |   validates :name, presence: true | ||||||
|   validates :project, presence: true |   validates :project, presence: true | ||||||
| 
 | 
 | ||||||
|   has_one :merge_access_level |   has_one :merge_access_level, dependent: :destroy | ||||||
|   has_one :push_access_level |   has_one :push_access_level, dependent: :destroy | ||||||
| 
 | 
 | ||||||
|   def commit |   def commit | ||||||
|     project.commit(self.name) |     project.commit(self.name) | ||||||
|   end |   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. |   # Returns all protected branches that match the given branch name. | ||||||
|   # This realizes all records from the scope built up so far, and does |   # This realizes all records from the scope built up so far, and does | ||||||
|   # _not_ return a relation. |   # _not_ return a relation. | ||||||
|  |  | ||||||
|  | @ -1,3 +1,5 @@ | ||||||
| class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base | class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base | ||||||
|   belongs_to :protected_branch |   belongs_to :protected_branch | ||||||
|  | 
 | ||||||
|  |   enum access_level: [:masters, :developers] | ||||||
| end | end | ||||||
|  |  | ||||||
|  | @ -1,3 +1,5 @@ | ||||||
| class ProtectedBranch::PushAccessLevel < ActiveRecord::Base | class ProtectedBranch::PushAccessLevel < ActiveRecord::Base | ||||||
|   belongs_to :protected_branch |   belongs_to :protected_branch | ||||||
|  | 
 | ||||||
|  |   enum access_level: [:masters, :developers] | ||||||
| end | end | ||||||
|  |  | ||||||
|  | @ -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 | ||||||
|  | @ -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 | ||||||
|  | @ -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 | ||||||
		Loading…
	
		Reference in New Issue