[PATCH conductor 1/2] BZ 811852 - ImagesController#edit_xml: fixed undefined method 'request_uri'

Jan Provaznik jprovazn at redhat.com
Wed Aug 22 08:28:23 UTC 2012


On 08/21/2012 08:34 PM, jzigmund at redhat.com wrote:
> From: Jozef Zigmund <jzigmund at redhat.com>
>
> 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



More information about the aeolus-devel mailing list