billing: Require successful charge to establish billing relationship.

There are several situations in which we want to create a Customer and
stripe.Customer object before we really have a billing relationship with a
customer. The main one is giving non-profit or educational discounts.

docs/subsystems/billing.md

@@ -22,18 +22,26 @@ Notes:
 * We generally try to store billing-related data in Stripe, rather than in
   Zulip database tables. We'd rather pay the penalty of making extra stripe
   API requests than deal with keeping two sources of data in sync.
+* A realm should have a customer object in Stripe if and only if it has a
+  `Customer` object in Zulip.
 
-The two main billing-related states for a realm are "have never had a
-billing relationship with Zulip" and its opposite. This is determined by
-`Customer.objects.filter(realm=realm).exists()`.  If a realm doesn't have a
-billing relationship, all the messaging, screens, etc. are geared towards
-making it easy to upgrade. If a realm does have a billing relationship, all
-the screens are geared toward making it easy to access current and
-historical billing information.
+The two main billing-related states for a realm are "have never successfully
+been charged for anything" and its opposite. This is determined by whether
+the `realm` has a corresponding `Customer` object with
+`has_billing_relationship=True`. There are only a few cases where a `realm`
+might have a `Customer` object with `has_billing_relationship=False`:
+* They are approved as a non-profit or otherwise have a partial discount,
+  but haven't entered any payment info.
+* They entered valid payment info, but the initial charge failed (rare but
+  possible).
 
-Note that having a billing relationship doesn't necessarily mean they are on
-a paid plan, or have been in the past. E.g. adding a coupon for a potential
-customer requires creating a Customer object.
+If a realm doesn't have a billing relationship, all the messaging, screens,
+etc. are geared towards making it easy to upgrade. If a realm does have a
+billing relationship, all the screens are geared toward making it easy to
+access current and historical billing information.
+
+Note that having a billing relationship doesn't necessarily mean they are
+currently on a paid plan, or that they currently have a card on file.
 
 Notes:
 * When manually testing, I find I often run `Customer.objects.all().delete()`
@@ -41,6 +49,7 @@ Notes:
 * 4242424242424242 is Stripe's test credit card, also useful for manually
   testing. You can put anything in the address fields, any future expiry
   date, and anything for the CVV code.
+  <https://stripe.com/docs/testing#cards-responses> has some other fun ones.
 
 ## BillingProcessor
 

stubs/stripe/__init__.pyi

@@ -10,6 +10,7 @@ class Customer:
     default_source: Card
     created: int
     id: str
+    source: str
     subscriptions: SubscriptionListObject
 
     @staticmethod
@@ -21,6 +22,10 @@ class Customer:
                source: str) -> Customer:
         ...
 
+    @staticmethod
+    def save(idempotency_key: str) -> Customer:
+        ...
+
 class Invoice:
     amount_due: int
 

zilencer/lib/stripe.py

@@ -8,6 +8,7 @@ import ujson
 from django.conf import settings
 from django.db import transaction
 from django.utils.translation import ugettext as _
+from django.utils.timezone import now as timezone_now
 from django.core.signing import Signer
 import stripe
 
@@ -156,6 +157,21 @@ def do_create_customer_with_payment_source(user: UserProfile, stripe_token: str)
         billing_user=user)
     return stripe_customer
 
+@catch_stripe_errors
+def do_replace_payment_source(user: UserProfile, stripe_token: str) -> stripe.Customer:
+    stripe_customer = stripe_get_customer(Customer.objects.get(realm=user.realm).stripe_customer_id)
+    stripe_customer.source = stripe_token
+    # Deletes existing card: https://stripe.com/docs/api#update_customer-source
+    # This can also have other side effects, e.g. it will try to pay certain past-due
+    # invoices: https://stripe.com/docs/api#update_customer
+    updated_stripe_customer = stripe_customer.save(
+        # stripe_token[:4] = 'tok_', so 8 characters of entropy
+        idempotency_key='do_replace_payment_source:%s' % (stripe_token[:12]))
+    RealmAuditLog.objects.create(
+        realm=user.realm, acting_user=user, event_type=RealmAuditLog.STRIPE_CARD_ADDED,
+        event_time=timezone_now())
+    return updated_stripe_customer
+
 @catch_stripe_errors
 def do_subscribe_customer_to_plan(stripe_customer: stripe.Customer, stripe_plan_id: str,
                                   seat_count: int, tax_percent: float) -> None:
@@ -165,6 +181,7 @@ def do_subscribe_customer_to_plan(stripe_customer: stripe.Customer, stripe_plan_
         billing_logger.error("Stripe customer %s trying to subscribe to %s, "
                              "but has an active subscription" % (stripe_customer.id, stripe_plan_id))
         raise BillingError('subscribing with existing subscription', BillingError.TRY_RELOADING)
+    # Success implies the customer was charged: https://stripe.com/docs/billing/lifecycle#active
     stripe_subscription = stripe.Subscription.create(
         customer=stripe_customer.id,
         billing='charge_automatically',
@@ -178,6 +195,8 @@ def do_subscribe_customer_to_plan(stripe_customer: stripe.Customer, stripe_plan_
         print(''.join(['"create_subscription": ', str(stripe_subscription), ',']))  # nocoverage
     customer = Customer.objects.get(stripe_customer_id=stripe_customer.id)
     with transaction.atomic():
+        customer.has_billing_relationship = True
+        customer.save(update_fields=['has_billing_relationship'])
         customer.realm.has_seat_based_plan = True
         customer.realm.save(update_fields=['has_seat_based_plan'])
         RealmAuditLog.objects.create(
@@ -197,7 +216,11 @@ def do_subscribe_customer_to_plan(stripe_customer: stripe.Customer, stripe_plan_
                 extra_data=ujson.dumps({'quantity': current_seat_count}))
 
 def process_initial_upgrade(user: UserProfile, plan: Plan, seat_count: int, stripe_token: str) -> None:
-    stripe_customer = do_create_customer_with_payment_source(user, stripe_token)
+    customer = Customer.objects.filter(realm=user.realm).first()
+    if customer is None:
+        stripe_customer = do_create_customer_with_payment_source(user, stripe_token)
+    else:
+        stripe_customer = do_replace_payment_source(user, stripe_token)
     do_subscribe_customer_to_plan(
         stripe_customer=stripe_customer,
         stripe_plan_id=plan.stripe_plan_id,

zilencer/migrations/0011_customer_has_billing_relationship.py

@@ -0,0 +1,20 @@
+# -*- coding: utf-8 -*-
+# Generated by Django 1.11.14 on 2018-08-14 01:32
+from __future__ import unicode_literals
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('zilencer', '0010_billingprocessor'),
+    ]
+
+    operations = [
+        migrations.AddField(
+            model_name='customer',
+            name='has_billing_relationship',
+            field=models.BooleanField(default=False),
+        ),
+    ]

zilencer/models.py

@@ -39,6 +39,10 @@ class RemotePushDeviceToken(AbstractPushDeviceToken):
 class Customer(models.Model):
     realm = models.OneToOneField(Realm, on_delete=models.CASCADE)  # type: Realm
     stripe_customer_id = models.CharField(max_length=255, unique=True)  # type: str
+    # Becomes True the first time a payment successfully goes through, and never
+    # goes back to being False
+    has_billing_relationship = models.BooleanField(default=False)  # type: bool
+
     billing_user = models.ForeignKey(UserProfile, on_delete=models.SET_NULL, null=True)
 
     def __str__(self) -> str:

zilencer/tests/test_stripe.py

@@ -213,6 +213,63 @@ class StripeTest(ZulipTestCase):
             event_type=RealmAuditLog.STRIPE_PLAN_QUANTITY_RESET).values_list('extra_data', flat=True).first()),
             {'quantity': new_seat_count})
 
+    @mock.patch("stripe.Customer.create", side_effect=mock_create_customer)
+    def test_upgrade_where_subscription_save_fails_at_first(self, create_customer: mock.Mock) -> None:
+        user = self.example_user("hamlet")
+        self.login(user.email)
+        with mock.patch('stripe.Subscription.create',
+                        side_effect=stripe.error.CardError('message', 'param', 'code', json_body={})):
+            self.client_post("/upgrade/", {'stripeToken': self.token,
+                                           'signed_seat_count': self.signed_seat_count,
+                                           'salt': self.salt,
+                                           'plan': Plan.CLOUD_ANNUAL})
+        # Check that we created a customer in stripe
+        create_customer.assert_called()
+        create_customer.reset_mock()
+        # Check that we created a Customer with has_billing_relationship=False
+        self.assertTrue(Customer.objects.filter(
+            stripe_customer_id=self.stripe_customer_id, has_billing_relationship=False).exists())
+        # Check that we correctly populated RealmAuditLog
+        audit_log_entries = list(RealmAuditLog.objects.filter(acting_user=user)
+                                 .values_list('event_type', flat=True).order_by('id'))
+        self.assertEqual(audit_log_entries, [RealmAuditLog.STRIPE_CUSTOMER_CREATED,
+                                             RealmAuditLog.STRIPE_CARD_ADDED])
+        # Check that we did not update Realm
+        realm = get_realm("zulip")
+        self.assertFalse(realm.has_seat_based_plan)
+        # Check that we still get redirected to /upgrade
+        response = self.client_get("/billing/")
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual('/upgrade/', response.url)
+
+        # mock_create_customer just returns a customer with no subscription object
+        with mock.patch("stripe.Subscription.create", side_effect=mock_customer_with_subscription):
+            with mock.patch("stripe.Customer.retrieve", side_effect=mock_create_customer):
+                with mock.patch("stripe.Customer.save", side_effect=mock_create_customer):
+                    self.client_post("/upgrade/", {'stripeToken': self.token,
+                                                   'signed_seat_count': self.signed_seat_count,
+                                                   'salt': self.salt,
+                                                   'plan': Plan.CLOUD_ANNUAL})
+        # Check that we do not create a new customer in stripe
+        create_customer.assert_not_called()
+        # Impossible to create two Customers, but check that we updated has_billing_relationship
+        self.assertTrue(Customer.objects.filter(
+            stripe_customer_id=self.stripe_customer_id, has_billing_relationship=True).exists())
+        # Check that we correctly populated RealmAuditLog
+        audit_log_entries = list(RealmAuditLog.objects.filter(acting_user=user)
+                                 .values_list('event_type', flat=True).order_by('id'))
+        self.assertEqual(audit_log_entries, [RealmAuditLog.STRIPE_CUSTOMER_CREATED,
+                                             RealmAuditLog.STRIPE_CARD_ADDED,
+                                             RealmAuditLog.STRIPE_CARD_ADDED,
+                                             RealmAuditLog.STRIPE_PLAN_CHANGED])
+        # Check that we correctly updated Realm
+        realm = get_realm("zulip")
+        self.assertTrue(realm.has_seat_based_plan)
+        # Check that we can no longer access /upgrade
+        response = self.client_get("/upgrade/")
+        self.assertEqual(response.status_code, 302)
+        self.assertEqual('/billing/', response.url)
+
     def test_upgrade_with_tampered_seat_count(self) -> None:
         self.login(self.example_email("hamlet"))
         response = self.client_post("/upgrade/", {
@@ -247,7 +304,8 @@ class StripeTest(ZulipTestCase):
         self.assertEqual('/upgrade/', response.url)
 
         Customer.objects.create(
-            realm=user.realm, stripe_customer_id=self.stripe_customer_id, billing_user=user)
+            realm=user.realm, stripe_customer_id=self.stripe_customer_id, billing_user=user,
+            has_billing_relationship=True)
         response = self.client_get("/billing/")
         self.assert_not_in_success_response(['We can also bill by invoice'], response)
         for substring in ['Your plan will renew on', '$%s.00' % (80 * self.quantity,),

zilencer/views.py

@@ -184,7 +184,8 @@ def initial_upgrade(request: HttpRequest) -> HttpResponse:
     error_message = ""
     error_description = ""  # only used in tests
 
-    if Customer.objects.filter(realm=user.realm).exists():
+    customer = Customer.objects.filter(realm=user.realm).first()
+    if customer is not None and customer.has_billing_relationship:
         return HttpResponseRedirect(reverse('zilencer.views.billing_home'))
 
     if request.method == 'POST':
@@ -232,6 +233,8 @@ def billing_home(request: HttpRequest) -> HttpResponse:
     customer = Customer.objects.filter(realm=user.realm).first()
     if customer is None:
         return HttpResponseRedirect(reverse('zilencer.views.initial_upgrade'))
+    if not customer.has_billing_relationship:
+        return HttpResponseRedirect(reverse('zilencer.views.initial_upgrade'))
 
     if not user.is_realm_admin and not user == customer.billing_user:
         context = {'admin_access': False}  # type: Dict[str, Any]