-
Notifications
You must be signed in to change notification settings - Fork 2
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
Feat/generate certificate management command #86
Conversation
src/backend/joanie/core/management/commands/create_certificates.py
Outdated
Show resolved
Hide resolved
src/backend/joanie/core/management/commands/create_certificates.py
Outdated
Show resolved
Hide resolved
src/backend/joanie/core/management/commands/create_certificates.py
Outdated
Show resolved
Hide resolved
b263e8f
to
ee222fe
Compare
ee222fe
to
7005e52
Compare
7005e52
to
46249d8
Compare
src/backend/joanie/core/admin.py
Outdated
""" | ||
codes = [course.code for course in queryset] | ||
certificate_generated_count = management.call_command( | ||
"create_certificates", courses=codes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it should be done like this... especially as the list of codes may be big. I would be better to factorize the code in a function that can be called by your command and by this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's try to resume your thought just to be sure I understand your request.
Do you mean, I should not use call_command
here but create a function that I'll call here and within the management command?
Then instead to pass a long list of course codes as arguments, I could iterate over selected courses?
About this point, I could write something like this
certificate_generated_count = 0
for course in queryset.iterator():
certificate_generated_count += management.call_command("create_certificates", courses=course.code)
summarize_certification_to_user(request, certificate_generated_count)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that's what I meant but that's not what you proposed in the second part of your answer? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I misspoke. The code suggestion was only about my second point :
Then instead to pass a long list of course codes as arguments, I could iterate over selected courses?
But I'm not quit sure to understand why I should not use the management command here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I create an helpers
module with two methods generate_certificate_for_order
and generate_certificates_for_orders
.
course_runs = models.CourseRun.objects.filter( | ||
course__in=graded_courses, | ||
is_gradable=True, | ||
start__lte=timezone.now(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or end_lte? I don't think we want to grade before the end of the course?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my point of view, the is_gradable
flag should mediate this question. If a course leader wants to allow certification before the end of the course, he could and if he wants to wait the end of the course, he just has to update is_gradable
once the course ended.
) | ||
|
||
enrollments = order.enrollments.filter( | ||
course_run__in=course_runs, is_active=True |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a small trick here: you say "generate for order" but you may select enrollments here that are related to other orders staging the same course run... 🤔 Add a filter clause and a test if I'm right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using order.enrollments
so there is no risk to get enrollment related to other order, nope?
src/backend/joanie/core/management/commands/create_certificates.py
Outdated
Show resolved
Hide resolved
src/backend/joanie/core/management/commands/create_certificates.py
Outdated
Show resolved
Hide resolved
src/backend/joanie/core/management/commands/create_certificates.py
Outdated
Show resolved
Hide resolved
f9784d9
to
4174c40
Compare
bc08ca4
to
f1ece51
Compare
src/backend/joanie/core/management/commands/create_certificates.py
Outdated
Show resolved
Hide resolved
190e16a
to
42123af
Compare
course_run.course.product_relations.update(is_graded=False) | ||
course = factories.CourseFactory(products=[product]) | ||
order = factories.OrderFactory(product=product, course=course) | ||
certificate = models.Certificate.objects.filter(order=order) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it shouldn't be called certificate when it's a query. Same for other occurences in other test files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I renamed it certificate_qs
1714e42
to
f4324e5
Compare
This boolean field allow a course leader to mark its session as gradable. Gradable course runs can be used to generate a certificate.
This property is a shortcut which retrieve the enrollment grade from its related LMS then check if it is passed. To prevent to spam LMS, the setting `JOANIE_ENROLLMENT_GRADE_CACHE_TTL` is used to store grade state in cache. By default, for 10 minutes.
Create a management command which aims to be called by a cron task to generate certificate at a regular interval. This command accepts three options (course, product and order) to restrict review to those resources.
django-object-actions allows us to add custom actions with admin change views.
f4324e5
to
7cf84f4
Compare
Create custom actions across all relevant models (products, course and orders) to generate certificates through the bunch of provided resources.
7cf84f4
to
7620e67
Compare
Purpose
Now that we are able to retrieve enrollment grade from LMS, we need to be able to generate certificates.
The purpose of this pull request is to add a management command to generate certificate at regular interval through a cron task but also to allow staff users to generate certificates on demand through custom admin actions.
Furthermore, staff users are able to flag course runs as gradable in order to prevent certificate generation.
We added actions to generate certificates into three models (Product, Course and Order). So in this way, staff users are able to generate certificates for:
one or several orders
one or several products
one or several courses
a course - product couple
Proposal
Related issue: #75