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

Add event streams product features #16021

Merged
merged 2 commits into from
Sep 25, 2017

Conversation

imtayadeway
Copy link
Contributor

@abellotti
Copy link
Member

LGTM!! Thanks @imtayadeway

I'll wait for a 👍 from @h-kataria before merging.

@dclarizio
Copy link

@abellotti perhaps these new API only features should have their own parent key so they don't get mixed up in the features tree, which currently tried to mimic the UI hierarchy. I discussed with @imtayadeway and we're thinking maybe a parent key for "metrics" and "event_streams" called "api" with a description of "Features Exclusive to the API".

If later they become UI features, we can move them to the appropriate place and, as long as the feature IDs don't change, no code changes will be needed.

@imtayadeway imtayadeway force-pushed the event-streams-product-features branch from 3b3ee41 to 6b59b0b Compare September 22, 2017 20:59
@imtayadeway
Copy link
Contributor Author

@dclarizio I made the suggested changes 👍 LMK if there's anything else 🙇‍♂️

@miq-bot
Copy link
Member

miq-bot commented Sep 22, 2017

Checked commits imtayadeway/manageiq@e43a195~...6b59b0b with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@abellotti
Copy link
Member

I like the API specific section.

Are these visible in the Classic UI when editing a role ? i.e. being able to setup a role with or without these specific entitlements on any user role or are they only inherited by a super admin ?

@dclarizio
Copy link

@abellotti they should! @imtayadeway can you get a screenshot from a User Role screen to verify?

@imtayadeway
Copy link
Contributor Author

@abellotti @dclarizio it looks like they don't automatically. From what I can grok they have to be added here: https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/presenters/menu/default_menu.rb

@h-kataria
Copy link
Contributor

@martinpovolny features added here look good, but i see one issue that these features will not show up on Role Features tree due to the way features tree is being built using menu presenters. API node in this PR will not be presented as a Menu in UI and the way features tree is built we do need a main menu section for all top level features. I think there will be more of these types on node in features tree that will not be presented in UI menus but need to be visible in Features tree, for example SUI, GO etc.
Do you have any suggestions on how we should proceed with showing such items in features tree.

@h-kataria
Copy link
Contributor

h-kataria commented Sep 25, 2017

@martinpovolny i got API node to show up in Product features tree by changing line https://github.com/ManageIQ/manageiq-ui-classic/blob/master/app/presenters/tree_builder_ops_rbac_features.rb#L44 to

%w(all_vm_rules api_exclusive).each do |additional_feature|
      top_nodes << MiqProductFeature.obj_features[additional_feature][:feature]
    end

is that the right way to do it?

screenshot from 2017-09-25 12-40-55

@imtayadeway
Copy link
Contributor Author

@abellotti I think this should be GTG. Thanks so much @dclarizio and @h-kataria !

@martinpovolny
Copy link
Member

@h-kataria : yes, that's simply the place where we add the hardcoded exceptions to the generic rules of forming that tree.

@dclarizio
Copy link

@imtayadeway @abellotti this is good to go. @h-kataria will create the PR on the UI side to show the API section in the features tree.

h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request Sep 25, 2017
Changes to show features in Features tree that do not show up on Menus in Classic UI. Follow up PR for ManageIQ/manageiq#16021
@h-kataria
Copy link
Contributor

h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request Sep 25, 2017
Changes to show features in Features tree that do not show up on Menus in Classic UI. Follow up PR for ManageIQ/manageiq#16021
@abellotti
Copy link
Member

Thank you @imtayadeway for this enhancement and thank you all for your input/recommendations. This LGTM!! 👍

@abellotti abellotti merged commit 9d01848 into ManageIQ:master Sep 25, 2017
@abellotti abellotti added this to the Sprint 70 Ending Oct 2, 2017 milestone Sep 25, 2017
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request Sep 25, 2017
Changes to show features in Features tree that do not show up on Menus in Classic UI. Follow up PR for ManageIQ/manageiq#16021
h-kataria added a commit to h-kataria/manageiq-ui-classic that referenced this pull request Sep 25, 2017
Changes to show features in Features tree that do not show up on Menus in Classic UI. Follow up PR for ManageIQ/manageiq#16021
xeviknal pushed a commit to xeviknal/manageiq-ui-classic that referenced this pull request Oct 9, 2017
Changes to show features in Features tree that do not show up on Menus in Classic UI. Follow up PR for ManageIQ/manageiq#16021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants