Address feedback

Signed-off-by: Alfredo Sumaran <alfredo@gitlab.com>

5 files changed, 78 insertions, 96 deletions

diff --git a/CHANGELOG b/CHANGELOG
index f18867c5379..57020ca9ca1 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -114,7 +114,6 @@ v 8.7.4
   - Fix always showing build notification message when switching between merge requests
   - Links for Redmine issue references are generated correctly again (Benedikt Huss)
   - Fix an issue when filtering merge requests with more than one label. !3886
-  - Fix links on wiki pages for relative url setups. !4026 (Artem Sidorenko)
 
 v 8.7.3
   - Emails, Gitlab::Email::Message, Gitlab::Diff, and Premailer::Adapter::Nokogiri are now instrumented
diff --git a/app/helpers/todos_helper.rb b/app/helpers/todos_helper.rb
index 150d826ac44..27f33b3eded 100644
--- a/app/helpers/todos_helper.rb
+++ b/app/helpers/todos_helper.rb
@@ -38,18 +38,15 @@ module TodosHelper
   end
 
   def todo_target_state_pill(todo)
-    klass = 'status-box '
-    klass << "status-box-#{todo.target.state.dasherize}"
-
-    content_tag(:span, nil, class: klass) do
-      todo.target.state.capitalize
+    if show_todo_state?(todo)
+      content_tag(:span, nil, class: 'target-status') do
+        content_tag(:span, nil, class: "status-box status-box-#{todo.target.state.dasherize}") do
+          todo.target.state.capitalize
+        end
+      end
     end
   end
 
-  def show_todo_state?(todo)
-    (todo.target.is_a?(MergeRequest) || todo.target.is_a?(Issue)) && ['closed', 'merged'].include?(todo.target.state)
-  end
-
   def todos_filter_params
     {
       state:      params[:state],
@@ -108,4 +105,10 @@ module TodosHelper
 
     options_from_collection_for_select(types, 'name', 'title', params[:type])
   end
+
+  private
+
+  def show_todo_state?(todo)
+    (todo.target.is_a?(MergeRequest) || todo.target.is_a?(Issue)) && ['closed', 'merged'].include?(todo.target.state)
+  end
 end
diff --git a/app/views/dashboard/todos/_todo.html.haml b/app/views/dashboard/todos/_todo.html.haml
index f544f87b581..db567d6df3b 100644
--- a/app/views/dashboard/todos/_todo.html.haml
+++ b/app/views/dashboard/todos/_todo.html.haml
@@ -4,8 +4,7 @@
     .todo-title.title
       - unless todo.build_failed?
         - if show_todo_state?(todo)
-          %span.target-status
-            = todo_target_state_pill(todo)
+          = todo_target_state_pill(todo)
 
         %span.author-name
           - if todo.author
diff --git a/spec/features/todos/target_state_spec.rb b/spec/features/todos/target_state_spec.rb
new file mode 100644
index 00000000000..604b3ee4056
--- /dev/null
+++ b/spec/features/todos/target_state_spec.rb
@@ -0,0 +1,65 @@
+require 'rails_helper'
+
+feature 'Todo target states', feature: true do
+  let(:user)          { create(:user) }
+  let(:author)        { create(:user) }
+  let(:project)       { create(:project) }
+
+  before do
+    login_as user
+  end
+
+  scenario 'on a closed issue todo has closed label' do
+    issue_closed = create(:issue, state: 'closed')
+    create_todo issue_closed
+    visit dashboard_todos_path
+
+    page.within '.todos-list' do
+      expect(page).to have_content('Closed')
+    end
+  end
+
+  scenario 'on an open issue todo does not have an open label' do
+    issue_open = create(:issue)
+    create_todo issue_open
+    visit dashboard_todos_path
+
+    page.within '.todos-list' do
+      expect(page).not_to have_content('Open')
+    end
+  end
+
+  scenario 'on a merged merge request todo has merged label' do
+    mr_merged = create(:merge_request, :simple, author: user, state: 'merged')
+    create_todo mr_merged
+    visit dashboard_todos_path
+
+    page.within '.todos-list' do
+      expect(page).to have_content('Merged')
+    end
+  end
+
+  scenario 'on a closed merge request todo has closed label' do
+    mr_closed = create(:merge_request, :simple, author: user, state: 'closed')
+    create_todo mr_closed
+    visit dashboard_todos_path
+
+    page.within '.todos-list' do
+      expect(page).to have_content('Closed')
+    end
+  end
+
+  scenario 'on an open merge request todo does not have an open label' do
+    mr_open = create(:merge_request, :simple, author: user)
+    create_todo mr_open
+    visit dashboard_todos_path
+
+    page.within '.todos-list' do
+      expect(page).not_to have_content('Open')
+    end
+  end
+
+  def create_todo(target)
+    create(:todo, :mentioned, user: user, project: project, target: target, author: author)
+  end
+end
diff --git a/spec/features/todos/target_state_todos_spec.rb b/spec/features/todos/target_state_todos_spec.rb
deleted file mode 100644
index c7c5cdb66b0..00000000000
--- a/spec/features/todos/target_state_todos_spec.rb
+++ /dev/null
@@ -1,84 +0,0 @@
-require 'rails_helper'
-
-describe 'Todos > Target State Labels' do
-  let(:user)          { create(:user) }
-  let(:author)        { create(:user) }
-  let(:project)       { create(:project) }
-  let(:issue_open)    { create(:issue) }
-  let(:issue_closed)  { create(:issue, state: 'closed') }
-  let(:mr_open)       { create(:merge_request, :simple, author: user) }
-  let(:mr_merged)     { create(:merge_request, :simple, author: user, state: 'merged') }
-  let(:mr_closed)     { create(:merge_request, :simple, author: user, state: 'closed') }
-
-  describe 'GET /dashboard/todos' do
-    context 'On a todo for a Closed Issue' do
-      before do
-        create(:todo, :mentioned, user: user, project: project, target: issue_closed, author: author)
-        login_as user
-        visit dashboard_todos_path
-      end
-
-      it 'has closed label' do
-        page.within '.todos-list' do
-          expect(page).to have_content('Closed')
-        end
-      end
-    end
-
-    context 'On a todo for a Open Issue' do
-      before do
-        create(:todo, :mentioned, user: user, project: project, target: issue_open, author: author)
-        login_as user
-        visit dashboard_todos_path
-      end
-
-      it 'does not have a open label' do
-        page.within '.todos-list' do
-          expect(page).not_to have_content('Open')
-        end
-      end
-    end
-
-    context 'On a todo for a merged Merge Request' do
-      before do
-        create(:todo, :mentioned, user: user, project: project, target: mr_merged, author: author)
-        login_as user
-        visit dashboard_todos_path
-      end
-
-      it 'has merged label' do
-        page.within '.todos-list' do
-          expect(page).to have_content('Merged')
-        end
-      end
-    end
-
-    context 'On a todo for a closed Merge Request' do
-      before do
-        create(:todo, :mentioned, user: user, project: project, target: mr_closed, author: author)
-        login_as user
-        visit dashboard_todos_path
-      end
-
-      it 'has closed label' do
-        page.within '.todos-list' do
-          expect(page).to have_content('Closed')
-        end
-      end
-    end
-
-    context 'On a todo for a open Merge Request' do
-      before do
-        create(:todo, :mentioned, user: user, project: project, target: mr_open, author: author)
-        login_as user
-        visit dashboard_todos_path
-      end
-
-      it 'does not have a open label' do
-        page.within '.todos-list' do
-          expect(page).not_to have_content('Open')
-        end
-      end
-    end
-  end
-end