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: add XApiBaseEnrollmentFilter #108

Merged
merged 1 commit into from
Nov 20, 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
11 changes: 11 additions & 0 deletions eox_nelp/edxapp_wrapper/backends/course_overviews_m_v1.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
This file contains all the necessary dependencies from
https://github.com/eduNEXT/edunext-platform/tree/master/openedx/core/djangoapps/content/course_overviews
"""
from openedx.core.djangoapps.content.course_overviews.api import get_course_overviews # pylint: disable=import-error
from openedx.core.djangoapps.content.course_overviews.models import CourseOverview # pylint: disable=import-error


Expand All @@ -14,3 +15,13 @@ def get_course_overview_model():
CourseOverview model.
"""
return CourseOverview


def get_course_overviews_method():
"""Allow to get get_course_overviews method from
https://github.com/eduNEXT/edunext-platform/blob/master/openedx/core/djangoapps/content/course_overviews/api.py

Returns:
get_course_overviews method.
"""
return get_course_overviews
2 changes: 2 additions & 0 deletions eox_nelp/edxapp_wrapper/course_overviews.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
Attributes:
backend:Imported module by using the plugin settings.
CourseOverview: Wrapper CourseOverview model.
get_course_overviews: Wrapper of et_course_overviews method.
"""
from importlib import import_module

Expand All @@ -12,3 +13,4 @@
backend = import_module(settings.EOX_NELP_COURSE_OVERVIEWS_BACKEND)

CourseOverview = backend.get_course_overview_model()
get_course_overviews = backend.get_course_overviews_method()
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
"""Test backend for course overviews module."""
from django.db import models
from mock import Mock
from opaque_keys.edx.django.models import CourseKeyField

from eox_nelp.edxapp_wrapper.test_backends import create_test_model
Expand All @@ -19,3 +20,11 @@ def get_course_overview_model():
return create_test_model(
"CourseOverview", "eox_nelp", __package__, course_overview_fields
)


def get_course_overviews_method():
"""Return test function.
Returns:
Mock class.
"""
return Mock()
91 changes: 89 additions & 2 deletions eox_nelp/openedx_filters/tests/xapi/tests_filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,14 @@

Classes:
XApiActorFilterTestCase: Tests cases for XApiActorFilter filter class.
XApiBaseEnrollmentFilterTestCase: Test cases for XApiBaseEnrollmentFilter filter class.
"""
from django.contrib.auth import get_user_model
from django.test import TestCase
from tincan import Agent
from mock import patch
from tincan import Activity, ActivityDefinition, Agent, LanguageMap

from eox_nelp.openedx_filters.xapi.filters import XApiActorFilter
from eox_nelp.openedx_filters.xapi.filters import DEFAULT_LANGUAGE, XApiActorFilter, XApiBaseEnrollmentFilter

User = get_user_model()

Expand Down Expand Up @@ -68,3 +70,88 @@ def test_update_given_actor(self):

self.assertEqual(f"mailto:{self.email}", actor.mbox)
self.assertEqual(self.username, actor.name)


class XApiBaseEnrollmentFilterTestCase(TestCase):
"""Test class for XApiBaseEnrollmentFilter filter class."""

def setUp(self):
"""Setup common conditions for every test case"""
self.filter = XApiBaseEnrollmentFilter(
filter_type="event_routing_backends.processors.xapi.enrollment_events.base_enrollment.get_object",
running_pipeline=["eox_nelp.openedx_filters.xapi.filters.XApiBaseEnrollmentFilter"],
)

def test_course_id_not_found(self):
""" Test case when the course_id cannot be extracted from the Activity id.

Expected behavior:
- Returned value is the same as the given value.
"""
activity = Activity(
id="https://example.com/course/course-v1-invalid-edx+CS105+2023-T3",
)

returned_activity = self.filter.run_filter(result=activity)["result"]

self.assertEqual(activity, returned_activity)

@patch("eox_nelp.openedx_filters.xapi.filters.get_course_from_id")
def test_update_result(self, get_course_mock):
""" Test case when name and description are updated.

Expected behavior:
- Definition name has been updated.
- Definition description has been updated.
- get_course_from_id was called with the right parameter.
"""
course = {
"display_name": "new-course-name",
"language": "ar",
"short_description": "This is a short description",
}

get_course_mock.return_value = course
activity = Activity(
id="https://example.com/course/course-v1:edx+CS105+2023-T3",
definition=ActivityDefinition(
type="testing",
name=LanguageMap(en="old-course-name"),
),
)

returned_activity = self.filter.run_filter(result=activity)["result"]

self.assertEqual({course['language']: course["display_name"]}, returned_activity.definition.name)
self.assertEqual({course['language']: course["short_description"]}, returned_activity.definition.description)
get_course_mock.assert_called_once_with("course-v1:edx+CS105+2023-T3")

@patch("eox_nelp.openedx_filters.xapi.filters.get_course_from_id")
def test_invalid_language(self, get_course_mock):
""" Test case when language has not been set.

Expected behavior:
- Definition name has been updated with default language.
- Definition description has been updated with default language.
- get_course_from_id was called with the right parameter.
"""
course = {
"display_name": "new-course-name",
"language": None,
"short_description": "This is a short description",
}

get_course_mock.return_value = course
activity = Activity(
id="https://example.com/course/course-v1:edx+CS105+2023-T3",
definition=ActivityDefinition(
type="testing",
name=LanguageMap(en="old-course-name"),
),
)

returned_activity = self.filter.run_filter(result=activity)["result"]

self.assertEqual({DEFAULT_LANGUAGE: course["display_name"]}, returned_activity.definition.name)
self.assertEqual({DEFAULT_LANGUAGE: course["short_description"]}, returned_activity.definition.description)
get_course_mock.assert_called_once_with("course-v1:edx+CS105+2023-T3")
50 changes: 49 additions & 1 deletion eox_nelp/openedx_filters/xapi/filters.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,16 @@

Filters:
XApiActorFilter: Modifies the standard ACtor in order to include the name attribute.
XApiBaseEnrollmentFilter: Updates enrollment object definition.
"""
from django.contrib.auth import get_user_model
from openedx_filters import PipelineStep
from tincan import Agent
from tincan import Agent, LanguageMap

from eox_nelp.utils import extract_course_id_from_string, get_course_from_id

User = get_user_model()
DEFAULT_LANGUAGE = "en"


class XApiActorFilter(PipelineStep):
Expand Down Expand Up @@ -51,3 +55,47 @@ def run_filter(self, result): # pylint: disable=arguments-differ
return {
"result": result
}


class XApiBaseEnrollmentFilter(PipelineStep):
"""This filter is designed to modify object attributes of an enrollment event, this will add
the description field and will change the name based on the course language.

How to set:
OPEN_EDX_FILTERS_CONFIG = {
"event_routing_backends.processors.xapi.enrollment_events.base_enrollment.get_object": {
"pipeline": ["eox_nelp.openedx_filters.xapi.filters.XApiBaseEnrollmentFilter"],
"fail_silently": False,
},
}
"""

def run_filter(self, result): # pylint: disable=arguments-differ
"""Modifies name and description attributes of the activity's definition.

Arguments:
result <Activity>: Object activity for events related to enrollments.

Returns:
Activity: Modified activity.
"""
course_id = extract_course_id_from_string(result.id)

if course_id:
# Get course course data
course = get_course_from_id(course_id)
display_name = course["display_name"]
description = course["short_description"]
course_language = course["language"] or DEFAULT_LANGUAGE # Set default value if language is not found

# Create new attributes based on the course properties
definition_name = LanguageMap(**({course_language: display_name} if display_name is not None else {}))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here you are validating display_name is not None, but also is possible that course_language is empty None
Peek 2023-11-20 13-18

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@johanv26 How did you set the course language to None since that option is not available on studio ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually is a very good question.
I tried to defined here, but is not working
image

But on the other hand my course_overview record of that object is empty.
From admin is difficult to change.
Peek 2023-11-20 16-41

Myabe there is a mismatch of mongo and mysql course_overview...

description = LanguageMap(**({course_language: description} if description is not None else {}))

# Updates current result
result.definition.name = definition_name
result.definition.description = description

return {
"result": result
}
101 changes: 101 additions & 0 deletions eox_nelp/tests/test_utils.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
"""This file contains all the test for the utils.py file.

Classes:
ExtractCourseIdFromStringTestCase: Tests cases for the extract_course_id_from_string method.
GetCourseFromIdTestCase: Tests cases for the get_course_from_id method.
"""
from ddt import data, ddt
from django.test import TestCase
from mock import patch
from opaque_keys.edx.keys import CourseKey

from eox_nelp.utils import extract_course_id_from_string, get_course_from_id


@ddt
class ExtractCourseIdFromStringTestCase(TestCase):
"""Test class for the extract_course_id_from_string method."""

@data(
"this is a long string",
"hjsdgafhawsdbfhsdyafgbhjsdbf1784561553415534",
"this is a similar string course-v1:77777554a45sd4a2ad42s45d",
"https://example.com/course/course-v1edx+CS105+2023-T3"
)
def test_course_id_not_found(self, value):
""" Test that the method returns an empty string when any course_id is not found

Expected behavior:
- Returned value is an empty string
"""
result = extract_course_id_from_string(value)

self.assertEqual("", result)

@data(
["course-v1:edx+PS874+2023-T3", "this is a course-v1:edx+PS874+2023-T3/) long string"],
["course-v1:edunext+CS105+2019-P3", "hjsdgafhawsdbfhgbhjsdbf1784561553415534course-v1:edunext+CS105+2019-P3"],
["course-v1:NELC+TR675+2022-K3", "this course-v1:NELC+TR675+2022-K3/ is a similar string course-v1:77775s45d"],
["course-v1:edx+CS105+2023-T3", "https://example.com/course/course-v1:edx+CS105+2023-T3"],
)
def test_course_id_found(self, value):
""" Test that the method returns right course id.

Expected behavior:
- Returned value is the expected course id.
"""
result = extract_course_id_from_string(value[1])

self.assertEqual(value[0], result)

def test_multiple_valid_ids(self):
""" Test that the method returns the first value that matches the regular expression.

Expected behavior:
- Returned value is the expected course id.
"""
string = (
"/course-v1:edx+CS105+2023-T3/"
"/course-v1:NELC+TR675+2022-K3/"
"/course-v1:edx+PS874+2023-T3/"
"/course-v1:edunext+CS105+2019-P3/"
)

result = extract_course_id_from_string(string)

self.assertEqual("course-v1:edx+CS105+2023-T3", result)


class GetCourseFromIdTestCase(TestCase):
"""Test class for the get_course_from_id method."""

@patch("eox_nelp.utils.get_course_overviews")
def test_course_id_not_found(self, course_overviews_mock):
""" Test that the method raises the right exception when there are no courses.

Expected behavior:
- Raises ValueError exception.
- get_course_overviews method was called with the right parameter.
"""
course_id = "course-v1:edx+CS105+2023-T3"
course_overviews_mock.return_value = []

self.assertRaises(ValueError, get_course_from_id, course_id)
course_overviews_mock.assert_called_once_with([CourseKey.from_string(course_id)])

@patch("eox_nelp.utils.get_course_overviews")
def test_course_id_found(self, course_overviews_mock):
""" Test that the method returns the expected value.

Expected behavior:
- Returned value is the expected course.
- get_course_overviews method was called with the right parameter.
"""
course_id = "course-v1:edx+CS105+2023-T3"
expected_course = {"name": "test-course", "short_description": "This is a great course"}
course_overviews_mock.return_value = [expected_course]

course = get_course_from_id(course_id)

self.assertEqual(expected_course, course)
course_overviews_mock.assert_called_once_with([CourseKey.from_string(course_id)])
41 changes: 41 additions & 0 deletions eox_nelp/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,12 @@
import re
from copy import copy

from opaque_keys.edx.keys import CourseKey

from eox_nelp.edxapp_wrapper.course_overviews import get_course_overviews

NATIONAL_ID_REGEX = r"^[1-2]\d{9}$"
COURSE_ID_REGEX = r'(course-v1:[^/+]+(/|\+)[^/+]+(/|\+)[^/?]+)'
Copy link
Collaborator

Choose a reason for hiding this comment

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

Where was extracted this Regex?
It seems a little different than
https://github.com/openedx/edx-platform/blob/master/openedx/core/constants.py#L12

>>> from django.conf import settings
>>> settings.COURSE_KEY_REGEX
'(?:[^/+]+(/|\\+)[^/+]+(/|\\+)[^/?]+)'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

from that place the modification is the course-v1:



def map_instance_attributes_to_dict(instance, attributes_mapping):
Expand Down Expand Up @@ -78,3 +83,39 @@ def is_valid_national_id(national_id, raise_exception=False):
)

return check_national_id


def extract_course_id_from_string(string):
"""This return a sub-string that matches the course_ir regex

Arguments:
string: This is a string that could contains a sub-string that matches with the course_id form.

Returns:
course_id <string>: Returns course id or an empty string.
"""
matches = re.search(COURSE_ID_REGEX, string)

if matches:
return matches.group()

return ""


def get_course_from_id(course_id):
"""
Get Course object using the `course_id`.

Arguments:
course_id (str) : ID of the course

Returns:
Course
"""
course_key = CourseKey.from_string(course_id)
course_overviews = get_course_overviews([course_key])

if course_overviews:
return course_overviews[0]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Here I was testing and found like an error
Peek 2023-11-17 16-33

Copy link
Collaborator

Choose a reason for hiding this comment

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

Actually, this seem not return a list...
image

Copy link
Collaborator

@johanseto johanseto Nov 17, 2023

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@andrey-canon andrey-canon Nov 17, 2023

Choose a reason for hiding this comment

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

I have to implement the right method


raise ValueError(f"Course with id {course_id} does not exist.")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you prefer the raise of an exception instead a None return as the CourseOverview do ?
https://github.com/openedx/edx-platform/blob/master/openedx/core/djangoapps/content/course_overviews/models.py#L415

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a copy of this

Loading