Add Issue and Merge Request titles to Todo items

Only displays the todo body if the todo has a note.
This is to avoid redundant Issue or Merge Request titles
displayed both in the Todo title and body.

10 files changed, 90 insertions, 60 deletions

diff --git a/app/assets/stylesheets/pages/todos.scss b/app/assets/stylesheets/pages/todos.scss
index 7b64c67ae34..ece0ac04baf 100644
--- a/app/assets/stylesheets/pages/todos.scss
+++ b/app/assets/stylesheets/pages/todos.scss
@@ -72,12 +72,7 @@
   @include transition(opacity);
 
   .todo-title {
-    display: flex;
-
     > .title-item {
-      flex: 0 0 auto;
-      margin: 0 2px;
-
       &:first-child {
         margin-left: 0;
       }
@@ -105,8 +100,12 @@
     font-size: 14px;
   }
 
-  .action-name {
-    font-weight: $gl-font-weight-normal;
+  .todo-label,
+  .todo-project {
+    a {
+      color: $blue-600;
+      font-weight: $gl-font-weight-normal;
+    }
   }
 
   .todo-body {
@@ -170,7 +169,7 @@
   }
 }
 
-@include media-breakpoint-down(xs) {
+@include media-breakpoint-down(sm) {
   .todo {
     .avatar {
       display: none;
@@ -179,7 +178,6 @@
 
   .todo-item {
     .todo-title {
-      flex-flow: row wrap;
       margin-bottom: 10px;
 
       .todo-label {
diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb
index 38142bc68cb..f5333bb332e 100644
--- a/app/helpers/todos_helper.rb
+++ b/app/helpers/todos_helper.rb
@@ -33,7 +33,23 @@ module TodosHelper
         todo.target_reference
       end
 
-    link_to text, todo_target_path(todo), class: 'has-tooltip', title: todo.target.title
+    link_to text, todo_target_path(todo)
+  end
+
+  def todo_target_title(todo)
+    if todo.target
+      "\"#{todo.target.title}\""
+    else
+      ""
+    end
+  end
+
+  def todo_parent_path(todo)
+    if todo.parent.is_a?(Group)
+      link_to todo.parent.name, group_path(todo.parent)
+    else
+      link_to_project(todo.project)
+    end
   end
 
   def todo_target_type_name(todo)
diff --git a/app/models/todo.rb b/app/models/todo.rb
index 240c91da5b6..1ec04189482 100644
--- a/app/models/todo.rb
+++ b/app/models/todo.rb
@@ -186,9 +186,9 @@ class Todo < ApplicationRecord
 
   def target_reference
     if for_commit?
-      target.reference_link_text(full: true)
+      target.reference_link_text
     else
-      target.to_reference(full: true)
+      target.to_reference
     end
   end
 
diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml
index 8cdfc7369a0..fdb71d3a221 100644
--- a/app/views/dashboard/todos/_todo.html.haml
+++ b/app/views/dashboard/todos/_todo.html.haml
@@ -2,41 +2,49 @@
   .todo-avatar
     = author_avatar(todo, size: 40)
 
-  .todo-item.todo-block
-    .todo-title.title
+  .todo-item.todo-block.align-self-center
+    .todo-title
       - unless todo.build_failed? || todo.unmergeable?
         = todo_target_state_pill(todo)
 
-        .title-item.author-name
+        %span.title-item.author-name.bold
           - if todo.author
             = link_to_author(todo, self_added: todo.self_added?)
           - else
             (removed)
 
-      .title-item.action-name
+      %span.title-item.action-name
         = todo_action_name(todo)
 
-      .title-item.todo-label
+      %span.title-item.todo-label.todo-target-link
         - if todo.target
           = todo_target_link(todo)
         - else
-          (removed)
+          = _("(removed)")
+
+      %span.title-item.todo-target-title
+        = todo_target_title(todo)
+
+      %span.title-item.todo-project.todo-label
+        at
+        = todo_parent_path(todo)
 
       - if todo.self_assigned?
-        .title-item.action-name
+        %span.title-item.action-name
           to yourself
 
-      .title-item
+      %span.title-item
         &middot;
 
-      .title-item
+      %span.title-item.todo-timestamp
         #{time_ago_with_tooltip(todo.created_at)}
         = todo_due_date(todo)
 
-    .todo-body
-      .todo-note.break-word
-        .md
-          = first_line_in_markdown(todo, :body, 150, project: todo.project)
+    - if todo.note.present?
+      .todo-body
+        .todo-note.break-word
+          .md
+            = first_line_in_markdown(todo, :body, 150, project: todo.project)
 
   - if todo.pending?
     .todo-actions
diff --git a/changelogs/unreleased/todos-include-issue-mr-titles.yml b/changelogs/unreleased/todos-include-issue-mr-titles.yml
new file mode 100644
index 00000000000..a9fb9116070
--- /dev/null
+++ b/changelogs/unreleased/todos-include-issue-mr-titles.yml
@@ -0,0 +1,5 @@
+---
+title: Add Issue and Merge Request titles to Todo items
+merge_request: 30435
+author: Arun Kumar Mohan
+type: changed
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 0568bdbdfca..0cd177f99f6 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -309,6 +309,9 @@ msgstr ""
 msgid "(external source)"
 msgstr ""
 
+msgid "(removed)"
+msgstr ""
+
 msgid "+ %{amount} more"
 msgstr ""
 
diff --git a/spec/features/dashboard/todos/todos_filtering_spec.rb b/spec/features/dashboard/todos/todos_filtering_spec.rb
index f273e416597..efa163042f9 100644
--- a/spec/features/dashboard/todos/todos_filtering_spec.rb
+++ b/spec/features/dashboard/todos/todos_filtering_spec.rb
@@ -31,9 +31,9 @@ describe 'Dashboard > User filters todos', :js do
   end
 
   it 'displays all todos without a filter' do
-    expect(page).to have_content issue1.to_reference(full: true)
-    expect(page).to have_content merge_request.to_reference(full: true)
-    expect(page).to have_content issue2.to_reference(full: true)
+    expect(page).to have_content issue1.to_reference(full: false)
+    expect(page).to have_content merge_request.to_reference(full: false)
+    expect(page).to have_content issue2.to_reference(full: false)
   end
 
   it 'filters by project' do
@@ -58,9 +58,9 @@ describe 'Dashboard > User filters todos', :js do
 
     wait_for_requests
 
-    expect(page).to     have_content issue1.to_reference(full: true)
-    expect(page).to     have_content merge_request.to_reference(full: true)
-    expect(page).not_to have_content issue2.to_reference(full: true)
+    expect(page).to     have_content "issue #{issue1.to_reference} \"issue\" at #{group1.name} / project_1"
+    expect(page).to     have_content "merge request #{merge_request.to_reference}"
+    expect(page).not_to have_content "issue #{issue2.to_reference} \"issue\" at #{group2.name} / project_3"
   end
 
   context 'Author filter' do
diff --git a/spec/features/dashboard/todos/todos_sorting_spec.rb b/spec/features/dashboard/todos/todos_sorting_spec.rb
index 3870c661784..421a66c6d48 100644
--- a/spec/features/dashboard/todos/todos_sorting_spec.rb
+++ b/spec/features/dashboard/todos/todos_sorting_spec.rb
@@ -42,33 +42,33 @@ describe 'Dashboard > User sorts todos' do
       click_link 'Last created'
 
       results_list = page.find('.todos-list')
-      expect(results_list.all('p')[0]).to have_content('merge_request_1')
-      expect(results_list.all('p')[1]).to have_content('issue_1')
-      expect(results_list.all('p')[2]).to have_content('issue_3')
-      expect(results_list.all('p')[3]).to have_content('issue_2')
-      expect(results_list.all('p')[4]).to have_content('issue_4')
+      expect(results_list.all('.todo-title')[0]).to have_content('merge_request_1')
+      expect(results_list.all('.todo-title')[1]).to have_content('issue_1')
+      expect(results_list.all('.todo-title')[2]).to have_content('issue_3')
+      expect(results_list.all('.todo-title')[3]).to have_content('issue_2')
+      expect(results_list.all('.todo-title')[4]).to have_content('issue_4')
     end
 
     it 'sorts with newest created todos first' do
       click_link 'Oldest created'
 
       results_list = page.find('.todos-list')
-      expect(results_list.all('p')[0]).to have_content('issue_4')
-      expect(results_list.all('p')[1]).to have_content('issue_2')
-      expect(results_list.all('p')[2]).to have_content('issue_3')
-      expect(results_list.all('p')[3]).to have_content('issue_1')
-      expect(results_list.all('p')[4]).to have_content('merge_request_1')
+      expect(results_list.all('.todo-title')[0]).to have_content('issue_4')
+      expect(results_list.all('.todo-title')[1]).to have_content('issue_2')
+      expect(results_list.all('.todo-title')[2]).to have_content('issue_3')
+      expect(results_list.all('.todo-title')[3]).to have_content('issue_1')
+      expect(results_list.all('.todo-title')[4]).to have_content('merge_request_1')
     end
 
     it 'sorts by label priority' do
       click_link 'Label priority'
 
       results_list = page.find('.todos-list')
-      expect(results_list.all('p')[0]).to have_content('issue_3')
-      expect(results_list.all('p')[1]).to have_content('merge_request_1')
-      expect(results_list.all('p')[2]).to have_content('issue_1')
-      expect(results_list.all('p')[3]).to have_content('issue_2')
-      expect(results_list.all('p')[4]).to have_content('issue_4')
+      expect(results_list.all('.todo-title')[0]).to have_content('issue_3')
+      expect(results_list.all('.todo-title')[1]).to have_content('merge_request_1')
+      expect(results_list.all('.todo-title')[2]).to have_content('issue_1')
+      expect(results_list.all('.todo-title')[3]).to have_content('issue_2')
+      expect(results_list.all('.todo-title')[4]).to have_content('issue_4')
     end
   end
 
@@ -93,9 +93,9 @@ describe 'Dashboard > User sorts todos' do
       click_link 'Label priority'
 
       results_list = page.find('.todos-list')
-      expect(results_list.all('p')[0]).to have_content('issue_1')
-      expect(results_list.all('p')[1]).to have_content('issue_2')
-      expect(results_list.all('p')[2]).to have_content('merge_request_1')
+      expect(results_list.all('.todo-title')[0]).to have_content('issue_1')
+      expect(results_list.all('.todo-title')[1]).to have_content('issue_2')
+      expect(results_list.all('.todo-title')[2]).to have_content('merge_request_1')
     end
   end
 end
diff --git a/spec/features/dashboard/todos/todos_spec.rb b/spec/features/dashboard/todos/todos_spec.rb
index b98a04b0bda..867281da1e6 100644
--- a/spec/features/dashboard/todos/todos_spec.rb
+++ b/spec/features/dashboard/todos/todos_spec.rb
@@ -3,10 +3,10 @@
 require 'spec_helper'
 
 describe 'Dashboard Todos' do
-  let(:user)    { create(:user) }
+  let(:user)    { create(:user, username: 'john') }
   let(:author)  { create(:user) }
   let(:project) { create(:project, :public) }
-  let(:issue)   { create(:issue, due_date: Date.today) }
+  let(:issue)   { create(:issue, due_date: Date.today, title: "Fix bug") }
 
   context 'User does not have todos' do
     before do
@@ -135,7 +135,7 @@ describe 'Dashboard Todos' do
 
       it 'shows issue assigned to yourself message' do
         page.within('.js-todos-all') do
-          expect(page).to have_content("You assigned issue #{issue.to_reference(full: true)} to yourself")
+          expect(page).to have_content("You assigned issue #{issue.to_reference} \"Fix bug\" at #{project.namespace.owner_name} / #{project.name} to yourself")
         end
       end
     end
@@ -148,7 +148,7 @@ describe 'Dashboard Todos' do
 
       it 'shows you added a todo message' do
         page.within('.js-todos-all') do
-          expect(page).to have_content("You added a todo for issue #{issue.to_reference(full: true)}")
+          expect(page).to have_content("You added a todo for issue #{issue.to_reference} \"Fix bug\" at #{project.namespace.owner_name} / #{project.name}")
           expect(page).not_to have_content('to yourself')
         end
       end
@@ -162,7 +162,7 @@ describe 'Dashboard Todos' do
 
       it 'shows you mentioned yourself message' do
         page.within('.js-todos-all') do
-          expect(page).to have_content("You mentioned yourself on issue #{issue.to_reference(full: true)}")
+          expect(page).to have_content("You mentioned yourself on issue #{issue.to_reference} \"Fix bug\" at #{project.namespace.owner_name} / #{project.name}")
           expect(page).not_to have_content('to yourself')
         end
       end
@@ -176,14 +176,14 @@ describe 'Dashboard Todos' do
 
       it 'shows you directly addressed yourself message' do
         page.within('.js-todos-all') do
-          expect(page).to have_content("You directly addressed yourself on issue #{issue.to_reference(full: true)}")
+          expect(page).to have_content("You directly addressed yourself on issue #{issue.to_reference} \"Fix bug\" at #{project.namespace.owner_name} / #{project.name}")
           expect(page).not_to have_content('to yourself')
         end
       end
     end
 
     context 'approval todo' do
-      let(:merge_request) { create(:merge_request) }
+      let(:merge_request) { create(:merge_request, title: "Fixes issue") }
 
       before do
         create(:todo, :approval_required, user: user, project: project, target: merge_request, author: user)
@@ -192,7 +192,7 @@ describe 'Dashboard Todos' do
 
       it 'shows you set yourself as an approver message' do
         page.within('.js-todos-all') do
-          expect(page).to have_content("You set yourself as an approver for merge request #{merge_request.to_reference(full: true)}")
+          expect(page).to have_content("You set yourself as an approver for merge request #{merge_request.to_reference} \"Fixes issue\" at #{project.namespace.owner_name} / #{project.name}")
           expect(page).not_to have_content('to yourself')
         end
       end
@@ -354,7 +354,7 @@ describe 'Dashboard Todos' do
     it 'links to the pipelines for the merge request' do
       href = pipelines_project_merge_request_path(project, todo.target)
 
-      expect(page).to have_link "merge request #{todo.target.to_reference(full: true)}", href: href
+      expect(page).to have_link "merge request #{todo.target.to_reference}", href: href
     end
   end
 end
diff --git a/spec/models/todo_spec.rb b/spec/models/todo_spec.rb
index 9aeef7c3b4b..ce17704acbd 100644
--- a/spec/models/todo_spec.rb
+++ b/spec/models/todo_spec.rb
@@ -121,12 +121,12 @@ describe Todo do
       subject.target_type = 'Commit'
       subject.commit_id = commit.id
 
-      expect(subject.target_reference).to eq commit.reference_link_text(full: true)
+      expect(subject.target_reference).to eq commit.reference_link_text(full: false)
     end
 
     it 'returns full reference for issuables' do
       subject.target = issue
-      expect(subject.target_reference).to eq issue.to_reference(full: true)
+      expect(subject.target_reference).to eq issue.to_reference(full: false)
     end
   end