Ref #708: fix Acoustic newsletters subscription timestamps

In this pull-request, I fix a piece of code that never worked: the newsletter subscription timestamps sent to Acoustic were never read from the database.

Basically schemas do not read the timestamps fields from the DB. This could be a possible fix, but my confidence in its elegance is kind of low.

This codebase has a really dark power of demotivation. Especially if you ever worked with tools like Django Rest Framework.

Plus, most test case assertions are not designed in a flexible way.

ctms/acoustic_service.py

@@ -240,14 +240,12 @@ def _newsletter_converter(self, acoustic_main_table, contact, newsletters_mappin
             }
 
             if newsletter.name in newsletters_mapping:
-                newsletter_dict = newsletter.dict()
-                _today = datetime.date.today().isoformat()
-                newsletter_template["create_timestamp"] = newsletter_dict.get(
-                    "create_timestamp", _today
-                )
-                newsletter_template["update_timestamp"] = newsletter_dict.get(
-                    "update_timestamp", _today
-                )
+                newsletter_template[
+                    "create_timestamp"
+                ] = newsletter.create_timestamp.date().isoformat()
+                newsletter_template[
+                    "update_timestamp"
+                ] = newsletter.update_timestamp.date().isoformat()
                 newsletter_template["newsletter_name"] = newsletter.name
                 newsletter_template["newsletter_unsub_reason"] = newsletter.unsub_reason
                 _source = newsletter.source

ctms/crud.py

@@ -10,6 +10,9 @@
 from sqlalchemy.dialects.postgresql import insert
 from sqlalchemy.orm import Session, joinedload, load_only, selectinload
 
+from ctms.schemas.newsletter import NewsletterTableSchema
+from ctms.schemas.waitlist import WaitlistTableSchema
+
 from .auth import hash_password
 from .backport_legacy_waitlists import format_legacy_vpn_relay_waitlist_input
 from .database import Base
@@ -173,7 +176,22 @@ def get_contact_by_email_id(db: Session, email_id: UUID4) -> Optional[ContactSch
     email = get_email(db, email_id)
     if email is None:
         return None
-    return ContactSchema.from_email(email)
+    contact = ContactSchema.from_email(email)
+
+    # In the `ContactSchema` instance, the newsletters and waitlists are
+    # instances of `NewsletterSchema` and `WaitlistSchema`,
+    # which loose the `create_timestamp` and `update_timestamp` fields.
+    # Note: the whole code base with regards to models and schemas is a real house of cards.
+    contact_newsletters = get_newsletters_by_email_id(db, email_id)
+    contact.newsletters = [
+        NewsletterTableSchema.from_newsletter(nl) for nl in contact_newsletters
+    ]
+    contact_waitlists = get_waitlists_by_email_id(db, email_id)
+    contact.waitlists = [
+        WaitlistTableSchema.from_waitlist(wl) for wl in contact_waitlists
+    ]
+
+    return contact
 
 
 def get_contacts_by_any_id(

ctms/schemas/newsletter.py

@@ -1,9 +1,13 @@
 from datetime import datetime, timezone
-from typing import Literal, Optional
+from typing import TYPE_CHECKING, Literal, Optional
 
-from pydantic import AnyUrl, Field
+from pydantic import UUID4, AnyUrl, Field
 
 from .base import ComparableBase
+from .email import EMAIL_ID_DESCRIPTION, EMAIL_ID_EXAMPLE
+
+if TYPE_CHECKING:
+    from ctms.models import Newsletter
 
 
 class NewsletterBase(ComparableBase):
@@ -46,6 +50,38 @@ class Config:
 NewsletterSchema = NewsletterBase
 
 
+class NewsletterTableSchema(NewsletterBase):
+    email_id: UUID4 = Field(
+        description=EMAIL_ID_DESCRIPTION,
+        example=EMAIL_ID_EXAMPLE,
+    )
+    create_timestamp: datetime = Field(
+        description="Newsletter data creation timestamp",
+        example="2020-12-05T19:21:50.908000+00:00",
+    )
+    update_timestamp: datetime = Field(
+        description="Newsletter data update timestamp",
+        example="2021-02-04T15:36:57.511000+00:00",
+    )
+
+    @classmethod
+    def from_newsletter(cls, newsletter: "Newsletter") -> "NewsletterTableSchema":
+        return cls(
+            email_id=newsletter.email_id,
+            name=newsletter.name,
+            subscribed=newsletter.subscribed,
+            format=newsletter.format,
+            lang=newsletter.lang,
+            source=newsletter.source,
+            unsub_reason=newsletter.unsub_reason,
+            create_timestamp=newsletter.create_timestamp,
+            update_timestamp=newsletter.update_timestamp,
+        )
+
+    class Config:
+        extra = "forbid"
+
+
 class UpdatedNewsletterInSchema(NewsletterInSchema):
     update_timestamp: datetime = Field(
         default_factory=lambda: datetime.now(timezone.utc),

ctms/schemas/waitlist.py

@@ -1,11 +1,14 @@
 from datetime import datetime, timezone
-from typing import Optional
+from typing import TYPE_CHECKING, Optional
 
 from pydantic import UUID4, AnyUrl, Field, root_validator
 
 from .base import ComparableBase
 from .email import EMAIL_ID_DESCRIPTION, EMAIL_ID_EXAMPLE
 
+if TYPE_CHECKING:
+    from ctms.models import Waitlist
+
 
 class WaitlistBase(ComparableBase):
     """
@@ -95,6 +98,17 @@ class WaitlistTableSchema(WaitlistBase):
         example="2021-02-04T15:36:57.511000+00:00",
     )
 
+    @classmethod
+    def from_waitlist(cls, waitlist: "Waitlist") -> "WaitlistTableSchema":
+        return cls(
+            email_id=waitlist.email_id,
+            name=waitlist.name,
+            source=waitlist.source,
+            fields=waitlist.fields,
+            create_timestamp=waitlist.create_timestamp,
+            update_timestamp=waitlist.update_timestamp,
+        )
+
     class Config:
         extra = "forbid"
 

tests/unit/conftest.py

@@ -28,6 +28,7 @@
     get_all_acoustic_fields,
     get_all_acoustic_newsletters_mapping,
     get_amo_by_email_id,
+    get_contact_by_email_id,
     get_contacts_by_any_id,
     get_fxa_by_email_id,
     get_mofo_by_email_id,
@@ -218,7 +219,7 @@ def minimal_contact(minimal_contact_data, dbsession):
         get_metrics(),
     )
     dbsession.commit()
-    return minimal_contact_data
+    return get_contact_by_email_id(dbsession, minimal_contact_data.email.email_id)
 
 
 @pytest.fixture
@@ -325,7 +326,7 @@ def maximal_contact(dbsession, maximal_contact_data):
         get_metrics(),
     )
     dbsession.commit()
-    return maximal_contact_data
+    return get_contact_by_email_id(dbsession, maximal_contact_data.email.email_id)
 
 
 @pytest.fixture
@@ -353,7 +354,7 @@ def example_contact(dbsession, example_contact_data):
         get_metrics(),
     )
     dbsession.commit()
-    return example_contact_data
+    return get_contact_by_email_id(dbsession, example_contact_data.email.email_id)
 
 
 @pytest.fixture

tests/unit/test_acoustic_service.py

@@ -7,7 +7,7 @@
 
 from ctms import acoustic_service
 from ctms.acoustic_service import CTMSToAcousticService
-from ctms.crud import get_contact_by_email_id
+from ctms.crud import get_contact_by_email_id, get_newsletters_by_email_id
 from ctms.schemas.contact import ContactSchema
 
 CTMS_ACOUSTIC_MAIN_TABLE_ID = "1"
@@ -103,6 +103,32 @@ def test_ctms_to_acoustic_newsletters(
     ]
 
 
+def test_ctms_to_acoustic_newsletter_timestamps(
+    dbsession,
+    base_ctms_acoustic_service,
+    minimal_contact,
+    main_acoustic_fields,
+    acoustic_newsletters_mapping,
+):
+    # Set timestamps on DB objects.
+    newsletters = get_newsletters_by_email_id(dbsession, minimal_contact.email.email_id)
+    app_dev_nl = [nl for nl in newsletters if nl.name == "app-dev"][0]
+    app_dev_nl.create_timestamp = "1982-05-08T13:20"
+    app_dev_nl.update_timestamp = "2023-06-19T12:17"
+    dbsession.add(app_dev_nl)
+    dbsession.commit()
+    # Reload contact from DB.
+    minimal_contact = get_contact_by_email_id(dbsession, minimal_contact.email.email_id)
+
+    (_, newsletters_rows, _) = base_ctms_acoustic_service.convert_ctms_to_acoustic(
+        minimal_contact, main_acoustic_fields, acoustic_newsletters_mapping
+    )
+
+    app_dev_row = [r for r in newsletters_rows if r["newsletter_name"] == "app-dev"][0]
+    assert app_dev_row["create_timestamp"] == "1982-05-08"
+    assert app_dev_row["update_timestamp"] == "2023-06-19"
+
+
 def test_ctms_to_acoustic_waitlists_minimal(
     base_ctms_acoustic_service,
     minimal_contact,

tests/unit/test_api_patch.py

@@ -19,6 +19,11 @@
 )
 
 
+def omit_keys(dic, *keys):
+    """Return copy without specified keys"""
+    return {k: v for k, v in dic.items() if k not in keys}
+
+
 def swap_bool(existing):
     """Use the opposite of the existing value for this boolean."""
     return not existing
@@ -64,6 +69,14 @@ def test_patch_one_new_value(client, contact_name, group_name, key, value, reque
     """PATCH can update a single value."""
     contact = request.getfixturevalue(contact_name)
     expected = json.loads(CTMSResponse(**contact.dict()).json())
+    expected["newsletters"] = [
+        omit_keys(nl, "email_id", "create_timestamp", "update_timestamp")
+        for nl in expected["newsletters"]
+    ]
+    expected["waitlists"] = [
+        omit_keys(nl, "email_id", "create_timestamp", "update_timestamp")
+        for nl in expected["waitlists"]
+    ]
     existing_value = expected[group_name][key]
 
     # Set dynamic test values
@@ -130,6 +143,14 @@ def test_patch_to_default(client, maximal_contact, group_name, key):
     """PATCH can set a field to the default value."""
     email_id = maximal_contact.email.email_id
     expected = json.loads(CTMSResponse(**maximal_contact.dict()).json())
+    expected["newsletters"] = [
+        omit_keys(nl, "email_id", "create_timestamp", "update_timestamp")
+        for nl in expected["newsletters"]
+    ]
+    expected["waitlists"] = [
+        omit_keys(nl, "email_id", "create_timestamp", "update_timestamp")
+        for nl in expected["waitlists"]
+    ]
     existing_value = expected[group_name][key]
 
     # Load the default value from the schema
@@ -191,6 +212,14 @@ def test_patch_cannot_set_timestamps(client, maximal_contact):
     assert expected["products"] == []
     assert "products" not in actual
     actual["products"] = []
+    expected["newsletters"] = [
+        omit_keys(nl, "email_id", "create_timestamp", "update_timestamp")
+        for nl in expected["newsletters"]
+    ]
+    expected["waitlists"] = [
+        omit_keys(nl, "email_id", "create_timestamp", "update_timestamp")
+        for nl in expected["waitlists"]
+    ]
     # The response shows computed fields for retro-compat. Contact schema
     # does not have them.
     # TODO waitlist: remove once Basket reads from `waitlists` list.
@@ -323,7 +352,9 @@ def test_patch_to_unsubscribe(client, maximal_contact):
     """PATCH can unsubscribe by setting a newsletter field."""
     email_id = maximal_contact.email.email_id
     existing_news_data = maximal_contact.newsletters[1].dict()
-    assert existing_news_data == {
+    assert omit_keys(
+        existing_news_data, "email_id", "create_timestamp", "update_timestamp"
+    ) == {
         "format": "T",
         "lang": "fr",
         "name": "common-voice",
@@ -476,7 +507,10 @@ def test_patch_does_not_add_an_unsubscribed_waitlist(client, maximal_contact):
 def test_patch_to_update_a_waitlist(client, maximal_contact):
     """PATCH can update a waitlist."""
     email_id = maximal_contact.email.email_id
-    existing = [wl.dict() for wl in maximal_contact.waitlists]
+    existing = [
+        omit_keys(wl.dict(), "email_id", "create_timestamp", "update_timestamp")
+        for wl in maximal_contact.waitlists
+    ]
     existing[0]["fields"]["geo"] = "ca"
     patch_data = {"waitlists": existing}
     resp = client.patch(f"/ctms/{email_id}", json=patch_data, allow_redirects=True)
@@ -491,8 +525,9 @@ def test_patch_to_update_a_waitlist(client, maximal_contact):
 def test_patch_to_remove_a_waitlist(client, maximal_contact):
     """PATCH can remove a single waitlist."""
     email_id = maximal_contact.email.email_id
-    existing = [wl.dict() for wl in maximal_contact.waitlists]
-    patch_data = {"waitlists": [{**existing[-1], "subscribed": False}]}
+    patch_data = {
+        "waitlists": [{"name": maximal_contact.waitlists[-1].name, "subscribed": False}]
+    }
     resp = client.patch(f"/ctms/{email_id}", json=patch_data, allow_redirects=True)
     assert resp.status_code == 200
     actual = resp.json()

tests/unit/test_bulk.py

@@ -109,6 +109,18 @@ def test_get_ctms_bulk_by_timerange(
     assert "items" in results
     assert len(results["items"]) > 0
     dict_contact_expected = sorted_list[1].dict()
+    # Since the contact data contains more info than the response, let's strip fields
+    # to be able to compare dict down the line (sic).
+    omit_fields = ("email_id", "create_timestamp", "update_timestamp")
+    dict_contact_expected["newsletters"] = [
+        {k: v for k, v in nl.items() if k not in omit_fields}
+        for nl in dict_contact_expected["newsletters"]
+    ]
+    dict_contact_expected["waitlists"] = [
+        {k: v for k, v in wl.items() if k not in omit_fields}
+        for wl in dict_contact_expected["waitlists"]
+    ]
+
     dict_contact_actual = CTMSResponse.parse_obj(results["items"][0]).dict()
     # products list is not (yet) in output schema
     assert dict_contact_expected["products"] == []

tests/unit/test_crud.py

@@ -453,9 +453,10 @@ def test_get_bulk_contacts_some_after_higher_limit(
         limit=2,
         after_email_id=after_id,
     )
-    assert len(bulk_contact_list) == 2
-    assert last_contact in bulk_contact_list
-    assert sorted_list[-2] in bulk_contact_list
+    bulk_contact_list_ids = [c.email.email_id for c in bulk_contact_list]
+    assert len(bulk_contact_list_ids) == 2
+    assert last_contact.email.email_id in bulk_contact_list_ids
+    assert sorted_list[-2].email.email_id in bulk_contact_list_ids
 
 
 def test_get_bulk_contacts_some_after(
@@ -482,7 +483,7 @@ def test_get_bulk_contacts_some_after(
         after_email_id=after_id,
     )
     assert len(bulk_contact_list) == 1
-    assert last_contact in bulk_contact_list
+    assert bulk_contact_list[0].email.email_id == last_contact.email.email_id
 
 
 def test_get_bulk_contacts_some(
@@ -502,9 +503,10 @@ def test_get_bulk_contacts_some(
         limit=10,
     )
     assert len(bulk_contact_list) >= 3
-    assert example_contact in bulk_contact_list
-    assert maximal_contact in bulk_contact_list
-    assert minimal_contact in bulk_contact_list
+    bulk_contact_list_ids = [c.email.email_id for c in bulk_contact_list]
+    assert example_contact.email.email_id in bulk_contact_list_ids
+    assert maximal_contact.email.email_id in bulk_contact_list_ids
+    assert minimal_contact.email.email_id in bulk_contact_list_ids
 
 
 def test_get_bulk_contacts_one(dbsession, example_contact):