Code style changes and refactor

app/assets/javascripts/right_sidebar.js

@@ -120,14 +120,14 @@ Sidebar.prototype.todoUpdateDone = function(data) {
       .removeClass('is-loading')
       .enable()
       .attr('aria-label', $el.data(`${attrPrefix}Text`))
-      .attr('data-delete-path', deletePath)
-      .attr('title', $el.data(`${attrPrefix}Text`));
+      .attr('title', $el.data(`${attrPrefix}Text`))
+      .data('deletePath', deletePath);
     if ($el.hasClass('has-tooltip')) {
       $el.tooltip('_fixTitle');
     }
-    if ($el.data(`${attrPrefix}Icon`)) {
+    if (typeof $el.data('isCollapsed') !== 'undefined') {
       $elText.html($el.data(`${attrPrefix}Icon`));
     } else {
       $elText.text($el.data(`${attrPrefix}Text`));

app/controllers/projects/merge_requests/conflicts_controller.rb

@@ -69,6 +69,6 @@ class Projects::MergeRequests::ConflictsController < Projects::MergeRequests::Ap
   end
   def serializer
-    MergeRequestSerializer.new(current_user: current_user, project: merge_request.project)
+    MergeRequestSerializer.new(current_user: current_user, project: project)
   end
 end

app/helpers/issuables_helper.rb

@@ -24,42 +24,40 @@ module IssuablesHelper
   end
   def sidebar_milestone_tooltip_label(milestone)
-    if milestone && milestone[:due_date]
-      "#{milestone[:title]}<br/>#{sidebar_milestone_remaining_days(milestone)}"
-    else
-      _('Milestone') + (milestone ? "<br/>#{milestone[:title]}" : "")
-    end
+    return _('Milestone') unless milestone.present?
+
+    [milestone[:title], sidebar_milestone_remaining_days(milestone) || _('Milestone')].join('<br/>')
   end
   def sidebar_milestone_remaining_days(milestone)
-    due_date_remaining_days(due_date: milestone[:due_date], start_date: milestone[:start_date]) if milestone[:due_date]
+    due_date_with_remaining_days(milestone[:due_date], milestone[:start_date])
   end
   def sidebar_due_date_tooltip_label(due_date)
-    _('Due date') + (due_date ? "<br/>#{due_date_remaining_days(due_date)}" : "")
+    [_('Due date'), due_date_with_remaining_days(due_date)].compact.join('<br/>')
   end
-  def due_date_remaining_days(due_date, start_date = nil)
-    "#{due_date.to_s(:medium)} (#{remaining_days_in_words(due_date: due_date, start_date: start_date)})"
+  def due_date_with_remaining_days(due_date, start_date = nil)
+    return unless due_date
+
+    "#{due_date.to_s(:medium)} (#{remaining_days_in_words(due_date, start_date)})"
   end
   def sidebar_label_filter_path(base_path, label_name)
-    query_params = {label_name: [label_name]}.to_query
+    query_params = { label_name: [label_name] }.to_query
     "#{base_path}?#{query_params}"
   end
   def multi_label_name(current_labels, default_label)
-    if current_labels && current_labels.any?
-      title = current_labels.first.try(:title) || current_labels.first[:title]
-      if current_labels.size > 1
-        "#{title} +#{current_labels.size - 1} more"
-      else
-        title
-      end
+    return default_label if current_labels.blank?
+    title = current_labels.first.try(:title) || current_labels.first[:title]
+
+    if current_labels.size > 1
+      "#{title} +#{current_labels.size - 1} more"
     else
-      default_label
+      title
     end
   end
@@ -214,7 +212,7 @@ module IssuablesHelper
     first, last = labels.partition.with_index { |_, i| i < limit  }
     if labels && labels.any?
-      label_names = first.collect { |l| l[:title] }
+      label_names = first.collect { |label| label.fetch(:title) }
       label_names << "and #{last.size} more" unless last.empty?
       label_names.join(', ')
@@ -385,6 +383,23 @@ module IssuablesHelper
     params[:issuable_template] if issuable_templates(issuable).any? { |template| template[:name] == params[:issuable_template] }
   end
+  def issuable_todo_button_data(issuable, is_collapsed)
+    {
+      todo_text: _('Add todo'),
+      mark_text: _('Mark todo as done'),
+      todo_icon: sprite_icon('todo-add'),
+      mark_icon: sprite_icon('todo-done', css_class: 'todo-undone'),
+      issuable_id: issuable[:id],
+      issuable_type: issuable[:type],
+      create_path: issuable[:create_todo_path],
+      delete_path: issuable.dig(:current_user, :todo, :delete_path),
+      placement: is_collapsed ? 'left' : nil,
+      container: is_collapsed ? 'body' : nil,
+      boundary: 'viewport',
+      is_collapsed: is_collapsed
+    }
+  end
+
   def close_reopen_params(issuable, action)
     {
       issuable.model_name.to_s.underscore => { state_event: action }
@@ -401,16 +416,16 @@ module IssuablesHelper
     end
   end
-  def issuable_sidebar_options(sidebar_data)
+  def issuable_sidebar_options(issuable)
     {
-      endpoint: "#{sidebar_data[:issuable_json_path]}?serializer=sidebar_extras",
-      toggleSubscriptionEndpoint: sidebar_data[:toggle_subscription_path],
-      moveIssueEndpoint: sidebar_data[:move_issue_path],
-      projectsAutocompleteEndpoint: sidebar_data[:projects_autocomplete_path],
-      editable: sidebar_data[:can_edit],
-      currentUser: sidebar_data[:current_user],
+      endpoint: "#{issuable[:issuable_json_path]}?serializer=sidebar_extras",
+      toggleSubscriptionEndpoint: issuable[:toggle_subscription_path],
+      moveIssueEndpoint: issuable[:move_issue_path],
+      projectsAutocompleteEndpoint: issuable[:projects_autocomplete_path],
+      editable: issuable.dig(:current_user, :can_edit),
+      currentUser: issuable[:current_user],
       rootPath: root_path,
-      fullPath: sidebar_data[:project_full_path]
+      fullPath: issuable[:project_full_path]
     }
   end

app/helpers/milestones_helper.rb

@@ -167,7 +167,7 @@ module MilestonesHelper
   def milestone_tooltip_due_date(milestone)
     if milestone.due_date
-      "#{milestone.due_date.to_s(:medium)} (#{remaining_days_in_words(milestone)})"
+      "#{milestone.due_date.to_s(:medium)} (#{remaining_days_in_words(milestone.due_date, milestone.start_date)})"
     else
       _('Milestone')
     end

app/serializers/entity_date_helper.rb

@@ -44,13 +44,10 @@ module EntityDateHelper
   # It returns "Upcoming" for upcoming entities
   # If due date is provided, it returns "# days|weeks|months remaining|ago"
   # If start date is provided and elapsed, with no due date, it returns "# days elapsed"
-  def remaining_days_in_words(entity)
-    start_date = entity.try(:start_date) || entity.try(:[], :start_date)
-    due_date = entity.try(:due_date) || entity.try(:[], :due_date)
-
-    if due_date && due_date.past?
+  def remaining_days_in_words(due_date, start_date = nil)
+    if due_date&.past?
       content_tag(:strong, 'Past due')
-    elsif start_date && start_date.future?
+    elsif start_date&.future?
       content_tag(:strong, 'Upcoming')
     elsif due_date
       is_upcoming = (due_date - Date.today).to_i > 0
@@ -66,7 +63,7 @@ module EntityDateHelper
       remaining_or_ago = is_upcoming ? _("remaining") : _("ago")
       "#{content} #{remaining_or_ago}".html_safe
-    elsif start_date && start_date.past?
+    elsif start_date&.past?
       days = (Date.today - start_date).to_i
       "#{content_tag(:strong, days)} #{'day'.pluralize(days)} elapsed".html_safe
     end

app/serializers/issuable_sidebar_basic_entity.rb

+# frozen_string_literal: true
+
+class IssuableSidebarBasicEntity < Grape::Entity
+  include RequestAwareEntity
+
+  expose :id
+  expose :type do |issuable|
+    issuable.to_ability_name
+  end
+  expose :author_id
+  expose :project_id do |issuable|
+    issuable.project.id
+  end
+  expose :discussion_locked
+  expose :reference do |issuable|
+    issuable.to_reference(issuable.project, full: true)
+  end
+
+  expose :milestone, using: ::API::Entities::Milestone
+  expose :labels, using: LabelEntity
+
+  expose :current_user, if: lambda { |_issuable| current_user } do
+    expose :current_user, merge: true, using: API::Entities::UserBasic
+
+    expose :todo, using: IssuableSidebarTodoEntity do |issuable|
+      current_user.pending_todo_for(issuable)
+    end
+
+    expose :can_edit do |issuable|
+      can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project)
+    end
+
+    expose :can_move do |issuable|
+      issuable.can_move?(current_user)
+    end
+
+    expose :can_admin_label do |issuable|
+      can?(current_user, :admin_label, issuable.project)
+    end
+  end
+
+  expose :issuable_json_path do |issuable|
+    if issuable.is_a?(MergeRequest)
+      project_merge_request_path(issuable.project, issuable.iid, :json)
+    else
+      project_issue_path(issuable.project, issuable.iid, :json)
+    end
+  end
+
+  expose :namespace_path do |issuable|
+    issuable.project.namespace.full_path
+  end
+
+  expose :project_path do |issuable|
+    issuable.project.path
+  end
+
+  expose :project_full_path do |issuable|
+    issuable.project.full_path
+  end
+
+  expose :project_issuables_path do |issuable|
+    project = issuable.project
+    namespace = project.namespace
+
+    if issuable.is_a?(MergeRequest)
+      namespace_project_merge_requests_path(namespace, project)
+    else
+      namespace_project_issues_path(namespace, project)
+    end
+  end
+
+  expose :create_todo_path do |issuable|
+    project_todos_path(issuable.project)
+  end
+
+  expose :project_milestones_path do |issuable|
+    project_milestones_path(issuable.project, :json)
+  end
+
+  expose :project_labels_path do |issuable|
+    project_labels_path(issuable.project, :json, include_ancestor_groups: true)
+  end
+
+  expose :toggle_subscription_path do |issuable|
+    toggle_subscription_path(issuable)
+  end
+
+  expose :move_issue_path do |issuable|
+    move_namespace_project_issue_path(
+      namespace_id: issuable.project.namespace.to_param,
+      project_id: issuable.project,
+      id: issuable
+    )
+  end
+
+  expose :projects_autocomplete_path do |issuable|
+    autocomplete_projects_path(project_id: issuable.project.id)
+  end
+
+  private
+
+  def current_user
+    request.current_user
+  end
+end

app/serializers/issuable_sidebar_entity.rb

-# frozen_string_literal: true
-
-class IssuableSidebarEntity < Grape::Entity
-  include RequestAwareEntity
-
-  with_options if: { include_basic: true } do
-    expose :id
-    expose :type do |issuable|
-      issuable.to_ability_name
-    end
-    expose :author_id
-    expose :project_id do |issuable|
-      issuable.project.id
-    end
-    expose :discussion_locked
-    expose :reference do |issuable|
-      issuable.to_reference(issuable.project, full: true)
-    end
-
-    expose :current_user, using: UserEntity do |issuable|
-      request.current_user
-    end
-
-    # Relationships
-    expose :todo, using: IssuableSidebarTodoEntity do |issuable|
-      request.current_user.pending_todo_for(issuable) if request.current_user
-    end
-    expose :milestone, using: ::API::Entities::Milestone
-    expose :labels, using: LabelEntity
-
-    # Permissions
-    expose :signed_in do |issuable|
-      request.current_user.present?
-    end
-
-    expose :can_edit do |issuable|
-      can?(request.current_user, :"admin_#{issuable.to_ability_name}", issuable.project)
-    end
-
-    expose :can_move do |issuable|
-      request.current_user && issuable.can_move?(request.current_user)
-    end
-
-    expose :can_admin_label do |issuable|
-      can?(request.current_user, :admin_label, issuable.project)
-    end
-
-    # Paths
-    expose :issuable_json_path do |issuable|
-      if issuable.is_a?(MergeRequest)
-        project_merge_request_path(issuable.project, issuable.iid, :json)
-      else
-        project_issue_path(issuable.project, issuable.iid, :json)
-      end
-    end
-
-    expose :namespace_path do |issuable|
-      issuable.project.namespace.full_path
-    end
-
-    expose :project_path do |issuable|
-      issuable.project.path
-    end
-
-    expose :project_full_path do |issuable|
-      issuable.project.full_path
-    end
-
-    expose :project_issuables_path do |issuable|
-      project = issuable.project
-      namespace = project.namespace
-
-      if issuable.is_a?(MergeRequest)
-        namespace_project_merge_requests_path(namespace, project)
-      else
-        namespace_project_issues_path(namespace, project)
-      end
-    end
-
-    expose :create_todo_path do |issuable|
-      project_todos_path(issuable.project)
-    end
-
-    expose :project_milestones_path do |issuable|
-      project_milestones_path(issuable.project, :json)
-    end
-
-    expose :project_labels_path do |issuable|
-      project_labels_path(issuable.project, :json, include_ancestor_groups: true)
-    end
-
-    expose :toggle_subscription_path do |issuable|
-      toggle_subscription_path(issuable)
-    end
-
-    expose :move_issue_path do |issuable|
-      move_namespace_project_issue_path(
-        namespace_id: issuable.project.namespace.to_param,
-        project_id: issuable.project,
-        id: issuable
-      )
-    end
-
-    expose :projects_autocomplete_path do |issuable|
-      autocomplete_projects_path(project_id: issuable.project.id)
-    end
-  end
-
-  with_options if: { include_extras: true } do
-    include TimeTrackableEntity
-
-    expose :participants, using: ::API::Entities::UserBasic do |issuable|
-      issuable.participants(request.current_user)
-    end
-
-    expose :subscribed do |issuable|
-      issuable.subscribed?(request.current_user, issuable.project)
-    end
-  end
-end

app/serializers/issuable_sidebar_extras_entity.rb

+# frozen_string_literal: true
+
+class IssuableSidebarExtrasEntity < Grape::Entity
+  include RequestAwareEntity
+  include TimeTrackableEntity
+
+  expose :participants, using: ::API::Entities::UserBasic do |issuable|
+    issuable.participants(request.current_user)
+  end
+
+  expose :subscribed do |issuable|
+    issuable.subscribed?(request.current_user, issuable.project)
+  end
+end

app/serializers/issue_serializer.rb

@@ -7,10 +7,10 @@ class IssueSerializer < BaseSerializer
   def represent(issue, opts = {})
     entity =
       case opts[:serializer]
-      when 'sidebar', 'sidebar_extras'
-        opts[:include_basic] = (opts[:serializer] == 'sidebar')
-        opts[:include_extras] = (opts[:serializer] == 'sidebar_extras')
-        IssueSidebarEntity
+      when 'sidebar'
+        IssueSidebarBasicEntity
+      when 'sidebar_extras'
+        IssueSidebarExtrasEntity
       when 'board'
         IssueBoardEntity
       else

app/serializers/issue_sidebar_basic_entity.rb

+# frozen_string_literal: true
+
+class IssueSidebarBasicEntity < IssuableSidebarBasicEntity
+  expose :due_date
+  expose :confidential
+end

app/serializers/issue_sidebar_entity.rb

-# frozen_string_literal: true
-
-class IssueSidebarEntity < IssuableSidebarEntity
-  with_options if: { include_basic: true } do
-    expose :due_date
-    expose :confidential
-  end
-
-  with_options if: { include_extras: true } do
-    expose :assignees, using: API::Entities::UserBasic
-  end
-end

app/serializers/issue_sidebar_extras_entity.rb

+# frozen_string_literal: true
+
+class IssueSidebarExtrasEntity < IssuableSidebarExtrasEntity
+  expose :assignees, using: API::Entities::UserBasic
+end

app/serializers/merge_request_serializer.rb

@@ -7,10 +7,10 @@ class MergeRequestSerializer < BaseSerializer
   def represent(merge_request, opts = {})
     entity =
       case opts[:serializer]
-      when 'sidebar', 'sidebar_extras'
-        opts[:include_basic] = (opts[:serializer] == 'sidebar')
-        opts[:include_extras] = (opts[:serializer] == 'sidebar_extras')
-        MergeRequestSidebarEntity
+      when 'sidebar'
+        MergeRequestSidebarBasicEntity
+      when 'sidebar_extras'
+        IssuableSidebarExtrasEntity
       when 'basic'
         MergeRequestBasicEntity
       else

app/serializers/merge_request_sidebar_basic_entity.rb

+# frozen_string_literal: true
+
+class MergeRequestSidebarBasicEntity < IssuableSidebarBasicEntity
+  expose :assignee, if: lambda { |issuable| issuable.assignee } do
+    expose :assignee, merge: true, using: API::Entities::UserBasic
+
+    expose :can_merge do |issuable|
+      issuable.can_be_merged_by?(issuable.assignee)
+    end
+  end
+end

app/serializers/merge_request_sidebar_entity.rb

-# frozen_string_literal: true
-
-class MergeRequestSidebarEntity < IssuableSidebarEntity
-  with_options if: { include_basic: true } do
-    expose :assignee, using: API::Entities::UserBasic
-
-    expose :can_merge do |issuable|
-      issuable.can_be_merged_by?(issuable.assignee) if issuable.assignee
-    end
-  end
-end

app/views/projects/issues/show.html.haml

@@ -88,6 +88,4 @@
   %section.issuable-discussion
     = render 'projects/issues/discussion'
--# `assignees` is required for populating selected assignee values in the select box
-  This is should be removed when sidebar is converted to Vue.
 = render 'shared/issuable/sidebar', issuable_sidebar: @issuable_sidebar, assignees: @issue.assignees

app/views/projects/merge_requests/conflicts/show.html.haml

@@ -6,12 +6,8 @@
 .merge-request-details.issuable-details
   = render "projects/merge_requests/mr_box"
--# `assignees` is required for populating selected assignee values in the select box and rendering the assignee link
-  This is should be removed when sidebar is converted to Vue.
 = render 'shared/issuable/sidebar', issuable_sidebar: @issuable_sidebar, assignees: @merge_request.assignees
-= render 'shared/issuable/sidebar', issuable: @merge_request, issuable_sidebar: @issuable_sidebar
-
 #conflicts{ "v-cloak" => "true", data: { conflicts_path: conflicts_project_merge_request_path(@merge_request.project, @merge_request, format: :json),
     resolve_conflicts_path: resolve_conflicts_project_merge_request_path(@merge_request.project, @merge_request) } }
   .loading{ "v-if" => "isLoading" }

app/views/projects/merge_requests/show.html.haml

@@ -86,8 +86,6 @@
     .mr-loading-status
       = spinner
--# `assignees` is required for populating selected assignee values in the select box and rendering the assignee link
-  This is should be removed when sidebar is converted to Vue.
 = render 'shared/issuable/sidebar', issuable_sidebar: @issuable_sidebar, assignees: @merge_request.assignees
 - if @merge_request.can_be_reverted?(current_user)

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

+-# `assignees` is being passed in for populating selected assignee values in the select box and rendering the assignee link
+  This should be removed when this sidebar is converted to Vue since assignee data is also available in the `issuable_sidebar` hash
+
 - issuable_type = issuable_sidebar[:type]
-- signed_in = issuable_sidebar[:signed_in]
-- can_edit_issuable = issuable_sidebar[:can_edit]
+- signed_in = !!issuable_sidebar.dig(:current_user, :id)
+- can_edit_issuable = issuable_sidebar.dig(:current_user, :can_edit)
 %aside.right-sidebar.js-right-sidebar.js-issuable-sidebar{ data: { signed: { in: signed_in } }, class: sidebar_gutter_collapsed_class, 'aria-live' => 'polite' }
   .issuable-sidebar
@@ -119,7 +122,7 @@
               = icon('chevron-down', 'aria-hidden': 'true')
             .dropdown-menu.dropdown-select.dropdown-menu-paging.dropdown-menu-labels.dropdown-menu-selectable
               = render partial: "shared/issuable/label_page_default"
-              - if issuable_sidebar[:can_admin_label]
+              - if issuable_sidebar.dig(:current_user, :can_admin_label)
                 = render partial: "shared/issuable/label_page_create"
       = render_if_exists 'shared/issuable/sidebar_weight', issuable_sidebar: issuable_sidebar
@@ -149,7 +152,7 @@
               = project_ref
           = clipboard_button(text: project_ref, title: _('Copy reference to clipboard'), placement: "left", boundary: 'viewport')
-      - if issuable_sidebar[:can_move]
+      - if issuable_sidebar.dig(:current_user, :can_move)
         .block.js-sidebar-move-issue-block
           .sidebar-collapsed-icon{ data: { toggle: 'tooltip', placement: 'left', container: 'body', boundary: 'viewport' }, title: _('Move issue') }
             = custom_icon('icon_arrow_right')

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

 - issuable_type = issuable_sidebar[:type]
-- signed_in = issuable_sidebar[:signed_in]
-- can_edit_issuable = issuable_sidebar[:can_edit]
+- signed_in = !!issuable_sidebar.dig(:current_user, :id)
+- can_edit_issuable = issuable_sidebar.dig(:current_user, :can_edit)
 - if issuable_type == "issue"
   #js-vue-sidebar-assignees{ data: { field: "#{issuable_type}[assignee_ids]", signed_in: signed_in } }
@@ -25,7 +25,7 @@
   .value.hide-collapsed
     - if issuable_sidebar[:assignee]
       = link_to_member(@project, assignee, size: 32, extra_class: 'bold') do
-        - if issuable_sidebar[:can_merge]
+        - if issuable_sidebar[:assignee][:can_merge]
           %span.float-right.cannot-be-merged{ data: { toggle: 'tooltip', placement: 'left' }, title: _('Not allowed to merge') }
             = icon('exclamation-triangle', 'aria-hidden': 'true')
         %span.username