Uses the selected values from the controller not the params

Added tests for new merge requests from forked project & owned project

app/assets/javascripts/labels_select.js.coffee

@@ -9,8 +9,6 @@ class @LabelsSelect
       labelUrl = $dropdown.data('labels')
       issueUpdateURL = $dropdown.data('issueUpdate')
       selectedLabel = $dropdown.data('selected')
-      if selectedLabel? and not $dropdown.hasClass 'js-multiselect'
-        selectedLabel = selectedLabel.split(',')
       newLabelField = $('#new_label_name')
       newColorField = $('#new_label_color')
       showNo = $dropdown.data('show-no')

app/helpers/dropdowns_helper.rb

@@ -40,11 +40,7 @@ module DropdownsHelper
   end
 
   def dropdown_toggle(toggle_text, data_attr, options = {})
-    default_label = if options[:data]
-                      options[:data][:default_label]
-                    else
-                      ''
-                    end
+    default_label = data_attr[:default_label]
     content_tag(:button, class: "dropdown-menu-toggle #{options[:toggle_class] if options.has_key?(:toggle_class)}", id: (options[:id] if options.has_key?(:id)), type: "button", data: data_attr) do
       output = content_tag(:span, toggle_text, class: "dropdown-toggle-text #{'is-default' if toggle_text == default_label}")
       output << icon('chevron-down')

app/helpers/issuables_helper.rb

@@ -7,27 +7,16 @@ module IssuablesHelper
     "right-sidebar-#{sidebar_gutter_collapsed? ? 'collapsed' : 'expanded'}"
   end
 
-  def multi_label_name(current_labels, param, default_label)
-    # current_labels may be a string from before
-    if current_labels.is_a?(Array) && current_labels.any?
-      title = current_labels[0].try(:title) || current_labels[0]
+  def multi_label_name(current_labels, selected_param, default_label)
+    if current_labels.any?
+      title = current_labels.first.try(:title)
       if current_labels.count > 1
         "#{title} +#{current_labels.count - 1} more"
       else
         title
       end
-    elsif current_labels.is_a?(String)
-      if current_labels.nil? || current_labels.empty?
-        default_label
-      else
-        current_labels
-      end
     else
-      if !param.empty?
-        param
-      else
-        default_label
-      end
+      selected_param.presence || default_label
     end
   end
 

app/helpers/milestones_helper.rb

@@ -56,13 +56,7 @@ module MilestonesHelper
   end
 
   def milestone_dropdown_selected_text(selected)
-    project = @target_project || @project || @projects
-
-    if selected.is_a? Integer
-      Milestone.of_projects(project).where(id: selected).first.try(:title)
-    else
-      Milestone.of_projects(project).where(title: selected).first.try(:title)
-    end
+    selected.try(:title) unless selected.nil?
   end
 
   def milestone_remaining_days(milestone)

app/views/shared/issuable/_filter.html.haml

@@ -21,10 +21,10 @@
             placeholder: "Search assignee", data: { any_user: "Any Assignee", first_user: (current_user.username if current_user), null_user: true, current_user: true, project_id: (@project.id if @project), selected: params[:assignee_id], field_name: "assignee_id", default_label: "Assignee" } })
 
         .filter-item.inline.milestone-filter
-          = render "shared/issuable/milestone_dropdown", selected: params[:milestone_title], name: :milestone_title, show_any: true, show_upcoming: true
+          = render "shared/issuable/milestone_dropdown", selected: (@issuable_finder.milestones.first unless @issuable_finder.milestones.nil?), name: :milestone_title, show_any: true, show_upcoming: true
 
         .filter-item.inline.labels-filter
-          = render "shared/issuable/label_dropdown", selected_toggle: params[:label_name], data_options: { field_name: "label_name[]" }, show_create: controller.controller_name != "groups"
+          = render "shared/issuable/label_dropdown", selected: @issuable_finder.labels, use_id: false, selected_toggle: params[:label_name], data_options: { field_name: "label_name[]" }, show_create: controller.controller_name != "groups"
 
         .pull-right
           = render 'shared/sort_dropdown'

app/views/shared/issuable/_form.html.haml

+- project = @target_project || @project
 = form_errors(issuable)
 
 .form-group
@@ -52,26 +53,23 @@
         = f.label :assignee_id, "Assignee", class: "control-label #{"col-lg-4" if has_due_date}"
         .col-sm-10{ class: ("col-lg-8" if has_due_date) }
           .issuable-form-select-holder
-            - project = @target_project || @project
             - if issuable.assignee_id
-              = hidden_field_tag("#{issuable.class.model_name.param_key}[assignee_id]", issuable.assignee_id)
+              = f.hidden_field :assignee_id
             = dropdown_tag(user_dropdown_label(issuable.assignee_id, "Assignee"), options: { toggle_class: "js-dropdown-keep-input js-user-search js-issuable-form-dropdown js-assignee-search", title: "Filter by assignee", filter: true, dropdown_class: "dropdown-menu-user dropdown-menu-selectable dropdown-menu-assignee js-filter-submit",
-              placeholder: "Search assignee", data: { first_user: (current_user.username if current_user), null_user: true, current_user: true, project_id: (project.id if project), selected: issuable.assignee_id, field_name: "#{issuable.class.model_name.param_key}[assignee_id]", default_label: "Assignee" } })
+              placeholder: "Search assignee", data: { first_user: (current_user.username if current_user), null_user: true, current_user: true, project_id: (project.try(:id)), selected: issuable.assignee_id, field_name: "#{issuable.class.model_name.param_key}[assignee_id]", default_label: "Assignee" } })
       .form-group.issue-milestone
         = f.label :milestone_id, "Milestone", class: "control-label #{"col-lg-4" if has_due_date}"
         .col-sm-10{ class: ("col-lg-8" if has_due_date) }
           .issuable-form-select-holder
-            = render "shared/issuable/milestone_dropdown", selected: issuable.milestone_id, name: "#{issuable.class.model_name.param_key}[milestone_id]", show_any: false, show_upcoming: false, extra_class: "js-issuable-form-dropdown js-dropdown-keep-input"
+            = render "shared/issuable/milestone_dropdown", selected: issuable.milestone, name: "#{issuable.class.model_name.param_key}[milestone_id]", show_any: false, show_upcoming: false, extra_class: "js-issuable-form-dropdown js-dropdown-keep-input"
       .form-group
         - has_labels = issuable.project.labels.any?
-        - selected_labels = issuable.label_ids.any? ? issuable.label_ids : nil
         - label_dropdown_toggle = issuable.labels.map { |label| label.title }
-        - field_name = "#{issuable.class.model_name.param_key}[label_ids][]"
         = f.label :label_ids, "Labels", class: "control-label #{"col-lg-4" if has_due_date}"
-        = hidden_field_tag field_name, ""
+        = f.hidden_field :label_ids, multiple: true, value: ''
         .col-sm-10{ class: "#{"col-lg-8" if has_due_date} #{'issuable-form-padding-top' if !has_labels}" }
           .issuable-form-select-holder
-            = render "shared/issuable/label_dropdown", classes: ["js-issuable-form-dropdown"], selected: selected_labels, selected_toggle: label_dropdown_toggle, data_options: { field_name: field_name, show_any: "false" }
+            = render "shared/issuable/label_dropdown", classes: ["js-issuable-form-dropdown"], selected: issuable.labels, selected_toggle: label_dropdown_toggle, data_options: { field_name: "#{issuable.class.model_name.param_key}[label_ids][]", show_any: false }
     - if has_due_date
       .col-lg-6
         .form-group

app/views/shared/issuable/_label_dropdown.html.haml

@@ -3,33 +3,28 @@
 - extra_options = local_assigns.fetch(:extra_options, true)
 - filter_submit = local_assigns.fetch(:filter_submit, true)
 - show_footer = local_assigns.fetch(:show_footer, true)
+- use_id = local_assigns.fetch(:use_id, true)
 - data_options = local_assigns.fetch(:data_options, {})
 - classes = local_assigns.fetch(:classes, [])
 - selected = local_assigns.fetch(:selected, nil)
 - selected_toggle = local_assigns.fetch(:selected_toggle, nil)
-- dropdown_data = {toggle: 'dropdown', field_name: "label_name[]", show_no: "true", show_any: "true", selected: selected, project_id: project.try(:id), labels: labels_filter_path, default_label: "Labels"}
+- dropdown_data = {toggle: 'dropdown', field_name: "label_name[]", show_no: "true", show_any: "true", project_id: project.try(:id), labels: labels_filter_path, default_label: "Labels"}
 - dropdown_data.merge!(data_options)
 - classes << 'js-extra-options' if extra_options
 - classes << 'js-filter-submit' if filter_submit
 
-- if selected.present?
-  - labelQuery = { id: selected }
-  - useId = true
-- if selected_toggle.present?
-  - labelQuery = { title: selected_toggle }
-- if labelQuery
-  - selected = selected_labels(project, labelQuery)
+- if selected
   - selected.each do |label|
     - id = label.try(:id) || label
     - title = label.try(:title) || label
-    = hidden_field_tag data_options[:field_name], useId ? id : title, id: nil
+    = hidden_field_tag data_options[:field_name], use_id ? id : title, id: nil
 .dropdown
   %button.dropdown-menu-toggle.js-label-select.js-multiselect{class: classes.join(' '), type: "button", data: dropdown_data}
-    %span.dropdown-toggle-text{ class: ("is-default" if selected.nil? || selected.to_a.size == 0) }
-      = h(multi_label_name(selected.to_a, h(selected_toggle.to_a[0]), "Labels"))
+    %span.dropdown-toggle-text{ class: ("is-default" if selected.nil? || selected.empty?) }
+      = h(multi_label_name(selected, selected_toggle.to_a.first, "Labels"))
     = icon('chevron-down')
   .dropdown-menu.dropdown-select.dropdown-menu-paging.dropdown-menu-labels.dropdown-menu-selectable
     = render partial: "shared/issuable/label_page_default", locals: { title: "Filter by label", show_footer: show_footer, show_create: show_create }
-    - if show_create and project and can?(current_user, :admin_label, project)
+    - if show_create && project && can?(current_user, :admin_label, project)
       = render partial: "shared/issuable/label_page_create"
     = dropdown_loading

app/views/shared/issuable/_milestone_dropdown.html.haml

@@ -2,9 +2,9 @@
 - extra_class = extra_class || ''
 - selected_text = milestone_dropdown_selected_text(selected)
 - if selected.present?
-  = hidden_field_tag(name, selected)
+  = hidden_field_tag(name, selected.id)
 = dropdown_tag(milestone_dropdown_label(selected_text), options: { title: "Filter by milestone", toggle_class: "js-milestone-select js-filter-submit #{extra_class}", filter: true, dropdown_class: "dropdown-menu-selectable",
-  placeholder: "Search milestones", footer_content: project.present?, data: { show_no: true, show_any: show_any, show_upcoming: show_upcoming, field_name: name, selected: selected, project_id: project.try(:id), milestones: milestones_filter_dropdown_path, default_label: "Milestone" } }) do
+  placeholder: "Search milestones", footer_content: project.present?, data: { show_no: true, show_any: show_any, show_upcoming: show_upcoming, field_name: name, selected: selected.try(:title), project_id: project.try(:id), milestones: milestones_filter_dropdown_path, default_label: "Milestone" } }) do
   - if project && project.respond_to?(:namespace)
     %ul.dropdown-footer-list
       - if can? current_user, :admin_milestone, project

app/views/shared/issuable/_sidebar.html.haml

@@ -125,13 +125,13 @@
             - else
               %span.no-value None
           .selectbox.hide-collapsed
-            - selected_labels = params["#{issuable.to_ability_name}[label_names]"].to_a
-            - issuable.labels_array.each do |label|
+            - selected_labels = issuable.labels
+            - issuable.labels.each do |label|
               = hidden_field_tag "#{issuable.to_ability_name}[label_names][]", label.id, id: nil
             .dropdown
               %button.dropdown-menu-toggle.js-label-select.js-multiselect.js-label-sidebar-dropdown{type: "button", data: {toggle: "dropdown", default_label: "Labels", field_name: "#{issuable.to_ability_name}[label_names][]", ability_name: issuable.to_ability_name, show_no: "true", show_any: "true", project_id: (@project.id if @project), issue_update: issuable_json_path(issuable), labels: (namespace_project_labels_path(@project.namespace, @project, :json) if @project)}}
-                %span.dropdown-toggle-text{ class: ("is-default" if issuable.labels_array.empty?)}
-                  = multi_label_name(issuable.labels_array, h(selected_labels[0]), "Labels")
+                %span.dropdown-toggle-text{ class: ("is-default" if issuable.labels.empty?)}
+                  = h(multi_label_name(selected_labels, selected_labels.first, "Labels"))
                 = icon('chevron-down')
               .dropdown-menu.dropdown-select.dropdown-menu-paging.dropdown-menu-labels.dropdown-menu-selectable
                 = render partial: "shared/issuable/label_page_default"

spec/features/issues/form_spec.rb

@@ -23,37 +23,52 @@ describe 'New/edit issue', feature: true, js: true do
       fill_in 'issue_description', with: 'title'
 
       click_button 'Assignee'
+      page.within '.dropdown-menu-user' do
         click_link user.name
-
-      page.find '.js-assignee-search' do
+      end
+      expect(find('input[name="issue[assignee_id]"]', visible: false).value).to match(user.id.to_s)
+      page.within '.js-assignee-search' do
         expect(page).to have_content user.name
       end
 
       click_button 'Milestone'
+      page.within '.issue-milestone' do
         click_link milestone.title
-
-      page.find '.js-milestone-select' do
+      end
+      expect(find('input[name="issue[milestone_id]"]', visible: false).value).to match(milestone.id.to_s)
+      page.within '.js-milestone-select' do
         expect(page).to have_content milestone.title
       end
 
       click_button 'Labels'
+      page.within '.dropdown-menu-labels' do
         click_link label.title
         click_link label2.title
-
-      page.find '.js-label-select' do
-        expect(page).to have_content label2.title
       end
+      page.within '.js-label-select' do
+        expect(page).to have_content label.title
+      end
+      expect(page.all('input[name="issue[label_ids][]"]', visible: false)[1].value).to match(label.id.to_s)
+      expect(page.all('input[name="issue[label_ids][]"]', visible: false)[2].value).to match(label2.id.to_s)
 
       click_button 'Submit issue'
 
-      page.find '.issuable-sidebar' do
+      page.within '.issuable-sidebar' do
+        page.within '.assignee' do
           expect(page).to have_content user.name
+        end
+
+        page.within '.milestone' do
           expect(page).to have_content milestone.title
+        end
+
+        page.within '.labels' do
           expect(page).to have_content label.title
           expect(page).to have_content label2.title
         end
       end
     end
+  end
 
   context 'edit issue' do
     before do
@@ -61,25 +76,44 @@ describe 'New/edit issue', feature: true, js: true do
     end
 
     it 'should allow user to update issue' do
+      expect(find('input[name="issue[assignee_id]"]', visible: false).value).to match(user.id.to_s)
+      expect(find('input[name="issue[milestone_id]"]', visible: false).value).to match(milestone.id.to_s)
+
+      page.within '.js-user-search' do
         expect(page).to have_content user.name
+      end
+
+      page.within '.js-milestone-select' do
         expect(page).to have_content milestone.title
+      end
 
       click_button 'Labels'
+      page.within '.dropdown-menu-labels' do
         click_link label.title
         click_link label2.title
-
-      page.find '.js-label-select' do
-        expect(page).to have_content label2.title
       end
+      page.within '.js-label-select' do
+        expect(page).to have_content label.title
+      end
+      expect(page.all('input[name="issue[label_ids][]"]', visible: false)[1].value).to match(label.id.to_s)
+      expect(page.all('input[name="issue[label_ids][]"]', visible: false)[2].value).to match(label2.id.to_s)
 
       click_button 'Save changes'
 
-      page.find '.issuable-sidebar' do
+      page.within '.issuable-sidebar' do
+        page.within '.assignee' do
           expect(page).to have_content user.name
+        end
+
+        page.within '.milestone' do
           expect(page).to have_content milestone.title
+        end
+
+        page.within '.labels' do
           expect(page).to have_content label.title
           expect(page).to have_content label2.title
         end
       end
     end
+  end
 end

spec/features/merge_requests/form_spec.rb

@@ -13,6 +13,73 @@ describe 'New/edit merge request', feature: true, js: true do
   end
 
   context 'owned projects' do
+    before do
+      login_as(user)
+    end
+    
+    context 'new merge request' do
+      before do
+        visit new_namespace_project_merge_request_path(
+          project.namespace,
+          project,
+          merge_request: {
+            source_project_id: project.id,
+            target_project_id: project.id,
+            source_branch: 'fix',
+            target_branch: 'master'
+          })
+      end
+
+      it 'should create new merge request' do
+        click_button 'Assignee'
+        page.within '.dropdown-menu-user' do
+          click_link user.name
+        end
+        expect(find('input[name="merge_request[assignee_id]"]', visible: false).value).to match(user.id.to_s)
+        page.within '.js-assignee-search' do
+          expect(page).to have_content user.name
+        end
+
+        click_button 'Milestone'
+        page.within '.issue-milestone' do
+          click_link milestone.title
+        end
+        expect(find('input[name="merge_request[milestone_id]"]', visible: false).value).to match(milestone.id.to_s)
+        page.within '.js-milestone-select' do
+          expect(page).to have_content milestone.title
+        end
+
+        click_button 'Labels'
+        page.within '.dropdown-menu-labels' do
+          click_link label.title
+          click_link label2.title
+        end
+        page.within '.js-label-select' do
+          expect(page).to have_content label.title
+        end
+        expect(page.all('input[name="merge_request[label_ids][]"]', visible: false)[1].value).to match(label.id.to_s)
+        expect(page.all('input[name="merge_request[label_ids][]"]', visible: false)[2].value).to match(label2.id.to_s)
+
+        click_button 'Submit merge request'
+
+        page.within '.issuable-sidebar' do
+          page.within '.assignee' do
+            expect(page).to have_content user.name
+          end
+
+          page.within '.milestone' do
+            expect(page).to have_content milestone.title
+          end
+
+          page.within '.labels' do
+            expect(page).to have_content label.title
+            expect(page).to have_content label2.title
+          end
+        end
+      end
+    end
+
+    context 'edit merge request' do
       before do
         merge_request = create(:merge_request,
                                  source_project: project,
@@ -21,49 +88,129 @@ describe 'New/edit merge request', feature: true, js: true do
                                  target_branch: 'master'
                               )
 
-      login_as(user)
-
         visit edit_namespace_project_merge_request_path(project.namespace, project, merge_request)
       end
 
       it 'should update merge request' do
         click_button 'Assignee'
+        page.within '.dropdown-menu-user' do
           click_link user.name
-
-      page.find '.js-assignee-search' do
+        end
+        expect(find('input[name="merge_request[assignee_id]"]', visible: false).value).to match(user.id.to_s)
+        page.within '.js-assignee-search' do
           expect(page).to have_content user.name
         end
 
         click_button 'Milestone'
+        page.within '.issue-milestone' do
           click_link milestone.title
-
-      page.find '.js-milestone-select' do
+        end
+        expect(find('input[name="merge_request[milestone_id]"]', visible: false).value).to match(milestone.id.to_s)
+        page.within '.js-milestone-select' do
           expect(page).to have_content milestone.title
         end
 
         click_button 'Labels'
+        page.within '.dropdown-menu-labels' do
           click_link label.title
           click_link label2.title
-
-      page.find '.js-label-select' do
-        expect(page).to have_content label2.title
+        end
+        expect(page.all('input[name="merge_request[label_ids][]"]', visible: false)[1].value).to match(label.id.to_s)
+        expect(page.all('input[name="merge_request[label_ids][]"]', visible: false)[2].value).to match(label2.id.to_s)
+        page.within '.js-label-select' do
+          expect(page).to have_content label.title
         end
 
         click_button 'Save changes'
 
-      page.find '.issuable-sidebar' do
+        page.within '.issuable-sidebar' do
+          page.within '.assignee' do
             expect(page).to have_content user.name
+          end
+
+          page.within '.milestone' do
             expect(page).to have_content milestone.title
+          end
+
+          page.within '.labels' do
             expect(page).to have_content label.title
             expect(page).to have_content label2.title
           end
         end
       end
+    end
+  end
 
   context 'forked project' do
     before do
       fork_project.team << [user, :master]
+      login_as(user)
+    end
+
+    context 'new merge request' do
+      before do
+        visit new_namespace_project_merge_request_path(
+          fork_project.namespace,
+          fork_project,
+          merge_request: {
+            source_project_id: fork_project.id,
+            target_project_id: project.id,
+            source_branch: 'fix',
+            target_branch: 'master'
+          })
+      end
 
+      it 'should create new merge request' do
+        click_button 'Assignee'
+        page.within '.dropdown-menu-user' do
+          click_link user.name
+        end
+        expect(find('input[name="merge_request[assignee_id]"]', visible: false).value).to match(user.id.to_s)
+        page.within '.js-assignee-search' do
+          expect(page).to have_content user.name
+        end
+
+        click_button 'Milestone'
+        page.within '.issue-milestone' do
+          click_link milestone.title
+        end
+        expect(find('input[name="merge_request[milestone_id]"]', visible: false).value).to match(milestone.id.to_s)
+        page.within '.js-milestone-select' do
+          expect(page).to have_content milestone.title
+        end
+
+        click_button 'Labels'
+        page.within '.dropdown-menu-labels' do
+          click_link label.title
+          click_link label2.title
+        end
+        page.within '.js-label-select' do
+          expect(page).to have_content label.title
+        end
+        expect(page.all('input[name="merge_request[label_ids][]"]', visible: false)[1].value).to match(label.id.to_s)
+        expect(page.all('input[name="merge_request[label_ids][]"]', visible: false)[2].value).to match(label2.id.to_s)
+
+        click_button 'Submit merge request'
+
+        page.within '.issuable-sidebar' do
+          page.within '.assignee' do
+            expect(page).to have_content user.name
+          end
+
+          page.within '.milestone' do
+            expect(page).to have_content milestone.title
+          end
+
+          page.within '.labels' do
+            expect(page).to have_content label.title
+            expect(page).to have_content label2.title
+          end
+        end
+      end
+    end
+
+    context 'edit merge request' do
+      before do
         merge_request = create(:merge_request,
                                  source_project: fork_project,
                                  target_project: project,
@@ -71,42 +218,56 @@ describe 'New/edit merge request', feature: true, js: true do
                                  target_branch: 'master'
                               )
 
-      login_as(user)
-
         visit edit_namespace_project_merge_request_path(project.namespace, project, merge_request)
       end
 
       it 'should update merge request' do
         click_button 'Assignee'
+        page.within '.dropdown-menu-user' do
           click_link user.name
-
-      page.find '.js-assignee-search' do
+        end
+        expect(find('input[name="merge_request[assignee_id]"]', visible: false).value).to match(user.id.to_s)
+        page.within '.js-assignee-search' do
           expect(page).to have_content user.name
         end
 
         click_button 'Milestone'
+        page.within '.issue-milestone' do
           click_link milestone.title
-
-      page.find '.js-milestone-select' do
+        end
+        expect(find('input[name="merge_request[milestone_id]"]', visible: false).value).to match(milestone.id.to_s)
+        page.within '.js-milestone-select' do
           expect(page).to have_content milestone.title
         end
 
         click_button 'Labels'
+        page.within '.dropdown-menu-labels' do
           click_link label.title
           click_link label2.title
-
-      page.find '.js-label-select' do
-        expect(page).to have_content label2.title
+        end
+        expect(page.all('input[name="merge_request[label_ids][]"]', visible: false)[1].value).to match(label.id.to_s)
+        expect(page.all('input[name="merge_request[label_ids][]"]', visible: false)[2].value).to match(label2.id.to_s)
+        page.within '.js-label-select' do
+          expect(page).to have_content label.title
         end
 
         click_button 'Save changes'
 
-      page.find '.issuable-sidebar' do
+        page.within '.issuable-sidebar' do
+          page.within '.assignee' do
             expect(page).to have_content user.name
+          end
+
+          page.within '.milestone' do
             expect(page).to have_content milestone.title
+          end
+
+          page.within '.labels' do
             expect(page).to have_content label.title
             expect(page).to have_content label2.title
           end
         end
       end
+    end
+  end
 end