Sync email to Stripe when user changes email address#2432
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 131d46b5e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e6da0a3 to
5e34a60
Compare
jeremy
left a comment
There was a problem hiding this comment.
What about account ownership changes? It's not just the owning user's email changed, it's the the account owner email changed.
When an account owner changes their email address, update the corresponding Stripe customer record via a background job. Also handles ownership transfers: when a user becomes the account owner, their email is synced to Stripe. Responsibility chain: - User::NotifiesAccountOfEmailChange triggers on owner identity change or when a user becomes owner - Account::Billing#owner_email_changed enqueues sync job - Account::SyncStripeCustomerEmailJob performs the update with polynomial backoff retries - Account::Subscription#sync_customer_email_to_stripe calls Stripe API
5e34a60 to
6ccb0d1
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements automatic syncing of the account owner's email to Stripe when the owner changes their email address or when ownership is transferred to a different user. Previously, the Stripe customer email was only set once at subscription creation time and never updated.
Changes:
- Added
User::NotifiesAccountOfEmailChangeconcern that triggers when owner identity changes or a user becomes owner - Added
Account::Billing#owner_email_changedmethod to enqueue sync jobs - Added
Account::Subscription#sync_customer_email_to_stripeto update Stripe customer records - Added
Account::SyncStripeCustomerEmailJobto handle async Stripe updates - Added comprehensive test coverage for the notification flow and sync behavior
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| saas/app/models/user/notifies_account_of_email_change.rb | New concern that detects when owner email changes or ownership is transferred and notifies the account |
| saas/lib/fizzy/saas/engine.rb | Includes the new concern in the User model |
| saas/app/models/account/billing.rb | Adds method to enqueue sync job when owner email changes |
| saas/app/models/account/subscription.rb | Implements the Stripe customer email sync logic |
| saas/app/jobs/account/sync_stripe_customer_email_job.rb | Background job that calls the sync method with retry logic for Stripe errors |
| saas/test/models/user/notifies_account_of_email_change_test.rb | Tests for the user concern covering various scenarios |
| saas/test/models/account/billing_test.rb | Tests for job enqueueing behavior |
| saas/test/models/account/subscription_test.rb | Tests for the sync method covering edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Re: ownership changes - addressed. The concern now handles both cases: def account_owner_changed?
owner? && identity && (saved_change_to_identity_id? || became_owner?)
end
def became_owner?
saved_change_to_role? && role_before_last_save != "owner"
endTest coverage:
|
- Handle Stripe::InvalidRequestError in sync_customer_email_to_stripe (mirrors cancel method behavior for deleted customers) - Add test for deactivated owner (owner with nil identity) - Add test for deleted Stripe customer scenario
dd45880 to
6e8f44a
Compare
* main: (127 commits) Align board name start so it doesn't get too big Clean up card/events header layout Sync email to Stripe when user changes email address (#2432) Fix IDOR in webhook activation endpoint (#2431) Add card reactions to API docs and reactions_url to card JSON (#2427) Remove unnecessary claude plan Allow boosts on cards (#2411) Revert "Fix notification click URL by using correct data property" Add migration to remove draft cards from search index Guard search indexing with searchable? check Forbid comments on draft cards prefactor: update search to use published cards Fix notification click URL by using correct data property Wait for service worker to be active before subscribing Fix stuck state when permission granted but no subscription Extract Card::Commentable Include arm64 build in Docker workflow Remove unnecessary `await` in push handler Correctly initialise WebPush connection (#2417) Update models, views, and fixtures for polymorphic reactions ...
* main: (26 commits) Add a new Pins section to docs/API.md covering pin/unpin and the pinned cards list response. Align board name start so it doesn't get too big Clean up card/events header layout Sync email to Stripe when user changes email address (#2432) Fix IDOR in webhook activation endpoint (#2431) Add card reactions to API docs and reactions_url to card JSON (#2427) Remove unnecessary claude plan Allow boosts on cards (#2411) Revert "Fix notification click URL by using correct data property" Add migration to remove draft cards from search index Guard search indexing with searchable? check Forbid comments on draft cards prefactor: update search to use published cards Fix notification click URL by using correct data property Wait for service worker to be active before subscribing Fix stuck state when permission granted but no subscription Extract Card::Commentable Include arm64 build in Docker workflow Remove unnecessary `await` in push handler Correctly initialise WebPush connection (#2417) ...
Summary
When the account owner's email changes, the Stripe customer record now gets updated automatically. This handles two scenarios:
Root cause:
create_stripe_customerset email once at subscription creation time with no subsequent syncing.Fix: Layered responsibility across User → Account → Subscription:
User::NotifiesAccountOfEmailChange- Triggers on owner email or role changeAccount::Billing#owner_email_changed- Enqueues background jobAccount::Subscription#sync_customer_email_to_stripe- Updates Stripe customerAccount::SyncStripeCustomerEmailJob- Async with polynomial backoff retryChanges
saas/app/models/user/notifies_account_of_email_change.rb- User concernsaas/app/models/account/billing.rb- Account method to enqueue jobsaas/app/models/account/subscription.rb- Stripe sync methodsaas/app/jobs/account/sync_stripe_customer_email_job.rb- Background jobsaas/lib/fizzy/saas/engine.rb- Include concern in Usersaas/test/models/user/notifies_account_of_email_change_test.rb- User testssaas/test/models/account/billing_test.rb- Job enqueueing testssaas/test/models/account/subscription_test.rb- Sync method testsTest plan
bin/rails test saas/test/- All saas tests passFixes BUG-9504334396
Source: HackerOne #3522917