[PATCH conductor] Redmine #3210: Conductor should validate the provider, not just deltacloud-core instance.

Richard Su rwsu at redhat.com
Fri May 4 22:21:31 UTC 2012


On 05/02/2012 01:45 PM, Jason Guiditta wrote:
> https://www.aeolusproject.org/redmine/issues/3210
>
> This patch adds a new method to the provider validation to verify
> that the named provider is valid for a given deltacloud instance. In
> addition, it moves this check out of the controller and down to the
> model where it belongs on create action. It also adds errors to the
> activerecord/en.yml, running tests on that value rather than a string.
> ---
>   src/app/controllers/providers_controller.rb     |   17 ++++--------
>   src/app/models/provider.rb                      |   11 +++++++-
>   src/config/locales/activerecord/en.yml          |    4 +++
>   src/features/provider.feature                   |   22 +++------------
>   src/features/step_definitions/provider_steps.rb |   31 ++++++++++++++++++++--
>   src/features/support/custom.rb                  |    4 +++
>   src/spec/factories/provider.rb                  |    1 -
>   src/spec/models/provider_spec.rb                |   18 ++++++++-----
>   src/spec/spec_helper.rb                         |    4 +++
>   9 files changed, 72 insertions(+), 40 deletions(-)
>
> diff --git a/src/app/controllers/providers_controller.rb b/src/app/controllers/providers_controller.rb
> index 1335292..41cf5d5 100644
> --- a/src/app/controllers/providers_controller.rb
> +++ b/src/app/controllers/providers_controller.rb
> @@ -104,18 +104,13 @@ class ProvidersController<  ApplicationController
>         params[:provider][:provider_type_id] = provider_type.id
>       end
>       @provider = Provider.new(params[:provider])
> -    if !@provider.connect
> -      flash[:warning] = t"providers.flash.warning.connect_failed"
> -      render :action =>  "new"
> +    if @provider.save
> +      @provider.assign_owner_roles(current_user)
> +      flash[:notice] = t"providers.flash.notice.added"
> +      redirect_to edit_provider_path(@provider)
>       else
> -      if @provider.save
> -        @provider.assign_owner_roles(current_user)
> -        flash[:notice] = t"providers.flash.notice.added"
> -        redirect_to edit_provider_path(@provider)
> -      else
> -        flash[:warning] = t"providers.flash.error.not_added"
> -        render :action =>  "new"
> -      end
> +      flash[:warning] = t"providers.flash.error.not_added"
> +      render :action =>  "new"
>       end
>     end
>
> diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb
> index 2033a04..fc3040e 100644
> --- a/src/app/models/provider.rb
> +++ b/src/app/models/provider.rb
> @@ -251,7 +251,8 @@ class Provider<  ActiveRecord::Base
>
>     def validate_provider
>       if !nil_or_empty(url)
> -      errors.add("url", "must be a valid provider url") unless valid_framework?
> +      errors.add("url", I18n.t("activerecord.errors.provider.url")) unless valid_framework?
> +      errors.add("deltacloud_provider", I18n.t("activerecord.errors.provider.deltacloud_provider")) unless valid_provider?
>       end
>     end
>
> @@ -261,4 +262,12 @@ class Provider<  ActiveRecord::Base
>       connect.nil? ? false : true
>     end
>
> +  def valid_provider?
> +    if !nil_or_empty(deltacloud_provider)
> +      client = connect
> +      return false if client.nil?
> +      return false unless client.valid_provider? deltacloud_provider
> +    end
> +    true
> +  end
>   end
> diff --git a/src/config/locales/activerecord/en.yml b/src/config/locales/activerecord/en.yml
> index 3ac086f..86b250f 100644
> --- a/src/config/locales/activerecord/en.yml
> +++ b/src/config/locales/activerecord/en.yml
> @@ -343,3 +343,7 @@ en:
>           state: "State"
>           created_at: "Created At"
>           updated_at: "Updated At"
> +    errors:
> +      provider:
> +        url: "Must be a valid provider uri"
> +        deltacloud_provider: "Must be a valid provider identifier"
> diff --git a/src/features/provider.feature b/src/features/provider.feature
> index b1df22e..2ba7081 100644
> --- a/src/features/provider.feature
> +++ b/src/features/provider.feature
> @@ -46,27 +46,15 @@ Feature: Manage Providers
>       And I should see "Provider URL"
>
>     Scenario: Create a new Provider
> -    Given I am on the providers page
> -    And there is not a provider named "testprovider"
> -    When I follow "create_new_provider"
> -    Then I should be on the new provider page
> -    When I fill in "provider[name]" with "testprovider"
> -    And I fill in "provider[url]" with "http://localhost:3002/api"
> -    And I select "Amazon EC2" from "provider_provider_type_id"
> -    And I press "save"
> +    Given I am on the new provider page
> +    And I attempt to add a valid provider
>       Then I should be on the testprovider's edit provider page
> -    And I should see "Provider added"
> +    And I should see a confirmation message
>       And I should have a provider named "testprovider"
>
>     Scenario: Create a new Provider failure when using wrong url
> -    Given I am on the providers page
> -    And there is not a provider named "testprovider"
> -    When I follow "create_new_provider"
> -    Then I should be on the new provider page
> -    When I fill in "provider[name]" with "testprovider"
> -    And I fill in "provider[url]" with "http://localhost:3010/api"
> -    And I select "Amazon EC2" from "provider_provider_type_id"
> -    And I press "save"
> +    Given I am on the new provider page
> +    And I attempt to add a provider with an invalid url
>       Then I should be on the providers page
>       And I should see a warning message
>
> diff --git a/src/features/step_definitions/provider_steps.rb b/src/features/step_definitions/provider_steps.rb
> index fc25f87..605f49c 100644
> --- a/src/features/step_definitions/provider_steps.rb
> +++ b/src/features/step_definitions/provider_steps.rb
> @@ -13,13 +13,30 @@
>   #   See the License for the specific language governing permissions and
>   #   limitations under the License.
>   #
> +def new_provider(port,bool)
> +  destroy_provider("testprovider")
> +  stub_framework(bool)
> +  fill_in "provider[name]", :with =>  "testprovider"
> +  fill_in "provider[url]", :with =>  "http://localhost:#{port}/api"
> +  select("Amazon EC2", :from =>  "provider_provider_type_id")
> +  click_button "save"
> +end
> +
> +def destroy_provider(name)
> +  provider = Provider.find_by_name(name)
> +  if provider then provider.destroy end
> +end
> +
> +def stub_framework(bool)
> +  Provider.any_instance.stub(:valid_framework?).and_return(bool)
> +end
> +
>   Given /^there should not exist a provider named "([^\"]*)"$/ do |name|
>     Provider.find_by_name(name).should be_nil
>   end
>
>   Given /^there is not a provider named "([^"]*)"$/ do |name|
> -  provider = Provider.find_by_name(name)
> -  if provider then provider.destroy end
> +  destroy_provider(name)
>   end
>
>   Given /^there is a provider named "([^\"]*)"$/ do |name|
> @@ -27,7 +44,15 @@ Given /^there is a provider named "([^\"]*)"$/ do |name|
>   end
>
>   Given /^provider "([^"]*)" is not accessible$/ do |arg1|
> -  Provider.any_instance.stub(:valid_framework?).and_return(false)
> +  stub_framework(false)
> +end
> +
> +Given /^I attempt to add a valid provider$/ do
> +  new_provider(3002,true)
> +end
> +
> +Given /^I attempt to add a provider with an invalid url$/ do
> +  new_provider(3010,false)
>   end
>
>   Then /^I should have a provider named "([^\"]*)"$/ do |name|
> diff --git a/src/features/support/custom.rb b/src/features/support/custom.rb
> index a02427c..e529073 100644
> --- a/src/features/support/custom.rb
> +++ b/src/features/support/custom.rb
> @@ -23,6 +23,10 @@ Provider.class_eval do
>       true
>     end
>
> +  def valid_provider?
> +    true
> +  end
> +
>     def populate_realms
>       true
>     end
> diff --git a/src/spec/factories/provider.rb b/src/spec/factories/provider.rb
> index de6092d..a85b73b 100644
> --- a/src/spec/factories/provider.rb
> +++ b/src/spec/factories/provider.rb
> @@ -33,7 +33,6 @@ FactoryGirl.define do
>       name 'mock2'
>       provider_type { ProviderType.find_by_deltacloud_driver("mock") }
>       url 'http://localhost:3002/api'
> -    deltacloud_provider 'mock'
>       after_create { |p| p.realms<<  FactoryGirl.create(:realm3, :provider =>  p) }
>     end
>
> diff --git a/src/spec/models/provider_spec.rb b/src/spec/models/provider_spec.rb
> index 11cf71e..4d07f9d 100644
> --- a/src/spec/models/provider_spec.rb
> +++ b/src/spec/models/provider_spec.rb
> @@ -56,17 +56,21 @@ describe Provider do
>         end
>       end
>
> -    it "should be able to connect to the specified framework" do
> -      @provider.should be_valid
> -      @provider.connect.should_not be_nil
> -
> +    it "should set error if unable to connect to specified deltacloud instance" do
> +      @provider.stub(:valid_provider?).and_return(true)
>         @provider.url = "http://invalid.provider/url"
>         @provider.stub(:connect).and_return(nil)
> -      deltacloud = @provider.connect
>         @provider.should have(1).error_on(:url)
> -      @provider.errors[:url].first.should eql("must be a valid provider url")
> +      @provider.errors[:url].first.should eql(I18n.t("activerecord.errors.provider.url"))
> +      @provider.should_not be_valid
> +    end
> +
> +    it "should set error if unable to connect to specified deltacloud_provider" do
> +      @provider.stub(:valid_provider?).and_return(false)
> +      @provider.stub(:valid_framework?).and_return(true)
> +      @provider.should have(1).error_on(:deltacloud_provider)
> +      @provider.errors[:deltacloud_provider].first.should eql(I18n.t("activerecord.errors.provider.deltacloud_provider"))
>         @provider.should_not be_valid
> -      deltacloud.should be_nil
>       end
>
>       it "should require unique name" do
> diff --git a/src/spec/spec_helper.rb b/src/spec/spec_helper.rb
> index 09bb9f3..00cb1fd 100644
> --- a/src/spec/spec_helper.rb
> +++ b/src/spec/spec_helper.rb
> @@ -81,6 +81,10 @@ RSpec.configure do |config|
>     #  activate_authlogic
>     #end
>
> +  config.before(:each) do
> +    Provider.any_instance.stub(:valid_provider?).and_return(true)
> +    Provider.any_instance.stub(:valid_famework?).and_return(true)
> +  end
>     #config.after(:each, :type =>  :controller) do
>     #  current_user_session.destroy
>     #end
Ack and pushed.



More information about the aeolus-devel mailing list