Merge branch 'master' into hamza/PROD-4297

CODEOWNERS

@@ -1,2 +1 @@
 # The following users are the owners of all course-discovery files
-*       @edx/course-discovery-admins

catalog-info.yaml

@@ -18,7 +18,7 @@ metadata:
     openedx.org/arch-interest-groups: ""
     openedx.org/release: "master" 
 spec:
-  owner: group:course-discovery-admins
+  owner: group:2u-phoenix
   type: 'service'
   lifecycle: 'production'
   dependsOn:

course_discovery/apps/api/serializers.py

@@ -2372,6 +2372,7 @@ class Meta:
             'email',
             'programs',
             'description',
+            'status',
             'destination_url',
             'pathway_type',
             'course_run_statuses',

course_discovery/apps/api/tests/test_serializers.py

@@ -1588,6 +1588,7 @@ def test_data(self):
             'destination_url': pathway.destination_url,
             'pathway_type': pathway.pathway_type,
             'course_run_statuses': [],
+            'status': 'unpublished'
         }
         self.assertDictEqual(serializer.data, expected)
 

course_discovery/apps/api/v1/tests/test_views/test_pathways.py

@@ -5,6 +5,7 @@
 
 from course_discovery.apps.api.v1.tests.test_views.mixins import SerializationMixin
 from course_discovery.apps.core.tests.factories import USER_PASSWORD, UserFactory
+from course_discovery.apps.course_metadata.choices import PathwayStatus
 from course_discovery.apps.course_metadata.tests.factories import PathwayFactory, ProgramFactory
 
 
@@ -36,13 +37,16 @@ def setup(self, client, django_assert_num_queries, partner):
         self.partner = partner
         self.request = request
 
+    def create_pathway(self, status=PathwayStatus.Unpublished):
+        pathway = PathwayFactory(partner=self.partner, status=status)
+        program = ProgramFactory(partner=pathway.partner)
+        pathway.programs.add(program)
+        return pathway
+
     def test_pathway_list(self):
         pathways = []
         for _ in range(4):
-            pathway = PathwayFactory(partner=self.partner)
-            program = ProgramFactory(partner=pathway.partner)
-            pathway.programs.add(program)
-            pathways.append(pathway)
+            pathways.append(self.create_pathway())
         response = self.client.get(self.list_path)
         assert response.status_code == 200
         assert response.data['results'] == self.serialize_pathway(pathways, many=True)
@@ -57,3 +61,22 @@ def test_only_matching_partner(self):
         response = self.client.get(self.list_path)
         assert response.status_code == 200
         assert response.data['results'] == self.serialize_pathway([pathway], many=True)
+
+    @pytest.mark.parametrize("status", [PathwayStatus.Unpublished, PathwayStatus.Published, PathwayStatus.Retired])
+    def test_status_filtering(self, status):
+        published_pathway = self.create_pathway(status=PathwayStatus.Published)
+        unpublished_pathway = self.create_pathway(status=PathwayStatus.Unpublished)
+        retired_pathway = self.create_pathway(status=PathwayStatus.Retired)
+        pathways = [published_pathway, unpublished_pathway, retired_pathway]
+
+        # Simple get returns all Pathways
+        response = self.client.get(self.list_path)
+        assert response.status_code == 200
+        assert response.data["count"] == 3
+        assert response.data['results'] == self.serialize_pathway(pathways, many=True)
+
+        # Adding a query param filters the results
+        response = self.client.get(self.list_path + f'?status={status}')
+        assert response.status_code == 200
+        assert response.data["count"] == 1
+        assert response.data['results'] == self.serialize_pathway([locals()[f"{status}_pathway"]], many=True)

course_discovery/apps/api/v1/views/pathways.py

@@ -1,4 +1,5 @@
 """ Views for accessing Pathway data """
+from django_filters.rest_framework import DjangoFilterBackend
 from rest_framework import viewsets
 
 from course_discovery.apps.api import serializers
@@ -11,6 +12,8 @@
 class PathwayViewSet(CompressedCacheResponseMixin, viewsets.ReadOnlyModelViewSet):
     permission_classes = (ReadOnlyByPublisherUser,)
     serializer_class = serializers.PathwaySerializer
+    filter_backends = (DjangoFilterBackend,)
+    filterset_fields = ('status',)
 
     def get_queryset(self):
         excluded_restriction_types = get_excluded_restriction_types(self.request)

course_discovery/apps/course_metadata/choices.py

@@ -18,6 +18,12 @@ def REVIEW_STATES(cls):
         return [cls.LegalReview.value, cls.InternalReview.value]
 
 
+class PathwayStatus(models.TextChoices):
+    Unpublished = 'unpublished', _('Unpublished')
+    Published = 'published', _('Published')
+    Retired = 'retired', _('Retired')
+
+
 class CourseRunPacing(models.TextChoices):
     # Translators: Instructor-paced refers to course runs that operate on a schedule set by the instructor,
     # similar to a normal university course.

course_discovery/apps/course_metadata/migrations/0347_pathway_status.py

@@ -0,0 +1,28 @@
+# Generated by Django 4.2.18 on 2025-02-17 15:50
+
+from django.db import migrations, models
+
+from course_discovery.apps.course_metadata.choices import PathwayStatus
+
+
+def set_status_on_existing_pathways(apps, schema_editor):
+    # For all existing pathways, we set the status to Published.
+    # Any deviations from that should be handled manually.
+    Pathway = apps.get_model('course_metadata', 'Pathway')
+    Pathway.objects.update(status=PathwayStatus.Published)
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('course_metadata', '0346_archivecoursesconfig'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='pathway',
+            name='status',
+            field=models.CharField(choices=[('unpublished', 'Unpublished'), ('published', 'Published'), ('retired', 'Retired')], default='unpublished', max_length=255),
+        ),
+        migrations.RunPython(set_status_on_existing_pathways, migrations.RunPython.noop)
+    ]

course_discovery/apps/course_metadata/models.py

@@ -44,7 +44,7 @@
 from course_discovery.apps.course_metadata import emails
 from course_discovery.apps.course_metadata.choices import (
     CertificateType, CourseLength, CourseRunPacing, CourseRunRestrictionType, CourseRunStatus,
-    ExternalCourseMarketingType, ExternalProductStatus, PayeeType, ProgramStatus, ReportingType
+    ExternalCourseMarketingType, ExternalProductStatus, PathwayStatus, PayeeType, ProgramStatus, ReportingType
 )
 from course_discovery.apps.course_metadata.constants import SUBDIRECTORY_SLUG_FORMAT_REGEX, PathwayType
 from course_discovery.apps.course_metadata.fields import AutoSlugWithSlashesField, HtmlField, NullHtmlField
@@ -4371,6 +4371,11 @@ class Pathway(TimeStampedModel):
         choices=[(tag.value, tag.value) for tag in PathwayType],
         default=PathwayType.CREDIT.value,
     )
+    status = models.CharField(
+        default=PathwayStatus.Unpublished,
+        max_length=255, null=False, blank=False,
+        choices=PathwayStatus.choices
+    )
 
     def __str__(self):
         return self.name

course_discovery/apps/course_metadata/tests/test_admin.py

@@ -23,7 +23,7 @@
 from course_discovery.apps.core.tests.factories import USER_PASSWORD, PartnerFactory, UserFactory
 from course_discovery.apps.core.tests.helpers import make_image_file
 from course_discovery.apps.course_metadata.admin import DegreeAdmin, PositionAdmin, ProgramEligibilityFilter
-from course_discovery.apps.course_metadata.choices import ProgramStatus
+from course_discovery.apps.course_metadata.choices import PathwayStatus, ProgramStatus
 from course_discovery.apps.course_metadata.constants import PathwayType
 from course_discovery.apps.course_metadata.forms import PathwayAdminForm, ProgramAdminForm
 from course_discovery.apps.course_metadata.models import (
@@ -628,6 +628,7 @@ def test_program_with_same_partner(self):
             'email': '[email protected]',
             'programs': [program1.id],
             'pathway_type': PathwayType.CREDIT.value,
+            'status': PathwayStatus.Published
         }
         form = PathwayAdminForm(data=data)
 
@@ -650,6 +651,7 @@ def test_program_with_different_partner(self):
             'email': '[email protected]',
             'programs': [program1.id, program2.id],
             'pathway_type': PathwayType.INDUSTRY.value,
+            'status': PathwayStatus.Unpublished
         }
         form = PathwayAdminForm(data=data)
 

course_discovery/apps/course_metadata/utils.py

@@ -784,8 +784,6 @@ def clean_html(content):
         is_list_with_dir_attr_present = True
 
     cleaned = str(soup)
-    # Need to re-replace the · middot with the entity so that html2text can transform it to * for <ul> in markdown
-    cleaned = cleaned.replace('·', '&middot;')
     # Need to clean empty <b> and <p> tags which are converted to <hr/> by html2text
     cleaned = cleaned.replace('<p><b></b></p>', '')
     html_converter = HTML2TextWithLangSpans(bodywidth=None)

course_discovery/apps/tagging/emails.py

@@ -5,6 +5,8 @@
 from django.template.loader import get_template
 from django.urls import reverse
 
+from course_discovery.apps.publisher.utils import is_email_notification_enabled
+
 logger = logging.getLogger(__name__)
 
 
@@ -37,8 +39,20 @@ def send_email_for_course_verticals_update(report, to_users):
 def send_email_for_course_vertical_assignment(course, to_users):
     """
     Sends an email to specified users requesting action to assign vertical and sub-vertical
-    for a given course.
+    for a given course, but only to those who have email notifications enabled.
+
+    Arguments:
+        course(Object): course model instance
+        to_users(List): list of user objects
     """
+    email_enabled_users = [user.email for user in to_users if is_email_notification_enabled(user)]
+    if not email_enabled_users:
+        logger.exception(
+            f"Failed to send vertical assignment email for course '{course.title}' (UUID: {course.uuid})"
+            f"No recipients found."
+        )
+        return
+
     course_tagging_url = (
         f"{settings.DISCOVERY_BASE_URL}{reverse('tagging:course_tagging_detail', kwargs={'uuid': course.uuid})}"
     )
@@ -51,7 +65,7 @@ def send_email_for_course_vertical_assignment(course, to_users):
         f"Action Required: Assign Vertical and Sub-vertical for Course '{course.title}'",
         html_content,
         settings.PUBLISHER_FROM_EMAIL,
-        to_users,
+        email_enabled_users,
     )
     email.content_subtype = "html"
 
@@ -60,5 +74,5 @@ def send_email_for_course_vertical_assignment(course, to_users):
     except Exception as e:  # pylint: disable=broad-except
         logger.exception(
             f"Failed to send vertical assignment email for course '{course.title}' (UUID: {course.uuid}) to "
-            f"recipients {', '.join(to_users)}. Error: {str(e)}"
+            f"recipients {', '.join(email_enabled_users)}. Error: {str(e)}"
         )

course_discovery/apps/tagging/signals.py

@@ -1,5 +1,5 @@
 from django.conf import settings
-from django.contrib.auth.models import Group
+from django.contrib.auth import get_user_model
 from django.db.models.signals import post_save
 from django.dispatch import receiver
 
@@ -16,10 +16,11 @@ def notify_vertical_assignment(instance, created, **kwargs):
     if instance.draft or not created:
         return
 
+    User = get_user_model()
     management_groups = getattr(settings, "VERTICALS_MANAGEMENT_GROUPS", [])
 
-    groups = Group.objects.filter(name__in=management_groups).exclude(user__isnull=True)
-    recipients = set(groups.values_list('user__email', flat=True))
-
+    recipients = list(
+        User.objects.prefetch_related('groups').filter(groups__name__in=management_groups).distinct()
+    )
     if recipients:
         send_email_for_course_vertical_assignment(instance, recipients)

course_discovery/apps/tagging/tests/test_emails.py

@@ -5,7 +5,9 @@
 from django.core import mail
 from django.test import TestCase
 
+from course_discovery.apps.core.tests.factories import UserFactory
 from course_discovery.apps.course_metadata.tests.factories import CourseFactory
+from course_discovery.apps.publisher.tests.factories import UserAttributeFactory
 from course_discovery.apps.tagging.emails import send_email_for_course_vertical_assignment
 
 
@@ -15,19 +17,21 @@ class VerticalAssignmentEmailTests(TestCase):
     def setUp(self):
         self.group_name = settings.VERTICALS_MANAGEMENT_GROUPS[0]
         self.course = CourseFactory(title="Test Course", draft=False)
-        self.recipients = ["[email protected]", "[email protected]"]
+        self.user1 = UserFactory(email="[email protected]")
+        self.user2 = UserFactory(email="[email protected]")
+        self.recipients = [self.user1, self.user2]
         self.logger = logging.getLogger("course_discovery.apps.tagging.emails")
 
     def test_email_sent_to_recipients(self):
         """
-        Test that an email is sent to the specified recipients with the correct subject
+        Test that an email is sent to the specified recipients with the correct subject.
         """
         send_email_for_course_vertical_assignment(self.course, self.recipients)
 
         self.assertEqual(len(mail.outbox), 1)
 
         email = mail.outbox[0]
-        self.assertEqual(email.to, self.recipients)
+        self.assertEqual(email.to, [self.user1.email, self.user2.email])
         expected_subject = f"Action Required: Assign Vertical and Sub-vertical for Course '{self.course.title}'"
         self.assertEqual(email.subject, expected_subject)
 
@@ -42,3 +46,20 @@ def test_email_send_failure_logs_exception(self, mock_send):
             send_email_for_course_vertical_assignment(self.course, self.recipients)
 
         self.assertIn("Failed to send vertical assignment email", log_context.output[0])
+
+    def test_no_email_sent_when_user_notifications_disabled(self):
+        """
+        Test that if a user has disabled email notifications via their UserAttributes,
+        then no email is sent.
+        """
+
+        disabled_user = UserFactory(email="[email protected]")
+        UserAttributeFactory(user=disabled_user, enable_email_notification=False)
+
+        send_email_for_course_vertical_assignment(self.course, [disabled_user])
+
+        with self.assertLogs(logger=self.logger, level="ERROR") as log_context:
+            send_email_for_course_vertical_assignment(self.course, [disabled_user])
+
+        self.assertEqual(len(mail.outbox), 0)
+        self.assertIn("No recipients found.", log_context.output[0])

course_discovery/apps/tagging/tests/test_signals.py

@@ -34,11 +34,11 @@ def test_notify_vertical_assignment_email_sent(self, mock_send_email):
         course_run.status = CourseRunStatus.Reviewed
         course_run.save()
 
-        expected_recipients = {"user1@example.com", "user2@example.com"}
+        expected_recipients = [user1, user2]
         mock_send_email.assert_called_once()
         called_args = mock_send_email.call_args[0]
         self.assertEqual(called_args[0].uuid, course.uuid)
-        self.assertEqual(called_args[1], expected_recipients)
+        self.assertListEqual(called_args[1], expected_recipients)
 
     @mock.patch("course_discovery.apps.tagging.signals.send_email_for_course_vertical_assignment")
     def test_notify_vertical_assignment_email_when_course_is_draft(self, mock_send_email):

course_discovery/settings/base.py

@@ -118,7 +118,7 @@
     'waffle.middleware.WaffleMiddleware',
     'simple_history.middleware.HistoryRequestMiddleware',
     'edx_django_utils.cache.middleware.TieredCacheMiddleware',
-    'edx_rest_framework_extensions.middleware.RequestMetricsMiddleware',
+    'edx_rest_framework_extensions.middleware.RequestCustomAttributesMiddleware',
     'edx_rest_framework_extensions.auth.jwt.middleware.EnsureJWTAuthSettingsMiddleware',
 )
 

docs/decisions/0032-pathway-retirement.rst

@@ -0,0 +1,35 @@
+32. Retirement Mechanism for Pathways
+======================================
+
+Status
+--------
+Accepted (Feb 2025)
+
+Context
+---------
+Currently, there is no way to mark a pathway as retired. This is often necessary due to changing requirements
+for credit recognition on the partner organization's end, or the discontinuation of programs offered by them.
+In such cases, these pathways should be hidden from the learner dashboard and any credit requests against them
+should not be accepted.
+
+Decision
+----------
+A new field **status** will be added to the pathway model. This field will support three possible values: *unpublished*,
+*published* and *retired*. Existing pathways will be assigned the *published* status (through a data migration), while any new pathways will be set
+to *unpublished* by default.
+
+The **status** field will be exposed in the pathways endpoint, and the API will also support filtering by its value.
+
+Consequences
+--------------
+Consuming systems, such as credentials and edx-plaform, will have to ensure that they take the status field in consideration
+while processing pathways. Specifically, credentials will need to ensure that it does not allow credit redemption requests
+against retired pathways, and edx-platform will need to exclude retired pathways from the programs section of the learner dashboard.
+
+Alternatives Considered
+-------------------------
+One alternative considered was to hide retired pathways by default in the API responses. However, this approach
+was soon determined to be problematic because it could cause issues on the Credentials side, which has its own
+Pathway model (regularly synced with Discovery) having protected constraints with some other models. Additionally,
+it is more transparent to place the responsibility of correct usage on the consuming systems, rather than automatically
+filtering retired objects on discovery's end.