Skip to content

Add subgroups sorting#22295

Merged
ahus1 merged 2 commits intokeycloak:mainfrom
Todor13:issue/19348-sort-subgroups
Aug 7, 2023
Merged

Add subgroups sorting#22295
ahus1 merged 2 commits intokeycloak:mainfrom
Todor13:issue/19348-sort-subgroups

Conversation

@Todor13
Copy link
Contributor

@Todor13 Todor13 commented Aug 7, 2023

Closes #19348

Apply the same sorting to subgroups as the sorting of the top level groups.

@Todor13 Todor13 requested a review from a team August 7, 2023 12:11
@Todor13 Todor13 requested a review from a team as a code owner August 7, 2023 12:11
@ghost ghost added the team/store label Aug 7, 2023
@ahus1 ahus1 self-assigned this Aug 7, 2023
Copy link
Member

@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 tackling this issue which has been open for quite some time.

I saw that this PR was missing a test, so I created one: Todor13#1

Please have a look at the PR. Once it is merged, I can do a final review of this PR.

@Todor13
Copy link
Contributor Author

Todor13 commented Aug 7, 2023

Thank you for tackling this issue which has been open for quite some time.

I saw that this PR was missing a test, so I created one: Todor13#1

Please have a look at the PR. Once it is merged, I can do a final review of this PR.

@ahus1 Thank you for the review and the changes! I have merged your PR. Should I squash the commits so that we have only one commit?

@ahus1 ahus1 self-requested a review August 7, 2023 19:17
Copy link
Member

@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 reviewing the test case and providing this PR in the first place. All tests are green, and it now ready to be merged.

@ahus1 ahus1 merged commit dffa7a3 into keycloak:main Aug 7, 2023
@ahus1
Copy link
Member

ahus1 commented Aug 7, 2023

Should I squash the commits so that we have only one commit?

Generally speaking, you're on the safe side to merge the commits before the final review, especially when they are related like in this case. It might make it simpler and more straightforward for a review and a maintainer in the process.

A maintainer can also squash the changes via the GitHub API (what I did in this case) if they feel this is the appropriate thing to do. Overall it is for the maintainer to make sure that the commit log is meaningful to read and all changes can be tracked to their issues and related pull requests.

Thank you again for this contribution - the parent issue had already several votes, and I assume even more subscribers. It will be part of Keycloak 23 once it is released. If you want to give it a try, the next nightly release of Keycloak will contain your change.

ahus1 added a commit to ahus1/keycloak that referenced this pull request Oct 13, 2023
* Review comments to add a test, update the API description and adjust the map storage.

Closes keycloak#19348

Co-authored-by: Alexander Schwartz <aschwart@redhat.com>
(cherry picked from commit dffa7a3)
mhajas pushed a commit that referenced this pull request Oct 16, 2023
* Review comments to add a test, update the API description and adjust the map storage.

Closes #19348

Co-authored-by: Alexander Schwartz <aschwart@redhat.com>
(cherry picked from commit dffa7a3)
@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sort subgroups

2 participants