Skip to content

Convert user form to React#8916

Merged
jrafanie merged 7 commits into
ManageIQ:masterfrom
GilbertCherrie:convert_user_form
Apr 8, 2026
Merged

Convert user form to React#8916
jrafanie merged 7 commits into
ManageIQ:masterfrom
GilbertCherrie:convert_user_form

Conversation

@GilbertCherrie

@GilbertCherrie GilbertCherrie commented Sep 11, 2023

Copy link
Copy Markdown
Member

Fixes: and #6837, #6903, and #6904

Converted user form to React and added Cypress tests for the form.

Before:

Edit Admin User Form:
AdminUserBefore
Add User Form:
AddUserBefore
Edit User Form:
EditUserBefore
Copy User Form:
CopyUserBefore

After:

Edit Admin User Form:
AdminUserAfter
Add User Form:
AddUserAfter
Edit User Form:
EditUserAfter
Copy User Form:
CopyUserAfter

@GilbertCherrie GilbertCherrie requested a review from a team as a code owner September 11, 2023 13:11
@miq-bot miq-bot added the wip label Sep 11, 2023
Comment thread app/javascript/components/user-form/index.jsx Outdated
Comment thread app/javascript/components/user-form/index.jsx Outdated
Comment thread app/javascript/components/user-form/index.jsx Outdated
@GilbertCherrie GilbertCherrie force-pushed the convert_user_form branch 2 times, most recently from a959e72 to ffd13db Compare October 2, 2023 19:51
@GilbertCherrie GilbertCherrie force-pushed the convert_user_form branch 5 times, most recently from 444bd81 to 0b91982 Compare October 6, 2023 15:54
@GilbertCherrie GilbertCherrie mentioned this pull request Oct 24, 2023
15 tasks
@miq-bot

miq-bot commented Feb 19, 2024

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 added the stale label Feb 19, 2024
@GilbertCherrie GilbertCherrie force-pushed the convert_user_form branch 3 times, most recently from f181f47 to 55e116c Compare May 6, 2024 16:43
@kbrock

kbrock commented May 9, 2024

Copy link
Copy Markdown
Member

as mentioned elsewhere, passing attributes=miq_groups will bring back the groups

@GilbertCherrie GilbertCherrie changed the title [WIP] Convert user form to React Convert user form to React May 27, 2024
@GilbertCherrie GilbertCherrie force-pushed the convert_user_form branch 3 times, most recently from d34575a to 63b757b Compare May 27, 2024 20:13
@asirvadAbrahamVarghese asirvadAbrahamVarghese force-pushed the convert_user_form branch 2 times, most recently from 5e606da to d8d1a21 Compare March 23, 2026 14:43
Comment thread app/javascript/components/user-form/helper.js Outdated
@GilbertCherrie

Copy link
Copy Markdown
Member Author

@elsamaryv can you please review again

@elsamaryv

Copy link
Copy Markdown
Contributor

@elsamaryv can you please review again

LGTM. @jrafanie do you want to take another look at this PR?

});
}
miqSparkleOff();
} else {

@jrafanie jrafanie Apr 8, 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.

These 3 conditional blocks look very similar. It's hard to see the difference at a glance. How is this one 👇 different then the first one ☝️? I believe the first one is actually a "edit" and the one below is a "create". The second one has no credentials change. They all say "was saved" so it's not clear. We should consider refactoring and make the differences more obvious.

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.

Yeah the first 2 branches are edit (1 with password change and 1 without) and the last one is to create user. All 3 say 'was saved' as this is what the old form did I believe.

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, maybe it can be refactored in a followup so it's clearer what's different. We can change what's presented in a followup if it makes sense, such as is if it's consistently done that way elsewhere.

Comment thread app/javascript/components/user-form/index.jsx Outdated
Comment thread app/javascript/components/user-form/index.jsx
Comment thread app/javascript/components/user-form/index.jsx Outdated
Comment thread app/javascript/components/selected-groups-list/index.jsx Outdated
Comment thread app/javascript/components/user-form/helper.js Outdated
Comment thread app/javascript/components/selected-groups-list/index.jsx
Comment thread app/javascript/components/user-form/index.jsx Outdated
Comment thread app/javascript/components/user-form/helper.js
Comment thread app/javascript/components/user-form/helper.js Outdated
Comment thread app/javascript/components/user-form/index.jsx Outdated

@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. The other refactorings can be done in a followup.

@miq-bot

miq-bot commented Apr 8, 2026

Copy link
Copy Markdown
Member

Checked commits GilbertCherrie/manageiq-ui-classic@74105ae~...311a08c with ruby 3.3.10, rubocop 1.86.0, haml-lint 0.72.0, and yamllint 1.37.1
7 files checked, 6 offenses detected

app/controllers/ops_controller/ops_rbac.rb

app/views/ops/_rbac_user_details.html.haml

  • ⚠️ - Line 3 - Avoid using instance variables in partials views
  • ⚠️ - Line 4 - Style/RedundantConstantBase: Remove redundant ::.
  • ⚠️ - Line 8 - Avoid using instance variables in partials views
  • ⚠️ - Line 8 - Line is too long. [122/80]
  • ⚠️ - Line 8 - Style/RedundantParentheses: Don't use parentheses around a method call.

@jrafanie

jrafanie commented Apr 8, 2026

Copy link
Copy Markdown
Member

@GilbertCherrie I'm going to merge. I didn't notice the easy cops like trailing whitespace. Can you review and do a followup on the easy ones? Thanks!

@jrafanie jrafanie merged commit b59717b into ManageIQ:master Apr 8, 2026
11 checks passed
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.

7 participants