-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Fix: Auto-apply VAT ID to recurring subscription charges #2270
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
base: main
Are you sure you want to change the base?
Conversation
| def update_business_vat_id!(vat_id) | ||
| update!(business_vat_id: vat_id) if vat_id.present? && business_vat_id.blank? | ||
| end | ||
|
|
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.
Stores VAT ID on subscription only if not already set. This prevents overwriting if customer provides a different VAT ID later.
app/models/subscription.rb
Outdated
| def get_vat_id_from_original_purchase(purchase) | ||
| if original_purchase.purchase_sales_tax_info&.business_vat_id | ||
| purchase.business_vat_id = original_purchase.purchase_sales_tax_info.business_vat_id | ||
| elsif original_purchase.refunds.where("gumroad_tax_cents > 0").where("amount_cents = 0").exists? | ||
| purchase.business_vat_id = original_purchase.refunds.where("gumroad_tax_cents > 0").where("amount_cents = 0").first.business_vat_id | ||
| end | ||
| vat_id = business_vat_id.presence || | ||
| original_purchase.purchase_sales_tax_info&.business_vat_id.presence || | ||
| vat_id_from_original_purchase_refund || | ||
| vat_id_from_any_subscription_purchase_refund | ||
|
|
||
| purchase.business_vat_id = vat_id if vat_id.present? | ||
| end |
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.
Priority order for VAT ID lookup:
- Subscription's stored VAT ID (new)
- Original purchase's sales tax info
- Original purchase's VAT refunds
- Any subscription purchase's VAT refunds (new - catches VAT ID added on recurring charges)
| def vat_id_from_original_purchase_refund | ||
| original_purchase.refunds | ||
| .where("gumroad_tax_cents > 0") | ||
| .where("amount_cents = 0") | ||
| .order(created_at: :desc) | ||
| .find { |refund| refund.business_vat_id.present? } | ||
| &.business_vat_id | ||
| end |
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.
Finds VAT ID from VAT-only refunds on ANY subscription purchase, not just the original. Uses Ruby filtering because business_vat_id is stored in json_data column.
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.
One-time service to populate business_vat_id for existing subscriptions that have valid VAT IDs from purchases or refunds. Run after deployment: Onetime::BackfillSubscriptionVatIds.process
| self.purchase_sales_tax_info = purchase_sales_tax_info | ||
| self.purchase_sales_tax_info.save! | ||
|
|
||
| subscription&.update_business_vat_id!(purchase_sales_tax_info.business_vat_id) if purchase_sales_tax_info.business_vat_id.present? |
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.
If VAT ID is validated during checkout, store it on subscription for future recurring charges.
|
@ershad Can you please review this, thanks! |
ershad
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.
@yashranaway _a added a few comments, please take a look. Also discussed in the live stream (December 16).
| def update_business_vat_id!(vat_id) | ||
| update!(business_vat_id: vat_id) if vat_id.present? && business_vat_id.blank? | ||
| end |
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.
@michellelarney should we allow the buyer to use different VAT IDs in the course of a subscription? Or is it like, once they set a certain VAT in a subscription, we shouldn't allow to change that?
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.
Generally I don't think people would need to change it and would usually be using the same VAT ID for all charges. I would say it should just stay the same.
app/models/subscription.rb
Outdated
| vat_id = business_vat_id.presence || | ||
| original_purchase.purchase_sales_tax_info&.business_vat_id.presence || | ||
| vat_id_from_original_purchase_refund || | ||
| vat_id_from_any_subscription_purchase_refund |
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.
Reviewer note: Measure performance impact before merging.
|
Update: Addressed all review feedback:
Pending:
|
|
Hey @michellelarney just a reminder about this comment when you get a chance. thank you cc: @ershad |
|
@ershad Friendly ping on this. |
ershad
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.
@yashranaway reviewed during PR review session, please take a look at the comments.
db/migrate/20251210053000_add_business_vat_id_to_subscriptions.rb
Outdated
Show resolved
Hide resolved
|
Hey @ershad _a , addressed all comments and replied. Could you please take another look? 🙏 |
|
@ershad friendly ping on this, since core changes were accepted in review sessions only tests had fixes remaining which is done could you please take a look? |
|
@nyomanjyotisa I noticed you were recently assigned to this issue. This PR has already been through review, all feedback has been addressed, and tests are passing. Could you please take a look when you have time? |
Issue: #721
Description
Problem
Customers with recurring subscriptions had to manually request VAT refunds for each charge by entering their VAT ID through the invoice page. If they added a VAT ID after the initial purchase, future recurring charges would still have VAT applied.
The existing
get_vat_id_from_original_purchasemethod only checked the original purchase's VAT info and its refunds, not subsequent recurring charges. If a customer added a VAT ID after the initial purchase, it wouldn't propagate to future charges.Solution
When a valid VAT ID is stored (either at checkout or via VAT refund), all future recurring charges automatically skip VAT.
business_vat_idonSubscriptionmodelNote
The ~6,600 added lines are primarily VCR cassettes and test coverage.
Core implementation changes are minimal and tightly scoped.
Before/After
N/A - No UI changes, this is backend logic only.
Test Results
business_vat_idpropagates to recurring chargesupdate_business_vat_id!method works correctly (set when blank, ignore when already set)Checklist
AI Disclosure
I used Claude Opus via Claude Code for specs and tests generation.
All code was manually reviewed and verified by me.