Fixes bug that prevented ActiveFedora from deserializing classes #696
Fixes bug that prevented ActiveFedora from deserializing classes #696
Conversation
lib/active_fedora/core.rb
Outdated
There was a problem hiding this comment.
Would you please combine the two conditionals:
unless ActiveFedora.fedora.base_path == '/' || id.start_with? "#{ActiveFedora.fedora.base_path}/"
also freeze the string to prevent allocation on each trip through here: '/'.freeze
hectorcorrea
force-pushed
the
empty_base_path
branch
from
0d2bb58 to
e929f0c
Compare
1 similar comment
hectorcorrea
force-pushed
the
empty_base_path
branch
from
e929f0c to
ce952bf
Compare
spec/unit/core_spec.rb
Outdated
There was a problem hiding this comment.
How about testing id_to_uri directly rather than doing create/lookup to test that change?
There was a problem hiding this comment.
OK. I'll make that change tomorrow and resubmit.
There was a problem hiding this comment.
If you want to validate that the correct class is found, you should move the tests to spec/integration, because that would be an integration test not a unit test.
There was a problem hiding this comment.
OK. I've updated the tests to go directly against ActiveFedora::Base.id_to_uri and bypass Fedora.
hectorcorrea
force-pushed
the
empty_base_path
branch
from
ce952bf to
78ebe76
Compare
spec/unit/core_spec.rb
Outdated
There was a problem hiding this comment.
This test is now a duplicate of what is on line 96. Can we just remove it?
…ectly when the base_path setting in fedora.yaml was not set
hectorcorrea
force-pushed
the
empty_base_path
branch
from
78ebe76 to
669ac73
Compare
Fixes bug that prevented ActiveFedora from deserializing classes
3 participants
correctly when the base_path setting in fedora.yaml was not set.
Takes care of #657
Note: When I removed the base_path from config/fedora.yml and ran all tests I got 20+ failing tests. I spot checked several of them and the issues where because the tests were hard-coded to expect a /test/ on the URL or because the cleaner didn't know how to clean from fedora/rest rather than fedora/rest/test. I didn't think this should be an issue with the code given the new tests that evaluate the new changes.