Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make MiqProductFeature seeding pluggable #18806

Merged
merged 2 commits into from May 24, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 39 additions & 21 deletions app/models/miq_product_feature.rb
Expand Up @@ -6,6 +6,8 @@ class MiqProductFeature < ApplicationRecord
ALL_TASKS_FEATURE = "miq_task_all_ui".freeze
TENANT_ADMIN_FEATURE = "rbac_tenant".freeze

include_concern "Seeding"

acts_as_tree

has_and_belongs_to_many :miq_user_roles, :join_table => :miq_roles_features
Expand All @@ -18,8 +20,6 @@ class MiqProductFeature < ApplicationRecord
validates_presence_of :identifier
validates_uniqueness_of :identifier

FIXTURE_PATH = Rails.root.join(*["db", "fixtures", table_name])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

haha, I didn't see this until now... I guess we were really worried about hardcoding "miq_product_features"... I can't believe I didn't see table_name before...


DETAIL_ATTRS = [
:name,
:description,
Expand Down Expand Up @@ -62,10 +62,6 @@ def self.current_tenant_identifier(identifier)
tenant_identifier(identifier, User.current_tenant.id) if identifier && feature_details(identifier) && root_tenant_identifier?(identifier)
end

def self.feature_yaml(path = FIXTURE_PATH)
"#{path}.yml".freeze
end

def self.feature_root
features.keys.detect { |k| feature_parent(k).nil? }
end
Expand Down Expand Up @@ -148,29 +144,51 @@ def self.with_tenant_feature_root_features
where(:identifier => TENANT_FEATURE_ROOT_IDENTIFIERS)
end

def self.seed_tenant_miq_product_features
result = with_tenant_feature_root_features.map.each do |tenant_miq_product_feature|
Tenant.in_my_region.all.map { |tenant| tenant.build_miq_product_feature(tenant_miq_product_feature) }
end.flatten
def self.seed_single_tenant_miq_product_features(tenant)
result = MiqProductFeature.with_tenant_feature_root_features.map do |miq_product_feature|
{
:name => miq_product_feature.name,
:description => miq_product_feature.description,
:feature_type => 'admin',
:hidden => false,
:identifier => tenant_identifier(miq_product_feature.identifier, tenant.id),
:tenant_id => tenant.id,
:parent_id => miq_product_feature.id
}
end

MiqProductFeature.invalidate_caches
MiqProductFeature.create(result).map(&:identifier)
end

def self.seed_features(path = FIXTURE_PATH)
fixture_yaml = feature_yaml(path)
def self.seed_tenant_miq_product_features
Tenant.in_my_region.all.flat_map { |t| seed_single_tenant_miq_product_features(t) }
end

features = all.to_a.index_by(&:identifier)
seen = seed_from_hash(YAML.load_file(fixture_yaml), seen, nil, features)
def self.seed_features
transaction do
features = all.index_by(&:identifier)

root_feature = MiqProductFeature.find_by(:identifier => SUPER_ADMIN_FEATURE)
Dir.glob(path.join("*.yml")).each do |fixture|
seed_from_hash(YAML.load_file(fixture), seen, root_feature)
root_file, other_files = seed_files

seen = seed_from_hash(YAML.load_file(root_file), seen, nil, features)
root_feature = find_by(:identifier => SUPER_ADMIN_FEATURE)

other_files.each do |file|
seed_from_array(YAML.load_file(file), seen, root_feature)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me say this to make sure I understand.

  1. seed_from_hash is used for the core product features where the "everything" identifier is the root feature and all children fall under it

  2. seed_from_array is for seeding an array of features all at the same level: direct children of the root "everything" feature. Each can have their own trees of features but they all get the "everything" feature as the root.

Copy link
Member Author

@Fryguy Fryguy May 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kind of? Yes. I didn't want to add the seed_from_array, but plugins can bring an array of features, so it made sense to abstract walking an array of hashes.

end

tenant_identifiers = seed_tenant_miq_product_features
deletes = where.not(:identifier => seen.values.flatten + tenant_identifiers).destroy_all
_log.info("Deleting product features: #{deletes.collect(&:identifier).inspect}") unless deletes.empty?
seen
end
end

tenant_identifiers = seed_tenant_miq_product_features
deletes = where.not(:identifier => seen.values.flatten + tenant_identifiers).destroy_all
_log.info("Deleting product features: #{deletes.collect(&:identifier).inspect}") unless deletes.empty?
seen
def self.seed_from_array(array, seen = nil, parent = nil, features = nil)
Array.wrap(array).each do |hash|
seed_from_hash(hash, seen, parent, features)
end
end

def self.seed_from_hash(hash, seen = nil, parent = nil, features = nil)
Expand Down
30 changes: 30 additions & 0 deletions app/models/miq_product_feature/seeding.rb
@@ -0,0 +1,30 @@
class MiqProductFeature < ApplicationRecord
module Seeding
extend ActiveSupport::Concern

RELATIVE_FIXTURE_PATH = "db/fixtures/miq_product_features".freeze
FIXTURE_PATH = Rails.root.join(RELATIVE_FIXTURE_PATH).freeze

module ClassMethods
def seed_files
return seed_root_filename, seed_core_files + seed_plugin_files
Copy link
Member

@jrafanie jrafanie May 24, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Fryguy If I'm reading correctly, this doesn't break the UI plugin work by @martinpovolny because they should still be picked up in seed_core_file, right? Note, I'm not sure if we still need it but I'm making sure that I understand it. It looks to be still intact.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, this keeps @martinpovolny 's original "directory of feature yamls" in core, however that's not being used anywhere, so while we have it we may want to consider dropping it. On the plugin side though it could be useful to have separate yamls per feature in a dir, so I basically made plugins work like core.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have it we may want to consider dropping it

Yes, I'd like to see it dropped. Also it does not support any nesting if I remember correctly.

Someone might me using it: https://documentation.commvault.com/commvault/v11_adminconsole/article?p=87519.htm and they should be provided with a way to migrate.

What we need with this new way is documentation.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, this return value feels strange to me, but I understand why it's done this way. Maybe in a future PR we can pull the everything feature out of the root file and remove the special handling of that file.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this idea, and definitely think we should do it in a followup.

end

private

def seed_root_filename
"#{FIXTURE_PATH}.yml"
end

def seed_core_files
Dir.glob("#{FIXTURE_PATH}/*.y{,a}ml").sort
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I appreciate the sort on Dir.glob ❤️

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I basically always sort that method now, because of its unpredictability... I'm almost tempted to create a more_core_extensions method that enforces it :)

end

def seed_plugin_files
Vmdb::Plugins.flat_map do |plugin|
Dir.glob("#{plugin.root.join(RELATIVE_FIXTURE_PATH)}{.yml,.yaml,/*.yml,/*.yaml}").sort
end
end
end
end
end
22 changes: 1 addition & 21 deletions app/models/tenant.rb
Expand Up @@ -302,27 +302,7 @@ def allowed?
end

def create_miq_product_features_for_tenant_nodes
result = MiqProductFeature.with_tenant_feature_root_features.map.each do |tenant_miq_product_feature|
build_miq_product_feature(tenant_miq_product_feature)
end.flatten

result = MiqProductFeature.create(result).map(&:identifier)

MiqProductFeature.invalidate_caches
result
end

def build_miq_product_feature(miq_product_feature)
attrs = {
:name => miq_product_feature.name, :description => miq_product_feature.description,
:feature_type => 'admin',
:hidden => false,
:identifier => MiqProductFeature.tenant_identifier(miq_product_feature.identifier, id),
:tenant_id => id,
:parent_id => miq_product_feature.id
}

attrs
MiqProductFeature.seed_single_tenant_miq_product_features(self)
end

private
Expand Down
115 changes: 66 additions & 49 deletions spec/models/miq_product_feature_spec.rb
@@ -1,6 +1,3 @@
require 'tmpdir'
require 'pathname'

describe MiqProductFeature do
let(:miq_product_feature_class) do
Class.new(described_class) do
Expand All @@ -26,40 +23,51 @@ def self.tenant_features_in_hash
)
end

it "is properly configured" do
everything = YAML.load_file(described_class.feature_yaml)
traverse_product_features(everything) do |pf|
expect(pf).to include(*described_class::REQUIRED_ATTRIBUTES)
expect(pf.keys - described_class::ALLOWED_ATTRIBUTES).to be_empty
expect(pf[:children]).not_to be_empty if pf.key?(:children)
def assert_product_feature_attributes(pf)
expect(pf).to include(*described_class::REQUIRED_ATTRIBUTES)
expect(pf.keys - described_class::ALLOWED_ATTRIBUTES).to be_empty
expect(pf[:children]).not_to be_empty if pf.key?(:children)
end

def traverse_product_feature_children(pfs, &block)
pfs.each do |pf|
yield pf
traverse_product_feature_children(pf[:children], &block) if pf.key?(:children)
end
end

def traverse_product_features(product_feature, &block)
block.call(product_feature)
if product_feature.key?(:children)
product_feature[:children].each { |child| traverse_product_features(child, &block) }
it "is properly configured" do
root_file, other_files = described_class.seed_files

hash = YAML.load_file(root_file)
assert_product_feature_attributes(hash)

traverse_product_feature_children(hash[:children]) do |pf|
assert_product_feature_attributes(pf)
end

other_files.each do |f|
hash = YAML.load_file(f)
traverse_product_feature_children(Array.wrap(hash)) do |pf|
assert_product_feature_attributes(pf)
end
end
end

context ".seed" do
it "creates feature identifiers once on first seed, changes nothing on second seed" do
status_seed1 = nil
expect { status_seed1 = MiqProductFeature.seed }.to change(MiqProductFeature, :count)
expect(status_seed1[:created]).to match_array status_seed1[:created].uniq
expect(status_seed1[:updated]).to match_array []
expect(status_seed1[:unchanged]).to match_array []

status_seed2 = nil
expect { status_seed2 = MiqProductFeature.seed }.not_to change(MiqProductFeature, :count)
expect(status_seed2[:created]).to match_array []
expect(status_seed2[:updated]).to match_array []
expect(status_seed2[:unchanged]).to match_array status_seed1[:created]
expect { MiqProductFeature.seed }.to change(MiqProductFeature, :count)
orig_ids = MiqProductFeature.pluck(:id)
expect { MiqProductFeature.seed }.not_to change(MiqProductFeature, :count)
expect(MiqProductFeature.pluck(:id)).to match_array orig_ids
end
end

context ".seed_features" do
let(:feature_path) { Pathname.new(@tempfile.path.sub(/\.yml/, '')) }
let(:tempdir) { Dir.mktmpdir }
let(:feature_path) { File.join(tempdir, "miq_product_features") }
let(:root_file) { File.join(tempdir, "miq_product_features.yml") }

let(:base) do
{
:feature_type => "node",
Expand All @@ -76,47 +84,56 @@ def traverse_product_features(product_feature, &block)
end

before do
@tempdir = Dir.mktmpdir
@tempfile = Tempfile.new(['feature', '.yml'], @tempdir)
@tempfile.write(YAML.dump(base))
@tempfile.close
File.write(root_file, base.to_yaml)

expect(described_class).to receive(:seed_files).at_least(:once) do
[root_file, Dir.glob(File.join(feature_path, "*.yml"))]
end
end

after do
@tempfile.unlink
Dir.rmdir(@tempdir)
FileUtils.rm_rf(tempdir)
end

it "existing records" do
it "creates/updates/deletes records" do
deleted = FactoryBot.create(:miq_product_feature, :identifier => "xxx")
changed = FactoryBot.create(:miq_product_feature, :identifier => "dialog_new_editor", :name => "XXX")
unchanged = FactoryBot.create(:miq_product_feature_everything)
unchanged_orig_updated_at = unchanged.updated_at

MiqProductFeature.seed_features(feature_path)
MiqProductFeature.seed_features

expect { deleted.reload }.to raise_error(ActiveRecord::RecordNotFound)
expect(changed.reload.name).to eq("One")
expect(unchanged.reload.updated_at).to be_within(0.1).of unchanged_orig_updated_at
end

it "additional yaml feature" do
additional = {
:feature_type => "node",
:identifier => "dialog_edit_editor",
:children => []
}
context "additional yaml feature" do
before do
additional_hash = {
:feature_type => "node",
:identifier => "dialog_edit_editor",
:children => []
}

additional_array = [
{
:feature_type => "node",
:identifier => "policy_edit_editor",
:children => []
}
]

Dir.mkdir(feature_path)
additional_file = Tempfile.new(['additional', '.yml'], feature_path)
additional_file.write(YAML.dump(additional))
additional_file.close
FileUtils.mkdir_p(feature_path)
File.write(File.join(feature_path, "additional_hash.yml"), additional_hash.to_yaml)
File.write(File.join(feature_path, "additional_array.yml"), additional_array.to_yaml)
end

status_seed = MiqProductFeature.seed_features(feature_path)
expect(MiqProductFeature.count).to eq(3)
expect(status_seed[:created]).to match_array %w(everything dialog_new_editor dialog_edit_editor)
it "creates/updates/deletes records" do
MiqProductFeature.seed_features

additional_file.unlink
Dir.rmdir(feature_path)
expect(MiqProductFeature.pluck(:identifier)).to match_array %w(everything dialog_new_editor dialog_edit_editor policy_edit_editor)
end
end

context 'dynamic product features' do
Expand Down Expand Up @@ -151,7 +168,7 @@ def traverse_product_features(product_feature, &block)
let!(:tenant) { FactoryBot.create(:tenant, :parent => root_tenant) }

before do
MiqProductFeature.seed_features(feature_path)
MiqProductFeature.seed_features
end

it "creates tenant features" do
Expand Down
8 changes: 3 additions & 5 deletions spec/models/tenant_spec.rb
Expand Up @@ -876,12 +876,10 @@
end
end

describe "#destroy_tenant_feature_for_tenant_node" do
it "destroys product features" do
tenant_product_feature.destroy
it "destroys product features on destroy" do
tenant_product_feature.destroy

expect(MiqProductFeature.where(:identifier => ["dialog_edit_editor_tenant_#{tenant_product_feature.id}", "ab_group_admin_tenant_#{tenant_product_feature.id}"], :feature_type => 'tenant').count).to be_zero
end
expect(MiqProductFeature.where(:identifier => ["dialog_edit_editor_tenant_#{tenant_product_feature.id}", "ab_group_admin_tenant_#{tenant_product_feature.id}"], :feature_type => 'tenant').count).to be_zero
end
end
end
Expand Down
9 changes: 8 additions & 1 deletion spec/support/evm_spec_helper.rb
Expand Up @@ -96,7 +96,14 @@ def self.specific_product_features(*features)

def self.seed_specific_product_features(*features)
features.flatten!
hashes = YAML.load_file(MiqProductFeature.feature_yaml)

root_file, other_files = MiqProductFeature.seed_files

hashes = YAML.load_file(root_file)
other_files.each do |f|
hashes[:children] += Array.wrap(YAML.load_file(f))
end

filtered = filter_specific_features([hashes], features).first
MiqProductFeature.seed_from_hash(filtered)
MiqProductFeature.seed_tenant_miq_product_features
Expand Down