[PATCH] BZ 839145 added check for empty image name; re-organized error handling for new image

Matt Wagner matt.wagner at redhat.com
Thu Aug 30 17:05:23 UTC 2012


On Tue, Aug 21, 2012 at 12:01:50PM -0400, Tzu-Mainn Chen wrote:
> ---
>  src/app/controllers/images_controller.rb |   18 ++++++++++++++----
>  src/config/locales/en.yml                |    1 +
>  2 files changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/src/app/controllers/images_controller.rb b/src/app/controllers/images_controller.rb
> index f04307d..c9e238c 100644
> --- a/src/app/controllers/images_controller.rb
> +++ b/src/app/controllers/images_controller.rb
> @@ -230,24 +230,34 @@ class ImagesController < ApplicationController
>      @environment = PoolFamily.find(params[:environment])
>      check_permissions
>      @name = params[:name]
> +    errors = Array.new

This is perfectly valid Ruby, but I prefer "errors = []" and think
that's more common and more "Ruby-ish." The two are equivalent.

> +
> +    if @name.empty?
> +      errors << t('images.flash.error.no_name')
> +    end
>  
>      if params.has_key? :image_url
>        url = params[:image_url]
>        begin
>          xml_source = RestClient.get(url, :accept => :xml)
>        rescue RestClient::Exception, SocketError, URI::InvalidURIError, Errno::ECONNREFUSED, Errno::ETIMEDOUT
> -        flash.now[:error] = t('images.flash.error.invalid_url')
> -        render :new and return
> +        errors << t('images.flash.error.invalid_url')
>        end
>      else
>        file = params[:image_file]
>        xml_source = file && file.read
>        unless xml_source
> -        flash.now[:error] = t('images.flash.error.no_file')
> -        render :new and return
> +        errors << t('images.flash.error.no_file')
>        end
>      end
>  
> +    if !errors.empty?

Also some degree of personal opinion, but negative conditionals are evil
and confusing. (Granted, "If 'errors' aren't empty" is pretty easy to
understand here.) So in an ideal world I think this would read "if
errors.present?" or "if errors.any?".

> +      @accounts = @environment.provider_accounts.enabled.
> +        list_for_user(current_session, current_user, Privilege::USE)
> +      flash.now[:error] = errors
> +      render :new and return
> +    end
> +
>      begin
>        doc = TemplateXML.new(xml_source)
>      rescue Nokogiri::XML::SyntaxError => e
> diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
> index 178d5aa..dae0387 100644
> --- a/src/config/locales/en.yml
> +++ b/src/config/locales/en.yml
> @@ -965,6 +965,7 @@ en:
>          not_found: Image not found
>        error:
>          invalid_url: Could not load the provided URL
> +        no_name: You must specify the Image Template name
>          no_file: You must specify the Image Template XML file
>          no_template: "The Image doesn't have an Image Template because it was originally imported, rather than built in Conductor."
>          no_provider_accounts: "Images cannot be built. There are no enabled Provider Accounts associated with this Environment."
> -- 
> 1.7.6.5

ACK. I have two nits above, but both are just opinions on syntax, so I'm
going to push this unaltered.

-- Matt



More information about the aeolus-devel mailing list