Skip to content

Conversation

@edewit
Copy link
Contributor

@edewit edewit commented Jul 7, 2023

Fetching of the subgroups is now paginates in the tree and the detail view

subgroup pagination in the treeview
image

tree is now lazy populated on select

fixes: #19954

@edewit edewit requested a review from a team as a code owner July 7, 2023 18:26
@ghost ghost added the team/ui label Jul 7, 2023
Copy link
Contributor

@ssilvert ssilvert 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. I'm going to try it out later.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
Copy link

ghost commented Jul 7, 2023

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest#testPersistenceMultipleNodesClientSessionsAtRandomNode

Keycloak CI - Store Model Tests

java.lang.AssertionError: Execution didn't complete: java.lang.RuntimeException: org.infinispan.util.concurrent.TimeoutException: ISPN000658: Initial state transfer timed out for cache offlineSessions on node-9
	at org.junit.Assert.fail(Assert.java:89)
	at org.keycloak.testsuite.model.KeycloakModelTest.inIndependentFactories(KeycloakModelTest.java:436)
	at org.keycloak.testsuite.model.session.OfflineSessionPersistenceTest.testPersistenceMultipleNodesClientSessionsAtRandomNode(OfflineSessionPersistenceTest.java:249)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
...

Report flaky test

@edewit edewit requested a review from ssilvert July 10, 2023 06:10
@edewit edewit requested a review from a team as a code owner July 10, 2023 13:59
Copy link
Contributor

@ssilvert ssilvert left a comment

Choose a reason for hiding this comment

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

This doesn't fix the problem for me. Did you try to partial import the json attached to the issue? Import works fine (you just have to let it finish). But then I try to go to the Groups section and it hangs.

On a more minor note, the show/hide works well, but I think the "show" icon should be a right arrow instead of a hamburger menu.

@edewit edewit closed this Jul 11, 2023
@edewit edewit reopened this Jul 11, 2023
@edewit edewit requested a review from ssilvert July 13, 2023 08:19
@edewit edewit changed the title added lazy parameter lazy populate the treeview for groups Jul 13, 2023
Copy link
Contributor

@ssilvert ssilvert left a comment

Choose a reason for hiding this comment

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

I've tested the latest revision on this PR and it works great. Still needs code review.

@jonkoops
Copy link
Contributor

@ssilvert I think the tests are still failing on this one, but I will do quick rebase to make sure.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
Copy link

ghost commented Jul 26, 2023

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.webauthn.account.WebAuthnTransportLocaleTest#multipleTransports

Keycloak CI - WebAuthn IT (chrome)

java.lang.AssertionError: Expected OIDCLogin but was  (https://localhost:8543/auth/realms/test/protocol/openid-connect/auth?client_id=account-console&redirect_uri=https%3A%2F%2Flocalhost%3A8543%2Fauth%2Frealms%2Ftest%2Faccount%2F%23%2Fsecurity%2Fsigningin&state=f2eaf2f8-49d9-4767-8e10-50d381480f3b&response_mode=fragment&response_type=code&scope=openid&nonce=bd40d161-df0d-454e-8d62-5dbed66e831e&code_challenge=fd6gg3ma-YcwS3i0OKDDTQCUz5h33PzuNkLcWQW5HOE&code_challenge_method=S256)
	at org.junit.Assert.fail(Assert.java:89)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.keycloak.testsuite.page.AbstractPage.assertCurrent(AbstractPage.java:110)
	at jdk.internal.reflect.GeneratedMethodAccessor613.invoke(Unknown Source)
...

Report flaky test

@jonkoops
Copy link
Contributor

Looks like it has to do with the changes introduced by Erik, we'll have to wait until he's back next week.

@alice-wondered
Copy link
Contributor

We have been working on the same idea with a slightly different direction by expanding out the storage implementations and then updating the surrounding utility and resource classes to account for pagination. It isn't in a final state yet but since comments mention issue not being solved curious if it would be welcome here: main...Redhat-Alice:keycloak:subgroup-model-expansion

@edewit edewit enabled auto-merge (squash) August 4, 2023 08:08
@cypress
Copy link

cypress bot commented Aug 4, 2023

1 flaky tests on run #8403 ↗︎

0 527 48 0 Flakiness 1

Details:

Merge 3d4795c into 5e21ff5...
Project: Keycloak Admin UI Commit: 6351abdc59 ℹ️
Status: Passed Duration: 17:36 💡
Started: Aug 4, 2023 6:55 PM Ended: Aug 4, 2023 7:12 PM
Flakiness  cypress/e2e/authentication_test.spec.ts • 1 flaky test • chrome

View Output Video

Test Artifacts
Authentication test > should add a condition Output Screenshots

This comment has been generated by cypress-bot as a result of this project's GitHub integration settings.

@zwitter1
Copy link
Contributor

zwitter1 commented Aug 4, 2023

Just a heads up. I had a few issues in testing this branch. The pagination on the left side wasn't working when I'd switch the count to 10 per page.

I also don't get the caret on the dropdowns for groups with subgroups until I click on the group.

@ssilvert
Copy link
Contributor

ssilvert commented Aug 4, 2023

We have been working on the same idea with a slightly different direction by expanding out the storage implementations and then updating the surrounding utility and resource classes to account for pagination. It isn't in a final state yet but since comments mention issue not being solved curious if it would be welcome here: main...Redhat-Alice:keycloak:subgroup-model-expansion

I believe this is solved now and we intend to merge the PR soon. If you want to suggest an improvement, go ahead and create an enhancement request or discussion.

@edewit edewit merged commit 3396198 into keycloak:main Aug 4, 2023
@edewit edewit deleted the issue-19954 branch August 5, 2023 07:12
jonkoops pushed a commit to jonkoops/keycloak that referenced this pull request Aug 23, 2023
* added lazy parameter

fixes: keycloak#19954

* changed to only have the parameter

* fixed merge errors

* removed the `lazy` and now add subgroups on select

* lint

* fixed prettier

* fixed nullpointer

* fixed member tab
jonkoops added a commit that referenced this pull request Aug 23, 2023
This was referenced Sep 4, 2023
@stianst stianst mentioned this pull request Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Admin UI hangs with many subgroups

5 participants