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

FEAT update SiteTree permissions in CMS #2862

Conversation

andrewandante
Copy link
Contributor

@andrewandante andrewandante commented Jun 15, 2023

Linked to silverstripe/silverstripe-framework#10819

Allows SiteTree objects to have permissions assigned to a individual users, rather than having to assign it directly to groups.

Issue

@andrewandante
Copy link
Contributor Author

andrewandante commented Jun 22, 2023

This is now all working alongside the Framework PR; however, the ViewerMembers and EditorMembers fields always show in the CMS, instead of dynamically show/hide depending on the ViewType/EditType. Can't seem to pinpoint what is doing that, any tips?

EDIT: Oh no, it's entwine? https://github.com/silverstripe/silverstripe-cms/pull/1747/files#diff-f8b125682490aaedb06618d1784af3223e62462cade2ad266673d6688399ca56R205-R237

EDIT EDIT: Fixed over here: https://github.com/silverstripe/silverstripe-admin/pull/1531/files

@andrewandante andrewandante force-pushed the FEAT_add_cms_interface_for_user_inherited_permission branch from 8e05cd1 to 632aff9 Compare June 22, 2023 10:43
@andrewandante andrewandante marked this pull request as ready for review June 26, 2023 04:51
@GuySartorelli
Copy link
Member

GuySartorelli commented Jul 5, 2023

There's a couple of PHP linting issues in the CI run which need to be ironed out, please

@andrewandante andrewandante force-pushed the FEAT_add_cms_interface_for_user_inherited_permission branch from 632aff9 to 3735f3a Compare July 5, 2023 04:24
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

Just one small change here, otherwise it all looks okay to me

code/Model/SiteTree.php Outdated Show resolved Hide resolved
code/Model/SiteTree.php Show resolved Hide resolved
Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

For some reason when I try this locally, the Viewer Users and Editor Users fields don't save. It's probably more to do with the changes you made to ListboxField than anything here, but given that's merged already I'll put the note here instead.
Can you please investigate and assuming you can reproduce this, find out where that bug's coming from? The value is being passed in as part of the form submission request, I can see that from my browser.

It does work fine in react contexts - just the entwine context that's borked.

@andrewandante
Copy link
Contributor Author

Can you please investigate and assuming you can reproduce this, find out where that bug's coming from?

Yep can reproduce, I saw it last night and have spent a decent chunk of today implementing XDebug in a new dev environment 😅 so I'll see what I can find - I think it's likely do with the validation step. Thanks for all your reviewing thus far!

@andrewandante andrewandante force-pushed the FEAT_add_cms_interface_for_user_inherited_permission branch from 3735f3a to 8b723d6 Compare July 6, 2023 04:59
@andrewandante andrewandante force-pushed the FEAT_add_cms_interface_for_user_inherited_permission branch from 8b723d6 to 14eb767 Compare July 6, 2023 05:59
@andrewandante
Copy link
Contributor Author

This Framework commit I think fixes the issue - locally I now have both the entwine and react versions working 🤞

Copy link
Member

@GuySartorelli GuySartorelli left a comment

Choose a reason for hiding this comment

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

LGTM

@GuySartorelli GuySartorelli merged commit e91b7c7 into silverstripe:5 Jul 6, 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.

2 participants