Added V2 functionality to handle queries against a program_uuid inste… #136

davec-edx · 2016-12-20T16:17:07Z

Part of the conversion from program_id to program_uuid... Added a V2 endpoint for credentials that would function using program_uuid instead of a program_id.

Added unit tests for this V2 functionality
Adjusted V1 unit tests to confirm it is a V1 request not a V2
ECOM-6482
@edx/ecommerce

clintonb · 2016-12-20T16:19:25Z

credentials/apps/api/filters.py

@@ -5,6 +5,17 @@
 from credentials.apps.credentials.models import UserCredential


+class ProgramFilterByUuid(django_filters.FilterSet):


This should be UserCredentialFilter going forward since it is a filter for UserCredential objects. Sub-class it, add the program_id filter, call it something else, and use the sub-class for the v1 API. You could also move the sub-class to /api/v1/filters.py.

clintonb · 2016-12-20T16:22:01Z

credentials/apps/api/tests/test_views.py

+                                                                      'program_id': self.program_id}, expected={
+            'error': 'A program_uuid query string parameter should not appear in V1 queries.'
+        })
+
    def test_list_with_program_id_filter(self):
        """ Verify the list endpoint supports filter data by program_id."""
        program_cert = factories.ProgramCertificateFactory(program_id=001)


clintonb · 2016-12-20T16:22:06Z

credentials/apps/api/tests/test_views.py

    def test_list_with_program_id_filter(self):
        """ Verify the list endpoint supports filter data by program_id."""
        program_cert = factories.ProgramCertificateFactory(program_id=001)
        factories.UserCredentialFactory.create(credential=program_cert)
        self.assert_list_with_id_filter(data={'program_id': self.program_id})

+    def test_list_with_program_invalid_id_filter(self):
+        """ Verify the list endpoint supports filter data by program_id."""
+        program_cert = factories.ProgramCertificateFactory(program_id=001)


clintonb · 2016-12-20T16:22:52Z

credentials/apps/api/v1/views.py

@@ -54,6 +54,10 @@ def list(self, request, *args, **kwargs):
            raise ValidationError(
                {'error': 'A program_id query string parameter is required for filtering program credentials.'})

+        if self.request.query_params.get('program_uuid'):
+            raise ValidationError(
+                {'error': 'A program_uuid query string parameter should not appear in V1 queries.'})


Why not? We can be forwards-compatible.

We shouldn't be injecting additional query parameters, the consumer is either calling a V1 or a V2, if they have the uuid parameter it should be V2. If we do this, then what's the point of versioning the api?

I would leave the v1 API alone. It is already setup properly to respond to requests that don't include the program_id parameter. If a client passes the program_uuid parameter, the v1 API shouldn't care. It's the same as if the client passed the foobar parameter.

This still needs to be removed.

This was a helper for the upgrade to V2. You are correct, the parameter will be ignored, but it will result in the wrong type of error (incorrect parameter vs missing). When we deprecate it it will go away.
Removing it now.

clintonb · 2016-12-20T16:23:32Z

credentials/apps/api/v2/tests/__init__.py

@@ -0,0 +1 @@
+# Create your tests in sub-packages prefixed with "test_" (e.g. test_views).


Useless comment is useless. Also, where are the tests for ProgramsCredentialsViewSet?

clintonb · 2016-12-20T16:26:01Z

credentials/apps/api/v2/urls.py

+router = DefaultRouter()  # pylint: disable=invalid-name
+# URL can not have hyphen as it is not currently supported by slumber
+# as mentioned https://github.com/samgiles/slumber/issues/44
+router.register(r'program_credentials', views.ProgramsCredentialsViewSet, base_name='programcredential')


You also need to include the other resources from API v1.

clintonb · 2016-12-20T16:26:20Z

credentials/apps/api/v2/views.py

@@ -0,0 +1,37 @@
+"""
+Credentials service API views (v1).


These docstrings are rarely useful. Remove them.

clintonb · 2016-12-20T16:26:24Z

credentials/apps/api/v2/urls.py

@@ -0,0 +1,11 @@
+""" API v2 URLs. """


clintonb · 2016-12-20T16:27:17Z

credentials/apps/api/v2/views.py

+                {'error': 'A UUID query string parameter is required for filtering program credentials.'})
+
+        # Confirmation that we are not supplying both parameters. We should only be providing the program_uuid in V2
+        if self.request.query_params.get('program_id'):


This check is not useful. Just ignore the parameter.

I would treat this as any other arbitrary parameter. We don't explicitly block calls made with other parameters. Why block this one? If they aren't using UUID, we'll error out. If they are, they will get the expected result. The odds of anyone using both are low. This is unnecessary defensive coding.

This still needs to be removed, along with associated tests.

We talked about this on Tuesday and thought the resolution was different. API security dictates that we should A) Validate that all parameters/options that are given are of the correct type, and B) Validate that extra parameters/options are not given. This is to prevent injections that could be potentially harmful. Additionally, in a versioned API we can add helper methods for deprecation of an older version so that consumers of the API are checked to ensure they are not passing "old" parameters. We should add a subtask to the deprecation of the old version to remove this.

The query string parameters have nothing to do with security. Yes, we will validate parameters we do care about. When we get parameters we don't care about, we just ignore them. This is the case for all of our APIs. The only place where we drop requests with unexpected parameters are for APIs behind Amazon's API Gateway, and that is only due to a (now lifted) restriction of API Gateway.

We (edx.org) are the only consumers of this API. There is no need for the helpers. The work to modify edx-platform to use this new version of the API is already in the review state: https://github.com/edx/edx-platform/pull/14259.

If this were an API with a larger user base, such as the Catalog API, I would agree that we should be more careful. Since we have exactly one consumer, all you're doing here is adding debt.

@davec-edx I'd like to second @clintonb here. If a parameter we don't expect or otherwise need is provided, it'll be ignored. I don't see the value in including this.

clintonb · 2016-12-20T16:30:06Z

credentials/apps/api/tests/test_views.py


    def test_permission_required(self):
        """ Verify that requests require explicit model permissions. """
        self.assert_permission_required({'program_id': self.program_id, 'status': UserCredential.AWARDED})


+class ProgramCredentialViewSetTestsV2(CredentialViewSetTests):


This belongs in the v2 package.

clintonb

The overall direction is great; however, I would like to remove the notion of program_id completely from v2 (including tests). Feel free to disregard my comments on test cleanup. We'll be back in this repo for ECOM-4639, and can take care of that then.

clintonb · 2017-01-03T17:09:26Z

credentials/apps/api/tests/test_views.py

 JSON_CONTENT_TYPE = 'application/json'
 LOGGER_NAME = 'credentials.apps.credentials.issuers'
 LOGGER_NAME_SERIALIZER = 'credentials.apps.api.serializers'


+class CredentialViewSetTests(APITestCase):


This should be a mixin—inherit object—to avoid executing setUp unnecessarily. The test runner considers anything that inherits TestCase to be a test class. Also, rename to CredentialViewSetTestMixin.

This still needs to be addressed.

clintonb · 2017-01-03T17:15:51Z

credentials/apps/api/v1/tests/test_api.py

@@ -0,0 +1,91 @@
+"""


This docstring can go away.

clintonb · 2017-01-03T17:16:57Z

credentials/apps/api/v1/tests/test_api.py

+"""
+Tests for credentials service views.
+"""
+# pylint: disable=no-member


We generally discourage file-wide disabling of PyLint warnings. Does this need to be disabled for anything outside of setUp (which uses factories)?

clintonb · 2017-01-03T17:17:23Z

credentials/apps/api/v1/tests/test_api.py

+
+
+class ProgramCredentialViewSetTests(CredentialViewSetTests):
+    """ Tests for ProgramCredentialViewSetTests. """


This docstring doesn't add any value. It can go.

clintonb · 2017-01-03T17:17:34Z

credentials/apps/api/v1/tests/test_api.py

+class ProgramCredentialViewSetTests(CredentialViewSetTests):
+    """ Tests for ProgramCredentialViewSetTests. """
+
+    list_path = reverse("api:v1:programcredential-list")


Single quotes.

clintonb · 2017-01-03T17:38:37Z

credentials/apps/api/tests/test_views.py

+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, expected)
+
+    def assert_list_with_status_filter(self, data, should_exist=True):


This method is weird given that it hides the status filter in the data argument. Find a way to combine this with the method above. Both seem to do almost the same thing.

This can be ignored. We'll fix it later.

clintonb · 2017-01-03T17:39:32Z

credentials/apps/api/v2/tests/test_api.py

+
+    def test_list_with_status_filter(self):
+        """ Verify the list endpoint supports filtering by status."""
+        factories.UserCredentialFactory.create_batch(2, status="revoked", username=self.user_credential.username)


UserCredential.REVOKED

fixed in v1 and v2

clintonb · 2017-01-03T17:42:42Z

credentials/apps/api/v2/tests/test_api.py

+    def test_list_with_status_filter(self):
+        """ Verify the list endpoint supports filtering by status."""
+        factories.UserCredentialFactory.create_batch(2, status="revoked", username=self.user_credential.username)
+        self.assert_list_with_status_filter(data={'program_uuid': self.program_uuid, 'status': UserCredential.AWARDED})


I don't follow this test, which is a side-effect of these helper methods being used. It would actually be somewhat clearer to explicitly create the data, make the request, and validate the response in each test. The reliance on the helper methods was good in theory to de-dupe code. However, we seem to have taken that to an extreme that complicates understanding some of the tests.

clintonb · 2017-01-03T17:43:21Z

credentials/apps/api/v2/views.py

+from credentials.apps.api.permissions import UserCredentialViewSetPermissions
+from credentials.apps.api.serializers import UserCredentialCreationSerializer, UserCredentialSerializer
+from credentials.apps.credentials.models import UserCredential
+


Remove this new line.

fixed v1 and v2

clintonb · 2017-01-03T17:43:31Z

credentials/apps/api/v2/views.py

+
+from rest_framework import mixins, viewsets
+from rest_framework.exceptions import ValidationError
+from credentials.apps.api.filters import CourseFilter


This should be grouped below.

fixed in v1 and v2

clintonb · 2017-01-04T21:31:39Z

@davec-edx please rebase to prepare for merging.

clintonb · 2017-01-04T21:40:13Z

credentials/apps/api/v2/views.py

+from credentials.apps.api.permissions import UserCredentialViewSetPermissions
+from credentials.apps.api.serializers import UserCredentialCreationSerializer, UserCredentialSerializer
+from credentials.apps.api.v2.filters import UserCredentialFilter
+


Remove this newline

clintonb · 2017-01-04T21:40:37Z

credentials/apps/api/v2/views.py

+                {'error': 'A UUID query string parameter is required for filtering program credentials.'})
+
+        # Confirmation that we are not supplying both parameters. We should only be providing the program_uuid in V2
+        if self.request.query_params.get('program_id'):


This still needs to be removed, along with associated tests.

clintonb · 2017-01-04T21:42:38Z

credentials/apps/api/v2/tests/test_views.py

+from django.core.urlresolvers import reverse
+from rest_framework.test import APIRequestFactory, APITestCase
+
+from credentials.apps.api.tests.test_views import (CredentialViewSetTests, BaseUserCredentialViewSetTests,


Move the contents within the parentheses to ease indentation issues:

from credentials.apps.api.tests.test_views import ( CredentialViewSetTests, BaseUserCredentialViewSetTests, BaseUserCredentialViewSetPermissionsTests, BaseCourseCredentialViewSetTests )

ahh, I see what you're looking for -- fixed

clintonb · 2017-01-04T21:43:02Z

credentials/apps/api/v1/views.py

@@ -54,6 +54,10 @@ def list(self, request, *args, **kwargs):
            raise ValidationError(
                {'error': 'A program_id query string parameter is required for filtering program credentials.'})

+        if self.request.query_params.get('program_uuid'):
+            raise ValidationError(
+                {'error': 'A program_uuid query string parameter should not appear in V1 queries.'})


This still needs to be removed.

clintonb · 2017-01-04T21:43:17Z

credentials/apps/api/v1/views.py

 from credentials.apps.api.permissions import UserCredentialViewSetPermissions
 from credentials.apps.api.serializers import UserCredentialCreationSerializer, UserCredentialSerializer
-from credentials.apps.credentials.models import UserCredential
+from credentials.apps.api.v1.filters import UserCredentialFilter



Remove the newline.

clintonb · 2017-01-04T21:43:50Z

credentials/apps/api/tests/test_views.py

 JSON_CONTENT_TYPE = 'application/json'
 LOGGER_NAME = 'credentials.apps.credentials.issuers'
 LOGGER_NAME_SERIALIZER = 'credentials.apps.api.serializers'


+class CredentialViewSetTests(APITestCase):


This still needs to be addressed.

clintonb · 2017-01-04T21:44:13Z

credentials/apps/api/tests/test_views.py

+        response = self.client.get(self.list_path, data)
+        self.assertEqual(response.status_code, 403)
+
+    def assert_list_without_id_filter(self, path, expected, data=None):


This can be ignored. We'll fix it later.

clintonb · 2017-01-04T21:44:15Z

credentials/apps/api/tests/test_views.py

+        self.assertEqual(response.status_code, 400)
+        self.assertEqual(response.data, expected)
+
+    def assert_list_with_id_filter(self, data=None, should_exist=True):


This can be ignored. We'll fix it later.

clintonb · 2017-01-04T21:44:19Z

credentials/apps/api/tests/test_views.py

+        self.assertEqual(response.status_code, 200)
+        self.assertEqual(response.data, expected)
+
+    def assert_list_with_status_filter(self, data, should_exist=True):


This can be ignored. We'll fix it later.

rlucioni · 2017-01-06T20:01:52Z

credentials/apps/api/v2/views.py

+
+
+class ProgramsCredentialsViewSet(mixins.ListModelMixin, viewsets.GenericViewSet):
+    """It will return the all credentials for programs based on the program_uuid."""


Could you clean up this docstring?

rlucioni · 2017-01-06T20:04:22Z

credentials/apps/api/v2/views.py

+                {'error': 'A UUID query string parameter is required for filtering program credentials.'})
+
+        # Confirmation that we are not supplying both parameters. We should only be providing the program_uuid in V2
+        if self.request.query_params.get('program_id'):


@davec-edx I'd like to second @clintonb here. If a parameter we don't expect or otherwise need is provided, it'll be ignored. I don't see the value in including this.

…ad of program_id Added unit tests for this V2 functionality Adjusted V1 unit tests to confirm it is a V1 request not a V2 ECOM-6482

clintonb suggested changes Dec 20, 2016

View reviewed changes

davec-edx force-pushed the ecom6482 branch 5 times, most recently from 1ec47e3 to ea3868a Compare December 21, 2016 17:52

clintonb reviewed Jan 3, 2017

View reviewed changes

davec-edx force-pushed the ecom6482 branch 3 times, most recently from 0e05c22 to 8e6cc63 Compare January 3, 2017 21:01

clintonb suggested changes Jan 4, 2017

View reviewed changes

davec-edx force-pushed the ecom6482 branch 3 times, most recently from 5803017 to 97f1cf9 Compare January 5, 2017 16:04

clintonb approved these changes Jan 5, 2017

View reviewed changes

davec-edx force-pushed the ecom6482 branch from 97f1cf9 to 14f3265 Compare January 6, 2017 02:46

rlucioni reviewed Jan 6, 2017

View reviewed changes

Added V2 functionality to handle queries against a program_uuid inste…

c6fa3bf

…ad of program_id Added unit tests for this V2 functionality Adjusted V1 unit tests to confirm it is a V1 request not a V2 ECOM-6482

davec-edx force-pushed the ecom6482 branch from 14f3265 to c6fa3bf Compare January 9, 2017 22:17

davec-edx merged commit d6f7fc2 into master Jan 9, 2017

davec-edx deleted the ecom6482 branch January 9, 2017 22:21

		@@ -5,6 +5,17 @@
		from credentials.apps.credentials.models import UserCredential


		class ProgramFilterByUuid(django_filters.FilterSet):

		@@ -0,0 +1 @@
		# Create your tests in sub-packages prefixed with "test_" (e.g. test_views).



		class ProgramCredentialViewSetTests(CredentialViewSetTests):
		""" Tests for ProgramCredentialViewSetTests. """



		class ProgramsCredentialsViewSet(mixins.ListModelMixin, viewsets.GenericViewSet):
		"""It will return the all credentials for programs based on the program_uuid."""

Added V2 functionality to handle queries against a program_uuid inste… #136

Added V2 functionality to handle queries against a program_uuid inste… #136

Conversation

davec-edx commented Dec 20, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintonb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clintonb commented Jan 4, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

davec-edx commented Dec 20, 2016 •

edited

Loading