Fix #733: fix replacing the list of waitlists in PUT requests

ctms/backport_legacy_waitlists.py

@@ -102,8 +102,15 @@ def format_legacy_vpn_relay_waitlist_input(
                 # `relay_waitlist` field was specified.
                 if input_relay_newsletters:
                     # We are subscribing to a `relay-*-waitlist` newsletter.
-                    # We don't care whether the contact had already subscribed
-                    # to another Relay waitlist, we just subscribe.
+                    # We want to keep the other Relay waitlists already subscribed.
+                    for waitlist in relay_waitlists:
+                        to_update.append(
+                            WaitlistInSchema(
+                                name=waitlist.name,
+                                subscribed=waitlist.subscribed,
+                                fields=waitlist.fields,
+                            )
+                        )
                     for newsletter in input_relay_newsletters:
                         name = newsletter["name"].replace("-waitlist", "")
                         to_update.append(

ctms/crud.py

@@ -451,14 +451,20 @@ def create_newsletter(
 def create_or_update_newsletters(
     db: Session, email_id: UUID4, newsletters: List[NewsletterInSchema]
 ):
+    # Start by deleting the existing newsletters that are not specified as input.
+    # We delete instead of set subscribed=False, because we want an idempotent
+    # round-trip of PUT/GET at the API level.
     names = [
         newsletter.name for newsletter in newsletters if not newsletter.is_default()
     ]
     db.query(Newsletter).filter(
         Newsletter.email_id == email_id, Newsletter.name.notin_(names)
     ).delete(
+        # Do not bother synchronizing objects in the session.
+        # We won't have stale objects because the next upsert query will update
+        # the other remaining objects (equivalent to `Waitlist.name.in_(names)`).
         synchronize_session=False
-    )  # This doesn't need to be synchronized because the next query only alters the other remaining rows. They can happen in whatever order. If you plan to change what the rest of this function does, consider changing this as well!
+    )
 
     if newsletters:
         stmt = insert(Newsletter).values(
@@ -488,7 +494,23 @@ def create_waitlist(
 def create_or_update_waitlists(
     db: Session, email_id: UUID4, waitlists: List[WaitlistInSchema]
 ):
-    if waitlists:
+    # Start by deleting the existing waitlists that are not specified as input.
+    # We delete instead of set subscribed=False, because we want an idempotent
+    # round-trip of PUT/GET at the API level.
+    # Note: the contact is marked as pending synchronization at the API routers level.
+    names = [waitlist.name for waitlist in waitlists if not waitlist.is_default()]
+    db.query(Waitlist).filter(
+        Waitlist.email_id == email_id, Waitlist.name.notin_(names)
+    ).delete(
+        # Do not bother synchronizing objects in the session.
+        # We won't have stale objects because the next upsert query will update
+        # the other remaining objects (equivalent to `Waitlist.name.in_(names)`).
+        synchronize_session=False
+    )
+    waitlists_to_upsert = [
+        WaitlistInSchema(**waitlist.dict()) for waitlist in waitlists
+    ]
+    if waitlists_to_upsert:
         stmt = insert(Waitlist).values(
             [{"email_id": email_id, **wl.dict()} for wl in waitlists]
         )

docs/adrs/waitlists_subscribed.md → docs/adrs/004-waitlists_subscribed.md

@@ -58,6 +58,10 @@ The cost is *Mid* and not *Low*, because it requires a tiny change in Basket to
 
 In parallel, if we look at possible evolutions of the synchronization code of CTMS, like the synchronization queue presented in *Option 4*, we could consider it regrettable to mimic newsletters. However, this evolution is not officially planned yet, and may never happen. Plus, since it will be the exact same approach as for newsletters, migrating both together to the new solution won't represent much additional effort, compared to just migrating newsletters.
 
+> Note:
+> As for newsletters, the upsert operation of a contact via a PUT request will permanently
+> delete existing waitlist entries that are not listed in the incoming payload.
+
 **Robustness**: High
 
 When synchronizing with Acoustic, we would simply delete waitlists entries where `subscribed` is `false`. It does not affect existing waitlists entries where `subscribed` is `true`. The operation is idempotent and can be interrupted and retried without impact.
@@ -78,6 +82,8 @@ Since we don't delete waitlists when we receive unsubscription, we loose track o
 
 The Acoustic has endpoints to delete relational tables by key, and the resulting code should not be too complex.
 
+The fact that upsert operations via `PUT` requests overwrite list attributes, and delete existing records has no impact with this solution.
+
 **Cost**: Mid
 
 Would require to use Acoustic API endpoints that are not currently used in CTMS.
@@ -138,6 +144,8 @@ Note: If the text with the unsubscribe reason would have to be stored in CTMS an
 
 The concept is simple. But until this solution is applied to all kind of records (contact, newsletters, etc.), and the old one completely removed, this adds up to the current complexity of the code base.
 
+The tracking of deleted objects must be put in place consistently on every model in every situation. Including for example when objects are deleted as the result of a list attribute overwrite in upsert operations via `PUT` requests.
+
 **Cost**: High
 
 This is a whole project in itself.

tests/unit/conftest.py

@@ -49,15 +49,17 @@
 APP_FOLDER = os.path.dirname(TEST_FOLDER)
 
 # Common test cases to use in parameterized tests
-# List of tuples comprised of name fixture name (defined below) and fields to
-# be overwritten, if any
+# List of tuples comprised of name fixture name (defined below) and the list
+# of fields that are given a default value in that fixture, and therefore
+# should not be overwritten on update.
 SAMPLE_CONTACT_PARAMS = [
     ("minimal_contact_data", set()),
     ("maximal_contact_data", set()),
     ("example_contact_data", set()),
     ("to_add_contact_data", set()),
     ("simple_default_contact_data", {"amo"}),
     ("default_newsletter_contact_data", {"newsletters"}),
+    ("default_waitlist_contact_data", {"waitlists"}),
 ]
 
 FAKE_STRIPE_CUSTOMER_ID = fake_stripe_id("cus", "customer")
@@ -522,6 +524,21 @@ def default_newsletter_contact(
     return default_newsletter_contact_data
 
 
+@pytest.fixture
+def default_waitlist_contact_data():
+    contact = ContactSchema(
+        email=schemas.EmailInSchema(
+            basket_token="6D854AA2-C1CF-4DC0-9581-2641AD3FA52D",
+            email_id=UUID("0A6ECCA3-D007-4BD4-B596-C09DA94F0FEF"),
+            mailing_country="us",
+            primary_email="with-default-waitlist@example.com",
+            sfdc_id="7772A000001aBAcXZY",
+        ),
+        waitlists=[],
+    )
+    return contact
+
+
 @pytest.fixture
 def sample_contacts(minimal_contact, maximal_contact, example_contact):
     return {

tests/unit/routers/contacts/test_api.py

@@ -5,6 +5,7 @@
 import pytest
 
 from ctms.schemas import ContactInSchema, ContactSchema, NewsletterInSchema
+from ctms.schemas.waitlist import WaitlistInSchema
 from tests.unit.conftest import SAMPLE_CONTACT_PARAMS
 
 API_TEST_CASES: Tuple[Tuple[str, str, Any], ...] = (
@@ -90,15 +91,15 @@ def test_authorized_api_call_succeeds(client, example_contact, method, path, par
             assert resp.status_code == 200
 
 
-def _subscribe(contact):
+def _subscribe_newsletter(contact):
     contact.newsletters.append(NewsletterInSchema(name="new-newsletter"))
 
 
-def _unsubscribe(contact):
+def _unsubscribe_newsletter(contact):
     contact.newsletters = contact.newsletters[0:-1]
 
 
-def _subscribe_and_change(contact):
+def _subscribe_newsletters_and_change(contact):
     if contact.newsletters:
         contact.newsletters[-1].subscribed = not contact.newsletters[-1].subscribed
     contact.newsletters.append(
@@ -109,6 +110,13 @@ def _subscribe_and_change(contact):
     )
 
 
+def _subscribe_waitlists_and_change(contact):
+    if contact.waitlists:
+        contact.waitlists[-1].subscribed = not contact.waitlists[-1].subscribed
+    contact.waitlists.append(WaitlistInSchema(name="a-waitlist", subscribed=False))
+    contact.waitlists.append(WaitlistInSchema(name="another-waitlist", subscribed=True))
+
+
 def _un_amo(contact):
     if contact.amo:
         del contact.amo
@@ -119,11 +127,12 @@ def _change_email(contact):
 
 
 _test_get_put_modifiers = [
-    _subscribe,
-    _unsubscribe,
+    _subscribe_newsletter,
+    _unsubscribe_newsletter,
     _un_amo,
     _change_email,
-    _subscribe_and_change,
+    _subscribe_newsletters_and_change,
+    _subscribe_waitlists_and_change,
 ]
 
 

tests/unit/test_crud.py

@@ -31,7 +31,12 @@
     retry_acoustic_record,
     schedule_acoustic_record,
 )
-from ctms.models import AcousticField, AcousticNewsletterMapping, PendingAcousticRecord
+from ctms.models import (
+    AcousticField,
+    AcousticNewsletterMapping,
+    Email,
+    PendingAcousticRecord,
+)
 from ctms.schemas import (
     AddOnsInSchema,
     EmailInSchema,
@@ -530,6 +535,32 @@ def test_get_multiple_contacts_by_any_id(
         assert sorted(newsletter_names) == newsletter_names
 
 
+def test_create_or_update_contact_related_objects(dbsession, email_factory):
+    email = email_factory(
+        newsletters=3,
+        waitlists=3,
+    )
+    dbsession.flush()
+
+    new_source = "http://waitlists.example.com"
+    putdata = ContactPutSchema(
+        email=EmailInSchema(email_id=email.email_id, primary_email=email.primary_email),
+        newsletters=[
+            NewsletterInSchema(name=email.newsletters[0].name, source=new_source)
+        ],
+        waitlists=[WaitlistInSchema(name=email.waitlists[0].name, source=new_source)],
+    )
+    create_or_update_contact(dbsession, email.email_id, putdata, None)
+    dbsession.commit()
+
+    updated_email = dbsession.get(Email, email.email_id)
+    # Existing related objects were deleted and replaced by the specified list.
+    assert len(updated_email.newsletters) == 1
+    assert len(updated_email.waitlists) == 1
+    assert updated_email.newsletters[0].source == new_source
+    assert updated_email.waitlists[0].source == new_source
+
+
 @pytest.mark.parametrize("with_lock", (True, False))
 def test_get_stripe_customer_by_existing_fxa_id(
     dbsession, with_lock, stripe_customer_factory