Commit 3e88873f authored by Timothy Andrew's avatar Timothy Andrew Committed by Alfredo Sumaran

Implement changes from @dbalexandre's review.

1. Set `gon.current_project_id` in a protected branches controller,
   since that's the only place this is used.

2. Move flash notice to the redirect message.

3. Rename the `js_access_levels` method to `access_levels_options`

4. Scope a `find` while sending a branch protection email.

5. Use scopes to simplify finding users with push/merge access for a
   project.

6. Use `change_column_null`  to avoid an `ActiveRecord::IrreversibleMigration`
parent f18bdbb1
...@@ -25,7 +25,6 @@ class Projects::ApplicationController < ApplicationController ...@@ -25,7 +25,6 @@ class Projects::ApplicationController < ApplicationController
project_path = "#{namespace}/#{id}" project_path = "#{namespace}/#{id}"
@project = Project.find_with_namespace(project_path) @project = Project.find_with_namespace(project_path)
gon.current_project_id = @project.id if @project
if can?(current_user, :read_project, @project) && !@project.pending_delete? if can?(current_user, :read_project, @project) && !@project.pending_delete?
if @project.path_with_namespace != project_path if @project.path_with_namespace != project_path
......
module Projects module Projects
module ProtectedBranches module ProtectedBranches
class MergeAccessLevelsController < Projects::ProtectedBranches::ApplicationController class MergeAccessLevelsController < ProtectedBranches::ApplicationController
before_action :load_protected_branch before_action :load_protected_branch, only: [:destroy]
def destroy def destroy
@merge_access_level = @protected_branch.merge_access_levels.find(params[:id]) @merge_access_level = @protected_branch.merge_access_levels.find(params[:id])
@merge_access_level.destroy @merge_access_level.destroy
flash[:notice] = "Successfully deleted. #{@merge_access_level.humanize} will not be able to merge into this protected branch." redirect_to namespace_project_protected_branch_path(@project.namespace, @project, @protected_branch),
redirect_to namespace_project_protected_branch_path(@project.namespace, @project, @protected_branch) notice: "Successfully deleted. #{@merge_access_level.humanize} will not be able to merge into this protected branch."
end end
end end
end end
......
module Projects module Projects
module ProtectedBranches module ProtectedBranches
class PushAccessLevelsController < Projects::ProtectedBranches::ApplicationController class PushAccessLevelsController < ProtectedBranches::ApplicationController
before_action :load_protected_branch before_action :load_protected_branch, only: [:destroy]
def destroy def destroy
@push_access_level = @protected_branch.push_access_levels.find(params[:id]) @push_access_level = @protected_branch.push_access_levels.find(params[:id])
@push_access_level.destroy @push_access_level.destroy
flash[:notice] = "Successfully deleted. #{@push_access_level.humanize} will not be able to push to this protected branch." redirect_to namespace_project_protected_branch_path(@project.namespace, @project, @protected_branch),
redirect_to namespace_project_protected_branch_path(@project.namespace, @project, @protected_branch) notice: "Successfully deleted. #{@push_access_level.humanize} will not be able to push to this protected branch."
end end
end end
end end
......
...@@ -25,7 +25,7 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController ...@@ -25,7 +25,7 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
def show def show
@matching_branches = @protected_branch.matching(@project.repository.branches) @matching_branches = @protected_branch.matching(@project.repository.branches)
gon.push(js_access_levels) gon.push(access_levels_options)
end end
def update def update
...@@ -78,6 +78,7 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController ...@@ -78,6 +78,7 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
def load_gon_index def load_gon_index
params = { open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } } params = { open_branches: @project.open_branches.map { |br| { text: br.name, id: br.name, title: br.name } } }
params.merge!(current_project_id: @project.id) if @project
gon.push(params.merge(access_levels_options)) gon.push(params.merge(access_levels_options))
end end
end end
...@@ -108,16 +108,8 @@ class ProjectMember < Member ...@@ -108,16 +108,8 @@ class ProjectMember < Member
def delete_member_branch_protection def delete_member_branch_protection
if user.present? && project.present? if user.present? && project.present?
push_access_levels = ProtectedBranch::PushAccessLevel.joins(:protected_branch). project.protected_branches.merge_access_by_user(user).destroy_all
where('protected_branches.project_id' => self.project.id, project.protected_branches.push_access_by_user(user).destroy_all
'protected_branch_push_access_levels.user_id' => self.user.id)
merge_access_levels = ProtectedBranch::MergeAccessLevel.joins(:protected_branch).
where('protected_branches.project_id' => self.project.id,
'protected_branch_merge_access_levels.user_id' => self.user.id)
push_access_levels.destroy_all
merge_access_levels.destroy_all
end end
end end
......
...@@ -14,6 +14,20 @@ class ProtectedBranch < ActiveRecord::Base ...@@ -14,6 +14,20 @@ class ProtectedBranch < ActiveRecord::Base
accepts_nested_attributes_for :push_access_levels accepts_nested_attributes_for :push_access_levels
accepts_nested_attributes_for :merge_access_levels accepts_nested_attributes_for :merge_access_levels
# Scopes
# Returns all merge access levels (for protected branches in scope) that grant merge
# access to the given user.
def self.merge_access_by_user(user)
MergeAccessLevel.joins(:protected_branch).where(protected_branch_id: self.ids).merge(MergeAccessLevel.by_user(user))
end
# Returns all push access levels (for protected branches in scope) that grant push
# access to the given user.
def self.push_access_by_user(user)
PushAccessLevel.joins(:protected_branch).where(protected_branch_id: self.ids).merge(PushAccessLevel.by_user(user))
end
def commit def commit
project.commit(self.name) project.commit(self.name)
end end
......
...@@ -8,6 +8,8 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base ...@@ -8,6 +8,8 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER, validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER,
Gitlab::Access::DEVELOPER] } Gitlab::Access::DEVELOPER] }
scope :by_user, -> (user) { where(user: user ) }
def self.human_access_levels def self.human_access_levels
{ {
Gitlab::Access::MASTER => "Masters", Gitlab::Access::MASTER => "Masters",
......
...@@ -9,6 +9,8 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base ...@@ -9,6 +9,8 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
Gitlab::Access::DEVELOPER, Gitlab::Access::DEVELOPER,
Gitlab::Access::NO_ACCESS] } Gitlab::Access::NO_ACCESS] }
scope :by_user, -> (user) { where(user: user ) }
def self.human_access_levels def self.human_access_levels
{ {
Gitlab::Access::MASTER => "Masters", Gitlab::Access::MASTER => "Masters",
......
...@@ -18,7 +18,7 @@ class AllowNullsForProtectedBranchAccessLevels < ActiveRecord::Migration ...@@ -18,7 +18,7 @@ class AllowNullsForProtectedBranchAccessLevels < ActiveRecord::Migration
# disable_ddl_transaction! # disable_ddl_transaction!
def change def change
change_column :protected_branch_merge_access_levels, :access_level, :integer, null: true change_column_null :protected_branch_merge_access_levels, :access_level, true
change_column :protected_branch_push_access_levels, :access_level, :integer, null: true change_column_null :protected_branch_push_access_levels, :access_level, true
end end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment