Skip to content

feat: Emit course completion related events#28415

Merged
ziafazal merged 1 commit intoopenedx:masterfrom
edly-io:ERTE-77
Sep 1, 2021
Merged

feat: Emit course completion related events#28415
ziafazal merged 1 commit intoopenedx:masterfrom
edly-io:ERTE-77

Conversation

@rehanedly
Copy link

@rehanedly rehanedly commented Aug 6, 2021

Emit following events:1. edx.course.grade.now_passed event from lms/djangoapps/certificates/signals.py on COURSE_GRADE_NOW_PASSED receiver in listen_for_passing_grade function2. edx.course.grade.now_failed event from lms/djangoapps/certificates/signals.py on COURSE_GRADE_NOW_FAILED receiver in _listen_for_failing_grade function3. edx.course.completed event fromedx-platform/lms/djangoapps/grades/models.py on passed_timestamp save in update_or_create function

Please give the pull request a short but descriptive title.
Use conventional commits to separate and summarize commits logically:
https://open-edx-proposals.readthedocs.io/en/latest/oep-0051-bp-conventional-commits.html

Use this template as a guide. Omit sections that don't apply. You may link to information rather than copy it.
More details about the template are at openedx/openedx-proposals#180
(link will be updated when that document merges)
-->

Description

Here is the ADR for this implemetation https://github.com/edx/edx-platform/pull/28424/files

Useful information to include:

  • Which edX user roles will this change impact? Common user roles are "Learner", "Course Author",
    "Developer", and "Operator".
  • Include screenshots for changes to the UI (ideally, both "before" and "after" screenshots, if applicable).
  • Provide links to the description of corresponding configuration changes. Remember to correctly annotate these
    changes.

Supporting information

Link to other information about the change, such as Jira issues, GitHub issues, or Discourse discussions.
Be sure to check they are publicly readable, or if not, repeat the information here.

Testing instructions

Please provide detailed step-by-step instructions for testing this change.

Deadline

"None" if there's no rush, or provide a specific date or event (and reason) if there is one.

Other 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.

@openedx-webhooks openedx-webhooks added needs triage open-source-contribution PR author is not from Axim or 2U labels Aug 6, 2021
@openedx-webhooks
Copy link

openedx-webhooks commented Aug 6, 2021

Thanks for the pull request, @rehanedly! I've created OSPR-5955 to keep track of it in JIRA, where we prioritize reviews. 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.

@rehanedly rehanedly requested a review from a team August 6, 2021 14:29
@rehanedly rehanedly force-pushed the ERTE-77 branch 2 times, most recently from 5834eb2 to 0847ac7 Compare August 6, 2021 14:49
@rehanedly rehanedly changed the title Erte 77 [ERTE-77] Emit course completion related events Aug 6, 2021
@rehanedly rehanedly changed the title [ERTE-77] Emit course completion related events WIP [ERTE-77] Emit course completion related events Aug 7, 2021
@rehanedly rehanedly marked this pull request as draft August 7, 2021 19:21
@rehanedly rehanedly changed the title WIP [ERTE-77] Emit course completion related events WIP [ERTE-77] feat: Emit course completion related events Aug 7, 2021
@rehanedly rehanedly force-pushed the ERTE-77 branch 5 times, most recently from 52cebb6 to e78de4f Compare August 9, 2021 07:58
@natabene
Copy link
Contributor

natabene commented Aug 9, 2021

@rehanedly Thank you for your contribution. Please let me know once it is ready for our review.

@openedx-webhooks openedx-webhooks added waiting on author PR author needs to resolve review requests, answer questions, fix tests, etc. and removed needs triage labels Aug 11, 2021
@rehanedly rehanedly force-pushed the ERTE-77 branch 9 times, most recently from ed795e4 to 1392e17 Compare August 17, 2021 10:29
@natabene
Copy link
Contributor

natabene commented Aug 20, 2021

@rehanedly Thank you for letting me know. @nasthagiri Is this still part of the project @ziafazal is working on? Would you expect GTA to review this?

Copy link
Contributor

@nasthagiri nasthagiri left a comment

Choose a reason for hiding this comment

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

Thanks for this PR. It's a fundamental step in providing the needed platform events for emitting xAPI/Caliper completion events.

Copy link
Contributor

Choose a reason for hiding this comment

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

[doc request] Can you add documentation (comments or ADR) on the meaning of each of these newly defined events?

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah. Great. Can you add a link to the ADR from here as context?

Copy link
Author

Choose a reason for hiding this comment

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

sure

Copy link
Author

Choose a reason for hiding this comment

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

@nasthagiri can you please review this ADR and Approve it https://github.com/edx/edx-platform/pull/28424. Then I will add file link. Currently I can add file link from this PR

Copy link
Author

@rehanedly rehanedly Aug 25, 2021

Choose a reason for hiding this comment

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

Added ADR in description

Copy link
Contributor

Choose a reason for hiding this comment

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

[Seeking info] Why do some events contain the username in addition to the user_id?

Copy link
Author

Choose a reason for hiding this comment

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

@nasthagiri We need username to get_anonymous_user_id_by_username in event routing backend.

Copy link
Contributor

Choose a reason for hiding this comment

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

huh. Interesting. Could we update event-routing-backend to leverage user_id or username, whichever is available?
As decided in OEP-32, The LMS user_id should be used for all internal events. The username is PII and we'd like to limit its usage as much as possible - especially in events and APIs.

Copy link
Author

Choose a reason for hiding this comment

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

removed username

Copy link
Contributor

Choose a reason for hiding this comment

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

[double checking] Are event_transaction_id and event_transaction_type meaningful in these new events?

Copy link
Author

@rehanedly rehanedly Aug 25, 2021

Choose a reason for hiding this comment

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

@nasthagiri Added those for consistency with other events of grades. These fields might not be meaning full for xAPI/Caliper but could be meaningful for other backends like logging or segmentation and these fields are present in many of other events emits from edx-platform.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good.

Copy link
Contributor

Choose a reason for hiding this comment

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

[doc readability request] This is a critical comment that explains the definition of this signal. Would you rephrase the comment to something like:

This Signal indicates that the user has received a passing grade in the course for the first time. Any subsequent grade changes that may vary the passing/failing status will not re-trigger this event.

Copy link
Author

Choose a reason for hiding this comment

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

Done

Copy link
Contributor

Choose a reason for hiding this comment

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

[renaming consideration] Since course "completion" is an emerging term in the platform with different opinions on how to define it, can we have the core Grades layer be more precise on what this event entails? Specifically, what if we rename this event to COURSE_GRADE_PASSED_FIRST_TIME ?

Copy link
Author

Choose a reason for hiding this comment

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

Done

@nasthagiri
Copy link
Contributor

nasthagiri commented Aug 25, 2021

Would someone from @edx/masters-devs-gta also want to review this PR?

@nasthagiri nasthagiri removed the request for review from a team August 25, 2021 12:03
@nasthagiri
Copy link
Contributor

[Nit] Since ERTE-77 is meaningful only internally to Edly, could we remove it from the PR title? This would be in the same spirit as the recommendation to remove it from the commit subject line in OEP-51.

@rehanedly rehanedly changed the title [ERTE-77] feat: Emit course completion related events feat: Emit course completion related events Aug 25, 2021
@rehanedly rehanedly force-pushed the ERTE-77 branch 2 times, most recently from 10a1ae7 to eb6a7ef Compare August 25, 2021 14:05
Copy link
Contributor

Choose a reason for hiding this comment

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

[request to follow-through on renaming] Can you follow through on updating the name of the course_completed events elsewhere in this PR?

Essentially, within the core grades layer, let's be precise of what the event actually is. Then, in a higher layer (for now, it could be at the xapi-caliper transformation layer), we can decide that a course_grade_passed_first_time event translates to a course_completion event.

Copy link
Author

Choose a reason for hiding this comment

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

@nasthagiri I have renamed this event. we will. handle translation in the event routing backend

@edx-status-bot
Copy link

Your PR has finished running tests. There were no failures.

@rehanedly rehanedly requested a review from nasthagiri August 27, 2021 07:40
@ziafazal ziafazal merged commit 5a382d8 into openedx:master Sep 1, 2021
@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the staging environment in preparation for a release to production.

@edx-pipeline-bot
Copy link
Contributor

EdX Release Notice: This PR has been deployed to the production environment.

@openedx-webhooks
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merged open-source-contribution PR author is not from Axim or 2U

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants