[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