From 63774018b01047bff1b0af8925a7c47887d56f41 Mon Sep 17 00:00:00 2001 From: hamzawaleed01 Date: Mon, 19 Feb 2024 15:54:58 +0500 Subject: [PATCH 1/3] feat: unlink canvas user if decommissioned on canvas side --- integrated_channels/canvas/client.py | 34 +++++++++++++++---- .../test_canvas/test_client.py | 30 +++++++++++++++- 2 files changed, 57 insertions(+), 7 deletions(-) diff --git a/integrated_channels/canvas/client.py b/integrated_channels/canvas/client.py index 277f9488fc..d866c24d6e 100644 --- a/integrated_channels/canvas/client.py +++ b/integrated_channels/canvas/client.py @@ -20,6 +20,7 @@ refresh_session_if_expired, stringify_and_store_api_record, ) +from enterprise.models import EnterpriseCustomerUser LOGGER = logging.getLogger(__name__) @@ -682,13 +683,34 @@ def _search_for_canvas_user_by_email(self, user_email): get_users_by_email_response = rsps.json() try: - canvas_user_id = get_users_by_email_response[0]['id'] + canvas_user_id = get_users_by_email_response[0]["id"] + return canvas_user_id except (KeyError, IndexError) as error: - raise ClientError( - "No Canvas user ID found associated with email: {}".format(user_email), - HTTPStatus.NOT_FOUND.value - ) from error - return canvas_user_id + # learner is decommissioned on Canvas side - unlink it from enterprise + try: + enterprise_customer = self.enterprise_configuration.enterprise_customer + # Unlink user from related Enterprise Customer + EnterpriseCustomerUser.objects.unlink_user( + enterprise_customer=enterprise_customer, + user_email=user_email, + ) + raise ClientError( + "No Canvas user ID found associated with email: {} - User unlinked from enterprise now".format( + user_email + ), + HTTPStatus.NOT_FOUND.value, + ) from error + except Exception as e: # pylint: disable=broad-except + LOGGER.error( + generate_formatted_log( + self.enterprise_configuration.channel_code(), + self.enterprise_configuration.enterprise_customer.uuid, + None, + None, + f"Error occurred while unlinking a Canvas learner: {user_email}. " + f"Error: {e}", + ) + ) def _get_canvas_user_courses_by_id(self, user_id): """Helper method to retrieve all courses that a Canvas user is enrolled in.""" diff --git a/tests/test_integrated_channels/test_canvas/test_client.py b/tests/test_integrated_channels/test_canvas/test_client.py index 278206286d..f3a6820f9e 100644 --- a/tests/test_integrated_channels/test_canvas/test_client.py +++ b/tests/test_integrated_channels/test_canvas/test_client.py @@ -7,10 +7,13 @@ import random import unittest from unittest import mock -from urllib.parse import urljoin +from urllib.parse import quote_plus, urljoin +from unittest.mock import patch import pytest import responses +import requests + from freezegun import freeze_time from requests.models import Response @@ -21,6 +24,7 @@ from integrated_channels.exceptions import ClientError from integrated_channels.integrated_channel.client import IntegratedChannelHealthStatus from test_utils import factories +from enterprise.models import EnterpriseCustomerUser IntegratedChannelAPIRequestLogs = apps.get_model( "integrated_channel", "IntegratedChannelAPIRequestLogs" @@ -419,6 +423,30 @@ def test_completion_level_reporting_included_in_final_grade(self): 'is_assessment_grade' ) + @responses.activate + def test_search_for_deleted_user(self): + """ + ``_search_for_canvas_user_by_email`` should handle exception for deleted users gracefully + by unlinking that user from enterprise + """ + responses.add( + responses.POST, self.oauth_url, json=self._token_response(), status=200 + ) + path = f"/api/v1/accounts/{self.account_id}/users" + query_params = f'?search_term={quote_plus("test@test.com")}' # emails with unique symbols such as `+` cause issues + get_user_id_from_email_url = urljoin(self.url_base, path + query_params) + responses.add(responses.GET, get_user_id_from_email_url, json=[], status=200) + canvas_api_client = CanvasAPIClient(self.enterprise_config) + canvas_api_client._create_session() # pylint: disable=protected-access + assert responses.calls[0].request.url == self.oauth_url + + with mock.patch.object( + EnterpriseCustomerUser.objects, "unlink_user" + ) as unlink_user_mock: + canvas_api_client._search_for_canvas_user_by_email(self.canvas_email) + unlink_user_mock.assert_called_once() + assert len(responses.calls) == 2 + def test_create_client_session_with_oauth_access_key(self): """ Test instantiating the client will fetch and set the session's oauth access key""" with responses.RequestsMock() as rsps: From 873ecb27c608fa21f6b8094a6645937e9400d5a4 Mon Sep 17 00:00:00 2001 From: hamzawaleed01 Date: Tue, 20 Feb 2024 12:47:00 +0500 Subject: [PATCH 2/3] chore: remove unused imports --- integrated_channels/canvas/client.py | 4 ++-- .../test_canvas/test_client.py | 13 ++++++------- 2 files changed, 8 insertions(+), 9 deletions(-) diff --git a/integrated_channels/canvas/client.py b/integrated_channels/canvas/client.py index d866c24d6e..e8083a9a2f 100644 --- a/integrated_channels/canvas/client.py +++ b/integrated_channels/canvas/client.py @@ -12,6 +12,7 @@ from django.apps import apps +from enterprise.models import EnterpriseCustomerUser from integrated_channels.canvas.utils import CanvasUtil # pylint: disable=cyclic-import from integrated_channels.exceptions import ClientError from integrated_channels.integrated_channel.client import IntegratedChannelApiClient, IntegratedChannelHealthStatus @@ -20,7 +21,6 @@ refresh_session_if_expired, stringify_and_store_api_record, ) -from enterprise.models import EnterpriseCustomerUser LOGGER = logging.getLogger(__name__) @@ -651,7 +651,7 @@ def _extract_integration_id(self, data): return integration_id - def _search_for_canvas_user_by_email(self, user_email): + def _search_for_canvas_user_by_email(self, user_email): # pylint: disable=inconsistent-return-statements """ Helper method to make an api call to Canvas using the user's email as a search term. diff --git a/tests/test_integrated_channels/test_canvas/test_client.py b/tests/test_integrated_channels/test_canvas/test_client.py index f3a6820f9e..ca2dc912af 100644 --- a/tests/test_integrated_channels/test_canvas/test_client.py +++ b/tests/test_integrated_channels/test_canvas/test_client.py @@ -8,23 +8,20 @@ import unittest from unittest import mock from urllib.parse import quote_plus, urljoin -from unittest.mock import patch import pytest import responses -import requests - from freezegun import freeze_time from requests.models import Response from django.apps import apps +from enterprise.models import EnterpriseCustomerUser from integrated_channels.canvas.client import MESSAGE_WHEN_COURSE_WAS_DELETED, CanvasAPIClient from integrated_channels.canvas.utils import CanvasUtil from integrated_channels.exceptions import ClientError from integrated_channels.integrated_channel.client import IntegratedChannelHealthStatus from test_utils import factories -from enterprise.models import EnterpriseCustomerUser IntegratedChannelAPIRequestLogs = apps.get_model( "integrated_channel", "IntegratedChannelAPIRequestLogs" @@ -433,8 +430,10 @@ def test_search_for_deleted_user(self): responses.POST, self.oauth_url, json=self._token_response(), status=200 ) path = f"/api/v1/accounts/{self.account_id}/users" - query_params = f'?search_term={quote_plus("test@test.com")}' # emails with unique symbols such as `+` cause issues - get_user_id_from_email_url = urljoin(self.url_base, path + query_params) + # emails with unique symbols such as `+` cause issues + query_params = f'?search_term={quote_plus("test@test.com")}' + get_user_id_from_email_url = urljoin( + self.url_base, path + query_params) responses.add(responses.GET, get_user_id_from_email_url, json=[], status=200) canvas_api_client = CanvasAPIClient(self.enterprise_config) canvas_api_client._create_session() # pylint: disable=protected-access @@ -443,7 +442,7 @@ def test_search_for_deleted_user(self): with mock.patch.object( EnterpriseCustomerUser.objects, "unlink_user" ) as unlink_user_mock: - canvas_api_client._search_for_canvas_user_by_email(self.canvas_email) + canvas_api_client._search_for_canvas_user_by_email(self.canvas_email) # pylint: disable=protected-access unlink_user_mock.assert_called_once() assert len(responses.calls) == 2 From d7f0cfe13081233bbf5af561590f1574f0cfcb4b Mon Sep 17 00:00:00 2001 From: hamzawaleed01 Date: Tue, 20 Feb 2024 14:23:05 +0500 Subject: [PATCH 3/3] refactor: merge new test with existing test --- .../test_canvas/test_client.py | 74 +++++-------------- 1 file changed, 18 insertions(+), 56 deletions(-) diff --git a/tests/test_integrated_channels/test_canvas/test_client.py b/tests/test_integrated_channels/test_canvas/test_client.py index ca2dc912af..45ad649df3 100644 --- a/tests/test_integrated_channels/test_canvas/test_client.py +++ b/tests/test_integrated_channels/test_canvas/test_client.py @@ -7,7 +7,7 @@ import random import unittest from unittest import mock -from urllib.parse import quote_plus, urljoin +from urllib.parse import urljoin import pytest import responses @@ -162,39 +162,27 @@ def test_expires_at_is_updated_after_session_expiry(self): canvas_api_client._create_session() # pylint: disable=protected-access assert canvas_api_client.expires_at > orig_expires_at + @responses.activate def test_search_for_canvas_user_with_400(self): """ - Test that we properly raise exceptions if the client can't find the edx user in Canvas while reporting - grades (assessment and course level reporting both use the same method of retrieval). + Test that we properly raise exception and unlink user if the client can't find the edx user in Canvas + while reporting grades (assessment and course level reporting both use the same method of retrieval). """ - with responses.RequestsMock() as rsps: - rsps.add( - responses.GET, - self.canvas_users_url, - body="[]", - status=200 - ) - canvas_api_client = CanvasAPIClient(self.enterprise_config) - - # Searching for canvas users will require the session to be created - rsps.add( - responses.POST, - self.oauth_url, - json=self._token_response(), - status=200 - ) - canvas_api_client._create_session() # pylint: disable=protected-access + responses.add( + responses.POST, self.oauth_url, json=self._token_response(), status=200 + ) + responses.add(responses.GET, self.canvas_users_url, json=[], status=200) + canvas_api_client = CanvasAPIClient(self.enterprise_config) + canvas_api_client._create_session() # pylint: disable=protected-access + assert responses.calls[0].request.url == self.oauth_url - with pytest.raises(ClientError) as client_error: - canvas_api_client._search_for_canvas_user_by_email(self.canvas_email) # pylint: disable=protected-access - assert IntegratedChannelAPIRequestLogs.objects.count() == 2 - assert client_error.value.message == \ - "Course: {course_id} not found registered in Canvas for Edx " \ - "learner: {canvas_email}/Canvas learner: {canvas_user_id}.".format( - course_id=self.course_id, - canvas_email=self.canvas_email, - canvas_user_id=self.canvas_user_id - ) + with mock.patch.object( + EnterpriseCustomerUser.objects, "unlink_user" + ) as unlink_user_mock: + canvas_api_client._search_for_canvas_user_by_email(self.canvas_email) # pylint: disable=protected-access + unlink_user_mock.assert_called_once() + assert len(responses.calls) == 2 + assert IntegratedChannelAPIRequestLogs.objects.count() == 2 def test_assessment_reporting_with_no_canvas_course_found(self): """ @@ -420,32 +408,6 @@ def test_completion_level_reporting_included_in_final_grade(self): 'is_assessment_grade' ) - @responses.activate - def test_search_for_deleted_user(self): - """ - ``_search_for_canvas_user_by_email`` should handle exception for deleted users gracefully - by unlinking that user from enterprise - """ - responses.add( - responses.POST, self.oauth_url, json=self._token_response(), status=200 - ) - path = f"/api/v1/accounts/{self.account_id}/users" - # emails with unique symbols such as `+` cause issues - query_params = f'?search_term={quote_plus("test@test.com")}' - get_user_id_from_email_url = urljoin( - self.url_base, path + query_params) - responses.add(responses.GET, get_user_id_from_email_url, json=[], status=200) - canvas_api_client = CanvasAPIClient(self.enterprise_config) - canvas_api_client._create_session() # pylint: disable=protected-access - assert responses.calls[0].request.url == self.oauth_url - - with mock.patch.object( - EnterpriseCustomerUser.objects, "unlink_user" - ) as unlink_user_mock: - canvas_api_client._search_for_canvas_user_by_email(self.canvas_email) # pylint: disable=protected-access - unlink_user_mock.assert_called_once() - assert len(responses.calls) == 2 - def test_create_client_session_with_oauth_access_key(self): """ Test instantiating the client will fetch and set the session's oauth access key""" with responses.RequestsMock() as rsps: