Merge pull request #112 from mozilla-it/issue-55

Contact PUT

.pre-commit-config.yaml

@@ -5,6 +5,7 @@ repos:
         name: pylint
         entry: pylint
         language: system
+        exclude: ^(migrations|tests)/
         types: [python]
   - repo: https://github.com/pre-commit/pre-commit-hooks
     rev: v2.1.0

ctms/app.py

@@ -21,6 +21,7 @@
 )
 from .crud import (
     create_contact,
+    create_or_update_contact,
     get_api_client_by_id,
     get_contact_by_email_id,
     get_contacts_by_any_id,
@@ -32,6 +33,7 @@
     ApiClientSchema,
     BadRequestResponse,
     ContactInSchema,
+    ContactPutSchema,
     ContactSchema,
     CTMSResponse,
     EmailSchema,
@@ -249,7 +251,7 @@ def read_ctms_by_email_id(
 
 @app.post(
     "/ctms",
-    summary="Create a contact, generating an id",
+    summary="Create a contact, generating an id if not specified.",
     responses={409: {"model": BadRequestResponse}},
     tags=["Public"],
 )
@@ -276,6 +278,43 @@ def create_ctms_contact(
     return RedirectResponse(status_code=303, url=f"/ctms/{email_id}")
 
 
+@app.put(
+    "/ctms/{email_id}",
+    summary="""Create or replace a contact, an email_id must be provided.
+               Compare this to POST where we will generate one for you if you want.
+               This is intended to be used to send back a contact you have modified locally
+               and therefore the input schema is a full Contact.""",
+    responses={409: {"model": BadRequestResponse}, 422: {"model": BadRequestResponse}},
+    tags=["Public"],
+)
+def create_or_update_ctms_contact(
+    contact: ContactPutSchema,
+    email_id: UUID = Path(..., title="The Email ID"),
+    db: Session = Depends(get_db),
+    api_client: ApiClientSchema = Depends(get_enabled_api_client),
+):
+    if contact.email.email_id:
+        if contact.email.email_id != email_id:
+            raise HTTPException(
+                status_code=422,
+                detail="email_id in path must match email_id in contact",
+            )
+    else:
+        contact.email.email_id = email_id
+    try:
+        create_or_update_contact(db, email_id, contact)
+        db.commit()
+    except Exception as e:  # pylint:disable = W0703
+        db.rollback()
+        if isinstance(e, IntegrityError):
+            raise HTTPException(
+                status_code=409,
+                detail="Contact with primary_email or basket_token already exists",
+            ) from e
+        raise e from e
+    return RedirectResponse(status_code=303, url=f"/ctms/{email_id}")
+
+
 @app.get(
     "/identities",
     summary="Get identities associated with alternate IDs",

ctms/crud.py

@@ -1,6 +1,7 @@
 from typing import Dict, List, Optional
 
 from pydantic import UUID4, EmailStr
+from sqlalchemy.dialects.postgresql import insert
 from sqlalchemy.orm import Session, joinedload, selectinload
 
 from .auth import hash_password
@@ -16,7 +17,9 @@
     AddOnsInSchema,
     ApiClientSchema,
     ContactInSchema,
+    ContactPutSchema,
     EmailInSchema,
+    EmailPutSchema,
     FirefoxAccountsInSchema,
     NewsletterInSchema,
     VpnWaitlistInSchema,
@@ -138,18 +141,50 @@ def create_amo(db: Session, email_id: UUID4, amo: AddOnsInSchema):
     db.add(db_amo)
 
 
+def create_or_update_amo(db: Session, email_id: UUID4, amo: Optional[AddOnsInSchema]):
+    if not amo or amo.is_default():
+        db.query(AmoAccount).filter(AmoAccount.email_id == email_id).delete()
+        return
+    stmt = insert(AmoAccount).values(email_id=email_id, **amo.dict())
+    stmt = stmt.on_conflict_do_update(
+        index_elements=[AmoAccount.email_id], set_=amo.dict()
+    )
+    db.execute(stmt)
+
+
 def create_email(db: Session, email: EmailInSchema):
     db_email = Email(**email.dict())
     db.add(db_email)
 
 
+def create_or_update_email(db: Session, email: EmailPutSchema):
+    stmt = insert(Email).values(**email.dict())
+    stmt = stmt.on_conflict_do_update(
+        index_elements=[Email.email_id], set_=email.dict()
+    )
+    db.execute(stmt)
+
+
 def create_fxa(db: Session, email_id: UUID4, fxa: FirefoxAccountsInSchema):
     if fxa.is_default():
         return
     db_fxa = FirefoxAccount(email_id=email_id, **fxa.dict())
     db.add(db_fxa)
 
 
+def create_or_update_fxa(
+    db: Session, email_id: UUID4, fxa: Optional[FirefoxAccountsInSchema]
+):
+    if not fxa or fxa.is_default():
+        (db.query(FirefoxAccount).filter(FirefoxAccount.email_id == email_id).delete())
+        return
+    stmt = insert(FirefoxAccount).values(email_id=email_id, **fxa.dict())
+    stmt = stmt.on_conflict_do_update(
+        index_elements=[FirefoxAccount.email_id], set_=fxa.dict()
+    )
+    db.execute(stmt)
+
+
 def create_vpn_waitlist(
     db: Session, email_id: UUID4, vpn_waitlist: VpnWaitlistInSchema
 ):
@@ -159,13 +194,49 @@ def create_vpn_waitlist(
     db.add(db_vpn_waitlist)
 
 
+def create_or_update_vpn_waitlist(
+    db: Session, email_id: UUID4, vpn_waitlist: Optional[VpnWaitlistInSchema]
+):
+    if not vpn_waitlist or vpn_waitlist.is_default():
+        db.query(VpnWaitlist).filter(VpnWaitlist.email_id == email_id).delete()
+        return
+    stmt = insert(VpnWaitlist).values(email_id=email_id, **vpn_waitlist.dict())
+    stmt = stmt.on_conflict_do_update(
+        index_elements=[VpnWaitlist.email_id], set_=vpn_waitlist.dict()
+    )
+    db.execute(stmt)
+
+
 def create_newsletter(db: Session, email_id: UUID4, newsletter: NewsletterInSchema):
     if newsletter.is_default():
         return
     db_newsletter = Newsletter(email_id=email_id, **newsletter.dict())
     db.add(db_newsletter)
 
 
+def create_or_update_newsletters(
+    db: Session, email_id: UUID4, newsletters: List[NewsletterInSchema]
+):
+    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(
+        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(
+            [{"email_id": email_id, **n.dict()} for n in newsletters]
+        )
+        stmt = stmt.on_conflict_do_update(
+            constraint="uix_email_name", set_=dict(stmt.excluded)
+        )
+
+        db.execute(stmt)
+
+
 def create_contact(db: Session, email_id: UUID4, contact: ContactInSchema):
     create_email(db, contact.email)
     if contact.amo:
@@ -178,6 +249,14 @@ def create_contact(db: Session, email_id: UUID4, contact: ContactInSchema):
         create_newsletter(db, email_id, newsletter)
 
 
+def create_or_update_contact(db: Session, email_id: UUID4, contact: ContactPutSchema):
+    create_or_update_email(db, contact.email)
+    create_or_update_amo(db, email_id, contact.amo)
+    create_or_update_fxa(db, email_id, contact.fxa)
+    create_or_update_vpn_waitlist(db, email_id, contact.vpn_waitlist)
+    create_or_update_newsletters(db, email_id, contact.newsletters)
+
+
 def create_api_client(db: Session, api_client: ApiClientSchema, secret):
     hashed_secret = hash_password(secret)
     db_api_client = ApiClient(hashed_secret=hashed_secret, **api_client.dict())

ctms/models.py

@@ -18,6 +18,7 @@
 
 class Email(Base):
     __tablename__ = "emails"
+    __mapper_args__ = {"eager_defaults": True}
 
     email_id = Column(UUID(as_uuid=True), primary_key=True)
     primary_email = Column(String(255), unique=True, nullable=False)
@@ -76,7 +77,7 @@ class Newsletter(Base):
 
     email = relationship("Email", back_populates="newsletters", uselist=False)
 
-    UniqueConstraint("email_id", "name", name="uix_email_name")
+    __table_args__ = (UniqueConstraint("email_id", "name", name="uix_email_name"),)
 
 
 class FirefoxAccount(Base):

ctms/schemas/__init__.py

@@ -1,7 +1,13 @@
 from .addons import AddOnsInSchema, AddOnsSchema
 from .api_client import ApiClientSchema
-from .contact import ContactInSchema, ContactSchema, CTMSResponse, IdentityResponse
-from .email import EmailInSchema, EmailSchema
+from .contact import (
+    ContactInSchema,
+    ContactPutSchema,
+    ContactSchema,
+    CTMSResponse,
+    IdentityResponse,
+)
+from .email import EmailInSchema, EmailPutSchema, EmailSchema
 from .fxa import FirefoxAccountsInSchema, FirefoxAccountsSchema
 from .newsletter import NewsletterInSchema, NewsletterSchema
 from .vpn import VpnWaitlistInSchema, VpnWaitlistSchema

ctms/schemas/contact.py

@@ -1,11 +1,11 @@
-from typing import List, Literal, Optional
+from typing import List, Literal, Optional, Set
 from uuid import UUID
 
 from pydantic import BaseModel, EmailStr, Field
 
 from .addons import AddOnsInSchema, AddOnsSchema
 from .base import ComparableBase
-from .email import EmailInSchema, EmailSchema
+from .email import EmailBase, EmailInSchema, EmailPutSchema, EmailSchema
 from .fxa import FirefoxAccountsInSchema, FirefoxAccountsSchema
 from .newsletter import NewsletterInSchema, NewsletterSchema
 from .vpn import VpnWaitlistInSchema, VpnWaitlistSchema
@@ -37,12 +37,29 @@ def as_identity_response(self) -> "IdentityResponse":
             sfdc_id=getattr(self.email, "sfdc_id", None),
         )
 
-
-class ContactInSchema(ComparableBase):
+    def find_default_fields(self) -> Set[str]:
+        """Return names of fields that contain default values only"""
+        default_fields = set()
+        if hasattr(self, "amo") and self.amo and self.amo.is_default():
+            default_fields.add("amo")
+        if hasattr(self, "fxa") and self.fxa and self.fxa.is_default():
+            default_fields.add("fxa")
+        if (
+            hasattr(self, "vpn_waitlist")
+            and self.vpn_waitlist
+            and self.vpn_waitlist.is_default()
+        ):
+            default_fields.add("vpn_waitlist")
+        if all(n.is_default() for n in self.newsletters):
+            default_fields.add("newsletters")
+        return default_fields
+
+
+class ContactInBase(ComparableBase):
     """A contact as provided by callers."""
 
     amo: Optional[AddOnsInSchema] = None
-    email: EmailInSchema
+    email: EmailBase
     fxa: Optional[FirefoxAccountsInSchema] = None
     newsletters: List[NewsletterInSchema] = Field(
         default=[],
@@ -70,6 +87,18 @@ def _noneify(field):
         return True
 
 
+class ContactInSchema(ContactInBase):
+    """A contact as provided by callers when using POST. This is nearly identical to the ContactPutSchema but doesn't require an email_id."""
+
+    email: EmailInSchema
+
+
+class ContactPutSchema(ContactInBase):
+    """A contact as provided by callers when using POST. This is nearly identical to the ContactInSchema but does require an email_id."""
+
+    email: EmailPutSchema
+
+
 class CTMSResponse(BaseModel):
     """
     Response for /ctms/<email_id>

ctms/schemas/email.py

@@ -98,8 +98,19 @@ class EmailSchema(EmailBase):
 
 
 class EmailInSchema(EmailBase):
+    """Nearly identical to EmailPutSchema but the email_id is not required."""
+
     email_id: Optional[UUID4] = Field(
         default=None,
         description=EMAIL_ID_DESCRIPTION,
         example=EMAIL_ID_EXAMPLE,
     )
+
+
+class EmailPutSchema(EmailBase):
+    """Nearly identical to EmailInSchema but the email_id is required."""
+
+    email_id: UUID4 = Field(
+        description=EMAIL_ID_DESCRIPTION,
+        example=EMAIL_ID_EXAMPLE,
+    )

migrations/versions/20210319_3c94af7ca946_add_unique_constraint_to_newsletters.py

@@ -0,0 +1,23 @@
+"""add unique constraint to newsletters
+
+Revision ID: 3c94af7ca946
+Revises: 435d9701a9a4
+Create Date: 2021-03-19 11:39:52.506358
+
+"""
+import sqlalchemy as sa
+from alembic import op
+
+# revision identifiers, used by Alembic.
+revision = "3c94af7ca946"  # pragma: allowlist secret
+down_revision = "435d9701a9a4"  # pragma: allowlist secret
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    op.create_unique_constraint("uix_email_name", "newsletters", ["email_id", "name"])
+
+
+def downgrade():
+    op.drop_constraint("uix_email_name", "newsletters", type_="unique")

pyproject.toml

@@ -73,10 +73,12 @@ testpaths = [
     output-format = 'colorized'
     [tool.pylint.FORMAT]
     good-names = 'e,db,SessionLocal' # Normally pylint objects to these due to not being snake_case
+    [tool.pylint.TYPECHECK]
+    generated-members='alembic.*'
     [tool.pylint.MASTER]
     extension-pkg-whitelist='pydantic'
     ignore='third_party'
-    ignore-patterns = "test_.*.py" # Not worth fixing yet
+    ignore-patterns = "migrations/.*,tests/.*/.*" # Not worth fixing yet
 
 [tool.black]
 line-length = 88