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.

Sign up for GitHub

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

Jump to bottom

Dynamic product features according to tenants #18102

Merged
merged 2 commits into from
Oct 30, 2018
Merged

Dynamic product features according to tenants #18102

merged 2 commits into from
Oct 30, 2018

Conversation

lpichler
Copy link
Contributor

@lpichler lpichler commented Oct 16, 2018 •
edited by simaishi
Loading

@lpichler lpichler mentioned this pull request Oct 16, 2018
Closed
9 tasks
@miq-bot miq-bot added the wip label Oct 16, 2018
@lpichler lpichler force-pushed the dynamic_product_features branch 4 times, most recently from dbf07ff to ddc18a9 Compare October 17, 2018 19:33
gtanzillo
gtanzillo reviewed Oct 18, 2018
View reviewed changes
app/models/miq_product_feature.rb Outdated
@@ -9,6 +9,7 @@ class MiqProductFeature < ApplicationRecord
has_and_belongs_to_many :miq_user_roles, :join_table => :miq_roles_features
has_many :miq_product_features_shares
has_many :shares, :through => :miq_product_features_shares
belongs_to :tenant, :dependent => :destroy
Copy link
Member

Choose a reason for hiding this comment

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

Will this delete the tenant if a product feature is deleted?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes it needs to be removed, thanks 👍

@lpichler lpichler force-pushed the dynamic_product_features branch 2 times, most recently from 705b1a6 to b6565ed Compare October 19, 2018 11:51
@lpichler
Copy link
Contributor Author

@gtanzillo
if we would like to use bulk insert this gem looks very promising
https://github.com/zdennis/activerecord-import

gtanzillo
gtanzillo reviewed Oct 19, 2018
View reviewed changes
app/models/tenant.rb Outdated
after_create :create_tenant_group
before_destroy :ensure_can_be_destroyed
after_create :create_tenant_group, :create_miq_product_features_for_tenant_nodes
before_destroy :ensure_can_be_destroyed, :destroy_tenant_feature_for_tenant_node
Copy link
Member

Choose a reason for hiding this comment

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

I don't think that you need to call destroy_tenant_feature_for_tenant_node. The has_many :miq_product_features, :dependent => :destroy in this class should take care of deleting the product features associated with the tenant being deleted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you are right, thanks 👍

app/models/tenant.rb Outdated
result
end

def create_miq_product_feature(miq_product_feature)
Copy link
Member

Choose a reason for hiding this comment

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

This should be named build_... instead of create_... since it doesn't actually create the miq_product_feature

gtanzillo
gtanzillo reviewed Oct 19, 2018
View reviewed changes
db/fixtures/miq_product_features.yml Outdated
:children:
- :name: Add
:description: Add Dialog in the Dialog Editor
:feature_type: admin
:hidden: true
:feature_type: tenant_node
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps it would be better to just add a new column for this instead of overloading feature_type. Maybe a boolean column called per_tenant? I'll leave it up to you for the implementation.

@lpichler lpichler force-pushed the dynamic_product_features branch 3 times, most recently from 21b7aac to 36b8c28 Compare October 30, 2018 13:54
@lpichler lpichler changed the title [WIP] Dynamic product features according to tenants Dynamic product features according to tenants Oct 30, 2018
@miq-bot miq-bot removed the wip label Oct 30, 2018
@miq-bot
Copy link
Member

miq-bot commented Oct 30, 2018

Checked commits lpichler/manageiq@3bed523~...cd56be0 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
7 files checked, 0 offenses detected
Everything looks fine. 🍰

@gtanzillo gtanzillo merged commit e391dec into ManageIQ:master Oct 30, 2018
@lpichler lpichler mentioned this pull request Oct 31, 2018
Merged
@lpichler lpichler deleted the dynamic_product_features branch October 31, 2018 14:27
@lpichler
Copy link
Contributor Author

@miq-bot add_label hammer/yes

lpichler pushed a commit to lpichler/manageiq that referenced this pull request Oct 31, 2018
@gtanzillo @lpichler
Merge pull request ManageIQ#18102 from lpichler/dynamic_product_features
2e6f9ac 
Dynamic product features according to tenants
@simaishi
Copy link
Contributor

@lpichler @gtanzillo This PR is marked as hammer/yes, but depends on a schema PR ManageIQ/manageiq-schema#291, which isn't going to hammer branch. Should this be hammer/no?

@lpichler
Copy link
Contributor Author

@simaishi I am sorry it was added too early, so for now hammer/no and we are waiting for answer
https://bugzilla.redhat.com/show_bug.cgi?id=1468795#c16

@lpichler
Copy link
Contributor Author

lpichler commented Nov 2, 2018

@miq-bot remove_label hammer/yes

will need to wait for decision ^

@miq-bot miq-bot removed the hammer/yes label Nov 2, 2018
@JPrause
Copy link
Member

JPrause commented Nov 8, 2018

@miq-bot add_label blocker

@miq-bot miq-bot added the blocker label Nov 8, 2018
gtanzillo added a commit to gtanzillo/manageiq that referenced this pull request Nov 8, 2018
@gtanzillo
Authorize user with non-dynamic product feature if included in user's…
d819beb 
… role

- This was a bug in the logic in ManageIQ#18102
- This fixes test failures in the API repo
@gtanzillo gtanzillo mentioned this pull request Nov 8, 2018
Merged
@lpichler lpichler mentioned this pull request Nov 9, 2018
Merged
gtanzillo added a commit to gtanzillo/manageiq that referenced this pull request Nov 12, 2018
@gtanzillo
Authorize user with non-dynamic product feature if included in user's…
ec15bb0 
… role

- This was a bug in the logic in ManageIQ#18102
- This fixes test failures in the API repo
@lpichler
Copy link
Contributor Author

@miq-bot add_label hammer/yes

simaishi pushed a commit that referenced this pull request Nov 14, 2018
@gtanzillo @simaishi
Merge pull request #18102 from lpichler/dynamic_product_features
ac5f85f 
Dynamic product features according to tenants

(cherry picked from commit e391dec)

https://bugzilla.redhat.com/show_bug.cgi?id=1468795
@simaishi
Copy link
Contributor

Hammer backport details:

$ git log -1
commit ac5f85f3a66368b6a5ededfcb3695f178d935139
Author: Gregg Tanzillo <gtanzill@redhat.com>
Date:   Tue Oct 30 16:51:09 2018 -0400

    Merge pull request #18102 from lpichler/dynamic_product_features
    
    Dynamic product features according to tenants
    
    (cherry picked from commit e391dec9c57dce99d67555c329906ec2fb71e759)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1468795

@mfeifer mfeifer added this to the Sprint 98 Ending Nov 6, 2018 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gtanzillo gtanzillo gtanzillo approved these changes

Assignees

@gtanzillo gtanzillo

Labels
blocker core/rbac core/tenancy enhancement hammer/backported
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lpichler @miq-bot @simaishi @JPrause @gtanzillo @mfeifer