diff --git a/lib/main/base_views.py b/lib/main/base_views.py index 19297d3196..64d8a9b514 100644 --- a/lib/main/base_views.py +++ b/lib/main/base_views.py @@ -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): diff --git a/lib/main/tests/base.py b/lib/main/tests/base.py index 8d530ee0a4..f040a9515d 100644 --- a/lib/main/tests/base.py +++ b/lib/main/tests/base.py @@ -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: diff --git a/lib/main/tests/inventory.py b/lib/main/tests/inventory.py index a4d2639fce..914a999ffc 100644 --- a/lib/main/tests/inventory.py +++ b/lib/main/tests/inventory.py @@ -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