[PATCH conductor] Adds missing nil value check in ProvidersController

pblaho at redhat.com pblaho at redhat.com
Tue Sep 4 18:20:27 UTC 2012


From: Petr Blaho <pblaho at redhat.com>

https://bugzilla.redhat.com/show_bug.cgi?id=848472
---
 src/app/controllers/providers_controller.rb       |  236 +++++++++++----------
 src/spec/controllers/providers_controller_spec.rb |   36 ++--
 2 files changed, 141 insertions(+), 131 deletions(-)

diff --git a/src/app/controllers/providers_controller.rb b/src/app/controllers/providers_controller.rb
index f3a9302..2a163e4 100644
--- a/src/app/controllers/providers_controller.rb
+++ b/src/app/controllers/providers_controller.rb
@@ -46,7 +46,7 @@ class ProvidersController < ApplicationController
 
   def filter
     redirect_to_original({ "from_date" => params[:from_date],
-                           "to_date" => params[:to_date] })
+                         "to_date" => params[:to_date] })
   end
 
   def new
@@ -89,8 +89,8 @@ class ProvidersController < ApplicationController
     @tab_captions = [t("properties"), t('hw_profiles'), t('realm_s'), t("provider_accounts.index.provider_accounts"), t('services'), t('history'), t('permissions')]
     @details_tab = params[:details_tab].blank? ? t("properties") : params[:details_tab]
     @details_tab = 'properties' unless ['properties', 'hw_profiles', 'realms',
-                                        'provider_accounts', 'services', 'history',
-                                        'permissions'].include?(@details_tab)
+      'provider_accounts', 'services', 'history',
+      'permissions'].include?(@details_tab)
 
     if params.delete :test_provider
       test_connection(@provider)
@@ -203,7 +203,7 @@ class ProvidersController < ApplicationController
 
   protected
 
-    # looking for ProviderType based on content of provider_type tag in xml
+  # looking for ProviderType based on content of provider_type tag in xml
   def parse_provider_type
     if params.has_key?(:provider) && params[:provider].is_a?(Hash) \
       && params[:provider].has_key?(:provider_type) \
@@ -211,7 +211,9 @@ class ProvidersController < ApplicationController
       provider_type_hash = params[:provider].delete(:provider_type)
       provider_type_id = provider_type_hash[:id]
       provider_type = ProviderType.find_by_id(provider_type_id)
-      params[:provider][:provider_type_id] = provider_type.id
+      if provider_type
+        params[:provider][:provider_type_id] = provider_type.id
+      end
     end
   end
 
@@ -227,7 +229,7 @@ class ProvidersController < ApplicationController
 
   def load_providers
     @providers = Provider.includes(:provider_type).list_for_user(current_session, current_user,
-                                        Privilege::VIEW).order("providers.name")
+                                                                 Privilege::VIEW).order("providers.name")
   end
 
   def disable_provider
@@ -307,22 +309,22 @@ class ProvidersController < ApplicationController
     @realms = @provider.realms.apply_filters(:preset_filter_id => params[:provider_realms_preset_filter], :search_filter => params[:provider_realms_search])
     #TODO add links to real data for history,properties,permissions
     @tabs = [{:name => t('connectivity'), :view => 'edit', :id => 'connectivity'},
-             {:name => t('accounts'), :view => 'provider_accounts/list', :id => 'accounts', :count => @provider.provider_accounts.count},
-             {:name => t('provider_realms.provider_realms'), :view => 'provider_realms/list', :id => 'realms', :count => @realms.count},
-             #{:name => 'Roles & Permissions', :view => @view, :id => 'roles', :count => @provider.permissions.count},
+      {:name => t('accounts'), :view => 'provider_accounts/list', :id => 'accounts', :count => @provider.provider_accounts.count},
+      {:name => t('provider_realms.provider_realms'), :view => 'provider_realms/list', :id => 'realms', :count => @realms.count},
+      #{:name => 'Roles & Permissions', :view => @view, :id => 'roles', :count => @provider.permissions.count},
     ]
     add_permissions_tab(@provider, "edit_")
     details_tab_name = params[:details_tab].blank? ? 'connectivity' : params[:details_tab]
     details_tab_name = 'connectivity' unless
-      ['connectivity', 'accounts', 'realms', 'permissions'].include?(details_tab_name)
+    ['connectivity', 'accounts', 'realms', 'permissions'].include?(details_tab_name)
     @details_tab = @tabs.find {|t| t[:id] == details_tab_name} || @tabs.first[:name].downcase
 
     if @details_tab[:id] == 'accounts'
       @provider_accounts = @provider.provider_accounts.
         apply_filters(:preset_filter_id =>
-                        params[:provider_accounts_preset_filter],
-                      :search_filter => params[:provider_accounts_search]).
-        list_for_user(current_session, current_user, Privilege::VIEW)
+                      params[:provider_accounts_preset_filter],
+                        :search_filter => params[:provider_accounts_search]).
+                        list_for_user(current_session, current_user, Privilege::VIEW)
     end
     #@permissions = @provider.permissions if @details_tab[:id] == 'roles'
 
@@ -332,19 +334,19 @@ class ProvidersController < ApplicationController
   def load_headers
     @header = [
       { :name => t('providers.index.provider_name'), :class => 'center',
-                 :sortable => false },
-      { :name => t('providers.index.provider_type'), :class => 'center',
-                 :sortable => false },
-      { :name => t('providers.index.running_instances'), :class => 'center',
-                 :sortable => false },
-      { :name => t('providers.index.pending_instances'), :class => 'center',
-                 :sortable => false },
-      { :name => t('providers.index.error_instances'), :class => 'center',
-                 :sortable => false },
-      { :name => t('providers.index.historical_running_instances'), :class => 'center',
-                 :sortable => false },
-      { :name => t('providers.index.historical_error_instances'), :class => 'center',
-                 :sortable => false },
+        :sortable => false },
+        { :name => t('providers.index.provider_type'), :class => 'center',
+          :sortable => false },
+          { :name => t('providers.index.running_instances'), :class => 'center',
+            :sortable => false },
+            { :name => t('providers.index.pending_instances'), :class => 'center',
+              :sortable => false },
+              { :name => t('providers.index.error_instances'), :class => 'center',
+                :sortable => false },
+                { :name => t('providers.index.historical_running_instances'), :class => 'center',
+                  :sortable => false },
+                  { :name => t('providers.index.historical_error_instances'), :class => 'center',
+                    :sortable => false },
     ]
   end
 
@@ -378,10 +380,10 @@ class ProvidersController < ApplicationController
       if Instance::FAILED_STATES.include?(state)
         @statistics[provider_id]["error_instances"] += count.to_i
       elsif [Instance::STATE_RUNNING, Instance::STATE_SHUTTING_DOWN].
-               include?(state)
+        include?(state)
         @statistics[provider_id]["running_instances"] += count.to_i
       elsif [Instance::STATE_NEW, Instance::STATE_PENDING].
-               include?(state)
+        include?(state)
         @statistics[provider_id]["pending_instances"] += count.to_i
       end
     end
@@ -394,96 +396,96 @@ class ProvidersController < ApplicationController
       where("time_last_running <= :to_date and
              (time_last_stopped is null
               or time_last_stopped >= :from_date)",
-            :to_date => @to_date.to_datetime.end_of_day,
-            :from_date => @from_date.to_datetime.beginning_of_day
-            ).
-      group("provider_id, state")
-
-    historical_running_provider_counts.each do |provider_count|
-      provider_id = provider_count["provider_id"]
-      count = provider_count["count"]
-
-      @statistics[provider_id]["historical_running_instances"] += count.to_i
-    end
-
-    # instances that threw an error between historical date range
-    historical_error_provider_counts = ProviderAccount.joins(:instances).
-      merge(Instance.unscoped).
-      select("provider_id, count(*) as count").
-      where(:provider_id => @providers.map{|provider| provider.id}).
-      where("instances.state" => Instance::FAILED_STATES).
-      where("instances.updated_at between :from_date and :to_date",
-            :states => Instance::FAILED_STATES,
-            :to_date => @to_date.to_datetime.end_of_day,
-            :from_date => @from_date.to_datetime.beginning_of_day
-            ).
-      group("provider_id")
-
-    historical_error_provider_counts.each do |provider_count|
-      provider_id = provider_count["provider_id"]
-      count = provider_count["count"]
-
-      @statistics[provider_id]["historical_error_instances"] += count.to_i
-    end
-
-    # all running instances during historical date range
-    historical_instances = Instance.unscoped.
-      find(:all,
-           :conditions => ["time_last_running <= ? and
+              :to_date => @to_date.to_datetime.end_of_day,
+              :from_date => @from_date.to_datetime.beginning_of_day
+           ).
+             group("provider_id, state")
+
+           historical_running_provider_counts.each do |provider_count|
+             provider_id = provider_count["provider_id"]
+             count = provider_count["count"]
+
+             @statistics[provider_id]["historical_running_instances"] += count.to_i
+           end
+
+           # instances that threw an error between historical date range
+           historical_error_provider_counts = ProviderAccount.joins(:instances).
+             merge(Instance.unscoped).
+             select("provider_id, count(*) as count").
+             where(:provider_id => @providers.map{|provider| provider.id}).
+             where("instances.state" => Instance::FAILED_STATES).
+             where("instances.updated_at between :from_date and :to_date",
+                   :states => Instance::FAILED_STATES,
+                   :to_date => @to_date.to_datetime.end_of_day,
+                   :from_date => @from_date.to_datetime.beginning_of_day
+                  ).
+                    group("provider_id")
+
+                  historical_error_provider_counts.each do |provider_count|
+                    provider_id = provider_count["provider_id"]
+                    count = provider_count["count"]
+
+                    @statistics[provider_id]["historical_error_instances"] += count.to_i
+                  end
+
+                  # all running instances during historical date range
+                  historical_instances = Instance.unscoped.
+                    find(:all,
+                         :conditions => ["time_last_running <= ? and
                              (time_last_stopped is null
                               or time_last_stopped >= ?)",
-                           @to_date.to_datetime.end_of_day,
-                           @from_date.to_datetime.beginning_of_day],
-           :include => {:provider_account => [:provider]}
-           )
-
-    @datasets = ChartDatasets.new(@from_date, @to_date)
-    events = Array.new
-
-    historical_instances.each do |instance|
-      provider_account = instance.provider_account
-
-      if check_privilege(Privilege::VIEW, provider_account)
-        label = provider_account.nil? ?
-                  'Unknown' :
-                  provider_account.provider.name +
-                  " (" + provider_account.name + ")"
-
-        # see if instance started before from_date
-        if instance.time_last_running <= @from_date.to_datetime.beginning_of_day
-          @datasets.increment_count(label,1)
-          @datasets.increment_count("All",1)
-        else
-          events << {
-            "time" => instance.time_last_running,
-            "label" => label,
-            "increment" => 1
-          }
-        end
-
-        if !instance.time_last_stopped.nil? &&
-            instance.time_last_stopped >= instance.time_last_running &&
-            instance.time_last_stopped <= @to_date.to_datetime.end_of_day
-          events << {
-            "time" => instance.time_last_stopped,
-            "label" => label,
-            "increment" => -1
-          }
-        end
-      end
-    end
-
-    @datasets.initialize_datasets
-
-    events.sort_by {|event| event["time"]}.each do |event|
-      timestamp = event["time"].to_i * 1000
-      increment = event["increment"]
-
-      [ event["label"], "All" ].each { |label|
-        @datasets.add_dataset_point(label,timestamp,increment)
-      }
-    end
-
-    @datasets.finalize_datasets
+                              @to_date.to_datetime.end_of_day,
+                              @from_date.to_datetime.beginning_of_day],
+                              :include => {:provider_account => [:provider]}
+                        )
+
+                        @datasets = ChartDatasets.new(@from_date, @to_date)
+                        events = Array.new
+
+                        historical_instances.each do |instance|
+                          provider_account = instance.provider_account
+
+                          if check_privilege(Privilege::VIEW, provider_account)
+                            label = provider_account.nil? ?
+                              'Unknown' :
+                              provider_account.provider.name +
+                              " (" + provider_account.name + ")"
+
+                            # see if instance started before from_date
+                            if instance.time_last_running <= @from_date.to_datetime.beginning_of_day
+                              @datasets.increment_count(label,1)
+                              @datasets.increment_count("All",1)
+                            else
+                              events << {
+                                "time" => instance.time_last_running,
+                                "label" => label,
+                                "increment" => 1
+                              }
+                            end
+
+                            if !instance.time_last_stopped.nil? &&
+                              instance.time_last_stopped >= instance.time_last_running &&
+                              instance.time_last_stopped <= @to_date.to_datetime.end_of_day
+                              events << {
+                                "time" => instance.time_last_stopped,
+                                "label" => label,
+                                "increment" => -1
+                              }
+                            end
+                          end
+                        end
+
+                        @datasets.initialize_datasets
+
+                        events.sort_by {|event| event["time"]}.each do |event|
+                          timestamp = event["time"].to_i * 1000
+                          increment = event["increment"]
+
+                          [ event["label"], "All" ].each { |label|
+                            @datasets.add_dataset_point(label,timestamp,increment)
+                          }
+                        end
+
+                        @datasets.finalize_datasets
   end
 end
diff --git a/src/spec/controllers/providers_controller_spec.rb b/src/spec/controllers/providers_controller_spec.rb
index 42c6560..6b4ad18 100644
--- a/src/spec/controllers/providers_controller_spec.rb
+++ b/src/spec/controllers/providers_controller_spec.rb
@@ -198,8 +198,8 @@ describe ProvidersController do
             post :create, :provider => { :name => provider.name,
               :url => provider.url,
               :provider_type => {
-                :id => provider.provider_type.id
-              }
+              :id => provider.provider_type.id
+            }
             }
           end
 
@@ -222,7 +222,15 @@ describe ProvidersController do
           end
 
           context "with incorrect parameters" do
-            let(:provider) { FactoryGirl.build(:invalid_provider) }
+            let(:provider_type_id) do
+              provider_type = FactoryGirl.create(:provider_type)
+              id = provider_type.id
+            end
+            let(:provider) do
+              provider = FactoryGirl.build(:invalid_provider, :provider_type_id => provider_type_id)
+              provider.provider_type.destroy
+              provider
+            end
 
             it_behaves_like "http Bad Request"
             it_behaves_like "responding with XML"
@@ -288,14 +296,14 @@ describe ProvidersController do
               let(:provider) { FactoryGirl.build(:mock_provider) }
               let(:provider_data) {
                 {
-                  :name => provider.name,
-                  :url => provider.url,
-                  :provider_type =>
-                    {
-                      :id => provider.provider_type.id
-                    }
+                :name => provider.name,
+                :url => provider.url,
+                :provider_type =>
+                {
+                  :id => provider.provider_type.id
                 }
               }
+              }
 
               it_behaves_like "http OK"
               it_behaves_like "responding with XML"
@@ -317,13 +325,13 @@ describe ProvidersController do
               let(:other_provider) { FactoryGirl.create(:mock_provider) }
               let(:provider_data) {
                 {
-                  :name => other_provider.name,
-                  :provider_type =>
-                    {
-                      :id => provider.provider_type.id
-                    }
+                :name => other_provider.name,
+                :provider_type =>
+                {
+                  :id => provider.provider_type.id
                 }
               }
+              }
 
               it_behaves_like "http Bad Request"
               it_behaves_like "responding with XML"
-- 
1.7.7.6




More information about the aeolus-devel mailing list