Don't try to parse empty dates

[awead]

I'm getting invalid date when I have a date property that's empty: ''

Not sure if this is a valid use case or not, but I'm getting it in the context of a Sufia app. Maybe I need to rectify this at the form/presenter level instead? Otherwise, here's the PR...

[awead]

Gah! Cut and pasted the wrong commit message...

[awead]

Now I'm getting issues with calling empty? on FixNum. I'll update this... suggestions welcome.

[awead]

Ok, got this sorted.

[ojlyytinen]

In the case of an empty string value for something that should be a date, I think adapt_single_attribute_value should probably return nil. What I hoped to do with the pull request that introduced this bit of code was that date attributes would consistently be DateTimes, or nils if they're not present, rather than sometimes also be strings.

This still leaves the possibility that adapt_single_attribute_value might encounter a value that should be a date and is a non-empty string that's not parseable as a date. I think raising an exception is the right thing to do in this case.

So I would change this to something like

       def adapt_single_attribute_value(value, attribute_name)
         if date_attribute?(attribute_name)
           return nil unless value.present?
           DateTime.parse(value)
         else
           value
         end
       end

[awead]

@ojlyytinen Good point! It's been amended.

[jcoyne]

Why don't we store dates in the profile json serialized to iso8601, then we can just see if the field looks like a data and then parse it? That would avoid a lot of these problems which I'm encountering too. We have a date field that is sometimes a DateTime and sometimes a string (e.g. "circa 1840").

[ojlyytinen]

I wouldn't return a date only on the basis that something looks like a date. That could lead to unexpectedly getting DateTimes where you expect a String. For example, "101-001" is a valid iso8601 date. Even ignoring unusual, but valid, dates someone could put just a date in a field meant for textual notes which could then unexpectedly come back as a DateTime.

I think it might be a good idea to have some kind of easy way to parse any field with a user supplied method. Then anyone could add any kind of parsing behaviour they wanted. Could we have something similar to serialize in ActiveRecord? Then you could do something along these lines.

class Foo < ActiveFedora::Base
  property :test, predicate: ::RDF::URI.new('http://www.example.com/test')
  serialize :test, SomeClass
end

class SomeClass
  def self.dump(obj)
    # serialize obj
  end
  def self.load(data)
    # deserialize data
  end
end

[awead]

Why not just override adapt_single_attribute_value in your model? Or my original solution was to just return the value if it wasn't "datable." That would solve @jcoyne's problem of DateTime vs. String. In any case, we ought to merge something. I'm still having to pin to this branch because of the original parsing of empty values.

[ojlyytinen]

As far as I'm concerned, I think this PR is fine as it is. Overriding adapt_single_attribute_value works if anyone needs special parsing behaviour.

[awead]

@ojlyytinen @jcoyne how do we feel about this? Are we okay merging?

[ojlyytinen]

I think this is fine to go in as is.