On 08/21/2012 08:34 PM, [email protected] wrote:
> From: Jozef Zigmund <[email protected]>
>
> When user made typo in URL like 'http;//' instead of 'http://'
> then he got "undefined method `request_uri' for
> #<URI::Generic:0x7fb611605c60>"
>
> https://bugzilla.redhat.com/show_bug.cgi?id=811852
> ---
> src/app/controllers/images_controller.rb | 28 +++++++++++++++++++---------
> src/config/locales/en.yml | 1 +
> 2 files changed, 20 insertions(+), 9 deletions(-)
>
> diff --git a/src/app/controllers/images_controller.rb
> b/src/app/controllers/images_controller.rb
> index d9ab289..7be1849 100644
> --- a/src/app/controllers/images_controller.rb
> +++ b/src/app/controllers/images_controller.rb
> @@ -216,21 +216,31 @@ class ImagesController < ApplicationController
> @environment = PoolFamily.find(params[:environment])
> check_permissions
> @name = params[:name]
> -
> 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
> + if params[:image_url].present?
> + url = params[:image_url]
> + uri = URI.parse(url)
> + if uri.kind_of?(URI::HTTP)
you can use simpler "url =~ URI::regexp" to test if url format is correct
> + begin
> + xml_source = RestClient.get(url, :accept => :xml)
> + rescue RestClient::Exception, SocketError, URI::InvalidURIError,
> Errno::ECONNREFUSED, Errno::ETIMEDOUT
> + flash[:error] = t('images.flash.error.invalid_url')
> + redirect_to new_image_path(:environment => @environment) and
> return
we should keep 'render' instead of 'redirect_to' - with redirect form data will
be reset (also any object error messages will be lost)
> + end
> + else
> + flash[:error] = t('images.flash.error.invalid_url')
> + redirect_to new_image_path(:environment => @environment) and return
> + end
> + else
> + flash[:error] = t('images.flash.error.no_url')
> + redirect_to new_image_path(:environment => @environment) and return
> 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
> + flash[:error] = t('images.flash.error.no_file')
> + redirect_to new_image_path(:environment => @environment) and return
> end
> end
>
> diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml
> index 8d2fb7d..99765c3 100644
> --- a/src/config/locales/en.yml
> +++ b/src/config/locales/en.yml
> @@ -968,6 +968,7 @@ en:
> 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."
> no_provider_accounts_for_import: "Images cannot be imported. No
> Provider Accounts are currently enabled for this Environment."
> + no_url: URL for Image Template XML can't be blank
> new:
> new_image: New Image
> description:
>
Hi,
as we already discussed, it would be nice to DRY this "import from xml" logic
which is similar for both controllers - you might create protected method in
application_controller, something like below. (+ a few comments inline)
def import_xml_from_url(url)
if url.blank?
flash[:error] = t('import_xml.flash.warning.no_url_provided')
elsif not url =~ URI::regexp
flash[:error] = t('import_xml.flash.warning.not_valid_or_reachable', :url
=> url)
else
begin
response = RestClient.get(url, :accept => :xml)
if response.code == 200
return response
else
# flash message here, that download failed
end
rescue RestClient::Exception, SocketError, URI::InvalidURIError,
Errno::ECONNREFUSED, Errno::ETIMEDOUT
flash[:error] = t('import_xml.flash.warning.not_valid_or_reachable',
:url => url)
end
end
return nil
end
Jan
