From 2f640202599b9ba4676cd8afabc6672ce6cf528d Mon Sep 17 00:00:00 2001 From: Jansen Kantor Date: Thu, 30 Mar 2023 12:01:51 -0400 Subject: [PATCH] feat: get info from catalog service (#32009) --- lms/djangoapps/courseware/tests/test_views.py | 152 ++++++++---------- lms/djangoapps/courseware/views/views.py | 93 +++++++---- 2 files changed, 123 insertions(+), 122 deletions(-) diff --git a/lms/djangoapps/courseware/tests/test_views.py b/lms/djangoapps/courseware/tests/test_views.py index c012047a25..45c969b7b1 100644 --- a/lms/djangoapps/courseware/tests/test_views.py +++ b/lms/djangoapps/courseware/tests/test_views.py @@ -28,7 +28,6 @@ from edx_toggles.toggles.testutils import override_waffle_flag from freezegun import freeze_time from opaque_keys.edx.keys import CourseKey, UsageKey from pytz import UTC -from openedx.core.djangoapps.content.course_overviews.tests.factories import CourseOverviewFactory from openedx.core.djangoapps.waffle_utils.models import WaffleFlagCourseOverrideModel from rest_framework import status from web_fragments.fragment import Fragment @@ -3098,41 +3097,6 @@ class TestRenderPublicVideoXBlock(TestBasePublicVideoXBlock): self.assertEqual(expected_status_code, response.status_code) self.assertEqual(expected_status_code, embed_response.status_code) - def test_get_org_logo_none(self): - # Given a course with no organizational logo - self.setup_course() - target_video = self.video_block_public - - # When I render the page - response = self.get_response(usage_key=target_video.location, is_embed=False) - content = response.content.decode('utf-8') - - # Then the page does not render an org logo - org_logo = re.search('', content) - self.assertIsNone(org_logo) - - @patch('lms.djangoapps.courseware.views.views.get_course_organization') - def test_get_org_logo(self, mock_get_org): - # Given a course with an organizational logo - self.setup_course() - target_video = self.video_block_public - - mock_org_logo_url = "/assets/foo" - mock_org_logo = MagicMock() - mock_org_logo.url = mock_org_logo_url - - mock_get_org.return_value = { - "logo": mock_org_logo - } - - # When I render the page - response = self.get_response(usage_key=target_video.location, is_embed=False) - content = response.content.decode('utf-8') - - # Then the page does render an org logo - org_logo = re.search(f'', content) - self.assertIsNotNone(org_logo) - class TestRenderXBlockSelfPaced(TestRenderXBlock): # lint-amnesty, pylint: disable=test-inherits-tests """ @@ -3537,6 +3501,7 @@ class TestPublicVideoXBlockView(TestBasePublicVideoXBlock): """Test Public Video XBlock View""" request = RequestFactory().get('/?utm_source=edx.org&utm_medium=referral&utm_campaign=video') base_block = PublicVideoXBlockView(request=request) + default_utm_params = {'utm_source': 'edx.org', 'utm_medium': 'referral', 'utm_campaign': 'video'} @contextmanager def mock_get_learn_more_url(self, **kwargs): @@ -3545,8 +3510,18 @@ class TestPublicVideoXBlockView(TestBasePublicVideoXBlock): PublicVideoXBlockView, 'get_learn_more_button_url', **kwargs - ) as mock_get_learn_more_url: - yield mock_get_learn_more_url + ) as mock_get_url: + yield mock_get_url + + @contextmanager + def mock_get_catalog_course_data(self, **kwargs): + """ Helper for mocking get_catalog_course_data """ + with patch.object( + PublicVideoXBlockView, + 'get_catalog_course_data', + **kwargs + ) as mock_get_data: + yield mock_get_data def test_get_template_and_context(self): """ @@ -3556,11 +3531,46 @@ class TestPublicVideoXBlockView(TestBasePublicVideoXBlock): fragment = MagicMock() with patch.object(self.video_block_public, "render", return_value=fragment): with self.mock_get_learn_more_url(): - template, context = self.base_block.get_template_and_context(self.course, self.video_block_public) + with self.mock_get_catalog_course_data(): + template, context = self.base_block.get_template_and_context(self.course, self.video_block_public) assert template == 'public_video.html' assert context['fragment'] == fragment assert context['course'] == self.course + @ddt.unpack + @ddt.data( + (None, None, {}), + ('uuid', None, {}), + ('uuid', {}, {'org_logo': None, 'marketing_url': None}), + ) + def test_get_catalog_course_data(self, mock_get_uuid, mock_get_data, expected_response): + self.setup_course() + with patch('lms.djangoapps.courseware.views.views.get_course_uuid_for_course', return_value=mock_get_uuid): + with patch('lms.djangoapps.courseware.views.views.get_course_data', return_value=mock_get_data): + assert self.base_block.get_catalog_course_data(self.course) == expected_response + + @ddt.unpack + @ddt.data( + ({}, None), + ({'marketing_url': 'www.somesite.com/this'}, 'www.somesite.com/this'), + ({'marketing_url': 'www.somesite.com/this?utm_source=jansen'}, 'www.somesite.com/this'), + ) + def test_get_catalog_course_marketing_url(self, input_data, expected_url): + url = self.base_block._get_catalog_course_marketing_url(input_data) + assert url == expected_url + + @ddt.unpack + @ddt.data( + ({}, None), + ({'owners': []}, None), + ({'owners': [{}]}, None), + ({'owners': [{'logo_image_url': 'somesite.org/image'}]}, 'somesite.org/image'), + ({'owners': [{'logo_image_url': 'firsturl'}, {'logo_image_url': 'secondurl'}]}, 'firsturl'), + ) + def test_get_catalog_course_owner_logo(self, input_data, expected_url): + url = self.base_block._get_catalog_course_owner_logo(input_data) + assert url == expected_url + @ddt.data("poster", None) def test_get_social_sharing_metadata(self, poster_url): """ @@ -3601,61 +3611,25 @@ class TestPublicVideoXBlockView(TestBasePublicVideoXBlock): url = self.base_block.build_url(base_url, params, utm_params) assert url == 'http://test.server?param1=value1¶m2=value2&utm_source=edx.org' - @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": False}) - def test_get_learn_more_button_url__marketing_not_enabled(self): + def assert_url_with_params(self, url, base_url, params): + if params: + assert url == base_url + '?' + urlencode(params) + else: + assert url == base_url + + @ddt.data({}, {'marketing_url': 'some_url'}) + def test_get_learn_more_button_url(self, catalog_course_info): """ - Test that when the marketing site isn't enabled, the learn more button - goes to the 'about_course' view + If we have a marketing url from the catalog service, use that. Otherwise + use the courseware about_course """ self.setup_course() - CourseOverviewFactory.create( - id=self.course.id, - ) - url = self.base_block.get_learn_more_button_url( - self.course, {} - ) - self.assertEqual( - url, - reverse('about_course', kwargs={'course_id': str(self.course.id)}) - ) - - @ddt.unpack - @ddt.data( - (True, False), - (False, True), - (True, True), - (True, False) - ) - @patch.dict(settings.FEATURES, {"ENABLE_MKTG_SITE": True}) - def test_get_learn_more_button_url(self, has_marketing_url, has_override): - """ - Test for learn more button url behavior when the marketing site is - enabled but considering overrides and missing marketing urls. - """ - self.setup_course() - utm_params = {'utm_source': 'askjeeves'} - overview = CourseOverviewFactory.create(id=self.course.id) - if has_marketing_url: - overview.marketing_url = 'mysite.com/this_course' - overview.save() - - url_overrides = {'someothercourse': 'someotherurl'} - overridden_url = "goto.this/site/instead" - if has_override: - url_overrides[str(self.course.id)] = overridden_url - - with override_settings(MKTG_URL_OVERRIDES=url_overrides): - url = self.base_block.get_learn_more_button_url(self.course, utm_params) - - if has_override: - expected_url = overridden_url - elif has_marketing_url: - expected_url = overview.marketing_url + url = self.base_block.get_learn_more_button_url(self.course, catalog_course_info, self.default_utm_params) + if 'marketing_url' in catalog_course_info: + expected_url = catalog_course_info['marketing_url'] else: expected_url = reverse('about_course', kwargs={'course_id': str(self.course.id)}) - - self.assertIn(expected_url, url) - self.assertIn(urlencode(utm_params), url) + self.assert_url_with_params(url, expected_url, self.default_utm_params) class TestPublicVideoXBlockEmbedView(TestBasePublicVideoXBlock): diff --git a/lms/djangoapps/courseware/views/views.py b/lms/djangoapps/courseware/views/views.py index 8b713a038c..c711165044 100644 --- a/lms/djangoapps/courseware/views/views.py +++ b/lms/djangoapps/courseware/views/views.py @@ -38,7 +38,6 @@ from markupsafe import escape from opaque_keys import InvalidKeyError from opaque_keys.edx.keys import CourseKey, UsageKey from openedx_filters.learning.filters import CourseAboutRenderStarted -from organizations.api import get_course_organization from pytz import UTC from requests.exceptions import ConnectionError, Timeout # pylint: disable=redefined-builtin from rest_framework import status @@ -102,7 +101,12 @@ from lms.djangoapps.instructor.enrollment import uses_shib from lms.djangoapps.instructor.views.api import require_global_staff from lms.djangoapps.survey import views as survey_views from lms.djangoapps.verify_student.services import IDVerificationService -from openedx.core.djangoapps.catalog.utils import get_programs, get_programs_with_type +from openedx.core.djangoapps.catalog.utils import ( + get_course_data, + get_course_uuid_for_course, + get_programs, + get_programs_with_type +) from openedx.core.djangoapps.content.course_overviews.models import CourseOverview from openedx.core.djangoapps.credit.api import ( get_credit_requirement_status, @@ -1787,13 +1791,13 @@ class PublicVideoXBlockView(BasePublicVideoXBlockView): fragment = video_block.render('public_view', context={ 'public_video_embed': False, }) - learn_more_url, enroll_url = self.get_public_video_cta_button_urls(course) + catalog_course_data = self.get_catalog_course_data(course) + learn_more_url, enroll_url = self.get_public_video_cta_button_urls(course, catalog_course_data) social_sharing_metadata = self.get_social_sharing_metadata(course, video_block) - org_logo = self.get_organization_logo_from_course(course) context = { 'fragment': fragment, 'course': course, - 'org_logo': org_logo, + 'org_logo': catalog_course_data.get('org_logo'), 'social_sharing_metadata': social_sharing_metadata, 'learn_more_url': learn_more_url, 'enroll_url': enroll_url, @@ -1806,15 +1810,44 @@ class PublicVideoXBlockView(BasePublicVideoXBlockView): } return 'public_video.html', context - def get_organization_logo_from_course(self, course): + def get_catalog_course_data(self, course): """ - Get organization logo for this course + Get information from the catalog service for this course """ - course_org = get_course_organization(course.id) + course_uuid = get_course_uuid_for_course(course.id) + if course_uuid is None: + return {} + catalog_course_data = get_course_data( + course_uuid, + ['owner', 'url_slug'], + ) + if catalog_course_data is None: + return {} - if course_org and course_org['logo']: - return course_org['logo'].url - return None + return { + 'org_logo': self._get_catalog_course_owner_logo(catalog_course_data), + 'marketing_url': self._get_catalog_course_marketing_url(catalog_course_data), + } + + def _get_catalog_course_marketing_url(self, catalog_course_data): + """ + Helper to extract url and remove any potential utm queries. + The discovery API includes UTM info unless you request it to not be included. + The request for the UUIDs will cache the response within the LMS so we need + to strip it here. + """ + marketing_url = catalog_course_data.get('marketing_url') + if marketing_url is None: + return marketing_url + url_parts = urlparse(marketing_url) + return self._replace_url_query(url_parts, {}) + + def _get_catalog_course_owner_logo(self, catalog_course_data): + """ Helper to safely extract the course owner image url from the catalog course """ + owners_data = catalog_course_data.get('owners', []) + if len(owners_data) == 0: + return None + return owners_data[0].get('logo_image_url', None) def get_social_sharing_metadata(self, course, video_block): """ @@ -1836,32 +1869,23 @@ class PublicVideoXBlockView(BasePublicVideoXBlockView): ) } - def get_learn_more_button_url(self, course, utm_params): + def get_learn_more_button_url(self, course, catalog_course_data, utm_params): """ If the marketing site is enabled and a course has a marketing page, use that URL. If not, point to the `about_course` view. Override all with the MKTG_URL_OVERRIDES setting. """ - course_key = str(course.id) - course_overview = CourseOverview.get_from_id(course.id) - if course_overview.has_marketing_url(): - base_url = course_overview.marketing_url - else: - base_url = reverse('about_course', kwargs={'course_id': course_key}) - - marketing_url_overrides = configuration_helpers.get_value( - 'MKTG_URL_OVERRIDES', - settings.MKTG_URL_OVERRIDES - ) - base_url = marketing_url_overrides.get(course_key, base_url) + base_url = catalog_course_data.get('marketing_url', None) + if base_url is None: + base_url = reverse('about_course', kwargs={'course_id': str(course.id)}) return self.build_url(base_url, {}, utm_params) - def get_public_video_cta_button_urls(self, course): + def get_public_video_cta_button_urls(self, course, catalog_course_data): """ Get the links for the 'enroll' and 'learn more' buttons on the public video page """ utm_params = self.get_utm_params() - learn_more_url = self.get_learn_more_button_url(course, utm_params) + learn_more_url = self.get_learn_more_button_url(course, catalog_course_data, utm_params) enroll_url = self.build_url( reverse('register_user'), { @@ -1889,15 +1913,18 @@ class PublicVideoXBlockView(BasePublicVideoXBlockView): """ if not params and not utm_params: return base_url - url_parts = urlparse(base_url) + parsed_url = urlparse(base_url) full_params = {**params, **utm_params} + return self._replace_url_query(parsed_url, full_params) + + def _replace_url_query(self, parsed_url, query): return urlunparse(( - url_parts.scheme, - url_parts.netloc, - url_parts.path, - url_parts.params, - urlencode(full_params), - url_parts.fragment + parsed_url.scheme, + parsed_url.netloc, + parsed_url.path, + parsed_url.params, + urlencode(query) if query else '', + parsed_url.fragment ))