From 87494c8fd0374f0ee9ed8d0cb176d7ade3f02772 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Fri, 1 Sep 2023 17:39:47 +0530 Subject: [PATCH 1/6] feat: adds support for OAuth 2 client credentials auth in xapi The xAPI client currently only supports sending events using the HTTP Basic authentication. This commit expands the authentication method to also include OAuth 2.0 Client Credentials flow. --- .../0008_adds_auth_method_scope_and_url.py | 28 +++++++++ integrated_channels/xapi/models.py | 62 +++++++++++++++++++ test_utils/factories.py | 5 +- .../test_xapi/test_models.py | 55 ++++++++++++++++ 4 files changed, 149 insertions(+), 1 deletion(-) create mode 100644 integrated_channels/xapi/migrations/0008_adds_auth_method_scope_and_url.py diff --git a/integrated_channels/xapi/migrations/0008_adds_auth_method_scope_and_url.py b/integrated_channels/xapi/migrations/0008_adds_auth_method_scope_and_url.py new file mode 100644 index 0000000000..a13becf0dc --- /dev/null +++ b/integrated_channels/xapi/migrations/0008_adds_auth_method_scope_and_url.py @@ -0,0 +1,28 @@ +# Generated by Django 3.2.12 on 2023-09-01 07:26 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('xapi', '0007_auto_20220405_2311'), + ] + + operations = [ + migrations.AddField( + model_name='xapilrsconfiguration', + name='auth_method', + field=models.CharField(choices=[('BASIC', 'HTTP Basic'), ('OAUTH2_CC', 'OAuth 2.0 Client Credentials')], default='BASIC', help_text='The Authentication Method to use when sending the xAPI data to the endpoint.', max_length=16, verbose_name='xAPI POST Authentication Method'), + ), + migrations.AddField( + model_name='xapilrsconfiguration', + name='auth_url', + field=models.URLField(blank=True, help_text='URL to use for authentication. Eg., Token URL for OAuth', null=True), + ), + migrations.AddField( + model_name='xapilrsconfiguration', + name='oauth_scope', + field=models.CharField(blank=True, help_text='The "scope" to pass for OAuth authentication.', max_length=255, null=True, verbose_name='OAuth scope'), + ), + ] diff --git a/integrated_channels/xapi/models.py b/integrated_channels/xapi/models.py index 7275d88b2f..9e8b17541e 100644 --- a/integrated_channels/xapi/models.py +++ b/integrated_channels/xapi/models.py @@ -4,7 +4,12 @@ import base64 +import requests + +from functools import cached_property + from django.contrib import auth +from django.core.exceptions import ValidationError from django.db import models from django.utils.translation import gettext_lazy as _ @@ -16,6 +21,11 @@ User = auth.get_user_model() +class XAPIAuthMethods(models.TextChoices): + HTTP_BASIC = 'BASIC', _('HTTP Basic') + OAUTH2_CLIENT_CREDS = 'OAUTH2_CC', _('OAuth 2.0 Client Credentials') + + class XAPILRSConfiguration(TimeStampedModel): """ xAPI LRS configurations. @@ -39,6 +49,25 @@ class XAPILRSConfiguration(TimeStampedModel): null=False, help_text=_('Is this configuration active?'), ) + auth_method = models.CharField( + max_length=16, + verbose_name="xAPI POST Authentication Method", + choices=XAPIAuthMethods.choices, + default=XAPIAuthMethods.HTTP_BASIC, + help_text=_('The Authentication Method to use when sending the xAPI data to the endpoint.') + ) + auth_url = models.URLField( + blank=True, + null=True, + help_text=_("URL to use for authentication. Eg., Token URL for OAuth") + ) + oauth_scope = models.CharField( + max_length=255, + verbose_name=_('OAuth scope'), + blank=True, + null=True, + help_text=_('The "scope" to pass for OAuth authentication.') + ) class Meta: app_label = 'xapi' @@ -62,10 +91,43 @@ def authorization_header(self): """ Authorization header for authenticating requests to LRS. """ + if self.auth_method == XAPIAuthMethods.OAUTH2_CLIENT_CREDS: + return f'Bearer {self.access_token}' + return 'Basic {}'.format( base64.b64encode('{key}:{secret}'.format(key=self.key, secret=self.secret).encode()).decode() ) + def clean(self): + errors = {} + # Don't allow OAuth2 Client Credentials method to be set without auth_url, or oauth_scope + if self.auth_method == XAPIAuthMethods.OAUTH2_CLIENT_CREDS: + if not self.auth_url: + errors['auth_url'] = _("Authentication URL is required for the OAuth2 authentication method.") + if not self.oauth_scope: + errors['oauth_scope'] = _("OAuth scope is required for the OAuth2 authentication method.") + + if errors: + raise ValidationError(errors) + + @cached_property + def access_token(self): + """ + Gets the access token from the OAuth2 Authentication endpoint return it. + """ + if self.auth_method != XAPIAuthMethods.OAUTH2_CLIENT_CREDS: + raise RuntimeError("Access Token can be fetched only for OAuth2 Client Credentials authenication method.") + + data = { + "grant_type": "client_credentials", + "scope": self.oauth_scope, + "client_id": self.key, + "client_secret": self.secret + } + response = requests.post(self.auth_url, data=data) + response.raise_for_status() + return response.json()["access_token"] + class XAPILearnerDataTransmissionAudit(LearnerDataTransmissionAudit): """ diff --git a/test_utils/factories.py b/test_utils/factories.py index a883ab948b..92fa1e3876 100644 --- a/test_utils/factories.py +++ b/test_utils/factories.py @@ -60,7 +60,7 @@ SAPSuccessFactorsGlobalConfiguration, SapSuccessFactorsLearnerDataTransmissionAudit, ) -from integrated_channels.xapi.models import XAPILearnerDataTransmissionAudit, XAPILRSConfiguration +from integrated_channels.xapi.models import XAPIAuthMethods, XAPILearnerDataTransmissionAudit, XAPILRSConfiguration FAKER = FakerFactory.create() User = auth.get_user_model() @@ -786,6 +786,9 @@ class Meta: key = factory.LazyAttribute(lambda x: FAKER.slug()) secret = factory.LazyAttribute(lambda x: FAKER.uuid4()) active = True + auth_method = XAPIAuthMethods.HTTP_BASIC + auth_url = factory.LazyAttribute(lambda x: FAKER.url()) + oauth_scope = factory.LazyAttribute(lambda x: FAKER.slug()) class XAPILearnerDataTransmissionAuditFactory(factory.django.DjangoModelFactory): diff --git a/tests/test_integrated_channels/test_xapi/test_models.py b/tests/test_integrated_channels/test_xapi/test_models.py index 3496bb9b61..ce8fd51c99 100644 --- a/tests/test_integrated_channels/test_xapi/test_models.py +++ b/tests/test_integrated_channels/test_xapi/test_models.py @@ -3,9 +3,15 @@ """ import base64 +import json import unittest +import responses + from pytest import mark +from django.core.exceptions import ValidationError + +from integrated_channels.xapi.models import XAPIAuthMethods, XAPILRSConfiguration from test_utils import factories @@ -19,6 +25,9 @@ class TestXAPILRSConfiguration(unittest.TestCase): def setUp(self): super().setUp() self.x_api_lrs_config = factories.XAPILRSConfigurationFactory() + self.x_api_oauth_lrs_config = factories.XAPILRSConfigurationFactory( + auth_method=XAPIAuthMethods.OAUTH2_CLIENT_CREDS + ) def test_string_representation(self): """ @@ -41,6 +50,52 @@ def test_authorization_header(self): ) assert expected_header == self.x_api_lrs_config.authorization_header + with responses.RequestsMock() as rsps: + rsps.add( + responses.POST, + self.x_api_oauth_lrs_config.auth_url, + body=json.dumps({ + 'access_token': 'test_token', + 'token_type': 'bearer', + 'expires_in': 3600 + }), + status=200, + content_type="application/json" + ) + expected_header = 'Bearer test_token' + + assert expected_header == self.x_api_oauth_lrs_config.authorization_header + + def test_auth_url_and_scope_are_required_oauth2_auth_method(self): + """ + Test that a validation error is raised when the auth_url or scope is not set for auth method OAUTH2_CC. + """ + conf = XAPILRSConfiguration( + enterprise_customer=factories.EnterpriseCustomerFactory(), + endpoint="https://xapi.endpoint", + key="key", + secret="secret", + active=True, + auth_method="OAUTH2_CC", + ) + with self.assertRaises(ValidationError) as context: + conf.full_clean() + self.assertIn('auth_url', context.exception.message) + self.assertIn('oauth_scope', context.exception.message) + + conf.auth_url = "https://auth.url" + + with self.assertRaises(ValidationError) as context: + conf.full_clean() + self.assertIn('oauth_scope', context.exception.message) + + conf.auth_url = None + conf.oauth_scope = "xapi:write" + + with self.assertRaises(ValidationError) as context: + conf.full_clean() + self.assertIn('auth_url', context.exception.message) + @mark.django_db class TestXAPILearnerDataTransmissionAudit(unittest.TestCase): From 29fbf5eab574ce62edec0e066ec2ca8f4cb4d975 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Fri, 1 Sep 2023 17:56:47 +0530 Subject: [PATCH 2/6] fix: consider all 2xx HTTP codes as successful xapi statement --- integrated_channels/xapi/utils.py | 2 +- tests/test_integrated_channels/test_xapi/test_utils.py | 6 ++++++ 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/integrated_channels/xapi/utils.py b/integrated_channels/xapi/utils.py index d2af0ad551..f5b63d1f2f 100644 --- a/integrated_channels/xapi/utils.py +++ b/integrated_channels/xapi/utils.py @@ -147,6 +147,6 @@ def is_success_response(response_fields): Returns: Boolean """ success_response = False - if response_fields['status'] == 200: + if 200 <= response_fields['status'] <= 299: success_response = True return success_response diff --git a/tests/test_integrated_channels/test_xapi/test_utils.py b/tests/test_integrated_channels/test_xapi/test_utils.py index d25c25c6b4..36f82c5814 100644 --- a/tests/test_integrated_channels/test_xapi/test_utils.py +++ b/tests/test_integrated_channels/test_xapi/test_utils.py @@ -151,6 +151,12 @@ def test_is_success_response(self): response_fields = {'status': 200, 'error_message': None} self.assertTrue(is_success_response(response_fields)) + response_fields = {'status': 201, 'error_message': None} + self.assertTrue(is_success_response(response_fields)) + + response_fields = {'status': 202, 'error_message': None} + self.assertTrue(is_success_response(response_fields)) + response_fields = {'status': 400, 'error_message': None} self.assertFalse(is_success_response(response_fields)) From b7775b468804dc824a05657b5ace949421424b9a Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Fri, 1 Sep 2023 18:26:01 +0530 Subject: [PATCH 3/6] fix: quality issues --- integrated_channels/xapi/models.py | 7 +++---- tests/test_integrated_channels/test_xapi/test_models.py | 3 +-- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/integrated_channels/xapi/models.py b/integrated_channels/xapi/models.py index 9e8b17541e..edd7dea98d 100644 --- a/integrated_channels/xapi/models.py +++ b/integrated_channels/xapi/models.py @@ -3,11 +3,10 @@ """ import base64 +from functools import cached_property import requests -from functools import cached_property - from django.contrib import auth from django.core.exceptions import ValidationError from django.db import models @@ -50,8 +49,8 @@ class XAPILRSConfiguration(TimeStampedModel): help_text=_('Is this configuration active?'), ) auth_method = models.CharField( - max_length=16, - verbose_name="xAPI POST Authentication Method", + max_length=16, + verbose_name="xAPI POST Authentication Method", choices=XAPIAuthMethods.choices, default=XAPIAuthMethods.HTTP_BASIC, help_text=_('The Authentication Method to use when sending the xAPI data to the endpoint.') diff --git a/tests/test_integrated_channels/test_xapi/test_models.py b/tests/test_integrated_channels/test_xapi/test_models.py index ce8fd51c99..842ffc32df 100644 --- a/tests/test_integrated_channels/test_xapi/test_models.py +++ b/tests/test_integrated_channels/test_xapi/test_models.py @@ -7,12 +7,11 @@ import unittest import responses - from pytest import mark + from django.core.exceptions import ValidationError from integrated_channels.xapi.models import XAPIAuthMethods, XAPILRSConfiguration - from test_utils import factories From c9872eeef16c0345d47b6e1664f93a2ce9d63d77 Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Wed, 13 Sep 2023 16:47:20 +0530 Subject: [PATCH 4/6] feat: adds ability to send plain course, courserun ids in xapi --- integrated_channels/xapi/admin/__init__.py | 4 +++ integrated_channels/xapi/constants.py | 4 +++ ...se_id_in_statements_flag_to_xapi_config.py | 18 +++++++++++ integrated_channels/xapi/models.py | 7 +++++ integrated_channels/xapi/statements/base.py | 18 ++++++----- .../statements/learner_course_completion.py | 7 +++-- .../statements/learner_course_enrollment.py | 7 +++-- integrated_channels/xapi/utils.py | 3 ++ .../test_statements/test_course_completion.py | 30 ++++++++++++++++++- .../test_learner_course_enrollment.py | 28 ++++++++++++++++- 10 files changed, 111 insertions(+), 15 deletions(-) create mode 100644 integrated_channels/xapi/migrations/0009_adds_plain_course_id_in_statements_flag_to_xapi_config.py diff --git a/integrated_channels/xapi/admin/__init__.py b/integrated_channels/xapi/admin/__init__.py index d3c3d551fa..6ba204fefd 100644 --- a/integrated_channels/xapi/admin/__init__.py +++ b/integrated_channels/xapi/admin/__init__.py @@ -19,6 +19,10 @@ class XAPILRSConfigurationAdmin(admin.ModelAdmin): 'version', 'key', 'secret', + 'auth_method', + 'auth_url', + 'oauth_scope', + 'plain_course_id_in_statements' ) list_display = ( diff --git a/integrated_channels/xapi/constants.py b/integrated_channels/xapi/constants.py index 9e49e81dfc..d63d43caa6 100644 --- a/integrated_channels/xapi/constants.py +++ b/integrated_channels/xapi/constants.py @@ -12,3 +12,7 @@ # Constants for edX grades MIN_SCORE = 0 MAX_SCORE = 100 + +# Constants for xAPI Object ID types +OBJECT_ID_URI = 'uri' +OBJECT_ID_PLAIN = 'plain' diff --git a/integrated_channels/xapi/migrations/0009_adds_plain_course_id_in_statements_flag_to_xapi_config.py b/integrated_channels/xapi/migrations/0009_adds_plain_course_id_in_statements_flag_to_xapi_config.py new file mode 100644 index 0000000000..16fbc01677 --- /dev/null +++ b/integrated_channels/xapi/migrations/0009_adds_plain_course_id_in_statements_flag_to_xapi_config.py @@ -0,0 +1,18 @@ +# Generated by Django 3.2.12 on 2023-09-13 05:33 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('xapi', '0008_adds_auth_method_scope_and_url'), + ] + + operations = [ + migrations.AddField( + model_name='xapilrsconfiguration', + name='plain_course_id_in_statements', + field=models.BooleanField(default=False, help_text='Uses the plain course ID (eg., course-v1:X+Y+Z) instead of the URI format in xAPI statements.', verbose_name='Use plain Course ID in xAPI Statements'), + ), + ] diff --git a/integrated_channels/xapi/models.py b/integrated_channels/xapi/models.py index edd7dea98d..193f97075c 100644 --- a/integrated_channels/xapi/models.py +++ b/integrated_channels/xapi/models.py @@ -67,6 +67,13 @@ class XAPILRSConfiguration(TimeStampedModel): null=True, help_text=_('The "scope" to pass for OAuth authentication.') ) + plain_course_id_in_statements = models.BooleanField( + default=False, + blank=False, + null=False, + verbose_name=_("Use plain Course ID in xAPI Statements"), + help_text=_('Uses the plain course ID (eg., course-v1:X+Y+Z) instead of the URI format in xAPI statements.') + ) class Meta: app_label = 'xapi' diff --git a/integrated_channels/xapi/statements/base.py b/integrated_channels/xapi/statements/base.py index f2cee0b81e..61d88f8d5f 100644 --- a/integrated_channels/xapi/statements/base.py +++ b/integrated_channels/xapi/statements/base.py @@ -4,7 +4,7 @@ from tincan import Activity, ActivityDefinition, Agent, LanguageMap, Statement -from integrated_channels.xapi.constants import X_API_ACTIVITY_COURSE +from integrated_channels.xapi.constants import OBJECT_ID_PLAIN, X_API_ACTIVITY_COURSE class EnterpriseStatement(Statement): @@ -38,12 +38,13 @@ def get_actor(self, user, user_social_auth): mbox='mailto:{email}'.format(email=user.email), ) - def get_object(self, domain, course_overview, object_type): + def get_object(self, domain, course_overview, object_type, object_id_type): """ Returns the object (activity) component of the Enterprise xAPI statement. Arguments: course_overview (CourseOverview): CourseOverview. object_type (string): Object type for activity. + object_id_type (string): The format to use for the Object ID. OBJECT_ID_URI or OBJECT_ID_PLAIN """ name = (course_overview.display_name or '').encode("ascii", "ignore").decode('ascii') @@ -53,11 +54,14 @@ def get_object(self, domain, course_overview, object_type): if object_type is not None and object_type == 'course': activity_id = course_overview.course_key - xapi_activity_id = 'https://{domain}/xapi/activities/{object_type}/{activity_id}'.format( - domain=domain, - object_type=object_type, - activity_id=activity_id - ) + if object_id_type == OBJECT_ID_PLAIN: + xapi_activity_id = course_overview.id + else: + xapi_activity_id = 'https://{domain}/xapi/activities/{object_type}/{activity_id}'.format( + domain=domain, + object_type=object_type, + activity_id=activity_id + ) xapi_object_extensions = {} diff --git a/integrated_channels/xapi/statements/learner_course_completion.py b/integrated_channels/xapi/statements/learner_course_completion.py index 804d6902b1..12e5a79407 100644 --- a/integrated_channels/xapi/statements/learner_course_completion.py +++ b/integrated_channels/xapi/statements/learner_course_completion.py @@ -4,7 +4,7 @@ from tincan import LanguageMap, Result, Score, Verb -from integrated_channels.xapi.constants import MAX_SCORE, MIN_SCORE, X_API_VERB_COMPLETED +from integrated_channels.xapi.constants import MAX_SCORE, MIN_SCORE, OBJECT_ID_URI, X_API_VERB_COMPLETED from integrated_channels.xapi.statements.base import EnterpriseStatement @@ -13,7 +13,7 @@ class LearnerCourseCompletionStatement(EnterpriseStatement): xAPI Statement to serialize data related to course completion. """ - def __init__(self, site, user, user_social_auth, course_overview, course_grade, object_type, *args, **kwargs): + def __init__(self, site, user, user_social_auth, course_overview, course_grade, object_type, object_id_type=OBJECT_ID_URI, *args, **kwargs): """ Initialize and populate statement with learner info and course info. @@ -22,11 +22,12 @@ def __init__(self, site, user, user_social_auth, course_overview, course_grade, user_social_auth (UserSocialAuth): UserSocialAuth object for learner course_overview (CourseOverview): course overview object containing course details. course_grade (CourseGrade): User grade in the course. + object_id_type (string): Either OBJECT_ID_PLAIN or OBJECT_ID_URI. Defaults to OBJECT_ID_URI. """ kwargs.update( actor=self.get_actor(user, user_social_auth), verb=self.get_verb(), - object=self.get_object(site.domain, course_overview, object_type), + object=self.get_object(site.domain, course_overview, object_type, object_id_type), result=self.get_result(course_grade), ) super().__init__(*args, **kwargs) diff --git a/integrated_channels/xapi/statements/learner_course_enrollment.py b/integrated_channels/xapi/statements/learner_course_enrollment.py index 7e3254df43..ddb29d95f8 100644 --- a/integrated_channels/xapi/statements/learner_course_enrollment.py +++ b/integrated_channels/xapi/statements/learner_course_enrollment.py @@ -4,7 +4,7 @@ from tincan import LanguageMap, Verb -from integrated_channels.xapi.constants import X_API_VERB_REGISTERED +from integrated_channels.xapi.constants import OBJECT_ID_URI, X_API_VERB_REGISTERED from integrated_channels.xapi.statements.base import EnterpriseStatement @@ -13,7 +13,7 @@ class LearnerCourseEnrollmentStatement(EnterpriseStatement): xAPI statement to serialize data related to course registration. """ - def __init__(self, site, user, user_social_auth, course_overview, object_type, *args, **kwargs): + def __init__(self, site, user, user_social_auth, course_overview, object_type, object_id_type=OBJECT_ID_URI, *args, **kwargs): """ Initialize and populate statement with learner info and course info. @@ -22,11 +22,12 @@ def __init__(self, site, user, user_social_auth, course_overview, object_type, * user_social_auth (UserSocialAuth): UserSocialAuth object of learner for the enterprise if learner has a linked third party auth account course_overview (CourseOverview): course overview object containing course details. + object_id_type (string): Either OBJECT_ID_PLAIN or OBJECT_ID_URI. Defaults to OBJECT_ID_URI. """ kwargs.update( actor=self.get_actor(user, user_social_auth), verb=self.get_verb(), - object=self.get_object(site.domain, course_overview, object_type) + object=self.get_object(site.domain, course_overview, object_type, object_id_type) ) super().__init__(*args, **kwargs) diff --git a/integrated_channels/xapi/utils.py b/integrated_channels/xapi/utils.py index f5b63d1f2f..8fbfe1821d 100644 --- a/integrated_channels/xapi/utils.py +++ b/integrated_channels/xapi/utils.py @@ -7,6 +7,7 @@ from enterprise.tpa_pipeline import get_user_social_auth from integrated_channels.exceptions import ClientError from integrated_channels.xapi.client import EnterpriseXAPIClient +from integrated_channels.xapi.constants import OBJECT_ID_PLAIN, OBJECT_ID_URI from integrated_channels.xapi.statements.learner_course_completion import LearnerCourseCompletionStatement from integrated_channels.xapi.statements.learner_course_enrollment import LearnerCourseEnrollmentStatement @@ -85,6 +86,7 @@ def send_course_enrollment_statement(lrs_configuration, user, course_overview, o user_social_auth, course_overview, object_type, + OBJECT_ID_PLAIN if lrs_configuration.plain_course_id_in_statements else OBJECT_ID_URI ) response_fields = _send_statement( @@ -124,6 +126,7 @@ def send_course_completion_statement(lrs_configuration, course_overview, course_grade, object_type, + OBJECT_ID_PLAIN if lrs_configuration.plain_course_id_in_statements else OBJECT_ID_URI ) response_fields = _send_statement( diff --git a/tests/test_integrated_channels/test_xapi/test_statements/test_course_completion.py b/tests/test_integrated_channels/test_xapi/test_statements/test_course_completion.py index f643e41706..180d3bf26f 100644 --- a/tests/test_integrated_channels/test_xapi/test_statements/test_course_completion.py +++ b/tests/test_integrated_channels/test_xapi/test_statements/test_course_completion.py @@ -9,7 +9,7 @@ from faker import Factory as FakerFactory from pytest import mark -from integrated_channels.xapi.constants import X_API_ACTIVITY_COURSE, X_API_VERB_COMPLETED +from integrated_channels.xapi.constants import OBJECT_ID_PLAIN, X_API_ACTIVITY_COURSE, X_API_VERB_COMPLETED from integrated_channels.xapi.statements.learner_course_completion import LearnerCourseCompletionStatement from test_utils import factories @@ -41,6 +41,7 @@ def setUp(self): self.object_id_course = 'https://{domain}/xapi/activities/course/{activity_id}'.format( domain=self.site.domain, activity_id=self.course_overview.course_key) + self.plain_course_id = self.course_overview.id self.object_id_courserun = 'https://{domain}/xapi/activities/courserun/{activity_id}'.format( domain=self.site.domain, @@ -181,3 +182,30 @@ def test_statement_notpassed(self): 'course' ) self.assertDictEqual(json.loads(statement.to_json()), self.expected_notpassed) + + def test_statement_with_different_course_id_formats(self): + """ + Validate statement contains Object IDs in the specified format. + """ + statement = LearnerCourseCompletionStatement( + self.site, + self.user, + self.mock_social_auth, + self.course_overview, + self.course_grade, + 'course' + ) # defaults to URI format + + self.assertEqual(json.loads(statement.to_json())["object"]["id"], self.object_id_course) + + statement = LearnerCourseCompletionStatement( + self.site, + self.user, + self.mock_social_auth, + self.course_overview, + self.course_grade, + 'course', + OBJECT_ID_PLAIN + ) + + self.assertEqual(json.loads(statement.to_json())["object"]["id"], self.plain_course_id) diff --git a/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py b/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py index ff50654ff0..13408e527b 100644 --- a/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py +++ b/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py @@ -9,7 +9,7 @@ from faker import Factory as FakerFactory from pytest import mark -from integrated_channels.xapi.constants import X_API_ACTIVITY_COURSE, X_API_VERB_REGISTERED +from integrated_channels.xapi.constants import OBJECT_ID_PLAIN, X_API_ACTIVITY_COURSE, X_API_VERB_REGISTERED from integrated_channels.xapi.statements.learner_course_enrollment import LearnerCourseEnrollmentStatement from test_utils import factories @@ -39,6 +39,7 @@ def setUp(self): self.object_id_course = 'https://{domain}/xapi/activities/course/{activity_id}'.format( domain=self.site.domain, activity_id=self.course_overview.course_key) + self.plain_course_id = self.course_overview.course_key self.object_id_courserun = 'https://{domain}/xapi/activities/courserun/{activity_id}'.format( domain=self.site.domain, @@ -138,3 +139,28 @@ def test_statement_courserun(self): 'courserun', ) self.assertDictEqual(json.loads(statement.to_json()), self.expected_courserun) + + def test_statement_with_different_course_id_formats(self): + """ + Validate statement contains Object IDs in the specified format. + """ + statement = LearnerCourseEnrollmentStatement( + self.site, + self.user, + self.mock_social_auth, + self.course_overview, + 'course' + ) # defaults to URI format + + self.assertEqual(json.loads(statement.to_json())["object"]["id"], self.object_id_course) + + statement = LearnerCourseEnrollmentStatement( + self.site, + self.user, + self.mock_social_auth, + self.course_overview, + 'course', + OBJECT_ID_PLAIN, + ) + + self.assertEqual(json.loads(statement.to_json())["object"]["id"], self.plain_course_id) From bc64c2ce365fc3212b13eb86c2ada345a8b1f3bf Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Thu, 14 Sep 2023 11:43:29 +0530 Subject: [PATCH 5/6] fix: failing tests in the xapi module --- .../test_xapi/test_commands/test_send_course_enrollments.py | 5 +---- .../test_statements/test_learner_course_enrollment.py | 2 +- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/tests/test_integrated_channels/test_xapi/test_commands/test_send_course_enrollments.py b/tests/test_integrated_channels/test_xapi/test_commands/test_send_course_enrollments.py index 2e339480c5..a02be9cd62 100644 --- a/tests/test_integrated_channels/test_xapi/test_commands/test_send_course_enrollments.py +++ b/tests/test_integrated_channels/test_xapi/test_commands/test_send_course_enrollments.py @@ -287,10 +287,7 @@ def test_is_already_transmitted(self): Command.is_already_transmitted(xapi_transmissions, user_id, course_id) - @mock.patch( - 'integrated_channels.xapi.utils.is_success_response', - mock.MagicMock(return_value=False) - ) + @mock.patch(MODULE_PATH + 'is_success_response', mock.MagicMock(return_value=False)) @mock.patch(MODULE_PATH + 'send_course_enrollment_statement') def test_transmit_course_enrollments_transmit_fail_skip(self, mock_send_statement): # pylint: disable=import-outside-toplevel diff --git a/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py b/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py index 13408e527b..248e548392 100644 --- a/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py +++ b/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py @@ -39,7 +39,7 @@ def setUp(self): self.object_id_course = 'https://{domain}/xapi/activities/course/{activity_id}'.format( domain=self.site.domain, activity_id=self.course_overview.course_key) - self.plain_course_id = self.course_overview.course_key + self.plain_course_id = self.course_overview.id self.object_id_courserun = 'https://{domain}/xapi/activities/courserun/{activity_id}'.format( domain=self.site.domain, From abfed9502cac334aa1a7d5217ed233a0776e459d Mon Sep 17 00:00:00 2001 From: Arunmozhi Date: Thu, 14 Sep 2023 12:27:53 +0530 Subject: [PATCH 6/6] fix: quality issues --- .../statements/learner_course_completion.py | 17 +++++++++++--- .../statements/learner_course_enrollment.py | 6 ++--- .../test_statements/test_course_completion.py | 23 +++++++++++++------ .../test_learner_course_enrollment.py | 14 ++++++++--- 4 files changed, 44 insertions(+), 16 deletions(-) diff --git a/integrated_channels/xapi/statements/learner_course_completion.py b/integrated_channels/xapi/statements/learner_course_completion.py index 12e5a79407..a4457312b0 100644 --- a/integrated_channels/xapi/statements/learner_course_completion.py +++ b/integrated_channels/xapi/statements/learner_course_completion.py @@ -4,7 +4,7 @@ from tincan import LanguageMap, Result, Score, Verb -from integrated_channels.xapi.constants import MAX_SCORE, MIN_SCORE, OBJECT_ID_URI, X_API_VERB_COMPLETED +from integrated_channels.xapi.constants import MAX_SCORE, MIN_SCORE, X_API_VERB_COMPLETED from integrated_channels.xapi.statements.base import EnterpriseStatement @@ -13,7 +13,18 @@ class LearnerCourseCompletionStatement(EnterpriseStatement): xAPI Statement to serialize data related to course completion. """ - def __init__(self, site, user, user_social_auth, course_overview, course_grade, object_type, object_id_type=OBJECT_ID_URI, *args, **kwargs): + def __init__( + self, + site, + user, + user_social_auth, + course_overview, + course_grade, + object_type, + object_id_type, + *args, + **kwargs + ): """ Initialize and populate statement with learner info and course info. @@ -22,7 +33,7 @@ def __init__(self, site, user, user_social_auth, course_overview, course_grade, user_social_auth (UserSocialAuth): UserSocialAuth object for learner course_overview (CourseOverview): course overview object containing course details. course_grade (CourseGrade): User grade in the course. - object_id_type (string): Either OBJECT_ID_PLAIN or OBJECT_ID_URI. Defaults to OBJECT_ID_URI. + object_id_type (string): Either OBJECT_ID_PLAIN or OBJECT_ID_URI. """ kwargs.update( actor=self.get_actor(user, user_social_auth), diff --git a/integrated_channels/xapi/statements/learner_course_enrollment.py b/integrated_channels/xapi/statements/learner_course_enrollment.py index ddb29d95f8..6bf2d36a4b 100644 --- a/integrated_channels/xapi/statements/learner_course_enrollment.py +++ b/integrated_channels/xapi/statements/learner_course_enrollment.py @@ -4,7 +4,7 @@ from tincan import LanguageMap, Verb -from integrated_channels.xapi.constants import OBJECT_ID_URI, X_API_VERB_REGISTERED +from integrated_channels.xapi.constants import X_API_VERB_REGISTERED from integrated_channels.xapi.statements.base import EnterpriseStatement @@ -13,7 +13,7 @@ class LearnerCourseEnrollmentStatement(EnterpriseStatement): xAPI statement to serialize data related to course registration. """ - def __init__(self, site, user, user_social_auth, course_overview, object_type, object_id_type=OBJECT_ID_URI, *args, **kwargs): + def __init__(self, site, user, user_social_auth, course_overview, object_type, object_id_type, *args, **kwargs): """ Initialize and populate statement with learner info and course info. @@ -22,7 +22,7 @@ def __init__(self, site, user, user_social_auth, course_overview, object_type, o user_social_auth (UserSocialAuth): UserSocialAuth object of learner for the enterprise if learner has a linked third party auth account course_overview (CourseOverview): course overview object containing course details. - object_id_type (string): Either OBJECT_ID_PLAIN or OBJECT_ID_URI. Defaults to OBJECT_ID_URI. + object_id_type (string): Either OBJECT_ID_PLAIN or OBJECT_ID_URI. """ kwargs.update( actor=self.get_actor(user, user_social_auth), diff --git a/tests/test_integrated_channels/test_xapi/test_statements/test_course_completion.py b/tests/test_integrated_channels/test_xapi/test_statements/test_course_completion.py index 180d3bf26f..6034fd2a1c 100644 --- a/tests/test_integrated_channels/test_xapi/test_statements/test_course_completion.py +++ b/tests/test_integrated_channels/test_xapi/test_statements/test_course_completion.py @@ -9,7 +9,12 @@ from faker import Factory as FakerFactory from pytest import mark -from integrated_channels.xapi.constants import OBJECT_ID_PLAIN, X_API_ACTIVITY_COURSE, X_API_VERB_COMPLETED +from integrated_channels.xapi.constants import ( + OBJECT_ID_PLAIN, + OBJECT_ID_URI, + X_API_ACTIVITY_COURSE, + X_API_VERB_COMPLETED, +) from integrated_channels.xapi.statements.learner_course_completion import LearnerCourseCompletionStatement from test_utils import factories @@ -151,7 +156,8 @@ def test_statement_course(self): self.mock_social_auth, self.course_overview, self.course_grade, - 'course' + 'course', + OBJECT_ID_URI, ) self.assertDictEqual(json.loads(statement.to_json()), self.expected_course) @@ -165,7 +171,8 @@ def test_statement_courserun(self): self.mock_social_auth, self.course_overview, self.course_grade, - 'courserun' + 'courserun', + OBJECT_ID_URI, ) self.assertDictEqual(json.loads(statement.to_json()), self.expected_courserun) @@ -179,7 +186,8 @@ def test_statement_notpassed(self): self.mock_social_auth, self.course_overview, self.course_grade_notpassed, - 'course' + 'course', + OBJECT_ID_URI, ) self.assertDictEqual(json.loads(statement.to_json()), self.expected_notpassed) @@ -193,8 +201,9 @@ def test_statement_with_different_course_id_formats(self): self.mock_social_auth, self.course_overview, self.course_grade, - 'course' - ) # defaults to URI format + 'course', + OBJECT_ID_URI, + ) self.assertEqual(json.loads(statement.to_json())["object"]["id"], self.object_id_course) @@ -205,7 +214,7 @@ def test_statement_with_different_course_id_formats(self): self.course_overview, self.course_grade, 'course', - OBJECT_ID_PLAIN + OBJECT_ID_PLAIN, ) self.assertEqual(json.loads(statement.to_json())["object"]["id"], self.plain_course_id) diff --git a/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py b/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py index 248e548392..608abc1056 100644 --- a/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py +++ b/tests/test_integrated_channels/test_xapi/test_statements/test_learner_course_enrollment.py @@ -9,7 +9,12 @@ from faker import Factory as FakerFactory from pytest import mark -from integrated_channels.xapi.constants import OBJECT_ID_PLAIN, X_API_ACTIVITY_COURSE, X_API_VERB_REGISTERED +from integrated_channels.xapi.constants import ( + OBJECT_ID_PLAIN, + OBJECT_ID_URI, + X_API_ACTIVITY_COURSE, + X_API_VERB_REGISTERED, +) from integrated_channels.xapi.statements.learner_course_enrollment import LearnerCourseEnrollmentStatement from test_utils import factories @@ -124,6 +129,7 @@ def test_statement_course(self): self.mock_social_auth, self.course_overview, 'course', + OBJECT_ID_URI, ) self.assertDictEqual(json.loads(statement.to_json()), self.expected_course) @@ -137,6 +143,7 @@ def test_statement_courserun(self): self.mock_social_auth, self.course_overview, 'courserun', + OBJECT_ID_URI, ) self.assertDictEqual(json.loads(statement.to_json()), self.expected_courserun) @@ -149,8 +156,9 @@ def test_statement_with_different_course_id_formats(self): self.user, self.mock_social_auth, self.course_overview, - 'course' - ) # defaults to URI format + 'course', + OBJECT_ID_URI, + ) self.assertEqual(json.loads(statement.to_json())["object"]["id"], self.object_id_course)