Merge ~andreserl/maas:lp1785721 into maas:master

[Andres Rodriguez]

LP: #1785721 - Allow read, update and delete a resource pool with its name

[Mike Pontillo (mpontillo)]

Nice job implementing specifiers for the ResourcePool, and thanks for the docstring cleanup!

Overall, the code looks good to me. But you'll need to add tests in src/maasserver/api/tests/test_resourcepool.py to cover the additional functionality added in this branch.

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1785721 lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/3536/console
COMMIT: fe8373ae96ea19926f6660743adc0cf7224b0b14

review: Needs Fixing

[MAAS Lander (maas-lander)]

UNIT TESTS
-b lp1785721 lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: SUCCESS
COMMIT: e919f44bfcea73ad8df9a28d44428ab9c8e08b33

review: Approve

[Blake Rouse (blake-rouse)]

review: Needs Fixing

[Andres Rodriguez (andreserl)]

FWIW, CI has passed: http://162.213.35.104:8080/job/maas-git-manual/341/

I'll add the test.

[Blake Rouse (blake-rouse)]

Looks good. Thanks for adding the test.

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b lp1785721 lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/3551/consoleText

[Mike Pontillo (mpontillo)]

Thanks for adding the tests!

review: Approve

[MAAS Lander (maas-lander)]

LANDING
-b lp1785721 lp:~andreserl/maas/+git/maas into -b master lp:~maas-committers/maas

STATUS: FAILED BUILD
LOG: http://maas-ci-jenkins.internal:8080/job/maas/job/branch-tester/3557/consoleText

diff --git a/src/maasserver/api/resourcepools.py b/src/maasserver/api/resourcepools.py
index cb232d9..495b640 100644
--- a/src/maasserver/api/resourcepools.py
+++ b/src/maasserver/api/resourcepools.py
@@ -13,8 +13,11 @@ from maasserver.api.support import (
     ModelCollectionOperationsHandler,
     ModelOperationsHandler,
 )
+from maasserver.enum import NODE_PERMISSION
+from maasserver.exceptions import MAASAPIValidationError
 from maasserver.forms import ResourcePoolForm
 from maasserver.models import ResourcePool
+from piston3.utils import rc
 
 
 DISPLAYED_RESOURCEPOOL_FIELDS = (
@@ -45,14 +48,34 @@ class ResourcePoolHandler(ModelOperationsHandler):
 
         Returns 404 if the resource pool is not found.
         """
-        return super().read(request, id=id)
+        return ResourcePool.objects.get_resource_pool_or_404(
+            id, request.user, NODE_PERMISSION.VIEW)
 
     def update(self, request, id):
         """PUT request.  Update resource pool.
 
+        Please see the documentation for the 'create' operation for detailed
+        descriptions of each parameter.
+
+        Optional parameters
+        -------------------
+
+        name
+            Name of the resource pool.
+
+        description
+            Description of the resource pool.
+
         Returns 404 if the resource pool is not found.
         """
-        return super().update(request, id=id)
+
+        pool = ResourcePool.objects.get_resource_pool_or_404(
+            id, request.user, NODE_PERMISSION.ADMIN)
+        form = ResourcePoolForm(instance=pool, data=request.data)
+        if form.is_valid():
+            return form.save()
+        else:
+            raise MAASAPIValidationError(form.errors)
 
     def delete(self, request, id):
         """DELETE request.  Delete resource pool.
@@ -60,7 +83,10 @@ class ResourcePoolHandler(ModelOperationsHandler):
         Returns 404 if the resource pool is not found.
         Returns 204 if the resource pool is successfully deleted.
         """
-        return super().delete(request, id=id)
+        pool = ResourcePool.objects.get_resource_pool_or_404(
+            id, request.user, NODE_PERMISSION.ADMIN)
+        pool.delete()
+        return rc.DELETED
 
 
 class ResourcePoolsHandler(ModelCollectionOperationsHandler):
diff --git a/src/maasserver/api/tests/test_resourcepool.py b/src/maasserver/api/tests/test_resourcepool.py
index 7e93e43..4e3d2df 100644
--- a/src/maasserver/api/tests/test_resourcepool.py
+++ b/src/maasserver/api/tests/test_resourcepool.py
@@ -57,6 +57,19 @@ class TestResourcePoolAPI(APITestCase.ForUser):
         self.assertEqual(pool.name, new_name)
         self.assertEqual(pool.description, new_description)
 
+    def test_PUT_updates_pool_by_name(self):
+        self.become_admin()
+        pool = factory.make_ResourcePool()
+        new_name = factory.make_name('name')
+        new_description = factory.make_name('description')
+        response = self.client.put(
+            reverse('resourcepool_handler', args=[pool.name]),
+            {'name': new_name, 'description': new_description})
+        self.assertEqual(response.status_code, http.client.OK)
+        pool = reload_object(pool)
+        self.assertEqual(pool.name, new_name)
+        self.assertEqual(pool.description, new_description)
+
     def test_PUT_missing(self):
         self.become_admin()
         description = factory.make_string()
@@ -72,7 +85,7 @@ class TestResourcePoolAPI(APITestCase.ForUser):
             {'description': factory.make_string()})
         self.assertEqual(response.status_code, http.client.FORBIDDEN)
 
-    def test_DELETE_removes_pool(self):
+    def test_DELETE_removes_pool_by_id(self):
         self.become_admin()
         pool = factory.make_ResourcePool()
         response = self.client.delete(
@@ -80,6 +93,14 @@ class TestResourcePoolAPI(APITestCase.ForUser):
         self.assertEqual(response.status_code, http.client.NO_CONTENT)
         self.assertIsNone(reload_object(pool))
 
+    def test_DELETE_removes_pool_by_name(self):
+        self.become_admin()
+        pool = factory.make_ResourcePool()
+        response = self.client.delete(
+            reverse('resourcepool_handler', args=[pool.name]), {})
+        self.assertEqual(response.status_code, http.client.NO_CONTENT)
+        self.assertIsNone(reload_object(pool))
+
     def test_DELETE_default_pool_denied(self):
         self.become_admin()
         response = self.client.delete(
@@ -100,6 +121,4 @@ class TestResourcePoolAPI(APITestCase.ForUser):
         response = self.client.delete(
             reverse('resourcepool_handler', args=[pool.id]))
         self.assertEqual(response.status_code, http.client.NO_CONTENT)
-        response = self.client.delete(
-            reverse('resourcepool_handler', args=[pool.id]))
-        self.assertEqual(response.status_code, http.client.NO_CONTENT)
+        self.assertIsNone(reload_object(pool))
diff --git a/src/maasserver/models/__init__.py b/src/maasserver/models/__init__.py
index d58ef1e..9177321 100644
--- a/src/maasserver/models/__init__.py
+++ b/src/maasserver/models/__init__.py
@@ -280,6 +280,7 @@ UNRESTRICTED_READ_MODELS = (
     Domain,
     Fabric,
     FanNetwork,
+    ResourcePool,
     Space,
     Subnet,
     StaticRoute,
diff --git a/src/maasserver/models/resourcepool.py b/src/maasserver/models/resourcepool.py
index 377db23..d1651c3 100644
--- a/src/maasserver/models/resourcepool.py
+++ b/src/maasserver/models/resourcepool.py
@@ -11,7 +11,10 @@ __all__ = [
 
 from datetime import datetime
 
-from django.core.exceptions import ValidationError
+from django.core.exceptions import (
+    PermissionDenied,
+    ValidationError,
+)
 from django.db.models import (
     CharField,
     Manager,
@@ -21,13 +24,27 @@ from maasserver import DefaultMeta
 from maasserver.fields import MODEL_NAME_VALIDATOR
 from maasserver.models.cleansave import CleanSave
 from maasserver.models.timestampedmodel import TimestampedModel
+from maasserver.utils.orm import MAASQueriesMixin
 
 
 DEFAULT_RESOURCEPOOL_NAME = 'default'
 DEFAULT_RESOURCEPOOL_DESCRIPTION = 'Default pool'
 
 
-class ResourcePoolManager(Manager):
+class ResourcePoolQueriesMixin(MAASQueriesMixin):
+
+    def get_specifiers_q(self, specifiers, separator=':', **kwargs):
+        specifier_types = {
+            None: self._add_default_query,
+            'name': "__name",
+            'id': "__id",
+        }
+        return super(ResourcePoolQueriesMixin, self).get_specifiers_q(
+            specifiers, specifier_types=specifier_types, separator=separator,
+            **kwargs)
+
+
+class ResourcePoolManager(Manager, ResourcePoolQueriesMixin):
     """Manager for the :class:`ResourcePool` model."""
 
     def get_default_resource_pool(self):
@@ -43,6 +60,30 @@ class ResourcePoolManager(Manager):
                 'updated': now})
         return pool
 
+    def get_resource_pool_or_404(self, specifiers, user, perm):
+        """Fetch a `ResourcePool` by its id.  Raise exceptions if no
+        `ResourcePool` with this id exists or if the provided user has not
+        the required permission to access this `ResourcePool`.
+
+        :param specifiers: A specifier to uniquely locate the ResourcePool.
+        :type specifiers: unicode
+        :param user: The user that should be used in the permission check.
+        :type user: django.contrib.auth.models.User
+        :param perm: The permission to assert that the user has on the node.
+        :type perm: unicode
+        :raises: django.http.Http404_,
+            :class:`maasserver.exceptions.PermissionDenied`.
+
+        .. _django.http.Http404: https://
+           docs.djangoproject.com/en/dev/topics/http/views/
+           #the-http404-exception
+        """
+        pool = self.get_object_by_specifiers_or_raise(specifiers)
+        if user.has_perm(perm, pool):
+            return pool
+        else:
+            raise PermissionDenied()
+
 
 class ResourcePool(CleanSave, TimestampedModel):
     """A resource pool."""
diff --git a/src/maasserver/models/tests/test_resourcepool.py b/src/maasserver/models/tests/test_resourcepool.py
index 8984826..9875084 100644
--- a/src/maasserver/models/tests/test_resourcepool.py
+++ b/src/maasserver/models/tests/test_resourcepool.py
@@ -3,7 +3,11 @@
 
 """Test ResourcePool objects."""
 
-from django.core.exceptions import ValidationError
+from django.core.exceptions import (
+    PermissionDenied,
+    ValidationError,
+)
+from maasserver.enum import NODE_PERMISSION
 from maasserver.models.resourcepool import (
     DEFAULT_RESOURCEPOOL_DESCRIPTION,
     DEFAULT_RESOURCEPOOL_NAME,
@@ -63,3 +67,54 @@ class TestResourcePool(MAASServerTestCase):
         pool = ResourcePool.objects.get_default_resource_pool()
         factory.make_Node(pool=pool)
         self.assertRaises(ValidationError, pool.delete)
+
+
+class TestResourcePoolManagerGetResourcePoolOr404(MAASServerTestCase):
+
+    def test__user_view_returns_resource_pool(self):
+        user = factory.make_User()
+        pool = factory.make_ResourcePool()
+        self.assertEqual(
+            pool,
+            ResourcePool.objects.get_resource_pool_or_404(
+                pool.id, user, NODE_PERMISSION.VIEW))
+
+    def test__user_edit_raises_PermissionError(self):
+        user = factory.make_User()
+        pool = factory.make_ResourcePool()
+        self.assertRaises(
+            PermissionDenied,
+            ResourcePool.objects.get_resource_pool_or_404,
+            pool.id, user, NODE_PERMISSION.EDIT)
+
+    def test__user_admin_raises_PermissionError(self):
+        user = factory.make_User()
+        pool = factory.make_ResourcePool()
+        self.assertRaises(
+            PermissionDenied,
+            ResourcePool.objects.get_resource_pool_or_404,
+            pool.id, user, NODE_PERMISSION.ADMIN)
+
+    def test__admin_view_returns_resource_pool(self):
+        admin = factory.make_admin()
+        pool = factory.make_ResourcePool()
+        self.assertEqual(
+            pool,
+            ResourcePool.objects.get_resource_pool_or_404(
+                pool.id, admin, NODE_PERMISSION.VIEW))
+
+    def test__admin_edit_returns_resource_pool(self):
+        admin = factory.make_admin()
+        pool = factory.make_ResourcePool()
+        self.assertEqual(
+            pool,
+            ResourcePool.objects.get_resource_pool_or_404(
+                pool.id, admin, NODE_PERMISSION.EDIT))
+
+    def test__admin_admin_returns_pool(self):
+        admin = factory.make_admin()
+        pool = factory.make_ResourcePool()
+        self.assertEqual(
+            pool,
+            ResourcePool.objects.get_resource_pool_or_404(
+                pool.id, admin, NODE_PERMISSION.ADMIN))