Skip to content

'Privileges' main page#1134

Merged
carma12 merged 1 commit into
freeipa:mainfrom
carma12:privileges-main-page
Jul 15, 2026
Merged

'Privileges' main page#1134
carma12 merged 1 commit into
freeipa:mainfrom
carma12:privileges-main-page

Conversation

@carma12

@carma12 carma12 commented Jul 2, 2026

Copy link
Copy Markdown
Collaborator

The 'Privileges' page should be located under 'Role-based access control' section and must show all the available privileges associated to the IPA server.

This PR has been built in the top of #1126 , so it must be merged first.

Assisted-by: Claude noreply@anthropic.com

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Sorry @carma12, your pull request is larger than the review limit of 150000 diff characters

@veronnicka

Copy link
Copy Markdown
Contributor

There is really a lot of changes this PR is built on, so I will wait for the rebase.

@veronnicka
veronnicka force-pushed the privileges-main-page branch from b2aa3d1 to dd1985d Compare July 6, 2026 14:13
@carma12
carma12 force-pushed the privileges-main-page branch from dd1985d to fc52789 Compare July 9, 2026 08:50
@carma12 carma12 added needs-review This PR is waiting on a review and removed Needs Rebase labels Jul 9, 2026
@carma12
carma12 force-pushed the privileges-main-page branch from fc52789 to c3a0a5e Compare July 9, 2026 08:51
@carma12

carma12 commented Jul 9, 2026

Copy link
Copy Markdown
Collaborator Author

@veronnicka - This PR is now ready to review.

@carma12
carma12 force-pushed the privileges-main-page branch from c3a0a5e to 8f4b887 Compare July 9, 2026 13:11

@duzda duzda left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, the help panel is not working

Comment thread src/components/modals/PrivilegeModals/AddPrivilegeModal.tsx Outdated
Comment thread src/pages/Privileges/Privileges.tsx Outdated
@carma12
carma12 force-pushed the privileges-main-page branch from 8f4b887 to 61a7810 Compare July 10, 2026 09:40
@carma12
carma12 requested a review from duzda July 10, 2026 09:40
@duzda

duzda commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Hi @carma12,

the help panel is not working

@carma12

carma12 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

Hi @carma12,

the help panel is not working

I completely overlooked this, sorry 😅 Will fix...

@carma12
carma12 force-pushed the privileges-main-page branch from 61a7810 to 2c4d532 Compare July 10, 2026 12:03
@carma12

carma12 commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator Author

@duzda - The code has been fixed based on your feedback (including the missing Contextual help links panel).

@veronnicka veronnicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hi, theres a bug when searching on a page other than the first one(the search on the first page works)

Search uses the current pagination offset (firstIdx / lastIdx) and a fix I propose would be to look in all of the results, so something like :

const submitSearchValue = () => {
  setPage(1);
  ...
  searchPrivileges({
    ...
    startIdx: 0,
    stopIdx: perPage,
  })
}

@veronnicka

Copy link
Copy Markdown
Contributor

Another thing I noticed, the search is not responsive- it only searches the value after clicking enter. The other pages implemented do have the reponsive approach, so the list in the table changes with each key put into the search. Is this something we want to address?

@carma12

carma12 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Hi, theres a bug when searching on a page other than the first one(the search on the first page works)

Search uses the current pagination offset (firstIdx / lastIdx) and a fix I propose would be to look in all of the results, so something like :

const submitSearchValue = () => {
  setPage(1);
  ...
  searchPrivileges({
    ...
    startIdx: 0,
    stopIdx: perPage,
  })
}

Nice catch, will check this asap...

@carma12

carma12 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Another thing I noticed, the search is not responsive- it only searches the value after clicking enter. The other pages implemented do have the reponsive approach, so the list in the table changes with each key put into the search. Is this something we want to address?

You are right: there is an inconsistency on how the data is being searched. I will fix this in this PR, but also will open a new PR to do it in other pages affected.

@carma12

carma12 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Hi, theres a bug when searching on a page other than the first one(the search on the first page works)
Search uses the current pagination offset (firstIdx / lastIdx) and a fix I propose would be to look in all of the results, so something like :

const submitSearchValue = () => {
  setPage(1);
  ...
  searchPrivileges({
    ...
    startIdx: 0,
    stopIdx: perPage,
  })
}

Nice catch, will check this asap...

This has been fixed in the code. Thank you!

@carma12

carma12 commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator Author

Another thing I noticed, the search is not responsive- it only searches the value after clicking enter. The other pages implemented do have the reponsive approach, so the list in the table changes with each key put into the search. Is this something we want to address?

You are right: there is an inconsistency on how the data is being searched. I will fix this in this PR, but also will open a new PR to do it in other pages affected.

I fixed the issue unrelated to this PR in this PR: #1139

@veronnicka veronnicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The changes here fixed the search- it reacts on the first click of the button, but Im a bit unsure about the results, please see the screenshots attached.

Image Image

@carma12

carma12 commented Jul 14, 2026

Copy link
Copy Markdown
Collaborator Author

The changes here fixed the search- it reacts on the first click of the button, but Im a bit unsure about the results, please see the screenshots attached.
Image Image

The results are provided by the privileges_find API call, that performs an LDAP search in the DS, so there is no much that we can do in terms of the accuracy (or lack of it) that we get. This most likely will also happen in the old WebUI...

@carma12
carma12 force-pushed the privileges-main-page branch from d982487 to 007838a Compare July 14, 2026 08:47
@duzda

duzda commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

It is a bit counterintuitive to how elsewhere search works, but let's keep it this way.

@veronnicka

Copy link
Copy Markdown
Contributor

The changes here fixed the search- it reacts on the first click of the button, but Im a bit unsure about the results, please see the screenshots attached.
Image Image

The results are provided by the privileges_find API call, that performs an LDAP search in the DS, so there is no much that we can do in terms of the accuracy (or lack of it) that we get. This most likely will also happen in the old WebUI...

Okay, I thought it might be something like that but wasnt sure. Thanks for the explanation!

@veronnicka veronnicka left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I have no further comments, providing an ack

The 'Privileges' page should be located
under 'Role-based access control' section
and must show all the available privileges
associated to the IPA server.

Assisted-by: Claude <noreply@anthropic.com>
Signed-off-by: Carla Martinez <carlmart@redhat.com>
@carma12
carma12 force-pushed the privileges-main-page branch from 007838a to 3dee9b5 Compare July 14, 2026 14:01
@carma12
carma12 merged commit 49c55fc into freeipa:main Jul 15, 2026
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

needs-review This PR is waiting on a review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants