Skip to content

Commit

Permalink
Remove current children from virtual object before initializing conte…
Browse files Browse the repository at this point in the history
…nt metadata

This prevents a situation where lots of manual clean-up is required to remove old children from a parent.

Fixes #409
  • Loading branch information
mjgiarlo committed Sep 17, 2019
1 parent 0de1104 commit 30a9708
Show file tree
Hide file tree
Showing 5 changed files with 148 additions and 41 deletions.
31 changes: 7 additions & 24 deletions .rubocop_todo.yml
@@ -1,6 +1,6 @@
# This configuration was generated by
# `rubocop --auto-gen-config`
# on 2019-09-10 10:01:59 -0500 using RuboCop version 0.74.0.
# on 2019-09-17 12:57:49 -0700 using RuboCop version 0.74.0.
# The point is for the user to remove these configuration records
# one by one as the offenses are removed from the code base.
# Note that changes in the inspected code, or installation of new
Expand All @@ -24,13 +24,7 @@ Lint/MissingCopEnableDirective:
Exclude:
- 'spec/dor/update_marc_record_service_spec.rb'

# Offense count: 2
Lint/UriEscapeUnescape:
Exclude:
- 'app/controllers/sdr_controller.rb'
- 'spec/controllers/sdr_controller_spec.rb'

# Offense count: 22
# Offense count: 27
Metrics/AbcSize:
Max: 87

Expand All @@ -43,7 +37,7 @@ Metrics/ClassLength:
Metrics/CyclomaticComplexity:
Max: 11

# Offense count: 26
# Offense count: 27
# Configuration parameters: CountComments, ExcludedMethods.
Metrics/MethodLength:
Max: 43
Expand Down Expand Up @@ -101,20 +95,11 @@ RSpec/Be:
Exclude:
- 'spec/services/public_xml_service_spec.rb'

# Offense count: 1
RSpec/BeforeAfterAll:
Exclude:
- 'spec/spec_helper.rb'
- 'spec/rails_helper.rb'
- 'spec/support/**/*.rb'
- 'spec/dor/update_marc_record_service_spec.rb'

# Offense count: 31
# Offense count: 30
# Configuration parameters: Prefixes.
# Prefixes: when, with, without
RSpec/ContextWording:
Exclude:
- 'spec/controllers/objects_controller_spec.rb'
- 'spec/controllers/sdr_controller_spec.rb'
- 'spec/dor/update_marc_record_service_spec.rb'
- 'spec/services/cleanup_reset_service_spec.rb'
Expand Down Expand Up @@ -166,12 +151,11 @@ RSpec/InstanceVariable:
- 'spec/services/cleanup_reset_service_spec.rb'
- 'spec/services/registration_service_spec.rb'

# Offense count: 41
# Offense count: 36
# Configuration parameters: EnforcedStyle.
# SupportedStyles: have_received, receive
RSpec/MessageSpies:
Exclude:
- 'spec/controllers/objects_controller_spec.rb'
- 'spec/dor/update_marc_record_service_spec.rb'
- 'spec/services/public_desc_metadata_service_spec.rb'
- 'spec/services/registration_service_spec.rb'
Expand All @@ -183,7 +167,7 @@ RSpec/NamedSubject:
Exclude:
- 'spec/services/thumbnail_service_spec.rb'

# Offense count: 22
# Offense count: 26
RSpec/NestedGroups:
Max: 5

Expand All @@ -201,10 +185,9 @@ RSpec/ScatteredLet:
- 'spec/services/public_desc_metadata_service_spec.rb'
- 'spec/services/public_xml_service_spec.rb'

# Offense count: 10
# Offense count: 8
RSpec/ScatteredSetup:
Exclude:
- 'spec/controllers/objects_controller_spec.rb'
- 'spec/controllers/sdr_controller_spec.rb'
- 'spec/controllers/versions_controller_spec.rb'
- 'spec/services/public_desc_metadata_service_spec.rb'
Expand Down
6 changes: 5 additions & 1 deletion app/services/constituent_service.rb
Expand Up @@ -28,7 +28,7 @@ def add(child_druids:)
# Make sure the parent is open before making modifications
VersionService.open(parent) unless VersionService.open?(parent)

ResetContentMetadataService.new(item: parent).reset
reset_metadata!

child_druids.each do |child_druid|
add_constituent(child_druid: child_druid)
Expand Down Expand Up @@ -60,6 +60,10 @@ def add_constituent(child_druid:)
VersionService.close(child, description: VERSION_CLOSE_DESCRIPTION, significance: VERSION_CLOSE_SIGNIFICANCE)
end

def reset_metadata!
ResetContentMetadataService.new(item: parent).reset
end

def parent
@parent ||= ItemQueryService.find_combinable_item(parent_druid)
end
Expand Down
28 changes: 23 additions & 5 deletions app/services/reset_content_metadata_service.rb
@@ -1,15 +1,33 @@
# frozen_string_literal: true

# Clears the contentMetadata datastream to the default, wiping out any members.
# item MUST allow modification, or save! will raise error
# Clears the contentMetadata datastream to the default, wiping out any members,
# and severing relationships bidirectionally.
#
# NOTE: item MUST allow modification, or `save!` will raise error
class ResetContentMetadataService
def initialize(item:, type: 'image')
DEFAULT_ITEM_TYPE = 'image'

attr_reader :item, :type

def initialize(item:, type: DEFAULT_ITEM_TYPE)
@item = item
@type = type
end

def reset
@item.contentMetadata.content = "<contentMetadata objectId='#{@item.id}' type='#{@type}'/>"
@item.save!
disown_current_children!

item.contentMetadata.content = "<contentMetadata objectId='#{item.id}' type='#{type}'/>"
item.save!
end

private

def disown_current_children!
item.contentMetadata.ng_xml.xpath('//resource/relationship/@objectId').map(&:content).each do |child_id|
child = Dor::Item.find(child_id)
child.clear_relationship(:is_constituent_of)
child.save! if child.relationships_are_dirty?
end
end
end
15 changes: 4 additions & 11 deletions spec/services/constituent_service_spec.rb
Expand Up @@ -64,6 +64,9 @@
allow(ItemQueryService).to receive(:find_combinable_item).with('druid:parent1').and_return(parent)
allow(ItemQueryService).to receive(:find_combinable_item).with('druid:child1').and_return(child1)
allow(ItemQueryService).to receive(:find_combinable_item).with('druid:child2').and_return(child2)

# Stub `#reset_metadata!` because ResetContentMetadataService is tested in its own spec
allow(instance).to receive(:reset_metadata!)
end

context 'when the parent is open for modification' do
Expand All @@ -77,17 +80,7 @@
end

it 'merges objects' do
expect(parent.contentMetadata.content).to be_equivalent_to <<~XML
<contentMetadata objectId="druid:parent1" type="image">
<resource sequence="1" id="#{namespaceless}_1" type="image">
<relationship type="alsoAvailableAs" objectId="#{child1.id}"/>
</resource>
<resource sequence="2" id="#{namespaceless}_2" type="image">
<externalFile objectId="druid:child2" resourceId="bb000ff1111_1" fileId="bb000ff1111.jpg" mimetype="image/jpeg"/>
<relationship type="alsoAvailableAs" objectId="#{child2.id}"/>
</resource>
</contentMetadata>
XML
expect(instance).to have_received(:reset_metadata!).once
expect(child1).to have_received(:clear_relationship).once
expect(child2).to have_received(:clear_relationship).once
expect(child1.object_relations[:is_constituent_of]).to eq [parent]
Expand Down
109 changes: 109 additions & 0 deletions spec/services/reset_content_metadata_service_spec.rb
@@ -0,0 +1,109 @@
# frozen_string_literal: true

require 'rails_helper'

RSpec.describe ResetContentMetadataService do
subject(:service) { described_class.new(args) }

let(:args) { { item: parent } }
let(:child1) do
instance_double(Dor::Item,
id: 'druid:child1',
relationships_are_dirty?: true,
save!: true,
clear_relationship: nil)
end
let(:child2) do
instance_double(Dor::Item,
id: 'druid:child2',
relationships_are_dirty?: true,
save!: true,
clear_relationship: nil)
end
let(:content_metadata) do
instance_double(Dor::ContentMetadataDS,
ng_xml: xml,
'content=': nil)
end
let(:default_content_xml) { "<contentMetadata objectId='druid:parent' type='image'/>" }
let(:parent) do
instance_double(Dor::Item,
id: 'druid:parent',
contentMetadata: content_metadata,
save!: true)
end
let(:predicate) { :is_constituent_of }

describe '.new' do
let(:xml) { '<notUsed/>' }

context 'when only the `item:` argument is included' do
it 'uses the supplied `item` attribute' do
expect(service.item).to eq(parent)
end

it 'uses the default `type` attribute' do
expect(service.type).to eq(described_class::DEFAULT_ITEM_TYPE)
end
end

context 'when `item:` and `type:` arguments are included' do
let(:args) { { item: parent, type: custom_type } }
let(:custom_type) { 'book' }

it 'uses the supplied `type` attribute' do
expect(service.type).to eq(custom_type)
end
end
end

describe '#reset' do
context 'without child relationships recorded in content metadata' do
let(:xml) { Nokogiri::XML(default_content_xml) }

before do
allow(Dor::Item).to receive(:find)
end

it 'resets content metadata without touching children' do
service.reset
expect(content_metadata).to have_received(:content=).with(default_content_xml).once
expect(parent).to have_received(:save!).once
expect(Dor::Item).not_to have_received(:find)
end
end

context 'with child relationships recorded in content metadata' do
let(:xml) do
Nokogiri::XML(
<<~XML
<contentMetadata objectId="druid:parent1" type="image">
<resource sequence="1" id="druid:child1_1" type="image">
<relationship type="alsoAvailableAs" objectId="druid:child1"/>
</resource>
<resource sequence="2" id="druid:child2_2" type="image">
<relationship type="alsoAvailableAs" objectId="druid:child2"/>
</resource>
</contentMetadata>
XML
)
end

before do
allow(Dor::Item).to receive(:find).with(child1.id).and_return(child1)
allow(Dor::Item).to receive(:find).with(child2.id).and_return(child2)
end

it 'severs the child relationships and resets content metadata' do
service.reset
expect(content_metadata).to have_received(:content=).with(default_content_xml).once
expect(parent).to have_received(:save!).once
expect(Dor::Item).to have_received(:find).exactly(2).times
expect(child1).to have_received(:clear_relationship).with(predicate).once
expect(child2).to have_received(:clear_relationship).with(predicate).once
expect(child1).to have_received(:save!)
expect(child2).to have_received(:save!)
end
end
end
end

0 comments on commit 30a9708

Please sign in to comment.