-
Notifications
You must be signed in to change notification settings - Fork 375
Fix custom buttons and widgets with role names or group descriptions that can change #9954
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
Changes from all commits
4934a7e
740008b
2281d43
c776fe9
94eb81f
31b0b39
f679c1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -852,12 +852,7 @@ def button_set_record_vars(button) | |
| end | ||
| button.visibility ||= {} | ||
| if @edit[:new][:visibility_typ] == "role" | ||
| roles = [] | ||
| @edit[:new][:roles].each do |r| | ||
| role = MiqUserRole.find_by(:id => r) | ||
| roles.push(role.name) if role | ||
| end | ||
| button.visibility[:roles] = roles | ||
| button.visibility[:roles] = @edit[:new][:roles] | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. previous code was doing a lot of work to persist names, now we just persist the ids |
||
| else | ||
| button.visibility[:roles] = ["_ALL_"] | ||
| end | ||
|
|
@@ -1023,16 +1018,7 @@ def button_set_form_vars | |
| # Visibility Box | ||
| if @custom_button.visibility && @custom_button.visibility[:roles] | ||
| @edit[:new][:visibility_typ] = @custom_button.visibility[:roles][0] == "_ALL_" ? "all" : "role" | ||
| if @custom_button.visibility[:roles][0] == "_ALL_" | ||
| @edit[:new][:roles] = ["_ALL_"] | ||
| else | ||
| @edit[:new][:roles] ||= [] | ||
| @custom_button.visibility[:roles].each do |r| | ||
| role = MiqUserRole.find_by(:name => r) | ||
| @edit[:new][:roles].push(role.id) if role | ||
| end | ||
| end | ||
| @edit[:new][:roles].sort! if @edit[:new][:roles].present? | ||
| @edit[:new][:roles] = @custom_button.visibility[:roles][0] == "_ALL_" ? ["_ALL_"] : @custom_button.visibility[:roles].sort | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Again, a lot of lookups by name to push a sorted list of ids, which is what we now have in the db, so this is simpler. |
||
| end | ||
|
|
||
| @edit[:sorted_user_roles] = [] | ||
|
|
@@ -1096,9 +1082,8 @@ def buttons_get_node_info(node) | |
| end | ||
| @sb[:user_roles] = [] | ||
| if @custom_button.visibility && @custom_button.visibility[:roles] && @custom_button.visibility[:roles][0] != "_ALL_" | ||
| MiqUserRole.all.sort_by(&:name).each do |r| | ||
| @sb[:user_roles].push(r.name) if @custom_button.visibility[:roles].include?(r.name) | ||
| end | ||
| role_ids = @custom_button.visibility[:roles] | ||
| @sb[:user_roles] = MiqUserRole.where(:id => role_ids).order(:name).pluck(:name) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sanbox needs names, so we pluck the names sorted. |
||
| end | ||
| @resolve[:new][:target_class] = "ServiceTemplate" | ||
| dialog_id = @custom_button.resource_action.dialog_id | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,9 +76,8 @@ def ab_get_node_info(node) | |
| end | ||
| @sb[:user_roles] = [] | ||
| if @custom_button.visibility && @custom_button.visibility[:roles] && @custom_button.visibility[:roles][0] != "_ALL_" | ||
| MiqUserRole.all.sort_by(&:name).each do |r| | ||
| @sb[:user_roles].push(r.name) if @custom_button.visibility[:roles].include?(r.name) | ||
| end | ||
| role_ids = @custom_button.visibility[:roles] | ||
| @sb[:user_roles] = MiqUserRole.where(:id => role_ids).order(:name).pluck(:name) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same |
||
| end | ||
| dialog_id = @custom_button.resource_action.dialog_id | ||
| @sb[:dialog_label] = dialog_id ? Dialog.find(dialog_id).label : "" | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -292,15 +292,12 @@ def widget_get_node_info | |
| if @widget.visibility && @widget.visibility[:roles] | ||
| @sb[:user_roles] = [] | ||
| if @widget.visibility[:roles][0] != "_ALL_" | ||
| Rbac.filtered(MiqUserRole).sort_by(&:name).each do |r| | ||
| @sb[:user_roles].push(r.name) if @widget.visibility[:roles].include?(r.name) | ||
| end | ||
| role_ids = @widget.visibility[:roles] | ||
| @sb[:user_roles] = Rbac.filtered(MiqUserRole.where(:id => role_ids)).order(:name).pluck(:name) | ||
| end | ||
| elsif @widget.visibility && @widget.visibility[:groups] | ||
| @sb[:groups] = [] | ||
| Rbac.filtered(MiqGroup.non_tenant_groups_in_my_region).sort_by(&:description).each do |r| | ||
| @sb[:groups].push(r.description) if @widget.visibility[:groups].include?(r.description) | ||
| end | ||
| group_ids = @widget.visibility[:groups] | ||
| @sb[:groups] = Rbac.filtered(MiqGroup.where(:id => group_ids)).order(:description).pluck(:description) | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. again, a bit simpler since we can just check what ids we're allowed to see and pluck their name or description. |
||
| end | ||
| end | ||
| end | ||
|
|
@@ -335,13 +332,17 @@ def widget_set_form_vars | |
| if @widget.visibility[:roles][0] == "_ALL_" | ||
| @edit[:new][:roles] = ["_ALL_"] | ||
| else | ||
| roles = Rbac.filtered(MiqUserRole.where(:name => @widget.visibility[:roles])) | ||
| @edit[:new][:roles] = roles.collect(&:id).sort | ||
| role_ids = Rbac.filtered(MiqUserRole.where(:id => @widget.visibility[:roles])) | ||
| @edit[:new][:roles] = role_ids.pluck(:id).sort | ||
| end | ||
| elsif @widget.visibility[:groups] | ||
| @edit[:new][:visibility_typ] = "group" | ||
| groups = Rbac.filtered(MiqGroup.in_my_region.where(:description => @widget.visibility[:groups])) | ||
| @edit[:new][:groups] = groups.collect(&:id).sort | ||
| if @widget.visibility[:groups][0] == "_ALL_" | ||
| @edit[:new][:groups] = ["_ALL_"] | ||
| else | ||
| group_ids = Rbac.filtered(MiqGroup.where(:id => @widget.visibility[:groups])) | ||
| @edit[:new][:groups] = group_ids.pluck(:id).sort | ||
| end | ||
| end | ||
| end | ||
| @edit[:sorted_user_roles] = | ||
|
|
@@ -564,21 +565,11 @@ def widget_set_record_vars(widget) | |
| widget.content_type = WIDGET_CONTENT_TYPE[@sb[:wtype]] | ||
| widget.visibility ||= {} | ||
| if @edit[:new][:visibility_typ] == "group" | ||
| groups = [] | ||
| @edit[:new][:groups].each do |g| | ||
| group = MiqGroup.find(g) | ||
| groups.push(group.description) if g == group.id.to_s | ||
| end | ||
| widget.visibility[:groups] = groups | ||
| widget.visibility[:groups] = @edit[:new][:groups] | ||
| widget.visibility.delete(:roles) if widget.visibility[:roles] | ||
| else | ||
| if @edit[:new][:visibility_typ] == "role" | ||
| roles = [] | ||
| @edit[:new][:roles].each do |r| | ||
| role = MiqUserRole.find(r) | ||
| roles.push(role.name) if r == role.id.to_s | ||
| end | ||
| widget.visibility[:roles] = roles | ||
| widget.visibility[:roles] = @edit[:new][:roles] | ||
| else | ||
| widget.visibility[:roles] = ["_ALL_"] | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -30,7 +30,8 @@ def visibility_options(widget) | |
| if values.first == "_ALL_" | ||
| _("To All Users") | ||
| else | ||
| _("By %{typ}: %{values}") % {:typ => typ.to_s.titleize, :values => values.join(',')} | ||
| display_values = widget.visibility_values.sort.join(', ') | ||
| _("By %{typ}: %{values}") % {:typ => typ.to_s.titleize, :values => display_values} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm ok with this for this PR, but technically this breaks i18n, becaiuse "By Groups" or "By Roles" can't be translated independently as the translators only see |
||
| end | ||
| end | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,74 @@ | ||
| /* eslint-disable no-undef */ | ||
| import { flashClassMap } from '../../../../support/assertions/assertion_constants'; | ||
|
|
||
| describe('Automation > Embedded Automate > Customization > Buttons', () => { | ||
| beforeEach(() => { | ||
| cy.login(); | ||
| cy.menu('Automation', 'Embedded Automate', 'Customization'); | ||
| cy.get('#explorer_title_text'); | ||
| cy.accordion('Buttons'); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| cy.appDbState('restore'); | ||
| }); | ||
|
|
||
| describe('Button Creation', () => { | ||
| it('Creates a new button with role visibility', () => { | ||
| cy.selectAccordionItem(['Object Types', 'Availability Zone', '[Unassigned Buttons]']); | ||
| cy.toolbar('Configuration', 'Add a new Button'); | ||
| cy.get('#explorer_title_text').contains('Adding a new Button'); | ||
|
|
||
| cy.get('#name').type('Test Button'); | ||
| cy.get('#description').type('Test button description'); | ||
|
|
||
| cy.get('.icon-button').click(); | ||
| cy.get('.cds--modal-container').should('be.visible'); | ||
| cy.get(':nth-child(1) > span > .ff').click(); | ||
| cy.get('.cds--modal-footer > .cds--btn--primary').click(); | ||
|
|
||
| cy.tabs({ tabLabel: 'Advanced' }); | ||
|
|
||
| cy.get('#object_request').scrollIntoView(); | ||
| cy.get('#object_request').type('test_request'); | ||
|
|
||
| cy.interceptApi({ | ||
| alias: 'visibilityChanged', | ||
| urlPattern: '/miq_ae_customization/automate_button_field_changed', | ||
| triggerFn: () => { | ||
| cy.get('#form_role_visibility button[data-id="visibility_typ"]').scrollIntoView(); | ||
| cy.get('#form_role_visibility button[data-id="visibility_typ"]').click(); | ||
| cy.get('#form_role_visibility .dropdown-menu [data-original-index="1"] > a').click(); | ||
| }, | ||
| waitOnlyIfRequestIntercepted: true, | ||
| }); | ||
|
|
||
| cy.contains('User Roles').scrollIntoView(); | ||
|
|
||
| cy.interceptApi({ | ||
| alias: 'checkAuditorRole', | ||
| urlPattern: '/miq_ae_customization/automate_button_field_changed', | ||
| triggerFn: () => cy.get('#form_role_visibility').contains('EvmRole-auditor').closest('td').find('input[type="checkbox"]').check(), | ||
| waitOnlyIfRequestIntercepted: true, | ||
| }); | ||
|
|
||
| cy.interceptApi({ | ||
| alias: 'checkDesktopRole', | ||
| urlPattern: '/miq_ae_customization/automate_button_field_changed', | ||
| triggerFn: () => cy.get('#form_role_visibility').contains('EvmRole-desktop').closest('td').find('input[type="checkbox"]').check(), | ||
| waitOnlyIfRequestIntercepted: true, | ||
| }); | ||
|
|
||
| // TODO: cy.getFormButtonByTypeWithText expects button in main_content div, but it's in form_buttons_div | ||
| cy.get('#buttons_on > .btn-primary').click(); | ||
| cy.expect_flash(flashClassMap.success, 'was added'); | ||
|
|
||
| cy.get('.clickable-row').contains('Test Button').click(); | ||
|
|
||
| cy.get('#main_div').contains('Test Button'); | ||
| cy.get('#main_div').contains('test_request'); | ||
| cy.get('.visibility').contains('EvmRole-auditor'); | ||
| cy.get('.visibility').contains('EvmRole-desktop'); | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note, the cypress test creates button with the visibility values but doesn't go back and edit the form to verify the checkboxes afterwards. |
||
| }); | ||
| }); | ||
| }); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's one of the fixes to ensure the "xx_r1" becomes
1numbers so our checkboxes light up. Keeping track of string vs numeric numbers was confusing for me so I think this just fixes it. I couldn't break the visibility box for either custom buttons or widgets.