Add group create/update support to webapi.

Review Request #3239 — Created July 20, 2012 and discarded

Information

Review Board

Reviewers

Third stab at group support in webapi.

1. I can successfully create a group with a list of users (a json encoded string).  I couldn't figure how other webapi methods consume lists of things... so I opted to use json.  It seems like Djblets/Django consume ',' or ' ' separated lists but that seems fragile.  Using json seemed like the most robust solution.

2. Added a `create` and `update` method that create or update a group, respectively.  It is an error to create a group that already exists.  It is an error to update a group that does not exist.

 
Description From Last Updated

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

SM smacleod

There really should be a PUT method as well. Resources should be updated through update(), with PUT requests, the update …

SM smacleod

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

SM smacleod

Period at end of description.

SM smacleod

Period at end of description.

SM smacleod

I'm not sure if using a json list is the proper way to do this. Regardless, this should not be …

SM smacleod

Period at end of description.

SM smacleod

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

SM smacleod

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

SM smacleod

Again, code speaks for itself here.

SM smacleod

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

SM smacleod

See previous comment on INVALID_FORM_DATA.

SM smacleod

You should be returning something to the user identifying the new group that was created. Take a look at the …

SM smacleod

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

SM smacleod

"exist"

SM smacleod

Comments should end with a period.

SM smacleod

See previous comments.

SM smacleod

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

SM smacleod

This import should precede the other django.utils imports.

chipx86chipx86

This isn't quite right. This function is meant to return whether or not we are allowed to modify a specific …

chipx86chipx86

In these cases, just use double quotes for the string, instead of escaping ' Same with the ones below.

chipx86chipx86

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

chipx86chipx86

I don't feel comfortable with this. The way our API works is that each resource represents one object, and objects …

chipx86chipx86

visible, invite_only, and user_list are missing default values.

chipx86chipx86

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 …

chipx86chipx86

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

chipx86chipx86

Blank line between these.

chipx86chipx86

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

chipx86chipx86

This should be: return 201, { self.item_result_key: group, } We should never populate the payload directly. We always want to …

chipx86chipx86

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 …

chipx86chipx86

Should invert the if statement and do this at the above. See other classes for the proper way to organize …

chipx86chipx86

Same comment as above with double-quoted strings.

chipx86chipx86

Same comment as above.

chipx86chipx86

Needs defaults.

chipx86chipx86

Invert this as well. Needs to include the group as a parameter.

chipx86chipx86

This query will produce incorrect results, as it doesn't take into account the local_site_name. Ideally, you should use get_object to …

chipx86chipx86

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

chipx86chipx86

Style thing: No spaces before the ':', and every entry in a dictionary should have a trailing comma at the …

chipx86chipx86

All these should compare specifically against None, using 'if varname is not None'. Otherwise, blank strings will cause problems.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Same comment as above with the response.

chipx86chipx86

Can you get rid of the second sentence here? It doesn't add much and is likely to get out of …

daviddavid

This list should be alphabetical

daviddavid

Unindent this line by 4 spaces

daviddavid

There's an extra comma in here

daviddavid

There's an extra comma in here

daviddavid

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

daviddavid

There's an extra comma in here

daviddavid

Align 2nd and 3rd lines with the DOES_NOT_EXIST

daviddavid

Extra comma

daviddavid

This line looks like it's > 80 columns

daviddavid
SM
  1. 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.
  2. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Two blanks lines should be here, please add it back.
  3. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    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.
    1. I removed the "update if exists" convenience, and replaced it with two separate methods: create and update.  Create creates a group if it does not already exist.  Creating a group that already exists is an error (we wouldn't want to nuke a group we already have).  Update updates a group if it already exists.  Updating a group that doesn't exist is an error.
  4. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    You are missing the invite_only field. It should be something you can specify during creation.
  5. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Period at end of description.
  6. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Period at end of description.
  7. reviewboard/webapi/resources.py (Diff revision 1)
     
     
     
     
     
    Show all issues
    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.
  8. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Period at end of description.
  9. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Lines should be < 80 characters in length. Look at other methods for how to shorten.
  10. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    The code pretty much says this, can probably remove the comment.
  11. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Again, code speaks for itself here.
  12. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    You should probably return an INVALID_FORM_DATA response. Take a look at other create methods that return this (e.g. ReviewReplyDiffCommentResource.create() )
  13. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    See previous comment on INVALID_FORM_DATA.
  14. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    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.
  15. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Once the update() method is created, this case should probably just call in to that.
    1. I moved the code.  My reasoning is that we shouldn't "overwrite" a group if it exists.
  16. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    "exist"
  17. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    Comments should end with a period.
  18. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    See previous comments.
  19. reviewboard/webapi/resources.py (Diff revision 1)
     
     
    Show all issues
    As in the previous comment, this should be returning an identifier for the group as well.
  20. 
      
SH
SH
SH
  1. Bump.  Anyone able to review this besides Steve M.?
  2. 
      
SH
chipx86
  1. 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.
  2. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    This import should precede the other django.utils imports.
    1. Hm, I can't seem to close these issues. When I click "Fixed" it says resolved and offers to "Re-open", but upon page reload the issue is still open.
    2. I spoke too soon. Looks like they're marked as resolved correctly now.
  3. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
    Show all issues
    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).
  4. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    In these cases, just use double quotes for the string, instead of escaping '
    
    Same with the ones below.
  5. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
     
    Show all issues
    Indentation for all this is wrong. The ending } for required, and optional and its children, should be aligning with the respective lines from required.
  6. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
     
     
    Show all issues
    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.
  7. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
    Show all issues
    visible, invite_only, and user_list are missing default values.
  8. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    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.
    1. Hi Chris, I'm working with Stephen on improving this change.  We're not sure what to put in the RepositoryManager; what permissions the user should have to create a group?  Basically, user.has_perm(<what>) ?
    2. Nothing will go in RepositoryManager. This will be in a brand new ReviewGroupManager.
      
      You only want a can_create method there. I was only pointing to RepositoryManager as an example of this. This should return whether that user has the ability to create a group.
      
      That check should basically allow it if the user is an admin, or the admin of a localsite (if one is specified).
    3. Yeah, I meant ReviewGroupManager.  Should the test be something like:
      
        return user.is_superuser or (local_site and local_site.is_mutable_by(user))
      
      ?
    4. Yep, that looks about right.
  9. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
    Show all issues
    These aren't necessary if you set the defaults properly. @webapi_request_fields will ensure we get booleans.
  10. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
    Show all issues
    Blank line between these.
  11. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    We don't need this. We already saved during the creation above.
  12. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    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.
  13. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    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.
  14. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
    Show all issues
    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.
  15. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Same comment as above with double-quoted strings.
  16. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
     
     
    Show all issues
    Same comment as above.
  17. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Needs defaults.
  18. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    Invert this as well.
    
    Needs to include the group as a parameter.
  19. reviewboard/webapi/resources.py (Diff revision 2)
     
     
    Show all issues
    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.
  20. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues
    This should be the same error we'd use above for a conflicting group.
  21. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
    Show all issues
    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.
  22. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
    Show all issues
    All these should compare specifically against None, using 'if varname is not None'.
    
    Otherwise, blank strings will cause problems.
  23. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
     
     
     
    Show all issues
    These will fail for similar reasons. Just assign directly. We don't want to only assign if setting to True.
  24. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    This can be removed, since we won't be handling user registration here.
  25. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
    Show all issues
    Same comment as above with the response.
  26. 
      
PH
  1. 
      
  2. reviewboard/webapi/resources.py (Diff revision 2)
     
     
     
     
     
     
    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?
    
  3. 
      
SH
david
  1. 
      
  2. reviewboard/reviews/managers.py (Diff revision 3)
     
     
     
    Show all issues
    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.
  3. reviewboard/webapi/resources.py (Diff revision 3)
     
     
     
    Show all issues
    This list should be alphabetical
  4. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Unindent this line by 4 spaces
  5. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    There's an extra comma in here
  6. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    There's an extra comma in here
  7. reviewboard/webapi/resources.py (Diff revision 3)
     
     
     
    Show all issues
    Align the second line at the same column as "INVALID_FORM_DATA"
  8. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    There's an extra comma in here
  9. reviewboard/webapi/resources.py (Diff revision 3)
     
     
     
    Show all issues
    Align 2nd and 3rd lines with the DOES_NOT_EXIST
  10. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    Extra comma
  11. reviewboard/webapi/resources.py (Diff revision 3)
     
     
    Show all issues
    This line looks like it's > 80 columns
  12. 
      
chipx86
  1. 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.
    1. Christian, thanks a lot. I'll follow the new RB http://reviews.reviewboard.org/r/3474/ for updates.
      
  2. 
      
SH
Review request changed
Status:
Discarded
Change Summary:
Superseded by /r/3474/