Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: allow redemption at requested prices #185

Merged
merged 1 commit into from
Nov 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions enterprise_subsidy/apps/api/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ class ErrorCodes:
"""
ENROLLMENT_ERROR = 'enrollment_error'
CONTENT_NOT_FOUND = 'content_not_found'
INVALID_REQUESTED_PRICE = 'invalid_requested_price'
TRANSACTION_CREATION_ERROR = 'transaction_creation_error'
LEDGER_LOCK_ERROR = 'ledger_lock_error'
INACTIVE_SUBSIDY_CREATION_ERROR = 'inactive_subsidy_creation_error'
Expand Down
22 changes: 21 additions & 1 deletion enterprise_subsidy/apps/api/v1/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,12 @@
from rest_framework import serializers

from enterprise_subsidy.apps.fulfillment.api import FulfillmentException
from enterprise_subsidy.apps.subsidy.models import ContentNotFoundForCustomerException, RevenueCategoryChoices, Subsidy
from enterprise_subsidy.apps.subsidy.models import (
ContentNotFoundForCustomerException,
PriceValidationError,
RevenueCategoryChoices,
Subsidy
)

logger = getLogger(__name__)

Expand Down Expand Up @@ -194,6 +199,14 @@ class TransactionCreationRequestSerializer(serializers.ModelSerializer):
required=False,
allow_null=True,
)
requested_price_cents = serializers.IntegerField(
required=False,
allow_null=True,
help_text=(
'The price, in USD cents, at which the caller requests the redemption be made. Must be >= 0.'
),
min_value=0,
)

class Meta:
"""
Expand All @@ -206,6 +219,7 @@ class Meta:
'content_key',
'subsidy_access_policy_uuid',
'metadata',
'requested_price_cents',
]
# Override lms_user_id, content_key, and subsidy_access_policy_uuid to each be required;
# their model field definitions have `required=False`.
Expand Down Expand Up @@ -247,6 +261,7 @@ def create(self, validated_data):
validated_data['content_key'],
validated_data['subsidy_access_policy_uuid'],
idempotency_key=validated_data.get('idempotency_key'),
requested_price_cents=validated_data.get('requested_price_cents'),
metadata=validated_data.get('metadata'),
)
except LedgerLockAttemptFailed as exc:
Expand All @@ -263,6 +278,11 @@ def create(self, validated_data):
f'in subsidy {subsidy.uuid}'
)
raise exc
except PriceValidationError as exc:
logger.error(
f'Invalid price requested for {validated_data} in subsidy {subsidy.uuid}'
)
raise exc
except FulfillmentException as exc:
logger.error(
f'Error fulfilling transactions for {validated_data} in subsidy {subsidy.uuid}'
Expand Down
81 changes: 77 additions & 4 deletions enterprise_subsidy/apps/api/v2/tests/test_transaction_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -704,6 +704,7 @@ def test_operator_creation_with_lock_failure_gets_429(self):
self.content_key_2,
uuid.UUID(self.subsidy_access_policy_1_uuid),
idempotency_key='my-idempotency-key',
requested_price_cents=None,
metadata=None,
)
assert response.json() == {'detail': 'Attempt to lock the Ledger failed, please try again.'}
Expand Down Expand Up @@ -755,13 +756,73 @@ def test_operator_creation_expected_422_errors(self, exception_to_raise, expecte
self.content_key_2,
uuid.UUID(self.subsidy_access_policy_1_uuid),
idempotency_key='my-idempotency-key',
requested_price_cents=None,
metadata=None,
)
assert response.json() == {
'detail': str(exception_to_raise),
'code': expected_error_code,
}

@mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.enterprise_client")
@mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.price_for_content")
@mock.patch("enterprise_subsidy.apps.content_metadata.api.ContentMetadataApi.get_content_summary")
@mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.lms_user_client")
def test_operator_creation_requested_price_invalid(
self,
mock_lms_user_client,
mock_get_content_summary,
mock_price_for_content,
mock_enterprise_client
):
"""
Tests that the admin transaction creation endpoint responds with a 422
when creating a transaction for an invalid requested price.
"""
self.set_up_operator()

canonical_price_cents = 1000
# request only half of the canonical price, which falls outside default allowable interval
requested_price_cents = 500
mock_lms_user_client.return_value.best_effort_user_data.return_value = {
'email': self.lms_user_email,
}
mock_enterprise_client.enroll.return_value = 'my-fulfillment-id'
mock_price_for_content.return_value = canonical_price_cents
mock_get_content_summary.return_value = {
'content_uuid': self.content_key_1,
'content_key': self.content_key_1,
'content_title': self.content_title_1,
'source': 'edX',
'mode': 'verified',
'content_price': canonical_price_cents,
'geag_variant_id': None,
}
url = reverse("api:v2:transaction-admin-list-create", args=[self.subsidy_1.uuid])
# use the same inputs as existing_transaction
request_data = {
'lms_user_id': STATIC_LMS_USER_ID,
'content_key': self.content_key_1,
'subsidy_access_policy_uuid': self.subsidy_access_policy_1_uuid,
'idempotency_key': 'my-idempotency-key',
'requested_price_cents': requested_price_cents,
'metadata': {
'foo': 'bar',
},
}

response = self.client.post(url, request_data)

self.assertEqual(response.status_code, status.HTTP_422_UNPROCESSABLE_ENTITY)
expected_error_detail = [
f'Requested price {requested_price_cents} for {self.content_key_1} outside of '
f'acceptable interval on canonical course price of {canonical_price_cents}.'
]
assert response.json() == {
'detail': str(expected_error_detail),
'code': ErrorCodes.INVALID_REQUESTED_PRICE,
}

def test_operator_creation_required_fields_validation_eror(self):
"""
Tests that an authenticated operator receives a 400 response
Expand All @@ -770,14 +831,19 @@ def test_operator_creation_required_fields_validation_eror(self):
self.set_up_operator()

url = reverse("api:v2:transaction-admin-list-create", args=[self.subsidy_1.uuid])
payload = {
'anything': 'goes',
'requested_price_cents': -100,
}

response = self.client.post(url, {'anything': 'goes'})
response = self.client.post(url, payload)

assert response.status_code == status.HTTP_400_BAD_REQUEST
assert response.json() == {
'content_key': ['This field is required.'],
'lms_user_id': ['This field is required.'],
'subsidy_access_policy_uuid': ['This field is required.'],
'requested_price_cents': ['Ensure this value is greater than or equal to 0.'],
}

@mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.enterprise_client")
Expand Down Expand Up @@ -832,8 +898,10 @@ def test_operator_creation_happy_path_transaction_exists(self, mock_price_for_co
@mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.price_for_content")
@mock.patch("enterprise_subsidy.apps.content_metadata.api.ContentMetadataApi.get_content_summary")
@mock.patch("enterprise_subsidy.apps.subsidy.models.Subsidy.lms_user_client")
@ddt.data(True, False)
def test_operator_creation_happy_path_201(
self,
use_requested_price,
mock_lms_user_client,
mock_get_content_summary,
mock_price_for_content,
Expand All @@ -845,18 +913,20 @@ def test_operator_creation_happy_path_201(
"""
self.set_up_operator()

canonical_price_cents = 1000
requested_price_cents = 900 # only in use if use_requested_price is True
mock_lms_user_client.return_value.best_effort_user_data.return_value = {
'email': self.lms_user_email,
}
mock_enterprise_client.enroll.return_value = 'my-fulfillment-id'
mock_price_for_content.return_value = 1000
mock_price_for_content.return_value = canonical_price_cents
mock_get_content_summary.return_value = {
'content_uuid': self.content_key_1,
'content_key': self.content_key_1,
'content_title': self.content_title_1,
'source': 'edX',
'mode': 'verified',
'content_price': 10000,
'content_price': canonical_price_cents,
'geag_variant_id': None,
}
url = reverse("api:v2:transaction-admin-list-create", args=[self.subsidy_1.uuid])
Expand All @@ -870,6 +940,8 @@ def test_operator_creation_happy_path_201(
'foo': 'bar',
},
}
if use_requested_price:
request_data['requested_price_cents'] = requested_price_cents

response = self.client.post(url, request_data)

Expand All @@ -891,7 +963,8 @@ def test_operator_creation_happy_path_201(
assert response_data["subsidy_access_policy_uuid"] == request_data["subsidy_access_policy_uuid"]
assert response_data["metadata"] == {'foo': 'bar'}
assert response_data["unit"] == self.subsidy_1.ledger.unit
assert response_data["quantity"] == -1000
assert response_data["fulfillment_identifier"] == 'my-fulfillment-id'
assert response_data["reversal"] is None
assert response_data["state"] == TransactionStateChoices.COMMITTED
expected_quantity = -1 * (requested_price_cents if use_requested_price else canonical_price_cents)
assert response_data["quantity"] == expected_quantity
21 changes: 19 additions & 2 deletions enterprise_subsidy/apps/api/v2/views/transaction.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@
PERMISSION_CAN_READ_ALL_TRANSACTIONS,
PERMISSION_CAN_READ_TRANSACTIONS
)
from enterprise_subsidy.apps.subsidy.models import ContentNotFoundForCustomerException, Subsidy
from enterprise_subsidy.apps.subsidy.models import ContentNotFoundForCustomerException, PriceValidationError, Subsidy

logger = logging.getLogger(__name__)

Expand Down Expand Up @@ -156,8 +156,20 @@ def list(self, request, subsidy_uuid):
def post(self, *args, **kwargs):
"""
A create view that is accessible only to operators of the system.

It creates (or just gets, if a matching Transaction is found with same ledger and idempotency_key) a
transaction via the `Subsidy.redeem()` method.
transaction via the `Subsidy.redeem()` method. Normally, the logic of this view
is responsible for determining the price of the requested content key, with which
the redeemed transaction's quantity will be valued.

Note that, under some circumstances (for example, assigned learner content), it is
appropriate and allowable for the *caller* of this view to request a specific price
at which a redeemed transaction should occur. In these circumstances, this service
still does some validation of the requested price to ensure that it falls within
a reasonable interval around the *true* price of the related content key. See:

https://github.com/openedx/enterprise-access/blob/main/docs/decisions/0012-assignment-based-policies.rst
https://github.com/openedx/enterprise-access/blob/main/docs/decisions/0014-assignment-price-validation.rst
"""
return super().post(*args, **kwargs)

Expand Down Expand Up @@ -191,6 +203,11 @@ def create(self, request, subsidy_uuid):
detail=str(exc),
code=ErrorCodes.CONTENT_NOT_FOUND,
)
except PriceValidationError as exc:
raise TransactionCreationAPIException(
detail=str(exc),
code=ErrorCodes.INVALID_REQUESTED_PRICE,
)
except FulfillmentException as exc:
raise TransactionCreationAPIException(
detail=str(exc),
Expand Down
58 changes: 53 additions & 5 deletions enterprise_subsidy/apps/subsidy/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
from unittest import mock
from uuid import uuid4

from django.conf import settings
from django.core.exceptions import ValidationError
from django.db import models
from django.utils.functional import cached_property
Expand Down Expand Up @@ -50,6 +51,13 @@ class ContentNotFoundForCustomerException(Exception):
"""


class PriceValidationError(ValidationError):
"""
Raised in cases related to requested prices, when the requested price
fails our validation checks.
"""


class SubsidyReferenceChoices:
"""
Enumerate different choices for the type of object that the subsidy's reference_id points to. This is the type of
Expand Down Expand Up @@ -391,7 +399,15 @@ def rollback_transaction(self, ledger_transaction):
ledger_transaction.state = TransactionStateChoices.FAILED
ledger_transaction.save()

def redeem(self, lms_user_id, content_key, subsidy_access_policy_uuid, idempotency_key=None, metadata=None):
def redeem(
self,
lms_user_id,
content_key,
subsidy_access_policy_uuid,
idempotency_key=None,
requested_price_cents=None,
metadata=None,
):
"""
Redeem this subsidy and enroll the learner.

Expand All @@ -414,7 +430,7 @@ def redeem(self, lms_user_id, content_key, subsidy_access_policy_uuid, idempoten
if existing_transaction := self.get_committed_transaction_no_reversal(lms_user_id, content_key):
return (existing_transaction, False)

is_redeemable, content_price = self.is_redeemable(content_key)
is_redeemable, content_price = self.is_redeemable(content_key, requested_price_cents)

base_exception_msg = (
f'{self} cannot redeem {content_key} with price {content_price} '
Expand All @@ -430,6 +446,7 @@ def redeem(self, lms_user_id, content_key, subsidy_access_policy_uuid, idempoten
transaction = self._create_redemption(
lms_user_id,
content_key,
content_price,
subsidy_access_policy_uuid,
lms_user_email=lms_user_email,
content_title=content_title,
Expand Down Expand Up @@ -459,6 +476,7 @@ def _create_redemption(
self,
lms_user_id,
content_key,
content_price,
subsidy_access_policy_uuid,
content_title=None,
lms_user_email=None,
Expand Down Expand Up @@ -499,7 +517,7 @@ def _create_redemption(
All other exceptions raised during the creation of an enrollment. This should have already triggered
the rollback of a pending transaction.
"""
quantity = -1 * self.price_for_content(content_key)
quantity = -1 * content_price
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IDK why we made a second call to get the price here - we already have it from calling is_redeemable() further up in the body of redeem().

if not idempotency_key:
idempotency_key = create_idempotency_key_for_transaction(
self.ledger,
Expand Down Expand Up @@ -549,7 +567,25 @@ def _create_redemption(

return ledger_transaction

def is_redeemable(self, content_key):
def validate_requested_price(self, content_key, requested_price_cents, canonical_price_cents):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

borrowed from the enterprise-access implementation

"""
Validates that the requested redemption price (in USD cents)
is within some acceptable error bound interval.
"""
if requested_price_cents < 0:
raise PriceValidationError('Can only redeem non-negative content prices in cents.')

lower_bound = settings.ALLOCATION_PRICE_VALIDATION_LOWER_BOUND_RATIO * canonical_price_cents
upper_bound = settings.ALLOCATION_PRICE_VALIDATION_UPPER_BOUND_RATIO * canonical_price_cents
if not (lower_bound <= requested_price_cents <= upper_bound):
raise PriceValidationError(
f'Requested price {requested_price_cents} for {content_key} '
f'outside of acceptable interval on canonical course price of {canonical_price_cents}.'
)

return requested_price_cents

def is_redeemable(self, content_key, requested_price_cents=None):
"""
Check if this subsidy is redeemable (by anyone) at a given time.

Expand All @@ -558,11 +594,23 @@ def is_redeemable(self, content_key):
Args:
content_key (str): content key of content we may try to redeem.
redemption_datetime (datetime.datetime): The point in time to check for redemability.
requested_price_cents (int): An optional "override" price for the given content.
If present, we'll compare this quantity against the current balance,
instead of the price read from our catalog service. An override *must*
be within some reasonable bound of the real price.

Returns:
2-tuple of (bool: True if redeemable, int: price of content)
"""
content_price = self.price_for_content(content_key)
canonical_price_cents = self.price_for_content(content_key)
content_price = canonical_price_cents
if requested_price_cents:
content_price = self.validate_requested_price(
content_key,
requested_price_cents,
canonical_price_cents,
)

redeemable = False
if content_price is not None:
redeemable = self.current_balance() >= content_price
Expand Down
Loading
Loading