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

NEW add OnlyTheseMembers Inherited Permission type #10819

Conversation

andrewandante
Copy link
Contributor

@andrewandante andrewandante commented Jun 14, 2023

To Do:

  • Update batch method SQL queries
  • Add tests
  • Make companion PR for silverstripe-assets
  • Make companion PR for silverstripe-asset-admin
  • Make companion PR for silverstripe-cms
  • Add proper deprecation

Issue

@andrewandante andrewandante force-pushed the FEAT_add_only_individual_users_inherited_permission branch from e9ebc2a to c5e1774 Compare June 15, 2023 10:36
@andrewandante andrewandante force-pushed the FEAT_add_only_individual_users_inherited_permission branch from c5e1774 to 9774cd7 Compare June 19, 2023 05:19
@andrewandante andrewandante marked this pull request as ready for review June 19, 2023 05:19
@andrewandante andrewandante changed the title [WIP] add OnlyTheseMembers Inherited Permission type NEW add OnlyTheseMembers Inherited Permission type Jun 19, 2023
@andrewandante
Copy link
Contributor Author

Test failures seem unrelated to this PR:

ERROR [Emergency]: Uncaught Symfony\Component\Translation\Exception\InvalidResourceException: Error parsing YAML, invalid file "/home/runner/work/silverstripe-framework/silverstripe-framework/vendor/silverstripe/versioned-admin/lang/en.yml". Message: Duplicate key "SilverStripe\Admin\Navigator\SilverStripeNavigatorItem" detected at line 36 (near "  WONT_BE_SHOWN: 'Note: this message will not be shown to your visitors'").

@GuySartorelli
Copy link
Member

I've rerun the tests - all green

@andrewandante
Copy link
Contributor Author

Thanks @GuySartorelli !

Should this be merged first so that the test suites in the other repos have something to latch on to? Or would you prefer to do it all in a group?

Also, am I pointing at the right branches etc? I'm never quite confident with that stuff 😅

@GuySartorelli
Copy link
Member

All in a group, please. This one will ultimately be merged first, but I'd like to make sure all of the PRs play nicely together first - and since they're all related it'll be easier to review them as a whole so the whole picture is in my mind when I do it.
Yup, those are the correct branches.

The only think that looks a little off at a quick glance is the todo (just remove that, and create a new CMS 6 issue for any changes that aren't behind a @deprecated phpdoc) and the deprecation message - the language for when there's a replacement is "Use [FQCN::method()] instead" (except where the new method is in the same class - then just omit the FQCN:: bit) for both the phpdoc and the deprecation notice.

See https://docs.silverstripe.org/en/5/contributing/release_process/#deprecating-api and ignore the first part about {@link <class>} which needs to be reviewed (I know the @link style is nicer for IDEs but it's harder for us to work with when automating changelogs)

@andrewandante andrewandante force-pushed the FEAT_add_only_individual_users_inherited_permission branch from 9774cd7 to 4edc2fc Compare June 22, 2023 08:49
@andrewandante
Copy link
Contributor Author

Righto - I've tested all the PRs and they are working as expected for me. I've pushed all the combined branches to a demo repo here: https://github.com/andrewandante/silverstripe-only-this-user-demo/blob/main/composer.json#L11-L23 if you feel like playing around.

Last remaining thing is that when applied to assets, the list of members renders as a CheckboxSetField, because there's no React form for the ListboxField. I don't know whether you consider this a blocker or not but I don't know that I can build that form with any confidence.

@GuySartorelli
Copy link
Member

Thank you for this work, that's a great contribution 😁
I'm not sure when I'll get to reviewing this but know that it's on my radar. The ListboxField thing is a blocker, but I'll see if I can get a react component working for it, and if not I'll be able to request some specific changes on the PR to get it across the line.

@andrewandante
Copy link
Contributor Author

Thanks! If I get a chance I'll take another swing at it - I got reasonably close by copying the TreeDropdownMultiSelect stuff but then it needed, you know, a tree, and stopped working. The ReactStrap Select Multiple looks a bit gross, but could work in a pinch too - though it's not searchable.

@GuySartorelli
Copy link
Member

GuySartorelli commented Jun 26, 2023

It might help to look at TagField and AnchorSelectField which both use react-select (the same base library that TreeDropdownField uses)

@GuySartorelli
Copy link
Member

Sorry to be a pain but can you please do the ListboxField enhancement as its own PR? It's a blocker for this work but it's really its own concern that should be reviewed and merged on its own merits (and as a bonus that might even mean someone else will get to reviewing it before I get around to looking at the rest of these)

@andrewandante
Copy link
Contributor Author

please do the ListboxField enhancement as its own PR

Yeah no worries, I'll split it out 👍

@andrewandante andrewandante force-pushed the FEAT_add_only_individual_users_inherited_permission branch from 1af345f to 4edc2fc Compare June 28, 2023 03:58
@andrewandante andrewandante force-pushed the FEAT_add_only_individual_users_inherited_permission branch from 4edc2fc to 4b22ab4 Compare July 5, 2023 04:22
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.

This all looks okay to me. Will merge once the other PRs have finished being reviewed and are approved.
After merging, I'll rerun CI on those other PRs before merging them as well.

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.

Reapproving after the latest changes

@GuySartorelli GuySartorelli merged commit 62bd560 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