Add support to groups in protected branch dropdown

Unselect all other roles when selecting “No one”

Update tests and handle “No one” role option

Fix "Projected" to "Protected" misspelling

app/assets/javascripts/protected_branch_create.js.es6

@@ -6,6 +6,12 @@
     PUSH: 'push_access_levels',
   };
 
+  const LEVEL_TYPES = {
+    ROLE: 'role',
+    USER: 'user',
+    GROUP: 'group'
+  };
+
   gl.ProtectedBranchCreate = class {
     constructor() {
       this.$wrap = this.$form = $('#new_protected_branch');
@@ -72,14 +78,18 @@
         for (let i = 0; i < selectedItems.length; i++) {
           let current = selectedItems[i];
 
-          if (current.type === 'user') {
+          if (current.type === LEVEL_TYPES.USER) {
             levelAttributes.push({
               user_id: selectedItems[i].user_id
             });
-          } else if (current.type === 'role') {
+          } else if (current.type === LEVEL_TYPES.ROLE) {
             levelAttributes.push({
               access_level: selectedItems[i].access_level
             });
+          } else if (current.type === LEVEL_TYPES.GROUP) {
+            levelAttributes.push({
+              group_id: selectedItems[i].group_id
+            });
           }
         }
 

app/assets/javascripts/protected_branch_edit.js.es6

@@ -6,6 +6,12 @@
     PUSH: 'push_access_levels',
   };
 
+  const LEVEL_TYPES = {
+    ROLE: 'role',
+    USER: 'user',
+    GROUP: 'group'
+  };
+
   gl.ProtectedBranchEdit = class {
     constructor(options) {
       this.$wraps = {};
@@ -21,6 +27,7 @@
     }
 
     buildDropdowns() {
+
       // Allowed to merge dropdown
       this['merge_access_levels_dropdown'] = new gl.ProtectedBranchAccessDropdown({
         accessLevel: ACCESS_LEVELS.MERGE,
@@ -95,25 +102,33 @@
         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
+
+          // Do this only for users for now
+          // get the current data for selected items 
           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',
+            type: LEVEL_TYPES.USER,
             persisted: true,
             name: currentSelectedItem.name,
             username: currentSelectedItem.username,
             avatar_url: currentSelectedItem.avatar_url
           }
+        } else if (currentItem.group_id) {
+          itemToAdd = {
+            id: currentItem.id,
+            group_id: currentItem.group_id,
+            type: LEVEL_TYPES.GROUP,
+            persisted: true
+          };
         } else {
           itemToAdd = {
             id: currentItem.id,
             access_level: currentItem.access_level,
-            type: 'role',
+            type: LEVEL_TYPES.ROLE,
             persisted: true
           }
         }

app/assets/javascripts/protected_branches_access_select_ee.js.es6

-// Modified version of `UsersSelect` for use with access selection for protected branches.
-//
-// - Selections are sent via AJAX if `saveOnSelect` is `true`
-// - If `saveOnSelect` is `false`, the dropdown element must have a `field-name` data
-//   attribute. The DOM must contain two fields - "#{field-name}[access_level]" and "#{field_name}[user_id]"
-//   where the selections will be stored.
-
-class ProtectedBranchesAccessSelect {
-  constructor(container, saveOnSelect, selectDefault) {
-    this.container = container;
-    this.saveOnSelect = saveOnSelect;
-    this.selectDefault = selectDefault;
-    this.usersPath = "/autocomplete/users.json";
-    this.setupDropdown(".allowed-to-merge", gon.merge_access_levels, gon.selected_merge_access_levels);
-    this.setupDropdown(".allowed-to-push", gon.push_access_levels, gon.selected_push_access_levels);
-  }
-
-  setupDropdown(className, accessLevels, selectedAccessLevels) {
-    this.container.find(className).each((i, element) => {
-      var dropdown = $(element).glDropdown({
-        clicked: _.chain(this.onSelect).partial(element).bind(this).value(),
-        data: (term, callback) => {
-          this.getUsers(term, (users) => {
-            users = _(users).map((user) => _(user).extend({ type: "user" }));
-            accessLevels = _(accessLevels).map((accessLevel) => _(accessLevel).extend({ type: "role" }));
-            var accessLevelsWithUsers = accessLevels.concat("divider", users);
-            callback(_(accessLevelsWithUsers).reject((item) => _.contains(selectedAccessLevels, item.id)));
-          });
-        },
-        filterable: true,
-        filterRemote: true,
-        search: { fields: ['name', 'username'] },
-        selectable: true,
-        toggleLabel: (selected) => $(element).data('default-label'),
-        renderRow: (user) => {
-          if (user.before_divider != null) {
-            return "<li> <a href='#'>" + user.text + " </a> </li>";
-          }
-
-
-          var username = user.username ? "@" + user.username : null;
-          var avatar = user.avatar_url ? user.avatar_url : false;
-          var img = avatar ? "<img src='" + avatar + "' class='avatar avatar-inline' width='30' />" : '';
-
-          var listWithName = "<li> <a href='#' class='dropdown-menu-user-link'> " + img + " <strong class='dropdown-menu-user-full-name'> " + user.name + " </strong>";
-          var listWithUserName = username ? "<span class='dropdown-menu-user-username'> " + username + " </span>" : '';
-          var listClosingTags = "</a> </li>";
-
-          return listWithName + listWithUserName + listClosingTags;
-        }
-      });
-
-      if (this.selectDefault) {
-        $(dropdown).find('.dropdown-toggle-text').text(accessLevels[0].text);
-      }
-    });
-  }
-
-  onSelect(dropdown, selected, element, e) {
-    $(dropdown).find('.dropdown-toggle-text').text(selected.text || selected.name);
-
-    var access_level = selected.type == 'user' ? 40 : selected.id;
-    var user_id = selected.type == 'user' ? selected.id : null;
-
-    if (this.saveOnSelect) {
-      $.ajax({
-        type: "POST",
-        url: $(dropdown).data('url'),
-        dataType: "json",
-        data: {
-          _method: 'PATCH',
-          id: $(dropdown).data('id'),
-          protected_branch: {
-            ["" + ($(dropdown).data('type')) + "_attributes"]: [{
-              access_level: access_level,
-              user_id: user_id
-            }]
-          }
-        },
-        success: function() {
-          var row;
-          row = $(e.target);
-          row.closest('tr').effect('highlight');
-          row.closest('td').find('.access-levels-list').append("<li>" + selected.name + "</li>");
-          location.reload();
-        },
-        error: function() {
-          new Flash("Failed to update branch!", "alert");
-        }
-      });
-    } else {
-      var fieldName = $(dropdown).data('field-name');
-      $("input[name='" + fieldName + "[access_level]']").val(access_level);
-      $("input[name='" + fieldName + "[user_id]']").val(user_id);
-    }
-  }
-
-  getUsers(query, callback) {
-    var url = this.buildUrl(this.usersPath);
-    return $.ajax({
-      url: url,
-      data: {
-        search: query,
-        per_page: 20,
-        active: true,
-        project_id: gon.current_project_id,
-        push_code: true
-      },
-      dataType: "json"
-    }).done(function(users) {
-      callback(users);
-    });
-  }
-
-  buildUrl(url) {
-    if (gon.relative_url_root != null) {
-      url = gon.relative_url_root.replace(/\/$/, '') + url;
-    }
-    return url;
-  }
-}

app/helpers/branches_helper.rb

@@ -41,6 +41,8 @@ module BranchesHelper
           name: level.user.name,
           avatar_url: level.user.avatar_url
         }
+      elsif level.type == :group
+        { id: level.id, type: level.type, group_id: level.group_id }
       else
         { id: level.id, type: level.type, access_level: level.access_level }
       end

spec/features/protected_branches/access_control_ee_spec.rb

@@ -2,6 +2,7 @@ RSpec.shared_examples "protected branches > access control > EE" do
   [['merge', ProtectedBranch::MergeAccessLevel], ['push', ProtectedBranch::PushAccessLevel]].each do |git_operation, access_level_class|
     # Need to set a default for the `git_operation` access level that _isn't_ being tested
     other_git_operation = git_operation == 'merge' ? 'push' : 'merge'
+    roles = git_operation == 'merge' ? access_level_class.human_access_levels : access_level_class.human_access_levels.except(0)
 
     let(:users) { create_list(:user, 5) }
     let(:groups) { create_list(:group, 5) }
@@ -12,8 +13,6 @@ RSpec.shared_examples "protected branches > access control > EE" do
     end
 
     it "allows creating protected branches that roles, users, and groups can #{git_operation} to" do
-      roles = access_level_class.human_access_levels
-
       visit namespace_project_protected_branches_path(project.namespace, project)
 
       set_protected_branch_name('master')
@@ -32,8 +31,6 @@ RSpec.shared_examples "protected branches > access control > EE" do
     end
 
     it "allows updating protected branches so that roles and users can #{git_operation} to it" do
-      roles = access_level_class.human_access_levels
-
       visit namespace_project_protected_branches_path(project.namespace, project)
       set_protected_branch_name('master')
       set_allowed_to('merge')
@@ -42,7 +39,6 @@ RSpec.shared_examples "protected branches > access control > EE" do
       click_on "Protect"
 
       within(".js-protected-branch-edit-form") do
-
         set_allowed_to(git_operation, users.map(&:name))
         set_allowed_to(git_operation, groups.map(&:name))
         set_allowed_to(git_operation, roles.values)
@@ -57,8 +53,6 @@ RSpec.shared_examples "protected branches > access control > EE" do
     end
 
     it "allows updating protected branches so that roles and users cannot #{git_operation} to it" do
-      roles = access_level_class.human_access_levels
-
       visit namespace_project_protected_branches_path(project.namespace, project)
       set_protected_branch_name('master')
 
@@ -84,7 +78,6 @@ RSpec.shared_examples "protected branches > access control > EE" do
     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)
 
@@ -103,7 +96,6 @@ RSpec.shared_examples "protected branches > access control > EE" do
         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
@@ -115,4 +107,48 @@ RSpec.shared_examples "protected branches > access control > EE" do
       expect(ProtectedBranch.last.send("#{git_operation}_access_levels".to_sym).map(&:user_id)).to include(users.last.id)
     end
   end
+
+  context 'When updating a protected branch' do
+    it 'discards other roles when choosing "No one"' do
+      roles = ProtectedBranch::PushAccessLevel.human_access_levels.except(0)
+      visit namespace_project_protected_branches_path(project.namespace, project)
+      set_protected_branch_name('fix')
+      set_allowed_to('merge')
+      set_allowed_to('push', roles.values)
+      click_on "Protect"
+      wait_for_ajax
+
+      roles.each do |(access_type_id, _)|
+        expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(access_type_id)
+      end
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).not_to include(0)
+
+      within(".js-protected-branch-edit-form") do
+        set_allowed_to('push', 'No one')
+      end
+      wait_for_ajax
+
+      roles.each do |(access_type_id, _)|
+        expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).not_to include(access_type_id)
+      end
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(0)
+    end
+  end
+
+  context 'When creating a protected branch' do
+    it 'discards other roles when choosing "No one"' do
+      roles = ProtectedBranch::PushAccessLevel.human_access_levels.except(0)
+      visit namespace_project_protected_branches_path(project.namespace, project)
+      set_protected_branch_name('master')
+      set_allowed_to('merge')
+      set_allowed_to('push', ProtectedBranch::PushAccessLevel.human_access_levels.values) # Last item (No one) should deselect the other ones
+      click_on "Protect"
+      wait_for_ajax
+
+      roles.each do |(access_type_id, _)|
+        expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).not_to include(access_type_id)
+      end
+      expect(ProtectedBranch.last.push_access_levels.map(&:access_level)).to include(0)
+    end
+  end
 end

spec/features/protected_branches_spec.rb

 require 'spec_helper'
 Dir["./spec/features/protected_branches/*.rb"].sort.each { |f| require f }
 
-feature 'Projected Branches', feature: true, js: true do
+feature 'Protected Branches', feature: true, js: true do
   include WaitForAjax
 
   let(:user) { create(:user, :admin) }