[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