Mostly style stuff. Hopefully I've answered your questions enough to keep you going. I'm not really sure about the list stuff and the JSON, so hopefully someone else can weigh in.

Two blanks lines should be here, please add it back.

There really should be a PUT method as well. Resources should be updated through update(), with PUT requests, the update if exists in create is just a convenience.

You are missing the invite_only field. It should be something you can specify during creation.

Period at end of description.

Period at end of description.

I'm not sure if using a json list is the proper way to do this. Regardless, this should not be a required parameter, creating empty groups should be supported.

Period at end of description.

Lines should be < 80 characters in length. Look at other methods for how to shorten.

The code pretty much says this, can probably remove the comment.

Again, code speaks for itself here.

You should probably return an INVALID_FORM_DATA response. Take a look at other create methods that return this (e.g. ReviewReplyDiffCommentResource.create() )

See previous comment on INVALID_FORM_DATA.

You should be returning something to the user identifying the new group that was created. Take a look at the other create resources for examples.

Once the update() method is created, this case should probably just call in to that.

"exist"

Comments should end with a period.

See previous comments.

As in the previous comment, this should be returning an identifier for the group as well.

Bump.  Anyone able to review this besides Steve M.?

Sorry for dropping the ball on this change.

I think this is a great feature to have. There are issues, though, with how this is implemented. I have some notes on what needs to be done.

This import should precede the other django.utils imports.

This isn't quite right. This function is meant to return whether or not we are allowed to modify a specific group. However, it's not taking a group, and it's being used to determine if we're allowed to create at all.

Furthermore, the query is just returning the list of accessible groups. If there are any at all on the server, then this will claim the user can create new groups. If there are none at all, then no user can create groups. Not quite what we want.

Instead, this should take a group, and return the result of group.is_mutable_by(request.user).

In these cases, just use double quotes for the string, instead of escaping '

Same with the ones below.

Indentation for all this is wrong. The ending } for required, and optional and its children, should be aligning with the respective lines from required.

I don't feel comfortable with this.

The way our API works is that each resource represents one object, and objects are associated by links. Embedding children like this doesn't quite match our design.

Adding groups should be done by POSTing something to the users/ child of this resource. Now, right now, this functionality doesn't exist. The create and delete handlers would need to be added for ReviewGroupUserResource. The create function would need to just take a username as a parameter, and it would add that to the group.

visible, invite_only, and user_list are missing default values.

We can't use self.has_modify_permissions for this. What you need to do is create a 'can_create' function in ReviewGroupManager (reviews/managers.py) that works similarly to can_create in RepositoryManager (scmtools/managers.py), and call that instead.

These aren't necessary if you set the defaults properly. @webapi_request_fields will ensure we get booleans.

Blank line between these.

We don't need this. We already saved during the creation above.

This should be:

return 201, {
    self.item_result_key: group,
}

We should never populate the payload directly. We always want to reference an object so the central serializers will ensure the correct information is saved.

We shouldn't hard-core error types here. We need a GROUP_ALREADY_EXISTS error defined in errors.py.

Make sure to then add the error to the list in the decorator for this function.

Should invert the if statement and do this at the above. See other classes for the proper way to organize this. No else clause needed.

Same comment as above with double-quoted strings.

Same comment as above.

Needs defaults.

Invert this as well.

Needs to include the group as a parameter.

This query will produce incorrect results, as it doesn't take into account the local_site_name.

Ideally, you should use get_object to do this check, passing in the name and local_site_name.

This should be the same error we'd use above for a conflicting group.

Style thing: No spaces before the ':', and every entry in a dictionary should have a trailing comma at the end. Same with all other dicts you introduce in this change.

All these should compare specifically against None, using 'if varname is not None'.

Otherwise, blank strings will cause problems.

These will fail for similar reasons. Just assign directly. We don't want to only assign if setting to True.

This can be removed, since we won't be handling user registration here.

Same comment as above with the response.

I changed these to:

for field in ["display_name", "mailing_list", "visible", "invite_only"]:
    val = locals()[field]
    if val is not None:
        setattr(group, field, val)

is that OK?  Or would you rather I explicitly have an if statement for each of the variables?

Can you get rid of the second sentence here? It doesn't add much and is likely to get out of date if the code changes.

This list should be alphabetical

Unindent this line by 4 spaces

There's an extra comma in here

There's an extra comma in here

Align the second line at the same column as "INVALID_FORM_DATA"

There's an extra comma in here

Align 2nd and 3rd lines with the DOES_NOT_EXIST

Extra comma

This line looks like it's > 80 columns

I'll be finishing up this change and getting it in. There's style stuff, LocalSite fixes, and API result output, and unit tests that are critical to getting in before we can ship it.