feat: add XApiBaseEnrollmentFilter #108

andrey-canon · 2023-11-17T19:52:22Z

Description

This filter allows to implement the required object structure by the NELC team https://edunext.atlassian.net/browse/FUTUREX-251

Testing instructions

Use this version of event routing feat: implement XAPI openedx filters nelc/event-routing-backends#5
add the settings

        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,
            },
        }

This uses the language and the short_description studio fields, set them
Perform an enrollment or unenrollment event
Check the result, in my case I'm using aspects, so I can check that in the superset panel

Before

After

Additional information

Include anything else that will help reviewers and consumers understand the change.

Does this change depend on other changes elsewhere?
Any special concerns or limitations? For example: deprecations, migrations, security, or accessibility.
Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.

Checklist for Merge

Tested in a remote environment
Updated documentation
Rebased master/main
Squashed commits

johanseto · 2023-11-17T21:08:48Z

eox_nelp/utils.py

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


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 '(?:[^/+]+(/|\\+)[^/+]+(/|\\+)[^/?]+)'

from that place the modification is the course-v1:

johanseto · 2023-11-17T21:14:44Z

eox_nelp/utils.py

+        Course
+    """
+    course_key = CourseKey.from_string(course_id)
+    course_overviews = CourseOverview.get_from_ids([course_key])


Why if the methods is called get_course_from_id (from one id), you use a method for multiple ids instead of the method for one??

This method is a copy of this which implements this line course_overviews = get_course_overviews([course_key]) however I made a mistake and I copied the logic from the method get_course_overviews_from_ids instead of the get_course_overviews method

I copied the logic because we already have the CourseOverview model and was easier however the other logic requires a backend, so it will be easier to implement new api backend

johanseto · 2023-11-17T21:16:29Z

eox_nelp/utils.py

+    if course_overviews:
+        return course_overviews[0]
+
+    raise ValueError(f"Course with id {course_id} does not exist.")


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

This is a copy of this

johanseto · 2023-11-17T21:34:38Z

eox_nelp/utils.py

+    course_overviews = CourseOverview.get_from_ids([course_key])
+
+    if course_overviews:
+        return course_overviews[0]


Here I was testing and found like an error

Actually, this seem not return a list...

Ok yes is very weird but that method return like a dict of course_overviews:
https://github.com/openedx/edx-platform/blob/master/openedx/core/djangoapps/content/course_overviews/models.py#L438

I have to implement the right method

andrey-canon · 2023-11-18T00:04:14Z

@johanv26 I have already applied some changes and tested that by making an enrollment in the about page, could please you try again :D

johanseto

I tested again and passed last error, with course_overviews using a list of ordered_dict. But after passing that I had an error with course_language being None

johanseto · 2023-11-20T18:24:53Z

eox_nelp/openedx_filters/xapi/filters.py

+            course_language = course['language']
+
+            # Create new attributes based on the course properties
+            definition_name = LanguageMap(**({course_language: display_name} if display_name is not None else {}))


Here you are validating display_name is not None, but also is possible that course_language is empty None

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

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

But on the other hand my course_overview record of that object is empty.
From admin is difficult to change.

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

johanseto

LGTM. Is working after some changes.
feat: use default language when the course doesn't have that value

andrey-canon · 2023-11-20T21:50:55Z

Still I wonder how did you set to None the language, I couldn't do that I just considered that case, how did you create that course ?

This filter allows to implement the required object structure by the NELC team https://edunext.atlassian.net/browse/FUTUREX-251

johanseto · 2023-11-20T23:24:57Z

Still I wonder how did you set to None the language, I couldn't do that I just considered that case, how did you create that course ?

@andrey-canon I was reseaching a little and I found that seems this backend use the mysql course_overview as source.

From studio is possible to set with - and that saves in mongo with "". By the way, the mysql course_overview record is not updated. But in mongo and what we see in studio yes.

Here I also share you how in my local env I set arabic language, then using modulestore, yes my course language is ar but with the method of course_overview is None.

andrey-canon requested a review from johanseto November 17, 2023 19:52

github-actions bot added django_plugin test size/m m lines label labels Nov 17, 2023

johanseto self-assigned this Nov 17, 2023

johanseto requested changes Nov 17, 2023

View reviewed changes

andrey-canon requested a review from johanseto November 18, 2023 00:04

andrey-canon force-pushed the and/add_xapi_enrollment_object_filter branch from 83b1d4e to 8cf8feb Compare November 18, 2023 00:07

johanseto reviewed Nov 20, 2023

View reviewed changes

johanseto self-requested a review November 20, 2023 21:47

johanseto approved these changes Nov 20, 2023

View reviewed changes

feat: add XApiBaseEnrollmentFilter

5247237

This filter allows to implement the required object structure by the NELC team https://edunext.atlassian.net/browse/FUTUREX-251

andrey-canon force-pushed the and/add_xapi_enrollment_object_filter branch from dacc260 to 5247237 Compare November 20, 2023 22:26

andrey-canon merged commit cd9de65 into master Nov 20, 2023
7 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add XApiBaseEnrollmentFilter #108

feat: add XApiBaseEnrollmentFilter #108

andrey-canon commented Nov 17, 2023

johanseto Nov 17, 2023

andrey-canon Nov 17, 2023

johanseto Nov 17, 2023

andrey-canon Nov 17, 2023

andrey-canon Nov 17, 2023

andrey-canon Nov 17, 2023

johanseto Nov 17, 2023

andrey-canon Nov 17, 2023

johanseto Nov 17, 2023

johanseto Nov 17, 2023

johanseto Nov 17, 2023 •

edited

Loading

andrey-canon Nov 17, 2023 •

edited

Loading

andrey-canon commented Nov 18, 2023

johanseto left a comment •

edited

Loading

johanseto Nov 20, 2023

andrey-canon Nov 20, 2023

johanseto Nov 20, 2023

johanseto left a comment

andrey-canon commented Nov 20, 2023 •

edited

Loading

johanseto commented Nov 20, 2023

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

feat: add XApiBaseEnrollmentFilter #108

feat: add XApiBaseEnrollmentFilter #108

Conversation

andrey-canon commented Nov 17, 2023

Description

Testing instructions

Before

After

Additional information

Checklist for Merge

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanseto Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

andrey-canon Nov 17, 2023 • edited Loading

Choose a reason for hiding this comment

andrey-canon commented Nov 18, 2023

johanseto left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johanseto left a comment

Choose a reason for hiding this comment

andrey-canon commented Nov 20, 2023 • edited Loading

johanseto commented Nov 20, 2023

johanseto Nov 17, 2023 •

edited

Loading

andrey-canon Nov 17, 2023 •

edited

Loading

johanseto left a comment •

edited

Loading

andrey-canon commented Nov 20, 2023 •

edited

Loading