Hey Scott, I agree we should remove the code form the Settings controller since the settings are now being removed.
I'm not too sure on the need for removing the Quota AR object. This object is just a place holder for any default values. Although we only have 1 default value at this stage, looking past PHASE1 we likely need multiple. Rather than specifying a Key, Pair for each resource we can just update the Quota. The default Quota explained above is never used as Quota and is not monitored, just place holder. A *new* quota is created each time a user registers, based on the given parameters of the held in the default Quota. I know we are short on time here and we likely can't afford another iteration. So I will go ahead and make all the changes we agree on then, if I've manage to convince you we to use AR Quota, then we should be fine, else I will go ahead and make the changes you suggested. Thanks Martyn ----- Original Message ----- From: "Scott Seago" <[email protected]> To: [email protected] Cc: [email protected] Sent: Thursday, September 23, 2010 6:31:16 AM GMT +00:00 GMT Britain, Ireland, Portugal Subject: Re: [deltacloud-devel] [PATCH aggregator 2/2] Cloud Engine Policy: Self Service Registration [email protected] wrote: > From: martyntaylor <[email protected]> > > --- > src/app/controllers/dashboard_controller.rb | 15 ++++-- > src/app/controllers/settings_controller.rb | 55 > +++++++++++++++++++- > src/app/controllers/users_controller.rb | 4 -- > src/app/services/registration_service.rb | 23 ++++---- > src/db/migrate/20090803141507_create_pools.rb | 5 ++ > .../20100707000000_create_metadata_objects.rb | 14 +++++ > src/features/authentication.feature | 19 ------- > src/features/support/hooks.rb | 18 ++++++- > src/spec/controllers/users_controller_spec.rb | 25 ++++++--- > src/spec/factories/metadata_object.rb | 14 ++++-- > src/spec/models/registration_service_spec.rb | 22 ++++++++ > 11 files changed, 161 insertions(+), 53 deletions(-) > > diff --git a/src/app/controllers/dashboard_controller.rb > b/src/app/controllers/dashboard_controller.rb > index 5a83efb..c5a842a 100644 > --- a/src/app/controllers/dashboard_controller.rb > +++ b/src/app/controllers/dashboard_controller.rb > @@ -67,11 +67,16 @@ class DashboardController < ApplicationController > # FIXME filter to just those that the user has access to > @cloud_accounts = CloudAccount.find(:all) > > - # FIXME remove general role based permission check, replace w/ > - # more granular / per-permission-object permission checks on the > - # dashboard in the future (here and in dashboard views) > - @is_admin = @current_user.permissions.collect { |p| p.role }. > - find { |r| r.name == "Administrator" } > + > + # Now need to check any permissions are set since default permission and > pool > + # may not be set for the admin user > + if @current_user.permissions > + # FIXME remove general role based permission check, replace w/ > + # more granular / per-permission-object permission checks on the > + # dashboard in the future (here and in dashboard views) > + @is_admin = @current_user.permissions.collect { |p| p.role }. > + find { |r| r.name == "Administrator" } > + end > > @hide_getting_started = true > #...@hide_getting_started = > cookies["#...@current_user.login}_hide_getting_started"] > diff --git a/src/app/controllers/settings_controller.rb > b/src/app/controllers/settings_controller.rb > index 32ef39f..21499e4 100644 > --- a/src/app/controllers/settings_controller.rb > +++ b/src/app/controllers/settings_controller.rb > @@ -22,13 +22,66 @@ > class SettingsController < ApplicationController > before_filter :require_user > > + # Settings MetaData Keys > + ALLOW_SELF_SERVICE_LOGINS = "allow_self_service_logins" > + SELF_SERVICE_DEFAULT_POOL = "self_service_default_pool" > + SELF_SERVICE_DEFAULT_ROLE = "self_service_default_role" > I think we can get rid of all three of the above. self-service login will be on a different page from this one (this page will appear as the "treatments" page), and the default pool/role will ultimately be set by adding a default user group, and this pool/role will be granted to the user group. > + SELF_SERVICE_DEFAULT_QUOTA = "self_service_default_quota" > + > + KEYS = [ALLOW_SELF_SERVICE_LOGINS, SELF_SERVICE_DEFAULT_POOL, > SELF_SERVICE_DEFAULT_ROLE, SELF_SERVICE_DEFAULT_QUOTA] > + > def index > + @is_admin = is_admin? > @providers = Provider.list_for_user(@current_user, > Privilege::PROVIDER_VIEW) > end > > def self_service > + if !is_admin? > + raise PermissionError.new('You have insufficient privileges to perform > action.') > + return > + end > + > + @pools = Pool.list_for_user(@current_user, Privilege::POOL_MODIFY) > + @self_service_default_pool = > MetadataObject.lookup(SELF_SERVICE_DEFAULT_POOL) > + @self_service_default_pool_id = @self_service_default_pool == nil ? nil > : @self_service_default_pool.id.to_i > + > + @roles = Role.all > + @self_service_default_role = > MetadataObject.lookup(SELF_SERVICE_DEFAULT_ROLE) > + @self_service_default_role_id = @self_service_default_role == nil ? nil > : @self_service_default_role.id.to_i > + > + @allow_self_service_logins = > MetadataObject.lookup(ALLOW_SELF_SERVICE_LOGINS) == "true" ? true : false > @providers = Provider.list_for_user(@current_user, > Privilege::PROVIDER_VIEW) > - @pools = Pool.list_for_user(@current_user, Privilege::POOL_VIEW) > + > Again I think we can dump pool/role/logins here. Jay is this consistent with your understanding of presenting this on the "treatment" page that only allows quota setting here? > + @self_service_default_quota = > MetadataObject.lookup(SELF_SERVICE_DEFAULT_QUOTA) > end > > + def update > + KEYS.each do |key| > + if params[key] > + if key == SELF_SERVICE_DEFAULT_QUOTA > + self_service_default_quota = MetadataObject.lookup(key) > + self_service_default_quota.update_attributes(params[key]) > We should track this as a plain value representing the value of the default quota, not an AR quota object The reason I chose to do this, is for looking past Phase1, when we have more than 1 resource to monitor on Quota. Rather than having multiple Key, Value pairs why don't we just hold everything together in the Quota. This also works for Phase1 too. Altho, the benefit is not obvious at this point. > + elsif key == SELF_SERVICE_DEFAULT_POOL > + if Pool.exists?(params[key]) > + MetadataObject.set(key, Pool.find(params[key])) > + end > + elsif key == SELF_SERVICE_DEFAULT_ROLE > + if Role.exists?(params[key]) > + MetadataObject.set(key, Role.find(params[key])) > + end > + else > + MetadataObject.set(key, params[key]) > + end > + end > + end > + > + flash[:notice] = "Settings Updated!" > + redirect_to :action => 'self_service' > + end > + > + private > + def is_admin? > + is_admin = @current_user.permissions.collect { |p| p.role }.find { |r| > r.name == "Administrator" } > + return is_admin == nil ? false : true > + end > end > diff --git a/src/app/controllers/users_controller.rb > b/src/app/controllers/users_controller.rb > index 8488509..f61c6d2 100644 > --- a/src/app/controllers/users_controller.rb > +++ b/src/app/controllers/users_controller.rb > @@ -31,10 +31,6 @@ class UsersController < ApplicationController > require_privilege(Privilege::USER_MODIFY) unless current_user.nil? > @user = User.new(params[:user]) > > - #TODO Set Quota Values to SelfService Settings Default Quota > - @user_quota = Quota.new > - @user.quota_id = @user_quota.id > - > @registration = RegistrationService.new(@user) > if @registration.save > flash[:notice] = "User registered!" > diff --git a/src/app/services/registration_service.rb > b/src/app/services/registration_service.rb > index 140b92e..0baca83 100644 > --- a/src/app/services/registration_service.rb > +++ b/src/app/services/registration_service.rb > @@ -14,21 +14,22 @@ class RegistrationService > begin > User.transaction do > @user.save! > - @pool = Pool.create!({ :name => @user.login, :zone => Zone.default}) > > - @quota = Quota.new > - @quota.save! > + allow_self_service_logins = > MetadataObject.lookup("allow_self_service_logins") > + self_service_default_pool = > MetadataObject.lookup("self_service_default_pool") > + self_service_default_role = > MetadataObject.lookup("self_service_default_role") > + self_service_default_quota = > MetadataObject.lookup("self_service_default_quota") > > - @pool.quota_id = @quota.id > - @pool.save! > + @user_quota = Quota.new(:maximum_running_instances => > self_service_default_quota.maximum_running_instances, > + :maximum_total_instances => > self_service_default_quota.maximum_total_instances) > + @user_quota.save! > + @user.quota = @user_quota > We need a new quota object for each user, since the quota object also tracks usage. We should only be pulling the #instances number from the metadata bits rather than a quota object. This does create a new Quota object. Based on the values set in the Default Quota. > + @user.save! > > - raise "Role 'Instance Creator and User' doesn't exist" unless > - role = Role.find_by_name("Instance Creator and User") > + Permission.create!({:user => @user, :role => > self_service_default_role, :permission_object => self_service_default_pool}) > > - Permission.create!({:user => @user, > - :role => role, > - :permission_object => @pool}) > - end > + return true > + end > rescue > Rails.logger.error $!.message > Rails.logger.error $!.backtrace.join("\n ") > diff --git a/src/db/migrate/20090803141507_create_pools.rb > b/src/db/migrate/20090803141507_create_pools.rb > index e0c1dc7..58e3c3d 100644 > --- a/src/db/migrate/20090803141507_create_pools.rb > +++ b/src/db/migrate/20090803141507_create_pools.rb > @@ -30,6 +30,11 @@ class CreatePools < ActiveRecord::Migration > t.timestamps > end > > + quota = Quota.new > + quota.save! > + > > + default_pool = Pool.new(:name => "default_pool", :quota => quota, :zone > => Zone.first) > + default_pool.save! > end > > def self.down > diff --git a/src/db/migrate/20100707000000_create_metadata_objects.rb > b/src/db/migrate/20100707000000_create_metadata_objects.rb > index 2f3eeb5..5f00306 100644 > --- a/src/db/migrate/20100707000000_create_metadata_objects.rb > +++ b/src/db/migrate/20100707000000_create_metadata_objects.rb > @@ -31,6 +31,20 @@ class CreateMetadataObjects < ActiveRecord::Migration > > default_zone = Zone.first > MetadataObject.set("default_zone", default_zone) if default_zone > + > + default_pool = Pool.first > + > You should pull the pool by name here -- don't assume that there's only one pool define here. I will change this, I was just following your pattern with Zones. > + default_quota = Quota.new > + default_quota.save! > + > We don't need the default quota, since we just want to store an integer for the default (and we can leave this as 'nil'/unset for now since nil OK again, same argument as before == unlimited) > + default_role = Role.find_by_name("Instance Creator and User") > + settings = {"allow_self_service_logins" => "true", > + "self_service_default_quota" => default_quota, > We can leave out default quota setting here > + "self_service_default_pool" => default_pool, > + "self_service_default_role" => default_role} > + settings.each_pair do |key, value| > + MetadataObject.set(key, value) > + end > end > > def self.down > diff --git a/src/features/authentication.feature > b/src/features/authentication.feature > index e160dfb..8525311 100644 > --- a/src/features/authentication.feature > +++ b/src/features/authentication.feature > @@ -19,25 +19,6 @@ Feature: User authentication > And I press "Create Account" > Then I should be on testuser's user page > And I should see "User registered!" > - And I should have one private pool named "testuser" > - > - @register > - Scenario: Register as new user fails even if user is valid > - Given I am on the homepage > - And there are not any roles > - When I follow "Create one now" > - Then I should be on the new account page > - And I should see "New Account" > - When I fill in the following: > - | Choose a username | testuser | > - | Choose a password | secret | > - | Confirm password | secret | > - | First name | Joe | > - | Last name | Tester | > - | Email | [email protected] | > - And I press "Create Account" > - Then I should see "New Account" > - And I should see "user registration failed" > > Scenario: Log in as registered user > Given I am a registered user > diff --git a/src/features/support/hooks.rb b/src/features/support/hooks.rb > index 356cf0d..49842fc 100644 > --- a/src/features/support/hooks.rb > +++ b/src/features/support/hooks.rb > @@ -1,3 +1,19 @@ > Before do > @default_zone_metadata = Factory.create(:default_zone_metadata) > -end > + @allow_self_service_logins = Factory(:metadata_object, :key => > "allow_self_service_logins", :value => "true") > + > + @default_quota = Factory(:unlimited_quota) > + @self_service_default_quota = Factory(:metadata_object, :key => > "self_service_default_quota", > + :value => > @default_quota, > + :object_type => > "Quota") > + > + @default_pool = Factory(:pool, :name => "default_pool") > + @self_service_default_quota = Factory(:metadata_object, :key => > "self_service_default_pool", > + :value => > @default_pool, > + :object_type => > "Pool") > + > + @default_role = Role.find(:first, :conditions => ['name = ?', 'Instance > Creator and User']) > + @self_service_default_quota = Factory(:metadata_object, :key => > "self_service_default_role", > + :value => > @default_role, > + :object_type => > "Role") > +end > \ No newline at end of file > diff --git a/src/spec/controllers/users_controller_spec.rb > b/src/spec/controllers/users_controller_spec.rb > index 5add3c7..debc675 100644 > --- a/src/spec/controllers/users_controller_spec.rb > +++ b/src/spec/controllers/users_controller_spec.rb > @@ -7,6 +7,23 @@ describe UsersController do > @admin_permission = Factory :admin_permission > @admin = @admin_permission.user > activate_authlogic > + > + @allow_self_service_logins = Factory(:metadata_object, :key => > "allow_self_service_logins", :value => "true") > + > + @default_quota = Factory(:unlimited_quota) > + @self_service_default_quota = Factory(:metadata_object, :key => > "self_service_default_quota", > + :value => > @default_quota, > + :object_type => > "Quota") > + > + @default_pool = Factory(:pool, :name => "default_pool") > + @self_service_default_quota = Factory(:metadata_object, :key => > "self_service_default_pool", > + :value => > @default_pool, > + :object_type => > "Pool") > + > + @default_role = Role.find(:first, :conditions => ['name = ?', 'Instance > Creator and User']) > + @self_service_default_quota = Factory(:metadata_object, :key => > "self_service_default_role", > + :value => > @default_role, > + :object_type => > "Role") > end > > it "should call new method" do > @@ -29,14 +46,6 @@ describe UsersController do > :password => "testpass", > :password_confirmation => "testpass" } > }.should change{ User.count } > - p = Pool.find_by_name("tuser2") > - p.should_not be_nil > - > - p.name.should == "tuser2" > - p.permissions.size.should == 1 > - p.permissions.any? { > - |perm| perm.role.name.eql?('Instance Creator and User') > - }.should be_true > user = User.find(:first, :conditions => ['login = ?', "tuser2"]) > response.should redirect_to(user_url(user)) > end > diff --git a/src/spec/factories/metadata_object.rb > b/src/spec/factories/metadata_object.rb > index 04e8404..5dcf989 100644 > --- a/src/spec/factories/metadata_object.rb > +++ b/src/spec/factories/metadata_object.rb > @@ -1,5 +1,11 @@ > -Factory.define :default_zone_metadata, :class => MetadataObject do |o| > - o.key 'default_zone' > - o.value {Factory.create(:zone).id} > - o.object_type 'Zone' > +Factory.define :metadata_object do |o| > + o.key 'key' > + o.value 'value' > + o.object_type nil > end > + > +Factory.define :default_zone_metadata, :parent => :metadata_object do |o| > + o.key 'default_zone' > + o.value Factory.create(:zone).id > + o.object_type 'Zone' > +end > \ No newline at end of file > diff --git a/src/spec/models/registration_service_spec.rb > b/src/spec/models/registration_service_spec.rb > index 40f16f2..2a97d11 100644 > --- a/src/spec/models/registration_service_spec.rb > +++ b/src/spec/models/registration_service_spec.rb > @@ -39,4 +39,26 @@ describe RegistrationService do > end > end > end > + > + it "should register a user with default pool/quota/role when default > settings set" do > + @user = Factory :user > + @pool = Factory(:pool, :name => "default_pool") > + @role = Role.find_by_name("Instance Creator and User") > + @quota = Factory :quota > + > + MetadataObject.set("allow_self_service_logins", "true") > + MetadataObject.set("self_service_default_pool", @pool) > + MetadataObject.set("self_service_default_role", @role) > + MetadataObject.set("self_service_default_quota", @quota) > + > + @registration_service = RegistrationService.new(@user) > + @registration_service.save > + > + @pools = Pool.list_for_user(@user, Privilege::INSTANCE_VIEW) > + @pools.size.should == 1 > + @pools[0].name.should == "default_pool" > + > + @user.quota.maximum_running_instances.should == > @quota.maximum_running_instances > + @user.quota.maximum_total_instances.should == > @quota.maximum_total_instances > + end > end > _______________________________________________ deltacloud-devel mailing list [email protected] https://fedorahosted.org/mailman/listinfo/deltacloud-devel
