Skip to content

fix: make discovery service dependency optional for discussion notifications #37039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
asajjad2 wants to merge 1 commit into
base: master
Choose a base branch
from
+52 −8
Open

fix: make discovery service dependency optional for discussion notifications #37039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
12 changes: 12 additions & 0 deletions lms/envs/common.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -709,6 +709,18 @@
# .. setting_description: Sets the number of days after which the gradebook will freeze following the course's end.
GRADEBOOK_FREEZE_DAYS = 30

# .. setting_name: COURSE_ACCESS_DURATION_MIN_WEEKS
# .. setting_default: 4
# .. setting_description: Minimum course duration in weeks when Discovery service data is unavailable or course has no
# .. weeks_to_complete value. Used as fallback for course access duration calculations.
COURSE_ACCESS_DURATION_MIN_WEEKS = 4

# .. setting_name: COURSE_ACCESS_DURATION_MAX_WEEKS
# .. setting_default: 18
# .. setting_description: Maximum course duration in weeks. Course access duration is bounded by this upper limit
# .. regardless of Discovery service data.
COURSE_ACCESS_DURATION_MAX_WEEKS = 18

RETRY_CALENDAR_SYNC_EMAIL_MAX_ATTEMPTS = 5

############################# SET PATH INFORMATION #############################
Expand Down
30 changes: 22 additions & 8 deletions openedx/core/djangoapps/course_date_signals/utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,24 @@

from datetime import timedelta

from django.conf import settings
from openedx.core.djangoapps.catalog.utils import get_course_run_details
from openedx.core.djangoapps.catalog.models import CatalogIntegration

MIN_DURATION = timedelta(
weeks=getattr(settings, 'COURSE_DURATION_MIN_WEEKS', 4)
)
MAX_DURATION = timedelta(
weeks=getattr(settings, 'COURSE_DURATION_MAX_WEEKS', 18)
)
Copy link
Contributor

@pdpinch pdpinch Aug 25, 2025

Choose a reason for hiding this comment

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

Can you clarify in the PR description that MIN_DURATION and MAX_DURATION are currently hard-coded to 4 weeks and 18 weeks respectively, and that this PR makes them configurable?

I feel like this is going to require some input from the product working group, but having this context will help.

Copy link
Contributor Author

@asajjad2 asajjad2 Aug 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@pdpinch Updated in the additional changes section


MIN_DURATION = timedelta(weeks=4)
MAX_DURATION = timedelta(weeks=18)

def catalog_integration_enabled():
"""
Check if catalog integration is enabled
"""
catalog_integration = CatalogIntegration.current()
Copy link
Contributor

@pdpinch pdpinch Jul 17, 2025

Choose a reason for hiding this comment

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

I don't have an issue with this per se, but wouldn't it be clearer to test the setting that enables catalog integration?

Copy link
Contributor Author

@asajjad2 asajjad2 Jul 17, 2025

Choose a reason for hiding this comment

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

I only found this setting related to catalog existence on which we could place a check COURSE_CATALOG_API_URL. The setting has a default value that's always present even when Discovery isn't running, so checking it alone might incorrectly return True in broken configurations.

The CatalogIntegration.is_enabled() method, on the other hand is consistent with how other catalog functions determine enablement throughout the codebase and also seems to provides proper administrative control through the database.

return catalog_integration.is_enabled()


def get_expected_duration(course_id):
Expand All @@ -20,12 +33,13 @@ def get_expected_duration(course_id):

access_duration = MIN_DURATION

# The user course expiration date is the content availability date
# plus the weeks_to_complete field from course-discovery.
discovery_course_details = get_course_run_details(course_id, ['weeks_to_complete'])
expected_weeks = discovery_course_details.get('weeks_to_complete')
if expected_weeks:
access_duration = timedelta(weeks=expected_weeks)
if catalog_integration_enabled():
discovery_course_details = get_course_run_details(
course_id, ['weeks_to_complete']
)
expected_weeks = discovery_course_details.get('weeks_to_complete')
if expected_weeks:
access_duration = timedelta(weeks=expected_weeks)

# Course access duration is bounded by the min and max duration.
access_duration = max(MIN_DURATION, min(MAX_DURATION, access_duration))
Expand Down
11 changes: 11 additions & 0 deletions openedx/core/djangoapps/notifications/tests/test_filters.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
"""
from datetime import timedelta
from unittest import mock
from unittest.mock import patch

import ddt
from django.utils.timezone import now
Expand Down Expand Up @@ -43,6 +44,12 @@ class CourseExpirationTestCase(ModuleStoreTestCase):

def setUp(self):
super().setUp() # lint-amnesty, pylint: disable=super-with-arguments
self.catalog_patch = patch(
'openedx.core.djangoapps.catalog.models.CatalogIntegration.is_enabled',
return_value=True
)
self.catalog_patch.start()

self.course = CourseFactory(
start=now() - timedelta(weeks=10),
)
Expand All @@ -58,6 +65,10 @@ def setUp(self):
expired_audit.created = now() - timedelta(weeks=6)
expired_audit.save()

def tearDown(self):
self.catalog_patch.stop()
super().tearDown()

@mock.patch("openedx.core.djangoapps.course_date_signals.utils.get_course_run_details")
def test_audit_expired_filter_with_no_role(
self,
Expand Down
7 changes: 7 additions & 0 deletions openedx/features/course_duration_limits/tests/test_course_expiration.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

from datetime import timedelta
from unittest import mock
from unittest.mock import patch

import ddt
from django.conf import settings
Expand Down Expand Up @@ -46,6 +47,11 @@ class CourseExpirationTestCase(ModuleStoreTestCase, MasqueradeMixin):
"""Tests to verify the get_user_course_expiration_date function is working correctly"""
def setUp(self):
super().setUp() # lint-amnesty, pylint: disable=super-with-arguments
self.catalog_patch = patch(
'openedx.core.djangoapps.catalog.models.CatalogIntegration.is_enabled',
return_value=True
)
self.catalog_patch.start()
self.course = CourseFactory(
start=now() - timedelta(weeks=10),
)
Expand All @@ -72,6 +78,7 @@ def setUp(self):
add_course_mode(self.course)

def tearDown(self):
self.catalog_patch.stop()
CourseEnrollment.unenroll(self.user, self.course.id)
super().tearDown() # lint-amnesty, pylint: disable=super-with-arguments

Expand Down
Loading