Skip to content

G
gitlab-ce
Commit 656ee7be authored by Jacob Schatz's avatar Jacob Schatz
Browse files
Options

Merge branch '919-protected-branch-dropdown-all-users' into 'master' 

Resolve "Protected branches drop down does not show all users"

This MR refactors the implementation introduced in https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/581

Issues like #919 makes a refactor needed since the dropdowns were becoming too complex to maintain.

- Now the related dropdowns are based directly from `ProtectedBranchAccessDropdown` and its internal state is handled with JS only unlike its previous implementation that was a mix of JS and HTML inputs.
- Now selected users are appended making them always visible.

Closes #919

See merge request !687
parents 299ec600 532cf172
Expand all Hide whitespace changes
Inline Side-by-side
Showing with 354 additions and 249 deletions
app/assets/javascripts/allowed_to_merge_dropdown.js.es6 deleted 100644 → 0
View file @ 299ec600
/*= require protected_branch_access_dropdown */
(global => {
global.gl = global.gl || {};
class AllowedToMergeDropdown extends gl.ProtectedBranchAccessDropdown {
}
global.gl.AllowedToMergeDropdown = AllowedToMergeDropdown;
})(window);
app/assets/javascripts/allowed_to_push_dropdown.js.es6 deleted 100644 → 0
View file @ 299ec600
/*= require protected_branch_access_dropdown */
(global => {
global.gl = global.gl || {};
class AllowedToPushDropdown extends gl.ProtectedBranchAccessDropdown {
}
global.gl.AllowedToPushDropdown = AllowedToPushDropdown;
})(window);
app/assets/javascripts/gl_dropdown.js
View file @ 656ee7be
......@@ -352,7 +352,13 @@
if (self.options.clicked) {
self.options.clicked(selected, $el, e);
}
return $el.trigger('blur');
// Update label right after all modifications in dropdown has been done
if (self.options.toggleLabel) {
self.updateLabel(selected, $el, self);
}
$el.trigger('blur');
});
}
}
......@@ -529,7 +535,7 @@
} else {
if (!selected) {
value = this.options.id ? this.options.id(data) : data.id;
fieldName = typeof this.options.fieldName === 'function' ? this.options.fieldName() : this.options.fieldName;
fieldName = this.options.fieldName;
field = this.dropdown.parent().find("input[name='" + fieldName + "'][value='" + value + "']");
if (field.length) {
......@@ -589,6 +595,7 @@
GitLabDropdown.prototype.rowClicked = function(el) {
var field, fieldName, groupName, isInput, selectedIndex, selectedObject, value;
fieldName = this.options.fieldName;
isInput = $(this.el).is('input');
if (this.renderedData) {
groupName = el.data('group');
......@@ -600,7 +607,6 @@
selectedObject = this.renderedData[selectedIndex];
}
}
fieldName = typeof this.options.fieldName === 'function' ? this.options.fieldName(selectedObject) : this.options.fieldName;
value = this.options.id ? this.options.id(selectedObject, el) : selectedObject.id;
if (isInput) {
field = $(this.el);
......@@ -644,11 +650,6 @@
}
}
// Update label right after input has been added
if (this.options.toggleLabel) {
this.updateLabel(selectedObject, el, this);
}
return selectedObject;
};
......@@ -659,9 +660,6 @@
if (this.options.inputId != null) {
$input.attr('id', this.options.inputId);
}
if (selectedObject && selectedObject.type) {
$input.attr('data-type', selectedObject.type);
}
return this.dropdown.before($input);
};
......
app/assets/javascripts/protected_branch_create.js.es6
View file @ 656ee7be
......@@ -10,6 +10,12 @@
constructor() {
this.$wrap = this.$form = $('#new_protected_branch');
this.buildDropdowns();
this.$branchInput = this.$wrap.find('input[name="protected_branch[name]"]');
this.bindEvents();
}
bindEvents() {
this.$form.on('submit', this.onFormSubmit.bind(this));
}
buildDropdowns() {
......@@ -20,19 +26,19 @@
this.onSelectCallback = this.onSelect.bind(this);
// Allowed to Merge dropdown
new gl.AllowedToMergeDropdown({
accessLevel: ACCESS_LEVELS.MERGE,
this[`${ACCESS_LEVELS.MERGE}_dropdown`] = new gl.ProtectedBranchAccessDropdown({
$dropdown: $allowedToMergeDropdown,
accessLevelsData: gon.merge_access_levels,
onSelect: this.onSelectCallback
onSelect: this.onSelectCallback,
accessLevel: ACCESS_LEVELS.MERGE
});
// Allowed to Push dropdown
new gl.AllowedToPushDropdown({
accessLevel: ACCESS_LEVELS.PUSH,
this[`${ACCESS_LEVELS.PUSH}_dropdown`] = new gl.ProtectedBranchAccessDropdown({
$dropdown: $allowedToPushDropdown,
accessLevelsData: gon.push_access_levels,
onSelect: this.onSelectCallback
onSelect: this.onSelectCallback,
accessLevel: ACCESS_LEVELS.PUSH
});
// Protected branch dropdown
......@@ -44,11 +50,58 @@
// Enable submit button after selecting an option
onSelect() {
const $branchInput = this.$wrap.find('input[name="protected_branch[name]"]');
const $allowedToMergeInputs = this.$wrap.find('input[name^="protected_branch[merge_access_levels_attributes]"]');
const $allowedToPushInputs = this.$wrap.find('input[name^="protected_branch[push_access_levels_attributes]"]');
const $allowedToMerge = this[`${ACCESS_LEVELS.MERGE}_dropdown`].getSelectedItems();
const $allowedToPush = this[`${ACCESS_LEVELS.PUSH}_dropdown`].getSelectedItems();
let toggle = !(this.$wrap.find('input[name="protected_branch[name]"]').val() && $allowedToMerge.length && $allowedToPush.length);
this.$form.find('input[type="submit"]').attr('disabled', toggle);
}
getFormData() {
let formData = {
authenticity_token: this.$form.find('input[name="authenticity_token"]').val(),
protected_branch: {
name: this.$wrap.find('input[name="protected_branch[name]"]').val(),
}
};
for (let ACCESS_LEVEL in ACCESS_LEVELS) {
let selectedItems = this[`${ACCESS_LEVELS[ACCESS_LEVEL]}_dropdown`].getSelectedItems();
let levelAttributes = [];
this.$form.find('input[type="submit"]').attr('disabled', !($branchInput.val() && $allowedToMergeInputs.length && $allowedToPushInputs.length));
for (let i = 0; i < selectedItems.length; i++) {
let current = selectedItems[i];
if (current.type === 'user') {
levelAttributes.push({
user_id: selectedItems[i].user_id
});
} else if (current.type === 'role') {
levelAttributes.push({
access_level: selectedItems[i].access_level
});
}
}
formData.protected_branch[`${ACCESS_LEVELS[ACCESS_LEVEL]}_attributes`] = levelAttributes;
}
return formData;
}
onFormSubmit(e) {
e.preventDefault();
$.ajax({
method: 'POST',
data: this.getFormData()
})
.success(() => {
location.reload();
})
.fail(() => {
new Flash('Failed to protect the branch');
});
}
}
......
app/assets/javascripts/protected_branch_edit.js.es6
View file @ 656ee7be
(global => {
global.gl = global.gl || {};
const LEVEL_TYPES = {
USER: 'user',
ROLE: 'role',
};
const ACCESS_LEVELS = {
MERGE: 'merge_access_levels',
PUSH: 'push_access_levels',
......@@ -23,17 +18,11 @@
this.$wraps[ACCESS_LEVELS.PUSH] = this.$allowedToPushDropdown.closest(`.${ACCESS_LEVELS.PUSH}-container`);
this.buildDropdowns();
// Save initial state with existing dropdowns
this.state = {};
for (let ACCESS_LEVEL in ACCESS_LEVELS) {
this.state[`${ACCESS_LEVELS[ACCESS_LEVEL]}_attributes`] = this.getAccessLevelDataFromInputs(ACCESS_LEVEL);
}
}
buildDropdowns() {
// Allowed to merge dropdown
new gl.AllowedToMergeDropdown({
this['merge_access_levels_dropdown'] = new gl.ProtectedBranchAccessDropdown({
accessLevel: ACCESS_LEVELS.MERGE,
accessLevelsData: gon.merge_access_levels,
$dropdown: this.$allowedToMergeDropdown,
......@@ -42,7 +31,7 @@
});
// Allowed to push dropdown
new gl.AllowedToPushDropdown({
this['push_access_levels_dropdown'] = new gl.ProtectedBranchAccessDropdown({
accessLevel: ACCESS_LEVELS.PUSH,
accessLevelsData: gon.push_access_levels,
$dropdown: this.$allowedToPushDropdown,
......@@ -53,25 +42,6 @@
onSelectOption(item, $el, dropdownInstance) {
this.hasChanges = true;
let itemToDestroy;
let accessLevelState = this.state[`${dropdownInstance.accessLevel}_attributes`];
// If element is not active it means it has been active
if (!$el.is('.is-active')) {
// We need to know if the selected item was already saved
// if so we need to append the `_destroy` property
// in order to delete it from the database
// Retrieve the full data of the item we just selected
if (item.type === LEVEL_TYPES.USER) {
itemToDestroy = _.findWhere(accessLevelState, { user_id: item.id });
} else if (item.type === LEVEL_TYPES.ROLE) {
itemToDestroy = _.findWhere(accessLevelState, { access_level: item.id });
}
// State updated by reference
itemToDestroy['_destroy'] = 1;
}
}
onDropdownHide() {
......@@ -86,7 +56,9 @@
let formData = {};
for (let ACCESS_LEVEL in ACCESS_LEVELS) {
formData[`${ACCESS_LEVELS[ACCESS_LEVEL]}_attributes`] = this.consolidateAccessLevelData(ACCESS_LEVEL);
let accessLevelName = ACCESS_LEVELS[ACCESS_LEVEL];
formData[`${accessLevelName}_attributes`] = this[`${accessLevelName}_dropdown`].getInputData(accessLevelName);
}
return $.ajax({
......@@ -102,30 +74,11 @@
this.$wrap.effect('highlight');
this.hasChanges = false;
// Update State
for (let ACCESS_LEVEL in ACCESS_LEVELS) {
let accessLevel = ACCESS_LEVELS[ACCESS_LEVEL];
this.state[`${accessLevel}_attributes`] = [];
for (let i = 0; i < response[accessLevel].length; i++) {
let access = response[accessLevel][i];
let accessData = {};
if (access.user_id) {
accessData = {
id: access.id,
user_id: access.user_id,
};
} else {
accessData ={
id: access.id,
access_level: access.access_level,
};
}
this.state[`${accessLevel}_attributes`].push(accessData);
}
let accessLevelName = ACCESS_LEVELS[ACCESS_LEVEL];
// The data coming from server will be the new persisted *state* for each dropdown
this.setSelectedItemsToDropdown(response[accessLevelName], `${accessLevelName}_dropdown`);
}
},
error() {
......@@ -135,82 +88,41 @@
});
}
consolidateAccessLevelData(accessLevelKey) {
// State takes precedence
let accessLevel = ACCESS_LEVELS[accessLevelKey];
let accessLevelData = [];
let dataFromInputs = this.getAccessLevelDataFromInputs(accessLevelKey);
// Collect and format items that will be sent to the server
for (let i = 0; i < dataFromInputs.length; i++) {
let inState;
let adding;
var userId = parseInt(dataFromInputs[i].user_id);
// Inputs give us the *state* of the dropdown on the frontend before it's persisted
// so we need to compare them with the persisted state which can be get or set on this.state
if (userId) {
adding = LEVEL_TYPES.USER;
inState = _.findWhere(this.state[`${accessLevel}_attributes`], { user_id: userId });
} else {
adding = LEVEL_TYPES.ROLE;
inState = _.findWhere(this.state[`${accessLevel}_attributes`], { access_level: parseInt(dataFromInputs[i].access_level) });
}
if (inState) {
// collect item if it's already saved
accessLevelData.push(inState);
setSelectedItemsToDropdown(items = [], dropdownName) {
let itemsToAdd = [];
for (let i = 0; i < items.length; i++) {
let itemToAdd;
let currentItem = items[i];
if (currentItem.user_id) {
// Solo haciendo esto solo para usuarios por ahora
// obtenemos la data más actual de los items seleccionados
let selectedItems = this[dropdownName].getSelectedItems();
let currentSelectedItem = _.findWhere(selectedItems, { user_id: currentItem.user_id });
itemToAdd = {
id: currentItem.id,
user_id: currentItem.user_id,
type: 'user',
persisted: true,
name: currentSelectedItem.name,
username: currentSelectedItem.username,
avatar_url: currentSelectedItem.avatar_url
}
} else {
// format item according the level type
if (adding === LEVEL_TYPES.USER) {
accessLevelData.push({
user_id: parseInt(dataFromInputs[i].user_id)
});
} else if (adding === LEVEL_TYPES.ROLE) {
accessLevelData.push({
access_level: parseInt(dataFromInputs[i].access_level)
});
itemToAdd = {
id: currentItem.id,
access_level: currentItem.access_level,
type: 'role',
persisted: true
}
}
}
// Since we didn't considered inputs that were removed
// (because they are not present in the DOM anymore)
// We can get them from the state
this.state[`${accessLevel}_attributes`].forEach((item) => {
if (item._destroy) {
accessLevelData.push(item);
}
});
return accessLevelData;
}
getAccessLevelDataFromInputs(accessLevelKey) {
let accessLevels = [];
let accessLevel = ACCESS_LEVELS[accessLevelKey];
this.$wraps[accessLevel]
.find(`input[name^="protected_branch[${accessLevel}_attributes]"]`)
.map((i, el) => {
const $el = $(el);
const type = $el.data('type');
const value = parseInt($el.val());
const id = parseInt($el.data('id'));
let obj = {};
if (type === LEVEL_TYPES.ROLE) {
obj.access_level = value
} else if (type === LEVEL_TYPES.USER) {
obj.user_id = value;
}
if (id) obj.id = id;
accessLevels.push(obj);
});
itemsToAdd.push(itemToAdd);
}
return accessLevels;
this[dropdownName].setSelectedItems(itemsToAdd);
}
}
})(window);
app/controllers/projects/protected_branches_controller.rb
View file @ 656ee7be
......@@ -69,8 +69,8 @@ class Projects::ProtectedBranchesController < Projects::ApplicationController
def access_levels_options
{
push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text, before_divider: true } },
merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text, before_divider: true } },
push_access_levels: ProtectedBranch::PushAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
merge_access_levels: ProtectedBranch::MergeAccessLevel.human_access_levels.map { |id, text| { id: id, text: text } },
selected_merge_access_levels: @protected_branch.merge_access_levels.map { |access_level| access_level.user_id || access_level.access_level },
selected_push_access_levels: @protected_branch.push_access_levels.map { |access_level| access_level.user_id || access_level.access_level }
}
......
app/helpers/branches_helper.rb
View file @ 656ee7be
......@@ -29,4 +29,21 @@ module BranchesHelper
def project_branches
options_for_select(@project.repository.branch_names, @project.default_branch)
end
def access_levels_data(access_levels)
access_levels.map do |level|
if level.type == :user
{
id: level.id,
type: level.type,
user_id: level.user_id,
username: level.user.username,
name: level.user.name,
avatar_url: level.user.avatar_url
}
else
{ id: level.id, type: level.type, access_level: level.access_level }
end
end
end
end
app/views/projects/protected_branches/_access_level_dropdown.html.haml
View file @ 656ee7be
......@@ -3,17 +3,7 @@
%div{ class: "#{input_basic_name}-container" }
- if access_levels.present?
- access_levels.map.with_index do |level, i|
- if level.type == :user
- field_key = 'user_id'
- value = level.user_id
- else
- field_key = 'access_level'
- value = level.access_level
%input{ type: 'hidden', name: "protected_branch[#{input_basic_name}_attributes][#{i}][#{field_key}]",
value: value, data: { type: level.type, id: level.id } }
- dropdown_label = [pluralize(level_frequencies[:role], 'role'), pluralize(level_frequencies[:user], 'user')].to_sentence
= dropdown_tag(dropdown_label, options: { toggle_class: "#{toggle_class} js-multiselect", dropdown_class: 'dropdown-menu-user dropdown-menu-selectable', filter: true,
data: { default_label: default_label } })
data: { default_label: default_label, preselected_items: access_levels_data(access_levels) } })
spec/features/protected_branches/access_control_ee_spec.rb
View file @ 656ee7be
......@@ -17,6 +17,7 @@ RSpec.shared_examples "protected branches > access control > EE" do
click_on "Protect"
within(".protected-branches-list") { expect(page).to have_content('master') }
expect(ProtectedBranch.count).to eq(1)
roles.each { |(access_type_id, _)| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:access_level)).to include(access_type_id) }
users.each { |user| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(user.id) }
......@@ -45,5 +46,39 @@ RSpec.shared_examples "protected branches > access control > EE" do
roles.each { |(access_type_id, _)| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:access_level)).to include(access_type_id) }
users.each { |user| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(user.id) }
end
it "prepends selected users that can #{git_operation} to" do
users = create_list(:user, 21)
users.each { |user| project.team << [user, :developer] }
roles = access_level_class.human_access_levels
visit namespace_project_protected_branches_path(project.namespace, project)
# Create Protected Branch
set_protected_branch_name('master')
set_allowed_to(git_operation, roles.values)
set_allowed_to(other_git_operation)
click_on 'Protect'
# Update Protected Branch
within(".protected-branches-list") do
find(".js-allowed-to-#{git_operation}").click
find(".dropdown-input-field").set(users.last.name) # Find a user that is not loaded
wait_for_ajax
click_on users.last.name
find(".js-allowed-to-#{git_operation}").click # close
end
wait_for_ajax
# Verify the user is appended in the dropdown
find(".protected-branches-list .js-allowed-to-#{git_operation}").click
expect(page).to have_selector '.dropdown-content .is-active', text: users.last.name
expect(ProtectedBranch.count).to eq(1)
roles.each { |(access_type_id, _)| expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:access_level)).to include(access_type_id) }
expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(users.last.id)
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7