Commit 7ef44a97 authored by Pavel Shutsin's avatar Pavel Shutsin Committed by Robert Speicher

Add resolution mechanism for todos

It will be used for internal research
parent c2a32118
......@@ -17,7 +17,9 @@ class Dashboard::TodosController < Dashboard::ApplicationController
end
def destroy
TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
todo = current_user.todos.find(params[:id])
TodoService.new.resolve_todo(todo, current_user, resolved_by_action: :mark_done)
respond_to do |format|
format.html do
......@@ -31,7 +33,7 @@ class Dashboard::TodosController < Dashboard::ApplicationController
end
def destroy_all
updated_ids = TodoService.new.mark_todos_as_done(@todos, current_user)
updated_ids = TodoService.new.resolve_todos(@todos, current_user, resolved_by_action: :mark_all_done)
respond_to do |format|
format.html { redirect_to dashboard_todos_path, status: :found, notice: _('Everything on your to-do list is marked as done.') }
......@@ -41,13 +43,13 @@ class Dashboard::TodosController < Dashboard::ApplicationController
end
def restore
TodoService.new.mark_todos_as_pending_by_ids(params[:id], current_user)
TodoService.new.restore_todo(current_user.todos.find(params[:id]), current_user)
render json: todos_counts
end
def bulk_restore
TodoService.new.mark_todos_as_pending_by_ids(params[:ids], current_user)
TodoService.new.restore_todos(current_user.todos.for_ids(params[:ids]), current_user)
render json: todos_counts
end
......
......@@ -28,7 +28,9 @@ module Mutations
def mark_all_todos_done
return [] unless current_user
TodoService.new.mark_all_todos_as_done_by_user(current_user)
todos = TodosFinder.new(current_user).execute
TodoService.new.resolve_todos(todos, current_user, resolved_by_action: :api_all_done)
end
end
end
......
......@@ -30,7 +30,7 @@ module Mutations
private
def mark_done(todo)
TodoService.new.mark_todo_as_done(todo, current_user)
TodoService.new.resolve_todo(todo, current_user, resolved_by_action: :api_done)
end
end
end
......
......@@ -18,7 +18,7 @@ module Mutations
def resolve(id:)
todo = authorized_find!(id: id)
restore(todo.id) if todo.done?
restore(todo)
{
todo: todo.reset,
......@@ -28,8 +28,8 @@ module Mutations
private
def restore(id)
TodoService.new.mark_todos_as_pending_by_ids([id], current_user)
def restore(todo)
TodoService.new.restore_todo(todo, current_user)
end
end
end
......
......@@ -68,7 +68,7 @@ module Mutations
end
def restore(todos)
TodoService.new.mark_todos_as_pending(todos, current_user)
TodoService.new.restore_todos(todos, current_user)
end
end
end
......
......@@ -66,6 +66,8 @@ class Todo < ApplicationRecord
scope :with_entity_associations, -> { preload(:target, :author, :note, group: :route, project: [:route, { namespace: :route }]) }
scope :joins_issue_and_assignees, -> { left_joins(issue: :assignees) }
enum resolved_by_action: { system_done: 0, api_all_done: 1, api_done: 2, mark_all_done: 3, mark_done: 4 }, _prefix: :resolved_by
state_machine :state, initial: :pending do
event :done do
transition [:pending] => :done
......@@ -100,17 +102,17 @@ class Todo < ApplicationRecord
state.nil? ? exists?(target: target) : exists?(target: target, state: state)
end
# Updates the state of a relation of todos to the new state.
# Updates attributes of a relation of todos to the new state.
#
# new_state - The new state of the todos.
# new_attributes - The new attributes of the todos.
#
# Returns an `Array` containing the IDs of the updated todos.
def update_state(new_state)
# Only update those that are not really on that state
base = where.not(state: new_state).except(:order)
def batch_update(**new_attributes)
# Only update those that have different state
base = where.not(state: new_attributes[:state]).except(:order)
ids = base.pluck(:id)
base.update_all(state: new_state, updated_at: Time.current)
base.update_all(new_attributes.merge(updated_at: Time.current))
ids
end
......
......@@ -1630,10 +1630,6 @@ class User < ApplicationRecord
super.presence || build_user_detail
end
def todos_limited_to(ids)
todos.where(id: ids)
end
def pending_todo_for(target)
todos.find_by(target: target, state: :pending)
end
......
......@@ -350,7 +350,7 @@ class IssuableBaseService < BaseService
todo_service.mark_todo(issuable, current_user)
when 'done'
todo = TodosFinder.new(current_user).find_by(target: issuable)
todo_service.mark_todos_as_done_by_ids(todo, current_user) if todo
todo_service.resolve_todo(todo, current_user) if todo
end
end
# rubocop: enable CodeReuse/ActiveRecord
......
......@@ -32,7 +32,7 @@ module Issues
old_assignees = old_associations.fetch(:assignees, [])
if has_changes?(issue, old_labels: old_labels, old_assignees: old_assignees)
todo_service.mark_pending_todos_as_done(issue, current_user)
todo_service.resolve_todos_for_target(issue, current_user)
end
if issue.previous_changes.include?('title') ||
......@@ -68,7 +68,7 @@ module Issues
end
def handle_task_changes(issuable)
todo_service.mark_pending_todos_as_done(issuable, current_user)
todo_service.resolve_todos_for_target(issuable, current_user)
todo_service.update_issue(issuable, current_user)
end
......
......@@ -27,7 +27,7 @@ module MergeRequests
old_assignees = old_associations.fetch(:assignees, [])
if has_changes?(merge_request, old_labels: old_labels, old_assignees: old_assignees)
todo_service.mark_pending_todos_as_done(merge_request, current_user)
todo_service.resolve_todos_for_target(merge_request, current_user)
end
if merge_request.previous_changes.include?('title') ||
......@@ -73,7 +73,7 @@ module MergeRequests
end
def handle_task_changes(merge_request)
todo_service.mark_pending_todos_as_done(merge_request, current_user)
todo_service.resolve_todos_for_target(merge_request, current_user)
todo_service.update_merge_request(merge_request, current_user)
end
......
......@@ -30,7 +30,7 @@ class TodoService
# * mark all pending todos related to the target for the current user as done
#
def close_issue(issue, current_user)
mark_pending_todos_as_done(issue, current_user)
resolve_todos_for_target(issue, current_user)
end
# When we destroy a todo target we should:
......@@ -79,7 +79,7 @@ class TodoService
# * mark all pending todos related to the target for the current user as done
#
def close_merge_request(merge_request, current_user)
mark_pending_todos_as_done(merge_request, current_user)
resolve_todos_for_target(merge_request, current_user)
end
# When merge a merge request we should:
......@@ -87,7 +87,7 @@ class TodoService
# * mark all pending todos related to the target for the current user as done
#
def merge_merge_request(merge_request, current_user)
mark_pending_todos_as_done(merge_request, current_user)
resolve_todos_for_target(merge_request, current_user)
end
# When a build fails on the HEAD of a merge request we should:
......@@ -105,7 +105,7 @@ class TodoService
# * mark all pending todos related to the merge request for that user as done
#
def merge_request_push(merge_request, current_user)
mark_pending_todos_as_done(merge_request, current_user)
resolve_todos_for_target(merge_request, current_user)
end
# When a build is retried to a merge request we should:
......@@ -114,7 +114,7 @@ class TodoService
#
def merge_request_build_retried(merge_request)
merge_request.merge_participants.each do |user|
mark_pending_todos_as_done(merge_request, user)
resolve_todos_for_target(merge_request, user)
end
end
......@@ -151,76 +151,60 @@ class TodoService
# * mark all pending todos related to the awardable for the current user as done
#
def new_award_emoji(awardable, current_user)
mark_pending_todos_as_done(awardable, current_user)
resolve_todos_for_target(awardable, current_user)
end
# When marking pending todos as done we should:
#
# * mark all pending todos related to the target for the current user as done
#
def mark_pending_todos_as_done(target, user)
attributes = attributes_for_target(target)
pending_todos(user, attributes).update_all(state: :done)
user.update_todos_count_cache
# When user marks an issue as todo
def mark_todo(issuable, current_user)
attributes = attributes_for_todo(issuable.project, issuable, current_user, Todo::MARKED)
create_todos(current_user, attributes)
end
# When user marks some todos as done
def mark_todos_as_done(todos, current_user)
update_todos_state(todos, current_user, :done)
def todo_exist?(issuable, current_user)
TodosFinder.new(current_user).any_for_target?(issuable, :pending)
end
def mark_todos_as_done_by_ids(ids, current_user)
todos = todos_by_ids(ids, current_user)
mark_todos_as_done(todos, current_user)
end
# Resolves all todos related to target
def resolve_todos_for_target(target, current_user)
attributes = attributes_for_target(target)
def mark_all_todos_as_done_by_user(current_user)
todos = TodosFinder.new(current_user).execute
mark_todos_as_done(todos, current_user)
resolve_todos(pending_todos(current_user, attributes), current_user)
end
def mark_todo_as_done(todo, current_user)
return if todo.done?
todo.update(state: :done)
def resolve_todos(todos, current_user, resolution: :done, resolved_by_action: :system_done)
todos_ids = todos.batch_update(state: resolution, resolved_by_action: resolved_by_action)
current_user.update_todos_count_cache
end
# When user marks some todos as pending
def mark_todos_as_pending(todos, current_user)
update_todos_state(todos, current_user, :pending)
todos_ids
end
def mark_todos_as_pending_by_ids(ids, current_user)
todos = todos_by_ids(ids, current_user)
mark_todos_as_pending(todos, current_user)
end
def resolve_todo(todo, current_user, resolution: :done, resolved_by_action: :system_done)
return if todo.done?
# When user marks an issue as todo
def mark_todo(issuable, current_user)
attributes = attributes_for_todo(issuable.project, issuable, current_user, Todo::MARKED)
create_todos(current_user, attributes)
end
todo.update(state: resolution, resolved_by_action: resolved_by_action)
def todo_exist?(issuable, current_user)
TodosFinder.new(current_user).any_for_target?(issuable, :pending)
current_user.update_todos_count_cache
end
private
def restore_todos(todos, current_user)
todos_ids = todos.batch_update(state: :pending)
def todos_by_ids(ids, current_user)
current_user.todos_limited_to(Array(ids))
current_user.update_todos_count_cache
todos_ids
end
def update_todos_state(todos, current_user, state)
todos_ids = todos.update_state(state)
def restore_todo(todo, current_user)
return if todo.pending?
current_user.update_todos_count_cache
todo.update(state: :pending)
todos_ids
current_user.update_todos_count_cache
end
private
def create_todos(users, attributes)
Array(users).map do |user|
next if pending_todos(user, attributes).exists?
......@@ -252,9 +236,9 @@ class TodoService
return unless note.can_create_todo?
project = note.project
target = note.noteable
target = note.noteable
mark_pending_todos_as_done(target, author)
resolve_todos_for_target(target, author)
create_mention_todos(project, target, author, note, skip_users)
end
......
---
title: Store Todo resolution method
merge_request: 32753
author:
type: added
# frozen_string_literal: true
class AddTodoResolvedByAction < ActiveRecord::Migration[6.0]
include Gitlab::Database::MigrationHelpers
DOWNTIME = false
def up
with_lock_retries do
add_column :todos, :resolved_by_action, :integer, limit: 2
end
end
def down
with_lock_retries do
remove_column :todos, :resolved_by_action
end
end
end
......@@ -6522,7 +6522,8 @@ CREATE TABLE public.todos (
updated_at timestamp without time zone,
note_id integer,
commit_id character varying,
group_id integer
group_id integer,
resolved_by_action smallint
);
CREATE SEQUENCE public.todos_id_seq
......@@ -13959,6 +13960,7 @@ COPY "schema_migrations" (version) FROM STDIN;
20200519115908
20200519171058
20200519194042
20200520103514
20200525114553
20200525121014
20200526120714
......
......@@ -31,14 +31,14 @@ module Epics
old_labels = old_associations.fetch(:labels, [])
if has_changes?(epic, old_labels: old_labels)
todo_service.mark_pending_todos_as_done(epic, current_user)
todo_service.resolve_todos_for_target(epic, current_user)
end
todo_service.update_epic(epic, current_user, old_mentioned_users)
end
def handle_task_changes(epic)
todo_service.mark_pending_todos_as_done(epic, current_user)
todo_service.resolve_todos_for_target(epic, current_user)
todo_service.update_epic(epic, current_user)
end
......
......@@ -44,7 +44,7 @@ module MergeRequests
end
def mark_pending_todos_as_done(merge_request)
todo_service.mark_pending_todos_as_done(merge_request, current_user)
todo_service.resolve_todos_for_target(merge_request, current_user)
end
def calculate_approvals_metrics(merge_request)
......
......@@ -91,16 +91,18 @@ module API
requires :id, type: Integer, desc: 'The ID of the todo being marked as done'
end
post ':id/mark_as_done' do
TodoService.new.mark_todos_as_done_by_ids(params[:id], current_user)
todo = current_user.todos.find(params[:id])
TodoService.new.resolve_todo(todo, current_user, resolved_by_action: :api_done)
present todo, with: Entities::Todo, current_user: current_user
end
desc 'Mark all todos as done'
post '/mark_as_done' do
todos = find_todos
TodoService.new.mark_todos_as_done(todos, current_user)
TodoService.new.resolve_todos(todos, current_user, resolved_by_action: :api_all_done)
no_content!
end
......
......@@ -114,7 +114,7 @@ describe 'Dashboard Todos' do
context 'todo is stale on the page' do
before do
todos = TodosFinder.new(user, state: :pending).execute
TodoService.new.mark_todos_as_done(todos, user)
TodoService.new.resolve_todos(todos, user)
end
it_behaves_like 'deleting the todo'
......
......@@ -393,10 +393,10 @@ describe Todo do
end
end
describe '.update_state' do
describe '.batch_update' do
it 'updates the state of todos' do
todo = create(:todo, :pending)
ids = described_class.update_state(:done)
ids = described_class.batch_update(state: :done)
todo.reload
......@@ -407,7 +407,7 @@ describe Todo do
it 'does not update todos that already have the given state' do
create(:todo, :pending)
expect(described_class.update_state(:pending)).to be_empty
expect(described_class.batch_update(state: :pending)).to be_empty
end
it 'updates updated_at' do
......@@ -416,7 +416,7 @@ describe Todo do
Timecop.freeze(1.day.from_now) do
expected_update_date = Time.current.utc
ids = described_class.update_state(:done)
ids = described_class.batch_update(state: :done)
expect(Todo.where(id: ids).map(&:updated_at)).to all(be_like_time(expected_update_date))
end
......
This diff is collapsed.
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