From 6156f26c47d260d2cfe2ed673d4d9d140340219e Mon Sep 17 00:00:00 2001 From: lukemelia Date: Sun, 4 Nov 2007 23:06:46 +0000 Subject: [PATCH] Refactored TodosController#create method by introducing class to make it easier to work with the creation params. git-svn-id: http://www.rousette.org.uk/svn/tracks-repos/trunk@628 a4c988fc-2ded-0310-b66e-134b36920a42 --- tracks/app/controllers/application.rb | 3 +- tracks/app/controllers/todos_controller.rb | 99 ++++++++----- tracks/app/models/context.rb | 4 + tracks/app/models/preference.rb | 5 + tracks/app/models/project.rb | 5 + tracks/test/unit/preference_test.rb | 12 ++ .../unit/todo_create_params_helper_test.rb | 136 ++++++++++++++++++ 7 files changed, 228 insertions(+), 36 deletions(-) create mode 100644 tracks/test/unit/todo_create_params_helper_test.rb diff --git a/tracks/app/controllers/application.rb b/tracks/app/controllers/application.rb index 0c62cb39..5c55696d 100644 --- a/tracks/app/controllers/application.rb +++ b/tracks/app/controllers/application.rb @@ -178,8 +178,7 @@ class ApplicationController < ActionController::Base private def parse_date_per_user_prefs( s ) - return nil if s.blank? - Date.strptime(s, prefs.date_format) + prefs.parse_date(s) end def init_data_for_sidebar diff --git a/tracks/app/controllers/todos_controller.rb b/tracks/app/controllers/todos_controller.rb index 767b72f6..146c07c5 100644 --- a/tracks/app/controllers/todos_controller.rb +++ b/tracks/app/controllers/todos_controller.rb @@ -35,48 +35,28 @@ class TodosController < ApplicationController end def create - @todo = current_user.todos.build - p = params['request'] || params + p = TodoCreateParamsHelper.new(params, prefs) + p.parse_dates() unless mobile? - if p['todo']['show_from'] && !mobile? - p['todo']['show_from'] = parse_date_per_user_prefs(p['todo']['show_from']) - end + @todo = current_user.todos.build(p.attributes) - @todo.attributes = p['todo'] - - if p['todo']['project_id'].blank? && !p['project_name'].blank? && p['project_name'] != 'None' - project = current_user.projects.find_by_name(p['project_name'].strip) - unless project - project = current_user.projects.build - project.name = p['project_name'].strip - project.save - @new_project_created = true - end + if p.project_specified_by_name? + project = current_user.projects.find_or_create_by_name(p.project_name) + @new_project_created = project.new_record_before_save? @todo.project_id = project.id end - if p['todo']['context_id'].blank? && !p['context_name'].blank? - context = current_user.contexts.find_by_name(p['context_name'].strip) - unless context - context = current_user.contexts.build - context.name = p['context_name'].strip - context.save - @new_context_created = true - @not_done_todos = [@todo] - end + if p.context_specified_by_name? + context = current_user.contexts.find_or_create_by_name(p.context_name) + @new_context_created = context.new_record_before_save? + @not_done_todos = [@todo] if @new_context_created @todo.context_id = context.id end - if @todo.due? - @todo.due = parse_date_per_user_prefs(p['todo']['due']) unless mobile? - else - @todo.due = "" - end - @saved = @todo.save - if @saved - @todo.tag_with(params[:tag_list], current_user) if params[:tag_list] - @todo.reload + unless (@saved == false) || p.tag_list.blank? + @todo.tag_with(p.tag_list, current_user) + @todo.tags.reload end respond_to do |format| @@ -303,7 +283,6 @@ class TodosController < ApplicationController end # /todos/tag/[tag_name] shows all the actions tagged with tag_name - # def tag @tag_name = params[:name] @@ -610,4 +589,56 @@ class TodosController < ApplicationController end end + class TodoCreateParamsHelper + + def initialize(params, prefs) + @params = params['request'] || params + @prefs = prefs + @attributes = params['request'] && params['request']['todo'] || params['todo'] + end + + def attributes + @attributes + end + + def show_from + @attributes['show_from'] + end + + def due + @attributes['due'] + end + + def project_name + @params['project_name'].strip unless @params['project_name'].nil? + end + + def context_name + @params['context_name'].strip unless @params['context_name'].nil? + end + + def tag_list + @params['tag_list'] + end + + def parse_dates() + @attributes['show_from'] = @prefs.parse_date(show_from) + @attributes['due'] = @prefs.parse_date(due) + @attributes['due'] ||= '' + end + + def project_specified_by_name? + return false unless @attributes['project_id'].blank? + return false if project_name.blank? + return false if project_name == 'None' + true + end + + def context_specified_by_name? + return false unless @attributes['context_id'].blank? + return false if context_name.blank? + true + end + + end end diff --git a/tracks/app/models/context.rb b/tracks/app/models/context.rb index 5ee1bf38..39cce497 100644 --- a/tracks/app/models/context.rb +++ b/tracks/app/models/context.rb @@ -38,6 +38,10 @@ class Context < ActiveRecord::Base s += "Context is #{hidden? ? 'Hidden' : 'Active'}." s += "

" s + end + + def new_record_before_save? + @new_record_before_save end end diff --git a/tracks/app/models/preference.rb b/tracks/app/models/preference.rb index b97bbbf7..8bf328b3 100644 --- a/tracks/app/models/preference.rb +++ b/tracks/app/models/preference.rb @@ -22,4 +22,9 @@ class Preference < ActiveRecord::Base return show_number_completed == 0 end + def parse_date(s) + return nil if s.blank? + Date.strptime(s, date_format) + end + end \ No newline at end of file diff --git a/tracks/app/models/project.rb b/tracks/app/models/project.rb index 6f7be18c..526788cc 100644 --- a/tracks/app/models/project.rb +++ b/tracks/app/models/project.rb @@ -90,6 +90,11 @@ class Project < ActiveRecord::Base def name=(value) self[:name] = value.gsub(/\s{2,}/, " ").strip end + + def new_record_before_save? + @new_record_before_save + end + end class NullProject diff --git a/tracks/test/unit/preference_test.rb b/tracks/test/unit/preference_test.rb index 61c1654f..27306282 100644 --- a/tracks/test/unit/preference_test.rb +++ b/tracks/test/unit/preference_test.rb @@ -19,5 +19,17 @@ class PreferenceTest < Test::Rails::TestCase assert @other_user.preference.show_project_on_todo_done assert !@admin_user.preference.show_project_on_todo_done end + + def test_parse_date + assert_equal Date.new(2007, 5, 20).to_s, @admin_user.preference.parse_date('20/5/2007').to_s + end + + def test_parse_date_returns_nil_if_string_is_empty + assert_nil @admin_user.preference.parse_date('') + end + + def test_parse_date_returns_nil_if_string_is_nil + assert_nil @admin_user.preference.parse_date(nil) + end end diff --git a/tracks/test/unit/todo_create_params_helper_test.rb b/tracks/test/unit/todo_create_params_helper_test.rb new file mode 100644 index 00000000..4b38b60d --- /dev/null +++ b/tracks/test/unit/todo_create_params_helper_test.rb @@ -0,0 +1,136 @@ +require File.dirname(__FILE__) + '/../test_helper' +require 'todos_controller' + +class TodoCreateParamsHelperTest < Test::Rails::TestCase + + def test_works_with_request_as_root_hash_entry + params = {'request' => { 'todo' => { 'description' => 'foo'}}} + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal({'description' => 'foo'}, params_helper.attributes) + end + + def test_works_with_todo_as_root_hash_entry + params = { 'todo' => { 'description' => 'foo'}} + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal({'description' => 'foo'}, params_helper.attributes) + end + + def test_show_from_accessor + expected_date = Time.now.to_date + params = { 'todo' => { 'show_from' => expected_date}} + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal(expected_date, params_helper.show_from) + end + + def test_due_accessor + expected_date = Time.now.to_date + params = { 'todo' => { 'due' => expected_date}} + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal(expected_date, params_helper.due) + end + + def test_tag_list_accessor + params = { 'todo' => { }, 'tag_list' => 'foo, bar'} + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal('foo, bar', params_helper.tag_list) + end + + def test_parse_dates_parses_show_from_date_based_on_prefs + params = { 'todo' => { 'show_from' => '5/20/07', 'due' => '5/23/07'}} + prefs = flexmock() + prefs.should_receive(:parse_date).with('5/20/07').and_return(Date.new(2007, 5, 20)) + prefs.should_receive(:parse_date).with('5/23/07').and_return(Date.new(2007, 5, 23)) + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + params_helper.parse_dates() + assert_equal Date.new(2007, 5, 20), params_helper.show_from + end + + def test_parse_dates_parses_due_date_based_on_prefs + params = { 'todo' => { 'show_from' => '5/20/07', 'due' => '5/23/07'}} + prefs = flexmock() + prefs.should_receive(:parse_date).with('5/20/07').and_return(Date.new(2007, 5, 20)) + prefs.should_receive(:parse_date).with('5/23/07').and_return(Date.new(2007, 5, 23)) + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + params_helper.parse_dates() + assert_equal Date.new(2007, 5, 23), params_helper.due + end + + def test_parse_dates_sets_due_to_empty_string_if_nil + params = { 'todo' => { 'show_from' => '5/20/07', 'due' => nil}} + prefs = flexmock() + prefs.should_receive(:parse_date).with('5/20/07').and_return(Date.new(2007, 5, 20)) + prefs.should_receive(:parse_date).with(nil).and_return(nil) + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + params_helper.parse_dates() + assert_equal '', params_helper.due + end + + def test_project_name_is_stripped_of_leading_and_trailing_whitespace + params = { 'project_name' => ' Visit New Orleans ' } + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal 'Visit New Orleans', params_helper.project_name + end + + def test_project_name_is_nil_when_unspecified + params = { } + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_nil params_helper.project_name + end + + def test_context_name_is_stripped_of_leading_and_trailing_whitespace + params = { 'context_name' => ' mobile phone ' } + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal 'mobile phone', params_helper.context_name + end + + def test_context_name_is_nil_when_unspecified + params = { } + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_nil params_helper.context_name + end + + def test_project_specified_by_name_is_false_when_project_id_is_specified + params = { 'todo' => { 'project_id' => 2 } } + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal false, params_helper.project_specified_by_name? + end + + def test_project_specified_by_name_is_false_when_project_name_is_blank + params = { 'project_name' => nil, 'todo' => {} } + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal false, params_helper.project_specified_by_name? + end + + def test_project_specified_by_name_is_false_when_project_name_is_none + params = { 'project_name' => 'None', 'todo' => {} } + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal false, params_helper.project_specified_by_name? + end + + def test_context_specified_by_name_is_false_when_context_id_is_specified + params = { 'todo' => { 'context_id' => 3 } } + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal false, params_helper.context_specified_by_name? + end + + def test_context_specified_by_name_is_false_when_context_name_is_blank + params = { 'context_name' => nil, 'todo' => {} } + prefs = flexmock() + params_helper = TodosController::TodoCreateParamsHelper.new(params, prefs) + assert_equal false, params_helper.context_specified_by_name? + end + +end \ No newline at end of file