Skip to content

Fix request page tag value#9250

Merged
jrafanie merged 3 commits into
ManageIQ:masterfrom
GilbertCherrie:fix_request_page_tag_value
Apr 8, 2026
Merged

Fix request page tag value#9250
jrafanie merged 3 commits into
ManageIQ:masterfrom
GilbertCherrie:fix_request_page_tag_value

Conversation

@GilbertCherrie

@GilbertCherrie GilbertCherrie commented Aug 15, 2024

Copy link
Copy Markdown
Member

Fixes how dialog tag field values are displayed on the service request page.

Before:
The first tag dropdown is a multi-select with 3 values selected, but the request page only shows the first value.
Screenshot 2026-03-17 at 6 37 45 PM

After:
This PR fixes this bug to show all the selected values.
Screenshot 2026-03-17 at 6 36 00 PM

@miq-bot

miq-bot commented Feb 10, 2025

Copy link
Copy Markdown
Member

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

1 similar comment
@miq-bot

miq-bot commented May 30, 2025

Copy link
Copy Markdown
Member

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@miq-bot

miq-bot commented Jan 12, 2026

Copy link
Copy Markdown
Member

This pull request has been automatically marked as stale because it has not been updated for at least 3 months.

If these changes are still valid, please remove the stale label, make any changes requested by reviewers (if any), and ensure that this issue is being looked at by the assigned/reviewer(s).

@jrafanie

Copy link
Copy Markdown
Member

@GilbertCherrie sorry, I lost this one, what's the status on it?

@GilbertCherrie GilbertCherrie force-pushed the fix_request_page_tag_value branch from 328d400 to 1c69831 Compare March 17, 2026 21:55
@GilbertCherrie GilbertCherrie force-pushed the fix_request_page_tag_value branch 2 times, most recently from 1e075e0 to 57316b3 Compare March 17, 2026 21:58
Comment thread app/views/miq_request/_request_dialog_details.html.haml Outdated
@GilbertCherrie GilbertCherrie force-pushed the fix_request_page_tag_value branch from 57316b3 to 69e2d59 Compare March 17, 2026 22:35
@GilbertCherrie

Copy link
Copy Markdown
Member Author

@Fryguy @jrafanie I updated this PR, and it should be good to go now. I left some comments in this code to remind us that if we ever fix the automation engine to return array values instead of strings separated by "\u001F" then we need to update the code for this file.

@GilbertCherrie GilbertCherrie force-pushed the fix_request_page_tag_value branch 2 times, most recently from 39ccd49 to cd34ac0 Compare March 17, 2026 22:42
@GilbertCherrie GilbertCherrie force-pushed the fix_request_page_tag_value branch from cd34ac0 to b90708b Compare March 17, 2026 22:53
- classifications = value.split(',').map do |c|
- classification = Classification.find_by(id: c.split('::').second)
- classification.description if classification
- # If the automation engine is changed to return arrays instead of strings separated by "\u001F" then we need to change the code below

@jrafanie jrafanie Mar 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure why we have \u001F as a string separator... does this align with what we're doing in

#9482
ManageIQ/manageiq-automation_engine#545

should this PR or 9482 be part of ManageIQ/manageiq-content#773, which has a list of a bunch of work items where some are completed.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Instead of arrays, currently we use \u001F as a separator. This PR fixed this issue: ManageIQ/manageiq-automation_engine#545 and enabled arrays, but after problems were found, it was reverted. This PR is independent of ManageIQ/manageiq-content#773, which contains the work items that need to be completed to allow for arrays and get rid of \u001F. Once that is all done we then need a follow up PR in the UI-classic repo to handle the arrays instead of strings.

@jrafanie jrafanie Mar 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

sent in chat, but if you can clarify in the 773 PR list of work items what work is done for the current way (this PR) and what is for the future way, (remove this separator and use arrays), as it's hard to follow what's fixing the current way and what's for the future (arrays) way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

ok, ManageIQ/manageiq-content#773 looks better. Thanks for clarifying.

@Fryguy Fryguy Mar 31, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure why we have \u001F as a string separator

Essentially, if you have an Array of objects in automate, we used to serialize those as a comma-separated list (I think through the automation URIs, since automate uses URIs everywhere). This works fine for integers, but for strings, the strings could themselves have commas in them. So you'd have this problem where the user would create "abc", "def,ghi", "jkl", which would render to the URI as abc,def,ghi,jkl, and in turn be deserialized incorrectly. So, the decision was made to change both the automate side and the UI side to separate by \u001F instead of comma. The reason \u001F is chosen is because \u001F is defined as the "Unit Separator". (i.e. it's a control character specifically for this purpose of acting as a data separator). Since it's a control characters, it's incredibly unlikely to be in usable user data.

See ManageIQ/manageiq-automation_engine#484 (comment)
Original Issue: ManageIQ/manageiq-automation_engine#480

@jrafanie jrafanie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As discussed, we'll add cypress tests for this and the other pages in PRs linked in ManageIQ/manageiq-content#773 as we'll likely need to make some changes to make it work with arrays instead of delimited strings so it might make sense to get the expected page layout in cypress before doing that work so we can verify the page is correct when converted to use arrays.

Comment thread app/views/miq_request/_request_dialog_details.html.haml Outdated
Comment thread app/views/miq_request/_request_dialog_details.html.haml Outdated
@jrafanie

jrafanie commented Apr 8, 2026

Copy link
Copy Markdown
Member

@GilbertCherrie LGTM, can the rubocops be fixed or does it make sense to ignore them?

@GilbertCherrie GilbertCherrie force-pushed the fix_request_page_tag_value branch from c113327 to 50fdfdb Compare April 8, 2026 18:45
@miq-bot

miq-bot commented Apr 8, 2026

Copy link
Copy Markdown
Member

Checked commits GilbertCherrie/manageiq-ui-classic@b90708b~...50fdfdb with ruby 3.3.10, rubocop 1.86.0, haml-lint 0.72.0, and yamllint 1.37.1
1 file checked, 4 offenses detected

app/views/miq_request/_request_dialog_details.html.haml

  • ⚠️ - Line 31 - 2 consecutive comments can be merged into one
  • ⚠️ - Line 37 - 2 consecutive comments can be merged into one
  • ⚠️ - Line 39 - Line is too long. [87/80]
  • ⚠️ - Line 40 - Line is too long. [93/80]

@jrafanie jrafanie left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM, just waiting on green

@jrafanie jrafanie merged commit 2302577 into ManageIQ:master Apr 8, 2026
11 checks passed
@GilbertCherrie GilbertCherrie deleted the fix_request_page_tag_value branch April 8, 2026 19:36
@Fryguy Fryguy added tal/yes and removed tal/yes? labels Apr 13, 2026
@Fryguy

Fryguy commented Apr 13, 2026

Copy link
Copy Markdown
Member

Backported to tal in commit e8ad716.

commit e8ad716ebef28b146a063b63234024c2c61ee1c1
Author: Joe Rafaniello <jrafanie@users.noreply.github.com>
Date:   Wed Apr 8 15:32:23 2026 -0400

    Merge pull request #9250 from GilbertCherrie/fix_request_page_tag_value
    
    Fix request page tag value
    
    (cherry picked from commit 2302577ec3568906509ea952e94ffe498075db12)

Fryguy pushed a commit that referenced this pull request Apr 13, 2026
Fix request page tag value

(cherry picked from commit 2302577)
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.

4 participants