From 4a98ee5669cd7f8ca8269cdbad090ef1ba82d1cf Mon Sep 17 00:00:00 2001 From: Reinier Balt Date: Fri, 28 Nov 2008 16:39:50 +0100 Subject: [PATCH 1/2] several fixes to recurring todos and adds some named_scopes fixes case where unmarking a complete todo which belongs to a recurring pattern results in two todos that both keep on recurring. With this fix a new todo will only be created when there are no active todos left belonging to that recurring pattern fixes tests that failed because of previous commits adds some named_scopes, inspired by http://railscasts.com/episodes/108 --- app/controllers/recurring_todos_controller.rb | 21 ++++++++--- app/controllers/todos_controller.rb | 37 +++++++++++-------- app/models/recurring_todo.rb | 5 +++ app/models/todo.rb | 5 ++- app/views/todos/toggle_check.js.rjs | 4 +- test/fixtures/todos.yml | 2 +- test/functional/projects_controller_test.rb | 6 +-- test/functional/todos_controller_test.rb | 6 +-- 8 files changed, 55 insertions(+), 31 deletions(-) diff --git a/app/controllers/recurring_todos_controller.rb b/app/controllers/recurring_todos_controller.rb index 60fb5a00..9928a4d5 100644 --- a/app/controllers/recurring_todos_controller.rb +++ b/app/controllers/recurring_todos_controller.rb @@ -6,8 +6,10 @@ class RecurringTodosController < ApplicationController append_before_filter :get_recurring_todo_from_param, :only => [:destroy, :toggle_check, :toggle_star, :edit, :update] def index - @recurring_todos = current_user.recurring_todos.find(:all, :conditions => ["state = ?", "active"]) - @completed_recurring_todos = current_user.recurring_todos.find(:all, :conditions => ["state = ?", "completed"]) + find_and_inactivate + + @recurring_todos = current_user.recurring_todos.active + @completed_recurring_todos = current_user.recurring_todos.completed @no_recurring_todos = @recurring_todos.size == 0 @no_completed_recurring_todos = @completed_recurring_todos.size == 0 @count = @recurring_todos.size @@ -129,7 +131,7 @@ class RecurringTodosController < ApplicationController def destroy # remove all references to this recurring todo - @todos = current_user.todos.find(:all, {:conditions => ["recurring_todo_id = ?", params[:id]]}) + @todos = @recurring_todo.todos @number_of_todos = @todos.size @todos.each do |t| t.recurring_todo_id = nil @@ -161,14 +163,15 @@ class RecurringTodosController < ApplicationController def toggle_check @saved = @recurring_todo.toggle_completion! - @count = current_user.recurring_todos.count(:all, :conditions => ["state = ?", "active"]) + @count = current_user.recurring_todos.active.count @remaining = @count if @recurring_todo.active? - @remaining = current_user.recurring_todos.count(:all, :conditions => ["state = ?", 'completed']) + @remaining = current_user.recurring_todos.completed.count # from completed back to active -> check if there is an active todo - @active_todos = current_user.todos.count(:all, {:conditions => ["state = ? AND recurring_todo_id = ?", 'active',params[:id]]}) + # current_user.todos.count(:all, {:conditions => ["state = ? AND recurring_todo_id = ?", 'active',params[:id]]}) + @active_todos = @recurring_todo.todos.active.count # create todo if there is no active todo belonging to the activated # recurring_todo @new_recurring_todo = create_todo_from_recurring_todo(@recurring_todo) if @active_todos == 0 @@ -255,5 +258,11 @@ class RecurringTodosController < ApplicationController def get_recurring_todo_from_param @recurring_todo = current_user.recurring_todos.find(params[:id]) end + + def find_and_inactivate + # find active recurring todos without active todos and inactivate them + recurring_todos = current_user.recurring_todos.active + recurring_todos.each { |rt| rt.toggle_completion! if rt.todos.not_completed.count == 0} + end end diff --git a/app/controllers/todos_controller.rb b/app/controllers/todos_controller.rb index aead74a1..15bbcca4 100644 --- a/app/controllers/todos_controller.rb +++ b/app/controllers/todos_controller.rb @@ -127,7 +127,7 @@ class TodosController < ApplicationController end # Toggles the 'done' status of the action - # + # def toggle_check @source_view = params['_source_view'] || 'todo' @original_item_due = @todo.due @@ -777,26 +777,31 @@ class TodosController < ApplicationController new_recurring_todo = nil recurring_todo = nil if todo.from_recurring_todo? - recurring_todo = current_user.recurring_todos.find(todo.recurring_todo_id) + recurring_todo = todo.recurring_todo + + # check if there are active todos belonging to this recurring todo. + # only add new one if all active todos are completed + if recurring_todo.todos.active.count == 0 - # check for next todo either from the due date or the show_from date - date_to_check = todo.due.nil? ? todo.show_from : todo.due + # check for next todo either from the due date or the show_from date + date_to_check = todo.due.nil? ? todo.show_from : todo.due - # if both due and show_from are nil, check for a next todo from now - date_to_check = Time.zone.now if date_to_check.nil? + # if both due and show_from are nil, check for a next todo from now + date_to_check = Time.zone.now if date_to_check.nil? - if recurring_todo.active? && recurring_todo.has_next_todo(date_to_check) + if recurring_todo.active? && recurring_todo.has_next_todo(date_to_check) - # shift the reference date to yesterday if date_to_check is furher in - # the past. This is to make sure we do not get older todos for overdue - # todos. I.e. checking a daily todo that is overdue with 5 days will - # create a new todo which is overdue by 4 days if we don't shift the - # date. Discard the time part in the compare. We pick yesterday so that - # new todos due for today will be created instead of new todos for - # tomorrow. - date = date_to_check.at_midnight >= Time.zone.now.at_midnight ? date_to_check : Time.zone.now-1.day + # shift the reference date to yesterday if date_to_check is furher in + # the past. This is to make sure we do not get older todos for overdue + # todos. I.e. checking a daily todo that is overdue with 5 days will + # create a new todo which is overdue by 4 days if we don't shift the + # date. Discard the time part in the compare. We pick yesterday so + # that new todos due for today will be created instead of new todos + # for tomorrow. + date = date_to_check.at_midnight >= Time.zone.now.at_midnight ? date_to_check : Time.zone.now-1.day - new_recurring_todo = create_todo_from_recurring_todo(recurring_todo, date) + new_recurring_todo = create_todo_from_recurring_todo(recurring_todo, date) + end end end return new_recurring_todo diff --git a/app/models/recurring_todo.rb b/app/models/recurring_todo.rb index 4fb4a49c..4561ed58 100644 --- a/app/models/recurring_todo.rb +++ b/app/models/recurring_todo.rb @@ -4,6 +4,8 @@ class RecurringTodo < ActiveRecord::Base belongs_to :project belongs_to :user + has_many :todos + attr_protected :user acts_as_state_machine :initial => :active, :column => 'state' @@ -20,6 +22,9 @@ class RecurringTodo < ActiveRecord::Base validates_presence_of :context + named_scope :active, :conditions => { :state => 'active'} + named_scope :completed, :conditions => { :state => 'completed'} + event :complete do transitions :to => :completed, :from => [:active] end diff --git a/app/models/todo.rb b/app/models/todo.rb index 972b032f..f432037d 100644 --- a/app/models/todo.rb +++ b/app/models/todo.rb @@ -4,7 +4,10 @@ class Todo < ActiveRecord::Base belongs_to :project belongs_to :user belongs_to :recurring_todo - + + named_scope :active, :conditions => { :state => 'active' } + named_scope :not_completed, :conditions => ['NOT state = ? ', 'completed'] + STARRED_TAG_NAME = "starred" acts_as_state_machine :initial => :active, :column => 'state' diff --git a/app/views/todos/toggle_check.js.rjs b/app/views/todos/toggle_check.js.rjs index d862862c..ee9c26e1 100644 --- a/app/views/todos/toggle_check.js.rjs +++ b/app/views/todos/toggle_check.js.rjs @@ -24,7 +24,9 @@ if @saved page.insert_html :bottom, item_container_id(@new_recurring_todo), :partial => 'todos/todo', :locals => { :todo => @new_recurring_todo, :parent_container_type => parent_container_type } page.visual_effect :highlight, dom_id(@new_recurring_todo, 'line'), {'startcolor' => "'#99ff99'"} else - page.notify :notice, "There is no next action after the recurring action you just finished. The recurrence is completed", 6.0 if @new_recurring_todo.nil? + if @todo.recurring_todo.todos.active.count == 0 + page.notify :notice, "There is no next action after the recurring action you just finished. The recurrence is completed", 6.0 if @new_recurring_todo.nil? + end end end diff --git a/test/fixtures/todos.yml b/test/fixtures/todos.yml index f4941c0c..4dcf984e 100644 --- a/test/fixtures/todos.yml +++ b/test/fixtures/todos.yml @@ -95,7 +95,7 @@ end completed_at: ~ user_id: 1 -7: +book: id: 7 context_id: 6 project_id: 3 diff --git a/test/functional/projects_controller_test.rb b/test/functional/projects_controller_test.rb index ee3df3e4..33e6ca95 100644 --- a/test/functional/projects_controller_test.rb +++ b/test/functional/projects_controller_test.rb @@ -223,9 +223,9 @@ class ProjectsControllerTest < TodoContainerControllerTestBase login_as :admin_user u = users(:admin_user) post :actionize, :state => "active", :format => 'js' - assert_equal 1, projects(:gardenclean).position - assert_equal 2, projects(:timemachine).position - assert_equal 3, projects(:moremoney).position + assert_equal 1, projects(:gardenclean).position + assert_equal 2, projects(:moremoney).position + assert_equal 3, projects(:timemachine).position end def test_alphabetize_sorts_active_projects_alphabetically diff --git a/test/functional/todos_controller_test.rb b/test/functional/todos_controller_test.rb index 9804b0b5..4888a7c4 100644 --- a/test/functional/todos_controller_test.rb +++ b/test/functional/todos_controller_test.rb @@ -180,7 +180,7 @@ class TodosControllerTest < Test::Rails::TestCase login_as(:admin_user) get :index, { :format => "rss" } assert_equal 'application/rss+xml', @response.content_type - # #puts @response.body + # puts @response.body assert_xml_select 'rss[version="2.0"]' do assert_select 'channel' do @@ -193,7 +193,7 @@ class TodosControllerTest < Test::Rails::TestCase assert_select 'description', /.*/ assert_select 'link', %r{http://test.host/contexts/.+} assert_select 'guid', %r{http://test.host/todos/.+} - assert_select 'pubDate', projects(:timemachine).updated_at.to_s(:rfc822) + assert_select 'pubDate', todos(:book).updated_at.to_s(:rfc822) end end end @@ -245,7 +245,7 @@ class TodosControllerTest < Test::Rails::TestCase assert_xml_select 'entry', 11 do assert_xml_select 'title', /.+/ assert_xml_select 'content[type="html"]', /.*/ - assert_xml_select 'published', /(#{Regexp.escape(projects(:timemachine).updated_at.xmlschema)}|#{Regexp.escape(projects(:moremoney).updated_at.xmlschema)})/ + assert_xml_select 'published', /(#{Regexp.escape(todos(:book).updated_at.xmlschema)}|#{Regexp.escape(projects(:moremoney).updated_at.xmlschema)})/ end end end From 4a78b9f97a02b0247de6c2c05e4b0ce06631f7fc Mon Sep 17 00:00:00 2001 From: Reinier Balt Date: Fri, 28 Nov 2008 16:52:54 +0100 Subject: [PATCH 2/2] fix case where deleting a completed todo which belongs to a recurring todo resulted in multiple todos belonging to the recurring pattern or no todo at all --- app/controllers/todos_controller.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/todos_controller.rb b/app/controllers/todos_controller.rb index 15bbcca4..ad3a762a 100644 --- a/app/controllers/todos_controller.rb +++ b/app/controllers/todos_controller.rb @@ -277,11 +277,11 @@ class TodosController < ApplicationController @original_item_due = @todo.due @context_id = @todo.context_id @project_id = @todo.project_id + + @saved = @todo.destroy # check if this todo has a related recurring_todo. If so, create next todo - @new_recurring_todo = check_for_next_todo(@todo) - - @saved = @todo.destroy + @new_recurring_todo = check_for_next_todo(@todo) if @saved respond_to do |format|