Added CRUD methods for Groups #95
Conversation
| @@ -58,7 +60,7 @@ def get(self, endpoint, **kwargs): | |||
| class Client(object): | |||
|
|
|||
| def __init__(self, server_url=None, session=None, auth=None, | |||
| bucket="default", collection=None, retry=0, retry_after=None): | |||
| bucket="default", group=None, collection=None, retry=0, retry_after=None): | |||
|
The code looks good to me. You've got some PEP-8 errors to solve: https://travis-ci.org/Kinto/kinto-http.py/jobs/159055428#L406 but appart from that it looks great thanks 👍 |
| endpoint = self.get_endpoint('groups', bucket=bucket) | ||
| return self._paginated(endpoint) | ||
|
|
||
| def create_group(self, group=None, bucket=None, |
There was a problem hiding this comment.
What happens if group is left None here?
We could do a POST on the plural endpoint instead of PUT and let the server generate a group name for us.
There was a problem hiding this comment.
How do you think we should make the server generate a group name? Using random?
There was a problem hiding this comment.
If you do a POST /buckets/x/groups the server will randomly generate a group id for you.
➜ ~ echo '{"data":{"members":[]}}' | http POST https://kinto.dev.mozaws.net/v1/buckets/x/groups --auth user:pass
HTTP/1.1 201 Created
Access-Control-Expose-Headers: Retry-After, Content-Length, Alert, Backoff
Connection: keep-alive
Content-Length: 172
Content-Type: application/json; charset=UTF-8
Date: Mon, 12 Sep 2016 08:07:15 GMT
Server: nginx
{
"data": {
"id": "1xEdx9UY",
"last_modified": 1473667635578,
"members": []
},
...
There was a problem hiding this comment.
Doing a POST is giving the error message : "Method not allowed for this endpoint". Am I doing something wrong?
|
This is a fantastic contribution so far! The code is clean, it has some tests... it's very cheerful! Thank you! We are used to comment inside the code lines, and it can sound a bit rough sometimes. Don't hesitate to ask questions or raise red flags if you think that's not relevant ;) There are some details to fix, but we're not far from merging ;) Happy coding! |
|
It looks good to me, thanks. Do you mind adding some documentation in the README and updating the CHANGELOG |
| @@ -7,7 +7,7 @@ This document describes changes between each past release. | |||
| 6.3.0 (unreleased) | |||
| ================== | |||
|
|
|||
| - Nothing changed yet. | |||
| - Added CRUD methods for the endpoint group. (#87) | |||
|
Excellent! I guess we can merge! The issue number in the changelog is a micro nitpick, we don't have strong consistency for this, plus the pull-request will be linked from the issue itself. |
|
Yes I just wanted to add some documentation about the new features in the README as well and after that we are ready to merge/release 👍 |
| @@ -117,11 +117,29 @@ If no specific bucket name is provided, the "default" bucket is used. | |||
| client.update_bucket('payments', permissions={}) | |||
|
|
|||
| # Or delete a bucket and everything under. | |||
| client.delete_bucket('payment') | |||
| client.delete_bucket('payment',if_exists=True) | |||
There was a problem hiding this comment.
nit: space missing between: , if_exists
| @@ -246,7 +324,7 @@ def test_single_record_can_overwrite(self): | |||
| client.create_collection() | |||
| created = client.create_record(data={'foo': 'bar'}, | |||
| permissions={'read': ['alexis']}) | |||
|
|
|||
There was a problem hiding this comment.
Removed all the extra blank lines now.
|
Looks good, thanks! |
|
Great job @mansimarkaur :) Thanks |
Fixes #87
Please let me know if anything's wrong.