-
-
reviewboard/webapi/resources.py (Diff revision 1) Two blanks lines should be here, please add it back.
-
reviewboard/webapi/resources.py (Diff revision 1) 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.
-
reviewboard/webapi/resources.py (Diff revision 1) You are missing the invite_only field. It should be something you can specify during creation.
-
-
-
reviewboard/webapi/resources.py (Diff revision 1) 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.
-
-
reviewboard/webapi/resources.py (Diff revision 1) Lines should be < 80 characters in length. Look at other methods for how to shorten.
-
reviewboard/webapi/resources.py (Diff revision 1) The code pretty much says this, can probably remove the comment.
-
-
reviewboard/webapi/resources.py (Diff revision 1) You should probably return an INVALID_FORM_DATA response. Take a look at other create methods that return this (e.g. ReviewReplyDiffCommentResource.create() )
-
-
reviewboard/webapi/resources.py (Diff revision 1) 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.
-
reviewboard/webapi/resources.py (Diff revision 1) Once the update() method is created, this case should probably just call in to that.
-
-
-
-
reviewboard/webapi/resources.py (Diff revision 1) As in the previous comment, this should be returning an identifier for the group as well.
Add group create/update support to webapi.
Review Request #3239 — Created July 20, 2012 and discarded
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. |
|
|
This isn't quite right. This function is meant to return whether or not we are allowed to modify a specific … |
|
|
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 … |
|
|
I don't feel comfortable with this. The way our API works is that each resource represents one object, and objects … |
|
|
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 … |
|
|
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 … |
|
|
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 … |
|
|
Should invert the if statement and do this at the above. See other classes for the proper way to organize … |
|
|
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 … |
|
|
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 … |
|
|
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. |
|
|
Can you get rid of the second sentence here? It doesn't add much and is likely to get out of … |
|
|
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 |
|
Description: |
|
---|
Change Summary:
Chris, can you recommend anyone that has work on the webapi take a peek at my diff?
People: |
|
---|
-
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.
-
reviewboard/webapi/resources.py (Diff revision 2) This import should precede the other django.utils imports.
-
reviewboard/webapi/resources.py (Diff revision 2) 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).
-
reviewboard/webapi/resources.py (Diff revision 2) In these cases, just use double quotes for the string, instead of escaping ' Same with the ones below.
-
reviewboard/webapi/resources.py (Diff revision 2) Indentation for all this is wrong. The ending } for required, and optional and its children, should be aligning with the respective lines from required.
-
reviewboard/webapi/resources.py (Diff revision 2) 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.
-
reviewboard/webapi/resources.py (Diff revision 2) visible, invite_only, and user_list are missing default values.
-
reviewboard/webapi/resources.py (Diff revision 2) 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.
-
reviewboard/webapi/resources.py (Diff revision 2) These aren't necessary if you set the defaults properly. @webapi_request_fields will ensure we get booleans.
-
-
reviewboard/webapi/resources.py (Diff revision 2) We don't need this. We already saved during the creation above.
-
reviewboard/webapi/resources.py (Diff revision 2) 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.
-
reviewboard/webapi/resources.py (Diff revision 2) 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.
-
reviewboard/webapi/resources.py (Diff revision 2) 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.
-
-
-
-
reviewboard/webapi/resources.py (Diff revision 2) Invert this as well. Needs to include the group as a parameter.
-
reviewboard/webapi/resources.py (Diff revision 2) 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.
-
reviewboard/webapi/resources.py (Diff revision 2) This should be the same error we'd use above for a conflicting group.
-
reviewboard/webapi/resources.py (Diff revision 2) 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.
-
reviewboard/webapi/resources.py (Diff revision 2) All these should compare specifically against None, using 'if varname is not None'. Otherwise, blank strings will cause problems.
-
reviewboard/webapi/resources.py (Diff revision 2) These will fail for similar reasons. Just assign directly. We don't want to only assign if setting to True.
-
reviewboard/webapi/resources.py (Diff revision 2) This can be removed, since we won't be handling user registration here.
-
-
-
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?
-
-
reviewboard/reviews/managers.py (Diff revision 3) 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.
-
-
-
-
-
reviewboard/webapi/resources.py (Diff revision 3) Align the second line at the same column as "INVALID_FORM_DATA"
-
-
-
-