-
Notifications
You must be signed in to change notification settings - Fork 1
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
Jlc/certificates metrics #96
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,15 +6,19 @@ | |
get_learners_metric: Return number of learners, for the visible courses. | ||
get_instructors_metric: Return number of instructors, for the visible courses. | ||
get_courses_metrics: Return metrics for the visible courses. | ||
get_course_certificates_metric: Return a dict metric representin the certificates of a course. | ||
""" | ||
from django.conf import settings | ||
from eox_core.edxapp_wrapper.certificates import get_generated_certificate | ||
|
||
from eox_nelp.edxapp_wrapper.branding import get_visible_courses | ||
from eox_nelp.edxapp_wrapper.modulestore import modulestore | ||
from eox_nelp.edxapp_wrapper.site_configuration import configuration_helpers | ||
from eox_nelp.edxapp_wrapper.student import CourseAccessRole, CourseEnrollment | ||
from eox_nelp.stats.decorators import cache_method | ||
|
||
GeneratedCertificate = get_generated_certificate() | ||
|
||
|
||
@cache_method | ||
def get_cached_courses(tenant): # pylint: disable=unused-argument | ||
|
@@ -70,6 +74,7 @@ def get_course_metrics(course_key): | |
user__is_staff=False, | ||
user__is_superuser=False | ||
).values('user').distinct().count() | ||
certificates = get_course_certificates_metric(course_key) | ||
|
||
return { | ||
"id": str(course_key), | ||
|
@@ -79,7 +84,8 @@ def get_course_metrics(course_key): | |
"sections": len(chapters), | ||
"sub_sections": len(sequentials), | ||
"units": len(verticals), | ||
"components": components | ||
"components": components, | ||
"certificates": certificates, | ||
} | ||
|
||
|
||
|
@@ -134,3 +140,52 @@ def get_courses_metrics(tenant): | |
metrics = [get_course_metrics(course.id) for course in courses] | ||
|
||
return {"total_courses": courses.count(), "metrics": metrics} | ||
|
||
|
||
def get_course_certificates_metric(course_key): | ||
""" | ||
Returns the total of certificates in a course. | ||
Args: | ||
course_key<opaque-key>: Course identifier. | ||
Return: | ||
<Dictionary>: Contains certificates of course metric. | ||
{ | ||
"verified": {...}, | ||
"honor": {...}, | ||
"audit": {}, | ||
"professional": {...}, | ||
"no-id-professional": { | ||
"downloadable": 5, | ||
"notpassing": 4, | ||
}, | ||
"masters": {...}, | ||
"executive-education": {...} | ||
"paid-executive-education":{...}, | ||
"paid-bootcamp": {...}, | ||
"total": 0 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. since you are splitting the certificates based on their status the total metric should reflect also that division There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you explain this one a little more, I mean I am setting the total without the division. "total" : {
"no-id-professional": 5,
"audit": 5
} Is that what you say? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I didn't say mode
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
@andrey-canon what about this approach ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. did you remove the modes ? |
||
} | ||
""" | ||
certificates = {} | ||
course_certificates_qs = GeneratedCertificate.objects.filter( | ||
course_id=course_key, | ||
) | ||
cert_statuses = [cert["status"] for cert in course_certificates_qs.values("status").distinct()] | ||
|
||
for mode in GeneratedCertificate.MODES: | ||
db_mode = mode[0] | ||
human_mode = mode[1] | ||
certificates[human_mode] = { | ||
cert_status: course_certificates_qs.filter( | ||
mode=db_mode, | ||
status=cert_status, | ||
).values("user").distinct().count() | ||
for cert_status in cert_statuses | ||
} | ||
certificates["total"] = { | ||
cert_status: course_certificates_qs.filter(status=cert_status).values("user").distinct().count() | ||
for cert_status in cert_statuses | ||
} | ||
|
||
return certificates |
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.
why are you creating a a backend if you are creating the model ?
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.
Because the backend allows me to load the class that passes the data to the test db.
eox-nelp/eox_nelp/stats/tests/tests_metrics.py
Line 273 in f398520
eox-nelp/eox_nelp/edxapp_wrapper/test_backends/certificates_m_v1.py
Lines 32 to 58 in f398520
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.
generally we don't duplicate models from edx-platform, why this case is so especial ?
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.
Actually is because in the test I use the model to generate the certificates in test
db
. So the mainly reason is that, as you can see there course_overview also was recreated, but with less fieldseox-nelp/eox_nelp/migrations/0001_initial.py
Lines 83 to 97 in f398520
The reasons is that I want to test against like "
real certs data"
...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 in the test, as you can see I don't mock any function of the certificates because the django test db has the model and data(
GeneratedCertficates
). So functions like GeneratedCertficates.objects... work and return data from test db.eox-nelp/eox_nelp/stats/tests/tests_metrics.py
Lines 273 to 296 in f398520
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.
Course overview was created because another model use that as foreign key, there was no another option
IMO there are another alternatives like Mock that could fit here without adding fields that you even don't use
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.
Actually, I started trying to use the mock approach, therefore as I used different Django queryset attributes and mixed them, I found that I have to do a lot of different mocks approaches and also with sense. So I stated to be a little mixed, so finally, I preferred to load the model and get data as the django queryset would work.
eox-nelp/eox_nelp/stats/tests/tests_metrics.py
Lines 257 to 269 in f398520
Also I set another fields, maybe extra but that could be useful if others created a certificate in the django db test model.
Actually all was loaded from here. Practically I have the same model that the openedx
https://github.com/openedx/edx-platform/blob/master/lms/djangoapps/certificates/models.py#L224-L240
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.
That's the point we can not copy all the models that openedx use