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

Pull in rhecosystem changes (need FEO support) #1635

Merged
merged 78 commits into from
Jul 31, 2024

Conversation

florkbr
Copy link
Contributor

@florkbr florkbr commented Jul 17, 2024

Description

We originally migrated rbac UI in-boundary via a different public repo: https://github.com/RHEcosystemAppEng/insights-rbac-ui. That repo is out of date with this one and also has changes on top of it for itless.

I've attempted to merge those changes back into rbac-ui in this PR for maintainability, consistency with what we do in our other repos, and to fast forward the UI. We will need extensive regression testing to ensure this PR does not regress either env.

I've opted NOT to squash these commits for historical reasons (as these changes were made by other engineers). I imagine we will also have some follow up around best practices, tech debt, etc.


Screenshots

N/A - ideally in commercial we should see no change


Checklist ☑️

  • PR only fixes one issue or story
  • Change reviewed for extraneous code
  • UI best practices adhered to
  • Commits squashed and meaningfully named
  • All PR checks pass locally (build, lint, test, E2E)

  • (Optional) QE: Needs QE attention (OUIA changed, perceived impact to tests, no test coverage)
  • (Optional) QE: Has been mentioned
  • (Optional) UX: Needs UX attention (end user UX modified, missing designs)
  • (Optional) UX: Has been mentioned

yzhao583 added 30 commits June 1, 2023 15:52
…he-user-page-of-the-IAM-console-to-add-the-activate/deactivate-user-feature

Modified the user page to add activate user or deactivate user feature
…he-RBAC-console-applying-new-UXD-to-support-bulk-actions-for-activate/deactivate-users

Modified the activate/deactivate user feature, and added the invite users feature
…eference-to-UGC

RHBKAAS-91: Remove reference to UGC for FEDRAMP
…ch-to-change-the-isAdmin-status-in-the-user-table

RHBKAAS-92: Add switch to change the is admin status in the user table
…es-when-user-is-not-org-admin

RHBKAAS-79: Hide pages when user is not org admin
…9-add-ability-to-avoid-current-users-to-deactivate-themselves

RHBKAAS-139: Add ability to avoid current users to deactivate themselves
@florkbr florkbr changed the title Pull in rhecosystem changes (need FEO support) [DO NOT MERGE] Pull in rhecosystem changes (need FEO support) Jul 17, 2024
@florkbr florkbr force-pushed the pull-in-rhecosystem-changes branch from 8298e2f to 4e8fa84 Compare July 18, 2024 18:25
@florkbr florkbr marked this pull request as ready for review July 18, 2024 18:34
@florkbr florkbr changed the title [DO NOT MERGE] Pull in rhecosystem changes (need FEO support) Pull in rhecosystem changes (need FEO support) Jul 18, 2024
setDeleteInfo({
title: removeText,
confirmButtonLabel: removeText,
text: removeModalText(
multipleMembersSelected ? selectedMembers.length : selectedMembers[0].uuid,
multipleMembersSelected ? selectedMembers.length : chrome.isFedramp ? selectedMembers[0].label : selectedMembers[0].uuid,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fhlavac I think I recall you converting us to using UUID (no worries if I'm wrong). Can you see if this logic seems valid or if label will no longer work with the changes we've made in master?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@fhlavac any insights here?

@florkbr florkbr force-pushed the pull-in-rhecosystem-changes branch from 4e8fa84 to b6f0e35 Compare July 18, 2024 21:23
@florkbr florkbr requested a review from fhlavac July 18, 2024 22:04
@@ -0,0 +1,4 @@
export const isEphem = insights.chrome.getEnvironment() === 'ephem';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a bit different from our chrome.isFedramp helper as we will use this to lookup the correct invite API base URL (from a static file in each env).

Copy link
Contributor

@aneelac22 aneelac22 left a comment

Choose a reason for hiding this comment

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

just curious about few details

src/Routing.tsx Outdated Show resolved Hide resolved
src/helpers/user/user-helper.js Show resolved Hide resolved
Copy link
Contributor

@Hyperkid123 Hyperkid123 left a comment

Choose a reason for hiding this comment

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

Just some high level comments. I'll leave the rest for the UI folks.

src/Routing.tsx Outdated Show resolved Hide resolved
@@ -8,6 +9,145 @@ const principalStatusApiMap = {
Inactive: 'disabled',
All: 'all',
};

const getBaseUrl = (url) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Note. We should be feeding it from chrome service and configure a env variable in the OC namespace. @florkbr can you create a ticket for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

src/helpers/user/user-helper.js Outdated Show resolved Hide resolved
src/helpers/user/user-helper.js Show resolved Hide resolved
src/helpers/user/user-helper.js Outdated Show resolved Hide resolved
src/smart-components/group/member/group-members.js Outdated Show resolved Hide resolved
src/smart-components/group/member/group-members.js Outdated Show resolved Hide resolved
src/smart-components/user/users.tsx Outdated Show resolved Hide resolved
src/smart-components/user/users.tsx Outdated Show resolved Hide resolved
src/smart-components/user/users.tsx Outdated Show resolved Hide resolved
src/Messages.js Outdated Show resolved Hide resolved
@florkbr florkbr force-pushed the pull-in-rhecosystem-changes branch from 0b6a419 to c011467 Compare July 22, 2024 21:52
Copy link
Contributor

@Hyperkid123 Hyperkid123 left a comment

Choose a reason for hiding this comment

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

Looks good now. I'll let @fhlavac and co to check if this works fine in commercial env.

@florkbr florkbr force-pushed the pull-in-rhecosystem-changes branch 2 times, most recently from 586cff7 to 64d2279 Compare July 29, 2024 22:55
@florkbr florkbr force-pushed the pull-in-rhecosystem-changes branch from 64d2279 to d354f56 Compare July 29, 2024 23:04
Copy link
Contributor

@fhlavac fhlavac left a comment

Choose a reason for hiding this comment

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

@florkbr tested locally and didn't found any issues for the commercial env

@fhlavac fhlavac merged commit 15ea84d into RedHatInsights:master Jul 31, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants