From 5b18d142ab065e2ebaa59a4df043967e91e720fd Mon Sep 17 00:00:00 2001 From: zawan-ila <87228907+zawan-ila@users.noreply.github.com> Date: Tue, 10 Dec 2024 07:31:27 +0500 Subject: [PATCH 1/3] Update draft counts in Analytics loader (#4510) * fix: update draft counts in Analytics loader * chore: update dockerfile --- Dockerfile | 3 +- .../data_loaders/analytics_api.py | 11 ++++++- .../data_loaders/tests/test_analytics_api.py | 31 ++++++++++++++++--- 3 files changed, 37 insertions(+), 8 deletions(-) diff --git a/Dockerfile b/Dockerfile index 1def84d725..e1e391094f 100644 --- a/Dockerfile +++ b/Dockerfile @@ -27,8 +27,7 @@ RUN apt-get update && \ libcairo2-dev \ python3-pip \ python${PYTHON_VERSION} \ - python${PYTHON_VERSION}-dev \ - python${PYTHON_VERSION}-distutils && \ + python${PYTHON_VERSION}-dev &&\ rm -rf /var/lib/apt/lists/* # Use UTF-8. diff --git a/course_discovery/apps/course_metadata/data_loaders/analytics_api.py b/course_discovery/apps/course_metadata/data_loaders/analytics_api.py index 4b9cd83b79..e47612e12d 100644 --- a/course_discovery/apps/course_metadata/data_loaders/analytics_api.py +++ b/course_discovery/apps/course_metadata/data_loaders/analytics_api.py @@ -77,6 +77,11 @@ def _process_course_run_summary(self, course_run_summary): course_run.enrollment_count = course_run_count course_run.recent_enrollment_count = course_run_recent_count course_run.save(update_fields=['enrollment_count', 'recent_enrollment_count'], suppress_publication=True) + draft_run = course_run.draft_version + if draft_run: + draft_run.enrollment_count = course_run_count + draft_run.recent_enrollment_count = course_run_recent_count + draft_run.save(update_fields=['enrollment_count', 'recent_enrollment_count'], suppress_publication=True) # Add course run total to course total in dictionary if course.uuid in self.course_dictionary: @@ -92,7 +97,11 @@ def _process_course_enrollment_count(self, course, count, recent_count): course.enrollment_count = count course.recent_enrollment_count = recent_count course.save(update_fields=['enrollment_count', 'recent_enrollment_count']) - + draft_course = course.draft_version + if draft_course: + draft_course.enrollment_count = count + draft_course.recent_enrollment_count = recent_count + draft_course.save(update_fields=['enrollment_count', 'recent_enrollment_count']) # Add course count to program dictionary for all programs for program in course.programs.all(): # add course total to program total in dictionary diff --git a/course_discovery/apps/course_metadata/data_loaders/tests/test_analytics_api.py b/course_discovery/apps/course_metadata/data_loaders/tests/test_analytics_api.py index 1abd965c66..6023686920 100644 --- a/course_discovery/apps/course_metadata/data_loaders/tests/test_analytics_api.py +++ b/course_discovery/apps/course_metadata/data_loaders/tests/test_analytics_api.py @@ -8,6 +8,7 @@ from course_discovery.apps.course_metadata.data_loaders.tests.mixins import DataLoaderTestMixin from course_discovery.apps.course_metadata.models import Course, CourseRun, Program from course_discovery.apps.course_metadata.tests.factories import CourseFactory, CourseRunFactory, ProgramFactory +from course_discovery.apps.course_metadata.utils import ensure_draft_world class AnalyticsAPIDataLoaderTests(DataLoaderTestMixin, TestCase): @@ -48,18 +49,20 @@ def _define_course_metadata(self): program = ProgramFactory() program.courses.set(courses.values()) - @responses.activate - def test_ingest(self): - self._define_course_metadata() - + def _mock_course_summaries(self, data): url = f'{self.api_url}course_summaries/' responses.add( method=responses.GET, url=url, - body=json.dumps(self.mocked_data), + body=json.dumps(data), match_querystring=False, content_type=JSON ) + + @responses.activate + def test_ingest(self): + self._define_course_metadata() + self._mock_course_summaries(self.mocked_data) self.loader.ingest() # For runs, let's just confirm that enrollment counts were recorded and add up counts for courses @@ -92,3 +95,21 @@ def test_ingest(self): programs = Program.objects.all() assert programs[0].enrollment_count == expected_program_enrollment_count assert programs[0].recent_enrollment_count == expected_program_recent_enrollment_count + + @responses.activate + def test_draft_versions_updated(self): + course = CourseFactory(key='OrgX+CS100') + course_run = CourseRunFactory(key='course-v1:OrgX+CS100+Y', course=course) + ensure_draft_world(course_run) + + analytics_api_response = [{ + 'course_id': 'course-v1:OrgX+CS100+Y', + 'count': '528', + 'recent_count_change': '87' + }] + self._mock_course_summaries(analytics_api_response) + self.loader.ingest() + + for obj in [Course.objects.first(), CourseRun.objects.first()]: + assert obj.draft_version.enrollment_count == 528 + assert obj.draft_version.recent_enrollment_count == 87 From 695c9c9fba11dc9d81dc6fec66f04ebf9ed3bc15 Mon Sep 17 00:00:00 2001 From: zawan-ila <87228907+zawan-ila@users.noreply.github.com> Date: Tue, 10 Dec 2024 08:56:36 +0500 Subject: [PATCH 2/3] chore: remove constraint on pytest-xdist (#4504) --------- Co-authored-by: edX requirements bot <49161187+edx-requirements-bot@users.noreply.github.com> --- conftest.py | 30 +++++++++++++++++++ .../tests/test_add_provisioning_data.py | 4 +-- requirements/constraints.txt | 8 ----- requirements/local.txt | 12 ++++---- requirements/production.txt | 6 ++-- 5 files changed, 40 insertions(+), 20 deletions(-) diff --git a/conftest.py b/conftest.py index eefb72f624..8c210b571d 100644 --- a/conftest.py +++ b/conftest.py @@ -9,6 +9,7 @@ from django_elasticsearch_dsl.registries import registry from elasticsearch_dsl.connections import get_connection from pytest_django.lazy_django import skip_if_no_django +from xdist.scheduler import LoadScopeScheduling from course_discovery.apps.core.tests.factories import PartnerFactory, SiteFactory @@ -16,6 +17,31 @@ TEST_DOMAIN = 'testserver.fake' +# List of test classes that are backed by TransactionTestCase +TTC = ['course_discovery/apps/course_metadata/management/commands/tests/test_refresh_course_metadata.py::' + 'RefreshCourseMetadataCommandTests', + 'course_discovery/apps/course_metadata/tests/test_admin.py::ProgramAdminFunctionalTests'] + + +class LoadScopeSchedulingDjangoOrdered(LoadScopeScheduling): + # pylint: disable=abstract-method + + # Recent versions of pytest-xdist change the order of test execution such that TransactionTestCases may + # be run before TestCases. Since TransactionTestCase based tests do not restore the data created + # in data migrations during cleanup, this can cause TestCases which rely on that data to fail. + # pytest-xdist has an open issue for this regression at `https://github.com/pytest-dev/pytest-xdist/issues/1083` + + # We extend the LoadScopeScheduling class used by pytest-xdist to push the TransactionTestCases (in our test suites) + # to the end of the workqueue. This ensures the proper ordering of TransactionTestCases + def _assign_work_unit(self, node) -> None: + if not hasattr(self, 'django_ordered'): + self.django_ordered = True # pylint: disable=attribute-defined-outside-init + for test_class in TTC: + if test_class in self.workqueue: + self.workqueue.move_to_end(test_class) + + return super()._assign_work_unit(node) + @pytest.fixture(scope='session', autouse=True) def django_cache_add_xdist_key_prefix(request): @@ -108,3 +134,7 @@ def clear_es_indexes(): conn = get_connection() for index_name in settings.ELASTICSEARCH_INDEX_NAMES.values(): conn.indices.delete(index=index_name + '_*') + + +def pytest_xdist_make_scheduler(config, log): + return LoadScopeSchedulingDjangoOrdered(config, log) diff --git a/course_discovery/apps/course_metadata/management/commands/tests/test_add_provisioning_data.py b/course_discovery/apps/course_metadata/management/commands/tests/test_add_provisioning_data.py index 62db24cce6..ebe237e4c3 100644 --- a/course_discovery/apps/course_metadata/management/commands/tests/test_add_provisioning_data.py +++ b/course_discovery/apps/course_metadata/management/commands/tests/test_add_provisioning_data.py @@ -6,7 +6,7 @@ import responses from django.core.management import call_command -from django.test import TransactionTestCase +from django.test import TestCase from course_discovery.apps.api.v1.tests.test_views.mixins import OAuth2Mixin from course_discovery.apps.course_metadata.models import ( @@ -14,7 +14,7 @@ ) -class AddProvisioningDataCommandTests(TransactionTestCase, OAuth2Mixin): +class AddProvisioningDataCommandTests(TestCase, OAuth2Mixin): """ Test suite for add_provisioning_data management command. """ diff --git a/requirements/constraints.txt b/requirements/constraints.txt index bfc6554750..11a3de4732 100644 --- a/requirements/constraints.txt +++ b/requirements/constraints.txt @@ -55,14 +55,6 @@ selenium==4.12.0 # Open AI version 1.0.0 dropped support for openai.ChatCompletion which is currently in use in enterprise. openai<=0.28.1 -# Version 3.5.0 is causing some tests (learner_pathway models, test_add_provisioning_data, etc.) to fail -# all of a sudden. The v3.5.0 introduces some ordering changes in `loadscope` dist and that is causing an existing test(s) -# that do not clean up properly to cause failures. Pinning the version to unblock requirements upgrade. -# This is happening on some other open source repositories as well. -# https://github.com/PrefectHQ/prefect/pull/11229/files -# https://github.com/hyperspy/hyperspy/pull/3274 -pytest-xdist < 3.5.0 - # 5.4.0 is breaking for Python 3.8 and 3.11 CI checks with error # importlib.resources' has no attribute 'files' # To be unpinned once course-discovery moves to Python 3.12 diff --git a/requirements/local.txt b/requirements/local.txt index 2d47ea3abd..debc874ba7 100644 --- a/requirements/local.txt +++ b/requirements/local.txt @@ -64,9 +64,9 @@ boltons==21.0.0 # face # glom # semgrep -boto3==1.35.71 +boto3==1.35.72 # via django-ses -botocore==1.35.71 +botocore==1.35.72 # via # boto3 # s3transfer @@ -352,7 +352,7 @@ edx-api-doc-tools==2.0.0 # via -r requirements/base.in edx-auth-backends==4.4.0 # via -r requirements/base.in -edx-ccx-keys==1.3.0 +edx-ccx-keys==2.0.2 # via # -r requirements/base.in # openedx-events @@ -675,10 +675,8 @@ pytest-responses==0.5.1 # via -r requirements/test.in pytest-split==0.10.0 # via -r requirements/local.in -pytest-xdist==3.4.0 - # via - # -c requirements/constraints.txt - # -r requirements/test.in +pytest-xdist==3.6.1 + # via -r requirements/test.in python-dateutil==2.9.0.post0 # via # -r requirements/base.in diff --git a/requirements/production.txt b/requirements/production.txt index eed6f20740..24a9566e5b 100644 --- a/requirements/production.txt +++ b/requirements/production.txt @@ -41,9 +41,9 @@ beautifulsoup4==4.12.3 # taxonomy-connector billiard==4.2.1 # via celery -boto3==1.35.71 +boto3==1.35.72 # via django-ses -botocore==1.35.71 +botocore==1.35.72 # via # boto3 # s3transfer @@ -288,7 +288,7 @@ edx-api-doc-tools==2.0.0 # via -r requirements/base.in edx-auth-backends==4.4.0 # via -r requirements/base.in -edx-ccx-keys==1.3.0 +edx-ccx-keys==2.0.2 # via # -r requirements/base.in # openedx-events From d92ed15a17fdfd50478defc707de31ca6dc9ee29 Mon Sep 17 00:00:00 2001 From: Ali Akbar <52413434+Ali-D-Akbar@users.noreply.github.com> Date: Tue, 10 Dec 2024 11:25:12 +0500 Subject: [PATCH 3/3] fix: set legal and internal review to publish during CSVLoader (#4515) --- .../apps/course_metadata/data_loaders/csv_loader.py | 3 ++- .../course_metadata/data_loaders/tests/test_csv_loader.py | 7 ++++--- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/course_discovery/apps/course_metadata/data_loaders/csv_loader.py b/course_discovery/apps/course_metadata/data_loaders/csv_loader.py index e270a832b3..0474333bf8 100644 --- a/course_discovery/apps/course_metadata/data_loaders/csv_loader.py +++ b/course_discovery/apps/course_metadata/data_loaders/csv_loader.py @@ -268,7 +268,8 @@ def ingest(self): # pylint: disable=too-many-statements course_run.refresh_from_db() - if course_run.status in [CourseRunStatus.Unpublished, CourseRunStatus.LegalReview]: + if course_run.status in [CourseRunStatus.Unpublished, CourseRunStatus.LegalReview, + CourseRunStatus.InternalReview]: if course_run.status == CourseRunStatus.Unpublished: # Pushing the run into LegalReview is necessary to ensure that the # url slug is correctly generated in subdirectory format diff --git a/course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py b/course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py index 1701de9463..6eccdba06d 100644 --- a/course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py +++ b/course_discovery/apps/course_metadata/data_loaders/tests/test_csv_loader.py @@ -724,8 +724,9 @@ def test_ingest_flow_for_preexisting_unpublished_course(self, jwt_decode_patch): ) @responses.activate - def test_ingest_flow_for_preexisting_course_having_run_in_legal_review_status( - self, jwt_decode_patch + @data(CourseRunStatus.LegalReview, CourseRunStatus.InternalReview) + def test_ingest_flow_for_preexisting_course_having_run_in_review_statuses( + self, status, jwt_decode_patch ): # pylint: disable=unused-argument """ Verify that the course run will be reviewed if csv loader updates data for a course having a run in legal @@ -744,7 +745,7 @@ def test_ingest_flow_for_preexisting_course_having_run_in_legal_review_status( course=course, key=self.COURSE_RUN_KEY, type=self.course_run_type, - status=CourseRunStatus.LegalReview, + status=status, go_live_date=datetime.datetime.now(UTC) - datetime.timedelta(days=5), draft=True, fixed_price_usd=111.11