From 1ebc843825dda109f9d5f79364bb4355f5056d78 Mon Sep 17 00:00:00 2001 From: Jane Sandberg Date: Mon, 14 Mar 2022 11:06:06 -0700 Subject: [PATCH 1/2] BookmarksController#create should not return success on invalid input Closes #2665 --- app/controllers/concerns/blacklight/bookmarks.rb | 2 +- spec/controllers/bookmarks_controller_spec.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/controllers/concerns/blacklight/bookmarks.rb b/app/controllers/concerns/blacklight/bookmarks.rb index e36ec6798d..9a56a84137 100644 --- a/app/controllers/concerns/blacklight/bookmarks.rb +++ b/app/controllers/concerns/blacklight/bookmarks.rb @@ -76,7 +76,7 @@ def create current_or_guest_user.save! unless current_or_guest_user.persisted? success = @bookmarks.all? do |bookmark| - current_or_guest_user.bookmarks.where(bookmark).exists? || current_or_guest_user.bookmarks.create(bookmark) + current_or_guest_user.bookmarks.where(bookmark).exists? || current_or_guest_user.bookmarks.create(bookmark).valid? end if request.xhr? diff --git a/spec/controllers/bookmarks_controller_spec.rb b/spec/controllers/bookmarks_controller_spec.rb index 283a6f5362..39ef1bc451 100644 --- a/spec/controllers/bookmarks_controller_spec.rb +++ b/spec/controllers/bookmarks_controller_spec.rb @@ -24,7 +24,7 @@ allow(@controller).to receive_message_chain(:current_or_guest_user, :existing_bookmark_for).and_return(false) allow(@controller).to receive_message_chain(:current_or_guest_user, :persisted?).and_return(true) allow(@controller).to receive_message_chain(:current_or_guest_user, :bookmarks, :where, :exists?).and_return(false) - allow(@controller).to receive_message_chain(:current_or_guest_user, :bookmarks, :create).and_return(false) + allow(@controller).to receive_message_chain(:current_or_guest_user, :bookmarks, :create, :valid?).and_return(false) put :update, xhr: true, params: { id: 'iamabooboo', format: :js } expect(response.code).to eq "500" end From f7d6964520e78a10978f2bfc929eb6dfe65a68e8 Mon Sep 17 00:00:00 2001 From: Jane Sandberg Date: Thu, 17 Mar 2022 12:56:35 -0700 Subject: [PATCH 2/2] Wrap bookmark batch creation in a transaction --- app/controllers/concerns/blacklight/bookmarks.rb | 9 ++++++--- spec/controllers/bookmarks_controller_spec.rb | 3 ++- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/controllers/concerns/blacklight/bookmarks.rb b/app/controllers/concerns/blacklight/bookmarks.rb index 9a56a84137..ff8965f604 100644 --- a/app/controllers/concerns/blacklight/bookmarks.rb +++ b/app/controllers/concerns/blacklight/bookmarks.rb @@ -75,12 +75,15 @@ def create current_or_guest_user.save! unless current_or_guest_user.persisted? - success = @bookmarks.all? do |bookmark| - current_or_guest_user.bookmarks.where(bookmark).exists? || current_or_guest_user.bookmarks.create(bookmark).valid? + bookmarks_to_add = @bookmarks.reject { |bookmark| current_or_guest_user.bookmarks.where(bookmark).exists? } + success = ActiveRecord::Base.transaction do + current_or_guest_user.bookmarks.create!(bookmarks_to_add) + rescue ActiveRecord::RecordInvalid + false end if request.xhr? - success ? render(json: { bookmarks: { count: current_or_guest_user.bookmarks.count } }) : render(plain: "", status: "500") + success ? render(json: { bookmarks: { count: current_or_guest_user.bookmarks.count } }) : render(json: current_or_guest_user.errors.full_messages, status: "500") else if @bookmarks.any? && success flash[:notice] = I18n.t('blacklight.bookmarks.add.success', count: @bookmarks.length) diff --git a/spec/controllers/bookmarks_controller_spec.rb b/spec/controllers/bookmarks_controller_spec.rb index 39ef1bc451..05df6104ca 100644 --- a/spec/controllers/bookmarks_controller_spec.rb +++ b/spec/controllers/bookmarks_controller_spec.rb @@ -24,7 +24,8 @@ allow(@controller).to receive_message_chain(:current_or_guest_user, :existing_bookmark_for).and_return(false) allow(@controller).to receive_message_chain(:current_or_guest_user, :persisted?).and_return(true) allow(@controller).to receive_message_chain(:current_or_guest_user, :bookmarks, :where, :exists?).and_return(false) - allow(@controller).to receive_message_chain(:current_or_guest_user, :bookmarks, :create, :valid?).and_return(false) + allow(@controller).to receive_message_chain(:current_or_guest_user, :bookmarks, :create!).and_raise(ActiveRecord::RecordInvalid) + allow(@controller).to receive_message_chain(:current_or_guest_user, :errors, :full_messages).and_return([1]) put :update, xhr: true, params: { id: 'iamabooboo', format: :js } expect(response.code).to eq "500" end