From e7a52de48d6ef243b744f221de7d5676c6735137 Mon Sep 17 00:00:00 2001 From: uzairr Date: Fri, 23 Jun 2023 22:15:48 +0500 Subject: [PATCH] feat: add url slug automation To make a consistent SEO experience, url slug generation strategy is modified.It will introduce its hook right after the course is transitioned into its Review. PROD-3395 --- course_discovery/apps/api/utils.py | 26 +++++++++- .../v1/tests/test_views/test_course_runs.py | 52 +++++++++++++++++-- .../apps/api/v1/views/course_runs.py | 10 +++- 3 files changed, 83 insertions(+), 5 deletions(-) diff --git a/course_discovery/apps/api/utils.py b/course_discovery/apps/api/utils.py index df420183cf..88bc1a2e1f 100644 --- a/course_discovery/apps/api/utils.py +++ b/course_discovery/apps/api/utils.py @@ -2,8 +2,10 @@ import functools import logging import math +import re from urllib.parse import parse_qsl, urlencode, urljoin +from django.conf import settings from django.core.files.base import ContentFile from django.db.models.fields.related import ManyToManyField from django.utils.translation import gettext as _ @@ -13,11 +15,33 @@ from course_discovery.apps.core.api_client.lms import LMSAPIClient from course_discovery.apps.core.utils import serialize_datetime -from course_discovery.apps.course_metadata.models import CourseRun +from course_discovery.apps.course_metadata.constants import SUBDIRECTORY_SLUG_FORMAT_REGEX +from course_discovery.apps.course_metadata.models import Course, CourseRun +from course_discovery.apps.course_metadata.utils import get_slug_for_course logger = logging.getLogger(__name__) +def set_subdirectory_slug_for_course(course_run): + """ + Sets the active url slug for draft and non-draft courses if the current + slug is not validated as per the new format. + """ + draft_course = Course.everything.get(key=course_run.course.key, draft=True) + is_slug_in_subdirectory_format = bool(re.match(SUBDIRECTORY_SLUG_FORMAT_REGEX, draft_course.active_url_slug)) + if not is_slug_in_subdirectory_format and draft_course.product_source.slug == settings.DEFAULT_PRODUCT_SOURCE_SLUG: + slug, error = get_slug_for_course(draft_course) + if slug: + draft_course.set_active_url_slug(slug) + if draft_course.official_version: + draft_course.official_version.set_active_url_slug(slug) + else: + raise Exception( # pylint: disable=broad-exception-raised + f"Slug generation Failed: unable to set active url slug for course: {draft_course.key} " + f"with error {error}" + ) + + def cast2int(value, name): """ Attempt to cast the provided value to an integer. diff --git a/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py b/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py index 1df0121fef..916c88ecd8 100644 --- a/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py +++ b/course_discovery/apps/api/v1/tests/test_views/test_course_runs.py @@ -9,6 +9,8 @@ import responses from django.contrib.auth.models import Group from django.db.models.functions import Lower +from django.test import override_settings +from edx_toggles.toggles.testutils import override_waffle_switch from freezegun import freeze_time from rest_framework.reverse import reverse from rest_framework.test import APIRequestFactory @@ -21,22 +23,26 @@ from course_discovery.apps.course_metadata.models import CourseRun, CourseRunType, Seat, SeatType from course_discovery.apps.course_metadata.tests.factories import ( CourseEditorFactory, CourseFactory, CourseRunFactory, CourseRunTypeFactory, CourseTypeFactory, OrganizationFactory, - PersonFactory, ProgramFactory, SeatFactory, SourceFactory, TrackFactory + PersonFactory, ProgramFactory, SeatFactory, SourceFactory, SubjectFactory, TrackFactory ) +from course_discovery.apps.course_metadata.toggles import IS_SUBDIRECTORY_SLUG_FORMAT_ENABLED +from course_discovery.apps.course_metadata.utils import is_valid_slug_format from course_discovery.apps.ietf_language_tags.models import LanguageTag from course_discovery.apps.publisher.tests.factories import OrganizationExtensionFactory +LOGGER_NAME = 'course_discovery.apps.api.utils' + @ddt.ddt class CourseRunViewSetTests(SerializationMixin, ElasticsearchTestMixin, OAuth2Mixin, APITestCase): def setUp(self): super().setUp() self.user = UserFactory(is_staff=True) - self.product_source = SourceFactory(name='test source') + self.product_source = SourceFactory(name='test source', slug='edx') self.client.force_authenticate(self.user) self.course_run = CourseRunFactory(course__partner=self.partner) self.course_run_2 = CourseRunFactory(course__key='Test+Course', course__partner=self.partner) - self.draft_course = CourseFactory(partner=self.partner, draft=True) + self.draft_course = CourseFactory(partner=self.partner, draft=True, product_source=self.product_source) self.draft_course_run = CourseRunFactory(course=self.draft_course, draft=True) self.draft_course_run.course.authoring_organizations.add(OrganizationFactory(key='course-id')) self.course_run_type = CourseRunTypeFactory(tracks=[TrackFactory()]) @@ -1279,3 +1285,43 @@ def test_change_status_and_ofac_info(self, patch_transaction): response = self.client.patch(url, patch_transaction['body'], format='json') assert response.status_code == patch_transaction['status_code'] + + @override_settings(DEFAULT_PRODUCT_SOURCE_SLUG='edx') + @ddt.data( + (IS_SUBDIRECTORY_SLUG_FORMAT_ENABLED, True), + (IS_SUBDIRECTORY_SLUG_FORMAT_ENABLED, False), + ) + @ddt.unpack + def test_url_restructuring_with_feature_flag_state(self, flag, state): + """ + Tests url slug restructuring must work under its relevant feature flag + """ + self.mock_patch_to_studio(self.draft_course_run.key) + course = self.draft_course_run.course + course.subjects.add(SubjectFactory(name='Subject1')) + + self.draft_course_run.status = CourseRunStatus.Unpublished + self.draft_course_run.save() + url = reverse('api:v1:course_run-detail', kwargs={'key': self.draft_course_run.key}) + body = { + 'course': self.draft_course_run.course.key, # required, so we need for a put + 'start': self.draft_course_run.start, # required, so we need for a put + 'end': self.draft_course_run.end, # required, so we need for a put + 'run_type': str(self.draft_course_run.type.uuid), # required, so we need for a put + 'draft': False, + } + + with override_waffle_switch(flag, active=state): + if state: + response = self.client.put(url, body, format='json') + draft_course_run = CourseRun.everything.get(key=self.draft_course_run.key, draft=True) + assert draft_course_run.course.active_url_slug.startswith('learn/') + else: + response = self.client.put(url, body, format='json') + draft_course_run = CourseRun.everything.get(key=self.draft_course_run.key, draft=True) + assert not draft_course_run.course.active_url_slug.startswith('learn/') + + assert is_valid_slug_format(draft_course_run.course.active_url_slug) + + assert response.status_code == 200, f"Status {response.status_code}: {response.content}" + assert draft_course_run.status == CourseRunStatus.LegalReview diff --git a/course_discovery/apps/api/v1/views/course_runs.py b/course_discovery/apps/api/v1/views/course_runs.py index e37ab2c3b4..1204564931 100644 --- a/course_discovery/apps/api/v1/views/course_runs.py +++ b/course_discovery/apps/api/v1/views/course_runs.py @@ -1,5 +1,6 @@ import logging +from django.conf import settings from django.db import models, transaction from django.db.models.functions import Lower from django.http.response import Http404 @@ -18,13 +19,16 @@ from course_discovery.apps.api.pagination import ProxiedPagination from course_discovery.apps.api.permissions import IsCourseRunEditorOrDjangoOrReadOnly from course_discovery.apps.api.serializers import MetadataWithRelatedChoices -from course_discovery.apps.api.utils import StudioAPI, get_query_param, reviewable_data_has_changed +from course_discovery.apps.api.utils import ( + StudioAPI, get_query_param, reviewable_data_has_changed, set_subdirectory_slug_for_course +) from course_discovery.apps.api.v1.exceptions import EditableAndQUnsupported from course_discovery.apps.core.utils import SearchQuerySetWrapper from course_discovery.apps.course_metadata.choices import CourseRunStatus from course_discovery.apps.course_metadata.constants import COURSE_RUN_ID_REGEX from course_discovery.apps.course_metadata.exceptions import EcommerceSiteAPIClientException from course_discovery.apps.course_metadata.models import Course, CourseEditor, CourseRun +from course_discovery.apps.course_metadata.toggles import IS_SUBDIRECTORY_SLUG_FORMAT_ENABLED from course_discovery.apps.course_metadata.utils import ensure_draft_world from course_discovery.apps.publisher.utils import is_publisher_user @@ -275,6 +279,7 @@ def _update_course_run(self, course_run, draft, changed, serializer, request, pr save_kwargs = {} # If changes are made after review and before publish, revert status to unpublished. # Unless we're just switching the status + product_source = course_run.course.product_source non_exempt_update = changed and course_run.status == CourseRunStatus.Reviewed if non_exempt_update: save_kwargs['status'] = CourseRunStatus.Unpublished @@ -286,6 +291,9 @@ def _update_course_run(self, course_run, draft, changed, serializer, request, pr # back into legal review if a non exempt field was changed (expected_program_name and expected_program_type) if not draft and (course_run.status == CourseRunStatus.Unpublished or non_exempt_update): save_kwargs['status'] = CourseRunStatus.LegalReview + if IS_SUBDIRECTORY_SLUG_FORMAT_ENABLED.is_enabled() and \ + product_source.slug == settings.DEFAULT_PRODUCT_SOURCE_SLUG: + set_subdirectory_slug_for_course(course_run) course_run = serializer.save(**save_kwargs)