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
5 changes: 3 additions & 2 deletions .github/workflows/ci.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ jobs:
fail-fast: false
matrix:
python-version: ['py312']
django-version: ['django42']
django-version: ['django42', 'django52']
db-version: ['mysql80']
pytest-split-group: [1, 2, 3, 4, 5, 6]
status: ['']
Expand Down Expand Up @@ -43,7 +43,8 @@ jobs:
# See https://github.com/actions/toolkit/issues/399
continue-on-error: ${{ matrix.status == 'ignored' }}
- name: Upload coverage
if: matrix.db-version == 'mysql80'
# Upload coverage only from the django52 job to avoid duplicate artifacts across the Django test matrix.
if: matrix.db-version == 'mysql80' && matrix.django-version == 'django52'
uses: actions/upload-artifact@v4
with:
name: coverage${{ matrix.pytest-split-group }}
Expand Down
3 changes: 2 additions & 1 deletion Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,8 @@ upgrade: $(COMMON_CONSTRAINTS_TXT)
pip-compile --rebuild --upgrade -o requirements/local.txt requirements/local.in
pip-compile --rebuild --upgrade -o requirements/production.txt requirements/production.in
# Let tox control the Django version for tests
grep -e "^django==" requirements/local.txt > requirements/django.txt
sed -i.tmp '/^[dD]jango==/d' requirements/test.txt
rm requirements/test.txt.tmp
sed -i.tmp '/^[dD]jango==/d' requirements/local.txt
rm -rf requirements/local.txt.tmp
chmod a+rw requirements/*.txt
Expand Down
6 changes: 3 additions & 3 deletions course_discovery/apps/api/fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,9 @@ def to_representation(self, value):
for size_key in value.field.variations:
# Get different sizes specs from the model field
# Then get the file path from the available files
sized_file = getattr(value, size_key, None)
if sized_file:
path = sized_file.url
if value and value.name:
variation_path = value.get_variation_name(value.name, size_key)
path = value.storage.url(variation_path)
serialized_image = serialized.setdefault(size_key, {})
# In case MEDIA_URL does not include scheme+host, ensure that the URLs are absolute and not relative
serialized_image['url'] = self.context['request'].build_absolute_uri(path)
Expand Down
7 changes: 6 additions & 1 deletion course_discovery/apps/api/tests/test_fields.py
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ def test_to_representation(self):
field._context = {'request': request} # pylint: disable=protected-access
expected = {
size_key: {
'url': '{}{}'.format('http://testserver', getattr(program.banner_image, size_key).url),
'url': '{}{}'.format(
'http://testserver',
program.banner_image.storage.url(
program.banner_image.get_variation_name(program.banner_image.name, size_key)
)
),
'width': program.banner_image.field.variations[size_key]['width'],
'height': program.banner_image.field.variations[size_key]['height']
} for size_key in program.banner_image.field.variations}
Expand Down
5 changes: 3 additions & 2 deletions course_discovery/apps/course_metadata/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -387,6 +387,9 @@ def save_model(self, request, obj, form, change):
obj.uploaded_by = request.user
super().save_model(request, obj, form, change)

@admin.display(
description='Task Result Status Summary'
)
def task_result_status_summary(self, obj):
"""
Returns a formatted string with the status and result of the task
Expand All @@ -400,8 +403,6 @@ def task_result_status_summary(self, obj):
except TaskResult.DoesNotExist:
return "No result found"

task_result_status_summary.short_description = 'Task Result Status Summary'


class CourseInline(admin.TabularInline):
model = Course
Expand Down
6 changes: 5 additions & 1 deletion course_discovery/apps/course_metadata/data_loaders/api.py
Original file line number Diff line number Diff line change
Expand Up @@ -446,7 +446,11 @@ def _load_ecommerce_data(self):
empty_course_run_type = CourseRunType.objects.get(slug=CourseRunType.EMPTY)
has_empty_type = (Q(type=empty_course_type, course_runs__seats__isnull=False) |
Q(course_runs__type=empty_course_run_type, course_runs__seats__isnull=False))
for course in Course.everything.filter(has_empty_type, partner=self.partner).distinct().iterator():
for course in (
Course.everything.filter(has_empty_type, partner=self.partner)
.distinct()
.iterator(chunk_size=settings.ITERATOR_CHUNK_SIZE)
):
if not calculate_course_type(course, commit=True):
logger.warning('Calculating course type failure occurred for [%s].', course)
self.processing_failure_occurred = True
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

https://django-simple-history.readthedocs.io/en/latest/utils.html#clean-duplicate-history
"""
from django.conf import settings
from django.core.management import CommandError
from django.utils import timezone
from simple_history.management.commands import clean_duplicate_history
Expand Down Expand Up @@ -100,5 +101,5 @@ def _process(self, to_process, date_back=None, dry_run=True):
pk__in=(m_qs.values_list(model._meta.pk.name).distinct())
)

for o in model_query.iterator():
for o in model_query.iterator(chunk_size=settings.ITERATOR_CHUNK_SIZE):
self._process_instance(o, model, stop_date=stop_date, dry_run=dry_run)
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,8 @@ def handle(self, *args, **options):
'editors',
).distinct()
logger.info(f'Found {courses_with_self_paced_runs.count()} courses with self-paced runs.')
courses_with_self_paced_runs = courses_with_self_paced_runs.iterator()
courses_with_self_paced_runs = courses_with_self_paced_runs.iterator(
chunk_size=settings.ITERATOR_CHUNK_SIZE)

for course in courses_with_self_paced_runs:
advertised_run = course.advertised_course_run
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ def setUp(self):
2,
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.course_type,
authoring_organizations=[self.organization]
)
Expand All @@ -55,7 +54,6 @@ def setUp(self):
2,
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
authoring_organizations=[self.organization],
card_image=factory.django.ImageField()
Expand Down Expand Up @@ -190,7 +188,6 @@ def test_populate_product_catalog_excludes_non_marketable_degrees(self):
DegreeFactory.create(
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Unpublished,
marketing_slug="valid-slug-1",
Expand All @@ -199,7 +196,6 @@ def test_populate_product_catalog_excludes_non_marketable_degrees(self):
DegreeFactory.create(
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Retired,
marketing_slug="valid-slug-2",
Expand All @@ -208,7 +204,6 @@ def test_populate_product_catalog_excludes_non_marketable_degrees(self):
DegreeFactory.create(
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Deleted,
marketing_slug="valid-slug-3",
Expand All @@ -217,7 +212,6 @@ def test_populate_product_catalog_excludes_non_marketable_degrees(self):
DegreeFactory.create(
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Active,
marketing_slug="",
Expand All @@ -228,7 +222,6 @@ def test_populate_product_catalog_excludes_non_marketable_degrees(self):
marketable_degree = DegreeFactory.create(
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Active,
marketing_slug="valid-marketing-slug",
Expand All @@ -239,7 +232,6 @@ def test_populate_product_catalog_excludes_non_marketable_degrees(self):
marketable_degree_with_no_language = DegreeFactory.create(
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Active,
marketing_slug="valid-marketing-slug",
Expand All @@ -252,7 +244,6 @@ def test_populate_product_catalog_excludes_non_marketable_degrees(self):
marketable_degree_2 = DegreeFactory.create(
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Active,
marketing_slug="valid-marketing-slug",
Expand Down Expand Up @@ -312,7 +303,6 @@ def test_populate_product_catalog_with_degrees_having_overrides(self):
degree = DegreeFactory.create(
product_source=self.source,
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Active,
marketing_slug="valid-marketing-slug",
Expand Down Expand Up @@ -353,7 +343,6 @@ def test_populate_product_catalog_supports_multiple_product_sources(self):
"""
marketable_degree = DegreeFactory.create(
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Active,
marketing_slug="valid-marketing-slug",
Expand All @@ -364,7 +353,6 @@ def test_populate_product_catalog_supports_multiple_product_sources(self):
)
marketable_degree_2 = DegreeFactory.create(
partner=self.partner,
additional_metadata=None,
type=self.program_type,
status=ProgramStatus.Active,
marketing_slug="valid-marketing-slug",
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import logging

from django.conf import settings
from django.core.management import BaseCommand, CommandError
from django.utils.translation import gettext as _

Expand All @@ -19,7 +20,8 @@ def handle(self, *args, **options):
# Since we know we will call unpublish_inactive_runs for nearly every single course in our catalog, let's
# try to optimize a little bit by only making one database query. We ask for all course runs, sort by course,
# then hand the set of published course runs into unpublish_inactive_runs.
published_runs = CourseRun.objects.filter(status=CourseRunStatus.Published).order_by('course').iterator()
published_runs = CourseRun.objects.filter(status=CourseRunStatus.Published).order_by(
'course').iterator(chunk_size=settings.ITERATOR_CHUNK_SIZE)

current_course = None
current_runs = set()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ def handle(self, *args, **options):
course_runs = course_runs.marketable()

# Reduce the memory usage
course_runs = course_runs.iterator()
course_runs = course_runs.iterator(chunk_size=settings.ITERATOR_CHUNK_SIZE)

for course_run in course_runs:
try:
Expand Down
9 changes: 5 additions & 4 deletions course_discovery/apps/course_metadata/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -1694,9 +1694,9 @@ def clean(self):

@property
def image_url(self):
if self.image:
return self.image.small.url

if self.image and self.image.name:
variation_path = self.image.get_variation_name(self.image.name, 'small')
return self.image.storage.url(variation_path)
return self.card_image_url

@property
Expand Down Expand Up @@ -1923,7 +1923,8 @@ def unpublish_inactive_runs(self, published_runs=None):
return False

if published_runs is None:
published_runs = self.course_runs.filter(status=CourseRunStatus.Published).iterator()
published_runs = self.course_runs.filter(status=CourseRunStatus.Published).iterator(
chunk_size=settings.ITERATOR_CHUNK_SIZE)
published_runs = frozenset(published_runs)

# Now separate out the active ones from the inactive
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -158,11 +158,11 @@ class Django:
"""

model = Course
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION

class Meta:
"""
Meta options.
"""

parallel_indexing = True
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION
Original file line number Diff line number Diff line change
Expand Up @@ -165,11 +165,11 @@ class Django:
"""

model = CourseRun
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION

class Meta:
"""
Meta options.
"""

parallel_indexing = True
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,11 @@ class Django:
"""

model = LearnerPathway
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION

class Meta:
"""
Meta options.
"""

parallel_indexing = True
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,11 @@ class Django:
"""

model = Person
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION
Copy link
Contributor Author

Choose a reason for hiding this comment

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

queryset_pagination was in Meta, which is ignored by django-elasticsearch-dsl. Moved it to the Django class as per the documentation.


class Meta:
"""
Meta options.
"""

parallel_indexing = True
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION
Original file line number Diff line number Diff line change
Expand Up @@ -136,11 +136,11 @@ class Django:
"""

model = Program
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION

class Meta:
"""
Meta options.
"""

parallel_indexing = True
queryset_pagination = settings.ELASTICSEARCH_DSL_QUERYSET_PAGINATION
2 changes: 1 addition & 1 deletion course_discovery/apps/course_metadata/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -186,7 +186,7 @@ def ensure_external_key_uniqueness__curriculum(sender, instance, **kwargs): # p
course_runs = CourseRun.objects.filter(
course__degree_course_curricula=instance,
external_key__isnull=False
).iterator()
).iterator(chunk_size=settings.ITERATOR_CHUNK_SIZE)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

In Django 5.0, QuerySet.iterator() requires an explicit chunk_size, especially when using prefetches. I set it to 2000 to match the previous default value used in earlier versions. Release Notes 5.0

check_curricula_and_related_programs_for_duplicate_external_key([instance], course_runs)


Expand Down
7 changes: 5 additions & 2 deletions course_discovery/apps/course_metadata/tests/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import ddt
import pytest
from bs4 import BeautifulSoup
from django import VERSION as DJANGO_VERSION
from django.contrib.admin.sites import AdminSite
from django.contrib.contenttypes.models import ContentType
from django.http import HttpRequest
Expand Down Expand Up @@ -485,7 +486,8 @@ def test_queryset_method_returns_eligible_programs(self):
""" Verify that one click purchase eligible programs pass the filter. """
verified_seat_type = factories.SeatTypeFactory.verified()
program_type = factories.ProgramTypeFactory(applicable_seat_types=[verified_seat_type])
program_filter = ProgramEligibilityFilter(None, {self.parameter_name: 1}, None, None)
value = [1] if DJANGO_VERSION >= (5, 2) else 1
program_filter = ProgramEligibilityFilter(None, {self.parameter_name: value}, None, None)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Django 5.0+ returns a tuple from SimpleListFilter.value(), even for single values.
Release note
This change in django 5.0 is for a bug fix discussed in the forums.

course_run = factories.CourseRunFactory(end=None, enrollment_end=None,)
factories.SeatFactory(course_run=course_run, type=verified_seat_type, upgrade_deadline=None)
one_click_purchase_eligible_program = factories.ProgramFactory(
Expand All @@ -498,7 +500,8 @@ def test_queryset_method_returns_eligible_programs(self):

def test_queryset_method_returns_ineligible_programs(self):
""" Verify programs ineligible for one-click purchase do not pass the filter. """
program_filter = ProgramEligibilityFilter(None, {self.parameter_name: 0}, None, None)
value = [0] if DJANGO_VERSION >= (5, 2) else 0
program_filter = ProgramEligibilityFilter(None, {self.parameter_name: value}, None, None)
one_click_purchase_ineligible_program = factories.ProgramFactory(one_click_purchase_enabled=False)
with self.assertNumQueries(4):
assert list(program_filter.queryset({}, Program.objects.all())) == [one_click_purchase_ineligible_program]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
from collections import ChainMap

import ddt
import factory
import pytest
from django.conf import settings
from django.contrib.sites.models import Site
Expand Down Expand Up @@ -32,6 +33,11 @@ class AlgoliaProxyProgramFactory(ProgramFactory):
class Meta:
model = AlgoliaProxyProgram

subscription = factory.RelatedFactory(
ProgramSubscriptionFactory,
factory_related_name='program',
)


class TestAlgoliaDataMixin():
ONE_MONTH_AGO = datetime.datetime.now(UTC) - datetime.timedelta(days=30)
Expand Down
2 changes: 1 addition & 1 deletion course_discovery/apps/course_metadata/tests/test_mixins.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ def test_wildcard_query_early_exit(self):
queryset = factories.CourseProxy.search(query=query)

self.assertEqual(len(queryset), self.total_courses)
self.assertQuerysetEqual(
self.assertQuerySetEqual(
queryset.order_by("id"),
factories.Course.objects.all().order_by("id"),
transform=lambda x: x
Expand Down
Loading
Loading