From 7b2ad2d5b99d84fc2d2c11a654085afc02a05bb1 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 25 Jul 2016 14:42:52 +0530 Subject: [PATCH] Implement review comments from @dbalexandre. 1. Remove `master_or_greater?` and `developer_or_greater?` in favor of `max_member_access`, which is a lot nicer. 2. Remove a number of instances of `include Gitlab::Database::MigrationHelpers` in migrations that don't need this module. Also remove comments where not necessary. 3. Remove duplicate entry in CHANGELOG. 4. Move `ProtectedBranchAccessSelect` from Coffeescript to ES6. 5. Split the `set_access_levels!` method in two - one each for `merge` and `push` access levels. --- CHANGELOG | 1 - app/assets/javascripts/protected_branches.js | 35 ------------ ...protected_branches_access_select.js.coffee | 40 -------------- .../protected_branches_access_select.js.es6 | 53 +++++++++++++++++++ app/models/project_team.rb | 8 --- .../protected_branch/merge_access_level.rb | 14 +++-- .../protected_branch/push_access_level.rb | 17 +++--- .../protected_branches/base_service.rb | 9 ++++ ...4938_add_protected_branches_push_access.rb | 2 - ...952_add_protected_branches_merge_access.rb | 2 - ...erge_to_protected_branches_merge_access.rb | 13 ----- ..._push_to_protected_branches_push_access.rb | 13 ----- ...lopers_can_push_from_protected_branches.rb | 13 ----- ...opers_can_merge_from_protected_branches.rb | 13 ----- 14 files changed, 81 insertions(+), 152 deletions(-) delete mode 100644 app/assets/javascripts/protected_branches.js delete mode 100644 app/assets/javascripts/protected_branches_access_select.js.coffee create mode 100644 app/assets/javascripts/protected_branches_access_select.js.es6 diff --git a/CHANGELOG b/CHANGELOG index c791776a891..4f1da451df0 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 - - 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 diff --git a/app/assets/javascripts/protected_branches.js b/app/assets/javascripts/protected_branches.js deleted file mode 100644 index db21a19964d..00000000000 --- a/app/assets/javascripts/protected_branches.js +++ /dev/null @@ -1,35 +0,0 @@ -(function() { - $(function() { - return $(".protected-branches-list :checkbox").change(function(e) { - var can_push, id, name, obj, url; - name = $(this).attr("name"); - if (name === "developers_can_push" || name === "developers_can_merge") { - id = $(this).val(); - can_push = $(this).is(":checked"); - url = $(this).data("url"); - return $.ajax({ - type: "PATCH", - url: url, - dataType: "json", - data: { - id: id, - protected_branch: ( - obj = {}, - obj["" + name] = can_push, - obj - ) - }, - success: function() { - var row; - row = $(e.target); - return row.closest('tr').effect('highlight'); - }, - error: function() { - return new Flash("Failed to update branch!", "alert"); - } - }); - } - }); - }); - -}).call(this); diff --git a/app/assets/javascripts/protected_branches_access_select.js.coffee b/app/assets/javascripts/protected_branches_access_select.js.coffee deleted file mode 100644 index a4d9b6eb616..00000000000 --- a/app/assets/javascripts/protected_branches_access_select.js.coffee +++ /dev/null @@ -1,40 +0,0 @@ -class @ProtectedBranchesAccessSelect - constructor: (@container, @saveOnSelect) -> - @container.find(".allowed-to-merge").each (i, element) => - fieldName = $(element).data('field-name') - $(element).glDropdown - data: gon.merge_access_levels - selectable: true - fieldName: fieldName - clicked: _.partial(@onSelect, element) - - @container.find(".allowed-to-push").each (i, element) => - fieldName = $(element).data('field-name') - $(element).glDropdown - data: gon.push_access_levels - selectable: true - fieldName: fieldName - clicked: _.partial(@onSelect, element) - - - onSelect: (dropdown, selected, element, e) => - $(dropdown).find('.dropdown-toggle-text').text(selected.text) - if @saveOnSelect - $.ajax - type: "POST" - url: $(dropdown).data('url') - dataType: "json" - data: - _method: 'PATCH' - id: $(dropdown).data('id') - protected_branch: - "#{$(dropdown).data('type')}": selected.id - - success: -> - row = $(e.target) - row.closest('tr').effect('highlight') - new Flash("Updated protected branch!", "notice") - - error: -> - new Flash("Failed to update branch!", "alert") - diff --git a/app/assets/javascripts/protected_branches_access_select.js.es6 b/app/assets/javascripts/protected_branches_access_select.js.es6 new file mode 100644 index 00000000000..93b7d7755a7 --- /dev/null +++ b/app/assets/javascripts/protected_branches_access_select.js.es6 @@ -0,0 +1,53 @@ +class ProtectedBranchesAccessSelect { + constructor(container, saveOnSelect) { + this.container = container; + this.saveOnSelect = saveOnSelect; + + this.container.find(".allowed-to-merge").each((i, element) => { + var fieldName = $(element).data('field-name'); + return $(element).glDropdown({ + data: gon.merge_access_levels, + selectable: true, + fieldName: fieldName, + clicked: _.chain(this.onSelect).partial(element).bind(this).value() + }); + }); + + + this.container.find(".allowed-to-push").each((i, element) => { + var fieldName = $(element).data('field-name'); + return $(element).glDropdown({ + data: gon.push_access_levels, + selectable: true, + fieldName: fieldName, + clicked: _.chain(this.onSelect).partial(element).bind(this).value() + }); + }); + } + + onSelect(dropdown, selected, element, e) { + $(dropdown).find('.dropdown-toggle-text').text(selected.text); + if (this.saveOnSelect) { + return $.ajax({ + type: "POST", + url: $(dropdown).data('url'), + dataType: "json", + data: { + _method: 'PATCH', + id: $(dropdown).data('id'), + protected_branch: { + ["" + ($(dropdown).data('type'))]: selected.id + } + }, + success: function() { + var row; + row = $(e.target); + return row.closest('tr').effect('highlight'); + }, + error: function() { + return new Flash("Failed to update branch!", "alert"); + } + }); + } + } +} diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 436d5bd2948..fdfaf052730 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -118,14 +118,6 @@ class ProjectTeam max_member_access(user.id) == Gitlab::Access::MASTER end - def master_or_greater?(user) - master?(user) || user.is_admin? - end - - def developer_or_greater?(user) - master_or_greater?(user) || developer?(user) - end - def member?(user, min_member_access = nil) member = !!find_member(user.id) diff --git a/app/models/protected_branch/merge_access_level.rb b/app/models/protected_branch/merge_access_level.rb index 632e47b028f..17a3a86c3e1 100644 --- a/app/models/protected_branch/merge_access_level.rb +++ b/app/models/protected_branch/merge_access_level.rb @@ -12,11 +12,15 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base end def check_access(user) - if masters? - user.can?(:push_code, project) if project.team.master_or_greater?(user) - elsif developers? - user.can?(:push_code, project) if project.team.developer_or_greater?(user) - end + return true if user.is_admin? + + min_member_access = if masters? + Gitlab::Access::MASTER + elsif developers? + Gitlab::Access::DEVELOPER + end + + project.team.max_member_access(user.id) >= min_member_access end def humanize diff --git a/app/models/protected_branch/push_access_level.rb b/app/models/protected_branch/push_access_level.rb index 35d4ad93231..22096b13300 100644 --- a/app/models/protected_branch/push_access_level.rb +++ b/app/models/protected_branch/push_access_level.rb @@ -13,13 +13,16 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base end def check_access(user) - if masters? - user.can?(:push_code, project) if project.team.master_or_greater?(user) - elsif developers? - user.can?(:push_code, project) if project.team.developer_or_greater?(user) - elsif no_one? - false - end + return false if no_one? + return true if user.is_admin? + + min_member_access = if masters? + Gitlab::Access::MASTER + elsif developers? + Gitlab::Access::DEVELOPER + end + + project.team.max_member_access(user.id) >= min_member_access end def humanize diff --git a/app/services/protected_branches/base_service.rb b/app/services/protected_branches/base_service.rb index 3a7c35327fe..f8741fcb3d5 100644 --- a/app/services/protected_branches/base_service.rb +++ b/app/services/protected_branches/base_service.rb @@ -1,13 +1,22 @@ module ProtectedBranches class BaseService < ::BaseService def set_access_levels! + set_merge_access_levels! + set_push_access_levels! + end + + protected + + def set_merge_access_levels! case params[:allowed_to_merge] when 'masters' @protected_branch.merge_access_level.masters! when 'developers' @protected_branch.merge_access_level.developers! end + end + def set_push_access_levels! case params[:allowed_to_push] when 'masters' @protected_branch.push_access_level.masters! diff --git a/db/migrate/20160705054938_add_protected_branches_push_access.rb b/db/migrate/20160705054938_add_protected_branches_push_access.rb index 512d99e4823..3031574fe2a 100644 --- a/db/migrate/20160705054938_add_protected_branches_push_access.rb +++ b/db/migrate/20160705054938_add_protected_branches_push_access.rb @@ -2,8 +2,6 @@ # for more information on how to write migrations for GitLab. class AddProtectedBranchesPushAccess < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - 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 9f82c0a8aa3..cf1cdb8b3b6 100644 --- a/db/migrate/20160705054952_add_protected_branches_merge_access.rb +++ b/db/migrate/20160705054952_add_protected_branches_merge_access.rb @@ -2,8 +2,6 @@ # for more information on how to write migrations for GitLab. class AddProtectedBranchesMergeAccess < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - 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 20ca9c3a488..c2b278ce673 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,19 +2,6 @@ # for more information on how to write migrations for GitLab. class MoveFromDevelopersCanMergeToProtectedBranchesMergeAccess < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - # When using the methods "add_concurrent_index" or "add_column_with_default" - # you must disable the use of transactions as these methods can not run in an - # existing transaction. When using "add_concurrent_index" make sure that this - # method is the _only_ method called in the migration, any other changes - # should go in a separate migration. This ensures that upon failure _only_ the - # index creation fails and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - 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 498fb393d61..5bc70283f60 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,19 +2,6 @@ # for more information on how to write migrations for GitLab. class MoveFromDevelopersCanPushToProtectedBranchesPushAccess < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - # When using the methods "add_concurrent_index" or "add_column_with_default" - # you must disable the use of transactions as these methods can not run in an - # existing transaction. When using "add_concurrent_index" make sure that this - # method is the _only_ method called in the migration, any other changes - # should go in a separate migration. This ensures that upon failure _only_ the - # index creation fails and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - 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 1e9977cfa6e..ad6ad43686d 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,19 +2,6 @@ # for more information on how to write migrations for GitLab. class RemoveDevelopersCanPushFromProtectedBranches < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - # When using the methods "add_concurrent_index" or "add_column_with_default" - # you must disable the use of transactions as these methods can not run in an - # existing transaction. When using "add_concurrent_index" make sure that this - # method is the _only_ method called in the migration, any other changes - # should go in a separate migration. This ensures that upon failure _only_ the - # index creation fails and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - def change remove_column :protected_branches, :developers_can_push, :boolean 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 43d02fbaed6..084914e423a 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,19 +2,6 @@ # for more information on how to write migrations for GitLab. class RemoveDevelopersCanMergeFromProtectedBranches < ActiveRecord::Migration - include Gitlab::Database::MigrationHelpers - - # When using the methods "add_concurrent_index" or "add_column_with_default" - # you must disable the use of transactions as these methods can not run in an - # existing transaction. When using "add_concurrent_index" make sure that this - # method is the _only_ method called in the migration, any other changes - # should go in a separate migration. This ensures that upon failure _only_ the - # index creation fails and can be retried or reverted easily. - # - # To disable transactions uncomment the following line and remove these - # comments: - # disable_ddl_transaction! - def change remove_column :protected_branches, :developers_can_merge, :boolean end