Skip to content

Commit

Permalink
fix: when we fetch inter-service data in can_redeem matters for perfo…
Browse files Browse the repository at this point in the history 
…rmance
  • Loading branch information
@iloveagent57
iloveagent57 committed Jul 27, 2023
1 parent fb9e347 commit dc9fa89
Show file tree
Hide file tree
Showing 2 changed files with 72 additions and 21 deletions.
37 changes: 22 additions & 15 deletions enterprise_access/apps/subsidy_access_policy/models.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -333,15 +333,9 @@ def can_redeem(self, lms_user_id, content_key, skip_customer_user_check=False):
* second element contains a reason code if the content is not redeemable,
* third a list of any transactions representing existing redemptions (any state).
"""
content_metadata = self.get_content_metadata(content_key)
subsidy_can_redeem_payload = self.subsidy_client.can_redeem(
self.subsidy_uuid,
lms_user_id,
content_key,
)

active_subsidy = subsidy_can_redeem_payload.get('active', False)
existing_transactions = subsidy_can_redeem_payload.get('all_transactions', [])
# inactive policy
if not self.active:
return (False, REASON_POLICY_EXPIRED, [])

# learner not associated to enterprise
if not skip_customer_user_check:
Expand All @@ -352,20 +346,33 @@ def can_redeem(self, lms_user_id, content_key, skip_customer_user_check=False):
if not self.catalog_contains_content_key(content_key):
return (False, REASON_CONTENT_NOT_IN_CATALOG, [])

# Wait to fetch content metadata with a call to the enterprise-subsidy
# service until we *know* that we'll need it.
content_metadata = self.get_content_metadata(content_key)

# no content key in content metadata
if not content_metadata:
return (False, REASON_CONTENT_NOT_IN_CATALOG, existing_transactions)
return (False, REASON_CONTENT_NOT_IN_CATALOG, [])

# TODO: Add Course Upgrade/Registration Deadline Passed Error here

# inactive subsidy
# We want to wait to do these checks that might require a call
# to the enterprise-subsidy service until we *know* we'll need the data.
subsidy_can_redeem_payload = self.subsidy_client.can_redeem(
self.subsidy_uuid,
lms_user_id,
content_key,
)

# Refers to a computed property of an EnterpriseSubsidy record
# that takes into account the start/expiration dates of the subsidy record.
active_subsidy = subsidy_can_redeem_payload.get('active', False)
existing_transactions = subsidy_can_redeem_payload.get('all_transactions', [])

# inactive subsidy?
if not active_subsidy:
return (False, REASON_SUBSIDY_EXPIRED, [])

# inactive policy
if not self.active:
return (False, REASON_POLICY_EXPIRED, [])

# can_redeem false from subsidy
if not subsidy_can_redeem_payload.get('can_redeem', False):
return (False, REASON_NOT_ENOUGH_VALUE_IN_SUBSIDY, existing_transactions)
Expand Down
56 changes: 50 additions & 6 deletions enterprise_access/apps/subsidy_access_policy/tests/test_models.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,10 @@
from uuid import uuid4

import ddt
import factory
import pytest
from django.core.cache import cache as django_cache
from django.test import TestCase

from enterprise_access.apps.core.tests.factories import UserFactory
from enterprise_access.apps.subsidy_access_policy.constants import (
REASON_CONTENT_NOT_IN_CATALOG,
REASON_LEARNER_MAX_ENROLLMENTS_REACHED,
Expand Down Expand Up @@ -40,8 +38,8 @@
class SubsidyAccessPolicyTests(TestCase):
""" SubsidyAccessPolicy model tests. """

user = factory.SubFactory(UserFactory)
course_id = factory.LazyFunction(uuid4)
lms_user_id = 12345
course_id = 'course-v1:DemoX:2T2023'

def setUp(self):
"""
Expand Down Expand Up @@ -175,6 +173,8 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args):
'transactions_for_learner': {'transactions': [], 'aggregates': {}},
'transactions_for_policy': {'results': [], 'aggregates': {'total_quantity': -200}},
'expected_policy_can_redeem': (False, REASON_CONTENT_NOT_IN_CATALOG, []),
'expect_content_metadata_fetch': False,
'expect_transaction_fetch': False,
},
{
# Learner is not in the enterprise, every other check would succeed.
Expand All @@ -186,6 +186,8 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args):
'transactions_for_learner': {'transactions': [], 'aggregates': {}},
'transactions_for_policy': {'results': [], 'aggregates': {'total_quantity': -200}},
'expected_policy_can_redeem': (False, REASON_LEARNER_NOT_IN_ENTERPRISE, []),
'expect_content_metadata_fetch': False,
'expect_transaction_fetch': False,
},
{
# The subsidy is not redeemable, every other check would succeed.
Expand Down Expand Up @@ -248,6 +250,8 @@ def test_object_creation_with_policy_type_in_kwarg(self, *args):
'transactions_for_learner': {'transactions': [], 'aggregates': {}},
'transactions_for_policy': {'results': [], 'aggregates': {'total_quantity': -200}},
'expected_policy_can_redeem': (False, REASON_POLICY_EXPIRED, []),
'expect_content_metadata_fetch': False,
'expect_transaction_fetch': False,
},
{
# The subsidy is not active, every other check would succeed.
Expand All @@ -271,6 +275,8 @@ def test_learner_enrollment_cap_policy_can_redeem(
transactions_for_learner,
transactions_for_policy,
expected_policy_can_redeem,
expect_content_metadata_fetch=True,
expect_transaction_fetch=True,
):
"""
Test the can_redeem method of PerLearnerEnrollmentCapLearnerCreditAccessPolicy model
Expand All @@ -288,10 +294,25 @@ def test_learner_enrollment_cap_policy_can_redeem(
if policy_is_active:
policy_record = self.per_learner_enroll_policy

can_redeem_result = policy_record.can_redeem(self.user, self.course_id)
can_redeem_result = policy_record.can_redeem(self.lms_user_id, self.course_id)

self.assertEqual(can_redeem_result, expected_policy_can_redeem, [])

if expect_content_metadata_fetch:
# it's actually called twice
self.mock_get_content_metadata.assert_called_with(self.course_id)
else:
self.assertFalse(self.mock_get_content_metadata.called)

if expect_transaction_fetch:
self.mock_subsidy_client.can_redeem.assert_called_once_with(
policy_record.subsidy_uuid,
self.lms_user_id,
self.course_id,
)
else:
self.assertFalse(self.mock_subsidy_client.can_redeem.called)

@ddt.data(
{
# Happy path: content in catalog, learner in enterprise, subsidy has value,
Expand All @@ -315,6 +336,8 @@ def test_learner_enrollment_cap_policy_can_redeem(
'transactions_for_learner': {'transactions': [], 'aggregates': {}},
'transactions_for_policy': {'results': [], 'aggregates': {'total_quantity': -200}},
'expected_policy_can_redeem': (False, REASON_CONTENT_NOT_IN_CATALOG, []),
'expect_content_metadata_fetch': False,
'expect_transaction_fetch': False,
},
{
# Learner is not in the enterprise, every other check would succeed.
Expand All @@ -326,6 +349,8 @@ def test_learner_enrollment_cap_policy_can_redeem(
'transactions_for_learner': {'transactions': [], 'aggregates': {}},
'transactions_for_policy': {'results': [], 'aggregates': {'total_quantity': -200}},
'expected_policy_can_redeem': (False, REASON_LEARNER_NOT_IN_ENTERPRISE, []),
'expect_content_metadata_fetch': False,
'expect_transaction_fetch': False,
},
{
# The subsidy is not redeemable, every other check would succeed.
Expand Down Expand Up @@ -388,6 +413,8 @@ def test_learner_enrollment_cap_policy_can_redeem(
'transactions_for_learner': {'transactions': [], 'aggregates': {}},
'transactions_for_policy': {'results': [], 'aggregates': {'total_quantity': -200}},
'expected_policy_can_redeem': (False, REASON_POLICY_EXPIRED, []),
'expect_content_metadata_fetch': False,
'expect_transaction_fetch': False,
},
{
# The subsidy is not active, every other check would succeed.
Expand All @@ -411,6 +438,8 @@ def test_learner_spend_cap_policy_can_redeem(
transactions_for_learner,
transactions_for_policy,
expected_policy_can_redeem,
expect_content_metadata_fetch=True,
expect_transaction_fetch=True,
):
"""
Test the can_redeem method of PerLearnerSpendCapLearnerCreditAccessPolicy model
Expand All @@ -428,10 +457,25 @@ def test_learner_spend_cap_policy_can_redeem(
if policy_is_active:
policy_record = self.per_learner_spend_policy

can_redeem_result = policy_record.can_redeem(self.user, self.course_id)
can_redeem_result = policy_record.can_redeem(self.lms_user_id, self.course_id)

self.assertEqual(can_redeem_result, expected_policy_can_redeem)

if expect_content_metadata_fetch:
# it's actually called twice
self.mock_get_content_metadata.assert_called_with(self.course_id)
else:
self.assertFalse(self.mock_get_content_metadata.called)

if expect_transaction_fetch:
self.mock_subsidy_client.can_redeem.assert_called_once_with(
policy_record.subsidy_uuid,
self.lms_user_id,
self.course_id,
)
else:
self.assertFalse(self.mock_subsidy_client.can_redeem.called)

def test_acquire_lock_release_lock_no_kwargs(self):
"""
Create one hypothetical sequence consisting of three actors and two policies. Each policy should only allow one
Expand Down

0 comments on commit dc9fa89

Please sign in to comment.