feat: filter out unenrollment and update filter order in Learner Prog… #643
Conversation
| def filter_search_enrollment(self, queryset, search_enrollment): | ||
| now=timezone.localdate() | ||
|
|
||
| if search_enrollment == 'enrolled': | ||
| return queryset.filter( | ||
| Q(unenrollment_date__isnull=True) | Q(unenrollment_date__gt=now) | ||
| ) | ||
| elif search_enrollment == 'unenrolled': | ||
| return queryset.filter(unenrollment_date__lte=now) | ||
| return queryset | ||
|
|
There was a problem hiding this comment.
As per my understanding, unenrollment_date will never be in the future. So we can simplify the code as shown below. I believe this will also simplify the unit tests.
def filter_search_enrollment(self, queryset, status):
"""
Filter enrollments based on enrollment `status`.
Args:
status (str): Enrollment status to filter by. Can be one of:
'enrolled' : unenrollment_date is NULL (currently enrolled)
'unenrolled' : unenrollment_date is NOT NULL (no longer enrolled)
Returns:
QuerySet: Filtered queryset of enrollments.
"""
if status == "enrolled":
return queryset.filter(unenrollment_date__isnull=True)
if status == "unenrolled":
return queryset.filter(unenrollment_date__isnull=False)
return queryset
muhammad-ammar
left a comment
muhammad-ammar
left a comment
There was a problem hiding this comment.
Good Work. One last thing. For every PR, you also need make changes in CHANGELOG.rst and https://github.com/openedx/edx-enterprise-data/blob/master/enterprise_data/__init__.py
Please see #641 to understand what changes are needed in the files mentioned above.
| ) | ||
|
|
||
| self.assertEqual(response.status_code, 200) | ||
| self.assertEqual(response.json()["count"], 2) No newline at end of file |
There was a problem hiding this comment.
Please add a new line at the end of file.
| super().tearDown() | ||
| EnterpriseLearnerEnrollment.objects.all().delete() | ||
|
|
||
| def _get_url(self): |
There was a problem hiding this comment.
I think instead of creating this method, we can declare a variable in setUp as shown below
self.url = reverse(
'v1:enterprise-learner-enrollment-list',
kwargs={'enterprise_id': self.enterprise_id}
)| # ----------------------------- | ||
| # 1. Test for enrolled learners | ||
| # ----------------------------- |
There was a problem hiding this comment.
The standard practice is to add docstrings in test functions. Please add docstrings in all tests and remove these comments.
For example,
def test_filter_enrolled(self):
"""
Test that the enrollments API returns the correct data when filtered by the 'enrolled' status.
"""
| enterprise_learner = EnterpriseLearnerFactory( | ||
| enterprise_customer_uuid=self.enterprise_id | ||
| ) | ||
|
|
||
| enrolled = EnterpriseLearnerEnrollmentFactory( | ||
| enterprise_user=enterprise_learner, | ||
| enterprise_customer_uuid=self.enterprise_id, | ||
| unenrollment_date=None, | ||
| ) | ||
|
|
||
| # Create unenrolled learner (unenrollment_date != NULL) | ||
| EnterpriseLearnerEnrollmentFactory( | ||
| enterprise_user=enterprise_learner, | ||
| enterprise_customer_uuid=self.enterprise_id, | ||
| unenrollment_date=timezone.now(), | ||
| ) |
There was a problem hiding this comment.
In all tests, we are creating data inside the tests, it would be great if we can move all the test data creation inside the setUp method.
enterprise_data/tests/test_utils.py
Outdated
| letter_grade = factory.lazy_attribute(lambda x: ' '.join(FAKER.words(nb=2)).title()) | ||
| progress_status = factory.lazy_attribute(lambda x: ' '.join(FAKER.words(nb=2)).title()) | ||
| enterprise_user_id = factory.Sequence(lambda n: n) | ||
| #enterprise_user_id = factory.Sequence(lambda n: n) |
There was a problem hiding this comment.
Looks like this was left by mistake?
enterprise_data/tests/test_utils.py
Outdated
| enterprise_user = factory.SubFactory( | ||
| EnterpriseLearnerFactory, | ||
| enterprise_user_id=factory.Sequence(lambda n: n+1) | ||
| ) |
There was a problem hiding this comment.
Could you please explain why we need to make changes in this file?
There was a problem hiding this comment.
The previous one was giving an error. FOREIGN KEY constraint failed when I was running Unit test cases.
sjasti-sonata-svg
force-pushed
the
subhashini/ENT-10811
branch
9 times, most recently
from
1035350 to
304fbae
Compare
sjasti-sonata-svg
force-pushed
the
subhashini/ENT-10811
branch
2 times, most recently
from
23cda29 to
9627c87
Compare
…ress Report correcting CHANGELOG.rst version number Fixed issues for checks failing. fixing issues for failing tests. Fixing test failures fixing whitespaces fixing issuse fixing qulity issues
sjasti-sonata-svg
force-pushed
the
subhashini/ENT-10811
branch
from
a52e5ea to
f9df750
Compare
2 participants
…ress Report
**This PR introduces a new unenrollment filter to the Learner Progress Report (LPR) and updates the ordering of existing filters.
Added Unenrollment option(Enrolled and UnEnrolled) to LPR filters.