This patchset fixes the bugs described in:
https://bugzilla.redhat.com/show_bug.cgi?id=640257
As the bug report combines three distinct issues, I decided to split the fix into three parts to make the eventual commit history more readable.
[PATCH cloud engine 1/3] BZ640257: wrong tooltips in Provider Accounts page [PATCH cloud engine 2/3] BZ640257: "Reset" button on the Cloud Account page [PATCH cloud engine 3/3] BZ640257: Required fields in cloud-accounts page
From: Tomas Sedovic tsedovic@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=640257
The "EC2 Access Key" and "Secret Acces Key" fields had an untintelligible `title` attribute because the i18n engine was looking for a dictionary fields that were missing. --- src/app/views/provider/accounts.haml | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/app/views/provider/accounts.haml b/src/app/views/provider/accounts.haml index 509e727..c406dd2 100644 --- a/src/app/views/provider/accounts.haml +++ b/src/app/views/provider/accounts.haml @@ -31,8 +31,8 @@ - disabled = 'disabled' unless has_account_modify?(@provider) - cloud_account_id = "cloud_accounts[#{acct.id}]" = text_field cloud_account_id, :label, :title => t('.account_name'), :value => acct.label, :disabled => disabled, :class => "grid_4 alpha" - = text_field cloud_account_id, :username, :title => t('.access_key'), :value => acct.username, :disabled => disabled, :class => "grid_3" - = password_field cloud_account_id, :password, :title => t('.secret_access_key'), :disabled => disabled, :class => "grid_3" + = text_field cloud_account_id, :username, :title => t('.user_name'), :value => acct.username, :disabled => disabled, :class => "grid_3" + = password_field cloud_account_id, :password, :title => t('.password'), :disabled => disabled, :class => "grid_3" = text_field "quota[#{acct.id}]", :maximum_running_instances, :title => t('.quota_instances'), :value => (acct.quota.maximum_running_instances.nil? ? "unlimited" : acct.quota.maximum_running_instances), :disabled => disabled, :id => "quota_instances#{acct.id}", :class => "grid_3 omega" %fieldset.nomargin.clearfix .grid_3.prefix_10.alpha.omega @@ -84,8 +84,8 @@ %span.required * %fieldset.nomargin.clearfix = text_field :cloud_account, :label, :title => t('.account_name'), :class => "grid_4 alpha" - = text_field :cloud_account, :username, :title => t('.access_key'), :class => "grid_3" - = password_field :cloud_account, :password, :title => t('.secret_access_key'), :class => "grid_3" + = text_field :cloud_account, :username, :title => t('.user_name'), :class => "grid_3" + = password_field :cloud_account, :password, :title => t('.password'), :class => "grid_3" = text_field "quota", :maximum_running_instances, :title => t('.quota_instances'), :value => "unlimited",:id => "quota_instances", :class => "grid_3 omega" %fieldset.nomargin.clearfix .grid_3.prefix_10.alpha.omega
From: Tomas Sedovic tsedovic@redhat.com
https://bugzilla.redhat.com/show_bug.cgi?id=640257
Previously, pressing the Reset button saved the entered input instead of resetting the previous values. --- src/app/controllers/cloud_accounts_controller.rb | 48 +++++++++++---------- 1 files changed, 25 insertions(+), 23 deletions(-)
diff --git a/src/app/controllers/cloud_accounts_controller.rb b/src/app/controllers/cloud_accounts_controller.rb index 9d75347..c8e94fe 100644 --- a/src/app/controllers/cloud_accounts_controller.rb +++ b/src/app/controllers/cloud_accounts_controller.rb @@ -71,31 +71,33 @@ class CloudAccountsController < ApplicationController end
def update_accounts - params[:cloud_accounts].each do |id, attributes| - @cloud_account = CloudAccount.find(id) - require_privilege(Privilege::ACCOUNT_MODIFY, @cloud_account.provider) + 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! end redirect_to :controller => 'provider', :action => 'accounts', :id => params[:provider][:id] end
From: Tomas Sedovic tsedovic@redhat.com
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(-)
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
tsedovic@redhat.com wrote:
From: Tomas Sedovic tsedovic@redhat.com
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
elsekick_condor
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
end endredirect_to :controller => "provider", :action => "accounts", :id => @provider, :cloud_account => params[:cloud_account]
@@ -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
end redirect_to :controller => 'provider', :action => 'accounts', :id => params[:provider][:id] endflash[:notice] = "Error updating the cloud account."
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
end endt.text :x509_cert_pub, :null => false t.timestamps
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@lists.fedorahosted.org