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

LPS-184639, LPS-184640, LPS-190570 Add a new table to map each folder id with its Permission Template settings #4997

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

ces-totrinh
Copy link

@ces-totrinh ces-totrinh commented Dec 11, 2023

Hi team,
Please help review this PR and give your feedback.
cc: @locpham97 @ces-quanhuynh @ces-luannguyen
Previous PR: brianchandotcom#142550

Motivation

LPS-184639 Add a new table to map each folder id with its propagation settings
LPS-184640 Update the checkbox value to the database when creating new Folder
LPS-190570 Make it work with publications
These tickets are subtasks of LPS-154515 As a documents manager I want to set permissions from folders to folders at creation time
Each folder will have a permissions propagation setting, and its value will affect how the permissions of newly created subfolders are determined

Solution

  • In a new module permission-template, creating a new entity PermissionTemplate to map each folderId with its permission template settings with these columns - companyId, groupId, classNameId, classPK, and permissionTemplateEnabled.
    classNameId identifies the kind of thing (see ClassNameLocalService) and classPK would be its primkey (e.g. folderId, ...).

  • Publication: Enable change-tracking-enabled to true for PermissionTemplate and create two new classes to integrate Liferay Publication Framework - PermissionTemplateCTDisplayRenderer and PermissionTemplateTableReferenceDefinition
    Regards,

@liferay-continuous-integration
Copy link
Collaborator

CI is automatically triggering the following test suites:

  •     ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: f781bbbd4519d89107ce7a55cbf0bbbfb2ff3703

Sender Branch:

Branch Name: LPS-154515-refactor
Branch GIT ID: 7b657c6f8a45a63bc23105a59d3d88e5bab4937d

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

Copy link
Collaborator

@marcogalluzzi marcogalluzzi left a comment

Choose a reason for hiding this comment

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

Nice work!

BTW, I think that the new module should be added in file module.properties

long companyId, long groupId, String className, long classPK,
boolean permissionTemplateEnabled) {

PermissionTemplate permissionTemplate = fetchPermissionTemplate(
Copy link
Collaborator

@marcogalluzzi marcogalluzzi Dec 12, 2023

Choose a reason for hiding this comment

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

Only one PermissionTemplate can exist with the key (className, classPK)? Or is it possible to have them repeated with a different companyId or groupId?

Copy link
Author

Choose a reason for hiding this comment

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

classPK is typically an auto-incrementing integer guaranteeing unique identifiers for entities of the same class. That's why I think entity duplication based solely on (className, classPK) is generally improbable.
However, if using companyId, groupId, classNameId and classPK to identify the entity, could help avoid a few scenarios where entity duplication might appear to occur.

Copy link
Collaborator

Choose a reason for hiding this comment

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

My concern wasn't about uniqueness of (className, classPK) which I know it should be. My question was about... can the same object (e.g. folder, KB Article, etc.) have different permission propagation settings for different Instances or Sites? Well, now with your change we can allow that 😄

@ces-totrinh
Copy link
Author

Hi @marcogalluzzi, thanks for your comment, I updated the PR, please help take a look. Thanks.

@ces-totrinh
Copy link
Author

ci:test:sf

@liferay-continuous-integration
Copy link
Collaborator

✔️ ci:test:sf - 1 out of 1 jobs passed in 3 minutes

Click here for more details.

Base Branch:

Branch Name: master
Branch GIT ID: 6af76e46fccdeab086dc5da2bd0f08cbddbfa39a

Sender Branch:

Branch Name: LPS-154515-refactor
Branch GIT ID: 725eb714f48092e28bfc8f0a645cbee2f9497877

1 out of 1jobs PASSED
1 Successful Jobs:
For more details click here.

@liferay-continuous-integration
Copy link
Collaborator

@adolfopa
Copy link
Collaborator

This may take some time to review and to move it forward. Putting it on hold.

@adolfopa
Copy link
Collaborator

adolfopa commented Jul 1, 2024

ci:report:27984803

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants