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

Add Events tab to relevant objects in the UI #34436

Merged
merged 1 commit into from
Jan 13, 2025

Conversation

antikalk
Copy link
Contributor

Fixes #29113

@antikalk antikalk requested a review from a team as a code owner October 29, 2024 09:16
@ssilvert ssilvert marked this pull request as draft October 29, 2024 12:17
@antikalk
Copy link
Contributor Author

This will add the events tab if enabled and authorized to:

  • ClientScopes (Admin Events)
  • Clients (User- and Admin Events)
  • Groups (Admin Events + Membership and Childgroup assignments events)
  • Identity-Providers (Admin Events)
  • Organizations (Admin Events + Membership events)
  • RealmRole (Admin Events)
  • User (User- and Admin Events)

@agagancarczyk
Copy link
Contributor

agagancarczyk commented Nov 13, 2024

Hi @antikalk, many thanks for this and your previous contribution.

We had to change this PR to a draft as we realized that some improvements were necessary after merging your previous PR: #34561. You can view these changes here: #34735 .

Now that these improvements are in place, we can proceed with this PR. However, I'd appreciate it if you could review the current implementation of the event tabs for Clients and Users as introduced in #34735 and make any relevant changes to this PR.

Let's keep this PR as a draft until it's ready for review. Thanks!

@antikalk antikalk force-pushed the issue-29113 branch 2 times, most recently from 038c304 to a6476a8 Compare November 15, 2024 13:48
@antikalk
Copy link
Contributor Author

@agagancarczyk I have updated the branch to include the changes. During development I noticed two changes:

  • As the activeFilters will not be initialized with the User-/Client param value anymore, the chip indicating the filter is not displayed initially. When I add an additional filter, or simply open and submit the filter menu, the chip will be displayed though. IMO it would make sense to hide the chip for a value that has been a component param all the time.
  • When there is no events being found initially the toolbarItem is hidden. I guess this is fine as no events will be available to filter for anyways.

@agagancarczyk
Copy link
Contributor

agagancarczyk commented Nov 19, 2024

@agagancarczyk I have updated the branch to include the changes. During development I noticed two changes:

  • As the activeFilters will not be initialized with the User-/Client param value anymore, the chip indicating the filter is not displayed initially. When I add an additional filter, or simply open and submit the filter menu, the chip will be displayed though. IMO it would make sense to hide the chip for a value that has been a component param all the time.
  • When there is no events being found initially the toolbarItem is hidden. I guess this is fine as no events will be available to filter for anyways.

Thanks @antikalk 1. Yes, we should hide the chip. 2. Yes, this behavior makes sense

@antikalk antikalk force-pushed the issue-29113 branch 2 times, most recently from 22ac4d7 to 3df3619 Compare November 19, 2024 17:40
@antikalk
Copy link
Contributor Author

@agagancarczyk I provided the change to the filter chips with this PR.

@antikalk antikalk marked this pull request as ready for review November 19, 2024 17:43
@agagancarczyk agagancarczyk removed their assignment Nov 28, 2024
@ssilvert
Copy link
Contributor

@antikalk Please fix conflicts.

@antikalk
Copy link
Contributor Author

@ssilvert done

@ssilvert
Copy link
Contributor

I was trying this out. Looks really nice.

I did notice one flaw in the empty state of all events. It's a general problem, but this PR makes it more relevant. See screen shot of admin events for a realm role:
image

Notice that it says, There are no admin events in this realm., which doesn't make a lot of sense for realm events. But also, there is no way to refresh this screen.

I think the best solution is to just replace the message There are no admin events in this realm. with a refresh button/link.

@ssilvert
Copy link
Contributor

@antikalk Sorry. Please fix conflicts again.

Will you be addressing the empty state issue I raised? If not, we can do it in another issue/PR.

Fixes keycloak#29113

Signed-off-by: Oliver Cremerius <antikalk@users.noreply.github.com>
@antikalk
Copy link
Contributor Author

antikalk commented Jan 11, 2025

@ssilvert I fixed the conflicts. In my opinion it would make sense to address the empty state issue in a different issue/pr.

Any chance of getting this into 26.1? Should we include the feature in the release notes?

@ssilvert ssilvert merged commit 2c3b4ff into keycloak:main Jan 13, 2025
59 checks passed
@ssilvert ssilvert mentioned this pull request Jan 13, 2025
2 tasks
@ssilvert
Copy link
Contributor

Thanks for the contribution @antikalk.

I created the cleanup issue: #36413

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Events tab to relevant objects in the UI
4 participants