diff --git a/enterprise_access/apps/api/filters/base.py b/enterprise_access/apps/api/filters/base.py index e564a287..6213851c 100644 --- a/enterprise_access/apps/api/filters/base.py +++ b/enterprise_access/apps/api/filters/base.py @@ -29,6 +29,6 @@ def get_filterset_class(self, view, queryset=None): """ Returns None if this is a retrieve() operation. """ - if view.action == 'retrieve': + if view.action in ('retrieve', 'update', 'partial_update'): return None return super().get_filterset_class(view, queryset) diff --git a/enterprise_access/apps/api/serializers/__init__.py b/enterprise_access/apps/api/serializers/__init__.py index e739a771..c10d60dd 100644 --- a/enterprise_access/apps/api/serializers/__init__.py +++ b/enterprise_access/apps/api/serializers/__init__.py @@ -13,7 +13,8 @@ SubsidyAccessPolicyRedeemableResponseSerializer, SubsidyAccessPolicyRedeemRequestSerializer, SubsidyAccessPolicyRedemptionRequestSerializer, - SubsidyAccessPolicyResponseSerializer + SubsidyAccessPolicyResponseSerializer, + SubsidyAccessPolicyUpdateRequestSerializer ) from .subsidy_requests import ( CouponCodeRequestSerializer, diff --git a/enterprise_access/apps/api/serializers/subsidy_access_policy.py b/enterprise_access/apps/api/serializers/subsidy_access_policy.py index b4a6e114..30f68ae5 100644 --- a/enterprise_access/apps/api/serializers/subsidy_access_policy.py +++ b/enterprise_access/apps/api/serializers/subsidy_access_policy.py @@ -17,6 +17,23 @@ logger = logging.getLogger(__name__) +def validate_no_extra_custom_fields_for_policy_class(policy_class, fields_and_values): + """ + Validates that no fields are included in the serializer + data pertaining to one type of SubsidyAccessPolicy that are + only valid for *another* type of SubsidyAccessPolicy. + e.g. ``per_learner_spend_limit`` is not a valid field for creating + or updating a ``PerLearnerEnrollmentSubsidyAccessPolicy`` record. + """ + unused_custom_fields = set(policy_class.ALL_CUSTOM_FIELDS) - set(policy_class.REQUIRED_CUSTOM_FIELDS) + if unused_custom_fields.intersection(fields_and_values.keys()): + return ( + f"Extraneous fields for {policy_class.__name__} policy type: " + f"{list(unused_custom_fields)}." + ) + return None + + class SubsidyAccessPolicyResponseSerializer(serializers.ModelSerializer): """ A read-only Serializer for responding to requests for ``SubsidyAccessPolicy`` records. @@ -138,12 +155,12 @@ def validate(self, attrs): custom_policy_field_errors.append( f"Missing fields for {policy_type} policy type: {policy_class.REQUIRED_CUSTOM_FIELDS}." ) - # Here's the "no more" part: - unused_custom_fields = set(policy_class.ALL_CUSTOM_FIELDS) - set(policy_class.REQUIRED_CUSTOM_FIELDS) - if unused_custom_fields.intersection(attrs.keys()): - custom_policy_field_errors.append( - f"Extraneous fields for {policy_type} policy type: {list(unused_custom_fields)}." - ) + + # Here's the "no more" part + extra_field_error_msg = validate_no_extra_custom_fields_for_policy_class(policy_class, attrs) + if extra_field_error_msg: + custom_policy_field_errors.append(extra_field_error_msg) + if custom_policy_field_errors: raise serializers.ValidationError(custom_policy_field_errors) @@ -239,6 +256,101 @@ class SubsidyAccessPolicyDeleteRequestSerializer(serializers.Serializer): ) +class SubsidyAccessPolicyUpdateRequestSerializer(serializers.ModelSerializer): + """ + Request Serializer for PUT or PATCH requests to update a subsidy access policy. + + For views: SubsidyAccessPolicyViewSet.update and SubsidyAccessPolicyViewSet.partial_update. + """ + class Meta: + model = SubsidyAccessPolicy + fields = ( + 'description', + 'active', + 'catalog_uuid', + 'subsidy_uuid', + 'access_method', + 'spend_limit', + 'per_learner_spend_limit', + 'per_learner_enrollment_limit', + ) + extra_kwargs = { + 'description': { + 'required': False, + 'allow_blank': False, + 'min_length': None, + 'max_length': 200, + 'trim_whitespace': True, + }, + 'active': { + 'allow_null': False, + 'required': False, + }, + 'catalog_uuid': { + 'allow_null': False, + 'required': False, + }, + 'subsidy_uuid': { + 'allow_null': False, + 'required': False, + }, + 'access_method': { + 'allow_null': False, + 'required': False, + }, + 'spend_limit': { + 'allow_null': True, + 'required': False, + }, + 'per_learner_enrollment_limit': { + 'allow_null': True, + 'required': False, + }, + 'per_learner_spend_limit': { + 'allow_null': True, + 'required': False, + }, + } + + def validate(self, attrs): + """ + Raises a ValidationError if any field not explicitly declared + as a field in this serializer defintion is provided as input. + """ + unknown = sorted(set(self.initial_data) - set(self.fields)) + if unknown: + raise serializers.ValidationError("Field(s) are not updatable: {}".format(", ".join(unknown))) + return attrs + + def update(self, instance, validated_data): + """ + Overwrites the update() method to check that no fields + that are valid in a type of SubsidyAccessPolicy that is *different* + from the type of ``instance`` are present in ``validated_data``. + + We have to do this validation here so that we have access + to a ``SubsidyAccessPolicy`` instance. It's not required + for the caller of the policy update view to provide a policy type, + so we can't infer the desired type from the request payload. + """ + extra_field_error_msg = validate_no_extra_custom_fields_for_policy_class( + instance.__class__, + validated_data, + ) + if extra_field_error_msg: + raise serializers.ValidationError(extra_field_error_msg) + + return super().update(instance, validated_data) + + def to_representation(self, instance): + """ + Once a SubsidyAccessPolicy has been updated, we want to serialize + more fields from the instance than are required in this, the input serializer. + """ + read_serializer = SubsidyAccessPolicyResponseSerializer(instance) + return read_serializer.data + + class SubsidyAccessPolicyRedeemableResponseSerializer(serializers.ModelSerializer): """ Response serializer to represent redeemable policies. diff --git a/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py b/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py index b65bf67a..e59cf332 100644 --- a/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py +++ b/enterprise_access/apps/api/v1/tests/test_subsidy_access_policy_views.py @@ -178,7 +178,7 @@ def test_policy_crud_views_unauthorized_forbidden(self, role_context_dict, expec @ddt.unpack def test_policy_crud_write_views_unauthorized_forbidden(self, role_context_dict, expected_response_code): """ - Tests that we get expected 40x responses for all of the policy readonly views. + Tests that we get expected 40x responses for all of the policy write views. """ # Set the JWT-based auth that we'll use for every request if role_context_dict: @@ -190,6 +190,13 @@ def test_policy_crud_write_views_unauthorized_forbidden(self, role_context_dict, response = self.client.delete(reverse('api:v1:subsidy-access-policies-detail', kwargs=request_kwargs)) self.assertEqual(response.status_code, expected_response_code) + # Test the update and partial_update views. + response = self.client.put(reverse('api:v1:subsidy-access-policies-detail', kwargs=request_kwargs)) + self.assertEqual(response.status_code, expected_response_code) + + response = self.client.patch(reverse('api:v1:subsidy-access-policies-detail', kwargs=request_kwargs)) + self.assertEqual(response.status_code, expected_response_code) + @ddt.ddt class TestAuthenticatedPolicyCRUDViews(CRUDViewTestMixin, APITestWithMocks): @@ -352,6 +359,151 @@ def test_destroy_view(self, request_payload, expected_change_reason): self.assertEqual(response.status_code, status.HTTP_200_OK) self.assertEqual(expected_response, response.json()) + @ddt.data(True, False) + def test_update_views(self, is_patch): + """ + Test that the update and partial_update views can modify certain + fields of a policy record. + """ + # Set the JWT-based auth to an operator. + self.set_jwt_cookie([ + {'system_wide_role': SYSTEM_ENTERPRISE_OPERATOR_ROLE, 'context': str(TEST_ENTERPRISE_UUID)} + ]) + + policy_for_edit = PerLearnerSpendCapLearnerCreditAccessPolicyFactory( + enterprise_customer_uuid=self.enterprise_uuid, + spend_limit=5, + active=False, + ) + + request_payload = { + 'description': 'the new description', + 'active': True, + 'catalog_uuid': str(uuid4()), + 'subsidy_uuid': str(uuid4()), + 'access_method': AccessMethods.ASSIGNED, + 'spend_limit': None, + 'per_learner_spend_limit': 10000, + } + + action = self.client.patch if is_patch else self.client.put + url = reverse( + 'api:v1:subsidy-access-policies-detail', + kwargs={'uuid': str(policy_for_edit.uuid)} + ) + response = action(url, data=request_payload) + + self.assertEqual(response.status_code, status.HTTP_200_OK) + + expected_response = { + 'access_method': AccessMethods.ASSIGNED, + 'active': True, + 'catalog_uuid': request_payload['catalog_uuid'], + 'description': request_payload['description'], + 'enterprise_customer_uuid': str(self.enterprise_uuid), + 'per_learner_enrollment_limit': None, + 'per_learner_spend_limit': request_payload['per_learner_spend_limit'], + 'policy_type': 'PerLearnerSpendCreditAccessPolicy', + 'spend_limit': request_payload['spend_limit'], + 'subsidy_uuid': request_payload['subsidy_uuid'], + 'uuid': str(policy_for_edit.uuid), + } + self.assertEqual(expected_response, response.json()) + + @ddt.data( + { + 'enterprise_customer_uuid': str(uuid4()), + 'uuid': str(uuid4()), + 'policy_type': 'PerLearnerEnrollmentCapCreditAccessPolicy', + 'created': '1970-01-01 12:00:00Z', + 'modified': '1970-01-01 12:00:00Z', + 'nonsense_key': 'ship arriving too late to save a drowning witch', + }, + ) + def test_update_views_fields_disallowed_for_update(self, request_payload): + """ + Test that the update and partial_update views can NOT modify fields + of a policy record that are not included in the update request serializer fields defintion. + """ + # Set the JWT-based auth to an operator. + self.set_jwt_cookie([ + {'system_wide_role': SYSTEM_ENTERPRISE_OPERATOR_ROLE, 'context': str(TEST_ENTERPRISE_UUID)} + ]) + + policy_for_edit = PerLearnerSpendCapLearnerCreditAccessPolicyFactory( + enterprise_customer_uuid=self.enterprise_uuid, + spend_limit=5, + active=False, + ) + url = reverse( + 'api:v1:subsidy-access-policies-detail', + kwargs={'uuid': str(policy_for_edit.uuid)} + ) + + expected_unknown_keys = ", ".join(sorted(request_payload.keys())) + + # Test the PUT view + response = self.client.put(url, data=request_payload) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + self.assertEqual( + response.json(), + {'non_field_errors': [f'Field(s) are not updatable: {expected_unknown_keys}']}, + ) + + # Test the PATCH view + response = self.client.patch(url, data=request_payload) + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + + self.assertEqual( + response.json(), + {'non_field_errors': [f'Field(s) are not updatable: {expected_unknown_keys}']}, + ) + + @ddt.data( + { + 'policy_class': PerLearnerSpendCapLearnerCreditAccessPolicyFactory, + 'request_payload': { + 'per_learner_enrollment_limit': 10, + }, + }, + { + 'policy_class': PerLearnerEnrollmentCapLearnerCreditAccessPolicyFactory, + 'request_payload': { + 'per_learner_spend_limit': 1000, + }, + }, + ) + @ddt.unpack + def test_update_view_validates_fields_vs_policy_type(self, policy_class, request_payload): + """ + Test that the update view can NOT modify fields + of a policy record that are relevant only to a different + type of policy. + """ + # Set the JWT-based auth to an operator. + self.set_jwt_cookie([ + {'system_wide_role': SYSTEM_ENTERPRISE_OPERATOR_ROLE, 'context': str(TEST_ENTERPRISE_UUID)} + ]) + + policy_for_edit = policy_class( + enterprise_customer_uuid=self.enterprise_uuid, + spend_limit=5, + active=False, + ) + url = reverse( + 'api:v1:subsidy-access-policies-detail', + kwargs={'uuid': str(policy_for_edit.uuid)} + ) + + response = self.client.put(url, data=request_payload) + + self.assertEqual(response.status_code, status.HTTP_400_BAD_REQUEST) + expected_error_message = ( + f"Extraneous fields for {policy_for_edit.__class__.__name__} policy type: " + f"{list(request_payload)}." + ) + self.assertEqual(response.json(), [expected_error_message]) + @ddt.ddt class TestAdminPolicyCreateView(CRUDViewTestMixin, APITestWithMocks): diff --git a/enterprise_access/apps/api/v1/views/subsidy_access_policy.py b/enterprise_access/apps/api/v1/views/subsidy_access_policy.py index abd825ed..c274b12a 100644 --- a/enterprise_access/apps/api/v1/views/subsidy_access_policy.py +++ b/enterprise_access/apps/api/v1/views/subsidy_access_policy.py @@ -66,7 +66,7 @@ SUBSIDY_ACCESS_POLICY_REDEMPTION_API_TAG = 'Subsidy Access Policy Redemption' -def policy_permission_detail_fn(request, *args, uuid=None): +def policy_permission_detail_fn(request, *args, uuid=None, **kwargs): """ Helper to use with @permission_required on detail-type endpoints (retrieve, update, partial_update, destroy). @@ -79,6 +79,7 @@ def policy_permission_detail_fn(request, *args, uuid=None): class SubsidyAccessPolicyViewSet( mixins.ListModelMixin, mixins.RetrieveModelMixin, + mixins.UpdateModelMixin, mixins.DestroyModelMixin, viewsets.GenericViewSet, ): @@ -99,6 +100,15 @@ def get_queryset(self): """ return SubsidyAccessPolicy.objects.all() + def get_serializer_class(self): + """ + Overrides the default behavior to return different + serializers depending on the request action. + """ + if self.action in ('update', 'partial_update'): + return serializers.SubsidyAccessPolicyUpdateRequestSerializer + return self.serializer_class + @extend_schema( tags=[SUBSIDY_ACCESS_POLICY_CRUD_API_TAG], summary='Retrieve subsidy access policy by UUID.', @@ -125,6 +135,40 @@ def list(self, request, *args, **kwargs): """ return super().list(request, *args, **kwargs) + @extend_schema( + tags=[SUBSIDY_ACCESS_POLICY_CRUD_API_TAG], + summary='Partially update (with a PUT) a subsidy access policy by UUID.', + request=serializers.SubsidyAccessPolicyUpdateRequestSerializer, + responses={ + status.HTTP_200_OK: serializers.SubsidyAccessPolicyResponseSerializer, + status.HTTP_404_NOT_FOUND: None, + }, + ) + @permission_required(SUBSIDY_ACCESS_POLICY_WRITE_PERMISSION, fn=policy_permission_detail_fn) + def update(self, request, *args, uuid=None, **kwargs): + """ + Updates a single `SubsidyAccessPolicy` record by uuid. All fields for the update are optional + (which is different from a standard PUT request). + """ + kwargs['partial'] = True + return super().update(request, *args, uuid=uuid, **kwargs) + + @extend_schema( + tags=[SUBSIDY_ACCESS_POLICY_CRUD_API_TAG], + summary='Partially update (with a PATCH) a subsidy access policy by UUID.', + request=serializers.SubsidyAccessPolicyUpdateRequestSerializer, + responses={ + status.HTTP_200_OK: serializers.SubsidyAccessPolicyResponseSerializer, + status.HTTP_404_NOT_FOUND: None, + }, + ) + @permission_required(SUBSIDY_ACCESS_POLICY_WRITE_PERMISSION, fn=policy_permission_detail_fn) + def partial_update(self, request, *args, uuid=None, **kwargs): + """ + Updates a single `SubsidyAccessPolicy` record by uuid. All fields for the update are optional. + """ + return super().partial_update(request, *args, uuid=uuid, **kwargs) + @extend_schema( tags=[SUBSIDY_ACCESS_POLICY_CRUD_API_TAG], summary='Soft-delete subsidy access policy by UUID.',