Skip to content
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.

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

Issue 3751 Allow to grant permissions to group #3752

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from

Conversation

okolesn
Copy link
Collaborator

@okolesn okolesn commented Oct 23, 2024

@PostFilter(USER_READ_FILTER)
@AclMaskList
public List<Role> loadRolesWithUsers() {
return new ArrayList<>(roleManager.loadAllRoles(true));
Copy link
Member

Choose a reason for hiding this comment

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

instead of recreating list once again (in dao class we already returning a list) let's just change signature of the corresponding methods

@PostFilter(USER_READ_FILTER)
@AclMaskList
public List<Role> loadRoles() {
return new ArrayList<>(roleManager.loadAllRoles(false));
Copy link
Member

Choose a reason for hiding this comment

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

same as previous comment

@@ -407,7 +403,7 @@ private void checkEntityExistsAndCanBeModified(final Long entityId, final AclCla
private Object loadEntity(final Long entityId, final AclClass entityClass) {
Object entity;
if (entityClass.equals(AclClass.ROLE)) {
Copy link
Member

Choose a reason for hiding this comment

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

since now Role is a SecuredEntity we can support same way to load it as for all other types of SecuredEntity

public ExtendedRole loadRoleWithUsers(Long roleId) {
loadRole(roleId);
public ExtendedRole loadRoleWithUsers(final Long roleId) {
load(roleId);
Copy link
Member

Choose a reason for hiding this comment

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

seems to me as an necessary load call, what you think?

@@ -115,6 +116,7 @@
r.user_default as role_user_default,
r.default_storage_id as role_default_storage_id,
r.default_profile_id as role_default_profile_id,
r.owner as role_owner
Copy link
Member

Choose a reason for hiding this comment

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

there should be a ,

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.

2 participants