Commit 6fdf383e authored by Robert Speicher's avatar Robert Speicher Committed by Robert Speicher

Merge branch 'fix-todos-counters' into 'master'

Ensure Todos counters doesn't count Todos for projects pending delete

Use `TodosFinder` instead of `current_user.todos` to filter projects
pending delete on Todos counters helpers.

Counters should not reflect the number of Todos displayed on the tabs.

Fixes #18633

See merge request !4663
parent 4a7ba511
...@@ -134,6 +134,7 @@ v 8.8.5 (unreleased) ...@@ -134,6 +134,7 @@ v 8.8.5 (unreleased)
- Use Git cached counters for branches and tags on project page - Use Git cached counters for branches and tags on project page
- Filter parameters for request_uri value on instrumented transactions. - Filter parameters for request_uri value on instrumented transactions.
- Cache user todo counts from TodoService - Cache user todo counts from TodoService
- Ensure Todos counters doesn't count Todos for projects pending delete
v 8.8.5 v 8.8.5
- Import GitHub repositories respecting the API rate limit !4166 - Import GitHub repositories respecting the API rate limit !4166
......
class Dashboard::TodosController < Dashboard::ApplicationController class Dashboard::TodosController < Dashboard::ApplicationController
before_action :find_todos, only: [:index, :destroy, :destroy_all] include TodosHelper
before_action :find_todos, only: [:index, :destroy_all]
def index def index
@todos = @todos.page(params[:page]) @todos = @todos.page(params[:page])
...@@ -8,14 +10,10 @@ class Dashboard::TodosController < Dashboard::ApplicationController ...@@ -8,14 +10,10 @@ class Dashboard::TodosController < Dashboard::ApplicationController
def destroy def destroy
TodoService.new.mark_todos_as_done([todo], current_user) TodoService.new.mark_todos_as_done([todo], current_user)
todo_notice = 'Todo was successfully marked as done.'
respond_to do |format| respond_to do |format|
format.html { redirect_to dashboard_todos_path, notice: todo_notice } format.html { redirect_to dashboard_todos_path, notice: 'Todo was successfully marked as done.' }
format.js { head :ok } format.js { head :ok }
format.json do format.json { render json: { count: todos_pending_count, done_count: todos_done_count } }
render json: { count: @todos.size, done_count: current_user.todos_done_count }
end
end end
end end
...@@ -25,20 +23,17 @@ class Dashboard::TodosController < Dashboard::ApplicationController ...@@ -25,20 +23,17 @@ class Dashboard::TodosController < Dashboard::ApplicationController
respond_to do |format| respond_to do |format|
format.html { redirect_to dashboard_todos_path, notice: 'All todos were marked as done.' } format.html { redirect_to dashboard_todos_path, notice: 'All todos were marked as done.' }
format.js { head :ok } format.js { head :ok }
format.json do format.json { render json: { count: todos_pending_count, done_count: todos_done_count } }
find_todos
render json: { count: @todos.size, done_count: current_user.todos_done_count }
end
end end
end end
private private
def todo def todo
@todo ||= current_user.todos.find(params[:id]) @todo ||= find_todos.find(params[:id])
end end
def find_todos def find_todos
@todos = TodosFinder.new(current_user, params).execute @todos ||= TodosFinder.new(current_user, params).execute
end end
end end
...@@ -123,7 +123,7 @@ class TodosFinder ...@@ -123,7 +123,7 @@ class TodosFinder
end end
def by_state(items) def by_state(items)
case params[:state] case params[:state].to_s
when 'done' when 'done'
items.done items.done
else else
......
module TodosHelper module TodosHelper
def todos_pending_count def todos_pending_count
current_user.todos_pending_count TodosFinder.new(current_user, state: :pending).execute.count
end end
def todos_done_count def todos_done_count
current_user.todos_done_count TodosFinder.new(current_user, state: :done).execute.count
end end
def todo_action_name(todo) def todo_action_name(todo)
......
...@@ -14,7 +14,12 @@ Feature: Dashboard Todos ...@@ -14,7 +14,12 @@ Feature: Dashboard Todos
Scenario: I mark todos as done Scenario: I mark todos as done
Then I should see todos assigned to me Then I should see todos assigned to me
And I mark the todo as done And I mark the todo as done
And I click on the "Done" tab Then I should see the todo marked as done
@javascript
Scenario: I mark all todos as done
Then I should see todos assigned to me
And I mark all todos as done
Then I should see all todos marked as done Then I should see all todos marked as done
@javascript @javascript
......
...@@ -26,14 +26,15 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps ...@@ -26,14 +26,15 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
end end
step 'I should see todos assigned to me' do step 'I should see todos assigned to me' do
page.within('.todos-pending-count') { expect(page).to have_content '4' }
expect(page).to have_content 'To do 4' expect(page).to have_content 'To do 4'
expect(page).to have_content 'Done 0' expect(page).to have_content 'Done 0'
expect(page).to have_link project.name_with_namespace expect(page).to have_link project.name_with_namespace
should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference}", merge_request.title) should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference}", merge_request.title)
should_see_todo(2, "John Doe mentioned you on issue ##{issue.iid}", "#{current_user.to_reference} Wdyt?") should_see_todo(2, "John Doe mentioned you on issue #{issue.to_reference}", "#{current_user.to_reference} Wdyt?")
should_see_todo(3, "John Doe assigned you issue ##{issue.iid}", issue.title) should_see_todo(3, "John Doe assigned you issue #{issue.to_reference}", issue.title)
should_see_todo(4, "Mary Jane mentioned you on issue ##{issue.iid}", issue.title) should_see_todo(4, "Mary Jane mentioned you on issue #{issue.to_reference}", issue.title)
end end
step 'I mark the todo as done' do step 'I mark the todo as done' do
...@@ -41,18 +42,40 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps ...@@ -41,18 +42,40 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
click_link 'Done' click_link 'Done'
end end
page.within('.todos-pending-count') { expect(page).to have_content '3' }
expect(page).to have_content 'To do 3' expect(page).to have_content 'To do 3'
expect(page).to have_content 'Done 1' expect(page).to have_content 'Done 1'
should_not_see_todo "John Doe assigned you merge request #{merge_request.to_reference}" should_not_see_todo "John Doe assigned you merge request #{merge_request.to_reference}"
end end
step 'I click on the "Done" tab' do step 'I mark all todos as done' do
click_link 'Mark all as done'
page.within('.todos-pending-count') { expect(page).to have_content '0' }
expect(page).to have_content 'To do 0'
expect(page).to have_content 'Done 4'
expect(page).not_to have_link project.name_with_namespace
should_not_see_todo "John Doe assigned you merge request #{merge_request.to_reference}"
should_not_see_todo "John Doe mentioned you on issue #{issue.to_reference}"
should_not_see_todo "John Doe assigned you issue #{issue.to_reference}"
should_not_see_todo "Mary Jane mentioned you on issue #{issue.to_reference}"
end
step 'I should see the todo marked as done' do
click_link 'Done 1' click_link 'Done 1'
expect(page).to have_link project.name_with_namespace
should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference}", merge_request.title, false)
end end
step 'I should see all todos marked as done' do step 'I should see all todos marked as done' do
click_link 'Done 4'
expect(page).to have_link project.name_with_namespace expect(page).to have_link project.name_with_namespace
should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference}", merge_request.title, false) should_see_todo(1, "John Doe assigned you merge request #{merge_request.to_reference}", merge_request.title, false)
should_see_todo(2, "John Doe mentioned you on issue #{issue.to_reference}", "#{current_user.to_reference} Wdyt?", false)
should_see_todo(3, "John Doe assigned you issue #{issue.to_reference}", issue.title, false)
should_see_todo(4, "Mary Jane mentioned you on issue #{issue.to_reference}", issue.title, false)
end end
step 'I filter by "Enterprise"' do step 'I filter by "Enterprise"' do
...@@ -76,7 +99,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps ...@@ -76,7 +99,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
end end
step 'I should not see todos related to "Mary Jane" in the list' do step 'I should not see todos related to "Mary Jane" in the list' do
should_not_see_todo "Mary Jane mentioned you on issue ##{issue.iid}" should_not_see_todo "Mary Jane mentioned you on issue #{issue.to_reference}"
end end
step 'I should not see todos related to "Merge Requests" in the list' do step 'I should not see todos related to "Merge Requests" in the list' do
...@@ -85,7 +108,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps ...@@ -85,7 +108,7 @@ class Spinach::Features::DashboardTodos < Spinach::FeatureSteps
step 'I should not see todos related to "Assignments" in the list' do step 'I should not see todos related to "Assignments" in the list' do
should_not_see_todo "John Doe assigned you merge request #{merge_request.to_reference}" should_not_see_todo "John Doe assigned you merge request #{merge_request.to_reference}"
should_not_see_todo "John Doe assigned you issue ##{issue.iid}" should_not_see_todo "John Doe assigned you issue #{issue.to_reference}"
end end
step 'I click on the todo' do step 'I click on the todo' do
......
...@@ -103,11 +103,15 @@ describe 'Dashboard Todos', feature: true do ...@@ -103,11 +103,15 @@ describe 'Dashboard Todos', feature: true do
before do before do
deleted_project = create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC, pending_delete: true) deleted_project = create(:project, visibility_level: Gitlab::VisibilityLevel::PUBLIC, pending_delete: true)
create(:todo, :mentioned, user: user, project: deleted_project, target: issue, author: author) create(:todo, :mentioned, user: user, project: deleted_project, target: issue, author: author)
create(:todo, :mentioned, user: user, project: deleted_project, target: issue, author: author, state: :done)
login_as(user) login_as(user)
visit dashboard_todos_path visit dashboard_todos_path
end end
it 'shows "All done" message' do it 'shows "All done" message' do
within('.todos-pending-count') { expect(page).to have_content '0' }
expect(page).to have_content 'To do 0'
expect(page).to have_content 'Done 0'
expect(page).to have_content "You're all done!" expect(page).to have_content "You're all done!"
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment