From c2c67f1640304df652adafa21212e95f6fb1fb8f Mon Sep 17 00:00:00 2001 From: Reinier Balt Date: Mon, 3 Feb 2014 10:48:21 +0100 Subject: [PATCH] use new model to handle updating of recurring todos --- app/controllers/recurring_todos_controller.rb | 104 +++++++----------- .../abstract_recurring_todos_builder.rb | 27 ++++- .../abstract_repeat_pattern.rb | 9 +- .../daily_recurring_todos_builder.rb | 7 +- .../monthly_recurring_todos_builder.rb | 10 +- .../recurring_todos_builder.rb | 6 +- .../weekly_recurring_todos_builder.rb | 11 +- .../yearly_recurring_todos_builder.rb | 14 +-- .../recurring_todos_controller_test.rb | 26 +++++ .../abstract_recurring_todos_builder_test.rb | 10 +- .../recurring_todos_builder_test.rb | 33 +++++- .../weekly_recurring_todos_builder_test.rb | 13 +++ 12 files changed, 166 insertions(+), 104 deletions(-) diff --git a/app/controllers/recurring_todos_controller.rb b/app/controllers/recurring_todos_controller.rb index 50b6b8bb..9190d893 100644 --- a/app/controllers/recurring_todos_controller.rb +++ b/app/controllers/recurring_todos_controller.rb @@ -3,7 +3,7 @@ class RecurringTodosController < ApplicationController helper :todos, :recurring_todos append_before_filter :init, :only => [:index, :new, :edit, :create] - append_before_filter :get_recurring_todo_from_param, :only => [:destroy, :toggle_check, :toggle_star, :edit, :update] + append_before_filter :get_recurring_todo_from_param, :only => [:destroy, :toggle_check, :toggle_star, :edit] def index @page_title = t('todos.recurring_actions_title') @@ -43,62 +43,15 @@ class RecurringTodosController < ApplicationController end def update - # TODO: write tests for updating - @recurring_todo.tag_with(params[:edit_recurring_todo_tag_list]) if params[:edit_recurring_todo_tag_list] + @recurring_todo = current_user.recurring_todos.find(params[:id]) + @original_item_context_id = @recurring_todo.context_id @original_item_project_id = @recurring_todo.project_id - # we needed to rename the recurring_period selector in the edit form because - # the form for a new recurring todo and the edit form are on the same page. - # Same goes for start_from and end_date - params['recurring_todo']['recurring_period']=params['recurring_edit_todo']['recurring_period'] - params['recurring_todo']['end_date']=parse_date_per_user_prefs(params['recurring_todo_edit_end_date']) - params['recurring_todo']['start_from']=parse_date_per_user_prefs(params['recurring_todo_edit_start_from']) + updater = RecurringTodos::RecurringTodosBuilder.new(current_user, edit_recurring_todo_params) - # update project - if params['recurring_todo']['project_id'].blank? && !params['project_name'].nil? - if params['project_name'] == 'None' - project = Project.null_object - else - project = current_user.projects.where(:name => params['project_name'].strip) - unless project - project = current_user.projects.build - project.name = params['project_name'].strip - project.save - @new_project_created = true - end - end - params["recurring_todo"]["project_id"] = project.id - end - - # update context - if params['recurring_todo']['context_id'].blank? && params['context_name'].present? - context = current_user.contexts.where(:name => params['context_name'].strip).first - unless context - context = current_user.contexts.build - context.name = params['context_name'].strip - context.save - @new_context_created = true - end - params["recurring_todo"]["context_id"] = context.id - end - - # make sure that we set weekly_return_xxx to empty (space) when they are - # not checked (and thus not present in params["recurring_todo"]) - %w{monday tuesday wednesday thursday friday saturday sunday}.each do |day| - params["recurring_todo"]["weekly_return_"+day]=' ' if params["recurring_todo"]["weekly_return_"+day].nil? - end - - selector_attributes = { - 'recurring_period' => recurring_todo_params['recurring_period'], - 'daily_selector' => recurring_todo_params['daily_selector'], - 'monthly_selector' => recurring_todo_params['monthly_selector'], - 'yearly_selector' => recurring_todo_params['yearly_selector'] - } - - @recurring_todo.assign_attributes(:recurring_period => recurring_todo_params[:recurring_period]) - @recurring_todo.assign_attributes(selector_attributes) - @saved = @recurring_todo.update_attributes recurring_todo_params + @saved = updater.update(@recurring_todo) + @recurring_todo.reload respond_to do |format| format.js @@ -108,16 +61,15 @@ class RecurringTodosController < ApplicationController def create builder = RecurringTodos::RecurringTodosBuilder.new(current_user, all_recurring_todo_params) @saved = builder.save - @recurring_todo = builder.saved_recurring_todo if @saved - @status_message = t('todos.recurring_action_saved') + @recurring_todo = builder.saved_recurring_todo @todo_saved = TodoFromRecurringTodo.new(current_user, @recurring_todo).create.nil? == false - if @todo_saved - @status_message += " / " + t('todos.new_related_todo_created_short') - else - @status_message += " / " + t('todos.new_related_todo_not_created_short') - end + + @status_message = + t('todos.recurring_action_saved') + " / " + + t("todos.new_related_todo_#{@todo_saved ? "" : "not_"}created_short") + @down_count = current_user.recurring_todos.active.count @new_recurring_todo = RecurringTodo.new else @@ -219,12 +171,38 @@ class RecurringTodosController < ApplicationController def all_recurring_todo_params # move context_name, project_name and tag_list into :recurring_todo hash for easier processing - params[:recurring_todo][:context_name] = params[:context_name] unless params[:context_name].blank? - params[:recurring_todo][:project_name] = params[:project_name] unless params[:project_name].blank? - params[:recurring_todo][:tag_list] = params[:tag_list] unless params[:tag_list].blank? + { context_name: :context_name, project_name: :project_name, tag_list: :tag_list}.each do |target,source| + move_into_recurring_todo_param(params, target, source) + end recurring_todo_params end + def edit_recurring_todo_params + # we needed to rename the recurring_period selector in the edit form because + # the form for a new recurring todo and the edit form are on the same page. + # Same goes for start_from and end_date + params['recurring_todo']['recurring_period'] = params['recurring_edit_todo']['recurring_period'] + + { context_name: :context_name, + project_name: :project_name, + tag_list: :edit_recurring_todo_tag_list, + end_date: :recurring_todo_edit_end_date, + start_from: :recurring_todo_edit_start_from}.each do |target,source| + move_into_recurring_todo_param(params, target, source) + end + + # make sure that we set weekly_return_xxx to empty (space) when they are + # not checked (and thus not present in params["recurring_todo"]) + %w{monday tuesday wednesday thursday friday saturday sunday}.each do |day| + params["recurring_todo"]["weekly_return_#{day}"]=' ' if params["recurring_todo"]["weekly_return_#{day}"].nil? + end + params['recurring_todo'] + end + + def move_into_recurring_todo_param(params, target, source) + params[:recurring_todo][target] = params[source] unless params[source].blank? + end + def init @days_of_week = [] 0.upto 6 do |i| diff --git a/app/models/recurring_todos/abstract_recurring_todos_builder.rb b/app/models/recurring_todos/abstract_recurring_todos_builder.rb index e06c31c5..3d9100e9 100644 --- a/app/models/recurring_todos/abstract_recurring_todos_builder.rb +++ b/app/models/recurring_todos/abstract_recurring_todos_builder.rb @@ -5,16 +5,11 @@ module RecurringTodos def initialize(user, attributes) @user = user @attributes = attributes - @filterred_attributes = filter_attributes(attributes) + @filterred_attributes = filter_attributes(@attributes) @saved = false end - def filter_attributes(attributes) - raise Exception.new, "filter_attributes should be overridden" - end - def filter_generic_attributes(attributes) - attributes['tag_list'] = { recurring_period: attributes["recurring_period"], description: attributes['description'], @@ -41,6 +36,16 @@ module RecurringTodos @recurring_todo.project = @filterred_attributes[:project] end + def update(recurring_todo) + @recurring_todo = @pattern.update_recurring_todo(recurring_todo) + @recurring_todo.context = @filterred_attributes[:context] + @recurring_todo.project = @filterred_attributes[:project] + + @saved = @recurring_todo.save + @recurring_todo.tag_with(@filterred_attributes[:tag_list]) if @saved && @filterred_attributes[:tag_list].present? + return @saved + end + def save build @saved = @recurring_todo.save @@ -60,6 +65,16 @@ module RecurringTodos @pattern.attributes end + def attributes_to_filter + raise Exception.new, "attributes_to_filter should be overridden" + end + + def filter_attributes(attributes) + @filterred_attributes = filter_generic_attributes(attributes) + attributes_to_filter.each{|key| @filterred_attributes[key] = attributes[key] if attributes.key?(key)} + @filterred_attributes + end + private def tag_list_or_empty_string(attributes) diff --git a/app/models/recurring_todos/abstract_repeat_pattern.rb b/app/models/recurring_todos/abstract_repeat_pattern.rb index 678e54b0..8ab291ac 100644 --- a/app/models/recurring_todos/abstract_repeat_pattern.rb +++ b/app/models/recurring_todos/abstract_repeat_pattern.rb @@ -3,15 +3,22 @@ module RecurringTodos class AbstractRepeatPattern def initialize(user, attributes) - @attributes = attributes @user = user + @attributes = attributes + @filterred_attributes = nil end def build_recurring_todo @recurring_todo = @user.recurring_todos.build(mapped_attributes) end + def update_recurring_todo(recurring_todo) + recurring_todo.assign_attributes(mapped_attributes) + recurring_todo + end + def mapped_attributes + # should be overwritten to map attributes to activerecord model @attributes end diff --git a/app/models/recurring_todos/daily_recurring_todos_builder.rb b/app/models/recurring_todos/daily_recurring_todos_builder.rb index 48f07836..5e95a0af 100644 --- a/app/models/recurring_todos/daily_recurring_todos_builder.rb +++ b/app/models/recurring_todos/daily_recurring_todos_builder.rb @@ -5,13 +5,12 @@ module RecurringTodos def initialize(user, attributes) super(user, attributes) + @pattern = DailyRepeatPattern.new(user, @filterred_attributes) end - def filter_attributes(attributes) - @filterred_attributes = filter_generic_attributes(attributes) - %w{daily_selector daily_every_x_days}.each{|key| @filterred_attributes[key] = attributes[key] if attributes.key?(key)} - @filterred_attributes + def attributes_to_filter + %w{daily_selector daily_every_x_days} end end diff --git a/app/models/recurring_todos/monthly_recurring_todos_builder.rb b/app/models/recurring_todos/monthly_recurring_todos_builder.rb index a343269e..7be9e230 100644 --- a/app/models/recurring_todos/monthly_recurring_todos_builder.rb +++ b/app/models/recurring_todos/monthly_recurring_todos_builder.rb @@ -7,17 +7,11 @@ module RecurringTodos @pattern = MonthlyRepeatPattern.new(user, @filterred_attributes) end - def filter_attributes(attributes) - @filterred_attributes = filter_generic_attributes(attributes) - + def attributes_to_filter %w{ monthly_selector monthly_every_x_day monthly_every_x_month monthly_every_x_month2 monthly_every_xth_day monthly_day_of_week - }.each do |key| - @filterred_attributes[key] = attributes[key] if attributes.key?(key) - end - - @filterred_attributes + } end end diff --git a/app/models/recurring_todos/recurring_todos_builder.rb b/app/models/recurring_todos/recurring_todos_builder.rb index ccbad04f..4d808fd9 100644 --- a/app/models/recurring_todos/recurring_todos_builder.rb +++ b/app/models/recurring_todos/recurring_todos_builder.rb @@ -19,7 +19,7 @@ module RecurringTodos if %w{daily weekly monthly yearly}.include?(selector) return eval("RecurringTodos::#{selector.capitalize}RecurringTodosBuilder.new(@user, @attributes)") else - raise Exception.new("Unknown recurrence selector (#{selector})") + raise Exception.new("Unknown recurrence selector in recurring_period (#{selector})") end end @@ -27,6 +27,10 @@ module RecurringTodos @builder.build end + def update(recurring_todo) + @builder.update(recurring_todo) + end + def save @project.save if @new_project_created @context.save if @new_context_created diff --git a/app/models/recurring_todos/weekly_recurring_todos_builder.rb b/app/models/recurring_todos/weekly_recurring_todos_builder.rb index 59233b4e..19fa9ae4 100644 --- a/app/models/recurring_todos/weekly_recurring_todos_builder.rb +++ b/app/models/recurring_todos/weekly_recurring_todos_builder.rb @@ -7,16 +7,11 @@ module RecurringTodos @pattern = WeeklyRepeatPattern.new(user, @filterred_attributes) end - def filter_attributes(attributes) - @filterred_attributes = filter_generic_attributes(attributes) - - weekly_attributes = %w{weekly_selector weekly_every_x_week} - %w{monday tuesday wednesday thursday friday saturday sunday}.each{|day| weekly_attributes << "weekly_return_#{day}"} - weekly_attributes.each{|key| @filterred_attributes[key] = attributes[key] if attributes.key?(key)} - - @filterred_attributes + def attributes_to_filter + %w{weekly_selector weekly_every_x_week} + %w{monday tuesday wednesday thursday friday saturday sunday}.map{|day| "weekly_return_#{day}" } end + end end \ No newline at end of file diff --git a/app/models/recurring_todos/yearly_recurring_todos_builder.rb b/app/models/recurring_todos/yearly_recurring_todos_builder.rb index 89cc8a60..7e1e4537 100644 --- a/app/models/recurring_todos/yearly_recurring_todos_builder.rb +++ b/app/models/recurring_todos/yearly_recurring_todos_builder.rb @@ -7,16 +7,10 @@ module RecurringTodos @pattern = YearlyRepeatPattern.new(user, @filterred_attributes) end - def filter_attributes(attributes) - @filterred_attributes = filter_generic_attributes(attributes) - - %w{ yearly_selector yearly_month_of_year yearly_month_of_year2 - yearly_every_x_day yearly_every_xth_day yearly_day_of_week - }.each do |key| - @filterred_attributes[key] = attributes[key] if attributes.key?(key) - end - - @filterred_attributes + def attributes_to_filter + %w{ yearly_selector yearly_month_of_year yearly_month_of_year2 + yearly_every_x_day yearly_every_xth_day yearly_day_of_week + } end end diff --git a/test/controllers/recurring_todos_controller_test.rb b/test/controllers/recurring_todos_controller_test.rb index 0a8c03df..2ee2916d 100644 --- a/test/controllers/recurring_todos_controller_test.rb +++ b/test/controllers/recurring_todos_controller_test.rb @@ -359,4 +359,30 @@ class RecurringTodosControllerTest < ActionController::TestCase assert_equal "completed", rt.state, "repeat pattern should be completed" end + def test_update_recurring_todo + login_as(:admin_user) + rt = recurring_todos(:call_bill_gates_every_day) + current_descr = rt.description + + put :update, + "recurring_todo" => { + "description" => "changed", + "daily_selector" => "daily_every_x_day", + "daily_every_x_days" => "2", + "ends_on" => "no_end_date", + "recurring_target" => "show_from_date" + }, + "recurring_edit_todo" => { + "recurring_period" => rt.recurring_period, + }, + "recurring_todo_edit_start_from" => "2/1/2013", + "end_date" => nil, + "ends_on" => "no_end_date", + "id" => "#{rt.id}", + "context_name" => "library", + format: :js + + assert_equal "changed", rt.reload.description + end + end diff --git a/test/models/recurring_todos/abstract_recurring_todos_builder_test.rb b/test/models/recurring_todos/abstract_recurring_todos_builder_test.rb index 30e2ba1f..21f083dc 100644 --- a/test/models/recurring_todos/abstract_recurring_todos_builder_test.rb +++ b/test/models/recurring_todos/abstract_recurring_todos_builder_test.rb @@ -68,7 +68,7 @@ module RecurringTodos assert_equal "", builder.attributes[:tag_list] end - def test_tags_should_be_saved + def test_tags_should_be_saved_on_create_and_update attributes = { 'recurring_period' => "daily", 'description' => "test", @@ -91,6 +91,14 @@ module RecurringTodos assert !builder.tag_list.present?, "tag list should not be present" assert builder.save, "it should be saved" assert_equal "", builder.saved_recurring_todo.tag_list, "tag list should be empty" + + # tags should be updated + rt = builder.saved_recurring_todo + attributes['tag_list'] = "bar, foo" + updater = RecurringTodosBuilder.new(@admin, attributes) + updater.update(rt) + rt.reload + assert_equal "bar, foo", rt.tag_list end diff --git a/test/models/recurring_todos/recurring_todos_builder_test.rb b/test/models/recurring_todos/recurring_todos_builder_test.rb index fe73634c..fbeff70e 100644 --- a/test/models/recurring_todos/recurring_todos_builder_test.rb +++ b/test/models/recurring_todos/recurring_todos_builder_test.rb @@ -102,7 +102,7 @@ module RecurringTodos end def test_project_is_optional - builder = RecurringTodosBuilder.new(@admin, { + attributes = { 'recurring_period' => "daily", 'description' => "test", 'context_name' => "my new context", @@ -110,13 +110,42 @@ module RecurringTodos 'recurring_target' => 'show_from_date', 'show_always' => true, 'start_from' => '01/01/01', - 'ends_on' => 'no_end_date'}) + 'ends_on' => 'no_end_date'} + + builder = RecurringTodosBuilder.new(@admin, attributes) assert_nil builder.project, "project should not exist" builder.save assert_nil builder.saved_recurring_todo.project end + def test_builder_can_update_description + attributes = { + 'recurring_period' => "daily", + 'description' => "test", + 'context_name' => "my new context", + 'daily_selector' => 'daily_every_work_day', + 'recurring_target' => 'show_from_date', + 'show_always' => true, + 'start_from' => '01/01/01', + 'ends_on' => 'no_end_date'} + + builder = RecurringTodosBuilder.new(@admin, attributes) + builder.save + rt = builder.saved_recurring_todo + + assert_equal "test", rt.description + + attributes['description'] = 'updated' + + updater = RecurringTodosBuilder.new(@admin, attributes) + updater.update(rt) + rt.reload + + assert_equal rt.id, builder.saved_recurring_todo.id + assert_equal "updated", rt.description + end + end end \ No newline at end of file diff --git a/test/models/recurring_todos/weekly_recurring_todos_builder_test.rb b/test/models/recurring_todos/weekly_recurring_todos_builder_test.rb index 76a5286b..1c2873cb 100644 --- a/test/models/recurring_todos/weekly_recurring_todos_builder_test.rb +++ b/test/models/recurring_todos/weekly_recurring_todos_builder_test.rb @@ -30,6 +30,19 @@ module RecurringTodos assert_equal "a repeating todo", result[:description], "description should be preserved" end + def test_attributes_to_filter + attributes = { + 'recurring_period' => 'weekly', + 'description' => 'a repeating todo', # generic + 'weekly_return_monday' => 'm', # weekly specific + } + + w = WeeklyRecurringTodosBuilder.new(@admin, attributes) + assert_equal 9, w.attributes_to_filter.size + assert w.attributes_to_filter.include?('weekly_selector'), "attributes_to_filter should return static attribute weekly_selector" + assert w.attributes_to_filter.include?('weekly_return_monday'), "attributes_to_filter should return generated weekly_return_xyz" + end + end end \ No newline at end of file