feat: initial implementation [BB-7595] #3

Agrendalath · 2023-11-14T01:56:42Z

Merge checklist:
Check off if complete or not applicable:

Version bumped
Changelog record added
Documentation updated (not only docstrings)
Fixup commits are squashed away
Unit tests added/updated
Manual testing instructions provided
Noted any: Concerns, dependencies, migration issues, deadlines, tickets

Testing instructions

Use the master branch of the edx-platform.
Clone and install this package. Run migrations.
Upload the assets attached to the Jira ticket (private).

Create two certificate types:

Completion:

Retrieval func: retrieve_course_completions
Generation func: generate_pdf_certificate

Custom options:

 {
     "name_y": 290,
     "course_name_y": 210,
     "template": "template_completion",
     "template_two_lines": "template_completion_two_line",
     "required_completion": 0.9
 }

Achievement:

Retrieval func: retrieve_subsection_grades
Generation func: generate_pdf_certificate

Custom options:

 {
     "name_y": 290,
     "course_name_y": 220,
     "template": "template_achievement",
     "required_grades": {
         "Homework": 0.4,
         "Exam": 0.9,
         "Total": 0.8
     }
 }

Create two course certificate configurations:
1. Completion - you can use course-v1:edX+DemoX+Demo_Course as the course ID and override the required_completion to make the testing easier.
2. Achievement - you can use course-v1:edX+DemoX+Demo_Course as the course ID and override the required_grades to make the testing easier.
Press the "generate certificates" button for each of the created course configurations.
Check that the certificates were generated correctly.

Notes

The docs are deployed to https://openedx-certificates.readthedocs.io.
The pluggability of processors and generators will be included in the next iteration.

Private-ref: BB-7595

docs/decisions/0002-architecture.rst

bradenmacdonald

I know this is a lot of feedback but I do think this looks nice! Feel free to push back on anything; a lot of this is trying to understand and push for clear documentation.

bradenmacdonald · 2023-11-15T19:35:11Z

README.rst


 |pypi-badge| |ci-badge| |codecov-badge| |doc-badge| |pyversions-badge|
-|license-badge| |status-badge|
+|license-badge| |status-badge| |visualization-badge|

 Purpose
 *******

 A pluggable service for preparing Open edX certificates.


Can we expand the README a bit? To explain what this is and why it's useful. You could put a short summary and link to the ADR if you want. Linking to the docs isn't helpful since they're empty.

If this is focused on the HMS use cases for now, we can say something like that - "This is focused on the use cases of HMS but designed to be a flexible foundation for any open edx certificate needs."

Can we also state how this compares to / differs from the credentials service ? Again you can link to the ADR.

What kind of certificates does this produce? PDF, HTML, badge, or "anything you implement as a plugin" ?

docs/decisions/0001-purpose-of-this-repo.rst

bradenmacdonald · 2023-11-15T19:51:26Z

docs/decisions/0002-architecture.rst

+   #. :ref:`Lesson <lesson>`.
+   #. :ref:`Pathway <pathway>`.
+
+#. This Django app uses `celerybeat`_ to periodically retrieve data from the external API. Retrieving this data is


what "external API"? ADR 0001 says this will use public Python APIs, which I would call "internal" (to Open edX). Is this interacting with SalesForce or something?

Also, can you add an explanation of why we're using cron instead of hooks ?

+1 on this. The hooks and filters question has been on my mind as well. As the openedx-filters tutorial, titled “How to use Open edX Filters?” is about using a custom pipeline to generate certificates, it will be one of the first questions from the community.

The ADR 001 states,

We do not need to pull data in real time. This service can retrieve data periodically, but the frequency should be configurable per course.

While real time is not needed, what would be the impact of doing it in 'near real-time' using a combination of Hooks and Celery tasks (without schedule), that we are avoiding here?

bradenmacdonald · 2023-11-15T19:52:02Z

docs/decisions/0002-architecture.rst

+            CertificateType [label="ExternalCertificateType"]
+            CourseConfiguration [label="ExternalCertificateCourseConfiguration"]
+            Certificate [label="ExternalCertificate"]
+            Asset [label="ExternalCertificateAsset"]


Why are these things all called "External"? Since the whole point of this repo seems to be that it plugs in to run "within" Open edX as a django app plugin, and it generates the certs within the LMS app, I would personally not think of them as External. And if they can be like "badges" I wouldn't think of them as "Certificates" either... A name like "FlexCredential" or "CertifiedCredential" or something seems more descriptive. Or you can name this repo something less ambiguous than "openedx-certficicates", for example "Aceso" then it could be "AcesoCredential", etc.

What I would consider an "External" certificate would be one issued by a third party like Accredible or Credly, which would be hosted on an entirely different domain outside of our control (it would be good to make sure our architecture supports those as an option, even if we don't need them now).

bradenmacdonald · 2023-11-15T20:43:24Z

openedx_certificates/processors.py

+"""
+This module contains processors for certificate criteria.
+
+The functions prefixed with `retrieve_` are automatically detected by the admin page and are used to retrieve the


by the admin page

I would have thought it's a cron job calling these, not manual trigger from the admin page?

The admin page seems to just list the functions in the choice dropdown, which once saved in the model, is then used elsewhere to actually get the list of user IDs.

The doc string here is a little confusing. It does make the actions in 2 places happen in the same place.

bradenmacdonald · 2023-11-15T20:45:00Z

openedx_certificates/compat.py

+
+
+def get_course_grade_factory():  # noqa: ANN201
+    """Get the course grade factory from Open edX."""


Can you mention what this factory does, and how it interacts with prefetch_...? I thought maybe it was for mocking grades during testing, but it seems that's not the case.

bradenmacdonald · 2023-11-15T20:46:53Z

openedx_certificates/processors.py

+The functions prefixed with `retrieve_` are automatically detected by the admin page and are used to retrieve the
+IDs of the users that meet the criteria for the certificate type.
+
+We will move this module to an external repository (a plugin).


I would suggest just putting this into a plugins/completion/processor.py file in the same repo, along with the corresponding generators (for "Certificate of completion" & "Badge of achievement/completion")

bradenmacdonald · 2023-11-15T20:51:20Z

requirements/quality.in

@@ -4,9 +4,6 @@

 -r test.txt               # Core and testing dependencies for this package



Nit: Personally I don't think it's better to have so many different requirements files. Just three - base, dev, and constraints - would cover every use case, even if dev will sometimes install more than is needed for a specific use case like quality or ci. But no need to change anything, just feel like mentioning it.

bradenmacdonald · 2023-11-15T20:53:51Z

setup.py

-"""
-Package metadata for openedx_certificates.
-"""
+"""Package metadata for openedx_certificates."""


Do we still need setup.py? Can't we declare all this using pyproject.toml which is the replacement, and which is more efficient since it doesn't have to be run as code before it can be used?

tecoholic · 2023-11-18T05:36:40Z

openedx_certificates/models.py

+        user = get_user_model().objects.get(id=user_id)
+        # Use the name from the profile if it is not empty. Otherwise, use the first and last name.
+        # We check if the profile exists because it is absent in unit tests.
+        user_full_name = getattr(getattr(user, 'profile', None), 'name', f"{user.first_name} {user.last_name}")


Q: Wouldn't it be better to mock user.profile in the tests than to adapt code for the tests?

tecoholic · 2023-11-18T05:37:32Z

openedx_certificates/models.py

+            try:
+                module_path, func_name = func_path.rsplit('.', 1)
+                module = import_module(module_path)
+                getattr(module, func_name)  # Will raise AttributeError if the function does not exist.


tecoholic · 2023-11-18T06:01:43Z

openedx_certificates/processors.py

+"""
+This module contains processors for certificate criteria.
+
+The functions prefixed with `retrieve_` are automatically detected by the admin page and are used to retrieve the


The admin page seems to just list the functions in the choice dropdown, which once saved in the model, is then used elsewhere to actually get the list of user IDs.

The doc string here is a little confusing. It does make the actions in 2 places happen in the same place.

tecoholic · 2023-11-18T06:18:15Z

openedx_certificates/processors.py

+          }
+
+        It means that the user must receive at least 40% in the Homework category and 90% in the Exam category.
+        The "Total" key is a special value used to specify the minimum required grade for all categories in the course.


Does the "Total"

specify the minimum required grade for all categories in the course.

or specifies the minimum required grade of the final grade, calculated using the weighted total of all other grades?

tecoholic · 2023-11-18T06:21:19Z

openedx_certificates/processors.py

+
+    users = get_course_enrollments(course_id)
+    grades = _get_grades_by_format(course_id, users)
+    log.debug(grades)


Nit: Include a description in the log.debug statements like log.debug("Grades: %s", grades) to get a better idea of what we are seeing in the logs?

tecoholic · 2023-11-20T06:46:52Z

openedx_certificates/generators.py

+    return font or 'Helvetica'
+
+
+def _write_text_on_template(template: any, font: str, username: str, course_name: str, options: dict[str, Any]) -> any:


Suggested change

def _write_text_on_template(template: any, font: str, username: str, course_name: str, options: dict[str, Any]) -> any:

def _write_course_and_user_on_template(template: any, font: str, username: str, course_name: str, options: dict[str, Any]) -> any:

Make the function name more specific and leave the text_on_template free for a more generic func(template, text, location,...) function that may be introduced later?

tecoholic · 2023-11-20T07:00:01Z

openedx_certificates/generators.py

+    """
+    log.info("Starting certificate generation for user %s", user.id)
+    # Get template from the ExternalCertificateAsset.
+    template_file = ExternalCertificateAsset.get_asset_by_slug(options['template'])


There appears to be a no. of functions with the last argument options proxy-ing kwargs. Not an issue in itself. But makes it difficult to understand the function's requirements in cases like this where options['template'] seems to be a required parameter that should be an independent argument.

There is also the case of me (and similar devs) being lazy. I tend to think the last param with dicts is probably optional (I know options doesn't mean optional, still you know) and the function most likely has default values defined, and it will just work with an empty dict. _write_text_on_template would work under this assumption, but not this function.

I understand, since this is a first iteration and this function will be pushed into a plugin, it is not a big deal now. Just thought I should mention this here, so it helps when defining this function's signature, that the plugins should adhere to.

tecoholic · 2023-11-20T07:54:31Z

docs/decisions/0002-architecture.rst

+   #. :ref:`Lesson <lesson>`.
+   #. :ref:`Pathway <pathway>`.
+
+#. This Django app uses `celerybeat`_ to periodically retrieve data from the external API. Retrieving this data is


+1 on this. The hooks and filters question has been on my mind as well. As the openedx-filters tutorial, titled “How to use Open edX Filters?” is about using a custom pipeline to generate certificates, it will be one of the first questions from the community.

tecoholic · 2023-11-20T07:58:58Z

docs/decisions/0002-architecture.rst

+   #. :ref:`Lesson <lesson>`.
+   #. :ref:`Pathway <pathway>`.
+
+#. This Django app uses `celerybeat`_ to periodically retrieve data from the external API. Retrieving this data is


The ADR 001 states,

We do not need to pull data in real time. This service can retrieve data periodically, but the frequency should be configurable per course.

While real time is not needed, what would be the impact of doing it in 'near real-time' using a combination of Hooks and Celery tasks (without schedule), that we are avoiding here?

docs/decisions/0002-architecture.rst

…o admin

Agrendalath self-assigned this Nov 14, 2023

Agrendalath force-pushed the agrendalath/bb-7595-initial_implementation branch 3 times, most recently from 420c6a6 to 8fce528 Compare November 14, 2023 13:56

Agrendalath added 2 commits November 14, 2023 15:32

style: add ruff

b65a645

style: add yamllint

bb52142

Agrendalath force-pushed the agrendalath/bb-7595-initial_implementation branch from b832ed7 to 3b8071d Compare November 14, 2023 14:32

Agrendalath marked this pull request as ready for review November 14, 2023 14:32

Agrendalath requested review from tecoholic and bradenmacdonald November 14, 2023 14:47

Agrendalath commented Nov 14, 2023

View reviewed changes

docs/decisions/0002-architecture.rst Show resolved Hide resolved

Agrendalath added 8 commits November 14, 2023 16:55

docs: create ADRs

515e647

docs: add repo visualization to README

f17423b

docs: update CHANGELOG

8045f3b

chore: update requirements

2189a53

test: exclude tests from coverage

d6d9105

build: make manage.py executable

6542508

feat: initial implementation

be229da

test: add tests

68631e1

Agrendalath force-pushed the agrendalath/bb-7595-initial_implementation branch from c9fb3e9 to 68631e1 Compare November 14, 2023 15:55

Agrendalath added 2 commits November 15, 2023 17:25

fixup! chore: update requirements

6181e4e

chore: upgrade tox from v3 to v4 and remove tox-battery

127df64

Agrendalath force-pushed the agrendalath/bb-7595-initial_implementation branch from 50ed981 to 127df64 Compare November 15, 2023 16:34

Agrendalath added 5 commits November 15, 2023 19:20

fixup! feat: initial implementation

16f9878

fixup! feat: initial implementation

be1e58e

fixup! feat: initial implementation

25d985b

fixup! test: add tests

a3edc9b

fixup! feat: initial implementation

a083d4f

bradenmacdonald reviewed Nov 15, 2023

View reviewed changes

tecoholic requested changes Nov 20, 2023

View reviewed changes

Agrendalath added 29 commits January 10, 2024 23:48

chore: update requirements

8ca21bb

docs: add quickstart docs

ee44ba0

fix: use correct format for generation function options docstring

bc2b028

feat: send emails for generated certificates

2be923b

fixup! feat: send emails for generated certificates

5115f40

fix: fix celery tasks

263aa51

feat: do not regenerate certificates all the time

aeeaa72

feat: do not send emails to inactive users

b28e022

fix: do not regenerate certificates from the celery task

d5d99f6

fixup! fix: do not regenerate certificates from the celery task

033a49a

feat: run Django admin generation tasks with Celery

7daf6f3

fixup! fix: fix celery tasks

4a225d8

fix: validate course ID in Django admin instead of raising an error

07335fb

temp: add certificate created and modified date to Django admin

711dbf6

feat: allow changing text color on generated PDF

bfcc991

feat: add issue date to PDF certificates

2de93b2

fixup! docs: add quickstart docs

de3b56e

fixup! feat: send emails for generated certificates

12626cf

feat: custom domains for certificates

de9b5a8

fixup! feat: add issue date to PDF certificates

2a88fd8

feat: display custom options for course configuration in Django admin

cc3230a

fixup! feat: display custom options for course configuration in Djang…

85854ca

…o admin

fixup! feat: display custom options for course configuration in Djang…

17ed355

…o admin

fixup! feat: display custom options for course configuration in Djang…

29ffdf3

…o admin

feat: encrypt generated PDF to prevent modifications

57b68b2

test: add migration checks to tox

5b31e4d

feat: allow overriding course name

ff92838

fix: support multiline course names

c52a657

fix: clean up periodic tasks when deleting course configurations

d799a24

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: initial implementation [BB-7595] #3

feat: initial implementation [BB-7595] #3

Agrendalath commented Nov 14, 2023 •

edited

Loading

bradenmacdonald left a comment

bradenmacdonald Nov 15, 2023

bradenmacdonald Nov 15, 2023

bradenmacdonald Nov 15, 2023

tecoholic Nov 20, 2023

tecoholic Nov 20, 2023

bradenmacdonald Nov 15, 2023 •

edited

Loading

bradenmacdonald Nov 15, 2023

tecoholic Nov 18, 2023

bradenmacdonald Nov 15, 2023

bradenmacdonald Nov 15, 2023

bradenmacdonald Nov 15, 2023

bradenmacdonald Nov 15, 2023

tecoholic Nov 18, 2023

tecoholic Nov 18, 2023

tecoholic Nov 18, 2023

tecoholic Nov 18, 2023

tecoholic Nov 18, 2023

tecoholic Nov 20, 2023

tecoholic Nov 20, 2023

tecoholic Nov 20, 2023

tecoholic Nov 20, 2023



		def get_course_grade_factory(): # noqa: ANN201
		"""Get the course grade factory from Open edX."""

		@@ -4,9 +4,6 @@

		-r test.txt # Core and testing dependencies for this package

		return font or 'Helvetica'


		def _write_text_on_template(template: any, font: str, username: str, course_name: str, options: dict[str, Any]) -> any:

	def _write_text_on_template(template: any, font: str, username: str, course_name: str, options: dict[str, Any]) -> any:
	def _write_course_and_user_on_template(template: any, font: str, username: str, course_name: str, options: dict[str, Any]) -> any:

feat: initial implementation [BB-7595] #3

Are you sure you want to change the base?

feat: initial implementation [BB-7595] #3

Conversation

Agrendalath commented Nov 14, 2023 • edited Loading

Testing instructions

Notes

bradenmacdonald left a 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

bradenmacdonald Nov 15, 2023 • 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Agrendalath commented Nov 14, 2023 •

edited

Loading

bradenmacdonald Nov 15, 2023 •

edited

Loading