----- Forwarded Message ----- From: "Martyn Taylor" <[email protected]> To: [email protected] Sent: Wednesday, October 13, 2010 11:53:30 AM GMT +00:00 GMT Britain, Ireland, Portugal Subject: Re: [deltacloud-devel] [PATCH cloud engine] Handle errors when adding a provider gracefully
Nack on this I'm afraid. Applies fine and all works hunky dory, the only thing I feel needs changing is the way that the error message is displayed, I think this should follow the same pattern other messages using flash messages to display Other than that works fine and applys Cheers Martyn ----- Original Message ----- From: [email protected] To: [email protected] Sent: Wednesday, October 13, 2010 10:35:52 AM GMT +00:00 GMT Britain, Ireland, Portugal Subject: [deltacloud-devel] [PATCH cloud engine] Handle errors when adding a provider gracefully From: Tomas Sedovic <[email protected]> When a user didn't enter a correct URL while adding a new provider, an unintelligible error was displayed and no way to correct it. This fix shown the propper error messages and leaves the user on the 'add provider' form to fix it. --- src/app/controllers/provider_controller.rb | 5 +- src/app/models/provider.rb | 3 +- src/app/stylesheets/aggregator.scss | 22 +++++----- src/app/views/provider/_form.haml | 62 ++++++++++++++++++++-------- src/app/views/provider/show.haml | 44 +------------------- src/config/locales/en.yml | 2 +- src/config/navigation.rb | 2 +- 7 files changed, 64 insertions(+), 76 deletions(-) diff --git a/src/app/controllers/provider_controller.rb b/src/app/controllers/provider_controller.rb index 5e4d32c..d338214 100644 --- a/src/app/controllers/provider_controller.rb +++ b/src/app/controllers/provider_controller.rb @@ -52,9 +52,10 @@ class ProviderController < ApplicationController def create require_privilege(Privilege::PROVIDER_MODIFY) + @providers = Provider.list_for_user(@current_user, Privilege::PROVIDER_MODIFY) @provider = Provider.new(params[:provider]) - if @provider.set_cloud_type && @provider.save && - @provider.populate_hardware_profiles + @provider.set_cloud_type + if @provider.save && @provider.populate_hardware_profiles flash[:notice] = "Provider added." redirect_to :action => "show", :id => @provider else diff --git a/src/app/models/provider.rb b/src/app/models/provider.rb index f5701aa..8ac99cd 100644 --- a/src/app/models/provider.rb +++ b/src/app/models/provider.rb @@ -56,7 +56,8 @@ class Provider < ActiveRecord::Base end def set_cloud_type - self.cloud_type = connect.driver_name + deltacloud = connect + self.cloud_type = deltacloud.driver_name unless deltacloud.nil? end def connect diff --git a/src/app/stylesheets/aggregator.scss b/src/app/stylesheets/aggregator.scss index e2b316b..6a3cd7d 100644 --- a/src/app/stylesheets/aggregator.scss +++ b/src/app/stylesheets/aggregator.scss @@ -839,6 +839,17 @@ fieldset.gap { margin-bottom: 7em; } +.fieldWithErrors { + display: inline-block; + border: 0; margin: 0; padding: 0; + input { + background-color: lighten($errorcl, 45%); + color: $errorcl; + } + label { + color: $errorcl; + } +} /* simple two column label + input pairs */ .dcloud_form { fieldset { @@ -855,17 +866,6 @@ fieldset.gap { display: inline-block; width: 20em; } - .fieldWithErrors { - display: inline-block; - border: 0; margin: 0; padding: 0; - input { - background-color: lighten($errorcl, 45%); - color: $errorcl; - } - label { - color: $errorcl; - } - } } .indented { margin: 10px 0 0; diff --git a/src/app/views/provider/_form.haml b/src/app/views/provider/_form.haml index e338a60..d0d1464 100644 --- a/src/app/views/provider/_form.haml +++ b/src/app/views/provider/_form.haml @@ -1,17 +1,45 @@ -.dcloud_form - = error_messages_for 'provider' - %h2 Add a cloud provider - %br/ - - form_tag :controller => :provider, :action => 'create' do - %ul - %li - %label - Name - %span Provide a descriptive name for this provider connection. - = text_field :provider, :name, :class => "txtfield" - %li - %label - URL - %span Enter the URL of the cloud provider. - = text_field:provider, :url, :class => "txtfield" - = submit_tag "Save", :class => "submit" +- readonly = controller.action_name == 'show' ? true : false +- new_provider = ['new', 'create'].include? controller.action_name += render :partial => 'providers' +#details.grid_13 + %nav.subsubnav + = render_navigation(:level => 4) + - if new_provider + - form_action = 'create' + - elsif controller.action_name == 'edit' + - form_action = 'update' + - form_for @provider, :url => {:controller => :provider, :action => form_action}, :class => "dcloud_form" do |f| + %fieldset + %label.grid_4.alpha.big{ :for => "provider_name" } + = t('.provider_name') + - unless readonly + %span.required + * + %label.grid_5.big{ :for => "provider_url" } + = t('.provider_url') + - unless readonly + %span.required + * + %div.grid_4.omega + = f.error_message_on :url, 'URL ' + = f.error_message_on :name, 'Name ' + = f.text_field :name, :title => t('.provider_name'), :value => (@provider.name if @provider), :disabled => ('disabled' if controller.action_name == 'show'), :class => "clear grid_4 alpha" + = f.text_field :url, :title => t('.provider_url'), :class => 'emailinput', :value => (@provider.url if @provider), :disabled => ('disabled' unless new_provider), :class => "grid_5" + - if controller.action_name == 'edit': + = f.hidden_field :id, :value => @provider.id + .clear.prefix_4.grid_5.suffix_4.alpha.omega + %span + ( + %a{ :href => ''}<> + = t('.test_connection') + ) + - unless readonly + %p.requirement + %span.required + * + \- + = t('.required_field') + - if controller.action_name == 'edit' + %input{ :type => 'submit', :value => t(:save), :name => 'save_provider', :id => 'save_provider' } + - elsif new_provider + %input{ :type => 'submit', :value => t(:add), :name => 'add_provider', :id => 'add_provider' } diff --git a/src/app/views/provider/show.haml b/src/app/views/provider/show.haml index 81e556b..c66fabf 100644 --- a/src/app/views/provider/show.haml +++ b/src/app/views/provider/show.haml @@ -1,43 +1 @@ -- readonly = controller.action_name == 'show' ? true : false -= render :partial => 'providers' -#details.grid_13 - %nav.subsubnav - = render_navigation(:level => 4) - - if controller.action_name == 'new' - - form_action = 'create' - - elsif controller.action_name == 'edit' - - form_action = 'update' - - form_tag :controller => :provider, :action => form_action, :class => "dcloud_form" do - %fieldset - %label.grid_4.alpha.big{ :for => "provider_name" } - = t('.provider_name') - - unless readonly - %span.required - * - %label.grid_5.big{ :for => "provider_url" } - = t('.provider_url') - - unless readonly - %span.required - * - /for error messages - %div.grid_4.omega - = text_field :provider, :name, :title => t('.provider_name'), :value => (@provider.name if @provider), :disabled => ('disabled' if controller.action_name == 'show'), :class => "clear grid_4 alpha" - = text_field :provider, :url, :title => t('.provider_url'), :class => 'emailinput', :value => (@provider.url if @provider), :disabled => ('disabled' unless controller.action_name == 'new'), :class => "grid_5" - - if controller.action_name == 'edit': - = hidden_field :provider, :id, :value => @provider.id - .clear.prefix_4.grid_5.suffix_4.alpha.omega - %span - ( - %a{ :href => ''}<> - = t('.test_connection') - ) - - unless readonly - %p.requirement - %span.required - * - \- - = t('.required_field') - - if controller.action_name == 'edit' - %input{ :type => 'submit', :value => t(:save), :name => 'save_provider', :id => 'save_provider' } - - elsif controller.action_name == 'new' - %input{ :type => 'submit', :value => t(:add), :name => 'add_provider', :id => 'add_provider' } += render :partial => 'form' diff --git a/src/config/locales/en.yml b/src/config/locales/en.yml index 95b7d13..0d8cc39 100644 --- a/src/config/locales/en.yml +++ b/src/config/locales/en.yml @@ -89,7 +89,7 @@ en: account: Account provider: providers: PROVIDERS - show: + form: provider_name: Provider Name provider_url: Provider URL caution_alt_text: Caution diff --git a/src/config/navigation.rb b/src/config/navigation.rb index 9580396..fdf521a 100644 --- a/src/config/navigation.rb +++ b/src/config/navigation.rb @@ -8,7 +8,7 @@ SimpleNavigation::Configuration.run do |navigation| first_level.item :administration, t(:administration), '#', :class => 'administration' do |second_level| second_level.item :system_settings, t(:system_settings), :controller => 'settings' do |third_level| third_level.item :manage_providers, t(:manage_providers), :controller => 'provider' do |fourth_level| - fourth_level.item :provider_summary, t(:provider_summary), { :controller => 'provider', :action => 'show', :id => (@provider.id if @provider) }, :highlights_on => /\/provider\/(show|edit|new)/ + fourth_level.item :provider_summary, t(:provider_summary), { :controller => 'provider', :action => 'show', :id => (@provider.id if @provider) }, :highlights_on => /\/provider\/(show|edit)/ fourth_level.item :provider_accounts, t(:provider_accounts), { :controller => 'provider', :action => 'accounts', :id => (@provider.id if @provider) }, :highlights_on => /\/provider\/accounts/ fourth_level.item :scheduling_policies, t(:scheduling_policies), '#' fourth_level.item :services_provided, t(:services_provided), '#' -- 1.7.2.3 _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel
