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.

Sign up for GitHub

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

Jump to bottom

Automember > 'User group rules' - Action buttons #626

Merged
merged 6 commits into from
Feb 7, 2025
Merged

Automember > 'User group rules' - Action buttons #626

merged 6 commits into from
Feb 7, 2025

Conversation

carma12
Copy link
Collaborator

@carma12 carma12 commented Jan 29, 2025

The action buttons for the 'User group rules' page have been implemented. This includes:

  • 'Add'
  • 'Delete'
  • Set default group via Selector component in toolbar
  • Integration test to check its functionality

This PR has been created based on this one: #625

@carma12 carma12 added needs-review This PR is waiting on a review tests PR related to testing labels Jan 29, 2025
@carma12 carma12 mentioned this pull request Jan 31, 2025
Open
@carma12 carma12 added WIP Work in Progress (do not merge) and removed needs-review This PR is waiting on a review labels Jan 31, 2025
@carma12 carma12 force-pushed the automem-user-groups-buttons branch 2 times, most recently from 586f9e5 to 9ef61ff Compare January 31, 2025 14:13
@carma12 carma12 added needs-review This PR is waiting on a review and removed WIP Work in Progress (do not merge) labels Jan 31, 2025
@carma12 carma12 force-pushed the automem-user-groups-buttons branch from 9ef61ff to 048f05c Compare January 31, 2025 14:17
@mreynolds389
Copy link
Collaborator

There are failing tests, looks possibly related? Please confirm if this is ready for review

@carma12 carma12 force-pushed the automem-user-groups-buttons branch from 048f05c to d0a8d97 Compare January 31, 2025 16:21
@carma12
Copy link
Collaborator Author

carma12 commented Jan 31, 2025

There are failing tests, looks possibly related? Please confirm if this is ready for review

I relaunched the tests when rebased, so I guess they should be ok.

I will double check them on Monday if some of them failed. Otherwise, green light to review :)

@carma12 carma12 force-pushed the automem-user-groups-buttons branch 2 times, most recently from ecd6691 to a8c3629 Compare February 3, 2025 09:05
@carma12
Copy link
Collaborator Author

carma12 commented Feb 3, 2025

I need to fix an issue affecting delete and add operations. Will let know when this is ready.

@carma12 carma12 force-pushed the automem-user-groups-buttons branch 4 times, most recently from 0d599e9 to 9100313 Compare February 4, 2025 12:19
@carma12
Copy link
Collaborator Author

carma12 commented Feb 5, 2025 •
edited
Loading

I need to fix an issue affecting delete and add operations. Will let know when this is ready.

@mreynolds389 - Since some tests were failing in multiple PRs (not only in this one), I created a new PR to fix them: #631

The new PR (#631) should be reviewed and merged first before this one (#626) to rebase the changes. This will hopefully keep the tests green.

@carma12
Automember > User group rules
2d0d985 
The Automember > 'User group rules' main
page must take the automember group's data,
the associated user groups, and its default
group.

Signed-off-by: Carla Martinez <[email protected]>
@carma12
'Add rules' functionality
f609bf5 
The 'Add' action button on Automembers >
'User groups' main page allows to associate
user group rules.

Signed-off-by: Carla Martinez <[email protected]>
@carma12
'Delete rules' functionality
bc82ceb 
The 'Delete' button functionality must be
implemented to handle the deletion of the
Automember rules (User groups).

Signed-off-by: Carla Martinez <[email protected]>
@carma12
'Default group' selector's functionality
aa55d64 
The Selector located in the toolbar should show
the default group in the automembers page. This
functionality should also allow to modify the
default group via API command and show the new
one.

Signed-off-by: Carla Martinez <[email protected]>
@carma12
Add integration test
6fabf1c 
The integration test must check the already
implemented interaction of the action buttons.

Signed-off-by: Carla Martinez <[email protected]>
@carma12 carma12 force-pushed the automem-user-groups-buttons branch from 9100313 to 6fabf1c Compare February 5, 2025 13:53
@carma12
Copy link
Collaborator Author

carma12 commented Feb 5, 2025

Already rebased this PR with the recent changes. Waiting the tests to execute.

@carma12 carma12 force-pushed the automem-user-groups-buttons branch from fe2d0f0 to 7c4b307 Compare February 5, 2025 15:36
@carma12
Copy link
Collaborator Author

carma12 commented Feb 5, 2025

The sudo_rules_settings test is failing again in this PR while it was not failing in #631. This is a bit confusing as the error is not consistent.

I created an extra commit to adjust it from here.

@carma12 carma12 force-pushed the automem-user-groups-buttons branch 9 times, most recently from e54b690 to 271ede8 Compare February 6, 2025 15:38
@carma12
Fix sudo rules settings test
019c1a2 
Sudo rules > Settings test is failing when trying to
add a new sudo allow command group. This needs to be
selected from a dual list selector and it fails
when clicking the arrow to perform a global search.

Signed-off-by: Carla Martinez <[email protected]>
@carma12 carma12 force-pushed the automem-user-groups-buttons branch from 271ede8 to 019c1a2 Compare February 7, 2025 09:29
@carma12
Copy link
Collaborator Author

carma12 commented Feb 7, 2025

@mreynolds389 - It seems the problem is fixed 🎉 . Feel free to keep reviewing :)

mreynolds389
mreynolds389 reviewed Feb 7, 2025
View reviewed changes
src/services/rpcAutomember.ts
@@ -24,6 +26,7 @@ import {
* API commands:
* - automember_default_group_show: https://freeipa.readthedocs.io/en/latest/api/automember_default_group_show.html
* - automember_find: https://freeipa.readthedocs.io/en/latest/api/automember_find.html
* - automember_add: https://freeipa.readthedocs.io/en/latest/api/automember_add.html
*/
Copy link
Collaborator

@mreynolds389 mreynolds389 Feb 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Minor but I guess you should add automember_del and automember_default_group_set

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

True. I can add them in another PR as it is not critical.

@mreynolds389 mreynolds389 added the ack Pull Request approved, it can be merged label Feb 7, 2025
@carma12 carma12 merged commit 4be963d into freeipa:main Feb 7, 2025
4 checks passed
@carma12 carma12 mentioned this pull request Feb 7, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mreynolds389 mreynolds389 mreynolds389 approved these changes

@miskopo miskopo Awaiting requested review from miskopo

Assignees
No one assigned
Labels
ack Pull Request approved, it can be merged needs-review This PR is waiting on a review tests PR related to testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@carma12 @mreynolds389