Implements delete for direct containers. #832
Conversation
|
Could you also add a test for this behavior in direct_container_spec.rb? |
|
@flyingzumwalt I added a couple of tests for direct_container and that uncovered a few bugs. I still need to (1) make sure the new tests that I added are correct and exercising the right functionality, and (2) figure out why 4 of the integration tests are now failing. |
|
|
||
| it "deletes the contained resource directly" do | ||
| expect(history.files).to eq [file1, file2] | ||
| file1.delete |
There was a problem hiding this comment.
@jcoyne could you confirm is this how we want to test the delete in direct containers? (1/2)
There was a problem hiding this comment.
Not quite. This is just deleting the file, but not through the association.
| @@ -63,7 +63,15 @@ def ldp_connection | |||
| # If this file has a parent with ldp#contains, we know it is not new. | |||
| # By tracking exists we prevent an unnecessary HEAD request. | |||
| def new_record? | |||
| !@exists && ldp_source.new? | |||
| !@exists && ldp_new_record? | |||
There was a problem hiding this comment.
@jcoyne this is what I needed to do to get the delete for direct_container to work. The call to target.delete(record) in collection_association.rb internally calls new_record? after the actual record has been deleted. I believe when Ruby processes target.delete(record) it's using the == operator defined in file.rb which in turns calls new_record.
There was a problem hiding this comment.
I wonder if we should be tracking @destroyed like core does? (https://github.com/projecthydra/active_fedora/blob/7ed4ca1cb4443b6b39dccea4dba18442809cb028/lib/active_fedora/persistence.rb#L49) Can you show me where target.delete(record) is calling ==?
There was a problem hiding this comment.
Tracking @destroyed might be another way to do it.
There is no explicit place in the code where we call == but when I step into the target.delete(record) line it takes me directly to the == so I suspect Ruby is internally calling == when deleting an element in an array.
There was a problem hiding this comment.
So, where is target.delete()? in CollectionAssociation?
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm not sure we need == to use new_record?. I'm working up a new PR to remove that. Please stand by.
There was a problem hiding this comment.
@hectorcorrea try rebasing against #840. Then would you be able to revert your change to new_record?
There was a problem hiding this comment.
@jcoyne that did it!
|
@jcoyne @flyingzumwalt I think this PR is good to go (despite coverall's meaningless warning) Eventually I would like to add another test in |
| @@ -221,6 +221,10 @@ def update_record(options = {}) | |||
| refresh | |||
| end | |||
|
|
|||
| def build_ldp_resource(id) | |||
There was a problem hiding this comment.
Can you remove this? I don't see it used anywhere.
There was a problem hiding this comment.
Actually, now I recall that we did have some reason to add it. Never mind.
Implements delete for direct containers.
Fixes #794