From 35ad6ea32a6e4f300de09f01fdf45fba48606af5 Mon Sep 17 00:00:00 2001 From: Mathieu Leplatre Date: Tue, 20 Jun 2023 13:38:04 +0200 Subject: [PATCH] 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 | 14 ++++------ ctms/crud.py | 20 +++++++++++++- ctms/schemas/newsletter.py | 40 +++++++++++++++++++++++++-- ctms/schemas/waitlist.py | 16 ++++++++++- tests/unit/conftest.py | 7 +++-- tests/unit/test_acoustic_service.py | 28 ++++++++++++++++++- tests/unit/test_api_patch.py | 43 ++++++++++++++++++++++++++--- tests/unit/test_bulk.py | 12 ++++++++ tests/unit/test_crud.py | 16 ++++++----- 9 files changed, 169 insertions(+), 27 deletions(-) diff --git a/ctms/acoustic_service.py b/ctms/acoustic_service.py index 63158718..cf7efffc 100644 --- a/ctms/acoustic_service.py +++ b/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 diff --git a/ctms/crud.py b/ctms/crud.py index f865087e..9838a0ac 100644 --- a/ctms/crud.py +++ b/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( diff --git a/ctms/schemas/newsletter.py b/ctms/schemas/newsletter.py index 1feed151..15615a8f 100644 --- a/ctms/schemas/newsletter.py +++ b/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), diff --git a/ctms/schemas/waitlist.py b/ctms/schemas/waitlist.py index c25f93be..2815860b 100644 --- a/ctms/schemas/waitlist.py +++ b/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" diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py index 328ecfc9..ca1678fe 100644 --- a/tests/unit/conftest.py +++ b/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 diff --git a/tests/unit/test_acoustic_service.py b/tests/unit/test_acoustic_service.py index 34583ad2..20c5f5de 100644 --- a/tests/unit/test_acoustic_service.py +++ b/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, diff --git a/tests/unit/test_api_patch.py b/tests/unit/test_api_patch.py index 0af4681a..05b364af 100644 --- a/tests/unit/test_api_patch.py +++ b/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() diff --git a/tests/unit/test_bulk.py b/tests/unit/test_bulk.py index 8460c164..bf5b7a46 100644 --- a/tests/unit/test_bulk.py +++ b/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"] == [] diff --git a/tests/unit/test_crud.py b/tests/unit/test_crud.py index 5e1ec8c1..0e20a7c7 100644 --- a/tests/unit/test_crud.py +++ b/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):