From 2f1c89ad675f57d5e0ad0c077f7fc7a655ffa73c Mon Sep 17 00:00:00 2001
From: Ikraam <ikraamghoor@gmail.com>
Date: Mon, 10 Aug 2020 16:50:50 +0200
Subject: [PATCH] Rescue & redirect nil-resource links consistently

The errors caused by non-existent resource links were
created at the point of loading the resources. The primary of which was
due to the resource_controller#parent method returning a `nil` when the
parent_id was not found. The solution was to change `find_by` to
`find_by!`, thereby throwing a `RecordNotFound` error, which could be
handled appropriately.

The secondary point of failure (for some) was created at the
resource_controller#collection method, which raised a `NoMethodError` by
attempting to call `parent.send(controller_name)` when `parent` was
`nil`. The expected behavior was that the `RecordNotFound` would be
raised in #parent and the code execution stopped after the successful
error handling. However, due to the behavior of the in-method rescue
(needed for error-handling context) in contrast to `Rescue_from`, the
code continues executing the `.send(controller_name)` after the
redirection, which then results in the mentioned `NoMethodError`. The
solution was to add a second condition in the return statement to ensure
that 'parent' was not 'nil' before `.send(controller_name)` is called.

Additionally, the resource_controller#resource_not_found method was
moved to the parent class (the base_controller). The move allowed all
children of the base_controller to have access to the method for flash
and redirection. The explicit return of a falsy value, `nil`, at the
end of this method stops the code from continuing to run and eventually
causing a 'DoubleRenderError' after the redirect.

The #resource_not_found was overridden in the resource_controller and
allowed to accept optional parameters for cases where the previously
existing redirection was not appropriate or threw errors due to the
incorrect URLs generated by the polymorphic
resource_controller#collection_url method.

Other controllers needed to be rescued on a case by case approach when
loading the resource to have the appropriate context to
redirect and flash the correct messages.

Tests were created for these problematic URLs to ensure consistency in
the future.
---
 .../spree/admin/base_controller.rb            |  6 ++++
 .../spree/admin/cancellations_controller.rb   |  2 ++
 .../admin/customer_returns_controller.rb      |  2 ++
 .../spree/admin/images_controller.rb          |  3 +-
 .../orders/customer_details_controller.rb     |  2 ++
 .../spree/admin/orders_controller.rb          |  3 +-
 .../spree/admin/payments_controller.rb        |  2 ++
 .../spree/admin/resource_controller.rb        | 16 +++++++----
 .../spree/admin/taxons_controller.rb          |  5 ++++
 .../spree/admin/variants_controller.rb        |  4 ++-
 .../admin/cancellations_controller_spec.rb    |  8 ++++++
 .../admin/customer_returns_controller_spec.rb |  8 ++++++
 .../spree/admin/images_controller_spec.rb     | 13 +++++++++
 .../customer_details_controller_spec.rb       |  7 +++++
 .../spree/admin/orders_controller_spec.rb     | 10 +++----
 .../spree/admin/payments_controller_spec.rb   |  8 ++++++
 .../spree/admin/prices_controller_spec.rb     |  9 ++++++
 .../product_properties_controller_spec.rb     | 15 ++++++++++
 .../admin/store_credits_controller_spec.rb    |  8 ++++++
 .../spree/admin/taxons_controller_spec.rb     | 28 +++++++++++++++++++
 .../spree/admin/variants_controller_spec.rb   |  9 ++++++
 21 files changed, 154 insertions(+), 14 deletions(-)
 create mode 100644 backend/spec/controllers/spree/admin/images_controller_spec.rb
 create mode 100644 backend/spec/controllers/spree/admin/taxons_controller_spec.rb

diff --git a/backend/app/controllers/spree/admin/base_controller.rb b/backend/app/controllers/spree/admin/base_controller.rb
index d87d62b19b1..f44ac0f894e 100644
--- a/backend/app/controllers/spree/admin/base_controller.rb
+++ b/backend/app/controllers/spree/admin/base_controller.rb
@@ -62,6 +62,12 @@ def lock_order
       def order_mutex_redirect_path
         edit_admin_order_path(@order)
       end
+
+      def resource_not_found(flash_class:, redirect_url:)
+        flash[:error] = flash_message_for(flash_class.new, :not_found)
+        redirect_to redirect_url
+        nil
+      end
     end
   end
 end
diff --git a/backend/app/controllers/spree/admin/cancellations_controller.rb b/backend/app/controllers/spree/admin/cancellations_controller.rb
index 3e7ac13e19b..b6fe42f8c27 100644
--- a/backend/app/controllers/spree/admin/cancellations_controller.rb
+++ b/backend/app/controllers/spree/admin/cancellations_controller.rb
@@ -36,6 +36,8 @@ def created_by
       def load_order
         @order = Spree::Order.find_by!(number: params[:order_id])
         authorize! action, @order
+      rescue ActiveRecord::RecordNotFound
+        resource_not_found(flash_class: Spree::Order, redirect_url: admin_orders_path)
       end
 
       def model_class
diff --git a/backend/app/controllers/spree/admin/customer_returns_controller.rb b/backend/app/controllers/spree/admin/customer_returns_controller.rb
index a1201399f68..cb6c204ef91 100644
--- a/backend/app/controllers/spree/admin/customer_returns_controller.rb
+++ b/backend/app/controllers/spree/admin/customer_returns_controller.rb
@@ -42,6 +42,8 @@ def find_resource
 
       def collection
         parent # trigger loading the order
+        return unless @order
+
         @collection ||= Spree::ReturnItem
           .accessible_by(current_ability, :read)
           .where(inventory_unit_id: @order.inventory_units.pluck(:id))
diff --git a/backend/app/controllers/spree/admin/images_controller.rb b/backend/app/controllers/spree/admin/images_controller.rb
index 13a2df3d64e..f281da56ac2 100644
--- a/backend/app/controllers/spree/admin/images_controller.rb
+++ b/backend/app/controllers/spree/admin/images_controller.rb
@@ -4,7 +4,6 @@ module Spree
   module Admin
     class ImagesController < ResourceController
       before_action :load_data
-
       create.before :set_viewable
       update.before :set_viewable
 
@@ -24,6 +23,8 @@ def load_data
           [variant.sku_and_options_text, variant.id]
         end
         @variants.insert(0, [t('spree.all'), @product.master.id])
+      rescue ActiveRecord::RecordNotFound
+        resource_not_found(flash_class: Spree::Product, redirect_url: admin_products_path)
       end
 
       def set_viewable
diff --git a/backend/app/controllers/spree/admin/orders/customer_details_controller.rb b/backend/app/controllers/spree/admin/orders/customer_details_controller.rb
index 37f74502cc0..734afa57809 100644
--- a/backend/app/controllers/spree/admin/orders/customer_details_controller.rb
+++ b/backend/app/controllers/spree/admin/orders/customer_details_controller.rb
@@ -61,6 +61,8 @@ def order_params
 
         def load_order
           @order = Spree::Order.includes(:adjustments).find_by!(number: params[:order_id])
+        rescue ActiveRecord::RecordNotFound
+          resource_not_found(flash_class: Spree::Order, redirect_url: admin_orders_path)
         end
 
         def model_class
diff --git a/backend/app/controllers/spree/admin/orders_controller.rb b/backend/app/controllers/spree/admin/orders_controller.rb
index 08fafabeb45..f8787479f8a 100644
--- a/backend/app/controllers/spree/admin/orders_controller.rb
+++ b/backend/app/controllers/spree/admin/orders_controller.rb
@@ -10,7 +10,6 @@ class OrdersController < Spree::Admin::BaseController
       around_action :lock_order, only: [:update, :advance, :complete, :confirm, :cancel, :resume, :approve, :resend]
 
       rescue_from Spree::Order::InsufficientStock, with: :insufficient_stock_error
-
       respond_to :html
 
       def index
@@ -168,6 +167,8 @@ def order_params
       def load_order
         @order = Spree::Order.includes(:adjustments).find_by!(number: params[:id])
         authorize! action, @order
+      rescue ActiveRecord::RecordNotFound
+        resource_not_found(flash_class: Spree::Order, redirect_url: admin_orders_path)
       end
 
       # Used for extensions which need to provide their own custom event links on the order details view.
diff --git a/backend/app/controllers/spree/admin/payments_controller.rb b/backend/app/controllers/spree/admin/payments_controller.rb
index 0a60ee9ea2a..eda6ca65665 100644
--- a/backend/app/controllers/spree/admin/payments_controller.rb
+++ b/backend/app/controllers/spree/admin/payments_controller.rb
@@ -92,6 +92,8 @@ def load_order
         @order = Spree::Order.find_by!(number: params[:order_id])
         authorize! action, @order
         @order
+      rescue ActiveRecord::RecordNotFound
+        resource_not_found(flash_class: Spree::Order, redirect_url: admin_orders_path)
       end
 
       def load_payment
diff --git a/backend/app/controllers/spree/admin/resource_controller.rb b/backend/app/controllers/spree/admin/resource_controller.rb
index 8406cf97d50..dbdcbd467b5 100644
--- a/backend/app/controllers/spree/admin/resource_controller.rb
+++ b/backend/app/controllers/spree/admin/resource_controller.rb
@@ -5,7 +5,9 @@ class Spree::Admin::ResourceController < Spree::Admin::BaseController
 
   helper_method :new_object_url, :edit_object_url, :object_url, :collection_url
   before_action :load_resource, except: :update_positions
-  rescue_from ActiveRecord::RecordNotFound, with: :resource_not_found
+  rescue_from ActiveRecord::RecordNotFound do
+    resource_not_found
+  end
   rescue_from ActiveRecord::RecordInvalid, with: :resource_invalid
 
   respond_to :html
@@ -134,9 +136,8 @@ def belongs_to(model_name, options = {})
     end
   end
 
-  def resource_not_found
-    flash[:error] = flash_message_for(model_class.new, :not_found)
-    redirect_to collection_url
+  def resource_not_found(flash_class: model_class, redirect_url: collection_url)
+    super
   end
 
   def model_class
@@ -193,12 +194,14 @@ def parent
     if parent?
       @parent ||= self.class.parent_data[:model_class]
                     .includes(self.class.parent_data[:includes])
-                    .find_by(self.class.parent_data[:find_by] => params["#{parent_model_name}_id"])
+                    .find_by!(self.class.parent_data[:find_by] => params["#{parent_model_name}_id"])
       instance_variable_set("@#{parent_model_name}", @parent)
     else
       Spree::Deprecation.warn "Calling #parent is deprecated on a ResourceController which has not defined a belongs_to"
       nil
     end
+  rescue ActiveRecord::RecordNotFound => e
+    resource_not_found(flash_class: e.model.constantize, redirect_url: spree.polymorphic_url([:admin, parent_model_name.pluralize]))
   end
 
   def parent?
@@ -222,7 +225,8 @@ def build_resource
   end
 
   def collection
-    return parent.send(controller_name) if parent?
+    return parent.send(controller_name) if parent? && parent
+
     if model_class.respond_to?(:accessible_by) && !current_ability.has_block?(params[:action], model_class)
       model_class.accessible_by(current_ability, action)
     else
diff --git a/backend/app/controllers/spree/admin/taxons_controller.rb b/backend/app/controllers/spree/admin/taxons_controller.rb
index 51005b3604e..1271b3af123 100644
--- a/backend/app/controllers/spree/admin/taxons_controller.rb
+++ b/backend/app/controllers/spree/admin/taxons_controller.rb
@@ -3,6 +3,7 @@
 module Spree
   module Admin
     class TaxonsController < Spree::Admin::BaseController
+      rescue_from ActiveRecord::RecordNotFound, with: :resource_not_found
       respond_to :html, :json, :js
 
       def index
@@ -83,6 +84,10 @@ def destroy
       def taxon_params
         params.require(:taxon).permit(permitted_taxon_attributes)
       end
+
+      def resource_not_found
+        super(flash_class: Taxon, redirect_url: admin_taxonomies_path)
+      end
     end
   end
 end
diff --git a/backend/app/controllers/spree/admin/variants_controller.rb b/backend/app/controllers/spree/admin/variants_controller.rb
index 002187290ee..8c1ff0861ce 100644
--- a/backend/app/controllers/spree/admin/variants_controller.rb
+++ b/backend/app/controllers/spree/admin/variants_controller.rb
@@ -43,8 +43,10 @@ def redirect_on_empty_option_values
       end
 
       def parent
-        @parent ||= Spree::Product.with_discarded.find_by(slug: params[:product_id])
+        @parent ||= Spree::Product.with_discarded.find_by!(slug: params[:product_id])
         @product = @parent
+      rescue ActiveRecord::RecordNotFound
+        resource_not_found(flash_class: Spree::Product, redirect_url: admin_products_path)
       end
     end
   end
diff --git a/backend/spec/controllers/spree/admin/cancellations_controller_spec.rb b/backend/spec/controllers/spree/admin/cancellations_controller_spec.rb
index a0a5c9a85f7..88dc509ca08 100644
--- a/backend/spec/controllers/spree/admin/cancellations_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/cancellations_controller_spec.rb
@@ -17,6 +17,14 @@
         expect(response).to be_ok
       end
     end
+
+    context "existent order id not given" do
+      it "redirects and flashes about the non-existent order" do
+        get :index, params: { order_id: 'non-existent-order' }
+        expect(response).to redirect_to(spree.admin_orders_path)
+        expect(flash[:error]).to eql("Order is not found")
+      end
+    end
   end
 
   describe "#cancel" do
diff --git a/backend/spec/controllers/spree/admin/customer_returns_controller_spec.rb b/backend/spec/controllers/spree/admin/customer_returns_controller_spec.rb
index c1f67bb9b94..ddbacb73e93 100644
--- a/backend/spec/controllers/spree/admin/customer_returns_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/customer_returns_controller_spec.rb
@@ -26,6 +26,14 @@ module Admin
         end
       end
 
+      context "existent order id not given" do
+        it "redirects and flashes about the non-existent order" do
+          get :index, params: { order_id: 'non-existent-order' }
+          expect(response).to redirect_to(spree.admin_orders_path)
+          expect(flash[:error]).to eql("Order is not found")
+        end
+      end
+
       describe "#new" do
         let(:order) { create(:shipped_order, line_items_count: 1) }
         let!(:inactive_reimbursement_type)      { create(:reimbursement_type, active: false) }
diff --git a/backend/spec/controllers/spree/admin/images_controller_spec.rb b/backend/spec/controllers/spree/admin/images_controller_spec.rb
new file mode 100644
index 00000000000..629d32da62a
--- /dev/null
+++ b/backend/spec/controllers/spree/admin/images_controller_spec.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Spree::Admin::ImagesController, type: :controller do
+  stub_authorization!
+
+  it "cannot find a non-existent product" do
+    get :index, params: { product_id: "non-existent-product" }
+    expect(response).to redirect_to(spree.admin_products_path)
+    expect(flash[:error]).to eql("Product is not found")
+  end
+end
diff --git a/backend/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb b/backend/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb
index 6710d7a0962..e78bf168c20 100644
--- a/backend/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/orders/customer_details_controller_spec.rb
@@ -53,6 +53,13 @@
           end
         end
       end
+      context "existent order id not given" do
+        it "redirects and flashes about the non-existent order" do
+          get :edit, params: { order_id: 'non-existent-order' }
+          expect(response).to redirect_to(spree.admin_orders_path)
+          expect(flash[:error]).to eql("Order is not found")
+        end
+      end
     end
 
     context "#update" do
diff --git a/backend/spec/controllers/spree/admin/orders_controller_spec.rb b/backend/spec/controllers/spree/admin/orders_controller_spec.rb
index de57fb1cf65..899821611be 100644
--- a/backend/spec/controllers/spree/admin/orders_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/orders_controller_spec.rb
@@ -396,13 +396,13 @@
     end
   end
 
-  context "order number not given" do
+  context "existent order number not given" do
     stub_authorization!
 
-    it "raise active record not found" do
-      expect {
-        get :edit, params: { id: 0 }
-      }.to raise_error ActiveRecord::RecordNotFound
+    it "cannot find non-existent order" do
+      get :edit, params: { id: 0 }
+      expect(response).to redirect_to(spree.admin_orders_path)
+      expect(flash[:error]).to eql("Order is not found")
     end
   end
 end
diff --git a/backend/spec/controllers/spree/admin/payments_controller_spec.rb b/backend/spec/controllers/spree/admin/payments_controller_spec.rb
index fabae0d7374..1cc20671f5e 100644
--- a/backend/spec/controllers/spree/admin/payments_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/payments_controller_spec.rb
@@ -152,6 +152,14 @@ module Admin
             expect(response).to redirect_to(spree.edit_admin_order_customer_path(order))
           end
         end
+
+        context "existent order id not given" do
+          it "redirects and flashes about the non-existent order" do
+            get :index, params: { order_id: 'non-existent-order' }
+            expect(response).to redirect_to(spree.admin_orders_path)
+            expect(flash[:error]).to eql("Order is not found")
+          end
+        end
       end
 
       describe '#fire' do
diff --git a/backend/spec/controllers/spree/admin/prices_controller_spec.rb b/backend/spec/controllers/spree/admin/prices_controller_spec.rb
index 0a865c22f77..21471b0143b 100644
--- a/backend/spec/controllers/spree/admin/prices_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/prices_controller_spec.rb
@@ -41,5 +41,14 @@
         expect(assigns(:product)).to eq(product)
       end
     end
+    context "existent product id not given" do
+      subject { get :index, params: { product_id: 'non-existent-product' } }
+
+      it "cannot find non-existent product" do
+        subject
+        expect(response).to redirect_to(spree.admin_products_path)
+        expect(flash[:error]).to eql("Product is not found")
+      end
+    end
   end
 end
diff --git a/backend/spec/controllers/spree/admin/product_properties_controller_spec.rb b/backend/spec/controllers/spree/admin/product_properties_controller_spec.rb
index 69b4e248034..b7eab913634 100644
--- a/backend/spec/controllers/spree/admin/product_properties_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/product_properties_controller_spec.rb
@@ -64,6 +64,21 @@ module Admin
               expect(assigns(:variant_property_rule)).to eq first_rule
             end
           end
+
+          context "existent product id not given" do
+            let(:parameters) do
+              {
+                product_id: 'non-existent-product'
+              }
+            end
+
+            before { subject }
+
+            it "cannot find non-existent product" do
+              expect(response).to redirect_to(spree.admin_products_path)
+              expect(flash[:error]).to eql("Product is not found")
+            end
+          end
         end
       end
     end
diff --git a/backend/spec/controllers/spree/admin/store_credits_controller_spec.rb b/backend/spec/controllers/spree/admin/store_credits_controller_spec.rb
index 66e0d6efe01..aa3b775795e 100644
--- a/backend/spec/controllers/spree/admin/store_credits_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/store_credits_controller_spec.rb
@@ -308,4 +308,12 @@
       end
     end
   end
+  context 'User does not exist' do
+    let(:store_credit) { create(:store_credit, user: user, category: a_credit_category) }
+    it "cannot find a store-credit for non-existent user" do
+      get :index, params: { user_id: 'non-existent-user', id: store_credit.id }
+      expect(response).to redirect_to(spree.admin_users_path)
+      expect(flash[:error]).to eql("User is not found")
+    end
+  end
 end
diff --git a/backend/spec/controllers/spree/admin/taxons_controller_spec.rb b/backend/spec/controllers/spree/admin/taxons_controller_spec.rb
new file mode 100644
index 00000000000..41d84efe002
--- /dev/null
+++ b/backend/spec/controllers/spree/admin/taxons_controller_spec.rb
@@ -0,0 +1,28 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe Spree::Admin::TaxonsController, type: :controller do
+  stub_authorization!
+
+  describe "#edit" do
+    let!(:taxon) { create(:taxon) }
+
+    it "finds existing taxon" do
+      get :edit, params: { taxonomy_id: taxon.taxonomy, id: taxon.id }
+      expect(response.status).to eq(200)
+    end
+
+    it "cannot find a non-existing taxon with existent taxonomy" do
+      get :edit, params: { taxonomy_id: taxon.taxonomy, id: 'non-existent-taxon' }
+      expect(response).to redirect_to(spree.admin_taxonomies_path)
+      expect(flash[:error]).to eql("Taxon is not found")
+    end
+
+    it "cannot find a existing taxon with non-existent taxonomy" do
+      get :edit, params: { taxonomy_id: 'non-existent-taxonomy', id: taxon.id }
+      expect(response).to redirect_to(spree.admin_taxonomies_path)
+      expect(flash[:error]).to eql("Taxon is not found")
+    end
+  end
+end
diff --git a/backend/spec/controllers/spree/admin/variants_controller_spec.rb b/backend/spec/controllers/spree/admin/variants_controller_spec.rb
index 3b971940a8a..92b217dec9a 100644
--- a/backend/spec/controllers/spree/admin/variants_controller_spec.rb
+++ b/backend/spec/controllers/spree/admin/variants_controller_spec.rb
@@ -53,6 +53,15 @@ module Admin
                 expect(assigns(:collection)).to include deleted_variant
               end
             end
+            context "existent product id not given" do
+              let(:params) { { product_id: 'non-existent-product' } }
+
+              it "cannot find non-existent product" do
+                subject
+                expect(response).to redirect_to(spree.admin_products_path)
+                expect(flash[:error]).to eql("Product is not found")
+              end
+            end
           end
         end
       end