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

[BB-9241] Add learner pathways cert generation #69

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

pkulkark
Copy link
Member

@pkulkark pkulkark commented Jan 22, 2025

Description

This PR adds the integration with learner pathways so that the cert service can be used to generate certificates for learner pathways. It replaces the course_id field with resource_id and resource_type fields, so that the cert service can be extended to other types of data resources (not just courses). If a course that is part of the learner pathway offers certificates then the eligibility criteria already defined is used. If not, then it defaults to completion criteria.

Supporting Information

OpenCraft Internal Jira ticket: BB-9241

Testing Instructions

  1. Create a learner pathway on your devstack and enroll users on it.
  2. In the admin panel, upload required cert assets.
  3. Setup new cert type for learner pathways and select the retrieval function retrieve_learner_paths_eligible_users.
  4. Setup a cert config similar to what is described in the quick start guide. For specifying specific course based completion criteria, use the following format in custom_options field:
{
  "course_options": {
    "course-v1:edX+DemoX+Demo_Course": {"required_completion": 0.5},
    "course-v1:edX+M12+1T2025": {"required_completion": 0.5}
  }
}
  1. Login as the enrolled user and complete the required courses.
  2. In the admin panel, click on Generate certificate and verify that all eligible users receive a certificate.

@pkulkark pkulkark force-pushed the pooja/bb9241-intergrate-learning-paths branch 25 times, most recently from 377a160 to dfd5de3 Compare January 27, 2025 20:16
@pkulkark pkulkark marked this pull request as ready for review January 28, 2025 19:17
@Agrendalath Agrendalath force-pushed the agrendalath/bb-7595-initial_implementation branch 2 times, most recently from 853d7d2 to 6a820f9 Compare January 29, 2025 21:06
Base automatically changed from agrendalath/bb-7595-initial_implementation to main January 29, 2025 21:15
@Agrendalath Agrendalath force-pushed the pooja/bb9241-intergrate-learning-paths branch from dfd5de3 to 971c2af Compare January 29, 2025 22:11
@Agrendalath
Copy link
Member

Agrendalath commented Jan 29, 2025

@pkulkark, I haven't fully reviewed the PR yet, as I was closing #3. I already rebased your PR after merging it.

That said, I have a couple of initial notes:

  1. As we're renaming the models, it might be worth thinking about more intuitive names for them than "External*" (ref). Do you have any ideas on how we could call them? This plugin is deployed to Prod but is not widely used (i.e., it's not integrated with the client's analytics yet), so it might be a good moment to make these changes.
  2. The name "resource" is very generic, so renaming everything from "course" to "resource" will significantly impact the readability. This is similar to when we used to call XBlocks "items" in the edx-platform, which made the code hard to read. As this name is also used in the Django admin, we should prepare a more verbose one. I think that the term LearningContext could be more suitable, but please see the next point before applying this change.
  3. This also got me thinking about how we want to represent the Learning Paths. I discussed it with the client today and concluded that we need a more user-friendly format than UUIDs. I'll follow up on the requirements for this by the end of the week. Therefore, I'd like to wait until we clarify this to avoid introducing unnecessarily complex migrations if we need to change the field type. If we end up with a more structured format, it might be a good idea to subclass the LearningContextKey and implement the ID validation.

Also, a small note: we should try to use the term "Learning Paths" instead of "pathways" or "learner pathways" to avoid confusion.

@pkulkark
Copy link
Member Author

@Agrendalath Thanks for the insight, that makes a lot of sense.

As we're renaming the models, it might be worth thinking about more intuitive names for them than "External*" (#71). Do you have any ideas on how we could call them? This plugin is deployed to Prod but is not widely used (i.e., it's not integrated with the client's analytics yet), so it might be a good moment to make these changes.

How about something like LearningCredential and LearningCredentialConfiguration? That could keep the door open for badges and other learning recognitions.

The name "resource" is very generic, so renaming everything from "course" to "resource" will significantly impact the readability. This is similar to when we used to call XBlocks "items" in the edx-platform, which made the code hard to read. As this name is also used in the Django admin, we should prepare a more verbose one. I think that the term LearningContext could be more suitable, but please see the next point before applying this change.

That's a good point.

This also got me thinking about how we want to represent the Learning Paths. I discussed it with the client today and concluded that we need a more user-friendly format than UUIDs. I'll follow up on the requirements for this by the end of the week. Therefore, I'd like to wait until we clarify this to avoid introducing unnecessarily complex migrations if we need to change the field type. If we end up with a more structured format, it might be a good idea to subclass the LearningContextKey and implement the ID validation.

That makes sense. I'll wait for an update before moving any further here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants