Fix some issues when creating a new object and posting to a subcollection. Tested with groups.

Also removed some dead code.
This commit is contained in:
Michael DeHaan 2013-05-15 18:31:00 -04:00
parent 1bc85f608c
commit ddf10251cc
3 changed files with 31 additions and 23 deletions

View File

@ -55,6 +55,10 @@ class BaseSubList(BaseList):
def post(self, request, *args, **kwargs):
# decide whether to return 201 with data (new object) or 204 (just associated)
created = False
ser = None
postable = getattr(self.__class__, 'postable', False)
if not postable:
return Response(status=status.HTTP_405_METHOD_NOT_ALLOWED)
@ -96,13 +100,6 @@ class BaseSubList(BaseList):
if not ser.is_valid():
return Response(status=status.HTTP_400_BAD_REQUEST, data=ser.errors)
# ask the usual access control settings
#if self.__class__.model in [ User ]:
# if not UserHelper.can_user_add(request.user, ser.init_data):
# raise PermissionDenied()
#else:
# if not self.__class__.model.can_user_add(request.user, ser.init_data):
# raise PermissionDenied()
if not check_user_access(request.user, self.model, 'add', ser.init_data):
raise PermissionDenied()
@ -117,7 +114,6 @@ class BaseSubList(BaseList):
# no attaching to yourself
raise PermissionDenied()
if self.__class__.parent_model != User:
# FIXME: refactor into smaller functions
@ -135,10 +131,8 @@ class BaseSubList(BaseList):
else:
raise exceptions.NotImplementedError()
else:
#if not obj.__class__.can_user_read(request.user, obj):
if not check_user_access(request.user, type(obj), 'read', obj):
raise PermissionDenied()
#if not self.__class__.parent_model.can_user_attach(request.user, main, obj, self.__class__.relationship, data):
# If we just created a new object, we may not yet be able to read it because it's not yet associated with its parent model.
if not check_user_access(request.user, self.parent_model, 'attach', main, obj, self.relationship, data, skip_sub_obj_read_check=True):
raise PermissionDenied()
@ -155,15 +149,16 @@ class BaseSubList(BaseList):
organization.admins.add(obj)
else:
#if not UserHelper.can_user_read(request.user, obj):
if not check_user_access(request.user, type(obj), 'read', obj):
raise PermissionDenied()
# FIXME: should generalize this
#if not UserHelper.can_user_attach(request.user, main, obj, self.__class__.relationship, data):
if not check_user_access(request.user, self.parent_model, 'attach', main, obj, self.relationship, data):
raise PermissionDenied()
return Response(status=status.HTTP_201_CREATED, data=ser.data)
# why are we returning here?
# return Response(status=status.HTTP_201_CREATED, data=ser.data)
created = True
subs = [ obj ]
else:
@ -189,9 +184,8 @@ class BaseSubList(BaseList):
if not check_user_access(request.user, self.parent_model, 'attach', main, sub, self.relationship, data):
raise PermissionDenied()
if sub in relationship.all():
return Response(status=status.HTTP_409_CONFLICT)
relationship.add(sub)
if sub not in relationship.all():
relationship.add(sub)
else:
if not request.user.is_superuser:
if type(main) != User:
@ -211,7 +205,11 @@ class BaseSubList(BaseList):
sub.name = "_deleted_%s_%s" % (str(datetime.time()), sub.name)
sub.active = False
sub.save()
return Response(status=status.HTTP_204_NO_CONTENT)
if created:
return Response(status=status.HTTP_201_CREATED, data=ser.data)
else:
return Response(status=status.HTTP_204_NO_CONTENT)
class BaseDetail(generics.RetrieveUpdateDestroyAPIView):

View File

@ -169,7 +169,7 @@ class BaseTestMixin(object):
assert response.status_code == expect, "expected status %s, got %s for url=%s as auth=%s: %s" % (expect, response.status_code, url, auth, response.content)
if method_name == 'head':
self.assertFalse(response.content)
if response.status_code not in [ 202, 204, 405, 409 ] and method_name != 'head' and response.content:
if response.status_code not in [ 202, 204, 405 ] and method_name != 'head' and response.content:
# no JSON responses in these at least for now, 409 should probably return some (FIXME)
return json.loads(response.content)
else:

View File

@ -13,6 +13,7 @@ from lib.main.tests.base import BaseTest
class InventoryTest(BaseTest):
def setUp(self):
super(InventoryTest, self).setUp()
self.setup_users()
self.organizations = self.make_organizations(self.super_django_user, 3)
@ -362,8 +363,8 @@ class InventoryTest(BaseTest):
g2.save()
# a super user can set subgroups
subgroups_url = '/api/v1/groups/1/children/'
child_url = '/api/v1/groups/2/'
subgroups_url = '/api/v1/groups/1/children/'
child_url = '/api/v1/groups/2/'
subgroups_url2 = '/api/v1/groups/3/children/'
subgroups_url3 = '/api/v1/groups/4/children/'
subgroups_url4 = '/api/v1/groups/5/children/'
@ -375,9 +376,18 @@ class InventoryTest(BaseTest):
self.assertEquals(checked['count'], 1)
# an org admin can set subgroups
self.post(subgroups_url2, data=got, expect=204, auth=self.get_normal_credentials())
# double post causes conflict error
self.post(subgroups_url2, data=got, expect=409, auth=self.get_normal_credentials())
posted = self.post(subgroups_url2, data=got, expect=204, auth=self.get_normal_credentials())
# see if we can post a completely new subgroup
new_data = dict(inventory=5, name='completely new', description='blarg?')
kids = self.get(subgroups_url2, expect=200, auth=self.get_normal_credentials())
self.assertEqual(kids['count'], 1)
posted2 = self.post(subgroups_url2, data=new_data, expect=201, auth=self.get_normal_credentials())
with_one_more_kid = self.get(subgroups_url2, expect=200, auth=self.get_normal_credentials())
self.assertEqual(with_one_more_kid['count'], 2)
# double post causes conflict error (actually, should it? -- just got a 204, already associated)
# self.post(subgroups_url2, data=got, expect=409, auth=self.get_normal_credentials())
checked = self.get(subgroups_url2, expect=200, auth=self.get_normal_credentials())
# a normal user cannot set subgroups