[email protected] wrote:
> From: Tomas Sedovic <[email protected]>
>
> https://bugzilla.redhat.com/show_bug.cgi?id=640257
>
> All the fields that are marked as "required" are now being checked and when
> the user doesn't put anything in them, the create/edit action fails.
> ---
> src/app/controllers/cloud_accounts_controller.rb | 83
> +++++++++++---------
> src/app/models/cloud_account.rb | 4 +
> .../20090801045212_create_cloud_accounts.rb | 8 +-
> src/spec/factories/cloud_account.rb | 4 +
> 4 files changed, 58 insertions(+), 41 deletions(-)
>
>
This seems to work for 'add' but is broken for 'edit'. If I edit an
account, password isn't displayed, so if I leave it blank (rather than
having to re-enter the whole thing) it fails validation. For 'edit'
operations (as opposed to 'add') we shouldn't require anything typed in
this field. Similar issue with the file upload widgets.
More generally, why are we using the custom "you must enter all required
fields" message (which doesn't say which fields) instead of relying on
the standard rails messages resulting from validates_presence_of. If we
use those the password/cert problems on 'edit' may fix themselves.
Scott
> diff --git a/src/app/controllers/cloud_accounts_controller.rb
> b/src/app/controllers/cloud_accounts_controller.rb
> index c8e94fe..9e2bd3e 100644
> --- a/src/app/controllers/cloud_accounts_controller.rb
> +++ b/src/app/controllers/cloud_accounts_controller.rb
> @@ -44,23 +44,27 @@ class CloudAccountsController < ApplicationController
> if params[:test_account]
> test_account(@cloud_account)
> redirect_to :controller => "provider", :action => "accounts", :id =>
> @provider, :cloud_account => params[:cloud_account]
> + elsif @cloud_account.valid?
> + quota = Quota.new
> + quota.maximum_running_instances =
> quota_from_string(params[:quota][:maximum_running_instances])
> + quota.save!
> + @cloud_account.quota_id = quota.id
> + @cloud_account.zones << Zone.default
> + @cloud_account.save!
> + if request.post? && @cloud_account.save &&
> @cloud_account.populate_realms
> + flash[:notice] = "Provider account added."
> + end
> + redirect_to :controller => "provider", :action => "accounts", :id =>
> @provider
> + kick_condor
> else
> - unless @cloud_account.valid_credentials?
> + if not @cloud_account.valid_credentials?
> flash[:notice] = "The entered credential information is incorrect"
> - redirect_to :controller => "provider", :action => "accounts", :id =>
> @provider
> + elsif @cloud_account.errors.on(:username)
> + flash[:notice] = "The access key
> '#{params[:cloud_account][:username]}' has already been taken."
> else
> - quota = Quota.new
> - quota.maximum_running_instances =
> quota_from_string(params[:quota][:maximum_running_instances])
> - quota.save!
> - @cloud_account.quota_id = quota.id
> - @cloud_account.zones << Zone.default
> - @cloud_account.save!
> - if request.post? && @cloud_account.save &&
> @cloud_account.populate_realms
> - flash[:notice] = "Provider account added."
> - end
> - redirect_to :controller => "provider", :action => "accounts", :id =>
> @provider
> - kick_condor
> + flash[:notice] = "You must fill in all the required fields"
> end
> + redirect_to :controller => "provider", :action => "accounts", :id =>
> @provider, :cloud_account => params[:cloud_account]
> end
> end
>
> @@ -71,33 +75,38 @@ class CloudAccountsController < ApplicationController
> end
>
> def update_accounts
> - if params[:update_cloud_accounts]
> - params[:cloud_accounts].each do |id, attributes|
> - @cloud_account = CloudAccount.find(id)
> - require_privilege(Privilege::ACCOUNT_MODIFY, @cloud_account.provider)
> + begin
> + if params[:update_cloud_accounts]
> + params[:cloud_accounts].each do |id, attributes|
> + @cloud_account = CloudAccount.find(id)
> + require_privilege(Privilege::ACCOUNT_MODIFY,
> @cloud_account.provider)
>
> - # blank password means the user didn't change it -- don't update it
> then
> - if attributes.has_key? :password and
> String(attributes[:password]).empty?
> - attributes.delete :password
> - end
> - @cloud_account.quota.maximum_running_instances =
> quota_from_string(params[:quota][id][:maximum_running_instances])
> - private_cert = attributes[:x509_cert_priv_file]
> - if private_cert.nil?
> - attributes.delete :x509_cert_priv
> - else
> - attributes[:x509_cert_priv] = private_cert.read
> - attributes.delete :x509_cert_priv_file
> - end
> - public_cert = attributes[:x509_cert_pub_file]
> - if public_cert.nil?
> - attributes.delete :x509_cert_pub
> - else
> - attributes[:x509_cert_pub] = public_cert.read
> - attributes.delete :x509_cert_pub_file
> + # blank password means the user didn't change it -- don't update
> it then
> + if attributes.has_key? :password and
> String(attributes[:password]).empty?
> + attributes.delete :password
> + end
> + @cloud_account.quota.maximum_running_instances =
> quota_from_string(params[:quota][id][:maximum_running_instances])
> + private_cert = attributes[:x509_cert_priv_file]
> + if private_cert.nil?
> + attributes.delete :x509_cert_priv
> + else
> + attributes[:x509_cert_priv] = private_cert.read
> + attributes.delete :x509_cert_priv_file
> + end
> + public_cert = attributes[:x509_cert_pub_file]
> + if public_cert.nil?
> + attributes.delete :x509_cert_pub
> + else
> + attributes[:x509_cert_pub] = public_cert.read
> + attributes.delete :x509_cert_pub_file
> + end
> + @cloud_account.update_attributes!(attributes)
> + @cloud_account.quota.save!
> end
> - @cloud_account.update_attributes!(attributes)
> - @cloud_account.quota.save!
> + flash[:notice] = "Account updated."
> end
> + rescue
> + flash[:notice] = "Error updating the cloud account."
> end
> redirect_to :controller => 'provider', :action => 'accounts', :id =>
> params[:provider][:id]
> end
> diff --git a/src/app/models/cloud_account.rb b/src/app/models/cloud_account.rb
> index cbe4d9f..5673e9a 100644
> --- a/src/app/models/cloud_account.rb
> +++ b/src/app/models/cloud_account.rb
> @@ -32,9 +32,13 @@ class CloudAccount < ActiveRecord::Base
>
> validates_presence_of :provider_id
>
> + validates_presence_of :label
> validates_presence_of :username
> validates_uniqueness_of :username, :scope => :provider_id
> validates_presence_of :password
> + validates_presence_of :account_number
> + validates_presence_of :x509_cert_pub
> + validates_presence_of :x509_cert_priv
>
> has_many :permissions, :as => :permission_object, :dependent => :destroy,
> :include => [:role],
> diff --git a/src/db/migrate/20090801045212_create_cloud_accounts.rb
> b/src/db/migrate/20090801045212_create_cloud_accounts.rb
> index e1f2e7b..042c406 100644
> --- a/src/db/migrate/20090801045212_create_cloud_accounts.rb
> +++ b/src/db/migrate/20090801045212_create_cloud_accounts.rb
> @@ -22,15 +22,15 @@
> class CreateCloudAccounts < ActiveRecord::Migration
> def self.up
> create_table :cloud_accounts do |t|
> - t.string :label
> + t.string :label, :null => false
> t.string :username, :null => false
> t.string :password, :null => false
> t.integer :provider_id, :null => false
> t.integer :quota_id
> t.integer :lock_version, :default => 0
> - t.string :account_number
> - t.text :x509_cert_priv
> - t.text :x509_cert_pub
> + t.string :account_number, :null => false
> + t.text :x509_cert_priv, :null => false
> + t.text :x509_cert_pub, :null => false
> t.timestamps
> end
> end
> diff --git a/src/spec/factories/cloud_account.rb
> b/src/spec/factories/cloud_account.rb
> index 27d0a1e..797fa47 100644
> --- a/src/spec/factories/cloud_account.rb
> +++ b/src/spec/factories/cloud_account.rb
> @@ -1,6 +1,10 @@
> Factory.define :cloud_account do |f|
> f.sequence(:username) { |n| "testUser#(n)" }
> f.password "testPassword"
> + f.sequence(:label) { |n| "test label#(n)" }
> + f.account_number "3141"
> + f.x509_cert_priv "x509 private key"
> + f.x509_cert_pub "x509 public key"
> f.association :provider
> end
>
>
_______________________________________________
deltacloud-devel mailing list
[email protected]
https://fedorahosted.org/mailman/listinfo/deltacloud-devel
