Subscription form #624

vaninawohlleb · 2017-08-23T12:49:43Z

create a new create_subscritption handler
remove submit-subscription handler?
implements more specific error handling
removes the check for POSt request (hm maybe we should revert this change actually)

This should be tested along with this PR (https://github.com/LeastAuthority/leastauthority.com-website/pull/56). For testing instructions check the above PR

vaninawohlleb · 2017-08-23T12:52:59Z

src/lae_site/handlers/create_subscription.py

+
+class CreateSubscription(HandlerBase):
+  def addHeaders(self, request):
+    request.setHeader('Access-Control-Allow-Origin', 'http://localhost:5000')


this shouldnt be hardcoded as we will need different domains for different environments. @exarkun what would be your suggestion for most secure way of adding this to the server vars?

What is this address meant to point at? Is it the main leastauthority website (where the signup form is hosted)? What are the consequences of this header?

As far as getting configuration into the server, that will probably start by adding something to k8s/secrets.{staging,production}.enc.yaml, updating k8s/infrastructure.yaml to extract that new secret item and pass it as an argument to lae_site.main, and then updating lae_site.main to take the new parameter and pass it onwards to the code that needs it (CreateSubscription). Possibly the first part is done already: we already have a domain secret item, we just haven't needed it in lae_site before.

this address is the client lektor site, and the header allows cross origin requests. So it will be different in staging and production environments

exarkun

Thanks! Various comments about the code inline. Comments about the user-facing experience on the other PR.

exarkun · 2017-08-24T12:15:14Z

src/lae_site/handlers/create_subscription.py

+
+class CreateSubscription(HandlerBase):
+  def addHeaders(self, request):
+    request.setHeader('Access-Control-Allow-Origin', 'http://localhost:5000')


What is this address meant to point at? Is it the main leastauthority website (where the signup form is hosted)? What are the consequences of this header?

As far as getting configuration into the server, that will probably start by adding something to k8s/secrets.{staging,production}.enc.yaml, updating k8s/infrastructure.yaml to extract that new secret item and pass it as an argument to lae_site.main, and then updating lae_site.main to take the new parameter and pass it onwards to the code that needs it (CreateSubscription). Possibly the first part is done already: we already have a domain secret item, we just haven't needed it in lae_site before.

exarkun · 2017-08-24T12:19:20Z

src/lae_site/handlers/create_subscription.py

+        )
+
+class CreateSubscription(HandlerBase):
+  def addHeaders(self, request):


Please use 4 space indents everywhere. Though Python gives you freedom to choose, 4 space indents is the only correct choice. :)

exarkun · 2017-08-24T12:22:50Z

src/lae_site/main.py

@@ -195,8 +195,6 @@ def main(reactor, *argv):
    d.addCallback(lambda ignored: Deferred())
    return d

-
-
 def site_for_options(reactor, options):
    provisioner = get_provisioner(
        reactor,


Just whitespace changes in this file, I think. Please revert.

exarkun · 2017-08-24T12:36:05Z

src/lae_site/handlers/__init__.py

@@ -8,8 +8,10 @@
 from twisted.python.filepath import FilePath

 from lae_site.handlers.web import JinjaHandler
+# TODO: Rename all handlers teh same way for consistency
 from lae_site.handlers.submit_subscription import SubmitSubscriptionHandler


This import could be removed now. SubmitSubscriptionHandler is no longer used. Perhaps all of submit_subscription.py can be removed, in fact. It looks like everything it was doing is now done in create_subscription.py. There are a couple other areas of code that try to import from submit_subscription - eg lae_site/main.py imports Stripe and Mailer from it but there are copies of those classes defined in create_subscription.py now so those imports could be switched.

The tests for submit_subscription.py can perhaps also be adapted to create_subscription.py. See lae_site/handlers/test/test_submit_subscription.py.

exarkun · 2017-08-24T12:41:30Z

src/lae_site/handlers/create_subscription.py

+@attr.s
+class Stripe(object):
+    key = attr.ib()
+    # email = "vtsoneva@gmail.com"


Some more dev/debug cruft to remove

exarkun · 2017-08-24T12:42:20Z

src/lae_site/handlers/create_subscription.py

@@ -0,0 +1,169 @@
+from twisted.web.resource import Resource


This import appears unused.

exarkun · 2017-08-24T12:50:39Z

src/lae_site/handlers/create_subscription.py

+            self.handle_stripe_create_customer_errors(traceback.format_exc(100), e,
+              details=e.message.encode("utf-8"),
+              email_subject="Stripe Card error",
+              notes=note)


I liked the old formatting better. 😁

exarkun · 2017-08-24T12:53:33Z

src/lae_site/handlers/create_subscription.py

+            self.handle_stripe_create_customer_errors(traceback.format_exc(100), e,
+              details="Something went wrong. Please try again, or contact"+
+                "support@leastauthority.com.",
+              email_subject="Stripe unexpected error")


It looks like some changes from master have been lost. There was a recent change to make these email_subject parameters all include the email address the user had entered. Those changes are present in submit_subscription.py but they aren't included here, I guess because you started from the version before they were introduced.

It seems that create_subscription.py and submit_subscription.py are substantially the same. In the future it would be easier (because of better tool support) to just edit in-place. For example, in this case, that would have let git merge the master changes in with yours (or at least show you conflict markers around areas that need your merge attention). This is also an argument in favor of leaving formatting alone (except in extreme circumstances) since formatting changes often result in merge conflicts that have to be resolved (tedious) for no significant benefit.

Thinking about it a little more, the easiest thing to do at this point might be to read through the git patch log for create_subscription.py and apply the patches (perhaps manually 😢) onto submit_subscription.py. Then git rm create_subscription.pyfrom the branch (and perhapsgit mv submit_subscription.py create_subscription.py`).

Let me know if I can help with any of the git operations.

exarkun · 2017-08-24T13:07:06Z

src/lae_site/templates/payment_verified.html


+{% extends '_base.html' %}
 {% block content %}

 <h3>Sign-Up Complete!</h3>


Just whitespace changes?

exarkun · 2017-08-24T13:52:47Z

src/lae_site/handlers/create_subscription.py

+    request.write(claim.describe(env).encode('utf-8'))
+    request.finish()
+
+def signup_failed(request, reason, customer, mailer):


The signature here doesn't match the caller. Line 149 doesn't pass the request in (and reason will be the first argument anyway). But it doesn't look like request is actually used - though perhaps it ought to be.

exarkun · 2017-08-28T15:01:31Z

k8s/infrastructure.yaml

@@ -534,6 +535,8 @@ spec:
          - 's4-subscription-manager'
          - '--domain'
          - '$(S4_DOMAIN)'
+          - '--cross-domain'
+          - '$(CROSS_DOMAIN)'


fwiw the only reason the parameter and its value are split into separate strings here is that I was just learning about how kubernetes env var interpolation works when I wrote this section. I wasn't sure it was allowed to interpolate into a string like --cross-domain=$(CROSS_DOMAIN) but as you can see above that style does work and I've used it elsewhere.

Doesn't really make any difference, just explaining why there are two different styles in this file.

…in for staging. localhost just for test puproses

…eastauthority.com into subscription_form_edits

it matters and javascript hates it

exarkun

Great, thank you. A number of very minor comments left inline. When they are addressed I think this should be ready for merge.

exarkun · 2017-09-07T16:38:31Z

src/lae_site/handlers/create_subscription.py

+      'support@leastauthority.com',
+      "A sign-up failed for <%s>." % (customer.email,),
+      headers,
+  )


Four space indentation for all Python code, please. :) Trailing newline at the end of the file is nice too.

exarkun · 2017-09-07T16:38:50Z

.gitignore

+# env file
+.env
+
+.DS_Store


Thanks for adding this ignore. Can you also delete the .DS_Store files that were added?

exarkun · 2017-09-07T16:40:11Z

.gitignore

@@ -26,3 +26,8 @@ telepresence.log

 # distutils gunk
 leastauthority.com.egg-info/
+
+# env file
+.env


Is it expected that folks will have a .env file around? Is this where env-cmd takes its values from? Or is there another tool that's reading this file to initialize environment variables for local/development deployments of the backend?

exarkun · 2017-09-07T16:41:08Z

k8s/secrets.staging.enc.yaml

@@ -1,90 +1,91 @@
-apiVersion: ENC[AES256_GCM,data:17o=,iv:DqkDV/AR/NcKNDVPHKj4lyGxIrvcR5p3zYKlHPZtZRg=,tag:d/SllcsYkK7NegeTph3hKg==,type:str]


I think we'll have to re-edit this file starting from current master. This file got changed in master meanwhile and the merge story for sops isn't very good (actually, there isn't really one). And I'll also have to make similar edits to the production secrets file to add the correct cross domain for production deployment.

I merged master onto this branch, but this file still has different data.

exarkun · 2017-09-07T16:41:23Z

src/lae_site/handlers/__init__.py

@@ -8,14 +8,15 @@
 from twisted.python.filepath import FilePath

 from lae_site.handlers.web import JinjaHandler
-from lae_site.handlers.submit_subscription import SubmitSubscriptionHandler
+# TODO: Rename all handlers teh same way for consistency


this TODO is actually done, so will remove the comment

exarkun · 2017-09-07T16:43:09Z

src/lae_site/handlers/create_subscription.py

        try:
            return self._stripe.create(
                authorization_token=stripe_authorization_token,
                plan_id=PLAN_ID,
                email=user_email,
            )
        except stripe.CardError as e:
+            # Always return 402 on card errors
+            request.setResponseCode(402)


You can get a symbolic constant for 402 from twisted.web.http: PAYMENT_REQUIRED

oh cool stuff!

exarkun · 2017-09-07T16:43:31Z

src/lae_site/handlers/create_subscription.py

+              traceback.format_exc(100), e,
+              details=e.message.encode("utf-8"),
+              email_subject="Stripe Card error ({})".format(user_email),
+              notes=note,


Four space indents here and in the other exception handling blocks below, too.

exarkun · 2017-09-07T16:44:57Z

src/lae_site/handlers/create_subscription.py

-                email_subject="Stripe API error ({})".format(user_email),
+              traceback.format_exc(100), e,
+              details="Our payment processor is temporarily unavailable,"+
+                  " please try again in\ a few moments.",


You can rely on compile-time literal string concatenation here.

( "foo" "bar" )

is the same as

( "foo" + "bar" )

except that the former is evaluated at compile time and the latter at runtime.

exarkun · 2017-09-07T16:45:33Z

src/lae_site/handlers/create_subscription.py

-                email_subject="Stripe Invalid Request error ({})".format(user_email),
+              traceback.format_exc(100), e,
+              details="Our payment processor is temporarily unavailable,"+
+                  " please try again in\ a few moments.",


trace \ in this string

exarkun · 2017-09-07T16:47:19Z

src/lae_site/handlers/s4_signup_style.py

@@ -23,4 +23,4 @@ def render_GET(self, request):
            S4_SIGNUP_STYLE_COOKIE,
            request.args.get(b"style", [b"email"])[0]
        )
-        return redirectTo(b"/s4-subscription-form", request)
+        return request


Hm. What does this do now?

should return the request instead of redirecting to s4-subscription-form with it

previously, it was an assignment followed by a bare string literal (a noop)

For whatever reason, this is how CORS works.

exarkun · 2017-09-12T14:20:04Z

src/lae_site/handlers/create_subscription.py

        except RenderErrorDetailsForBrowser as e:
-            tmpl = env.get_template('s4-subscription-form.html')
-            return tmpl.render({"errorblock": e.details}).encode('utf-8', 'replace')
+            return e.details

        # Initiate the provisioning service
        subscription = customer.subscriptions.data[0]


As far as I can tell, the cookie-based signup-style selection logic that starts on the next line no longer works. Signup is always via email even if the cookie is set to wormhole. Do you have any idea about why this might be? Something related to CORS?

vaninawohlleb requested a review from exarkun August 23, 2017 12:49

vaninawohlleb commented Aug 23, 2017

View reviewed changes

exarkun suggested changes Aug 24, 2017

View reviewed changes

exarkun assigned vaninawohlleb Aug 24, 2017

vaninawohlleb added 4 commits August 25, 2017 13:26

creates new handler create_subscription

4ce2abb

proper error handling showing also card errors

09a54c1

error handling

89b2024

cleanup

f77def1

vaninawohlleb force-pushed the subscription_form_edits branch from 8e2169a to f77def1 Compare August 25, 2017 12:27

vaninawohlleb added 7 commits August 25, 2017 14:50

addresses indentation and formatting comments

5343fb2

remove xtra argument in signup_failed

7db78e2

removes env file

0297ad5

remove git cache

282687d

spacing fixes

d52490c

adding cross domain var

8004da6

edit in gitignore

73194ab

exarkun reviewed Aug 28, 2017

View reviewed changes

vaninawohlleb and others added 13 commits August 29, 2017 13:14

adds cross-domain from options

764b415

pass cross-domain to createsubscription

5f18c73

chan ges for testing tools. Adds key and encrypt value for cross doma…

bd12891

…in for staging. localhost just for test puproses

clean up white space and xtra comments

73131b2

Change the cross-domain to be suitable for staging.

c481bf7

moves cross domain

1bca9e7

removes submit subscription

689f692

Re-unite eliot destination pieces

48bde16

re-encrypt name: name_unencrypted key breaks k8s

d912560

moves cross-domain to lae_site.main

37b6fa0

Merge branch 'subscription_form_edits' of github.com:LeastAuthority/l…

1b9771b

…eastauthority.com into subscription_form_edits

indentation adjustment

6a47099

drop the trailing slash

e05086e

it matters and javascript hates it

exarkun suggested changes Sep 7, 2017

View reviewed changes

vaninawohlleb and others added 17 commits September 11, 2017 16:40

fixing wrong indentation. using cool twisted stuff

108e5ad

pulled correct indentation for infrastucture file

8449573

deleted unneccessary files

a7c5449

merge master

6b0b9f4

whoops return cross domain onto secrets

fd952fd

edits in k8s/infstracture

dfaef25

Revert to master staging secrets

af38b33

Add cross-domain secret.

35dd5a7

Fix new secretKeyRef stanza

c345acd

Remove OSX artifacts

76c7ad5

symbolic constants for response codes

5fc7ff6

whitespace

ae4bdf5

string formatting

cebfbe9

make this a real multiline string

207ec97

previously, it was an assignment followed by a bare string literal (a noop)

redirect to the new signup resource

6e1121a

Test a negative path for signup.

183d87a

Stripe trailing /s from cross-domain values.

3cd6e11

For whatever reason, this is how CORS works.

exarkun reviewed Sep 12, 2017

View reviewed changes

vaninawohlleb added 2 commits September 13, 2017 15:57

remove wormhole cookie

69dcdb0

whitespace

ba06483

exarkun approved these changes Sep 13, 2017

View reviewed changes

exarkun merged commit d745de6 into master Sep 13, 2017

exarkun deleted the subscription_form_edits branch September 13, 2017 14:02

exarkun mentioned this pull request Sep 13, 2017

Securely transfer the introducer furl to new users #465

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Subscription form #624

Subscription form #624

vaninawohlleb commented Aug 23, 2017 •

edited

vaninawohlleb Aug 23, 2017

exarkun Aug 24, 2017

vaninawohlleb Aug 25, 2017

exarkun left a comment

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 24, 2017

exarkun Aug 28, 2017

exarkun left a comment

exarkun Sep 7, 2017

exarkun Sep 7, 2017

exarkun Sep 7, 2017

exarkun Sep 7, 2017

vaninawohlleb Sep 11, 2017

exarkun Sep 7, 2017

vaninawohlleb Sep 11, 2017

exarkun Sep 7, 2017

vaninawohlleb Sep 11, 2017

exarkun Sep 7, 2017

exarkun Sep 7, 2017

exarkun Sep 7, 2017

exarkun Sep 7, 2017

vaninawohlleb Sep 11, 2017 •

edited

exarkun Sep 12, 2017

		@@ -1,90 +1,91 @@
		apiVersion: ENC[AES256_GCM,data:17o=,iv:DqkDV/AR/NcKNDVPHKj4lyGxIrvcR5p3zYKlHPZtZRg=,tag:d/SllcsYkK7NegeTph3hKg==,type:str]

Subscription form #624

Subscription form #624

Conversation

vaninawohlleb commented Aug 23, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exarkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

exarkun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaninawohlleb Sep 11, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vaninawohlleb commented Aug 23, 2017 •

edited

vaninawohlleb Sep 11, 2017 •

edited