Merge 8385c37 into e34a6e0

app/controllers/versions_controller.rb

@@ -47,23 +47,18 @@ def build_error(msg, err)
   def open_params
     params.permit(
       :assume_accessioned,
-      vers_md_upd_info: [
-        :description,
-        :opening_user_name,
-        :significance
-      ]
-    ).to_h.deep_symbolize_keys
+      :description,
+      :opening_user_name,
+      :significance
+    ).to_h.symbolize_keys
   end
 
   def close_params
-    symbolized_hash = params.permit(
+    params.permit(
       :description,
       :significance,
       :start_accession,
       :version_num
     ).to_h.symbolize_keys
-    # Downstream code expects the significance value to be a symbol
-    symbolized_hash[:significance] = symbolized_hash[:significance].to_sym if symbolized_hash.key?(:significance)
-    symbolized_hash
   end
 end

app/services/version_service.rb

@@ -21,7 +21,9 @@ def initialize(work)
   # Increments the version number and initializes versioningWF for the object
   # @param [Hash] opts optional params
   # @option opts [Boolean] :assume_accessioned If true, does not check whether object has been accessioned.
-  # @option opts [Hash] :vers_md_upd_info If present, used to add to the events datastream and set the desc and significance on the versionMetadata datastream
+  # @option opts [String] :significance set significance (major/minor/patch) of version change
+  # @option opts [String] :description set description of version change
+  # @option opts [String] :opening_user_name add opening username to the events datastream
   # @raise [Dor::Exception] if the object hasn't been accessioned, or if a version is already opened
   def open(opts = {})
     sdr_version = try_to_get_current_version(opts[:assume_accessioned])
@@ -32,12 +34,13 @@ def open(opts = {})
 
     Dor::Config.workflow.client.create_workflow_by_name(work.pid, 'versioningWF')
 
-    vmd_upd_info = opts[:vers_md_upd_info]
-    return unless vmd_upd_info
+    return if (opts.keys & open_options_requiring_work_save).empty?
 
-    work.events.add_event('open', vmd_upd_info[:opening_user_name], "Version #{vmd_ds.current_version_id} opened")
-    vmd_ds.update_current_version(description: vmd_upd_info[:description], significance: vmd_upd_info[:significance].to_sym)
-    work.save
+    work.events.add_event('open', opts[:opening_user_name], "Version #{vmd_ds.current_version_id} opened") if opts[:opening_user_name]
+
+    vmd_ds.update_current_version(description: opts[:description], significance: opts[:significance].to_sym) if opts[:description] && opts[:significance]
+
+    work.save!
   end
 
   # Determines whether a new version can be opened for an object.
@@ -117,4 +120,10 @@ def accessioning?
   end
 
   attr_reader :work
+
+  private
+
+  def open_options_requiring_work_save
+    [:opening_user_name, :significance, :description]
+  end
 end

spec/controllers/versions_controller_spec.rb

@@ -35,7 +35,7 @@
       it 'forwards optional params to the VersionService#close method' do
         post :close_current, params: { object_id: item.pid }, body: %( {"description": "some text", "significance": "major"} ), as: :json
         expect(response.body).to match(/version 1 closed/)
-        expect(VersionService).to have_received(:close).with(item, description: 'some text', significance: :major)
+        expect(VersionService).to have_received(:close).with(item, description: 'some text', significance: 'major')
       end
     end
 
@@ -63,11 +63,9 @@
     let(:open_params) do
       {
         assume_accessioned: false,
-        vers_md_upd_info: {
-          significance: 'minor',
-          description: 'bar',
-          opening_user_name: opening_user_name
-        }
+        significance: 'minor',
+        description: 'bar',
+        opening_user_name: opening_user_name
       }
     end
     let(:opening_user_name) { 'foo' }

spec/services/version_service_spec.rb

@@ -48,23 +48,23 @@
         expect(workflow_client).to have_received(:create_workflow_by_name).with(obj.pid, 'versioningWF')
       end
 
-      it 'includes vers_md_upd_info' do
-        vers_md_upd_info = { significance: 'real_major', description: 'same as it ever was', opening_user_name: 'sunetid' }
+      it 'includes options' do
+        options = { significance: 'real_major', description: 'same as it ever was', opening_user_name: 'sunetid' }
         cur_vers = '2'
         allow(vmd_ds).to receive(:current_version).and_return(cur_vers)
-        allow(obj).to receive(:save)
+        allow(obj).to receive(:save!)
 
-        expect(ev_ds).to receive(:add_event).with('open', vers_md_upd_info[:opening_user_name], "Version #{cur_vers} opened")
-        expect(vmd_ds).to receive(:update_current_version).with(description: vers_md_upd_info[:description], significance: vers_md_upd_info[:significance].to_sym)
-        expect(obj).to receive(:save)
+        expect(ev_ds).to receive(:add_event).with('open', options[:opening_user_name], "Version #{cur_vers} opened")
+        expect(vmd_ds).to receive(:update_current_version).with(description: options[:description], significance: options[:significance].to_sym)
+        expect(obj).to receive(:save!)
 
-        described_class.open(obj, vers_md_upd_info: vers_md_upd_info)
+        described_class.open(obj, **options)
       end
 
-      it "doesn't include vers_md_upd_info" do
+      it "doesn't include options" do
         expect(ev_ds).not_to receive(:add_event)
         expect(vmd_ds).not_to receive(:update_current_version)
-        expect(obj).not_to receive(:save)
+        expect(obj).not_to receive(:save!)
 
         open
       end