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

Stage users #181

Merged
merged 1 commit into from
Nov 15, 2023
Merged

Stage users #181

merged 1 commit into from
Nov 15, 2023

Conversation

mreynolds389
Copy link
Collaborator

There are 2 commits breaking up this work. Next, Preserved Users will be added (that PR will be much smaller)

@mreynolds389 mreynolds389 requested a review from carma12 November 2, 2023 14:37
@mreynolds389 mreynolds389 force-pushed the stage_users branch 2 times, most recently from 0f07089 to 0e530d9 Compare November 3, 2023 16:10
Copy link
Collaborator

@carma12 carma12 left a comment

Choose a reason for hiding this comment

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

Some changes were requested. Also, some things that I noticed through the UI (not mentioned in the inline comments):

  • The status column is not needed in the 'Stage users' main table.
  • Under 'Stage users' > 'Add user':
    • The question mark icon in the Class field is not needed in this form.
    • The No private group checkbox field is not needed in this form.
    • The GID field is not needed in this form.

src/components/modals/AddUser.tsx Outdated Show resolved Hide resolved
src/components/modals/AddUser.tsx Outdated Show resolved Hide resolved
@@ -473,7 +496,7 @@ const AddUser = (props: PropsToAddUser) => {
},
{
id: "gid-form",
name: "GID",
name: GIDTitle,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Apparently, this field should be only available for 'Active' users, not 'Staged' ones. So this should be adapted to be here only if the from prop is "stage-users".

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Apparently, this field should be only available for 'Active' users, not 'Staged' ones. So this should be adapted to be here only if the from prop is "stage-users".

It is required for stage users

src/pages/ActiveUsers/ActiveUsers.tsx Show resolved Hide resolved
src/pages/StageUsers/StageUsers.tsx Outdated Show resolved Hide resolved
src/pages/StageUsers/StageUsers.tsx Outdated Show resolved Hide resolved
@mreynolds389
Copy link
Collaborator Author

@carma12 all changes made (I had to keep some changes for the modal field validation to work correctly). Please review...

@mreynolds389 mreynolds389 force-pushed the stage_users branch 2 times, most recently from 128e8b3 to bdb3e15 Compare November 7, 2023 13:56
Copy link
Collaborator

@carma12 carma12 left a comment

Choose a reason for hiding this comment

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

I added more comments requesting some tiny bits in the code (mostly naming and some Qs).

Apart from that, it seems that some changes in the AddUser component have been pushed in both commits (it was hard for me to find the recently requested changes). No need to correct that.

src/components/modals/ActivateStageUsers.tsx Outdated Show resolved Hide resolved
{
id: "no-members",
pfComponent: (
<Checkbox
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the purpose of this checkbox? This is not in the current WebUI either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh yeah, this is an option you can use with the CLI when activating users. Figured if the CLI can do it, then so should the UI.

[vagrant@server ~]$ ipa stageuser-activate --help
Usage: ipa [global-options] stageuser-activate LOGIN [options]

Activate a stage user.
Options:
  -h, --help    show this help message and exit
  --all         Retrieve and print all attributes from the server. Affects
                command output.
  --raw         Print entries as stored on the server. Only affects output
                format.
  --no-members  Suppress processing of membership attributes.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, didn't know about this. I think it is a nice feature to keep then :)

src/components/modals/ActivateStageUsers.tsx Outdated Show resolved Hide resolved
src/components/modals/ActivateStageUsers.tsx Outdated Show resolved Hide resolved
@mreynolds389
Copy link
Collaborator Author

I added more comments requesting some tiny bits in the code (mostly naming and some Qs).

Apart from that, it seems that some changes in the AddUser component have been pushed in both commits (it was hard for me to find the recently requested changes). No need to correct that.

Ok I'm not sure how that happened :-( This should be a single commit (not 2). Hmm I might have to recreate this PR from scratch...

@mreynolds389 mreynolds389 force-pushed the stage_users branch 2 times, most recently from 3922e7e to adb3a1d Compare November 8, 2023 19:21
@mreynolds389
Copy link
Collaborator Author

I added more comments requesting some tiny bits in the code (mostly naming and some Qs).
Apart from that, it seems that some changes in the AddUser component have been pushed in both commits (it was hard for me to find the recently requested changes). No need to correct that.

Ok I'm not sure how that happened :-( This should be a single commit (not 2). Hmm I might have to recreate this PR from scratch...

Ok, its all fixed now

Copy link
Collaborator

@carma12 carma12 left a comment

Choose a reason for hiding this comment

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

LGTM!

As we are still waiting for a fix for the gating integration tests problem, the merge of this PR will wait until the fix is released.

Context/TL;DR: The image runners that we are using for the CI are experiencing some errors that will be solved in the next release of macos-12 next week. Link1. Link2.)

{
id: "no-members",
pfComponent: (
<Checkbox
Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, didn't know about this. I think it is a nice feature to keep then :)

@miskopo
Copy link
Member

miskopo commented Nov 15, 2023

Since the integration tests are non-functional as of now due to the changes to IP address' ranges assigned to the runners, I recommend we treat them as non-blocking.
I have carefully reviewed your changes and they seem to be working as intended. Furthermore, current set of integration tests do not cover operations with Staged users.
The effort to fix the CI is tracked in #622.

Given these circumstances, I approve of merging despite integration tests are being skipped.

@miskopo miskopo self-requested a review November 15, 2023 15:14
@miskopo miskopo added the ack Pull Request approved, it can be merged label Nov 15, 2023
@mreynolds389 mreynolds389 merged commit 57bf689 into freeipa:main Nov 15, 2023
1 check passed
@mreynolds389 mreynolds389 deleted the stage_users branch November 15, 2023 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ack Pull Request approved, it can be merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants