Commit 7b2ad2d5 authored by Timothy Andrew's avatar Timothy Andrew

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.
parent b3a29b31
...@@ -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
- 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 - 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
......
(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);
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")
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");
}
});
}
}
}
...@@ -118,14 +118,6 @@ class ProjectTeam ...@@ -118,14 +118,6 @@ class ProjectTeam
max_member_access(user.id) == Gitlab::Access::MASTER max_member_access(user.id) == Gitlab::Access::MASTER
end 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) def member?(user, min_member_access = nil)
member = !!find_member(user.id) member = !!find_member(user.id)
......
...@@ -12,11 +12,15 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base ...@@ -12,11 +12,15 @@ class ProtectedBranch::MergeAccessLevel < ActiveRecord::Base
end end
def check_access(user) def check_access(user)
if masters? return true if user.is_admin?
user.can?(:push_code, project) if project.team.master_or_greater?(user)
min_member_access = if masters?
Gitlab::Access::MASTER
elsif developers? elsif developers?
user.can?(:push_code, project) if project.team.developer_or_greater?(user) Gitlab::Access::DEVELOPER
end end
project.team.max_member_access(user.id) >= min_member_access
end end
def humanize def humanize
......
...@@ -13,13 +13,16 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base ...@@ -13,13 +13,16 @@ class ProtectedBranch::PushAccessLevel < ActiveRecord::Base
end end
def check_access(user) def check_access(user)
if masters? return false if no_one?
user.can?(:push_code, project) if project.team.master_or_greater?(user) return true if user.is_admin?
min_member_access = if masters?
Gitlab::Access::MASTER
elsif developers? elsif developers?
user.can?(:push_code, project) if project.team.developer_or_greater?(user) Gitlab::Access::DEVELOPER
elsif no_one?
false
end end
project.team.max_member_access(user.id) >= min_member_access
end end
def humanize def humanize
......
module ProtectedBranches module ProtectedBranches
class BaseService < ::BaseService class BaseService < ::BaseService
def set_access_levels! def set_access_levels!
set_merge_access_levels!
set_push_access_levels!
end
protected
def set_merge_access_levels!
case params[:allowed_to_merge] case params[:allowed_to_merge]
when 'masters' when 'masters'
@protected_branch.merge_access_level.masters! @protected_branch.merge_access_level.masters!
when 'developers' when 'developers'
@protected_branch.merge_access_level.developers! @protected_branch.merge_access_level.developers!
end end
end
def set_push_access_levels!
case params[:allowed_to_push] case params[:allowed_to_push]
when 'masters' when 'masters'
@protected_branch.push_access_level.masters! @protected_branch.push_access_level.masters!
......
...@@ -2,8 +2,6 @@ ...@@ -2,8 +2,6 @@
# 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
include Gitlab::Database::MigrationHelpers
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,8 +2,6 @@ ...@@ -2,8 +2,6 @@
# 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
include Gitlab::Database::MigrationHelpers
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,19 +2,6 @@ ...@@ -2,19 +2,6 @@
# 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
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 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,19 +2,6 @@ ...@@ -2,19 +2,6 @@
# 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
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 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,19 +2,6 @@ ...@@ -2,19 +2,6 @@
# 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
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 def change
remove_column :protected_branches, :developers_can_push, :boolean remove_column :protected_branches, :developers_can_push, :boolean
end end
......
...@@ -2,19 +2,6 @@ ...@@ -2,19 +2,6 @@
# 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
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 def change
remove_column :protected_branches, :developers_can_merge, :boolean remove_column :protected_branches, :developers_can_merge, :boolean
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