-
Notifications
You must be signed in to change notification settings - Fork 908
Sync email to Stripe when user changes email address #2432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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