Rewrite most POST and PUT tests (#958)

Simplify `POST` and `PUT` tests to have less indirection. Data is built and `POST`ed or `PUT` directly in the test, and assertions are clearly made in the test.

The `post_contact` / `put_contact` fixtures are now only used in one final test, but it is complex enough that it deserves its own PR.

Some drive-by fixes in this commit include:

* Run `lint --fix` in `format` Make target

* Generate `UUID`s on database insert

This is achieved by enabling the [uuid-ossp](https://www.postgresql.org/docs/current/uuid-ossp.html) extension, which happens in the migration present in this commit

Makefile

@@ -54,6 +54,7 @@ lint: $(INSTALL_STAMP)
 .PHONY: format
 format: $(INSTALL_STAMP)
 	bin/lint.sh format --fix
+	bin/lint.sh lint --fix
 
 .PHONY: db-only
 db-only: .env

ctms/models.py

@@ -3,6 +3,7 @@
 from sqlalchemy import (
     JSON,
     TIMESTAMP,
+    UUID,
     Boolean,
     Date,
     DateTime,
@@ -13,7 +14,6 @@
     Text,
     UniqueConstraint,
 )
-from sqlalchemy.dialects.postgresql import UUID
 from sqlalchemy.ext.declarative import declared_attr
 from sqlalchemy.ext.hybrid import Comparator, hybrid_property
 from sqlalchemy.orm import DeclarativeBase, Mapped, mapped_column, relationship
@@ -54,7 +54,9 @@ class Email(Base, TimestampMixin):
     __tablename__ = "emails"
     __mapper_args__ = {"eager_defaults": True}
 
-    email_id: Mapped[UUID4] = mapped_column(UUID(as_uuid=True), primary_key=True)
+    email_id = mapped_column(
+        UUID, primary_key=True, server_default="uuid_generate_v4()"
+    )
     primary_email = mapped_column(String(255), unique=True, nullable=False)
     basket_token = mapped_column(String(255), unique=True)
     sfdc_id = mapped_column(String(255), index=True)

migrations/versions/20240807_3689b813fe22_default_email_id_on_insert.py

@@ -0,0 +1,46 @@
+"""default email_id on insert
+
+Revision ID: 3689b813fe22
+Revises: f2041a868dca
+Create Date: 2024-08-07 12:19:09.966182
+
+"""
+# pylint: disable=no-member invalid-name
+# no-member is triggered by alembic.op, which has dynamically added functions
+# invalid-name is triggered by migration file names with a date prefix
+# invalid-name is triggered by top-level alembic constants like revision instead of REVISION
+
+import sqlalchemy as sa
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "3689b813fe22"  # pragma: allowlist secret
+down_revision = "f2041a868dca"  # pragma: allowlist secret
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.execute('CREATE EXTENSION IF NOT EXISTS "uuid-ossp";')
+    op.alter_column(
+        "emails",
+        "email_id",
+        existing_type=sa.UUID(),
+        server_default=sa.func.uuid_generate_v4(),
+        existing_nullable=False,
+    )
+    # ### end Alembic commands ###
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.execute('DROP EXTENSION IF EXISTS "uuid-ossp";')
+    op.alter_column(
+        "emails",
+        "email_id",
+        existing_type=sa.UUID(),
+        server_default=None,
+        existing_nullable=False,
+    )
+    # ### end Alembic commands ###

tests/factories/models.py

@@ -104,8 +104,6 @@ class EmailFactory(BaseSQLAlchemyModelFactory):
     class Meta:
         model = models.Email
 
-    # Actual Python UUID objects, not just their string representation
-    email_id = factory.LazyFunction(uuid4)
     primary_email = factory.Faker("email")
     # though this column is full of UUIDs, they're stored as strings, which is
     # what Faker generates

tests/unit/routers/contacts/test_api_post.py

@@ -1,123 +1,166 @@
 """Unit tests for POST /ctms (create record)"""
 
 import logging
-from uuid import UUID
+from uuid import uuid4
 
-import pytest
+from fastapi.encoders import jsonable_encoder
 
-from tests.unit.conftest import SAMPLE_CONTACT_PARAMS
+from ctms import models, schemas
 
-from .test_api import _compare_written_contacts
 
-POST_TEST_PARAMS = pytest.mark.parametrize(
-    "post_contact", SAMPLE_CONTACT_PARAMS, indirect=True
-)
+def test_create_basic_no_email_id(client, dbsession):
+    """Most straightforward contact creation succeeds when email_id is not a key."""
 
+    contact_data = jsonable_encoder(
+        schemas.ContactInSchema(
+            email={"primary_email": "hello@example.com"}
+        ).model_dump(exclude_none=True)
+    )
+    assert "email_id" not in contact_data["email"].keys()
 
-@POST_TEST_PARAMS
-def test_create_basic_no_id(post_contact):
-    """Most straightforward contact creation succeeds when email_id is not a key."""
+    resp = client.post("/ctms", json=contact_data)
+    assert resp.status_code == 201
 
-    def _remove_id(contact):
-        del contact.email.email_id
-        return contact
+    assert dbsession.get(models.Email, resp.json()["email"]["email_id"])
 
-    saved_contacts, sample, email_id = post_contact(
-        modifier=_remove_id, check_redirect=False
-    )
-    _compare_written_contacts(
-        saved_contacts[0], sample, email_id, ids_should_be_identical=False
+
+def test_create_basic_email_id_is_none(client, dbsession):
+    """Most straightforward contact creation succeeds when email_id is not a key."""
+
+    contact_data = jsonable_encoder(
+        schemas.ContactInSchema(email={"primary_email": "hello@example.com"})
     )
+    assert contact_data["email"]["email_id"] is None
 
+    resp = client.post("/ctms", json=contact_data)
+    assert resp.status_code == 201
 
-@POST_TEST_PARAMS
-def test_create_basic_id_is_none(post_contact):
-    """Most straightforward contact creation succeeds when email_id is None."""
+    assert dbsession.get(models.Email, resp.json()["email"]["email_id"])
 
-    def _remove_id(contact):
-        contact.email.email_id = None
-        return contact
 
-    saved_contacts, sample, email_id = post_contact(
-        modifier=_remove_id, check_redirect=False
-    )
-    _compare_written_contacts(
-        saved_contacts[0], sample, email_id, ids_should_be_identical=False
+def test_create_basic_with_id(client, dbsession, email_factory):
+    """Most straightforward contact creation succeeds when email_id is specified."""
+    provided_email_id = str(uuid4())
+
+    contact_data = jsonable_encoder(
+        schemas.ContactInSchema(
+            email={"email_id": provided_email_id, "primary_email": "hello@example.com"}
+        )
     )
+    assert contact_data["email"]["email_id"] == provided_email_id
 
+    resp = client.post("/ctms", json=contact_data)
+    assert resp.status_code == 201
 
-@POST_TEST_PARAMS
-def test_create_basic_with_id(post_contact):
-    """Most straightforward contact creation succeeds when email_id is specified."""
-    saved_contacts, sample, email_id = post_contact()
-    _compare_written_contacts(saved_contacts[0], sample, email_id)
+    assert dbsession.get(models.Email, provided_email_id)
 
 
-@POST_TEST_PARAMS
-def test_create_basic_idempotent(post_contact):
+def test_create_basic_idempotent(client, dbsession):
     """Creating a contact works across retries."""
-    saved_contacts, sample, email_id = post_contact()
-    _compare_written_contacts(saved_contacts[0], sample, email_id)
-    saved_contacts, _, _ = post_contact(code=200)
-    _compare_written_contacts(saved_contacts[0], sample, email_id)
 
+    contact_data = jsonable_encoder(
+        schemas.ContactInSchema(email={"primary_email": "hello@example.com"})
+    )
 
-@POST_TEST_PARAMS
-def test_create_basic_with_id_collision(post_contact):
-    """Creating a contact with the same id but different data fails."""
-    _, sample, _ = post_contact()
+    resp = client.post("/ctms", json=contact_data)
+    assert resp.status_code == 201
+    assert dbsession.get(models.Email, resp.json()["email"]["email_id"])
+
+    resp = client.post("/ctms", json=resp.json())
+    assert resp.status_code == 200
+    assert dbsession.get(models.Email, resp.json()["email"]["email_id"])
+    assert dbsession.query(models.Email).count() == 1
 
-    def _change_mailing(contact):
-        assert contact.email.mailing_country != "mx", "sample data has changed"
-        contact.email.mailing_country = "mx"
-        return contact
 
-    # We set check_written to False because the rows it would check for normally
-    # are actually here due to the first write
-    saved_contacts, _, _ = post_contact(
-        modifier=_change_mailing, code=409, check_redirect=False, check_written=False
+def test_create_basic_with_id_collision(client, email_factory):
+    """Creating a contact with the same id but different data fails."""
+
+    contact_data = jsonable_encoder(
+        schemas.ContactInSchema(
+            email={"primary_email": "hello@example.com", "email_lang": "en"}
+        )
     )
-    assert saved_contacts[0].email.mailing_country == sample.email.mailing_country
 
+    resp = client.post("/ctms", json=contact_data)
+    assert resp.status_code == 201
+
+    modified_data = resp.json()
+    modified_data["email"]["email_lang"] = "XX"
+
+    resp = client.post("/ctms", json=modified_data)
+    assert resp.status_code == 409
 
-@POST_TEST_PARAMS
-def test_create_basic_with_basket_collision(post_contact):
+
+def test_create_basic_with_email_collision(client, email_factory):
     """Creating a contact with diff ids but same email fails.
     We override the basket token so that we know we're not colliding on that here.
     See test_create_basic_with_email_collision below for that check
     """
-    saved_contacts, orig_sample, email_id = post_contact()
-    _compare_written_contacts(saved_contacts[0], orig_sample, email_id)
 
-    def _change_basket(contact):
-        contact.email.email_id = UUID("229cfa16-a8c9-4028-a9bd-fe746dc6bf73")
-        contact.email.basket_token = UUID("df9f7086-4949-4b2d-8fcf-49167f8f783d")
-        return contact
+    colliding_email = "foo@example.com"
+    email_factory(primary_email=colliding_email)
 
-    saved_contacts, _, _ = post_contact(
-        modifier=_change_basket, code=409, check_redirect=False
+    contact_data = jsonable_encoder(
+        schemas.ContactInSchema(email={"primary_email": colliding_email})
     )
-    _compare_written_contacts(saved_contacts[0], orig_sample, email_id)
+
+    resp = client.post("/ctms", json=contact_data)
+    assert resp.status_code == 409
 
 
-@POST_TEST_PARAMS
-def test_create_basic_with_email_collision(post_contact):
+def test_create_basic_with_basket_collision(client, email_factory):
     """Creating a contact with diff ids but same basket token fails.
     We override the email so that we know we're not colliding on that here.
     See other test for that check
     """
-    saved_contacts, orig_sample, email_id = post_contact()
-    _compare_written_contacts(saved_contacts[0], orig_sample, email_id)
+    colliding_basket_token = str(uuid4())
+    email_factory(basket_token=colliding_basket_token)
+
+    contact_data = jsonable_encoder(
+        schemas.ContactInSchema(
+            email={
+                "primary_email": "hello@example.com",
+                "basket_token": colliding_basket_token,
+            }
+        )
+    )
 
-    def _change_primary_email(contact):
-        contact.email.email_id = UUID("229cfa16-a8c9-4028-a9bd-fe746dc6bf73")
-        contact.email.primary_email = "foo@example.com"
-        return contact
+    resp = client.post("/ctms", json=contact_data)
+    assert resp.status_code == 409
 
-    saved_contacts, _, _ = post_contact(
-        modifier=_change_primary_email, code=409, check_redirect=False
+
+def test_default_is_not_written(client, dbsession):
+    """Schema defaults are not written to the database"""
+
+    contact = schemas.ContactInSchema(
+        email=schemas.EmailInSchema(primary_email="hello@example.com"),
+        fxa=schemas.FirefoxAccountsInSchema(),
+        mofo=schemas.MozillaFoundationInSchema(),
+        amo=schemas.AddOnsInSchema(),
+        newsletters=[],
+        waitlists=[],
     )
-    _compare_written_contacts(saved_contacts[0], orig_sample, email_id)
+    for attr in ["amo", "fxa", "mofo"]:
+        assert getattr(contact, attr).is_default()
+
+    resp = client.post("/ctms", json=jsonable_encoder(contact.model_dump()))
+    assert resp.status_code == 201
+
+    for model in [
+        models.Newsletter,
+        models.Waitlist,
+        models.FirefoxAccount,
+        models.AmoAccount,
+        models.MozillaFoundationContact,
+    ]:
+        assert dbsession.query(model).count() == 0
+
+
+def test_post_example_contact(client, example_contact_data):
+    """We can POST the example contact data that we include in our Swagger docs"""
+
+    resp = client.post("/ctms", json=jsonable_encoder(example_contact_data))
+    assert resp.status_code == 201
 
 
 def test_create_with_non_json_is_error(client, caplog):