ACS-5506 Add description and hasSubgroups to groups API #2035
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
MichalKinas
force-pushed
the
feature/ACS-5506-be-handling-a-description-of-the-group-and-flag-for-subgroups
branch
from
6c09f8e to
e3c18b3
Compare
mpichura
reviewed
Jul 7, 2023
remote-api/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java
Outdated
Show resolved
Hide resolved
remote-api/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java
Outdated
Show resolved
Hide resolved
remote-api/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java
Outdated
Show resolved
Hide resolved
remote-api/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java
Outdated
Show resolved
Hide resolved
remote-api/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java
Outdated
Show resolved
Hide resolved
mpichura
reviewed
Jul 28, 2023
remote-api/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java
Outdated
Show resolved
Hide resolved
MichalKinas
force-pushed
the
feature/ACS-5506-be-handling-a-description-of-the-group-and-flag-for-subgroups
branch
from
353b18d to
e0e2f74
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
remote-api/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java
Dismissed
Show dismissed
Hide dismissed
remote-api/src/main/java/org/alfresco/rest/api/impl/GroupsImpl.java
Dismissed
Show dismissed
Hide dismissed
repository/src/test/java/org/alfresco/repo/security/authority/AuthorityServiceTest.java
Fixed
Show fixed
Hide fixed
repository/src/test/java/org/alfresco/repo/security/authority/AuthorityServiceTest.java
Fixed
Show fixed
Hide fixed
repository/src/test/java/org/alfresco/repo/security/authority/AuthorityServiceTest.java
Fixed
Show fixed
Hide fixed
repository/src/test/java/org/alfresco/repo/security/authority/AuthorityServiceTest.java
Fixed
Show fixed
Hide fixed
repository/src/test/java/org/alfresco/repo/security/authority/AuthorityServiceTest.java
Fixed
Show fixed
Hide fixed
MichalKinas
requested review from
mpichura,
tpage-alfresco,
Pawel-608 and
krdabrowski
tpage-alfresco
requested changes
Jan 19, 2024
There was a problem hiding this comment.
I don't like the asymmetry of this solution. We are relying on the AuthorityDAO to persist the group description, but I think the only way to retrieve it is with the NodeService. I think description is a reasonable field to add to the group object AuthorityInfo (providing it can continue to be unset) or alternatively we could just use the NodeService to set and retrieve it? (within a single transaction) Nb. AuthorityInfo is part of the @AlfrescoPublicApi and so we need to preserve the existing constructor.
The hasSubgroups flag requires extra service calls to populate and so should be an include parameter in the API.
MichalKinas
force-pushed
the
feature/ACS-5506-be-handling-a-description-of-the-group-and-flag-for-subgroups
branch
from
9e43682 to
d3c7342
Compare
repository/src/main/java/org/alfresco/repo/security/authority/AuthorityDAOImpl.java
Fixed
Show fixed
Hide fixed
repository/src/main/java/org/alfresco/repo/security/authority/AuthorityDAOImpl.java
Fixed
Show fixed
Hide fixed
repository/src/main/java/org/alfresco/repo/security/authority/AuthorityServiceImpl.java
Fixed
Show fixed
Hide fixed
repository/src/main/java/org/alfresco/repo/security/authority/AuthorityServiceImpl.java
Fixed
Show fixed
Hide fixed
repository/src/test/java/org/alfresco/repo/security/authority/AuthorityServiceTest.java
Fixed
Show fixed
Hide fixed
repository/src/test/java/org/alfresco/repo/security/authority/AuthorityServiceTest.java
Fixed
Show fixed
Hide fixed
packaging/tests/tas-restapi/src/test/java/org/alfresco/rest/groups/GroupsTests.java
Outdated
Show resolved
Hide resolved
repository/src/main/java/org/alfresco/repo/security/authority/AuthorityDAOImpl.java
Outdated
Show resolved
Hide resolved
repository/src/main/java/org/alfresco/repo/security/authority/AuthorityDAOImpl.java
Show resolved
Hide resolved
repository/src/main/java/org/alfresco/repo/security/authority/AuthorityDAOImpl.java
Outdated
Show resolved
Hide resolved
tpage-alfresco
approved these changes
Feb 1, 2024
There was a problem hiding this comment.
This looks much better now and I don't think it should have any performance implications for existing integrations (since both new fields are behind include flags). You mentioned looking at canned queries, which could improve the performance when loading both the description and display name.
LGTM 👍
remote-api/src/test/java/org/alfresco/rest/api/tests/client/data/Group.java
Fixed
Show fixed
Hide fixed
repository/src/main/java/org/alfresco/repo/security/authority/AuthorityServiceImpl.java
Fixed
Show fixed
Hide fixed
remote-api/src/test/java/org/alfresco/rest/api/tests/client/data/Group.java
Fixed
Show fixed
Hide fixed
repository/src/main/java/org/alfresco/repo/security/authority/AuthorityServiceImpl.java
Fixed
Show fixed
Hide fixed
MichalKinas
deleted the
feature/ACS-5506-be-handling-a-description-of-the-group-and-flag-for-subgroups
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See https://alfresco.atlassian.net/browse/ACS-5506