From 1eb754cd25421936f17f8599b24e85a1c6bedce0 Mon Sep 17 00:00:00 2001 From: Keenan Brock Date: Wed, 28 Oct 2015 11:26:56 -0400 Subject: [PATCH] move next sequence calculation into miq_group - fix error around tenant groups with a nil sequence - set sequence by default - set sequence for tenant default_group --- app/controllers/ops_controller/ops_rbac.rb | 2 - app/models/miq_group.rb | 14 ++---- ...51021174140_assign_tenant_default_group.rb | 2 + spec/factories/miq_group.rb | 2 +- ...174140_assign_tenant_default_group_spec.rb | 4 +- spec/models/miq_group_spec.rb | 46 ++++++++++++++++++- 6 files changed, 53 insertions(+), 17 deletions(-) diff --git a/app/controllers/ops_controller/ops_rbac.rb b/app/controllers/ops_controller/ops_rbac.rb index 7ce82c0949b..85fd3356469 100644 --- a/app/controllers/ops_controller/ops_rbac.rb +++ b/app/controllers/ops_controller/ops_rbac.rb @@ -1130,8 +1130,6 @@ def rbac_build_myco_tree # Set group record variables to new values def rbac_group_set_record_vars(group) role = MiqUserRole.find_by_id(@edit[:new][:role]) - groups = MiqGroup.all(:order => "sequence DESC") - group.sequence = groups.first.nil? ? 1 : groups.first.sequence + 1 group.description = @edit[:new][:description] group.miq_user_role = role group.tenant = Tenant.find_by_id(@edit[:new][:group_tenant]) if @edit[:new][:group_tenant] diff --git a/app/models/miq_group.rb b/app/models/miq_group.rb index ac7d7f12457..c814a5b4705 100644 --- a/app/models/miq_group.rb +++ b/app/models/miq_group.rb @@ -30,6 +30,7 @@ class MiqGroup < ActiveRecord::Base serialize :settings default_value_for :group_type, USER_GROUP + default_value_for(:sequence) { next_sequence } acts_as_miq_taggable include ReportableMixin @@ -67,17 +68,8 @@ def self.allows?(_group, _options = {}) true # Remove once this is implemented end - def self.add(attrs) - group = new(attrs) - if group.resource.nil? - groups = where(:resource_id => nil, :resource_type => nil).order("sequence DESC") - else - groups = group.resource.miq_groups.order("sequence DESC") - end - group.sequence = groups.first.nil? ? 1 : groups.first.sequence + 1 - group.save! - - group + def self.next_sequence + maximum(:sequence).to_i + 1 end def self.seed diff --git a/db/migrate/20151021174140_assign_tenant_default_group.rb b/db/migrate/20151021174140_assign_tenant_default_group.rb index b6566139613..046d6f0fc94 100644 --- a/db/migrate/20151021174140_assign_tenant_default_group.rb +++ b/db/migrate/20151021174140_assign_tenant_default_group.rb @@ -33,6 +33,8 @@ def self.create_tenant_group(tenant) create_with( :description => "Tenant #{tenant.name} #{tenant.id} access", :group_type => TENANT_GROUP, + :sequence => 1, + :guid => MiqUUID.new_guid, :miq_user_role_id => role.try(:id) ).find_or_create_by!(:tenant_id => tenant.id) end diff --git a/spec/factories/miq_group.rb b/spec/factories/miq_group.rb index 6bc07bf9bed..8151f52d27e 100644 --- a/spec/factories/miq_group.rb +++ b/spec/factories/miq_group.rb @@ -8,7 +8,7 @@ end guid { MiqUUID.new_guid } - + sequence(:sequence) # don't want to spend time looking these up description { |g| g.role ? "EvmGroup-#{g.role}" : generate(:miq_group_description) } after :build do |g, e| diff --git a/spec/migrations/20151021174140_assign_tenant_default_group_spec.rb b/spec/migrations/20151021174140_assign_tenant_default_group_spec.rb index 3f63ad614bf..65ac04cad0a 100644 --- a/spec/migrations/20151021174140_assign_tenant_default_group_spec.rb +++ b/spec/migrations/20151021174140_assign_tenant_default_group_spec.rb @@ -21,7 +21,9 @@ t.reload expect(t.default_miq_group_id).to be - expect(group_stub.find(t.default_miq_group_id).miq_user_role_id).to eq(tenant_role.id) + g = group_stub.find(t.default_miq_group_id) + expect(g.miq_user_role_id).to eq(tenant_role.id) + expect(g.sequence).to be end it "skips tenants that already have a group" do diff --git a/spec/models/miq_group_spec.rb b/spec/models/miq_group_spec.rb index a9eca9efc3a..7bf5a33f190 100644 --- a/spec/models/miq_group_spec.rb +++ b/spec/models/miq_group_spec.rb @@ -248,16 +248,18 @@ it "fails if referenced by user#current_group" do FactoryGirl.create(:user, :miq_groups => [group]) + current_count = MiqGroup.count expect { group.destroy }.to raise_error - MiqGroup.count.should eq 1 + MiqGroup.count.should eq current_count end it "fails if referenced by user#miq_groups" do group2 = FactoryGirl.create(:miq_group) FactoryGirl.create(:user, :miq_groups => [group, group2], :current_group => group2) + current_count = MiqGroup.count expect { group.destroy }.to raise_error - MiqGroup.count.should eq 2 + MiqGroup.count.should eq current_count end it "fails if referenced by a tenant#default_miq_group" do @@ -429,4 +431,44 @@ expect(MiqGroup.non_tenant_groups).to include(g) end end + + describe ".next_sequence" do + it "creates the first group" do + MiqGroup.delete_all + expect(MiqGroup.next_sequence).to eq(1) + end + + it "detects existing groups" do + expect(MiqGroup.next_sequence).to be < 999 # sanity check + FactoryGirl.create(:miq_group, :sequence => 999) + expect(MiqGroup.next_sequence).to eq(1000) + end + + it "handles nil sequences" do + MiqGroup.delete_all + g = FactoryGirl.create(:miq_group) + g.update_attribute(:sequence, nil) + + expect(MiqGroup.next_sequence).to eq(1) + end + + it "auto assigns a sequences" do + # don't want to get behavior from factory girl + g1 = MiqGroup.create(:description => "one") + g2 = MiqGroup.create(:description => "two") + + expect(g1.sequence).to be + expect(g2.sequence).to eq(g1.sequence + 1) + end + + it "builds a sequence based upon select criteria" do + expect(MiqGroup.next_sequence).to be < 999 # sanity check + + FactoryGirl.create(:miq_group, :description => "want 1", :sequence => 999) + FactoryGirl.create(:miq_group, :description => "want 2", :sequence => 1000) + FactoryGirl.create(:miq_group, :description => "dont want", :sequence => 1009) + + expect(MiqGroup.where("description like 'want%'").next_sequence).to eq(1001) + end + end end