diff --git a/api/app/controllers/spree/api/base_controller.rb b/api/app/controllers/spree/api/base_controller.rb index 99c3fea08d5..1678f3c99f8 100644 --- a/api/app/controllers/spree/api/base_controller.rb +++ b/api/app/controllers/spree/api/base_controller.rb @@ -16,14 +16,12 @@ class BaseController < ActionController::Base attr_accessor :current_api_user - class_attribute :error_notifier - before_action :load_user before_action :authorize_for_order, if: proc { order_token.present? } before_action :authenticate_user before_action :load_user_roles - rescue_from StandardError, with: :error_during_processing + rescue_from ActionController::ParameterMissing, with: :parameter_missing_error rescue_from ActiveRecord::RecordNotFound, with: :not_found rescue_from CanCan::AccessDenied, with: :unauthorized rescue_from Spree::Core::GatewayError, with: :gateway_error @@ -67,20 +65,19 @@ def unauthorized render "spree/api/errors/unauthorized", status: 401 end - def error_during_processing(exception) - Rails.logger.error exception.message - Rails.logger.error exception.backtrace.join("\n") - - error_notifier.call(exception, self) if error_notifier - - render json: { exception: exception.message }, status: 422 - end - def gateway_error(exception) @order.errors.add(:base, exception.message) invalid_resource!(@order) end + def parameter_missing_error(exception) + render json: { + exception: exception.message, + error: exception.message, + missing_param: exception.param + }, status: 422 + end + def requires_authentication? Spree::Api::Config[:requires_authentication] end diff --git a/api/app/controllers/spree/api/checkouts_controller.rb b/api/app/controllers/spree/api/checkouts_controller.rb index 800012c96d3..2048fb20ff9 100644 --- a/api/app/controllers/spree/api/checkouts_controller.rb +++ b/api/app/controllers/spree/api/checkouts_controller.rb @@ -115,11 +115,12 @@ def state_callback(before_or_after = :before) def after_update_attributes if params[:order] && params[:order][:coupon_code].present? - handler = PromotionHandler::Coupon.new(@order).apply + handler = PromotionHandler::Coupon.new(@order) + handler.apply if handler.error.present? @coupon_message = handler.error - respond_with(@order, default_template: 'spree/api/orders/could_not_apply_coupon') + respond_with(@order, default_template: 'spree/api/orders/could_not_apply_coupon', status: 422) return true end end diff --git a/api/app/controllers/spree/api/resource_controller.rb b/api/app/controllers/spree/api/resource_controller.rb index 34475e90fc7..7505fbd2028 100644 --- a/api/app/controllers/spree/api/resource_controller.rb +++ b/api/app/controllers/spree/api/resource_controller.rb @@ -56,6 +56,8 @@ def destroy else invalid_resource!(@object) end + rescue ActiveRecord::DeleteRestrictionError + render "spree/api/errors/delete_restriction", status: 422 end protected diff --git a/api/app/controllers/spree/api/stock_items_controller.rb b/api/app/controllers/spree/api/stock_items_controller.rb index 7047a8d6a9c..76d3d7ce35f 100644 --- a/api/app/controllers/spree/api/stock_items_controller.rb +++ b/api/app/controllers/spree/api/stock_items_controller.rb @@ -3,6 +3,8 @@ module Api class StockItemsController < Spree::Api::BaseController before_action :load_stock_location, only: [:index, :show, :create] + rescue_from StockLocation::InvalidMovementError, with: :render_stock_items_error + def index @stock_items = paginate(scope.ransack(params[:q]).result) respond_with(@stock_items) @@ -78,6 +80,10 @@ def adjust_stock_item_count_on_hand(count_on_hand_adjustment) @stock_movement = @stock_location.move(@stock_item.variant, count_on_hand_adjustment, current_api_user) @stock_item = @stock_movement.stock_item end + + def render_stock_items_error + render json: { error: Spree.t(:stock_not_below_zero) }, status: 422 + end end end end diff --git a/api/app/views/spree/api/errors/delete_restriction.v1.rabl b/api/app/views/spree/api/errors/delete_restriction.v1.rabl new file mode 100644 index 00000000000..804d8f49e0f --- /dev/null +++ b/api/app/views/spree/api/errors/delete_restriction.v1.rabl @@ -0,0 +1,2 @@ +object false +node(:error) { I18n.t(:delete_restriction_error, scope: "spree.api") } diff --git a/api/config/locales/en.yml b/api/config/locales/en.yml index 7971b2bc35b..1cfb79a8ae4 100644 --- a/api/config/locales/en.yml +++ b/api/config/locales/en.yml @@ -7,6 +7,7 @@ en: invalid_resource: "Invalid resource. Please fix errors and try again." resource_not_found: "The resource you were looking for could not be found." gateway_error: "There was a problem with the payment gateway: %{text}" + delete_restriction_error: "Cannot delete record." access: "API Access" key: "Key" clear_key: "Clear key" diff --git a/api/spec/controllers/spree/api/base_controller_spec.rb b/api/spec/controllers/spree/api/base_controller_spec.rb index ab85c7c70ec..f6ea2e8a89d 100644 --- a/api/spec/controllers/spree/api/base_controller_spec.rb +++ b/api/spec/controllers/spree/api/base_controller_spec.rb @@ -62,66 +62,10 @@ def index end end - it 'chatches StandardError' do - expect(subject).to receive(:authenticate_user).and_return(true) - expect(subject).to receive(:load_user_roles).and_return(true) - expect(subject).to receive(:index).and_raise("no joy") - get :index, params: { token: "fake_key" } - expect(json_response).to eq({ "exception" => "no joy" }) - expect(response.content_type).to eq("application/json") - end - - it 'raises Exception' do - expect(subject).to receive(:authenticate_user).and_return(true) - expect(subject).to receive(:load_user_roles).and_return(true) - expect(subject).to receive(:index).and_raise(Exception.new("no joy")) - expect { - get :index, params: { token: "fake_key" } - }.to raise_error(Exception, "no joy") - end - it "lets a subclass override the product associations that are eager-loaded" do expect(controller.respond_to?(:product_includes, true)).to be end - describe '#error_during_processing' do - controller(FakesController) do - # GET /foo - # Simulates a failed API call. - def foo - raise StandardError - end - end - - # What would be placed in config/initializers/spree.rb - Spree::Api::BaseController.error_notifier = proc do |e, controller| - MockHoneybadger.notify_or_ignore(e, rack_env: controller.request.env) - end - - ## - # Fake HB alert class - class MockHoneybadger - # https://github.com/honeybadger-io/honeybadger-ruby/blob/master/lib/honeybadger.rb#L136 - def self.notify_or_ignore(_exception, _opts = {}) - end - end - - before do - user = double(email: "spree@example.com") - allow(user).to receive_message_chain :spree_roles, pluck: [] - allow(Spree.user_class).to receive_messages find_by: user - @routes = ActionDispatch::Routing::RouteSet.new.tap do |r| - r.draw { get 'foo' => 'fakes#foo' } - end - end - - it 'should notify notify_error_during_processing' do - expect(MockHoneybadger).to receive(:notify_or_ignore).once.with(kind_of(Exception), rack_env: kind_of(Hash)) - get :foo, params: { token: 123 } - expect(response.status).to eq(422) - end - end - context 'insufficient stock' do before do expect(subject).to receive(:authenticate_user).and_return(true) diff --git a/api/spec/requests/spree/api/checkouts_controller_spec.rb b/api/spec/requests/spree/api/checkouts_controller_spec.rb index 5da814ceb91..56c2db90393 100644 --- a/api/spec/requests/spree/api/checkouts_controller_spec.rb +++ b/api/spec/requests/spree/api/checkouts_controller_spec.rb @@ -349,6 +349,14 @@ module Spree expect(PromotionHandler::Coupon).to receive(:new).with(order).and_call_original expect_any_instance_of(PromotionHandler::Coupon).to receive(:apply).and_return({ coupon_applied?: true }) put spree.api_checkout_path(order.to_param), params: { order_token: order.guest_token, order: { coupon_code: "foobar" } } + expect(response.status).to eq(200) + end + + it "renders error failing to apply coupon" do + order.update_column(:state, "payment") + put spree.api_checkout_path(order.to_param), params: { order_token: order.guest_token, order: { coupon_code: "foobar" } } + expect(response.status).to eq(422) + expect(json_response).to eq({ "error" => "The coupon code you entered doesn't exist. Please try again." }) end end diff --git a/api/spec/requests/spree/api/orders_controller_spec.rb b/api/spec/requests/spree/api/orders_controller_spec.rb index fc613a08f58..b64793ccc92 100644 --- a/api/spec/requests/spree/api/orders_controller_spec.rb +++ b/api/spec/requests/spree/api/orders_controller_spec.rb @@ -277,13 +277,6 @@ module Spree expect(json_response["checkout_steps"]).to eq(%w[address delivery confirm complete]) end - # Regression test for https://github.com/spree/spree/issues/1992 - it "can view an order not in a standard state" do - allow_any_instance_of(Order).to receive_messages user: current_api_user - order.update_column(:state, 'shipped') - get spree.api_order_path(order) - end - it "can not view someone else's order" do allow_any_instance_of(Order).to receive_messages user: stub_model(Spree::LegacyUser) get spree.api_order_path(order) diff --git a/api/spec/requests/spree/api/users_controller_spec.rb b/api/spec/requests/spree/api/users_controller_spec.rb index 46c51db7424..8b76ad024e3 100644 --- a/api/spec/requests/spree/api/users_controller_spec.rb +++ b/api/spec/requests/spree/api/users_controller_spec.rb @@ -142,8 +142,8 @@ module Spree it "cannot destroy user with orders" do create(:completed_order_with_totals, user: user) delete spree.api_user_path(user) - expect(json_response["exception"]).to eq "Spree::Core::DestroyWithOrdersError" expect(response.status).to eq(422) + expect(json_response).to eq({ "error" => "Cannot delete record." }) end end end diff --git a/backend/app/controllers/spree/admin/users_controller.rb b/backend/app/controllers/spree/admin/users_controller.rb index 5b16cdee9c3..f094cd4aebf 100644 --- a/backend/app/controllers/spree/admin/users_controller.rb +++ b/backend/app/controllers/spree/admin/users_controller.rb @@ -1,7 +1,7 @@ module Spree module Admin class UsersController < ResourceController - rescue_from Spree::Core::DestroyWithOrdersError, with: :user_destroy_with_orders_error + rescue_from ActiveRecord::DeleteRestrictionError, with: :user_destroy_with_orders_error after_action :sign_in_if_change_own_password, only: :update diff --git a/core/app/models/concerns/spree/user_methods.rb b/core/app/models/concerns/spree/user_methods.rb index e820b865f7a..fa362841487 100644 --- a/core/app/models/concerns/spree/user_methods.rb +++ b/core/app/models/concerns/spree/user_methods.rb @@ -17,7 +17,7 @@ module UserMethods has_many :stock_locations, through: :user_stock_locations has_many :spree_orders, foreign_key: "user_id", class_name: "Spree::Order" - has_many :orders, foreign_key: "user_id", class_name: "Spree::Order" + has_many :orders, foreign_key: "user_id", class_name: "Spree::Order", dependent: :restrict_with_exception has_many :store_credits, -> { includes(:credit_type) }, foreign_key: "user_id", class_name: "Spree::StoreCredit" has_many :store_credit_events, through: :store_credits diff --git a/core/app/models/spree/legacy_user.rb b/core/app/models/spree/legacy_user.rb index cc21523ac61..5ca3cdfc4cf 100644 --- a/core/app/models/spree/legacy_user.rb +++ b/core/app/models/spree/legacy_user.rb @@ -8,19 +8,11 @@ class LegacyUser < Spree::Base self.table_name = 'spree_users' - before_destroy :check_completed_orders - def self.model_name ActiveModel::Name.new Spree::LegacyUser, Spree, 'user' end attr_accessor :password attr_accessor :password_confirmation - - private - - def check_completed_orders - raise Spree::Core::DestroyWithOrdersError if orders.complete.present? - end end end diff --git a/core/lib/spree/core.rb b/core/lib/spree/core.rb index af491ae9082..d9f92d12f77 100644 --- a/core/lib/spree/core.rb +++ b/core/lib/spree/core.rb @@ -12,6 +12,8 @@ require 'ransack' require 'state_machines-activerecord' +require 'spree/deprecation' + # This is required because ActiveModel::Validations#invalid? conflicts with the # invalid state of a Payment. In the future this should be removed. StateMachines::Machine.ignore_method_conflicts = true @@ -45,7 +47,9 @@ module Core autoload :ProductFilters, "spree/core/product_filters" class GatewayError < RuntimeError; end - class DestroyWithOrdersError < StandardError; end + + include ActiveSupport::Deprecation::DeprecatedConstantAccessor + deprecate_constant 'DestroyWithOrdersError', ActiveRecord::DeleteRestrictionError, deprecator: Spree::Deprecation end end @@ -80,6 +84,5 @@ class DestroyWithOrdersError < StandardError; end require 'spree/core/role_configuration' require 'spree/core/stock_configuration' require 'spree/permission_sets' -require 'spree/deprecation' require 'spree/core/price_migrator' diff --git a/core/spec/models/spree/concerns/user_methods_spec.rb b/core/spec/models/spree/concerns/user_methods_spec.rb index 69f76466101..35955274249 100644 --- a/core/spec/models/spree/concerns/user_methods_spec.rb +++ b/core/spec/models/spree/concerns/user_methods_spec.rb @@ -38,4 +38,23 @@ it { is_expected.to be_nil } end end + + describe "deleting user" do + context "with no orders" do + it "fails validation" do + test_user.destroy! + expect(test_user).to be_destroyed + end + end + + context "with an order" do + let!(:order) { create(:order, user: test_user) } + + it "fails validation" do + expect { + test_user.destroy! + }.to raise_error(ActiveRecord::DeleteRestrictionError) + end + end + end end