On Thu, Aug 09, 2012 at 01:29:23PM +0200, [email protected] wrote: > From: Jiri Stransky <[email protected]> > > 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
