Skip to content
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
137 changes: 137 additions & 0 deletions openedx_events/learning/data.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,137 @@
"""
Data attributes for events within the architecture subdomain `learning`.

These attributes follow the form of attr objects specified in OEP-49 data
pattern.
"""
from datetime import datetime

import attr
from opaque_keys.edx.keys import CourseKey


@attr.s(frozen=True)
class UserNonPersonalData:
"""
Attributes defined for Open edX user object based on non-PII data.

Arguments:
id (int): unique identifier for the Django User object.
is_active (bool): indicates whether the user is active.
"""

id = attr.ib(type=int)
is_active = attr.ib(type=bool)


@attr.s(frozen=True)
class UserPersonalData:
"""
Attributes defined for Open edX user object based on PII data.

Arguments:
username (str): username associated with the Open edX user.
email (str): email associated with the Open edX user.
name (str): email associated with the Open edX user's profile.
"""

Copy link

Choose a reason for hiding this comment

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

Is there a reason we're not passing the ID of the student?

Copy link
Member Author

Choose a reason for hiding this comment

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

There's no specific reason besides security issues -given that the ID is numerical and auto-incremented-. However, after some digging, we found OEP-32 that supports the usage of the user_id:

The LMS user_id should be used for all internal events. Additionally, the LMS user_id can be used for public events, REST APIs, and JavaScript APIs.

So, let's adopt the usage of the user_id even with the downsides in this case.

What are your thoughts on this @felipemontoya?

Copy link
Member

Choose a reason for hiding this comment

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

I see you put it in the UserNonPersonalData attr class. Given the "LMS user_id can be used for public events" portion of OEP-32 I think we are in the clear

username = attr.ib(type=str)
email = attr.ib(type=str)
name = attr.ib(type=str, factory=str)
Copy link

Choose a reason for hiding this comment

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

Is is possible to separate out the PII parts (email, name) of this data into a separate object from the non-PII parts? I strongly suspect we'll want to annotate this stuff eventually, but having explicit separation is something simple we can do now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great suggestion! We're on it

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @ormsbee! We followed your suggestions. Now, the user is divided into two parts with PII and non-PII information

Copy link
Member Author

Choose a reason for hiding this comment

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

Hey @feanil!
We also changed StudentData to UserData :)



@attr.s(frozen=True)
class UserData(UserNonPersonalData):
"""
Attributes defined for Open edX user object.

This class extends UserNonPersonalData to include PII data completing the
user object.

Arguments:
pii (UserPersonalData): user's Personal Identifiable Information.
"""

pii = attr.ib(type=UserPersonalData)


@attr.s(frozen=True)
class CourseData:
"""
Attributes defined for Open edX Course Overview object.

Arguments:
course_key (str): identifier of the Course object.
display_name (str): display name associated with the course.
start (datetime): start date for the course.
end (datetime): end date for the course.
"""

course_key = attr.ib(type=CourseKey)
display_name = attr.ib(type=str, factory=str)
start = attr.ib(type=datetime, default=None)
end = attr.ib(type=datetime, default=None)


@attr.s(frozen=True)
class CourseEnrollmentData:
"""
Attributes defined for Open edX Course Enrollment object.
Copy link

Choose a reason for hiding this comment

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

Is there anything about the context of the enrollment that we want to capture? Like who did the enrolling?

Copy link
Member Author

Choose a reason for hiding this comment

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

That might be useful. What do you think about sending price?

Also, do you think It'd be helpful for COURSE_ENROLLMENT_CHANGE to capture de status? They're defined here:
https://github.com/edx/edx-platform/blob/650f176b0a5363bbfeccbcb975977f83ca8994ff/common/djangoapps/student/models.py#L95

Copy link

Choose a reason for hiding this comment

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

I would maybe send the money related stuff as a separate event from events related to learning. I'm thinking of it as a separation between things important from an ecommerce domain perspective and the teaching/learning perspective.

(Loosely held opinion)


Arguments:
user (UserData): user associated with the Course Enrollment.
course (CourseData): course where the user is enrolled in.
mode (str): course mode associated with the course.
is_active (bool): whether the enrollment is active.
creation_date (datetime): creation date of the enrollment.
created_by (UserData): if available, who created the enrollment.
"""

user = attr.ib(type=UserData)
course = attr.ib(type=CourseData)
mode = attr.ib(type=str)
is_active = attr.ib(type=bool)
creation_date = attr.ib(type=datetime)
created_by = attr.ib(type=UserData, default=None)

Copy link

Choose a reason for hiding this comment

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

Missing datetime of enrollment?

Copy link

Choose a reason for hiding this comment

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

Or is that because it's going in a separate higher-level datetime field for the event as a whole?

Copy link
Member Author

Choose a reason for hiding this comment

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

Indeed, we're missing the creation date of the enrollment. We'll probably -surely- need it. Also, the DateTime field for the event may differ from the enrollment definition, so we'll surely add it.


@attr.s(frozen=True)
class CertificateData:
"""
Attributes defined for Open edX Certificate data object.

Arguments:
user (UserData): user associated with the Certificate.
course (CourseData): course where the user obtained the certificate.
mode (str): course mode associated with the course.
grade (str): user's grade in this course run.
current_status (str): current certificate status.
previous_status (str): if available, pre-event certificate status.
download_url (str): URL where the PDF version of the certificate.
name (str): user's name.
"""

user = attr.ib(type=UserData)
course = attr.ib(type=CourseData)
mode = attr.ib(type=str)
grade = attr.ib(type=str)
download_url = attr.ib(type=str)
name = attr.ib(type=str)
current_status = attr.ib(type=str)
previous_status = attr.ib(type=str, factory=str)


@attr.s(frozen=True)
class CohortData:
"""
Attributes defined for Open edX Cohort Membership object.

Arguments:
user (UserData): user assigned to the group.
course (CourseData): course associated with the course group.
name (str): name of the cohort group.
"""

user = attr.ib(type=UserData)
course = attr.ib(type=CourseData)
name = attr.ib(type=str)
3 changes: 2 additions & 1 deletion requirements/base.in
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
# Core requirements for using this application
-c constraints.txt

django
attrs
django
edx-opaque-keys[django]
8 changes: 8 additions & 0 deletions requirements/base.txt
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,15 @@ django==2.2.24
# via
# -c requirements/constraints.txt
# -r requirements/base.in
edx-opaque-keys[django]==2.2.2
# via -r requirements/base.in
pbr==5.6.0
# via stevedore
pymongo==3.11.4
# via edx-opaque-keys
pytz==2021.1
# via django
sqlparse==0.4.1
# via django
stevedore==3.3.0
# via edx-opaque-keys
7 changes: 7 additions & 0 deletions requirements/dev.txt
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,8 @@ docutils==0.17.1
# readme-renderer
edx-lint==5.0.0
# via -r requirements/quality.txt
edx-opaque-keys[django]==2.2.2
# via -r requirements/quality.txt
filelock==3.0.12
# via
# -r requirements/ci.txt
Expand Down Expand Up @@ -213,6 +215,10 @@ pylint-plugin-utils==0.6
# -r requirements/quality.txt
# pylint-celery
# pylint-django
pymongo==3.11.4
# via
# -r requirements/quality.txt
# edx-opaque-keys
pyparsing==2.4.7
# via
# -r requirements/ci.txt
Expand Down Expand Up @@ -283,6 +289,7 @@ stevedore==3.3.0
# via
# -r requirements/quality.txt
# code-annotations
# edx-opaque-keys
text-unidecode==1.3
# via
# -r requirements/quality.txt
Expand Down
7 changes: 7 additions & 0 deletions requirements/doc.txt
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,8 @@ docutils==0.17.1
# readme-renderer
# restructuredtext-lint
# sphinx
edx-opaque-keys[django]==2.2.2
# via -r requirements/test.txt
edx-sphinx-theme==3.0.0
# via -r requirements/doc.in
idna==3.2
Expand Down Expand Up @@ -84,6 +86,10 @@ pygments==2.9.0
# doc8
# readme-renderer
# sphinx
pymongo==3.11.4
# via
# -r requirements/test.txt
# edx-opaque-keys
pyparsing==2.4.7
# via
# -r requirements/test.txt
Expand Down Expand Up @@ -148,6 +154,7 @@ stevedore==3.3.0
# -r requirements/test.txt
# code-annotations
# doc8
# edx-opaque-keys
text-unidecode==1.3
# via
# -r requirements/test.txt
Expand Down
7 changes: 7 additions & 0 deletions requirements/quality.txt
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,8 @@ docutils==0.17.1
# via readme-renderer
edx-lint==5.0.0
# via -r requirements/quality.in
edx-opaque-keys[django]==2.2.2
# via -r requirements/test.txt
idna==3.2
# via requests
importlib-metadata==4.6.1
Expand Down Expand Up @@ -124,6 +126,10 @@ pylint-plugin-utils==0.6
# via
# pylint-celery
# pylint-django
pymongo==3.11.4
# via
# -r requirements/test.txt
# edx-opaque-keys
pyparsing==2.4.7
# via
# -r requirements/test.txt
Expand Down Expand Up @@ -176,6 +182,7 @@ stevedore==3.3.0
# via
# -r requirements/test.txt
# code-annotations
# edx-opaque-keys
text-unidecode==1.3
# via
# -r requirements/test.txt
Expand Down
15 changes: 13 additions & 2 deletions requirements/test.txt
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ django==2.2.24
# via
# -c requirements/constraints.txt
# -r requirements/base.txt
edx-opaque-keys[django]==2.2.2
# via -r requirements/base.txt
iniconfig==1.1.1
# via pytest
jinja2==3.0.1
Expand All @@ -29,11 +31,17 @@ markupsafe==2.0.1
packaging==21.0
# via pytest
pbr==5.6.0
# via stevedore
# via
# -r requirements/base.txt
# stevedore
pluggy==0.13.1
# via pytest
py==1.10.0
# via pytest
pymongo==3.11.4
# via
# -r requirements/base.txt
# edx-opaque-keys
pyparsing==2.4.7
# via packaging
pytest==6.2.4
Expand All @@ -57,7 +65,10 @@ sqlparse==0.4.1
# -r requirements/base.txt
# django
stevedore==3.3.0
# via code-annotations
# via
# -r requirements/base.txt
# code-annotations
# edx-opaque-keys
text-unidecode==1.3
# via python-slugify
toml==0.10.2
Expand Down