feat: add new learning events for course grades and badges #303

kyrylo-kh · 2024-01-22T14:11:25Z

Description:

This is a prerequisite work for the new Badges with Credly integration feature.

Changes

Adds a new dependency:
- edx-ccx-keys
Implements new custom serializer:
- CcxCourseLocatorAvroSerializer
Implements new data classes:
- CcxCourseData (CCX specific course data)
- CoursePassingStatusData (current course grade passing status for user)
- CcxCoursePassingStatusData (current CCX course grade passing status for user)
- BadgeTemplateData (badge template credential type)
- BadgeData (badge user credential)
Adds new public signals:
- COURSE_PASSING_STATUS_UPDATED (course grade update with passing status)
- CCX_COURSE_PASSING_STATUS_UPDATED (CCX course grade update with passing status)
- BADGE_AWARDED (new badge awarding for user)
- BADGE_REVOKED (existing badge revocation for user)

ISSUE: openedx/platform-roadmap#280

Dependencies: this PR itself is a blocker for 2 PRs (LMS, Credentials) which are work in progress.

Merge deadline: ASAP when possible.

Installation instructions: nothing special.

Testing instructions: noting special.

Reviewers:

Merge checklist:

Post merge:

Create a tag
Check new version is pushed to PyPI after tag-triggered build is
finished.
Delete working branch (if not needed anymore)

Author concerns:

This PR is a blocker for further LMS/Credentials contribution, since both services depend on the future openedx-events version that includes these signals/data.

openedx-webhooks · 2024-01-22T14:11:30Z

Thanks for the pull request, @kyrylo-kh! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

supporting documentation
Open edX discussion forum threads
timeline information ("this must be merged by XX date", and why that is)
partner information ("this is a course on edx.org")
any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

mphilbrick211 · 2024-01-22T15:43:08Z

Hi @kyrylo-kh! Thanks for this contribution. It looks like you're contributing on behalf of Raccoon Gang. In order to get you added to Raccoon Gang's entity CLA agreement, please have your manager reach out to [email protected]. Thank you!

wowkalucky · 2024-01-29T11:20:11Z

Context: Credentials Badges (+Credly integration).

This is a WIP PR to make possible early feedback. We have started from the bare minimum, but the details are still a matter of shaping.

justinhynes · 2024-01-31T19:46:28Z

openedx_events/learning/data.py

+@attr.s(frozen=True)
+class UserCourseData:
+    """
+    Attributes defined for Open edX user course data object.
+
+    Arguments:
+        user (UserData): user associated with the Course Enrollment.
+        course (CourseData): course where the user is enrolled in.
+    """
+
+    user = attr.ib(type=UserData)
+    course = attr.ib(type=CourseData)


Instead of this just being a combination of UserData and CourseData, should we instead possibly make this a GradeData data class?

Since this is being used in the new COURSE_GRADE_NOW_PASSED and COURSE_GRADE_NOW_FAILED events, it could be useful to the folks who may be consuming this event in the future to have the actual grade data available as part of the event.

Hopefully we could include some items like percent_grade, letter_grade and other model fields from the PersistentCourseGrade model of the LMS?

Seems like we're missing an opportunity here to include that data for future event bus consumers.

* feat: [ACI-75] new public events * feat: [ACI-139, ACI-145] new events for badge

…urse (#5) * feat: [ACI-512] describe ccx course payload dataclass * feat: [ACI-510] define ccx-course-grade-passed signal * feat: [ACI-511] define ccx-course-grade-failed signal * feat: [ACI-510, ACI-511, ACI-512] new events and dataclass for ccx course * style: [ACI-512] fix line-too-long and trailing-whitespace --------- Co-authored-by: Andrii <[email protected]>

* feat: [ACI-512] update CCX events payload * chore(deps): upgrade dependencies * feat(deps): add edx-ccx-keys library

…ta models (#7)

Co-authored-by: Andrii <[email protected]>

* refactor: [ACI-502, ACI-503] redesign course grading events payload * refactor: include passing status to data class * refactor: merge passing/failing signals to a single status signal * refactor: update avro schemas

openedx_events/learning/signals.py

wowkalucky · 2024-04-15T10:50:52Z

@kyrylo-kh pls update the description as well.

requirements/base.txt

* fix: [ACI-152] fix django tests failure * style: [ACI-152] adjust code style to pylint requirements --------- Co-authored-by: Andrii <[email protected]>

* test: [ACI-949] increase coverage & fix status field problem * refactor: [ACI-949] remove redundant comments --------- Co-authored-by: Andrii <[email protected]>

mariajgrimaldi · 2024-04-26T13:37:00Z

Do you folks have a draft PR in the service from where these events will be sent?

wowkalucky · 2024-04-29T14:37:29Z

@mariajgrimaldi here related Platform draft PR.

wowkalucky · 2024-04-30T08:30:06Z

I like the suggestion of refactoring the status property with constants to is_passing = <bool> as a more explicit option.

mariajgrimaldi · 2024-04-30T13:50:06Z

openedx_events/event_bus/avro/tests/test_custom_serializers.py

+class TestCCXLocatorSerailizer(TestCase):
+    def test_serialize(self):
+        obj1 = CCXLocator(org="edx", course="DemoX", run="Demo_course", ccx="1")
+        expected1 = "ccx-v1:edx+DemoX+Demo_course+ccx@1"
+        result1 = CcxCourseLocatorAvroSerializer.serialize(obj1)
+        self.assertEqual(result1, expected1)
+
+    def test_deseialize(self):
+        data1 = "ccx-v1:edx+DemoX+Demo_course+ccx@1"
+        expected1 = CCXLocator(org="edx", course="DemoX", run="Demo_course", ccx="1")
+        result1 = CcxCourseLocatorAvroSerializer.deserialize(data1)
+        self.assertEqual(result1, expected1)


Could you add docstrings here? Thanks!

mariajgrimaldi · 2024-04-30T14:24:38Z

openedx_events/learning/data.py

+        in which the grade was updated.
+    """
+
+    status = attr.ib(type=str)


Picking up the conversation about whether using status='passing' | 'failing' or `is_passing=True':

Option 1 status='passing' | 'failing':

Pros

Explicit status attribute for CoursePassingStatus...

Cons

Hard-coded constant

No validation support yet. Status could be anything.

The integration PR would need to know the status format:

CCX_COURSE_PASSING_STATUS_UPDATED.send_event( course_passing_status=CcxCoursePassingStatusData( status='passing', user=UserData( pii=UserPersonalData( username=user.username, email=user.email, name=user.get_full_name(), ), id=user.id, is_active=user.is_active, ), course=CcxCourseData( ccx_course_key=course_id, master_course_key=course_id.to_course_locator(), ), ) )

status='passing' was previously CcxCoursePassingStatusData.PASSING, which was standardized but then dropped altogether with the validator, but for maintainability reasons, we shouldn't use status='passing'.

Option 2 is_passing=True:

Pros

It's similar to what's currently used by the PersistentCourseGrade model to send the events: https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/grades/course_grade_factory.py#L180 passed=grade.passed

Doesn't need validation, it could be either True or False

Cons

Is it explicit enough?

Which of the options should we choose? I'm leaning towards option 2, but you folks have more context on the problem you're solving. Please, let me know.

Hi @mariajgrimaldi.
We discussed it internally and decided to go with the second option: we'll update status in favor of the is_passing=True|False.

@mariajgrimaldi we've refactored as discussed. Please, review.
@GlugovGrGlib fyi.

* chore: Updating Python Requirements (openedx#343) * chore: Updating Python Requirements (openedx#344) * refactor: [ACI-972] change course status to is_passing & add docs * chore: [ACI-972] requirements update * fix: remove local development index from requirements --------- Co-authored-by: edX requirements bot <[email protected]> Co-authored-by: Andrii <[email protected]> Co-authored-by: wowkalucky <[email protected]>

GlugovGrGlib · 2024-05-07T19:40:08Z

@mariajgrimaldi This is ready for the final review, it'll be good if we could bump version and merge related edx-platform PR after requirements update before Redwood cut

requirements/base.in

mariajgrimaldi

Just a few minor comments left to address. Thank you!

wowkalucky · 2024-05-08T09:01:37Z

@mariajgrimaldi fixed.

mariajgrimaldi · 2024-05-08T12:51:00Z

@wowkalucky: could you please address this last comment? Thank you. I'll merge @ 11am est or once that's done.

mariajgrimaldi

I'll merge this now as is. Thank you again!

openedx-webhooks · 2024-05-08T15:20:20Z

@kyrylo-kh 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

mariajgrimaldi · 2024-05-08T15:39:41Z

v9.10.0 released! Thank you all.

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label Jan 22, 2024

mphilbrick211 added the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 22, 2024

mphilbrick211 removed the needs test run Author's first PR to this repository, awaiting test authorization from Axim label Jan 30, 2024

justinhynes reviewed Jan 31, 2024

View reviewed changes

wowkalucky mentioned this pull request Feb 7, 2024

[WIP] feat: [ACI-240] Badges documentation raccoongang/credentials#48

Open

kyrylo-kh force-pushed the aci.upstream branch 2 times, most recently from 4b67a57 to 19e9217 Compare April 12, 2024 15:02

kyrylo-kh and others added 9 commits April 14, 2024 17:47

feat: [ACI-75] new public events

3122960

feat: [ACI-139, ACI-145] new events for badge (#4)

4ca494d

* feat: [ACI-75] new public events * feat: [ACI-139, ACI-145] new events for badge

feat: [ACI-512] update CCX events payload (#6)

098ce1d

* feat: [ACI-512] update CCX events payload * chore(deps): upgrade dependencies * feat(deps): add edx-ccx-keys library

chore: [ACI-473] update badges related payload atttibutes to match da…

48b9ed7

…ta models (#7)

feat: [ACI-800] implement avro serializer for ccx locator (#9)

5f96415

Co-authored-by: Andrii <[email protected]>

refactor: [ACI-820] simplify course grade status event payload (#10)

6aa6c90

refactor: [ACI-814] update course passing events types (#11)

da07adf

kyrylo-kh force-pushed the aci.upstream branch from 19e9217 to a6dd92e Compare April 14, 2024 15:10

wowkalucky suggested changes Apr 15, 2024

View reviewed changes

openedx_events/learning/signals.py Outdated Show resolved Hide resolved

kyrylo-kh force-pushed the aci.upstream branch from a6dd92e to 869166b Compare April 15, 2024 18:42

kyrylo-kh mentioned this pull request Apr 16, 2024

feat: emit new course passing status events openedx/edx-platform#34524

Closed

kyrylo-kh force-pushed the aci.upstream branch from 869166b to 6d3168d Compare April 17, 2024 15:41

wowkalucky reviewed Apr 18, 2024

View reviewed changes

requirements/base.txt Outdated Show resolved Hide resolved

kyrylo-kh and others added 2 commits April 18, 2024 17:25

feat: add new learning events for course grades and badges

8558418

fix: [ACI-152] fix ci failure (#12)

9d2a80d

* fix: [ACI-152] fix django tests failure * style: [ACI-152] adjust code style to pylint requirements --------- Co-authored-by: Andrii <[email protected]>

wowkalucky force-pushed the aci.upstream branch from 6a918eb to 9d2a80d Compare April 18, 2024 14:25

fix: [ACI-152] fix ci failure (#12)

960877c

* fix: [ACI-152] fix django tests failure * style: [ACI-152] adjust code style to pylint requirements --------- Co-authored-by: Andrii <[email protected]>

wowkalucky and others added 3 commits April 25, 2024 17:55

chore: upgrade requirements

f290a49

Merge branch 'main' into aci.upstream

f165593

test: [ACI-949] increase coverage & fix status field problem (#13)

619289a

* test: [ACI-949] increase coverage & fix status field problem * refactor: [ACI-949] remove redundant comments --------- Co-authored-by: Andrii <[email protected]>

mariajgrimaldi reviewed Apr 30, 2024

View reviewed changes

andrii-hantkovskyi and others added 6 commits May 7, 2024 20:17

fix: remove obsolete changes

63f1dc9

Merge branch 'aci.main' into aci.upstream

3b208f8

Merge branch 'main' into aci.upstream

a72c7e4

fix: merge upstream requirements

02d715a

style: quality checks

1068d0a

GlugovGrGlib requested a review from mariajgrimaldi May 7, 2024 19:38

mariajgrimaldi reviewed May 8, 2024

View reviewed changes

requirements/base.in Outdated Show resolved Hide resolved

mariajgrimaldi reviewed May 8, 2024

View reviewed changes

style: add missing end of line

bc2c855

mariajgrimaldi approved these changes May 8, 2024

View reviewed changes

mariajgrimaldi merged commit 6d62a74 into openedx:main May 8, 2024
11 checks passed

wowkalucky deleted the aci.upstream branch May 8, 2024 18:39

GlugovGrGlib mentioned this pull request May 9, 2024

feat: emit passing status updated events for badging openedx/edx-platform#34749

Merged

mariajgrimaldi mentioned this pull request May 14, 2024

Allow adding _in validators to attr classes #348

Open

GlugovGrGlib mentioned this pull request May 15, 2024

feat: emit passing status updated events for badging openedx/edx-platform#34806

Merged

kyrylo-kh mentioned this pull request Jun 4, 2024

feat: add badges feature openedx/credentials#2489

Merged

KyryloKireiev mentioned this pull request Jul 23, 2024

feat: Backport adding Badge issuance to Redwood openedx/credentials#2528

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add new learning events for course grades and badges #303

feat: add new learning events for course grades and badges #303

kyrylo-kh commented Jan 22, 2024 •

edited

Loading

openedx-webhooks commented Jan 22, 2024 •

edited

Loading

mphilbrick211 commented Jan 22, 2024

wowkalucky commented Jan 29, 2024

justinhynes Jan 31, 2024

wowkalucky commented Apr 15, 2024

mariajgrimaldi commented Apr 26, 2024

wowkalucky commented Apr 29, 2024

wowkalucky commented Apr 30, 2024

mariajgrimaldi Apr 30, 2024

mariajgrimaldi Apr 30, 2024

wowkalucky May 7, 2024

wowkalucky May 7, 2024

GlugovGrGlib commented May 7, 2024 •

edited

Loading

mariajgrimaldi left a comment

wowkalucky commented May 8, 2024

mariajgrimaldi commented May 8, 2024 •

edited

Loading

mariajgrimaldi left a comment

openedx-webhooks commented May 8, 2024

mariajgrimaldi commented May 8, 2024

feat: add new learning events for course grades and badges #303

feat: add new learning events for course grades and badges #303

Conversation

kyrylo-kh commented Jan 22, 2024 • edited Loading

openedx-webhooks commented Jan 22, 2024 • edited Loading

mphilbrick211 commented Jan 22, 2024

wowkalucky commented Jan 29, 2024

justinhynes Jan 31, 2024

Choose a reason for hiding this comment

wowkalucky commented Apr 15, 2024

mariajgrimaldi commented Apr 26, 2024

wowkalucky commented Apr 29, 2024

wowkalucky commented Apr 30, 2024

mariajgrimaldi Apr 30, 2024

Choose a reason for hiding this comment

mariajgrimaldi Apr 30, 2024

Choose a reason for hiding this comment

Option 1 status='passing' | 'failing':

Option 2 is_passing=True:

wowkalucky May 7, 2024

Choose a reason for hiding this comment

wowkalucky May 7, 2024

Choose a reason for hiding this comment

GlugovGrGlib commented May 7, 2024 • edited Loading

mariajgrimaldi left a comment

Choose a reason for hiding this comment

wowkalucky commented May 8, 2024

mariajgrimaldi commented May 8, 2024 • edited Loading

mariajgrimaldi left a comment

Choose a reason for hiding this comment

openedx-webhooks commented May 8, 2024

mariajgrimaldi commented May 8, 2024

kyrylo-kh commented Jan 22, 2024 •

edited

Loading

openedx-webhooks commented Jan 22, 2024 •

edited

Loading

Option 1 `status='passing' | 'failing'`:

Option 2 `is_passing=True`:

GlugovGrGlib commented May 7, 2024 •

edited

Loading

mariajgrimaldi commented May 8, 2024 •

edited

Loading