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
10 changes: 10 additions & 0 deletions openedx/core/djangoapps/discussions/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
from openedx_events.learning.data import CourseDiscussionConfigurationData
from openedx_events.learning.signals import COURSE_DISCUSSIONS_CHANGED

from openedx.core.djangoapps.course_apps.models import CourseAppStatus
from openedx.core.djangoapps.discussions.models import (
DiscussionsConfiguration,
DiscussionTopicLink,
Expand Down Expand Up @@ -108,5 +109,14 @@ def update_course_discussion_config(configuration: CourseDiscussionConfiguration
context_key=course_key,
).update(enabled=configuration.enabled)

# Also update CourseAppStatus to keep the Pages & Resources UI in sync.
# The update_course_apps_status task may run before this handler due to
# the COURSE_PUBLISH_TASK_DELAY countdown, caching a stale enabled value.
Comment on lines +112 to +114
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have added the code here. We have 2 scenarios:

New course: No CourseAppStatus row exists yet. The handler creates one with the correct enabled value.
Old course: A CourseAppStatus row already exists (created by update_course_apps_status on a previous publish, or by initialize_course_app_status on first MFE page load). The handler updates it.

could you add an assertion for each of those scenarios into https://github.com/openedx/openedx-platform/blob/master/openedx/core/djangoapps/discussions/tests/test_handlers.py ?

CourseAppStatus.update_status_for_course_app(
course_key=course_key,
app_id="discussion",
enabled=configuration.enabled,
)

Comment thread
kdmccormick marked this conversation as resolved.

COURSE_DISCUSSIONS_CHANGED.connect(handle_course_discussion_config_update)
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
"""
Backfill DiscussionsConfiguration.enabled and CourseAppStatus.enabled for
existing courses to match CourseOverviewTab.is_hidden.

When a course is imported, the discussion tab's is_hidden value is carried over
from the source course. However, DiscussionsConfiguration.enabled and
CourseAppStatus.enabled default to True and are not updated from the imported
tab state, causing a desync.

This is a one-time backfill for existing courses. Going forward, the
update_course_discussion_config handler keeps these values in sync on every
course publish.
"""
Comment on lines +1 to +13
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

could you clarify that this is a backfill for existing courses, whereas the sync going forward is handled by update_course_discussion_config ?


from django.db import migrations


def sync_enabled_from_course_overview_tab(apps, schema_editor):
CourseOverviewTab = apps.get_model("course_overviews", "CourseOverviewTab")
Comment thread
kdmccormick marked this conversation as resolved.
DiscussionsConfiguration = apps.get_model("discussions", "DiscussionsConfiguration")
CourseAppStatus = apps.get_model("course_apps", "CourseAppStatus")

discussion_tabs = CourseOverviewTab.objects.filter(tab_id="discussion").select_related("course_overview")

for tab in discussion_tabs.iterator():
course_key = tab.course_overview_id
expected_enabled = not tab.is_hidden
DiscussionsConfiguration.objects.filter(
context_key=course_key,
).exclude(
enabled=expected_enabled,
).update(enabled=expected_enabled)
CourseAppStatus.objects.filter(
course_key=course_key,
app_id="discussion",
).exclude(
enabled=expected_enabled,
).update(enabled=expected_enabled)
Comment thread
kdmccormick marked this conversation as resolved.


class Migration(migrations.Migration):

dependencies = [
("discussions", "0018_auto_20230904_1054"),
("course_overviews", "0030_backfill_new_catalog_courseruns"),
("course_apps", "0002_alter_historicalcourseappstatus_options"),
]

operations = [
migrations.RunPython(sync_enabled_from_course_overview_tab, migrations.RunPython.noop),
]
37 changes: 37 additions & 0 deletions openedx/core/djangoapps/discussions/tests/test_handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
from opaque_keys.edx.keys import CourseKey
from openedx_events.learning.data import CourseDiscussionConfigurationData, DiscussionTopicContext

from openedx.core.djangoapps.course_apps.models import CourseAppStatus
from openedx.core.djangoapps.discussions.handlers import update_course_discussion_config
from openedx.core.djangoapps.discussions.models import DiscussionsConfiguration, DiscussionTopicLink

Expand Down Expand Up @@ -222,3 +223,39 @@ def test_existing_config_updated_enabled_from_configuration(self):
update_course_discussion_config(config_data)
db_config = DiscussionsConfiguration.objects.get(context_key=self.course_key)
assert db_config.enabled is False

def test_course_app_status_created_for_new_course(self):
"""
When no CourseAppStatus row exists yet (new course), the handler should
create one with the correct enabled value.
"""
new_key = CourseKey.from_string("course-v1:test+test+new_app_status")
assert not CourseAppStatus.objects.filter(course_key=new_key, app_id="discussion").exists()
config_data = CourseDiscussionConfigurationData(
course_key=new_key,
provider_type="openedx",
enabled=False,
plugin_configuration={},
)
update_course_discussion_config(config_data)
app_status = CourseAppStatus.objects.get(course_key=new_key, app_id="discussion")
assert app_status.enabled is False

def test_course_app_status_updated_for_existing_course(self):
"""
When a CourseAppStatus row already exists (old course), the handler
should update it with the correct enabled value.
"""
CourseAppStatus.objects.create(
course_key=self.course_key,
app_id="discussion",
enabled=True,
)
config_data = CourseDiscussionConfigurationData(
course_key=self.course_key,
provider_type="openedx",
enabled=False,
)
update_course_discussion_config(config_data)
app_status = CourseAppStatus.objects.get(course_key=self.course_key, app_id="discussion")
assert app_status.enabled is False
Loading