FEATURE - Moderators can create and manage groups (PR #10432)

GitHub

Is this a sensible way to move create out of the AdminConstraint (and in to the StaffConstraint on line 78)?

The API will return a value back in the JSON response if it wants the client side app to do a redirect… Is there a better way to do this?

To answer myself: the API probably should just return an error, and let the client work out where to redirect to.

Our standard JS style is to use camelCase for local variables

This is looking pretty good so far. I don’t think any of my comments are major as this is the correct approach.

This is a bit odd, mainly because the model itself shouldn’t be responsible for the redirect. The component or controller that calls save() should do this in the then() method on the promise.

I think this is the correct way to do it, because it’s no longer restricted to Admins, and the create action handles the guardian check.

Looks like the parent parameter is never used.

It might be cleaner to use a Fabricate(:moderator) rather than updating a user to be a moderator. I’ll leave it up to you to consider this as perhaps there’s another reason for using the existing user instance.

It might be DRYer to add another fixture to group-fixtures.js for a new group that has the can_admin_group set to true, rather than cloning and updating it over and over in a handful of tests.

I may have been thinking “the user has now been promoted to moderator”. But, you’re right: there is a moderator already being Fabricated, so I’ll use that.

Fair enough. I had a heck of a time getting those to work - I should have cut my losses much earlier and just created some new fixtures, as you suggest.

guardian.ensure_can_create!(Group) was being called from admin/groups_controller.rb, which would result in EnsureMagic calling can_create_group and passing in a param. Changing things to call guardian.ensure_can_create_group! allows us to get rid of the parent param.

:+1:

Looks good to me now!