Treat blank item parameters as nil

Author: dmolesUC

app/controllers/items_controller.rb

@@ -67,7 +67,7 @@ def set_item
 
   # Only allow a list of trusted parameters through.
   def item_params
-    @item_params ||= jsonapi_deserialize(params, only: Item::EDIT_ATTRS)
+    @item_params ||= jsonapi_deserialize(params, only: Item::EDIT_ATTRS).transform_values(&:presence)
   end
 
   def ensure_image_param!

app/models/item.rb

@@ -26,6 +26,7 @@ class Item < ApplicationRecord
   validate :multiple_terms_allowed
   validate :unsuppressed_item_has_image
   validate :unsuppressed_item_has_mms_id
+  validate :non_nil_mms_id_is_not_blank
 
   def multiple_terms_allowed
     terms_by_facet = terms.group_by(&:facet)
@@ -49,6 +50,12 @@ def unsuppressed_item_has_mms_id
     errors.add(:mms_id, 'unsuppressed item must have an MMS ID')
   end
 
+  def non_nil_mms_id_is_not_blank
+    return if mms_id.nil? || mms_id.present?
+
+    errors.add(:mms_id, 'MMS ID, if present, cannot be blank')
+  end
+
   # ------------------------------------------------------------
   # Scopes
 

spec/models/item_spec.rb

@@ -197,6 +197,50 @@
       expect(item.suppressed).to eq(true)
       expect(item.mms_id).to be_nil
     end
+
+    it 'does not allow an empty mms_id even for suppressed items' do
+      item = Item.take
+      mms_id_before = item.mms_id
+      updated_at_before = item.updated_at
+
+      item.suppressed = true
+      item.mms_id = ''
+      expect(item).not_to be_valid
+
+      expect { item.save! }.to raise_error(ActiveRecord::RecordInvalid)
+      item.reload
+      expect(item.updated_at).to eq(updated_at_before)
+      expect(item.mms_id).to eq(mms_id_before)
+    end
+
+    it 'does not allow a blank, non-empty mms_id even for suppressed items' do
+      item = Item.take
+      mms_id_before = item.mms_id
+      updated_at_before = item.updated_at
+
+      item.suppressed = true
+      item.mms_id = ' '
+      expect(item).not_to be_valid
+
+      expect { item.save! }.to raise_error(ActiveRecord::RecordInvalid)
+      item.reload
+      expect(item.updated_at).to eq(updated_at_before)
+      expect(item.mms_id).to eq(mms_id_before)
+    end
+
+    it 'allows multiple suppressed items with nil mms_id' do
+      aggregate_failures do
+        (0..1).each do |i|
+          title = "item #{i}"
+          item = Item.create!(title: title, suppressed: true)
+          expect(item).to be_persisted
+          item.reload
+          expect(item.mms_id).to be_nil
+          expect(item.title).to eq(title)
+          expect(item.suppressed).to eq(true)
+        end
+      end
+    end
   end
 
   describe 'synthetic accessors' do

spec/requests/items_spec.rb

@@ -437,6 +437,30 @@ def expected_errors_for(attributes, terms: valid_terms, image: valid_image, item
             expect(response.headers['Location']).to eq(item_url(item))
           end
 
+          it 'treats a blank MMS ID as nil' do
+            post_attributes = valid_attributes.except(:mms_id).merge(suppressed: true, mms_id: ' ')
+
+            payload = { data: { type: 'item', attributes: post_attributes, relationships: to_relationships(image: valid_image) } }
+            expect { post items_url, params: payload, as: :jsonapi }.to change(Item, :count).by(1)
+
+            expect(response).to have_http_status(:created)
+            expect(response.content_type).to start_with(JSONAPI::MEDIA_TYPE)
+
+            parsed_response = JSON.parse(response.body)
+            item_id = parsed_response['data']['id'].to_i
+
+            item = Item.find(item_id)
+            expect(item).not_to be_nil
+            expected_attributes = post_attributes.merge(mms_id: nil)
+            expected_attributes.each { |attr, val| expect(item.send(attr)).to eq(val) }
+
+            links = parsed_response.delete('links')
+            expect(links['self']).to eq(items_url)
+
+            expect(parsed_response).to contain_jsonapi_for(item)
+            expect(response.headers['Location']).to eq(item_url(item))
+          end
+
           it 'accepts a suppressed item without an image' do
             payload = {
               data: {
@@ -487,6 +511,24 @@ def expected_errors_for(attributes, terms: valid_terms, image: valid_image, item
             expect(actual_errors).to contain_exactly(*expected_json)
           end
 
+          it 'treats a blank title as missing' do
+            post_attributes = valid_attributes.merge(title: ' ')
+            payload = { data: { type: 'item', attributes: post_attributes, relationships: valid_relationships } }
+
+            allow(Rails.logger).to receive(:error)
+            expect { post items_url, params: payload, as: :jsonapi }.not_to change(Item, :count)
+            expect(Rails.logger).to have_received(:error).with(kind_of(ActiveRecord::RecordInvalid))
+
+            expect(response).to have_http_status(:unprocessable_entity)
+            expect(response.content_type).to start_with(JSONAPI::MEDIA_TYPE)
+
+            actual_errors = JSON.parse(response.body)['errors']
+            invalid_attributes = post_attributes.merge(title: nil)
+            expected_errors = expected_errors_for(invalid_attributes)
+            expected_json = expected_errors.map { |err| jsonapi_for(err) }
+            expect(actual_errors).to contain_exactly(*expected_json)
+          end
+
           it 'fails with 422 Unprocessable Entity for a missing image' do
             payload = {
               data: {
@@ -542,6 +584,24 @@ def expected_errors_for(attributes, terms: valid_terms, image: valid_image, item
             expect(actual_errors).to contain_exactly(*expected_json)
           end
 
+          it 'treats a blank MMS ID as missing' do
+            post_attributes = valid_attributes.merge(mms_id: ' ')
+            payload = { data: { type: 'item', attributes: post_attributes, relationships: valid_relationships } }
+
+            allow(Rails.logger).to receive(:error)
+            expect { post items_url, params: payload, as: :jsonapi }.not_to change(Item, :count)
+            expect(Rails.logger).to have_received(:error).with(kind_of(ActiveRecord::RecordInvalid))
+
+            expect(response).to have_http_status(:unprocessable_entity)
+            expect(response.content_type).to start_with(JSONAPI::MEDIA_TYPE)
+
+            actual_errors = JSON.parse(response.body)['errors']
+            invalid_attributes = post_attributes.merge(mms_id: nil)
+            expected_errors = expected_errors_for(invalid_attributes)
+            expected_json = expected_errors.map { |err| jsonapi_for(err) }
+            expect(actual_errors).to contain_exactly(*expected_json)
+          end
+
           it 'fails with 404 Not Found for an invalid term' do
             invalid_id = Term.maximum(:id) + 999
             payload = {
@@ -741,6 +801,38 @@ def expected_errors_for(attributes, terms: valid_terms, image: valid_image, item
 
             expect(parsed_response).to contain_jsonapi_for(item)
           end
+
+          it 'treats a blank MMS ID as nil' do
+            item = Item.take
+
+            old_attributes = item.attributes.slice(*Item::EDIT_ATTRS)
+            patch_attributes = old_attributes.merge(suppressed: true, mms_id: '')
+            expected_terms = item.terms.to_a
+
+            payload = { data: { type: 'item', id: item.id.to_s, attributes: patch_attributes,
+                                relationships: to_relationships(terms: expected_terms, image: item.image) } }
+            patch item_url(item), params: payload, as: :jsonapi
+
+            expect(response).to have_http_status(:ok)
+            expect(response.content_type).to start_with(JSONAPI::MEDIA_TYPE)
+
+            item.reload
+
+            expected_attributes = patch_attributes.merge(mms_id: nil)
+            expected_attributes.each do |attr, val|
+              actual = item.send(attr)
+              expect(actual).to eq(val), "Wrong value for #{attr}; expected #{val.inspect}, was #{actual.inspect}"
+            end
+
+            expect(item.terms).to contain_exactly(*expected_terms)
+
+            parsed_response = JSON.parse(response.body)
+
+            links = parsed_response.delete('links')
+            expect(links['self']).to eq(item_url(item))
+
+            expect(parsed_response).to contain_jsonapi_for(item)
+          end
         end
 
         describe 'failure' do