Refactor Overview Controller to make it more clear

Assigns meaningful names to the boolean conditions to make it easier
to understand, breaks down the big and nested if/else and converts the
specs to RSpec 3.

Note the check `!spree_current_user.admin?` has been removed because
in admin/base_controller_decorator.rb `#authorize_admin` is already
called.

.rubocop_todo.yml

@@ -2149,7 +2149,6 @@ Style/IfInsideElse:
   Exclude:
     - 'app/controllers/admin/column_preferences_controller.rb'
     - 'app/controllers/admin/variant_overrides_controller.rb'
-    - 'app/controllers/spree/admin/overview_controller_decorator.rb'
     - 'app/controllers/spree/admin/products_controller_decorator.rb'
 
 # Offense count: 1
@@ -2435,7 +2434,6 @@ Style/RedundantSelf:
 Style/RegexpLiteral:
   Exclude:
     - 'app/controllers/admin/enterprises_controller.rb'
-    - 'app/controllers/spree/admin/overview_controller_decorator.rb'
     - 'app/helpers/groups_helper.rb'
     - 'app/helpers/html_helper.rb'
     - 'app/models/enterprise.rb'

app/controllers/spree/admin/overview_controller_decorator.rb

@@ -1,26 +1,74 @@
 Spree::Admin::OverviewController.class_eval do
   def index
-    # TODO was sorted with is_distributor DESC as well, not sure why or how we want ot sort this now
-    @enterprises = Enterprise.managed_by(spree_current_user).order('is_primary_producer ASC, name')
+    @enterprises = Enterprise
+      .managed_by(spree_current_user)
+      .order('is_primary_producer ASC, name')
     @product_count = Spree::Product.active.managed_by(spree_current_user).count
     @order_cycle_count = OrderCycle.active.managed_by(spree_current_user).count
 
-    unspecified = spree_current_user.owned_enterprises.where(sells: 'unspecified')
-    outside_referral = !URI(request.referer.to_s).path.match(/^\/admin/)
-
-    if OpenFoodNetwork::Permissions.new(spree_current_user).manages_one_enterprise? && !spree_current_user.admin?
-      @enterprise = @enterprises.first
-      if outside_referral && unspecified.any?
-        redirect_to main_app.welcome_admin_enterprise_path(@enterprise)
-      else
-        render "single_enterprise_dashboard"
-      end
+    if first_access
+      redirect_to enterprises_path
     else
-      if outside_referral && unspecified.any?
-        redirect_to main_app.admin_enterprises_path
-      else
-        render "multi_enterprise_dashboard"
-      end
+      render dashboard_view
     end
   end
+
+  private
+
+  # Checks whether the user is accessing the admin for the first time
+  #
+  # @return [Boolean]
+  def first_access
+    outside_referral && incomplete_enterprise_registration?
+  end
+
+  # Checks whether the request comes from another admin page or not
+  #
+  # @return [Boolean]
+  def outside_referral
+    !URI(request.referer.to_s).path.match(%r{/admin})
+  end
+
+  # Checks that all of the enterprises owned by the current user have a 'sells'
+  # property specified, which indicates that the registration process has been
+  # completed
+  #
+  # @return [Boolean]
+  def incomplete_enterprise_registration?
+    @incomplete_enterprise_registration ||= spree_current_user
+      .owned_enterprises
+      .where(sells: 'unspecified')
+      .exists?
+  end
+
+  # Returns the appropriate enterprise path for the current user
+  #
+  # @return [String]
+  def enterprises_path
+    if managed_enterprises.size == 1
+      @enterprise = @enterprises.first
+      main_app.welcome_admin_enterprise_path(@enterprise)
+    else
+      main_app.admin_enterprises_path
+    end
+  end
+
+  # Returns the appropriate dashboard view for the current user
+  #
+  # @return [String]
+  def dashboard_view
+    if managed_enterprises.size == 1
+      @enterprise = @enterprises.first
+      :single_enterprise_dashboard
+    else
+      :multi_enterprise_dashboard
+    end
+  end
+
+  # Returns the list of enterprises the current user is manager of
+  #
+  # @return [ActiveRecord::Relation<Enterprise>]
+  def managed_enterprises
+    spree_current_user.enterprises
+  end
 end

spec/controllers/spree/admin/overview_controller_spec.rb

@@ -2,18 +2,20 @@ require 'spec_helper'
 
 describe Spree::Admin::OverviewController, type: :controller do
   include AuthenticationWorkflow
-  context "loading overview" do
-    let(:user) { create_enterprise_user(enterprise_limit: 2) }
 
+  describe "#index" do
     before do
-      controller.stub spree_current_user: user
+      allow(controller).to receive(:spree_current_user).and_return(user)
     end
 
     context "when user owns only one enterprise" do
+      let(:user) { create_enterprise_user }
       let!(:enterprise) { create(:distributor_enterprise, owner: user) }
 
       context "when the referer is not an admin page" do
-        before { @request.env['HTTP_REFERER'] = 'http://test.com/some_other_path' }
+        before do
+          @request.env['HTTP_REFERER'] = 'http://test.com/not_admin_path'
+        end
 
         context "and the enterprise has sells='unspecified'" do
           before do
@@ -22,14 +24,15 @@ describe Spree::Admin::OverviewController, type: :controller do
 
           it "redirects to the welcome page for the enterprise" do
             spree_get :index
-            response.should redirect_to welcome_admin_enterprise_path(enterprise)
+            expect(response)
+              .to redirect_to welcome_admin_enterprise_path(enterprise)
           end
         end
 
         context "and the enterprise does not have sells='unspecified'" do
           it "renders the single enterprise dashboard" do
             spree_get :index
-            response.should render_template "single_enterprise_dashboard"
+            expect(response).to render_template :single_enterprise_dashboard
           end
         end
       end
@@ -39,17 +42,21 @@ describe Spree::Admin::OverviewController, type: :controller do
 
         it "renders the single enterprise dashboard" do
           spree_get :index
-          response.should render_template "single_enterprise_dashboard"
+          expect(response).to render_template :single_enterprise_dashboard
         end
       end
     end
 
     context "when user owns multiple enterprises" do
+      let(:user) { create_enterprise_user(enterprise_limit: 2) }
+
       let!(:enterprise1) { create(:distributor_enterprise, owner: user) }
-      let!(:enterprise2) { create(:distributor_enterprise, owner: user) }
+      before { create(:distributor_enterprise, owner: user) }
 
       context "when the referer is not an admin page" do
-        before { @request.env['HTTP_REFERER'] = 'http://test.com/some_other_path' }
+        before do
+          @request.env['HTTP_REFERER'] = 'http://test.com/not_admin_path'
+        end
 
         context "and at least one owned enterprise has sells='unspecified'" do
           before do
@@ -58,14 +65,14 @@ describe Spree::Admin::OverviewController, type: :controller do
 
           it "redirects to the enterprises index" do
             spree_get :index
-            response.should redirect_to admin_enterprises_path
+            expect(response).to redirect_to admin_enterprises_path
           end
         end
 
         context "and no owned enterprises have sells='unspecified'" do
           it "renders the multiple enterprise dashboard" do
             spree_get :index
-            response.should render_template "multi_enterprise_dashboard"
+            expect(response).to render_template :multi_enterprise_dashboard
           end
         end
       end
@@ -75,7 +82,7 @@ describe Spree::Admin::OverviewController, type: :controller do
 
         it "renders the multiple enterprise dashboard" do
           spree_get :index
-          response.should render_template "multi_enterprise_dashboard"
+          expect(response).to render_template :multi_enterprise_dashboard
         end
       end
     end