Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion course_discovery/apps/api/tests/test_utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,7 @@ def test_generate_data_for_studio_api_without_team(self):
mock_logger.assert_called_with(
'No course team admin specified for course [%s]. This may result in a Studio course run '
'being created without a course team.',
run.key.split('/')[1]
run.key.split('+')[1]
)

def test_calculate_course_run_key_run_value_with_multiple_runs_per_trimester(self):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ def setUp(self):
self.user = UserFactory(is_staff=True, is_superuser=True)
self.client.force_authenticate(self.user)
self.course = CourseFactory(partner=self.partner, key='simple_key')
self.course_run = CourseRunFactory(course=self.course)
self.course_run = CourseRunFactory(course=self.course, key='simple/key/run')
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried but failed to figure out how to properly escape a modern-style key for elasticsearch (presumably have to encode the colon and plus somehow), so I just used an old-style key instead. The point of the test is whether we can pass the key around, not the encoding specifically, so I just called it a day. No worse than before anyway.

self.url_base = reverse('api:v1:catalog-query_contains')
self.error_message = 'CatalogQueryContains endpoint requires query and identifiers list(s)'

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import datetime
import urllib
from unittest import mock
from urllib.parse import urlencode

import ddt
import pytest
Expand Down Expand Up @@ -1031,7 +1032,7 @@ def test_filter_by_keys(self):
CourseRun.objects.all().delete()
expected = CourseRunFactory.create_batch(3, course__partner=self.partner)
keys = ','.join([course.key for course in expected])
url = '{root}?keys={keys}'.format(root=reverse('api:v1:course_run-list'), keys=keys)
url = '{root}?{params}'.format(root=reverse('api:v1:course_run-list'), params=urlencode({'keys': keys}))
self.assert_list_results(url, expected)

def test_filter_by_marketable(self):
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import datetime
from unittest import mock
from urllib.parse import urlencode

import ddt
import pytest
Expand Down Expand Up @@ -269,7 +270,7 @@ def test_list_key_filter(self):
""" Verify the endpoint returns a list of courses filtered by the specified keys. """
courses = CourseFactory.create_batch(3, partner=self.partner)
keys = ','.join([course.key for course in courses])
url = '{root}?keys={keys}'.format(root=reverse('api:v1:course-list'), keys=keys)
url = '{root}?{params}'.format(root=reverse('api:v1:course-list'), params=urlencode({'keys': keys}))

with self.assertNumQueries(49):
response = self.client.get(url)
Expand Down
3 changes: 3 additions & 0 deletions course_discovery/apps/course_metadata/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@
from django_extensions.db.models import TimeStampedModel
from elasticsearch.exceptions import RequestError
from elasticsearch_dsl.query import Q as ESDSLQ
from opaque_keys.edx.keys import CourseKey
from parler.models import TranslatableModel, TranslatedFieldsModel
from simple_history.models import HistoricalRecords
from solo.models import SingletonModel
Expand Down Expand Up @@ -1983,6 +1984,8 @@ def could_be_marketable(self):
"""
if not self.type.is_marketable:
return False
if CourseKey.from_string(self.key).deprecated: # Old Mongo courses are not marketed
return False
Comment on lines +1987 to +1988
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the actual point of this PR - rest is test fixes to accomodate this.

return not self.draft

@property
Expand Down
4 changes: 2 additions & 2 deletions course_discovery/apps/course_metadata/tests/factories.py
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,7 @@ def course_run_types(self, create, extracted, **kwargs):

class CourseFactory(SalesforceRecordFactory):
uuid = factory.LazyFunction(uuid4)
key = FuzzyText(prefix='course-id/')
key = FuzzyText(prefix='course-id+')
key_for_reruns = FuzzyText(prefix='OrgX+')
title = FuzzyText(prefix="Test çօմɾʂҽ ")
short_description = FuzzyText(prefix="Test çօմɾʂҽ short description")
Expand Down Expand Up @@ -292,7 +292,7 @@ class Meta:
class CourseRunFactory(SalesforceRecordFactory):
status = CourseRunStatus.Published
uuid = factory.LazyFunction(uuid4)
key = FuzzyText(prefix='course-run-id/', suffix='/fake')
key = FuzzyText(prefix='course-v1:org+', suffix='+fake')
external_key = None
course = factory.SubFactory(CourseFactory)
title_override = None
Expand Down
13 changes: 7 additions & 6 deletions course_discovery/apps/course_metadata/tests/test_emails.py
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,7 @@ def assertEmailNotSent(self, function, reason):
emails.logger.name,
'INFO',
StringComparison('Not sending notification email for template course_metadata/email/.* because ' +
reason),
re.escape(reason)),
)
)

Expand Down Expand Up @@ -149,22 +149,23 @@ def test_send_email_for_internal_review(self):
restricted_url = self.partner.lms_admin_url.rstrip('/') + '/embargo/restrictedcourse/'
self.assertEmailSent(
emails.send_email_for_internal_review,
f'^Review requested: {self.course_run.key} - {self.course_run.title}$',
f'^Review requested: {re.escape(self.course_run.key)} - {self.course_run.title}$',
[self.pc],
both_regexes=[
'Dear %s,' % self.pc.full_name,
'MyOrg has submitted %s for review.' % self.course_run.key,
'MyOrg has submitted %s for review.' % re.escape(self.course_run.key),
],
html_regexes=[
'<a href="%s">View this course run in Publisher</a> to review the changes and mark it as reviewed.' %
self.publisher_url,
'This is a good time to <a href="%s">review this course run in Studio</a>.' % self.studio_url,
'This is a good time to <a href="%s">review this course run in Studio</a>.' %
re.escape(self.studio_url),
'Visit the <a href="%s">restricted course admin page</a> to set embargo rules for this course, '
'as needed.' % restricted_url,
],
text_regexes=[
'\n\nPublisher page: %s\n' % self.publisher_url,
'\n\nStudio page: %s\n' % self.studio_url,
'\n\nStudio page: %s\n' % re.escape(self.studio_url),
'\n\nRestricted Course admin: %s\n' % restricted_url,
],
)
Expand Down Expand Up @@ -225,7 +226,7 @@ def test_send_email_for_go_live(self):
**kwargs,
)
self.assertEmailContains(
subject=f'^Published: {self.course_run.key} - {self.course_run.title}$',
subject=f'^Published: {re.escape(self.course_run.key)} - {self.course_run.title}$',
to_users=[self.pc],
index=1,
**kwargs,
Expand Down
4 changes: 2 additions & 2 deletions course_discovery/apps/course_metadata/tests/test_lookups.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import json
from urllib.parse import quote
from urllib.parse import quote, urlencode

import pytest
from django.test import TestCase
Expand All @@ -17,7 +17,7 @@
class TestAutocomplete:
def assert_valid_query_result(self, client, path, query, expected_result):
""" Asserts a query made against the given endpoint returns the expected result. """
response = client.get(path + f'?q={query}')
response = client.get(path + f'?{urlencode({"q": query})}')
data = json.loads(response.content.decode('utf-8'))
assert len(data['results']) == 1
assert data['results'][0]['text'] == str(expected_result)
Expand Down
11 changes: 11 additions & 0 deletions course_discovery/apps/course_metadata/tests/test_models.py
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,17 @@ def test_could_be_marketable(self, draft, type_is_marketable, expected):
course_run.save()
assert mock_publish_obj.called == expected

@ddt.data(
('old/mongo/key', False),
('course-v1:modern+style+key', True),
)
@ddt.unpack
def test_old_mongo_not_marketable(self, key, expected):
course_run = factories.CourseRunFactory.create(key=key)
factories.SeatFactory.create(course_run=course_run)
assert course_run.is_marketable == expected
assert course_run.could_be_marketable == expected


class CourseRunTestsThatNeedSetUp(OAuth2Mixin, TestCase):
"""
Expand Down