Commit cebcc417 authored by Timothy Andrew's avatar Timothy Andrew

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.
parent 0a8aeb46
...@@ -127,7 +127,6 @@ v 8.10.0 ...@@ -127,7 +127,6 @@ v 8.10.0
- Allow to define manual actions/builds on Pipelines and Environments - Allow to define manual actions/builds on Pipelines and Environments
- Fix pagination when sorting by columns with lots of ties (like priority) - 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 - Updated project header design
- Issuable collapsed assignee tooltip is now the users name - Issuable collapsed assignee tooltip is now the users name
- Fix compare view not changing code view rendering style - Fix compare view not changing code view rendering style
......
...@@ -171,6 +171,11 @@ ...@@ -171,6 +171,11 @@
break; break;
case 'search:show': case 'search:show':
new Search(); 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()) { switch (path.first()) {
case 'admin': case 'admin':
......
...@@ -14,6 +14,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base ...@@ -14,6 +14,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
def check_access(user) def check_access(user)
return true if user.is_admin? return true if user.is_admin?
project.team.max_member_access(user.id) >= access_level project.team.max_member_access(user.id) >= access_level
end end
......
...@@ -17,6 +17,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base ...@@ -17,6 +17,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
def check_access(user) def check_access(user)
return false if access_level == Gitlab::Access::NO_ACCESS return false if access_level == Gitlab::Access::NO_ACCESS
return true if user.is_admin? return true if user.is_admin?
project.team.max_member_access(user.id) >= access_level project.team.max_member_access(user.id) >= access_level
end end
......
...@@ -3,7 +3,7 @@ module ProtectedBranches ...@@ -3,7 +3,7 @@ module ProtectedBranches
attr_reader :protected_branch attr_reader :protected_branch
def execute 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) protected_branch = project.protected_branches.new(params)
......
...@@ -3,7 +3,7 @@ module ProtectedBranches ...@@ -3,7 +3,7 @@ module ProtectedBranches
attr_reader :protected_branch attr_reader :protected_branch
def execute(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 = protected_branch
@protected_branch.update(params) @protected_branch.update(params)
......
...@@ -24,6 +24,3 @@ ...@@ -24,6 +24,3 @@
= render partial: @protected_branches, locals: { can_admin_project: can_admin_project } = render partial: @protected_branches, locals: { can_admin_project: can_admin_project }
= paginate @protected_branches, theme: 'gitlab' = paginate @protected_branches, theme: 'gitlab'
:javascript
new ProtectedBranchesAccessSelect($(".protected-branches-list"), true, false);
...@@ -52,6 +52,3 @@ ...@@ -52,6 +52,3 @@
%hr %hr
= render "branches_list" = render "branches_list"
:javascript
new ProtectedBranchesAccessSelect($(".new_protected_branch"), false, true);
...@@ -2,6 +2,8 @@ ...@@ -2,6 +2,8 @@
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
class AddProtectedBranchesPushAccess < ActiveRecord::Migration class AddProtectedBranchesPushAccess < ActiveRecord::Migration
DOWNTIME = false
def change def change
create_table :protected_branch_push_access_levels do |t| 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.references :protected_branch, index: { name: "index_protected_branch_push_access" }, foreign_key: true, null: false
......
...@@ -2,6 +2,8 @@ ...@@ -2,6 +2,8 @@
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
class AddProtectedBranchesMergeAccess < ActiveRecord::Migration class AddProtectedBranchesMergeAccess < ActiveRecord::Migration
DOWNTIME = false
def change def change
create_table :protected_branch_merge_access_levels do |t| 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.references :protected_branch, index: { name: "index_protected_branch_merge_access" }, foreign_key: true, null: false
......
...@@ -2,6 +2,15 @@ ...@@ -2,6 +2,15 @@
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
class MoveFromDevelopersCanMergeToProtectedBranchesMergeAccess < ActiveRecord::Migration 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 def up
execute <<-HEREDOC execute <<-HEREDOC
INSERT into protected_branch_merge_access_levels (protected_branch_id, access_level, created_at, updated_at) INSERT into protected_branch_merge_access_levels (protected_branch_id, access_level, created_at, updated_at)
......
...@@ -2,6 +2,15 @@ ...@@ -2,6 +2,15 @@
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
class MoveFromDevelopersCanPushToProtectedBranchesPushAccess < ActiveRecord::Migration 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 def up
execute <<-HEREDOC execute <<-HEREDOC
INSERT into protected_branch_push_access_levels (protected_branch_id, access_level, created_at, updated_at) INSERT into protected_branch_push_access_levels (protected_branch_id, access_level, created_at, updated_at)
......
...@@ -2,7 +2,18 @@ ...@@ -2,7 +2,18 @@
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
class RemoveDevelopersCanPushFromProtectedBranches < ActiveRecord::Migration 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 remove_column :protected_branches, :developers_can_push, :boolean
end end
def down
add_column_with_default(:protected_branches, :developers_can_push, :boolean, default: false, null: false)
end
end end
...@@ -2,7 +2,18 @@ ...@@ -2,7 +2,18 @@
# for more information on how to write migrations for GitLab. # for more information on how to write migrations for GitLab.
class RemoveDevelopersCanMergeFromProtectedBranches < ActiveRecord::Migration 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 remove_column :protected_branches, :developers_can_merge, :boolean
end end
def down
add_column_with_default(:protected_branches, :developers_can_merge, :boolean, default: false, null: false)
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