Handmade Software Foundation Memberships#39
Conversation
…rite tests to ensure no conflicts Co-authored-by: Cursor <cursoragent@cursor.com>
…race period logic)
…verification banner
…using bot, including ability to manually sync
… Stripe backend bugfixes
bvisness
left a comment
bvisness
left a comment
There was a problem hiding this comment.
Looking pretty good overall. I have to go to bed but will resume review tomorrow.
| } | ||
|
|
||
| func (m AddDetailedSubscriptionInfo) Name() string { | ||
| return "AddDetailedSubscripitionInfo" |
There was a problem hiding this comment.
I would very much like to know how a typo snuck in here but not into the type name :P
(it does not matter one bit, but it is funny)
|
|
||
| func (m AddDetailedSubscriptionInfo) Up(ctx context.Context, tx pgx.Tx) error { | ||
| _, err := tx.Exec(ctx, ` | ||
| ALTER TABLE hmn_user |
There was a problem hiding this comment.
It's a bit odd to have so many migrations that all modify the user table. Migration files kind of serve double duty as modifying the schema and serving as reference for the database schema without having to pull up the Postgres CLI and run \d.
Basically what I am saying is that it would be convenient to roll all these migrations together into one big migration, so that you can see all the fields at once. (This also would have the advantage of reordering the columns.)
|
|
||
| CREATE TABLE user_payment ( | ||
| id SERIAL PRIMARY KEY, | ||
| user_id INTEGER NOT NULL REFERENCES hmn_user(id) ON DELETE CASCADE, |
There was a problem hiding this comment.
I'd probably prefer ON DELETE RESTRICT here, so that you can't delete the user record until you've cleaned up the membership. That way we would be reminded to go figure things out in the Stripe dashboard and make sure we've cleaned up any subscriptions.
| func (m AddThankYouEmailSent) Up(ctx context.Context, tx pgx.Tx) error { | ||
| _, err := tx.Exec(ctx, ` | ||
| ALTER TABLE hmn_user | ||
| ADD COLUMN thank_you_email_sent BOOLEAN NOT NULL DEFAULT false; |
| _, err := tx.Exec(ctx, | ||
| ` | ||
| ALTER TABLE hmn_user | ||
| ADD COLUMN is_subscribed BOOLEAN NOT NULL DEFAULT false; |
There was a problem hiding this comment.
Too generic a name imo; could easily apply to newletters and things. Let's make sure membership appears in all these new column names, e.g. has_membership, membership_stripe_customer_id, etc.
| if _, ok := seen[id]; ok { | ||
| return | ||
| } | ||
| seen[id] = struct{}{} |
There was a problem hiding this comment.
Is this trying to deduplicate price IDs? Why? It's in config; we will literally never put in duplicates.
| membership := map[string]struct{}{} | ||
| for _, id := range membershipWebhookPriceIDs() { | ||
| membership[id] = struct{}{} | ||
| } |
There was a problem hiding this comment.
And now building another hash map out of the slice of price IDs we just constructed while constructing a hash map of price IDs to deduplicate them! This is all very silly and totally unnecessary. membershipWebhookPriceIDs should just return a slice of all price ids from config, assuming no duplicates, and then you should just loop through that slice here, no hash map. It will almost certainly be faster than allocating a hash map and checking the hash map for a string.
Better yet, why is the value in config.go not itself just a slice of price IDs? I'm not sure there's a good reason to have one "primary" one and a list of "alternates", when you could just have a list of all price IDs, and we default to the first one in the list. Then you wouldn't need membershipWebhookPriceIDs at all; you would just loop over the list from config.
| return stripeWebhookKindUnknown, oops.New(err, "failed to load ticket price ids") | ||
| } | ||
|
|
||
| known := make(map[string]struct{}, len(ticketPriceIDs)) |
There was a problem hiding this comment.
Again, the hash map is superfluous. Consider what you have to do to build the hash map and then look up an item in it:
- Allocate the backing storage.
- Loop through
ticketPriceIDs, hashing each string (which means iterating every byte in the string) and then inserting into the backing storage (resolving collisions as necessary). - Loop through
priceIDs, and for each price ID, hash the string (iterating every byte) and look it up in the hash map:- If the hash matched an item, do a full
priceID == knownIDstring comparison because hashes can collide. If they match, returnstripeWebhookKindTicket, otherwise, move onto the next item in the hash map and run this step again, or continue to the next price ID because there was no match.
- If the hash matched an item, do a full
- (later) Garbage collect the backing storage.
Whereas if you you simply do a double for loop over ticketPriceIDs and priceIDs, you will just do m * n string comparisons and zero memory allocations, and that's that. But len(ticketPriceIDs) is currently 1, and in a few years it might be as high as 5, and priceIDs comes from the webhook event and I can't even imagine what would cause it to be greater than 1. At most I figure it could be 2, because that's the number of prices in our config and on the subscription.
I know it seems like the "right" thing to do is to avoid the double for loop, because n^2 algorithms are bad etc. But we are currently looking at a maximum of two string comparisons, and in the future maybe it would be like 10, and probably in many cases it's still less than that. This is nothing. The right thing to do is the stupid thing, which will nonetheless complete in much less than a millisecond and totally avoid allocating garbage that could impact the rest of the application.
|
I'm also about to head to bed, hoping to tackle some more of those prickly database structure and migration questions in the morning. I appreciate you going so in-depth into reviewing everything @bvisness. I think at some points I've been overly defensive in the code, creating checks for non-existent, (or very esoteric), cases and such. Most of the time this is just based on the mindset of, "oh well why not just check", instead of thinking of how something like a blank customer ID might actually occur. All of that's to say, I appreciate the very real lesson in engineering pragmaticism |
2 participants
Implementation of a new system that allows users to initiate and manage a membership with the Handmade software foundation, managing recurring payments, notification of users, logging, benefits and more. Fundamentally, the system is comprised of series of Stripe driven state machines, directing users to hosted payment pages, handling webhooks returned by Stripe, notifying users as defined by the state via email, and applying benefits as appropriate.
Though the core payment flow is simple at heart, (only requiring handling a few webhooks and minimal UI changes), complexity grows exponentially upon the integration of asynchronous payment methods such as ACH; rich email notifications; change of payment method; grace periods; cancellation; and bank account verification.
Furthermore, the asynchronous and, by it's nature, disorderly state of Stripe's webhooks results in much of the complexity found in this PR, requiring idempotency guards, event ledgers, and nuanced handlers to ensure that incoming data from Stripe is handled in the most Handmade way possible, mitigating the risk of double-triggered and disordered webhooks.
Webhooks needed to be dispatched between handlers for ticket sales and memberships, as there was significant overlap in webhook types which needed processing between the two systems. This requited the development of a combined router/handler which "dispatches" out events to either ticket or membership handlers based on priceID.
The system supports multiple currencies, (at this moment, EUR and USD); future support for additional tiers; management of Discord roles as a benefit of membership, (with the ability to manually sync roles); a customizable "grace period" for highly asynchronous payment methods like ACH; and the logging of user membership info for future analytics.
Additionally, the system includes a rich testing and debugging suite, (
admin membership), for easy analysis of the complex state machines which make up this system. The included tests are fully end to end, simulating actual Stripe webhooks as user actions, creating test customers, payment methods, and checkout sessions, then ensuring the correct database state. Further, these tests include the Mailpit test SMTP server to verify the correct notification emails are being sent at the appropriate point in a given scenario.This PR contains a complex system which spans many parts of
hmn. Due to the project requirements, the features needed, and the complex and asynchronous nature of real-world payment handling, the system footprint is larger than expected. Despite this, you will find these edits to be made with good reason, to address the many possible edge cases that may be found when interacting with numerous payment types, currencies, and configurations.The system has been broken up into components wherever possible to improve the "Handmade-like" serviceability and readability, and conventions have been followed from elsewhere in the project.