Refactor Protected Branches dropdown

- Do not use inputs to keep track of state of dropdown. It hurts my brain.
- Keep dropdown state with pure JS.
- Restore unused changes from previous implementation
  - gl_dropdown.js: No need to check if fieldName option is a function.
                    No need to add data-type to input.

app/assets/javascripts/allowed_to_merge_dropdown.js.es6

-/*= 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

-/*= 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

@@ -322,7 +322,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');
         });
       }
     }
@@ -482,7 +488,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) {
@@ -540,6 +546,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');
@@ -551,7 +558,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);
@@ -594,11 +600,6 @@
         }
       }
 
-      // Update label right after input has been added
-      if (this.options.toggleLabel) {
-        this.updateLabel(selectedObject, el, this);
-      }
-
       return selectedObject;
     };
 
@@ -608,9 +609,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

@@ -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 = [];
+
+        for (let i = 0; i < selectedItems.length; i++) {
+          let current = selectedItems[i];
 
-      this.$form.find('input[type="submit"]').attr('disabled', !($branchInput.val() && $allowedToMergeInputs.length && $allowedToPushInputs.length));
+          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

 (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];
+            let accessLevelName = 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);
-            }
+            // 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) });
-        }
+    setSelectedItemsToDropdown(items = [], dropdownName) {
+      let itemsToAdd = [];
 
-        if (inState) {
-          // collect item if it's already saved
-          accessLevelData.push(inState);
-        } 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)
-            });
-          }
-        }
-      }
+      for (let i = 0; i < items.length; i++) {
+        let itemToAdd;
+        let currentItem = items[i];
 
-      // 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);
-        }
-      });
+        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 });
 
-      return accessLevelData;
+          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 {
+          itemToAdd = {
+            id: currentItem.id,
+            access_level: currentItem.access_level,
+            type: 'role',
+            persisted: true
           }
-
-    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

@@ -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

@@ -29,4 +29,25 @@ module BranchesHelper
   def project_branches
     options_for_select(@project.repository.branch_names, @project.default_branch)
   end
+
+  def access_levels_data(access_levels)
+    levels = []
+
+    access_levels.map do |level|
+      if level.type == :user
+        levels << {
+          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
+        levels << { id: level.id, type: level.type, access_level: level.access_level }
+      end
+    end
+
+    levels
+  end
 end

app/views/projects/protected_branches/_access_level_dropdown.html.haml

@@ -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

@@ -17,6 +17,8 @@ RSpec.shared_examples "protected branches > access control > EE" do
 
       click_on "Protect"
 
+      sleep 1
+
       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 +47,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