Add subscribed/unsub_reason fields to waitlist (#707)

* Add subscribed/unsub reasons columns to DB model

* Do not delete records when recieving unsubscription

* Integration tests

* Expose and test setting of unsub_reason

* Be more verbose for integration tests

* Ignore unsubscribed waitlists in computed fields

* Update ctms/schemas/waitlist.py

Co-authored-by: grahamalama <gbeckley@mozilla.com>

* Subscribed field not nullable

* Rely on pytest verbose flag to debug retries of failing assertions

* Add ADR to explain the subscribed choice

* Re-evaluate the cost of the chosen solution

* Mention unsub_reason and reevaluate costs

* Adjust conclusion after meeting w/ Graham

---------

Co-authored-by: grahamalama <gbeckley@mozilla.com>

Makefile

@@ -44,7 +44,7 @@ $(INSTALL_STAMP): poetry.lock
 
 .PHONY: build
 build: .env
-	${DOCKER_COMPOSE} --version
+	${DOCKER_COMPOSE} version
 	${DOCKER_COMPOSE} build --build-arg userid=${CTMS_UID} --build-arg groupid=${CTMS_GID}
 
 .PHONY: lint
@@ -83,8 +83,11 @@ test: .env $(INSTALL_STAMP)
 
 .PHONY: integration-test
 integration-test: .env setup $(INSTALL_STAMP)
+	echo "Starting containers..."
 	${DOCKER_COMPOSE} --profile integration-test up --wait basket
+	echo "Start test suite..."
 	bin/integration-test.sh
+	echo "Done."
 ifneq (1, ${MK_KEEP_DOCKER_UP})
 	# Due to https://github.com/docker/compose/issues/2791 we have to explicitly
 	# rm all running containers

bin/integration-test.sh

@@ -17,5 +17,5 @@ cat tests/integration/basket-db-init.sql | $DOCKER_COMPOSE exec -T mysql mariadb
 # Create token in CTMS (will only work with specific CTMS_SECRET, see .sql source)
 cat tests/integration/ctms-db-init.sql | $DOCKER_COMPOSE exec -T postgres psql --user postgres -d postgres
 
-# docker-compose run basket django-admin loaddata ./basket/news/fixtures/newsletters.json
-$POETRY_RUN pytest tests/integration/
+# We don't capture tests output to trace retries of failing assertions.
+$POETRY_RUN pytest --capture=no tests/integration/

ctms/crud.py

@@ -479,38 +479,20 @@ def create_waitlist(
 ) -> Optional[Waitlist]:
     if waitlist.is_default():
         return None
-
-    # `create_waitlist` uses the `WaitlistInSchema``, which has a `subscribed`` property
-    # this makes sense because we're receiving this payload from Basket, and waitlists
-    # are just newsletters with naming conventions, so it has this property.
-    # Our Waitlist ORM model currently doesn't have an update property, so we need to remove it.
-    db_waitlist = Waitlist(email_id=email_id, **waitlist.dict(exclude={"subscribed"}))
+    db_waitlist = Waitlist(email_id=email_id, **waitlist.dict())
     db.add(db_waitlist)
     return db_waitlist
 
 
 def create_or_update_waitlists(
     db: Session, email_id: UUID4, waitlists: List[WaitlistInSchema]
 ):
-    # Remove waitlists that are marked as subscribed.
-    names_to_delete = [
-        waitlist.name for waitlist in waitlists if not waitlist.subscribed
-    ]
-    if names_to_delete:
-        db.query(Waitlist).filter(
-            Waitlist.email_id == email_id, Waitlist.name.in_(names_to_delete)
-        ).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!
-
     waitlists_to_upsert = [
-        UpdatedWaitlistInSchema(**waitlist.dict())
-        for waitlist in waitlists
-        if waitlist.subscribed
+        UpdatedWaitlistInSchema(**waitlist.dict()) for waitlist in waitlists
     ]
     if waitlists_to_upsert:
         stmt = insert(Waitlist).values(
-            [{"email_id": email_id, **wl.orm_dict()} for wl in waitlists_to_upsert]
+            [{"email_id": email_id, **wl.dict()} for wl in waitlists_to_upsert]
         )
         stmt = stmt.on_conflict_do_update(
             constraint="uix_wl_email_name", set_=dict(stmt.excluded)
@@ -643,19 +625,13 @@ def update_contact(
     if "waitlists" in update_data:
         if update_data["waitlists"] == "UNSUBSCRIBE":
             for waitlist_orm in existing.values():
-                db.delete(waitlist_orm)
-                email.waitlists.remove(waitlist_orm)
+                _update_orm(waitlist_orm, {"subscribed": False})
         else:
             for wl_update in update_data["waitlists"]:
                 if wl_update["name"] in existing:
                     waitlist_orm = existing[wl_update["name"]]
-                    # Delete waitlists when `subscribed` is False
-                    if not wl_update.get("subscribed", True):
-                        db.delete(waitlist_orm)
-                        email.waitlists.remove(waitlist_orm)
-                    else:
-                        # Update the Waitlist ORM object
-                        _update_orm(waitlist_orm, wl_update)
+                    # Update the Waitlist ORM object
+                    _update_orm(waitlist_orm, wl_update)
                 elif wl_update.get("subscribed", True):
                     new = create_waitlist(db, email_id, WaitlistInSchema(**wl_update))
                     email.waitlists.append(new)

ctms/models.py

@@ -127,6 +127,8 @@ class Waitlist(Base, TimestampMixin):
     email_id = Column(UUID(as_uuid=True), ForeignKey(Email.email_id), nullable=False)
     name = Column(String(255), nullable=False)
     source = Column(Text)
+    subscribed = Column(Boolean, nullable=False, default=True)
+    unsub_reason = Column(Text)
     fields = Column(JSON, nullable=False, server_default="'{}'::json")
 
     email = relationship("Email", back_populates="waitlists", uselist=False)

ctms/schemas/contact.py

@@ -384,6 +384,9 @@ def legacy_waitlists(cls, values):  # pylint: disable=no-self-argument
         values["vpn_waitlist"] = VpnWaitlistSchema()
         values["relay_waitlist"] = RelayWaitlistSchema()
         for waitlist in values.get("waitlists", []):
+            if not waitlist["subscribed"]:
+                # Ignore unsubscribed waitlists...
+                continue
             if isinstance(waitlist, dict):
                 # TODO: figure out why dict from `response_model` decorators param in app.py)
                 waitlist = WaitlistSchema(**waitlist)

ctms/schemas/waitlist.py

@@ -34,14 +34,21 @@ class WaitlistBase(ComparableBase):
     fields: dict = Field(
         default={}, description="Additional fields", example='{"platform": "linux"}'
     )
+    subscribed: bool = Field(
+        default=True, description="True to subscribe, False to unsubscribe"
+    )
+    unsub_reason: Optional[str] = Field(
+        default=None, description="Reason for unsubscribing"
+    )
 
     def __lt__(self, other):
         return self.name < other.name
 
     @root_validator
     def check_fields(cls, values):  # pylint:disable = no-self-argument
-        if "name" in values:
+        if "subscribed" in values and values["subscribed"] and "name" in values:
             validate_waitlist_fields(values["name"], values.get("fields", {}))
+        # If subscribed is False, we don't need to validate fields.
         return values
 
     class Config:
@@ -52,25 +59,7 @@ class Config:
 WaitlistSchema = WaitlistBase
 
 
-class WaitlistInSchema(WaitlistBase):
-    """Schema for input data."""
-
-    subscribed: bool = Field(
-        default=True, description="True to subscribe, False to unsubscribe"
-    )
-
-    @root_validator
-    def check_fields(cls, values):  # pylint:disable = no-self-argument
-        if "subscribed" in values and values["subscribed"]:
-            return super().check_fields(values)
-        # If subscribed is False, we don't need to validate fields.
-        return values
-
-    def orm_dict(self):
-        """TODO: is there a native way to exclude attrs for ORM?"""
-        dict_for_orm = self.dict()
-        del dict_for_orm["subscribed"]
-        return dict_for_orm
+WaitlistInSchema = WaitlistBase
 
 
 class UpdatedWaitlistInSchema(WaitlistInSchema):

docs/adrs/waitlists_subscribed.md

@@ -0,0 +1,153 @@
+# Waitlists `subscribed` field
+
+* Status: accepted
+* Deciders: <CTMS stakeholders>
+* Date: June 29, 2023
+
+## Context and Problem Statement
+
+Since #492, onboarding new waitlists does not require any code change, redeploy, etc.
+
+The waitlists data sent by Basket is stored without any intervention on the CMTS side.
+
+For the Basket service, waitlists are a specific kind of newsletter. For this reason, in order to remove a user from a waitlist, Basket will mimic what is done for newsletters and send the field `"subscribed": false` and an optional `unsub_reason` text field.
+
+We currently don't store these incoming fields in the database, and we just use the flag to delete existing waitlist records when set to `false`.
+
+For the synchronization of waitlists to Acoustic (#561), we have to implement this waitlist unsubscription situation.
+
+
+## Decision Drivers
+
+In order to choose our solution we considered the following criteria:
+
+- **Complexity**: Low → High: how complex is the solution
+- **Cost**: Low → High: Low → High: how much engineering efforts is required
+- **Robustness**: Low → High: how reliable is the solution
+- **Adaptability**: Low → High: capacity to adjust and conform to the business requirements
+
+
+## Considered Options
+
+1. [Option 1 - Add subscribed field](#option-1---add-subscribed-field)
+2. [Option 2 - Reset before add](#option-2---reset-before-add)
+3. [Option 3 - Fetch and compare](#option-3--fetch-and-compare)
+4. [Option 4 - Implement deletion queue](#option-4---implement-deletion-queue)
+
+## Decision Outcome
+
+Chosen option: *Option 1* because it is most pragmatic decision. And it has the best ratio complexity/robustness. Although it does not match entirely the waiting list concept, it will be understood by stakeholders since it follows Basket. It is indeed a missed opportunity to revamp the synchronization process using a queue, but it should be reasonably easy to migrate both newsletters and waitlists together when the time comes.
+
+## Pros and Cons of the Options
+
+### Option 1 - Add subscribed field
+
+This solution consists in adding the `subscribed : boolean` field to the waitlist table in the database, and turn it to `false` when Basket does. The `unsub_reason` text field holds an optional text.
+
+This is also known as *soft deletion*.
+
+**Complexity**: Low
+
+This mimics what currently exists for newsletters.
+
+**Cost**: Mid
+
+The change was implemented in CTMS in a matter of hours in pull-request #707.
+
+The cost is *Mid* and not *Low*, because it requires a tiny change in Basket to be implemented and deployed (filter the waitlist objects with `subscribed=true` in CTMS responses), and thus requires some coordination efforts.
+
+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.
+
+**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.
+If we want to store the `subscribed` column in Acoustic, like we plan to do for newsletters in #562, we just upsert all entries.
+
+**Adaptability**: Low
+
+We introduce this notion as a consequence of Basket modelling, and it may skew the concepts manipulated by stakeholders when querying Acoustic.
+
+Conceptually, the notion of unsubscription for a waitlist isn't really adequate. Users join and may leave waiting lists. They don't really "subscribe" to a waitlist.
+Although members of the waitlist may receive announcements of product availability or be invited to join a newsletter, waitlists are not implicitly turned into newsletters from which users have to unsubscribe.
+
+### Option 2 - Reset before add
+
+Since we don't delete waitlists when we receive unsubscription, we loose track of their previous state in CTMS, and can't determine which have to be deleted in Acoustic. In order to keep both sides in sync, we could delete all records in Acoustic, and re-add all records present in CTMS.
+
+**Complexity**: Low
+
+The Acoustic has endpoints to delete relational tables by key, and the resulting code should not be too complex.
+
+**Cost**: Mid
+
+Would require to use Acoustic API endpoints that are not currently used in CTMS.
+
+**Robustness**: Low
+
+This approach is fragile, because it would require at least two API calls.
+It would double the amount of requests sent to Acoustic, and will slow down synchronization.
+And it won't be transactional: if the deletion step works, but the addition part can never go through, we loose all records on the Acoustic side.
+
+**Adaptability**: Mid
+
+Acoustic just lists the users's waitlists. This matches reality and stakeholders notions.
+
+With this solution, the text with the unsubscribe reason isn't stored in CTMS nor Acoustic (although the form on the Basket would show it).
+
+### Option 3 - Fetch and compare
+
+Same as *Option 2*, except that we would fetch what is store on Acoustic to determine which entries have to be deleted.
+
+**Complexity**: Low
+
+More or less equivalent as *Option 2*, adding a trivial comparison step.
+
+**Cost**: Mid
+
+Same as *Option 2*, it would require to use Acoustic API endpoints that are not currently used in CTMS.
+
+**Robustness**: Mid
+
+This approach is robust in terms of data integrity. We are not exposed to TOCTOU issues since the data synchronization occurs from a single process. And other entities manipulating Acoustic are not likely to modify waitlists entries.
+
+It would double the amount of requests sent to Acoustic, and will slow down synchronization. Which makes the system less robust. Hence *Mid* instead of *High* on this criteria.
+
+**Adaptability**: Mid
+
+Acoustic just lists the users's waitlists. This matches reality and stakeholders notions.
+
+With this solution, the text with the unsubscribe reason isn't stored in CTMS nor Acoustic (although the form on the Basket would show it).
+
+### Option 4 - Implement deletion queue
+
+For this solution, we implement a deletion queue, as described in #571.
+
+When Basket sends an unsubscription to CTMS, we delete the record from the CTMS database, and store an entry in a synchronization queue. Like for example:
+
+```
+operation: delete
+tablename: waitlist
+key: {"email_id": "F9601A02-09C0-4C38-9C61-DC8F9F3CB79F", "name": "vpn"}
+```
+
+During the synchronization process, we process this queue, and remove entries from it, only if the deletion was successful on the Acoustic side.
+
+Note: If the text with the unsubscribe reason would have to be stored in CTMS and Acoustic, we would combine this with *Option 1*, and store an `update` operation in the synchronization queue.
+
+**Complexity**: Mid
+
+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.
+
+**Cost**: High
+
+This is a whole project in itself.
+
+**Robustness**: High
+
+The additions and removals from the queue are transactional with the reception and emission of Basket and Acoustic requests. The operations in the queue can be retried until success.
+
+**Adaptability**: High
+
+Acoustic just lists the users's waitlists. This matches reality and stakeholders notions.
+
+Combining with soft-deletion and storing the unsubscribe reason is possible if necessary, and does not fundamentally change the design.

migrations/versions/20230619_2a0489304e03_add_subscribed_unsub_reason_to_waitlist.py

@@ -0,0 +1,37 @@
+"""Add subscribed/unsub_reason to Waitlist
+
+Revision ID: 2a0489304e03
+Revises: a016433d3e8b
+Create Date: 2023-06-19 13:40:43.345044
+
+"""
+# 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 = "2a0489304e03"  # pragma: allowlist secret
+down_revision = "a016433d3e8b"  # pragma: allowlist secret
+branch_labels = None
+depends_on = None
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.add_column("waitlists", sa.Column("subscribed", sa.Boolean(), nullable=True))
+    op.add_column("waitlists", sa.Column("unsub_reason", sa.Text(), nullable=True))
+    # ### end Alembic commands ###
+
+    # Existing recoards are all subscriptions
+    op.execute("UPDATE waitlists SET subscribed = true")
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.drop_column("waitlists", "unsub_reason")
+    op.drop_column("waitlists", "subscribed")
+    # ### end Alembic commands ###

tests/factories/models.py

@@ -53,6 +53,7 @@ class Meta:
     name = factory.Sequence(lambda n: f"waitlist-{n}")
     source = factory.Faker("url")
     fields = {}
+    subscribed = True
 
     email = factory.SubFactory(factory="tests.factories.models.EmailFactory")