add some more tests

app/models/recurring_todos/abstract_recurring_todos_builder.rb

@@ -21,31 +21,16 @@ module RecurringTodos
     # build does not add tags. For tags, the recurring todos needs to be saved
     def build
       @recurring_todo = @pattern.build_recurring_todo(@mapped_attributes)
-
     end
 
     def update(recurring_todo)
       @recurring_todo = @pattern.update_recurring_todo(recurring_todo, @mapped_attributes)
-      @saved = @recurring_todo.save
-      @recurring_todo.tag_with(@filterred_attributes.get(:tag_list)) if @saved && @filterred_attributes.get(:tag_list).present?
-      @recurring_todo.reload
-      
-      return @saved
+      save_recurring_todo
     end
 
     def save
       build
-      @saved = @recurring_todo.save
-      @recurring_todo.tag_with(@filterred_attributes.get(:tag_list)) if @saved && @filterred_attributes.get(:tag_list).present?
-      return @saved
-    end
-
-    def save_collection(collection, collection_id)
-      # save object (project or context) and add its id to @mapped_attributes and remove the object from the attributes
-      object = @mapped_attributes.get(collection)
-      object.save
-      @mapped_attributes.set(collection_id, object.id)
-      @mapped_attributes.except(collection)
+      save_recurring_todo
     end
 
     def save_project
@@ -71,29 +56,31 @@ module RecurringTodos
     end
 
     def filter_attributes(attributes)
+      # get pattern independend attributes
       filterred_attributes = filter_generic_attributes(attributes)
-      attributes_to_filter.each{|key| filterred_attributes.set(key, attributes.get(key)) if attributes.key?(key)}
+      # append pattern specific attributes
+      attributes_to_filter.each{|key| filterred_attributes[key]= attributes[key] if attributes.key?(key)}
+
       filterred_attributes
     end
 
     def filter_generic_attributes(attributes)
       return Tracks::AttributeHandler.new(@user, {
-        recurring_period: attributes.get(:recurring_period),
-        description:      attributes.get(:description), 
-        notes:            attributes.get(:notes),
+        recurring_period: attributes[:recurring_period],
+        description:      attributes[:description],
+        notes:            attributes[:notes],
         tag_list:         tag_list_or_empty_string(attributes),
-        start_from:       attributes.get(:start_from),
-        end_date:         attributes.get(:end_date),
-        ends_on:          attributes.get(:ends_on),
-        show_always:      attributes.get(:show_always),
-        target:           attributes.get(:target),
-        project:          attributes.get(:project),
-        context:          attributes.get(:context),
-        project_id:       attributes.get(:project_id),
-        context_id:       attributes.get(:context_id),
-        target:           attributes.get(:recurring_target),
-        show_from_delta:  attributes.get(:recurring_show_days_before),
-        show_always:      attributes.get(:recurring_show_always)
+        start_from:       attributes[:start_from],
+        end_date:         attributes[:end_date],
+        ends_on:          attributes[:ends_on],
+        target:           attributes[:target],
+        project:          attributes[:project],
+        context:          attributes[:context],
+        project_id:       attributes[:project_id],
+        context_id:       attributes[:context_id],
+        target:           attributes[:recurring_target],
+        show_from_delta:  attributes[:recurring_show_days_before],
+        show_always:      attributes[:recurring_show_always]
       })
     end
 
@@ -103,8 +90,9 @@ module RecurringTodos
     end
 
     # helper method to be used in mapped_attributes in subclasses
+    # changes name of key from source_key to key
     def map(mapping, key, source_key)
-      mapping.set(key, mapping.get(source_key))
+      mapping[key] = mapping[source_key]
       mapping.except(source_key)
     end
 
@@ -117,7 +105,7 @@ module RecurringTodos
       return nil if key.nil?
 
       raise Exception.new, "recurrence selector pattern (#{key}) not given" unless @attributes.selector_key_present?(key)
-      selector = @attributes.get(key)
+      selector = @attributes[key]
 
       raise Exception.new, "unknown recurrence selector pattern: '#{selector}'" unless valid_selector?(selector)
 
@@ -131,9 +119,28 @@ module RecurringTodos
 
     private
 
+    def save_recurring_todo
+      @saved = @recurring_todo.save
+      save_tags if @saved
+      return @saved
+    end
+
+    def save_tags
+      @recurring_todo.tag_with(@filterred_attributes[:tag_list]) if @filterred_attributes[:tag_list].present?
+      @recurring_todo.reload
+    end
+
+    def save_collection(collection, collection_id)
+      # save object (project or context) and add its id to @mapped_attributes and remove the object from the attributes
+      object = @mapped_attributes[collection]
+      object.save
+      @mapped_attributes[collection_id] = object.id
+      @mapped_attributes.except(collection)
+    end
+
     def tag_list_or_empty_string(attributes)
       # avoid nil
-      attributes.get(:tag_list).blank? ? "" : attributes.get(:tag_list).strip
+      attributes[:tag_list].blank? ? "" : attributes[:tag_list].strip
     end
 
   end

app/models/recurring_todos/abstract_repeat_pattern.rb

@@ -46,6 +46,10 @@ module RecurringTodos
       @attributes = Tracks::AttributeHandler.new(@user, recurring_todo.attributes)
     end
 
+    def valid?
+      @recurring_todo.valid?
+    end
+
     def validate_not_blank(object, msg)
       errors[:base] << msg if object.blank?
     end
@@ -80,7 +84,7 @@ module RecurringTodos
         validate_not_nil(show_always, "Please select when to show the action")
         validate_not_blank(show_from_delta, "Please fill in the number of days to show the todo before the due date") unless show_always
       else
-        raise Exception.new, "unexpected value of recurrence target selector '#{target}'"
+        errors[:base] << "Unexpected value of recurrence target selector '#{target}'"
       end
     end
 
@@ -89,7 +93,7 @@ module RecurringTodos
     end
 
     def get(attribute)
-      @attributes.get attribute
+      @attributes[attribute]
     end
 
   end

app/models/recurring_todos/recurring_todos_builder.rb

@@ -12,15 +12,12 @@ module RecurringTodos
       parse_project
       parse_context
 
-      @builder = create_builder(@attributes.get(:recurring_period))
+      @builder = create_builder(@attributes[:recurring_period])
     end
 
     def create_builder(selector)
-      if %w{daily weekly monthly yearly}.include?(selector)
-        return eval("RecurringTodos::#{selector.capitalize}RecurringTodosBuilder.new(@user, @attributes)")
-      else
-        raise Exception.new("Unknown recurrence selector in :recurring_period (#{selector})")
-      end
+      raise "Unknown recurrence selector in :recurring_period (#{selector})" unless valid_selector? selector
+      eval("RecurringTodos::#{selector.capitalize}RecurringTodosBuilder.new(@user, @attributes)")
     end
 
     def build
@@ -50,18 +47,26 @@ module RecurringTodos
       @builder.attributes
     end
 
+    def pattern
+      @builder.pattern
+    end
+
     private
 
+    def valid_selector?(selector)
+      %w{daily weekly monthly yearly}.include?(selector)
+    end
+
     def parse_dates
       %w{end_date start_from}.each {|date| @attributes.parse_date date }
     end
 
     def parse_project
-      @project, @new_project_created = @attributes.parse_collection(:project, @user.projects, @attributes.project_name)
+      @project, @new_project_created = @attributes.parse_collection(:project, @user.projects)
     end
 
     def parse_context
-      @context, @new_context_created = @attributes.parse_collection(:context, @user.contexts, @attributes.context_name)
+      @context, @new_context_created = @attributes.parse_collection(:context, @user.contexts)
     end
 
   end

lib/tracks/attribute_handler.rb

@@ -13,6 +13,10 @@ module Tracks
       @attributes[attribute.to_sym] 
     end
 
+    def [](attribute)
+      get attribute
+    end
+
     def set(key, value)
       @attributes[key.to_sym] = value
     end
@@ -21,6 +25,10 @@ module Tracks
       @attributes[key.to_sym] ||= value
     end
 
+    def []=(attribute, value)
+      set attribute, value
+    end
+
     def except(key)
       AttributeHandler.new(@user, @attributes.except(key.to_sym))
     end
@@ -30,19 +38,19 @@ module Tracks
     end
 
     def selector_key_present?(key)      
-      @attributes.key?(key.to_sym)
+      key?(key)
     end
 
     def parse_date(date)
       set(date, @user.prefs.parse_date(get(date)))
     end
 
-    def parse_collection(object_type, relation, name)
+    def parse_collection(object_type, relation)
       object = nil
       new_object_created = false
 
       if specified_by_name?(object_type)
-        object, new_object_created = find_or_create_by_name(relation, name)
+        object, new_object_created = find_or_create_by_name(relation, object_name(object_type))
         # put id of object in @attributes, i.e. set :project_id to project.id
         @attributes[object_type.to_s + "_id"] = object.id unless new_object_created
       else
@@ -53,6 +61,10 @@ module Tracks
       return object, new_object_created
     end
 
+    def object_name(object_type)
+      send("#{object_type}_name")
+    end
+
     def attribute_with_id_of(object_type)
       map = { project: 'project_id', context: 'context_id' }
       get map[object_type]

test/models/attribute_handler_test.rb

@@ -0,0 +1,97 @@
+require_relative '../test_helper'
+
+class AttributeHandlerTest < ActiveSupport::TestCase
+  fixtures :users
+
+  def test_setting_attributes
+    h = Tracks::AttributeHandler.new(nil, {})
+
+    h.set('test', '123')
+    h['other']='one'
+    assert_equal '123', h.attributes[:test], ":test should be added"
+    assert_nil h.attributes['test'],         "string should be converted to symbol"
+    assert_equal 'one', h[:other],           ":other should be added as symbol using []="
+
+    assert_nil h.attributes[:new]
+    h.set_if_nil(:new, 'value')
+    assert_equal 'value', h.attributes[:new], "value should be set for new key"
+    h.set_if_nil(:new, 'other')
+    assert_equal 'value', h.attributes[:new], "value should not be set for existing key"
+
+    h.attributes[:empty] = nil 
+    h.set_if_nil(:empty, "test")
+    assert_equal "test", h.attributes[:empty], "nil value should be overwritten"
+  end
+
+  def test_getting_attributes
+    h = Tracks::AttributeHandler.new(nil, { :get => "me"} )
+    assert h.key?(:get),  "attributehandler should have key :get"
+    assert h.key?('get'), "attributehandler should have key :get"
+    assert_equal "me", h.attributes[:get], "attributehandler should have key :get"
+    assert_equal "me", h.get('get'), "key should be converted to symbol"
+    assert_equal "me", h[:get], "AttributeHandler should act like hash"
+  end
+
+  def test_removing_attributes
+    h = Tracks::AttributeHandler.new(nil, { :i_am => "here"} )
+    assert h.key?(:i_am)
+
+    h.except(:i_am)
+    assert h.key?(:i_am), "AttributeHandler should be immutable"
+
+    h2 = h.except("i_am")
+    assert !h2.key?(:i_am), "key as symbol should be removed"
+  end
+
+  def test_project_specified_by_name
+    h = Tracks::AttributeHandler.new(nil, { } )
+
+    assert !h.project_specified_by_name?, "project is not specified by id or by name"
+
+    h[:project_id]=4
+    assert !h.project_specified_by_name?, "project is specified by id, not by name"
+
+    h = h.except(:project_id)
+    h[:project_name] = "A project"
+    assert h.project_specified_by_name?, "project is specified by name"
+
+    h[:project_name] = "None"
+    assert !h.project_specified_by_name?, "None is special token to specify nil-project"
+  end
+
+  def test_context_specified_by_name
+    h = Tracks::AttributeHandler.new(nil, { } )
+    assert !h.context_specified_by_name?, "context is not specified by id or by name"
+
+    h["context_id"] = 4
+    assert !h.context_specified_by_name?, "context is specified by id, not by name"
+
+    h = h.except(:context_id)
+    h[:context_name] = "A context"
+    assert h.context_specified_by_name?, "context is specified by name"
+  end
+
+  def test_parse_collection
+    admin   = users(:admin_user)
+    project = admin.projects.first
+    h = Tracks::AttributeHandler.new(admin, { "project_id" => project.id } )
+
+    parsed_project, new_project_created = h.parse_collection(:project, admin.projects)
+    assert !new_project_created, "should find existing project"
+    assert_equal project.id, parsed_project.id, "it should find the project"
+
+    h = Tracks::AttributeHandler.new(admin, { "project_name" => project.name } )
+
+    parsed_project, new_project_created = h.parse_collection(:project, admin.projects)
+    assert !new_project_created, "should find existing project"
+    assert_equal project.id, parsed_project.id, "it should find the project"
+
+    h = Tracks::AttributeHandler.new(admin, { "project_name" => "new project" } )
+
+    parsed_project, new_project_created = h.parse_collection(:project, admin.projects)
+    assert new_project_created, "should detect that no project exist with that name"
+    assert_equal "new project", parsed_project.name, "it should return a new project"
+    assert !parsed_project.persisted?, "new project should not be persisted (yet)"
+  end
+
+end

test/models/recurring_todos/abstract_recurring_todos_builder_test.rb

@@ -46,7 +46,7 @@ module RecurringTodos
       }
 
       builder = RecurringTodosBuilder.new(@admin, attributes)
-      assert_equal "tag, this, that", builder.attributes.get(:tag_list)
+      assert_equal "tag, this, that", builder.attributes[:tag_list]
 
       # given attributes without tag_list
       attributes = {
@@ -55,7 +55,7 @@ module RecurringTodos
       }
 
       builder = RecurringTodosBuilder.new(@admin, attributes)
-      assert_equal "", builder.attributes.get(:tag_list)
+      assert_equal "", builder.attributes[:tag_list]
 
       # given attributes with nil tag_list
       attributes = {
@@ -65,7 +65,7 @@ module RecurringTodos
       }
 
       builder = RecurringTodosBuilder.new(@admin, attributes)
-      assert_equal "", builder.attributes.get(:tag_list)
+      assert_equal "", builder.attributes[:tag_list]
 
       # given attributes with empty tag_list ==> should be stripped
       attributes = {
@@ -75,7 +75,7 @@ module RecurringTodos
       }
 
       builder = RecurringTodosBuilder.new(@admin, attributes)
-      assert_equal "", builder.attributes.get(:tag_list)
+      assert_equal "", builder.attributes[:tag_list]
     end
 
     def test_tags_should_be_saved_on_create_and_update
@@ -137,27 +137,27 @@ module RecurringTodos
     def test_map_removes_mapped_key
       attributes = Tracks::AttributeHandler.new(@admin, { :source => "value"})
 
-      arp = WeeklyRecurringTodosBuilder.new(@admin, attributes)
-      attributes = arp.map(attributes, :target, :source)
+      a_builder = WeeklyRecurringTodosBuilder.new(@admin, attributes)
+      attributes = a_builder.map(attributes, :target, :source)
 
-      assert_equal "value", attributes.get(:target)
-      assert_nil attributes.get(:source)
+      assert_equal "value", attributes[:target]
+      assert_nil attributes[:source]
       assert !attributes.key?(:source)
     end
 
     def test_get_selector_removes_selector_from_hash
       attributes = Tracks::AttributeHandler.new(@admin, { :selector => "weekly" })
-      arp = WeeklyRecurringTodosBuilder.new(@admin, attributes)
+      a_builder = WeeklyRecurringTodosBuilder.new(@admin, attributes)
 
-      assert "weekly", arp.get_selector(:selector)
-      assert !arp.attributes.key?(:selector)
+      assert "weekly", a_builder.get_selector(:selector)
+      assert !a_builder.attributes.key?(:selector)
     end
 
     def test_get_selector_raises_exception_when_missing_selector
       attributes = Tracks::AttributeHandler.new(@admin, { })
-      arp = WeeklyRecurringTodosBuilder.new(@admin, attributes)
+      a_builder = WeeklyRecurringTodosBuilder.new(@admin, attributes)
 
-      assert_raise(Exception, "should raise exception when recurrence selector is missing"){ arp.get_selector(:selector) }
+      assert_raise(Exception, "should raise exception when recurrence selector is missing"){ a_builder.get_selector(:selector) }
     end
 
   end

test/models/recurring_todos/abstract_repeat_pattern_test.rb

@@ -9,6 +9,93 @@ module RecurringTodos
       @admin = users(:admin_user)
     end 
 
+    def test_pattern_builds_from_existing_recurring_todo
+      rt = @admin.recurring_todos.first
+
+      pattern = rt.pattern
+      assert pattern.is_a?(DailyRepeatPattern), "recurring todo should have daily pattern"
+    end
+
+    def test_validation_on_due_date
+      attributes = {
+        'recurring_period'     => 'weekly',
+        'recurring_target'     => 'due_date',
+        'description'          => 'a repeating todo',  # generic
+        'weekly_return_monday' => 'm',                 # weekly specific
+        'ends_on'              => 'ends_on_end_date',
+        'end_date'             => Time.zone.now + 1.week,
+        'context_id'           => @admin.contexts.first.id,
+        'start_from'           => Time.zone.now - 1.week,
+        'weekly_every_x_week'  => 1,
+      }
+
+      pattern = create_pattern(attributes)
+      assert !pattern.valid?, "should fail because show_always and show_from_delta are not there"
+
+      attributes['recurring_show_always'] = false
+      pattern = create_pattern(attributes)
+      assert !pattern.valid?, "should fail because show_from_delta is not there"
+
+      attributes[:recurring_show_days_before] = 5
+      pattern = create_pattern(attributes)
+      assert pattern.valid?, "should be valid:" + pattern.errors.full_messages.to_s
+    end
+
+    def test_validation_on_start_date
+      attributes = {
+        'recurring_period'           => 'weekly',
+        'recurring_target'           => 'due_date',
+        'description'                => 'a repeating todo',  # generic
+        'weekly_return_monday'       => 'm',                 # weekly specific
+        'ends_on'                    => 'ends_on_end_date',
+        'context_id'                 => @admin.contexts.first.id,
+        'end_date'                   => Time.zone.now + 1.week,
+        'weekly_every_x_week'        => 1,
+        'recurring_show_always'      => false,
+        'recurring_show_days_before' => 5,
+      }
+      pattern = create_pattern(attributes)
+      assert !pattern.valid?, "should be not valid because start_from is empty"
+
+      attributes['start_from'] = Time.zone.now - 1.week
+      pattern = create_pattern(attributes)
+      assert pattern.valid?, "should be valid: " + pattern.errors.full_messages.to_s
+    end
+
+    def test_validation_on_end_date
+      attributes = {
+        'recurring_period'           => 'weekly',
+        'recurring_target'           => 'due_date',
+        'description'                => 'a repeating todo',  # generic
+        'weekly_return_monday'       => 'm',                 # weekly specific
+        'ends_on'                    => 'invalid_value',
+        'context_id'                 => @admin.contexts.first.id,
+        'start_from'                 => Time.zone.now - 1.week,
+        'weekly_every_x_week'        => 1,
+        'recurring_show_always'      => false,
+        'recurring_show_days_before' => 5,
+      }
+
+      pattern = create_pattern(attributes)
+      assert !pattern.valid?
+
+      attributes['ends_on']='ends_on_end_date'
+      pattern = create_pattern(attributes)
+      assert !pattern.valid?, "should not be valid, because end_date is not supplied"
+
+      attributes['end_date']= Time.zone.now + 1.week
+      pattern = create_pattern(attributes)
+      assert pattern.valid?, "should be valid"
+    end
+
+    private
+
+    def create_pattern(attributes)
+      builder = RecurringTodosBuilder.new(@admin, attributes)
+      builder.build
+      builder.pattern
+    end
+
   end
 
 end

test/models/recurring_todos/recurring_todos_builder_test.rb

@@ -10,7 +10,11 @@ module RecurringTodos
     end 
 
     def test_create_builder_needs_selector
-      assert_raise(Exception){ builder = RecurringTodosBuilder.new(@admin, {}) }
+      assert_raise(RuntimeError){ builder = RecurringTodosBuilder.new(@admin, {}) }
+    end
+
+    def test_create_builder_needs_valid_selector
+      assert_raise(RuntimeError){ builder = RecurringTodosBuilder.new(@admin, { 'recurring_period' => 'wrong_value'}) }
     end
 
     def test_create_builder_uses_selector
@@ -35,8 +39,8 @@ module RecurringTodos
         'end_date' => '05/05/05'
         })
 
-      assert builder.attributes.get(:start_from).is_a?(ActiveSupport::TimeWithZone), "Dates should be parsed to ActiveSupport::TimeWithZone class"
-      assert builder.attributes.get(:end_date).is_a?(ActiveSupport::TimeWithZone), "Dates should be parsed to ActiveSupport::TimeWithZone class"
+      assert builder.attributes[:start_from].is_a?(ActiveSupport::TimeWithZone), "Dates should be parsed to ActiveSupport::TimeWithZone class"
+      assert builder.attributes[:end_date  ].is_a?(ActiveSupport::TimeWithZone), "Dates should be parsed to ActiveSupport::TimeWithZone class"
     end
 
     def test_exisisting_project_is_used