diff options
author | Timothy Andrew <mail@timothyandrew.net> | 2016-07-29 12:31:15 +0530 |
---|---|---|
committer | Timothy Andrew <mail@timothyandrew.net> | 2016-07-29 12:53:40 +0530 |
commit | 1ab405839fbb97666c97452f7e25987e072c00df (patch) | |
tree | 91748892a824ac21b9e6a96e29d31b71400c3448 | |
parent | eac460ba74940360fddcb45979cf9e830b58cf31 (diff) | |
download | gitlab-ce-18193-no-one-can-push.tar.gz |
Implement final review comments from @rymai.18193-no-one-can-push
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.
12 files changed, 53 insertions, 11 deletions
diff --git a/CHANGELOG b/CHANGELOG index ce50f1529b7..1ec47767b04 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -115,7 +115,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/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 |