Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Save time zone information for DateTimes. Solr returns DateTimes rather than Strings. #923

Merged
merged 1 commit into from
Oct 27, 2015

Conversation

ojlyytinen
Copy link
Contributor

As it stands, time zone information is not stored in Fedora, instead it's just always set to Z. All time information is effectively shifted by your local time offset if you take time zones into account. This results in unexpected behaviour where timestamps for example end up in the future if your local time zone offset is positive.

class Foo < ActiveFedora::Base
  property :date, predicate: ::RDF::DC.date do |index|
    index.type :date
  end
end

foo = Foo.create(date: [DateTime.now])
sleep 1
foo.date.first > DateTime.now # => true assuming your time offset is positive

There seems to be a bug in RDF::Literal::DateTime#to_s which prints out time zone as Z if you initialise the object with a DateTime rather than a String, so this should be fixed there. But a similar bug is present also in the monkey patch applied in lib/active_fedora.rb which is fixed in this PR.

The PR also makes Solr-loaded documents return DateTime objects rather than Strings when indexing type has been set to date. What used to happen is that the same field which returned a DateTime object when loaded from Fedora would return a String when loaded from Solr.

let(:date2) { DateTime.parse("2015-10-22T15:34:20.323-11:00") }
subject { Foo.create(date: [date], single_date: date2) }

describe "saving and loading in Fedora" do
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is the "load" happening in this scenario?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to reset attributes after save (or create). At least the time zone gets reset with create alone and that test fails. I could add a .reload in there though just in case.

@jcoyne
Copy link
Member

jcoyne commented Oct 23, 2015

Please rebase your changes and run rubocop. I suspect it will tell you to remove a bunch of returns at the end of methods.

as DateTime rather than String when index type is :date.
@ojlyytinen
Copy link
Contributor Author

We can't really use the field name suffixes to determine the type of the field since loading only uses the object_profile which doesn't have those suffixes. As far as I can tell, the only information we have about the field is what's in ActiveFedora::Attributes::NodeConfig and the attribute value itself. I've now changed it to parse the value as a DateTime if the node config has type == :date or class_name == DateTime. When defining properties these are set with index.type :date and class_name: 'DateTime', respectively. Either will work.

Rubocop didn't mind the returns at the end of methods but I removed them anyway. And it didn't like the complexity of adapt_attribute_value so I restructured it a little bit.

jcoyne added a commit that referenced this pull request Oct 27, 2015
Save time zone information for DateTimes. Solr returns DateTimes rather than Strings.
@jcoyne jcoyne merged commit 93ad980 into samvera:master Oct 27, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants