8000 add description to groups by edewit · Pull Request #39174 · keycloak/keycloak · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

add description to groups #39174

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
Merged
merged 8 commits into from
May 14, 2025
Merged

add description to groups #39174

merged 8 commits into from
May 14, 2025

Conversation

edewit
Copy link
Contributor
@edewit edewit commented Apr 24, 2025

fixes: #39172
Signed-off-by: Erik Jan de Wit erikjan.dewit@gmail.com

@edewit edewit requested review from a team as code owners April 24, 2025 08:05
@edewit edewit self-assigned this Apr 24, 2025
@edewit edewit requested review from a team as code owners April 24, 2025 08:05
@edewit
add description to groups
2f70b84 
fixes: keycloak#39172
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
ahus1
ahus1 reviewed Apr 25, 2025
View reviewed changes
model/jpa/src/main/resources/META-INF/jpa-changelog-master.xml Outdated
@@ -86,5 +86,6 @@
<include file="META-INF/jpa-changelog-26.0.0.xml"/>
<include file="META-INF/jpa-changelog-26.1.0.xml"/>
<include file="META-INF/jpa-changelog-26.2.0.xml"/>
<include file="META-INF/jpa-changelog-26.2.1.xml"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

With this being an enhancement, and while we don't backport enhancemnets, this should then be 26.3 and not a patch release. We also avoid updating databases in patch releases, as it could lead to downtimes when applied to large tables. While this might be ok for minor releases, this is unexpected for patch releases.

Suggested change
<include file="META-INF/jpa-changelog-26.2.1.xml"/>
<include file="META-INF/jpa-changelog-26.2.1.xml"/>

edewit added 2 commits April 26, 2025 09:40
@edewit
renamed
eb2543f 
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
@edewit
fixed test
804215b 
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
@edewit edewit requested a review from ahus1 May 5, 2025 06:41
@ahus1
Review
a7002ac 
Signed-off-by: Alexander Schwartz <aschwart@redhat.com>
ahus1
ahus1 previously requested changes May 13, 2025
View reviewed changes
Copy link
Contributor
@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Thank you for the change. I pushed one small commit regarding the DB.

I tested the UI manually, and found the following glitches that probably need fixing in this PR:

  • After creating the group and choosing the "Rename" action, the existing name of the group and the description are both empty. I expect them to be pre-filled with the name and description of the current group.
  • After changing the description in the popup and submitting it, the new name is saved to the database, but not the description. The description also doesn't change in the UI.

Please have a look at those two glitches. Thanks!

edewit added 3 commits May 13, 2025 14:43
@edewit
update description when renaming groups
91db627 
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
@edewit
changed rename group to edit
2df7bc9 
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
@edewit
fixed refresh
66488a5 
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
ahus1
ahus1 reviewed May 13, 2025
View reviewed changes
Copy link
Contributor
@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

The DB changes now look good to me, and I also manually tested the UI.

One small nit-pick: Please update the test case to also update the description, see

keycloak/js/libs/keycloak-admin-client/test/groups.spec.ts

Lines 69 to 74 in 97727db

it("update single groups", async () => {
const groupId = currentGroup.id;
await kcAdminClient.groups.update(
{ id: groupId! },
{ name: "another-group-name" },
);

@ahus1 ahus1 dismissed their stale review May 13, 2025 13:30

has been addressesed

@edewit
fixed test
a6b3617 
Signed-off-by: Erik Jan de Wit <erikjan.dewit@gmail.com>
@ahus1 ahus1 self-assigned this May 13, 2025
ahus1
ahus1 approved these changes May 13, 2025
View reviewed changes
Copy link
Contributor
@ahus1 ahus1 left a comment

Choose a reason for hiding this comment

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

Thank you for the change! Approving assuming the tests will end up green.

@keycloak-github-bot keycloak-github-bot bot mentioned this pull request May 13, 2025
Closed
@keycloak-github-bot
Copy link

Unreported flaky test detected

If the 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.forms.ResetPasswordTest#resetPasswordWrongSmtp

Keycloak CI - Base IT (5)

org.openqa.selenium.TimeoutException: 
java.net.SocketTimeoutException: Read timed out
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.11.0-1012-azure', java.version: '21.0.7'
Driver info: driver.version: HtmlUnitDriver
...

org.openqa.selenium.TimeoutException: 
java.net.SocketTimeoutException: Read timed out
Build info: version: '4.28.1', revision: '73f5ad48a2'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '6.11.0-1012-azure', java.version: '21.0.7'
Driver info: driver.version: HtmlUnitDriver
...

Report flaky test

keycloak-github-bot[bot]
keycloak-github-bot bot reviewed May 13, 2025
View reviewed changes
Copy link
@keycloak-github-bot keycloak-github-bot bot 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

@ahus1 ahus1 enabled auto-merge (squash) May 13, 2025 14:57
@ahus1 ahus1 merged commit cbd0d18 into keycloak:main May 14, 2025
76 checks passed
@Barathwaja Barathwaja mentioned this pull request May 26, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@keycloak-github-bot keycloak-github-bot[bot] keycloak-github-bot[bot] left review comments

@ssilvert ssilvert ssilvert approved these changes

@ahus1 ahus1 ahus1 approved these changes

Assignees

@edewit edewit

@ahus1 ahus1

Labels
flaky-test team/cloud-native team/core-clients team/core-iam team/ui
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add description to groups
3 participants
@edewit @ssilvert @ahus1
0