Refactor identity-related code #25921
Conversation
Align the Group-related service and transformers with the other Identity-related classes. The refactoring of the remaining Identity-related classes occurred during the initial Group service and REST API implementation.
Check if an entity has already been assigned within the Group and Role AddEntity processors.
Add test coverage for duplicate entity assignment in the Group and Role add entity processors.
|
@remcowesterhoud here's the fix for Group and Role duplicate entity assignment checks. @houssain-barouni I also piggybacked a refactoring to some Group-related service-layer code. It's meant to sync up the Group code with the changes introduced by this PR. At the time of the refactoring, the Group code wasn't being implemented but not merged. |
koevskinikola
requested review from
houssain-barouni and
remcowesterhoud
and removed request for
remcowesterhoud
There was a problem hiding this comment.
Thanks for tackling this @koevskinikola 🙇♂️ The checks look good to me
|
Wow I just noticed the RDBMS IT is failing here as well. I've been spending so much time trying to figure out why they're failing in my PR ... It's not my fault! 🎉 |
I think some of the failures are related to the refactoring I applied to the Grouo search classes. |
| @@ -25,7 +25,7 @@ public static final class Builder extends AbstractBuilder<Builder> | |||
| implements ObjectBuilder<GroupSort> { | |||
|
|
|||
| public Builder groupKey() { | |||
| currentOrdering = new FieldSorting("key", null); | |||
| currentOrdering = new FieldSorting("groupKey", null); | |||
There was a problem hiding this comment.
❌ With this change the sorting has a different name as the actual property in the GroupEntity and GroupItem. This will be confusing for the end user and they should be the same.
💭 This actually applies to the GroupFilter object as well. Probably we should align all classes/dtos to "groupKey"
This renaming also is the reason for the failing RDBMS tests. We map the property names in the GroupSearchColumn enum to actual DB column names.
There was a problem hiding this comment.
Hey @stoerti, I was considering aligning the GroupEntity classes as well, but I noticed they are used heavily in the RDBMS code, and didn't want to cause merge conflicts for you.
If you prefer to align them further, I can apply the refactoring 👍
There was a problem hiding this comment.
No worries, please go ahead 👍
I rather prefer an consistent API over some temporary merge conflicts 😄
Rename the key property of the GroupEntity and GroupItem classes to groupKey. This makes it clearer to users and aligns it with other Group and Identity-related classes.
github-actions
bot
added
the
component/c8-api
Description
Checklist
Related issues
closes #25905