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] 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 4f1da451df0..2b04c15b827 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 d212d66da1b..9e6901962c6 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 25a6ca6a8ee..b1112ee737d 100644
--- a/app/models/protected_branch/merge_access_level.rb
+++ b/app/models/protected_branch/merge_access_level.rb
@@ -14,6 +14,7 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
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 1999316aa26..6a5e49cf453 100644
--- a/app/models/protected_branch/push_access_level.rb
+++ b/app/models/protected_branch/push_access_level.rb
@@ -17,6 +17,7 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
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 50f79f491ce..6150a2a83c9 100644
--- a/app/services/protected_branches/create_service.rb
+++ b/app/services/protected_branches/create_service.rb
@@ -3,7 +3,7 @@ module ProtectedBranches
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 da4c96b3e5f..89d8ba60134 100644
--- a/app/services/protected_branches/update_service.rb
+++ b/app/services/protected_branches/update_service.rb
@@ -3,7 +3,7 @@ module ProtectedBranches
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 2498b57afb4..0603a014008 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 8270da6cd27..4efe44c7233 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 5c14d449e71..f27295524e1 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 789e3e04220..32adfa266cd 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 c2b278ce673..fa93936ced7 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 5bc70283f60..56f6159d1d8 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 ad6ad43686d..f563f660ddf 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 084914e423a..aa71e06d36e 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