Merge pull request #2938 from samvera/work_err_msg

improve single-membership collection error message

app/controllers/concerns/hyrax/works_controller_behavior.rb

@@ -56,7 +56,6 @@ def create
       else
         respond_to do |wants|
           wants.html do
-            flash[:error] = curation_concern.errors.messages[:collections].to_sentence
             build_form
             render 'new', status: :unprocessable_entity
           end
@@ -101,7 +100,6 @@ def update
       else
         respond_to do |wants|
           wants.html do
-            curation_concern.errors[:single_collection] << curation_concern.errors.messages[:collections].to_sentence
             build_form
             render 'edit', status: :unprocessable_entity
           end

app/services/hyrax/multiple_membership_checker.rb

@@ -60,9 +60,11 @@ def single_membership_types
 
       def build_error_message(problematic_collections)
         error_message_clauses = problematic_collections.map do |gid, list|
-          "#{collection_type_title_from_gid(gid)} (#{collection_titles_from_list(list)})"
+          I18n.t('hyrax.admin.collection_types.multiple_membership_checker.error_type_and_collections',
+                 type: collection_type_title_from_gid(gid),
+                 collections: collection_titles_from_list(list))
         end
-        "#{I18n.t('hyrax.admin.collection_types.multiple_membership_checker.error_preamble')}: #{error_message_clauses.join('; ')}"
+        "#{I18n.t('hyrax.admin.collection_types.multiple_membership_checker.error_preamble')}#{error_message_clauses.join('; ')}"
       end
 
       def collection_type_title_from_gid(gid)

app/views/hyrax/base/_form.html.erb

@@ -10,6 +10,7 @@
       <%= f.object.errors.full_messages_for(:base).send(SimpleForm.error_method) %>
       <%= render 'form_in_works_error', f: f %>
       <%= render 'form_ordered_members_error', f: f %>
+      <%= render 'form_collections_error', f: f %>
       <%= render 'form_visibility_error', f: f %>
     </div>
   <% end %>

app/views/hyrax/base/_form_collections_error.html.erb

@@ -0,0 +1 @@
+<%= f.full_error(:collections) %>

app/views/hyrax/base/_form_visibility_error.html.erb

@@ -1,4 +1,3 @@
 <%= f.full_error(:visibility) %>
 <%= f.full_error(:embargo_release_date) %>
 <%= f.full_error(:visibility_after_embargo) %>
-<%= f.full_error(:single_collection) %>

config/locales/hyrax.en.yml

@@ -239,7 +239,8 @@ en:
             actions: Actions
             name: Name
         multiple_membership_checker:
-          error_preamble: 'Error: You have specified more than one of the same single-membership collection types'
+          error_preamble: 'Error: You have specified more than one of the same single-membership collection type '
+          error_type_and_collections: '(type: %{type}, collections: %{collections})'
         new:
           header: Create New Collection Type
           submit: Save

spec/controllers/hyrax/generic_works_controller_spec.rb

@@ -262,20 +262,9 @@
     context 'when create fails' do
       let(:work) { create(:work) }
       let(:create_status) { false }
-      let(:errors) { double }
-      let(:messages) { double }
-      let(:error_messages) { ['Error: foo bar'] }
-
-      before do
-        allow(controller).to receive(:curation_concern).and_return(work)
-        allow(work).to receive(:errors).and_return(errors)
-        allow(errors).to receive(:messages).and_return(messages)
-        allow(messages).to receive(:[]).with(:collections).and_return(error_messages)
-      end
 
       it 'draws the form again' do
         post :create, params: { generic_work: { title: ['a title'] } }
-        expect(flash[:error]).to eq error_messages.to_sentence
         expect(response.status).to eq 422
         expect(assigns[:form]).to be_kind_of Hyrax::GenericWorkForm
         expect(response).to render_template 'new'
@@ -495,22 +484,9 @@
 
       describe 'update failed' do
         let(:actor) { double(update: false) }
-        let(:work) { create(:work) }
-        let(:errors) { double }
-        let(:messages) { double }
-        let(:error_messages) { ['Error: foo bar'] }
-
-        before do
-          allow(controller).to receive(:curation_concern).and_return(work)
-          allow(work).to receive(:errors).and_return(errors)
-          allow(errors).to receive(:messages).and_return(messages)
-          allow(messages).to receive(:[]).with(:collections).and_return(error_messages)
-          allow(errors).to receive(:[]).with(:single_collection).and_return(error_messages)
-        end
 
         it 'renders the form' do
           patch :update, params: { id: work, generic_work: {} }
-          expect(work.errors[:single_collection].to_sentence).to eq error_messages.to_sentence
           expect(assigns[:form]).to be_kind_of Hyrax::GenericWorkForm
           expect(response).to render_template('edit')
         end

spec/features/collection_multi_membership_spec.rb

@@ -107,8 +107,8 @@
             expect(page).to have_link 'Your Collections'
           end
 
-          err_message = "Error: You have specified more than one of the same single-membership collection types: " \
-                        "Single-membership 1 (#{new_collection.title.first} and #{old_collection.title.first})"
+          err_message = "Error: You have specified more than one of the same single-membership collection type " \
+                        "(type: Single-membership 1, collections: #{new_collection.title.first} and #{old_collection.title.first})"
           expect(page).to have_selector '.alert', text: err_message
         end
 
@@ -129,8 +129,8 @@
             element.click
           end
 
-          err_message = "Single collection Error: You have specified more than one of the same single-membership collection types: " \
-                        "Single-membership 1 (#{old_collection.title.first} and #{new_collection.title.first})"
+          err_message = "Error: You have specified more than one of the same single-membership collection type " \
+                        "(type: Single-membership 1, collections: #{old_collection.title.first} and #{new_collection.title.first})"
           expect(page).to have_selector '.help-block', text: err_message
         end
 
@@ -150,8 +150,8 @@
             expect(page).to have_link 'Your Collections'
           end
 
-          err_message = "Error: You have specified more than one of the same single-membership collection types: " \
-                        "Single-membership 1 (#{new_collection.title.first} and #{old_collection.title.first})"
+          err_message = "Error: You have specified more than one of the same single-membership collection type " \
+                        "(type: Single-membership 1, collections: #{new_collection.title.first} and #{old_collection.title.first})"
           expect(page).to have_selector '.alert', text: err_message
         end
       end

spec/services/hyrax/multiple_membership_checker_spec.rb

@@ -73,7 +73,7 @@
         expect(item).not_to receive(:member_of_collection_ids)
         expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_call_original
         expect(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: [collection_type.gid]).once.and_return(collections)
-        expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection types: Greedy (Foo and Bar)'
+        expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection type (type: Greedy, collections: Foo and Bar)'
       end
 
       context 'with multiple single membership collection types' do
@@ -84,7 +84,7 @@
           expect(item).not_to receive(:member_of_collection_ids)
           expect(checker).to receive(:single_membership_collections).with(collection_ids).once.and_call_original
           expect(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: collection_types).once.and_return(collections)
-          expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection types: Greedy (Foo and Bar)'
+          expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection type (type: Greedy, collections: Foo and Bar)'
         end
       end
     end
@@ -104,7 +104,7 @@
         expect(item).to receive(:member_of_collection_ids)
         expect(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: [collection_type.gid]).once.and_return(collections)
         expect(Collection).to receive(:where).with(id: [collection2.id], collection_type_gid_ssim: [collection_type.gid]).once.and_return([collection2])
-        expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection types: Greedy (Foo and Bar)'
+        expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection type (type: Greedy, collections: Foo and Bar)'
       end
 
       context 'with multiple single membership collection types' do
@@ -115,7 +115,7 @@
           expect(item).to receive(:member_of_collection_ids)
           expect(Collection).to receive(:where).with(id: collection_ids, collection_type_gid_ssim: collection_types).once.and_return(collections)
           expect(Collection).to receive(:where).with(id: [collection2.id], collection_type_gid_ssim: collection_types).once.and_return([collection2])
-          expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection types: Greedy (Foo and Bar)'
+          expect(subject).to eq 'Error: You have specified more than one of the same single-membership collection type (type: Greedy, collections: Foo and Bar)'
         end
       end
     end