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: Commerce Coordinator step in retirement pipeline #11

Closed
wants to merge 3 commits into from
Closed
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
15 changes: 15 additions & 0 deletions tubular/edx_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -517,3 +517,18 @@ def retire_learner(self, learner):
except HttpDoesNotExistException:
LOG.info("No license manager data found for user")
return True


class CommerceCoordinatorApi(BaseApiClient):
"""
Commerce-Coordinator API client.
"""
@_retry_lms_api()
def retire_learner(self, learner):
"""
Performs the learner retirement step for Commerce-Coordinator.
Passes the learner's LMS User Id instead of username.
"""
data = {'edx_lms_user_id': learner['user']['id']}
api_url = self.get_api_url('lms/user_retirement')
Copy link
Member

Choose a reason for hiding this comment

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

Do you plan to add the lms/user_retirement endpoint to the commerce-coordinator service? It currently doesn't seem to exist in the django urls.py routes: https://github.com/edx/commerce-coordinator/blob/main/commerce_coordinator/apps/lms/urls.py

Copy link
Member Author

Choose a reason for hiding this comment

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

return self._request('POST', api_url, json=data)
19 changes: 12 additions & 7 deletions tubular/scripts/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,8 @@
# Add top-level module path to sys.path before importing tubular code.
sys.path.append(path.dirname(path.dirname(path.abspath(__file__))))

from tubular.edx_api import CredentialsApi, DemographicsApi, EcommerceApi, LicenseManagerApi, \
LmsApi # pylint: disable=wrong-import-position
from tubular.edx_api import CommerceCoordinatorApi, CredentialsApi, DemographicsApi, \
EcommerceApi, LicenseManagerApi, LmsApi # pylint: disable=wrong-import-position
from tubular.braze_api import BrazeApi # pylint: disable=wrong-import-position
from tubular.segment_api import SegmentApi # pylint: disable=wrong-import-position
from tubular.salesforce_api import SalesforceApi # pylint: disable=wrong-import-position
Expand Down Expand Up @@ -142,10 +142,10 @@ def _setup_lms_api_or_exit(fail_func, fail_code, config):

def _setup_all_apis_or_exit(fail_func, fail_code, config):
"""
Performs setup of EdxRestClientApi instances for LMS, E-Commerce, Credentials, and
Demographics, as well as fetching the learner's record from LMS and validating that
it is in a state to work on. Returns the learner dict and their current stage in the
retirement flow.
Performs setup of EdxRestClientApi instances for LMS, E-Commerce, Credentials,
Demographics, and Commerce-Coordinator, as well as fetching the learner's
record from LMS and validating that it is in a state to work on. Returns the
learner dict and their current stage in the retirement flow.
"""
try:
lms_base_url = config['base_urls']['lms']
Expand All @@ -154,6 +154,7 @@ def _setup_all_apis_or_exit(fail_func, fail_code, config):
segment_base_url = config['base_urls'].get('segment', None)
demographics_base_url = config['base_urls'].get('demographics', None)
license_manager_base_url = config['base_urls'].get('license_manager', None)
commerce_coordinator_base_url = config['base_urls'].get('commerce_coordinator', None)
client_id = config['client_id']
client_secret = config['client_secret']
braze_api_key = config.get('braze_api_key', None)
Expand All @@ -180,7 +181,8 @@ def _setup_all_apis_or_exit(fail_func, fail_code, config):
('CREDENTIALS', credentials_base_url),
('SEGMENT', segment_base_url),
('HUBSPOT', hubspot_api_key),
('DEMOGRAPHICS', demographics_base_url)
('DEMOGRAPHICS', demographics_base_url),
('COMMERCE_COORDINATOR', commerce_coordinator_base_url)
):
if state[2] == service and service_url is None:
fail_func(fail_code, 'Service URL is not configured, but required for state {}'.format(state))
Expand Down Expand Up @@ -239,5 +241,8 @@ def _setup_all_apis_or_exit(fail_func, fail_code, config):
segment_auth_token,
segment_workspace_slug
)

if commerce_coordinator_base_url:
config['COMMERCE_COORDINATOR'] = CommerceCoordinatorApi(lms_base_url, commerce_coordinator_base_url , client_id, client_secret)
except Exception as exc: # pylint: disable=broad-except
fail_func(fail_code, 'Unexpected error occurred!', exc)
36 changes: 36 additions & 0 deletions tubular/tests/test_edx_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -558,3 +558,39 @@ def test_retire_learner(self, mock_method):
original_username=FAKE_ORIGINAL_USERNAME
)
)


class TestCommerceCoordinatorApi(OAuth2Mixin, unittest.TestCase):
"""
Test the edX Commerce-Coordinator API client.
"""

@responses.activate(registry=OrderedRegistry)
def setUp(self):
super().setUp()
self.mock_access_token_response()
self.lms_base_url = 'http://localhost:18000/'
self.commerce_coordinator_base_url = 'http://localhost:8140/'
self.commerce_coordinator_api = edx_api.CommerceCoordinatorApi(
self.lms_base_url,
self.commerce_coordinator_base_url,
'the_client_id',
'the_client_secret'
)

@patch.object(edx_api.CommerceCoordinatorApi, 'retire_learner')
def test_retire_learner(self, mock_method):
json_data = {
'edx_lms_user_id': get_fake_user_retirement()['user']['id']
}
responses.add(
POST,
urljoin(self.commerce_coordinator_base_url, 'lms/user_retirement'),
match=[matchers.json_params_matcher(json_data)]
)
self.commerce_coordinator_api.retire_learner(
learner=get_fake_user_retirement()
)
mock_method.assert_called_once_with(
learner=get_fake_user_retirement()
)
Comment on lines +591 to +596
Copy link
Member

@pwnage101 pwnage101 Jul 26, 2024

Choose a reason for hiding this comment

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

I know you just copy+pasted this from other tests, but LOL this is quite a thin test that doesn't actually run the retire_learner function. I'm not going to block on this, but if you feel motivated---I think we'd ideally mock the underlying self._requests call.

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay cool, I will make a note to create the new test once we've gotten this work through. Thank you!

Loading