[PATCH conductor] Strip whitespace from image id when importing images.

Matt Wagner matt.wagner at redhat.com
Thu Aug 9 14:51:38 UTC 2012


On Thu, Aug 09, 2012 at 01:29:23PM +0200, jstransk at redhat.com wrote:
> From: Jiri Stransky <jstransk at redhat.com>
> 
> This fixes https://bugzilla.redhat.com/show_bug.cgi?id=819059
> ---
>  src/app/controllers/images_controller.rb       |    2 +-
>  src/spec/controllers/images_controller_spec.rb |   49 ++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 1 deletions(-)
>  create mode 100644 src/spec/controllers/images_controller_spec.rb
> 
> diff --git a/src/app/controllers/images_controller.rb b/src/app/controllers/images_controller.rb
> index c3ca98c..00ae854 100644
> --- a/src/app/controllers/images_controller.rb
> +++ b/src/app/controllers/images_controller.rb
> @@ -194,7 +194,7 @@ class ImagesController < ApplicationController
>  
>      xml = "<image><name>#{params[:name]}</name></image>" unless params[:name].blank?
>      begin
> -      image = Image.import(account, params[:image_id], @environment, xml)
> +      image = Image.import(account, params[:image_id].strip, @environment, xml)
>        flash[:success] = t("images.import.image_imported")
>        redirect_to image_url(image.id) and return
>      rescue Exception => e
> diff --git a/src/spec/controllers/images_controller_spec.rb b/src/spec/controllers/images_controller_spec.rb
> new file mode 100644
> index 0000000..a684a0d
> --- /dev/null
> +++ b/src/spec/controllers/images_controller_spec.rb
> @@ -0,0 +1,49 @@
> +#
> +#   Copyright 2012 Red Hat, Inc.
> +#
> +#   Licensed under the Apache License, Version 2.0 (the "License");
> +#   you may not use this file except in compliance with the License.
> +#   You may obtain a copy of the License at
> +#
> +#       http://www.apache.org/licenses/LICENSE-2.0
> +#
> +#   Unless required by applicable law or agreed to in writing, software
> +#   distributed under the License is distributed on an "AS IS" BASIS,
> +#   WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> +#   See the License for the specific language governing permissions and
> +#   limitations under the License.
> +#
> +
> +require 'spec_helper'
> +
> +describe ImagesController do
> +
> +  fixtures :all
> +  before(:each) do
> +    @admin_permission = FactoryGirl.create :admin_permission
> +    @admin = @admin_permission.user
> +    @provider_account = FactoryGirl.create :mock_provider_account
> +    @pool_family = FactoryGirl.create :pool_family
> +  end
> +
> +  describe "#import" do
> +
> +    it "strips whitespace off the image id provided by the user" do
> +      ProviderAccount.stub(:find).and_return(@provider_account)
> +      PoolFamily.stub(:find).and_return(@pool_family)
> +      @image = mock('image')
> +      @image.stub(:id).and_return('456def')
> +
> +      Image.should_receive(:import).
> +            with { |provider_account, image_id, pool_family, xml| image_id == 'Mock_mock_123abc' }.
> +            and_return(@image)
> +
> +      mock_warden(@admin)
> +      post(:import, :image_id => ' Mock_mock_123abc   ',
> +           :name => 'imported',
> +           :provider_account => '1',
> +           :environment => 'some_environment')
> +    end
> +
> +  end
> +end

ACK, looks good! I have just pushed this to master.

It's been suggested that in controller tests, we should be using
mock_model[1] since we don't care about exercising actual model
functionality. However, none of our tests are currently doing this, so
this isn't a criticism of your patch -- it's good as-is! -- as much as a
note about something we as a team should consider switching to. :)

[1]
https://www.relishapp.com/rspec/rspec-rails/v/2-4/docs/mocks/mock-model



More information about the aeolus-devel mailing list