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

[4.x] Allowing adding a blueprint to user groups #6506

Merged
merged 40 commits into from Jun 22, 2023

Conversation

ryanmitchell
Copy link
Contributor

@ryanmitchell ryanmitchell commented Aug 16, 2022

Ok be gentle as I've not worked with Vue a whole lot.

This PR adds the ability to modify the blueprint associated with a user group by adding some new routes, controllers and borrowing from the existing user blueprint approach for this. This would allow developers to add new fields to the groups, such as images, descriptions or whatever they want.

The only 'breaking' change I can see is adding data() to user groups and how that would affect eloquent - would love to hear your thoughts on this.

I will definitely have missed some things here, so marking as a draft for now. But I would value feedback at this stage.

Closes: statamic/ideas#798

@jasonvarga
Copy link
Member

Looks like a good addition to me. What more feedback were you looking for at the moment?

@ryanmitchell
Copy link
Contributor Author

That’s probably enough - just wanted to know if it was worth spending time on or if there was a reason it wasn’t there already. If there’s anything approach wise that looks wrong to you do let me know.

resources/js/components/user-groups/PublishForm.vue Outdated Show resolved Hide resolved
resources/views/usergroups/edit.blade.php Outdated Show resolved Hide resolved
@jasonvarga
Copy link
Member

I think you're looking good! We haven't added the feature because no one wanted it yet. 🤷

@ryanmitchell
Copy link
Contributor Author

I think this is good to go, but the tests are failing due to a segmentation fault (something on Github's side?)

@ryanmitchell ryanmitchell marked this pull request as ready for review August 16, 2022 14:47
Copy link
Member

@jasonvarga jasonvarga left a comment

Choose a reason for hiding this comment

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

Nice. Some things I noticed:

  • When you save the user group in the CP, the page title disappears.
  • When you save the user group in the CP, no custom field data is added to the file.
  • The blueprint should be added to the blueprint index. (/cp/fields/blueprints)
  • UserGroup's augmentedArrayData will probably need to be updated in order for the custom fields to be used in templates, Add user_groups tag #6505, etc.

@ryanmitchell
Copy link
Contributor Author

Thats really helpful thank you. Ive got them all fixed now.

@jasonvarga
Copy link
Member

I'm not sure why the tests are failing like that. I pulled the suite down locally, used the same PHP version and composer deps, and it all passes. 🤔

@ryanmitchell
Copy link
Contributor Author

Same! No idea what’s going on.

@ryanmitchell
Copy link
Contributor Author

I've got all the tests passing by specifying a min version of phpunit (8.5.23) on the PHP8 mockery action. Hope this is an acceptable solution.

@jasonvarga
Copy link
Member

Yeah I'm fine with that.

Looks like Orchestra requires 8.5.21 at a minimum, and 8.5.23 fixes a memory leak. Segfault seems like that's probably what it is. sebastianbergmann/phpunit#4799

@ryanmitchell ryanmitchell changed the title Add ability to add a blueprint to user groups Allowing adding a blueprint to user groups Sep 9, 2022
@jasonvarga jasonvarga changed the base branch from 3.3 to 4.x June 16, 2023 20:16
@jasonvarga jasonvarga changed the title Allowing adding a blueprint to user groups [4.x] Allowing adding a blueprint to user groups Jun 16, 2023
# Conflicts:
#	.github/workflows/tests.yml
#	resources/js/components/user-groups/PublishForm.vue
#	resources/views/usergroups/edit.blade.php
#	routes/cp.php
@statamic statamic deleted a comment from what-the-diff bot Jun 21, 2023
@jasonvarga
Copy link
Member

This is in a good spot. I think there's just one more thing to sort out.

The handle. Before this PR, you'd get red warning text if you try to change the handle, telling you that references won't be updated.

Now that handle becomes just a basic blueprint field, that feature is gone. I'm fine with putting that text just into the instructions, but then it's also possible to not include the handle field at all. In that case, the handle gets updated based on the title, which is awkward.

I'm thinking this:

  • Leave off the handle field from the user group blueprint
  • Explicitly add it in the "create" route
  • Remove the handle-from-title fallback behavior - if you don't submit the handle, it just doesn't get updated.
  • If you want to add a handle field, it becomes the user's responsibility to put instructions saying that it could mess things up.

(Just putting these as notes to ourselves to sort out. You don't need to do anything, @ryanmitchell)

@ryanmitchell
Copy link
Contributor Author

Thanks for the work on this. Its genuinely appreciated.

@jasonvarga
Copy link
Member

jasonvarga commented Jun 22, 2023

We can't inject the handle field directly after the title field (we can add it to the top or bottom of the form, but that's awkward), and adding a feature to do so would require a fair bit of work. So we're just going to remove the handle field. At least for now.

So, by default, the handle will get snake_cased from the title on creation.
If you want control over the handle, you can add it to your blueprint.

@jasonvarga jasonvarga merged commit ad11f29 into statamic:4.x Jun 22, 2023
15 checks passed
@ryanmitchell ryanmitchell deleted the feature/add-data-to-user-groups branch June 22, 2023 17:55
@ryanmitchell
Copy link
Contributor Author

Amazing. So good to see this merge, thank you!

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.

Add image field for User groups section
2 participants