Fix custom buttons and widgets with role names or group descriptions that can change by jrafanie · Pull Request #9954 · ManageIQ/manageiq-ui-classic

jrafanie · 2026-04-03T16:31:35Z

Replaces #9709 #9710
Depends on:

Push down the visibility value logic to the widget model manageiq#23803
Fix an issue with blank visibility manageiq#23805
Convert role/group names/descriptions to IDs in visibility for CustomButton and MiqWidget manageiq-schema#852 (should be merged together only if both PRs are ready to go)

Previously, custom buttons and widgets stored role names and group descriptions in visibility settings, breaking when these changed.

Now, we store role IDs and group IDs instead. The schema migration above ensures existing data will get migrated to IDs as numbers instead of names/descriptions.

Changes:

Store IDs instead of names/descriptions:
- Role names → role IDs
- Group descriptions → group IDs
Consistent numeric IDs throughout (no mixed string/int)

Extracted from and for usage in: ManageIQ/manageiq-ui-classic#9954

jrafanie · 2026-04-03T16:36:49Z

      _("To All Users")
    else
-      _("By %{typ}: %{values}") % {:typ => typ.to_s.titleize, :values => values.join(',')}
+      display_values = widget.visibility_values.join(',')


uses the method added in ManageIQ/manageiq#23803

I think you should add some sorting here.

done... ~~they're IDs now so they'll likely be sorted in creation order.~~ I misspoke. These are coming back with the values so they will be sorted by name.

Fryguy · 2026-04-03T17:16:18Z

    end

    it "sets new role and preserves old role for custom button" do
      edit[:new][:roles] = [old_role.id, role.id.to_s] # old roles are represented by int and new ones by string


Not sure I understand why new ones are strings?

This is was part of the last set of changes, see c776fe9, "Consistently use numbers for group/role ids"

Related to manageiq-schema [1] which migrated existing role names to IDs in the visibility column. [1] ManageIQ/manageiq-schema#852 Before: visibility[:roles] stored role names (strings) After: visibility[:roles] stores role IDs (integers) Changes: - Saving: Store IDs directly from form - Loading: Read IDs directly from database - Display: Query role names from IDs for UI

- Fix widget_spec expectation to use group.id instead of group.description - Add test for group visibility display in widget_get_node_info - Remove obvious comments from Cypress tests (kept TODO comment) Related to ManageIQ/manageiq-schema#XXX where we converted visibility from storing role names/group descriptions to storing IDs. This ensures tests properly verify ID-based lookups and display of names/descriptions.

ManageIQ/manageiq#23803

Previously, with the data migration and UI code that stored ids as numbers in the database, the code that lit up the checkboxes failed to match ids as strings with the numbers from the database.

Followup to ManageIQ#23803 Fixes an issue seen when testing UI usage in ManageIQ/manageiq-ui-classic#9954 The client of this code shouldn't need to check if the visibility is missing the roles/groups key.

jrafanie · 2026-04-07T20:22:03Z

-        roles.push(role.name) if role
-      end
-      button.visibility[:roles] = roles
+      button.visibility[:roles] = @edit[:new][:roles]


previous code was doing a lot of work to persist names, now we just persist the ids

jrafanie · 2026-04-07T20:23:43Z

-        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


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.

jrafanie · 2026-04-07T20:24:25Z

-          @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)


sanbox needs names, so we pluck the names sorted.

jrafanie · 2026-04-07T20:24:42Z

-          @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)


jrafanie · 2026-04-07T20:26:12Z

-          @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)


again, a bit simpler since we can just check what ids we're allowed to see and pluck their name or description.

jrafanie · 2026-04-07T20:28:06Z

-          roles = Rbac.filtered(MiqUserRole.where(:name => @widget.visibility[:roles]))
-          @edit[:new][:roles] = roles.collect(&:id).sort
-        end
+        @edit[:new][:roles] = @widget.visibility[:roles][0] == "_ALL_" ? ["_ALL_"] : @widget.visibility[:roles].sort


Do we need to do Rbac.filtered here, like we do above?

presumably the RBAC was already checked?

I found another. I'll fix this. I don't see how rbac was already checked on these roles/groups so we should probably keep it.

ok, fixed... this should continue to have the same behavior as it did previously, but now uses IDs. I don't know if it makes sense to do rbac filtering here... I don't think MiqUserRole or MiqGroup participate in rbac... but anyway, the existing behavior is restored, even if it doesn't make sense.

Note, if the user has access to reports tree view and can modify the widgets or similarly, the embedded automate custom button section, they can change what roles/groups have visibility to this since groups and users don't participate in rbac.

I can try to resolve this in a followup PR but for the purposes of this change, it's probably best to keep the existing behavior and only change the names to ids.

jrafanie · 2026-04-07T20:30:26Z

        next unless var.starts_with?(prefix)

-        name = var.split(prefix).last
+        name = var.split(prefix).last.to_i


Here's one of the fixes to ensure the "xx_r1" becomes 1 numbers 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.

The miq_widget factory has separate sequences for both title and description attributes. When tests override :title, they skip the title sequence but the description sequence still increments, causing the sequences to become out of sync (e.g., title='widget_0000000000001', description='widget_0000000000002'). This caused test failures when: 1. A test overrode :title (causing sequence desync) 2. A subsequent test expected title and description to match 3. TreeBuilderReportWidgets uses widget.title for tree node display Fixed by: - Removing unnecessary :title overrides in widget_spec.rb (lines 96, 108, 124) These tests only care about visibility settings, not specific title text - Updating explorer_spec.rb to use widget.title instead of widget.description Added comment explaining TreeBuilderReportWidgets uses title attribute This makes tests resilient to execution order and prevents future sequence desync issues. The factory-generated titles are sufficient for these tests.

jrafanie · 2026-04-08T19:47:39Z

+      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');


Note, the cypress test creates button with the visibility values but doesn't go back and edit the form to verify the checkboxes afterwards.

Fryguy · 2026-04-09T13:46:40Z

    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}


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 "By %{typ}". However, this was there previously, so it's not changing anything. Something to keep in mind in a future PR.

miq-bot · 2026-04-10T12:25:58Z

Checked commits jrafanie/manageiq-ui-classic@4934a7e~...f679c1c with ruby 3.3.10, rubocop 1.86.0, haml-lint 0.72.0, and yamllint 1.37.1
10 files checked, 0 offenses detected
Everything looks fine. 🍪

jrafanie · 2026-04-10T12:45:37Z

@Fryguy @GilbertCherrie I think this is ready for final review. Both ManageIQ/manageiq-schema#852 and #9954 should be merged together.

GilbertCherrie

Tested it and it is working as expected after assigning a button/widget to a role and then changing the role name. The button/widget is still assigned to the role with the new name and the checkboxes on the forms show this. Also, tested groups for the widget form which works correctly too even if you change the group name.

Fryguy · 2026-04-13T13:08:18Z

Backported to tal in commit 09696c9.

commit 09696c942a4aa28423a9af07340c512554b5264b
Author: Jason Frey <fryguy9@gmail.com>
Date:   Mon Apr 13 09:06:58 2026 -0400

    Merge pull request #9954 from jrafanie/fix-custom-buttons-and-widgets-with-role-names-that-can-change
    
    Fix custom buttons and widgets with role names or group descriptions that can change
    
    (cherry picked from commit 81c6a891a8ecf75b6431b3a678631ade70db9859)

…-with-role-names-that-can-change Fix custom buttons and widgets with role names or group descriptions that can change (cherry picked from commit 81c6a89)

jrafanie requested a review from a team as a code owner April 3, 2026 16:31

jrafanie added a commit to jrafanie/manageiq that referenced this pull request Apr 3, 2026

Push down the visibility value logic to the widget model

4aef9d5

Extracted from and for usage in: ManageIQ/manageiq-ui-classic#9954

jrafanie added the wip label Apr 3, 2026

jrafanie mentioned this pull request Apr 3, 2026

Push down the visibility value logic to the widget model ManageIQ/manageiq#23803

Merged

miq-bot removed the wip label Apr 3, 2026

jrafanie commented Apr 3, 2026

View reviewed changes

Fryguy closed this Apr 3, 2026

Fryguy reopened this Apr 3, 2026

Fryguy reviewed Apr 3, 2026

View reviewed changes

Comment thread spec/helpers/report_helper_spec.rb Outdated

Fryguy reviewed Apr 3, 2026

View reviewed changes

Comment thread spec/helpers/report_helper_spec.rb Outdated

Fryguy self-assigned this Apr 3, 2026

Fryguy added the bug label Apr 3, 2026

jrafanie changed the title ~~Fix custom buttons and widgets with role names that can change~~ [WIP] Fix custom buttons and widgets with role names that can change Apr 3, 2026

miq-bot added the wip label Apr 3, 2026

jrafanie added 4 commits April 7, 2026 10:25

Simplify the report helper by pushing widgets logic to the model

2281d43

ManageIQ/manageiq#23803

Consistently use numbers for group/role ids

c776fe9

Previously, with the data migration and UI code that stored ids as numbers in the database, the code that lit up the checkboxes failed to match ids as strings with the numbers from the database.

jrafanie mentioned this pull request Apr 7, 2026

Fix an issue with blank visibility ManageIQ/manageiq#23805

Merged

jrafanie force-pushed the fix-custom-buttons-and-widgets-with-role-names-that-can-change branch from fd9260d to c776fe9 Compare April 7, 2026 16:10

jrafanie closed this Apr 7, 2026

jrafanie reopened this Apr 7, 2026

jrafanie closed this Apr 7, 2026

jrafanie reopened this Apr 7, 2026

jrafanie changed the title ~~[WIP] Fix custom buttons and widgets with role names that can change~~ [WIP] Fix custom buttons and widgets with role names and group descriptions that can change Apr 7, 2026

jrafanie changed the title ~~[WIP] Fix custom buttons and widgets with role names and group descriptions that can change~~ [WIP] Fix custom buttons and widgets with role names or group descriptions that can change Apr 7, 2026

jrafanie force-pushed the fix-custom-buttons-and-widgets-with-role-names-that-can-change branch from fd10bef to d0ead1c Compare April 7, 2026 20:09

jrafanie commented Apr 7, 2026

View reviewed changes

jrafanie added 2 commits April 7, 2026 16:33

Sort roles/groups (numerically by id) with space separator

31b0b39

jrafanie force-pushed the fix-custom-buttons-and-widgets-with-role-names-that-can-change branch from d0ead1c to 31b0b39 Compare April 7, 2026 20:34

jrafanie commented Apr 8, 2026

View reviewed changes

jrafanie added the tal/yes? label Apr 8, 2026

Fryguy reviewed Apr 9, 2026

View reviewed changes

Restore rbac filtering of roles/groups to what the user can access

f679c1c

jrafanie removed the wip label Apr 10, 2026

jrafanie changed the title ~~[WIP] Fix custom buttons and widgets with role names or group descriptions that can change~~ Fix custom buttons and widgets with role names or group descriptions that can change Apr 10, 2026

Fryguy approved these changes Apr 10, 2026

View reviewed changes

GilbertCherrie approved these changes Apr 10, 2026

View reviewed changes

jrafanie added tal/yes and removed tal/yes? labels Apr 10, 2026

Fryguy added tal/yes and removed tal/yes labels Apr 13, 2026

Fryguy merged commit 81c6a89 into ManageIQ:master Apr 13, 2026
11 checks passed

Fryguy added tal/backported and removed tal/yes labels Apr 13, 2026

jrafanie deleted the fix-custom-buttons-and-widgets-with-role-names-that-can-change branch April 13, 2026 17:07

Uh oh!

Conversation

jrafanie commented Apr 3, 2026 • edited by Fryguy Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrafanie Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrafanie Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jrafanie Apr 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miq-bot commented Apr 10, 2026

Uh oh!

jrafanie commented Apr 10, 2026

Uh oh!

GilbertCherrie left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fryguy commented Apr 13, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

jrafanie commented Apr 3, 2026 •

edited by Fryguy

Loading

jrafanie Apr 7, 2026 •

edited

Loading

jrafanie Apr 7, 2026 •

edited

Loading

jrafanie Apr 10, 2026 •

edited

Loading