diff --git a/conftest.py b/conftest.py index e7ca809f2f..a748689056 100644 --- a/conftest.py +++ b/conftest.py @@ -26,6 +26,7 @@ def default_settings(monkeypatch, settings): settings.FEATURES[features.IGNORE_EDX_FAILURES] = False settings.FEATURES[features.SYNC_ON_DASHBOARD_LOAD] = False + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = False @pytest.fixture(autouse=True) diff --git a/courses/admin.py b/courses/admin.py index 26b0a53a0a..ddf66bac90 100644 --- a/courses/admin.py +++ b/courses/admin.py @@ -32,6 +32,7 @@ PaidCourseRun, PaidProgram, PartnerSchool, + PartnerSchoolProgram, Program, ProgramCertificate, ProgramCollectionItem, @@ -1168,18 +1169,42 @@ def get_queryset(self, request): return qs.select_related("programcertificate", "courseruncertificate") +class PartnerSchoolProgramInline(admin.TabularInline): + """Inline for assigning a partner school to programs.""" + + model = PartnerSchoolProgram + extra = 1 + fields = ("program", "email", "alt_email") + autocomplete_fields = ("program",) + verbose_name = "Program assignment" + verbose_name_plural = ( + "Program assignments (add one row per recipient address; " + "leave email blank to use the school's default)" + ) + + @admin.register(PartnerSchool) class PartnerSchoolAdmin(TimestampedModelAdmin): """Admin for PartnerSchool""" model = PartnerSchool - list_display = ["name", "email"] + inlines = [PartnerSchoolProgramInline] + list_display = ["name", "email", "assigned_programs"] + list_filter = ["programs"] search_fields = ["name", "email"] def get_queryset(self, request): # noqa: ARG002 """Use the all_objects manager so we can see everything.""" - return self.model.all_objects.get_queryset() + return self.model.all_objects.get_queryset().prefetch_related("programs") + + @admin.display(description="Programs") + def assigned_programs(self, obj): + """Comma-separated list of assigned programs for the changelist.""" + + return ", ".join( + sorted({program.readable_id for program in obj.programs.all()}) + ) def delete_model(self, request, obj): # noqa: ARG002 """Soft-delete the model.""" diff --git a/courses/admin_test.py b/courses/admin_test.py index 55fcb86410..5d9e4a3c7c 100644 --- a/courses/admin_test.py +++ b/courses/admin_test.py @@ -5,7 +5,12 @@ from django.urls import reverse from courses.admin import CourseRunEnrollmentAdmin -from courses.factories import CourseRunEnrollmentFactory +from courses.factories import ( + CourseRunEnrollmentFactory, + PartnerSchoolFactory, + PartnerSchoolProgramFactory, + ProgramFactory, +) from courses.models import CourseRunEnrollment from openedx.constants import OPENEDX_ENROLLMENT_REPAIR_MAX_RETRIES @@ -56,3 +61,37 @@ def test_repair_exhausted_display(edx_enrolled, retry_count, expected): admin_instance = CourseRunEnrollmentAdmin(CourseRunEnrollment, django_admin.site) assert admin_instance.repair_exhausted(enrollment) is expected + + +def test_partner_school_admin_change_page_lists_program_inline(admin_client): + """The change page exposes the program assignment inline, including alt_email.""" + school = PartnerSchoolFactory.create() + program = ProgramFactory.create(title="Supply Chain Management") + PartnerSchoolProgramFactory.create(partner_school=school, program=program) + + resp = admin_client.get( + reverse("admin:courses_partnerschool_change", args=[school.id]) + ) + + assert resp.status_code == 200 + assert b"program_links" in resp.content + assert b"alt_email" in resp.content + + +def test_partner_school_admin_changelist_filters_by_program(admin_client): + """The changelist can be filtered down to one program's schools.""" + scm = ProgramFactory.create() + dedp = ProgramFactory.create() + scm_school = PartnerSchoolFactory.create(name="SCM Only School") + dedp_school = PartnerSchoolFactory.create(name="DEDP Only School") + PartnerSchoolProgramFactory.create(partner_school=scm_school, program=scm) + PartnerSchoolProgramFactory.create(partner_school=dedp_school, program=dedp) + + resp = admin_client.get( + reverse("admin:courses_partnerschool_changelist"), + {"programs__id__exact": scm.id}, + ) + + assert resp.status_code == 200 + assert b"SCM Only School" in resp.content + assert b"DEDP Only School" not in resp.content diff --git a/courses/api.py b/courses/api.py index 5aeffb84bf..eea610d2f8 100644 --- a/courses/api.py +++ b/courses/api.py @@ -24,6 +24,7 @@ first_or_none, has_equal_properties, ) +from mitol.olposthog.features import is_enabled from opaque_keys.edx.keys import CourseKey from requests.exceptions import ConnectionError as RequestsConnectionError from requests.exceptions import HTTPError @@ -48,6 +49,7 @@ Department, EnrollmentMode, PaidCourseRun, + PartnerSchool, Program, ProgramCertificate, ProgramEnrollment, @@ -1525,6 +1527,31 @@ def manage_program_certificate_access(user, program, revoke_state): return True +def partner_schools_for_program(program): + """ + Return the pathway schools a learner may share this program's record with. + + While ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS is off this returns every active + school, which is the pre-12321 behavior. Once the flag is on, only schools + assigned to this program are returned. + + `.distinct()` is required: a school with more than one recipient row for the + program joins once per row and would otherwise be listed twice. + + Args: + program (Program): the program whose record is being shared + + Returns: + QuerySet of PartnerSchool + """ + schools = PartnerSchool.objects.all() + + if is_enabled(features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS): + schools = schools.filter(programs=program).distinct() + + return schools + + def resolve_courseware_object_from_id( courseware_id: str, ) -> Program | Course | CourseRun | None: diff --git a/courses/api_test.py b/courses/api_test.py index 5d919369bd..b74eb29350 100644 --- a/courses/api_test.py +++ b/courses/api_test.py @@ -53,6 +53,7 @@ manage_course_run_certificate_access, manage_program_certificate_access, override_user_grade, + partner_schools_for_program, process_course_run_grade_certificate, pull_course_modes, sync_course_mode, @@ -76,6 +77,8 @@ CourseRunGradeFactory, DepartmentFactory, EnrollmentModeFactory, + PartnerSchoolFactory, + PartnerSchoolProgramFactory, ProgramCertificateFactory, ProgramEnrollmentFactory, ProgramFactory, @@ -98,6 +101,7 @@ ) from ecommerce.factories import LineFactory, OrderFactory, ProductFactory from ecommerce.models import Basket, OrderStatus +from main import features from main.constants import USER_MSG_TYPE_B2B_ENROLL_SUCCESS from main.test_utils import MockHttpError from openedx.constants import ( @@ -4148,3 +4152,59 @@ def _side_effect(user, program, force_create=False): # noqa: FBT002 assert stats["failed"] == 1 assert stats["processed"] == 2 assert mock_generate.call_count == 2 + + +def test_partner_schools_for_program_unfiltered_when_flag_off(settings): + """With the flag off every active school is returned, preserving old behavior.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = False + scm = ProgramFactory.create() + dedp = ProgramFactory.create() + scm_school = PartnerSchoolFactory.create(name="SCM School") + dedp_school = PartnerSchoolFactory.create(name="DEDP School") + PartnerSchoolProgramFactory.create(partner_school=scm_school, program=scm) + PartnerSchoolProgramFactory.create(partner_school=dedp_school, program=dedp) + + result = partner_schools_for_program(scm) + + assert sorted(school.name for school in result) == ["DEDP School", "SCM School"] + + +def test_partner_schools_for_program_filtered_when_flag_on(settings): + """With the flag on only the program's own schools are returned.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + scm = ProgramFactory.create() + dedp = ProgramFactory.create() + scm_school = PartnerSchoolFactory.create(name="SCM School") + dedp_school = PartnerSchoolFactory.create(name="DEDP School") + PartnerSchoolProgramFactory.create(partner_school=scm_school, program=scm) + PartnerSchoolProgramFactory.create(partner_school=dedp_school, program=dedp) + + result = partner_schools_for_program(scm) + + assert [school.name for school in result] == ["SCM School"] + + +def test_partner_schools_for_program_deduplicates_multi_recipient_school(settings): + """A school with two recipient rows appears once when the flag is on.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + program = ProgramFactory.create() + school = PartnerSchoolFactory.create(name="Reykjavik University") + PartnerSchoolProgramFactory.create( + partner_school=school, program=program, email="vd@example.com" + ) + PartnerSchoolProgramFactory.create( + partner_school=school, program=program, email="cs@example.com" + ) + + result = partner_schools_for_program(program) + + assert [school.name for school in result] == ["Reykjavik University"] + + +def test_partner_schools_for_program_excludes_unassigned_when_flag_on(settings): + """An untagged school is invisible once filtering is live.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + program = ProgramFactory.create() + PartnerSchoolFactory.create(name="Unassigned School") + + assert list(partner_schools_for_program(program)) == [] diff --git a/courses/factories.py b/courses/factories.py index bfec4df307..28e4b0a716 100644 --- a/courses/factories.py +++ b/courses/factories.py @@ -26,6 +26,7 @@ EnrollmentMode, LearnerProgramRecordShare, PartnerSchool, + PartnerSchoolProgram, Program, ProgramCertificate, ProgramEnrollment, @@ -386,6 +387,16 @@ class Meta: model = PartnerSchool +class PartnerSchoolProgramFactory(DjangoModelFactory): + partner_school = SubFactory(PartnerSchoolFactory) + program = SubFactory(ProgramFactory) + email = fuzzy.FuzzyText(suffix="@example.com") + alt_email = "" + + class Meta: + model = PartnerSchoolProgram + + class LearnerProgramRecordShareFactory(DjangoModelFactory): user = SubFactory(UserFactory) program = SubFactory(ProgramFactory) diff --git a/courses/mail_api.py b/courses/mail_api.py index 0546244071..47993d0111 100644 --- a/courses/mail_api.py +++ b/courses/mail_api.py @@ -3,6 +3,7 @@ import logging from mitol.mail.api import get_message_sender +from mitol.olposthog.features import is_enabled from courses.messages import ( CourseRunEnrollmentMessage, @@ -11,6 +12,7 @@ PartnerSchoolSharingMessage, ) from courses.models import CourseRun +from main import features from main.settings import SITE_BASE_URL log = logging.getLogger() @@ -77,13 +79,29 @@ def send_partner_school_sharing_message(learner_record): learner_record (LearnerProgramRecordShare): the learner record to send """ try: - with get_message_sender(PartnerSchoolSharingMessage) as sender: - sender.build_and_send_message( - learner_record.partner_school.email, - { - "learner_record": learner_record, - "record_link": f"{SITE_BASE_URL}/records/shared/{learner_record.share_uuid}", - }, + # Second of two deliberate flag reads for hq#12321 (the other is + # courses.api.partner_schools_for_program). Gating mail here keeps the + # flag's promise: entering program assignments cannot change delivery + # until the flag is flipped, so data-entry mistakes stay harmless. + if is_enabled(features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS): + recipients = learner_record.partner_school.emails_for_program( + learner_record.program ) + else: + recipients = [learner_record.partner_school.email] + context = { + "learner_record": learner_record, + "record_link": f"{SITE_BASE_URL}/records/shared/{learner_record.share_uuid}", + } + with get_message_sender(PartnerSchoolSharingMessage) as sender: + for recipient in recipients: + try: + sender.build_and_send_message(recipient, context) + except Exception: # pylint: disable=broad-except # noqa: PERF203 + log.exception( + "Error sending partner school sharing email to %s for share %s", + recipient, + learner_record.share_uuid, + ) except Exception: # pylint: disable=broad-except log.exception("Error sending partner school sharing email") diff --git a/courses/mail_api_test.py b/courses/mail_api_test.py index aa25553b6a..9241fdcfa3 100644 --- a/courses/mail_api_test.py +++ b/courses/mail_api_test.py @@ -6,6 +6,7 @@ CourseRunEnrollmentFactory, CourseRunFactory, LearnerProgramRecordShareFactory, + PartnerSchoolProgramFactory, ProgramFactory, ) from courses.mail_api import ( @@ -18,6 +19,7 @@ EnrollmentFailureMessage, PartnerSchoolSharingMessage, ) +from main import features from main.settings import SITE_BASE_URL pytestmark = pytest.mark.django_db @@ -74,17 +76,166 @@ def test_send_enrollment_failure_message(user, mocker, is_program): ) -def test_send_partner_school_sharing_message(mocker): - """Test that the partner school message goes to the right spot""" +def test_send_partner_school_sharing_message(mocker, settings): + """The record goes to the per-program recipient address.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True record = LearnerProgramRecordShareFactory() + PartnerSchoolProgramFactory.create( + partner_school=record.partner_school, + program=record.program, + email="scm@example.com", + ) record_link = f"{SITE_BASE_URL}/records/shared/{record.share_uuid}" patched_get_message_sender = mocker.patch("courses.mail_api.get_message_sender") mock_sender = patched_get_message_sender.return_value.__enter__.return_value send_partner_school_sharing_message(record) + patched_get_message_sender.assert_called_once_with(PartnerSchoolSharingMessage) + mock_sender.build_and_send_message.assert_called_once_with( + "scm@example.com", + {"learner_record": record, "record_link": record_link}, + ) + + +def test_send_partner_school_sharing_message_all_recipients(mocker, settings): + """A school with two recipients for the program gets one message each.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + record = LearnerProgramRecordShareFactory() + for email in ["vd@example.com", "cs@example.com"]: + PartnerSchoolProgramFactory.create( + partner_school=record.partner_school, + program=record.program, + email=email, + ) + record_link = f"{SITE_BASE_URL}/records/shared/{record.share_uuid}" + + patched_get_message_sender = mocker.patch("courses.mail_api.get_message_sender") + mock_sender = patched_get_message_sender.return_value.__enter__.return_value + + send_partner_school_sharing_message(record) + + context = {"learner_record": record, "record_link": record_link} + assert mock_sender.build_and_send_message.call_count == 2 + mock_sender.build_and_send_message.assert_any_call("vd@example.com", context) + mock_sender.build_and_send_message.assert_any_call("cs@example.com", context) + + +def test_send_partner_school_sharing_message_never_sends_to_alt_email(mocker, settings): + """alt_email is reference data only and must never receive a record.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + record = LearnerProgramRecordShareFactory() + PartnerSchoolProgramFactory.create( + partner_school=record.partner_school, + program=record.program, + email="primary@example.com", + alt_email="alternative@example.com", + ) + + patched_get_message_sender = mocker.patch("courses.mail_api.get_message_sender") + mock_sender = patched_get_message_sender.return_value.__enter__.return_value + + send_partner_school_sharing_message(record) + + recipients = [ + call.args[0] for call in mock_sender.build_and_send_message.call_args_list + ] + assert recipients == ["primary@example.com"] + assert "alternative@example.com" not in recipients + + +def test_send_partner_school_sharing_message_falls_back_to_school_email( + mocker, settings +): + """With no per-program address the school's own email is used.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + record = LearnerProgramRecordShareFactory() + record_link = f"{SITE_BASE_URL}/records/shared/{record.share_uuid}" + + patched_get_message_sender = mocker.patch("courses.mail_api.get_message_sender") + mock_sender = patched_get_message_sender.return_value.__enter__.return_value + + send_partner_school_sharing_message(record) + mock_sender.build_and_send_message.assert_called_once_with( record.partner_school.email, {"learner_record": record, "record_link": record_link}, ) + + +def test_send_partner_school_sharing_message_flag_off_ignores_program_links( + mocker, settings +): + """With the flag off, mail goes to the school's own address even when + per-program links exist. This is the guarantee that entering assignments + cannot change delivery before the flag is flipped. + """ + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = False + record = LearnerProgramRecordShareFactory() + record.partner_school.email = "generic@example.com" + record.partner_school.save() + PartnerSchoolProgramFactory.create( + partner_school=record.partner_school, + program=record.program, + email="dept@example.com", + ) + record_link = f"{SITE_BASE_URL}/records/shared/{record.share_uuid}" + + patched_get_message_sender = mocker.patch("courses.mail_api.get_message_sender") + mock_sender = patched_get_message_sender.return_value.__enter__.return_value + + send_partner_school_sharing_message(record) + + mock_sender.build_and_send_message.assert_called_once_with( + "generic@example.com", + {"learner_record": record, "record_link": record_link}, + ) + + +def test_send_partner_school_sharing_message_flag_off_single_recipient( + mocker, settings +): + """With the flag off, two program links still produce exactly one message.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = False + record = LearnerProgramRecordShareFactory() + record.partner_school.email = "generic@example.com" + record.partner_school.save() + for email in ["vd@example.com", "cs@example.com"]: + PartnerSchoolProgramFactory.create( + partner_school=record.partner_school, program=record.program, email=email + ) + + patched_get_message_sender = mocker.patch("courses.mail_api.get_message_sender") + mock_sender = patched_get_message_sender.return_value.__enter__.return_value + + send_partner_school_sharing_message(record) + + recipients = [ + call.args[0] for call in mock_sender.build_and_send_message.call_args_list + ] + assert recipients == ["generic@example.com"] + + +def test_send_partner_school_sharing_message_continues_after_failure(mocker, settings): + """A failure for one recipient must not prevent the others from being sent.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + record = LearnerProgramRecordShareFactory() + for email in ["first@example.com", "second@example.com"]: + PartnerSchoolProgramFactory.create( + partner_school=record.partner_school, program=record.program, email=email + ) + + patched_get_message_sender = mocker.patch("courses.mail_api.get_message_sender") + mock_sender = patched_get_message_sender.return_value.__enter__.return_value + mock_sender.build_and_send_message.side_effect = [ + Exception("mailgun rejected"), + None, + ] + + send_partner_school_sharing_message(record) + + recipients = [ + call.args[0] for call in mock_sender.build_and_send_message.call_args_list + ] + assert recipients == ["first@example.com", "second@example.com"] diff --git a/courses/migrations/0101_partnerschoolprogram.py b/courses/migrations/0101_partnerschoolprogram.py new file mode 100644 index 0000000000..3d3743c881 --- /dev/null +++ b/courses/migrations/0101_partnerschoolprogram.py @@ -0,0 +1,88 @@ +# Generated by Django 5.2.15 on 2026-07-29 12:47 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + dependencies = [ + ("courses", "0100_courserunenrollment_edx_enrollment_retry_count"), + ] + + operations = [ + migrations.AlterModelOptions( + name="partnerschool", + options={"ordering": ["name"]}, + ), + migrations.CreateModel( + name="PartnerSchoolProgram", + fields=[ + ( + "id", + models.AutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("created_on", models.DateTimeField(auto_now_add=True)), + ("updated_on", models.DateTimeField(auto_now=True)), + ( + "email", + models.TextField( + blank=True, + help_text="Recipient address for this program. Leave blank to use the school's default email.", + ), + ), + ( + "alt_email", + models.TextField( + blank=True, + help_text="Alternative contact address, recorded for reference only. Learner records are NOT sent here.", + ), + ), + ( + "partner_school", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="program_links", + to="courses.partnerschool", + ), + ), + ( + "program", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="partner_school_links", + to="courses.program", + ), + ), + ], + ), + migrations.AddField( + model_name="partnerschool", + name="programs", + field=models.ManyToManyField( + blank=True, + help_text="Programs this school accepts records for. Learners only see schools assigned to the program whose record they are sharing.", + related_name="partner_schools", + through="courses.PartnerSchoolProgram", + to="courses.program", + ), + ), + migrations.AddIndex( + model_name="partnerschoolprogram", + index=models.Index( + fields=["program", "partner_school"], + name="courses_par_program_e97557_idx", + ), + ), + migrations.AddConstraint( + model_name="partnerschoolprogram", + constraint=models.UniqueConstraint( + fields=("partner_school", "program", "email"), + name="partner_school_program_email_unique", + ), + ), + ] diff --git a/courses/models.py b/courses/models.py index 50cbeadce7..e4ece65609 100644 --- a/courses/models.py +++ b/courses/models.py @@ -2720,13 +2720,51 @@ class PartnerSchool(TimestampedModel): name = models.CharField(max_length=255) email = models.TextField(null=False) is_active = models.BooleanField(default=True, blank=True) + programs = models.ManyToManyField( + "courses.Program", + through="courses.PartnerSchoolProgram", + blank=True, + related_name="partner_schools", + help_text=( + "Programs this school accepts records for. Learners only see schools " + "assigned to the program whose record they are sharing." + ), + ) objects = PartnerSchoolActiveUndeleteManager() all_objects = models.Manager() + class Meta: + ordering = ["name"] + def __str__(self): return self.name + def emails_for_program(self, program): + """ + Return the recipient addresses for this school for the given program. + + A school may have more than one recipient per program (product confirmed + Reykjavik University notifies two offices for SDS), so this returns a list. + Falls back to the school's own email when no link supplies one, which keeps + pre-existing rows working. + + `PartnerSchoolProgram.alt_email` is deliberately NOT included: product + asked for it to be recorded for reference only. Do not add it here. + + Args: + program (Program): the program whose record is being shared + + Returns: + list of str: email addresses to send the record to + """ + emails = [ + link.email + for link in self.program_links.filter(program=program).order_by("id") + if link.email + ] + return emails or [self.email] + def delete(self, *, using=None, keep_parents=False): # noqa: ARG002 """Soft-delete the record.""" @@ -2735,6 +2773,54 @@ def delete(self, *, using=None, keep_parents=False): # noqa: ARG002 return (1, {"courses.PartnerSchool": 1}) +class PartnerSchoolProgram(TimestampedModel): + """ + Links a PartnerSchool to a Program along with the recipient address for that + program. + + There is deliberately no unique constraint on (partner_school, program): a + school may need records sent to more than one address for a single program. + Add one row per recipient. + """ + + partner_school = models.ForeignKey( + "courses.PartnerSchool", + on_delete=models.CASCADE, + related_name="program_links", + ) + program = models.ForeignKey( + "courses.Program", + on_delete=models.CASCADE, + related_name="partner_school_links", + ) + email = models.TextField( + blank=True, + help_text=( + "Recipient address for this program. Leave blank to use the school's " + "default email." + ), + ) + alt_email = models.TextField( + blank=True, + help_text=( + "Alternative contact address, recorded for reference only. Learner " + "records are NOT sent here." + ), + ) + + class Meta: + constraints = [ + UniqueConstraint( + name="partner_school_program_email_unique", + fields=("partner_school", "program", "email"), + ) + ] + indexes = [models.Index(fields=("program", "partner_school"))] + + def __str__(self): + return f"{self.partner_school.name} - {self.program.readable_id} <{self.email or self.partner_school.email}>" + + class LearnerProgramRecordShare(TimestampedModel): """ Tracks the sharing status of an individual learner's program record. diff --git a/courses/models_test.py b/courses/models_test.py index e1560e4e94..366c05e318 100644 --- a/courses/models_test.py +++ b/courses/models_test.py @@ -26,6 +26,8 @@ CourseRunCertificateFactory, CourseRunEnrollmentFactory, CourseRunFactory, + PartnerSchoolFactory, + PartnerSchoolProgramFactory, ProgramCertificateFactory, ProgramEnrollmentFactory, ProgramFactory, @@ -37,6 +39,7 @@ CourseRun, CourseRunEnrollment, PaidCourseRun, + PartnerSchool, Program, ProgramRequirement, ProgramRequirementNodeType, @@ -1710,3 +1713,93 @@ def test_nonvariant_filter(): assert CourseRun.objects.filter(course=cr1.course).nonvariant().count() == 2 assert CourseRun.all_objects.filter(course=cr1.course).count() == 5 assert CourseRun.all_objects.filter(course=cr1.course).nonvariant().count() == 2 + + +def test_partner_school_emails_for_program_uses_link_email(): + """emails_for_program returns the per-program address, not the school default.""" + school = PartnerSchoolFactory.create(email="default@example.com") + program = ProgramFactory.create() + PartnerSchoolProgramFactory.create( + partner_school=school, program=program, email="scm@example.com" + ) + + assert school.emails_for_program(program) == ["scm@example.com"] + + +def test_partner_school_emails_for_program_returns_all_recipients(): + """A school may notify several addresses for one program (Reykjavik under SDS).""" + school = PartnerSchoolFactory.create(email="default@example.com") + program = ProgramFactory.create() + PartnerSchoolProgramFactory.create( + partner_school=school, program=program, email="vd@example.com" + ) + PartnerSchoolProgramFactory.create( + partner_school=school, program=program, email="cs@example.com" + ) + + assert sorted(school.emails_for_program(program)) == [ + "cs@example.com", + "vd@example.com", + ] + + +def test_partner_school_emails_for_program_falls_back_to_school_email(): + """A link with a blank email falls back to the school's own address.""" + school = PartnerSchoolFactory.create(email="default@example.com") + program = ProgramFactory.create() + PartnerSchoolProgramFactory.create(partner_school=school, program=program, email="") + + assert school.emails_for_program(program) == ["default@example.com"] + + +def test_partner_school_emails_for_program_ignores_other_programs(): + """A link for a different program must not leak into this program's recipients.""" + school = PartnerSchoolFactory.create(email="default@example.com") + scm = ProgramFactory.create() + dedp = ProgramFactory.create() + PartnerSchoolProgramFactory.create( + partner_school=school, program=scm, email="scm@example.com" + ) + + assert school.emails_for_program(dedp) == ["default@example.com"] + + +def test_partner_school_alt_email_is_stored_but_not_a_recipient(): + """alt_email round-trips to the database but is never a send target.""" + school = PartnerSchoolFactory.create(email="default@example.com") + program = ProgramFactory.create() + link = PartnerSchoolProgramFactory.create( + partner_school=school, + program=program, + email="primary@example.com", + alt_email="alternative@example.com", + ) + link.refresh_from_db() + + assert link.alt_email == "alternative@example.com" + assert school.emails_for_program(program) == ["primary@example.com"] + + +def test_partner_school_alt_email_alone_does_not_make_a_recipient(): + """A link with only alt_email set falls back to the school default.""" + school = PartnerSchoolFactory.create(email="default@example.com") + program = ProgramFactory.create() + PartnerSchoolProgramFactory.create( + partner_school=school, + program=program, + email="", + alt_email="alternative@example.com", + ) + + assert school.emails_for_program(program) == ["default@example.com"] + + +def test_partner_school_default_ordering_is_alphabetical(): + """PartnerSchool queries come back sorted by name.""" + PartnerSchoolFactory.create(name="Zhejiang University") + PartnerSchoolFactory.create(name="Arizona State University") + PartnerSchoolFactory.create(name="Massachusetts Institute of Technology") + + names = list(PartnerSchool.objects.values_list("name", flat=True)) + + assert names == sorted(names) diff --git a/courses/serializers/v1/programs.py b/courses/serializers/v1/programs.py index 3206a1868b..a461550b33 100644 --- a/courses/serializers/v1/programs.py +++ b/courses/serializers/v1/programs.py @@ -6,6 +6,7 @@ from cms.serializers import ProgramPageSerializer from courses import models +from courses.api import partner_schools_for_program from courses.serializers.base import ( BaseProgramRequirementTreeSerializer, get_thumbnail_url, @@ -356,6 +357,6 @@ def to_representation(self, instance): "partner_schools": [] if anonymous else PartnerSchoolSerializer( - models.PartnerSchool.objects.all(), many=True + partner_schools_for_program(instance), many=True ).data, } diff --git a/courses/serializers/v1/programs_test.py b/courses/serializers/v1/programs_test.py index 66ebdb5b56..55f8ff3ebb 100644 --- a/courses/serializers/v1/programs_test.py +++ b/courses/serializers/v1/programs_test.py @@ -18,6 +18,7 @@ EnrollmentModeFactory, LearnerProgramRecordShareFactory, PartnerSchoolFactory, + PartnerSchoolProgramFactory, ProgramFactory, program_with_empty_requirements, # noqa: F401 program_with_requirements, # noqa: F401 @@ -30,6 +31,7 @@ ProgramRequirementTreeSerializer, ProgramSerializer, ) +from main import features from main.test_utils import assert_drf_json_equal from openedx.constants import EDX_ENROLLMENT_AUDIT_MODE, EDX_ENROLLMENT_VERIFIED_MODE from users.factories import UserFactory @@ -622,3 +624,88 @@ def test_program_requirement_serializer_valid(data): """Verify that the ProgramRequirementSerializer validates data""" serializer = ProgramRequirementSerializer(data=data) serializer.is_valid(raise_exception=True) + + +def test_learner_record_only_includes_schools_for_that_program(settings): + """A learner sharing an SCM record must not see DEDP-only schools.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + scm = ProgramFactory.create() + dedp = ProgramFactory.create() + user = UserFactory.create() + scm_school = PartnerSchoolFactory.create(name="SCM School") + dedp_school = PartnerSchoolFactory.create(name="DEDP School") + PartnerSchoolProgramFactory.create(partner_school=scm_school, program=scm) + PartnerSchoolProgramFactory.create(partner_school=dedp_school, program=dedp) + + data = LearnerRecordSerializer(scm, context={"user": user}).data + + assert [school["name"] for school in data["partner_schools"]] == ["SCM School"] + + +def test_learner_record_schools_are_alphabetical(settings): + """The school list is sorted by name regardless of insertion order.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + program = ProgramFactory.create() + user = UserFactory.create() + for name in ["Zhejiang University", "Arizona State University", "MIT"]: + school = PartnerSchoolFactory.create(name=name) + PartnerSchoolProgramFactory.create(partner_school=school, program=program) + + data = LearnerRecordSerializer(program, context={"user": user}).data + + assert [school["name"] for school in data["partner_schools"]] == [ + "Arizona State University", + "MIT", + "Zhejiang University", + ] + + +def test_learner_record_school_with_two_recipients_appears_once(settings): + """Two recipient rows for one school must not duplicate the dropdown entry.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + program = ProgramFactory.create() + user = UserFactory.create() + school = PartnerSchoolFactory.create(name="Reykjavik University") + PartnerSchoolProgramFactory.create( + partner_school=school, program=program, email="vd@example.com" + ) + PartnerSchoolProgramFactory.create( + partner_school=school, program=program, email="cs@example.com" + ) + + data = LearnerRecordSerializer(program, context={"user": user}).data + + assert [school["name"] for school in data["partner_schools"]] == [ + "Reykjavik University" + ] + + +def test_learner_record_excludes_unassigned_schools(settings): + """A school with no program assignment is shown to nobody.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + program = ProgramFactory.create() + user = UserFactory.create() + PartnerSchoolFactory.create(name="Unassigned School") + + data = LearnerRecordSerializer(program, context={"user": user}).data + + assert data["partner_schools"] == [] + + +def test_learner_record_shows_all_schools_when_flag_off(settings): + """With the flag off the pre-12321 behavior is preserved exactly.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = False + scm = ProgramFactory.create() + dedp = ProgramFactory.create() + user = UserFactory.create() + scm_school = PartnerSchoolFactory.create(name="SCM School") + dedp_school = PartnerSchoolFactory.create(name="DEDP School") + PartnerSchoolProgramFactory.create(partner_school=scm_school, program=scm) + PartnerSchoolProgramFactory.create(partner_school=dedp_school, program=dedp) + + data = LearnerRecordSerializer(scm, context={"user": user}).data + + assert [school["name"] for school in data["partner_schools"]] == [ + "DEDP School", + "SCM School", + ] diff --git a/courses/views/v1/__init__.py b/courses/views/v1/__init__.py index 91b622ad01..20148b91a3 100644 --- a/courses/views/v1/__init__.py +++ b/courses/views/v1/__init__.py @@ -19,6 +19,7 @@ from requests.exceptions import HTTPError from rest_framework import mixins, serializers, status, viewsets from rest_framework.decorators import permission_classes +from rest_framework.exceptions import ValidationError from rest_framework.generics import GenericAPIView from rest_framework.pagination import PageNumberPagination from rest_framework.permissions import IsAuthenticated @@ -32,6 +33,7 @@ deactivate_run_enrollment, get_relevant_course_run_qset, get_user_relevant_program_course_run_qset, + partner_schools_for_program, ) from courses.constants import ENROLL_CHANGE_STATUS_UNENROLLED from courses.models import ( @@ -632,6 +634,32 @@ class PartnerSchoolViewSet(viewsets.ReadOnlyModelViewSet): serializer_class = PartnerSchoolSerializer queryset = PartnerSchool.objects.all() + @extend_schema( + parameters=[ + OpenApiParameter( + name="program", + type=int, + description="Only return schools assigned to this program id.", + required=False, + ) + ] + ) + def list(self, request, *args, **kwargs): + return super().list(request, *args, **kwargs) + + def get_queryset(self): + """Optionally scope the list to a single program.""" + queryset = super().get_queryset() + program_id = self.request.query_params.get("program") + if program_id: + try: + program_id = int(program_id) + except ValueError: + msg = "program must be an integer id." + raise ValidationError({"program": msg}) from None + queryset = queryset.filter(programs__id=program_id).distinct() + return queryset + def get_enrolled_program_or_404(user, program_id: int) -> Program: """Return a program only if the user has an active enrollment for it.""" @@ -684,7 +712,9 @@ def post(self, request, pk): and request.data["partnerSchool"] is not None ): try: - school = PartnerSchool.objects.get(pk=request.data["partnerSchool"]) + school = partner_schools_for_program(program).get( + pk=request.data["partnerSchool"] + ) except PartnerSchool.DoesNotExist: return Response("Partner school not found.", status.HTTP_404_NOT_FOUND) diff --git a/courses/views/v1/views_test.py b/courses/views/v1/views_test.py index 2d95b6213f..08c97fbb2e 100644 --- a/courses/views/v1/views_test.py +++ b/courses/views/v1/views_test.py @@ -33,6 +33,7 @@ CourseRunFactory, LearnerProgramRecordShareFactory, PartnerSchoolFactory, + PartnerSchoolProgramFactory, ProgramCertificateFactory, ProgramEnrollmentFactory, ProgramFactory, @@ -1364,3 +1365,108 @@ def test_get_shared_learner_record_inactive_share_returns_404(user): assert resp.status_code == status.HTTP_404_NOT_FOUND assert resp.json() == [] + + +def test_share_learner_record_rejects_school_from_another_program( + user_drf_client, user, mocker, settings +): + """Sharing with a school that is not assigned to this program is refused.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = True + enrollment = ProgramEnrollmentFactory.create(user=user) + other_program = ProgramFactory.create() + other_school = PartnerSchoolFactory.create() + PartnerSchoolProgramFactory.create( + partner_school=other_school, program=other_program + ) + patched_send_email = mocker.patch( + "courses.views.v1.send_partner_school_email.delay" + ) + + resp = user_drf_client.post( + reverse("learner-record-share", kwargs={"pk": enrollment.program.id}), + data={"partnerSchool": other_school.id}, + ) + + assert resp.status_code == status.HTTP_404_NOT_FOUND + assert not LearnerProgramRecordShare.objects.filter( + user=user, partner_school=other_school + ).exists() + patched_send_email.assert_not_called() + + +def test_share_learner_record_allows_any_school_when_flag_off( + user_drf_client, user, mocker, settings +): + """With the flag off the endpoint accepts any school, as it does today.""" + settings.FEATURES[features.ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS] = False + enrollment = ProgramEnrollmentFactory.create(user=user) + unassigned_school = PartnerSchoolFactory.create() + patched_send_email = mocker.patch( + "courses.views.v1.send_partner_school_email.delay" + ) + + resp = user_drf_client.post( + reverse("learner-record-share", kwargs={"pk": enrollment.program.id}), + data={"partnerSchool": unassigned_school.id}, + ) + + assert resp.status_code == status.HTTP_200_OK + patched_send_email.assert_called_once() + + +def test_partner_schools_endpoint_filters_by_program(user_drf_client): + """The partner schools list can be scoped to a single program.""" + scm = ProgramFactory.create() + dedp = ProgramFactory.create() + scm_school = PartnerSchoolFactory.create(name="SCM School") + dedp_school = PartnerSchoolFactory.create(name="DEDP School") + PartnerSchoolProgramFactory.create(partner_school=scm_school, program=scm) + PartnerSchoolProgramFactory.create(partner_school=dedp_school, program=dedp) + + resp = user_drf_client.get( + reverse("v1:partner_schools_api-list"), {"program": scm.id} + ) + + assert resp.status_code == status.HTTP_200_OK + assert [school["name"] for school in resp.json()] == ["SCM School"] + + +def test_partner_schools_endpoint_unfiltered_by_default(user_drf_client): + """Without a program param the endpoint returns every active school.""" + program = ProgramFactory.create() + school = PartnerSchoolFactory.create(name="Some School") + PartnerSchoolProgramFactory.create(partner_school=school, program=program) + + resp = user_drf_client.get(reverse("v1:partner_schools_api-list")) + + assert resp.status_code == status.HTTP_200_OK + assert [s["name"] for s in resp.json()] == ["Some School"] + + +@pytest.mark.parametrize("program_id", ["abc", "1.5"]) +def test_partner_schools_endpoint_invalid_program_returns_400( + user_drf_client, program_id +): + """A non-integer program value returns a 400, not a 500 or an unfiltered response.""" + resp = user_drf_client.get( + reverse("v1:partner_schools_api-list"), {"program": program_id} + ) + + assert resp.status_code == status.HTTP_400_BAD_REQUEST + + +def test_partner_schools_endpoint_distinct_with_multiple_program_rows( + user_drf_client, +): + """A school with two recipient rows for the same program is returned once.""" + program = ProgramFactory.create() + school = PartnerSchoolFactory.create(name="Duplicate Rows School") + PartnerSchoolProgramFactory.create(partner_school=school, program=program) + PartnerSchoolProgramFactory.create(partner_school=school, program=program) + + resp = user_drf_client.get( + reverse("v1:partner_schools_api-list"), {"program": program.id} + ) + + assert resp.status_code == status.HTTP_200_OK + assert [s["name"] for s in resp.json()] == ["Duplicate Rows School"] diff --git a/drf_lint_baseline.json b/drf_lint_baseline.json index bb7d3e1a22..03b4fdf46d 100644 --- a/drf_lint_baseline.json +++ b/drf_lint_baseline.json @@ -1,5 +1,4 @@ [ - ".venv/lib/python3.13/site-packages/django/core/serializers/python.py:32:8:ORM001", "cms/serializers.py:115:16:ORM001", "cms/serializers.py:154:41:ORM001", "cms/serializers.py:321:12:ORM001", @@ -11,14 +10,13 @@ "courses/serializers/v1/base.py:75:20:ORM002", "courses/serializers/v1/courses.py:171:18:ORM001", "courses/serializers/v1/courses.py:57:16:ORM001", - "courses/serializers/v1/programs.py:180:12:ORM001", - "courses/serializers/v1/programs.py:195:12:ORM001", - "courses/serializers/v1/programs.py:207:12:ORM001", - "courses/serializers/v1/programs.py:293:20:ORM001", - "courses/serializers/v1/programs.py:302:27:ORM001", - "courses/serializers/v1/programs.py:317:16:ORM001", - "courses/serializers/v1/programs.py:334:17:ORM001", - "courses/serializers/v1/programs.py:359:16:ORM001", + "courses/serializers/v1/programs.py:181:12:ORM001", + "courses/serializers/v1/programs.py:196:12:ORM001", + "courses/serializers/v1/programs.py:208:12:ORM001", + "courses/serializers/v1/programs.py:294:20:ORM001", + "courses/serializers/v1/programs.py:303:27:ORM001", + "courses/serializers/v1/programs.py:318:16:ORM001", + "courses/serializers/v1/programs.py:335:17:ORM001", "courses/serializers/v2/courses.py:272:17:ORM002", "courses/serializers/v2/courses.py:337:18:ORM001", "courses/serializers/v2/departments.py:35:40:ORM002", @@ -71,9 +69,9 @@ "flexiblepricing/serializers.py:207:31:ORM001", "flexiblepricing/serializers.py:212:16:ORM001", "flexiblepricing/serializers.py:216:16:ORM001", - "users/serializers.py:225:16:ORM001", - "users/serializers.py:270:20:ORM001", - "users/serializers.py:317:19:ORM001", - "users/serializers.py:447:13:ORM001", - "users/serializers.py:483:11:ORM001" + "users/serializers.py:227:16:ORM001", + "users/serializers.py:280:20:ORM001", + "users/serializers.py:327:19:ORM001", + "users/serializers.py:459:13:ORM001", + "users/serializers.py:495:11:ORM001" ] diff --git a/main/features.py b/main/features.py index 60916960cc..f95925d13d 100644 --- a/main/features.py +++ b/main/features.py @@ -9,4 +9,8 @@ REDIRECT_LEARN_DASHBOARD = "redirect-to-learn-dashboard" +ENABLE_PROGRAM_SPECIFIC_PATHWAY_SCHOOLS = ( + "mitxonline-12321-program-specific-pathway-schools" +) + STRIPE_ENABLE_FEATURE_FLAG = "mitxonline-enable-stripe-payments"