From 03818943f678315d4fe49e5a8e12ad75ae407ca4 Mon Sep 17 00:00:00 2001 From: Kush <3647166+kushsharma@users.noreply.github.com> Date: Wed, 24 Jul 2024 13:07:46 +0530 Subject: [PATCH] fix: do not sync deleted test billing customer/subscriptions (#684) If a customer is deleted from billing provider but not in stripe, it was not getting marked deleted earlier, not it will be marked disabled correctly. Similarly if a subscription is deleted from billing provider which was marked as a test resource will also be marked canceled correctly in frontier and sync will no longer happen for these resources. Additionally, 0 amount billing usage reporting is disabled. Signed-off-by: Kush Sharma --- Makefile | 2 +- billing/checkout/service.go | 10 ++-- billing/customer/customer.go | 4 ++ billing/customer/service.go | 89 ++++++++++++++-------------- billing/invoice/service.go | 2 +- billing/subscription/service.go | 29 +++++++-- billing/subscription/subscription.go | 5 ++ proto/apidocs.swagger.yaml | 2 +- proto/v1beta1/models.pb.go | 2 +- proto/v1beta1/models.pb.validate.go | 4 +- 10 files changed, 92 insertions(+), 57 deletions(-) diff --git a/Makefile b/Makefile index 671c2f8af..600694e03 100644 --- a/Makefile +++ b/Makefile @@ -4,7 +4,7 @@ TAG := $(shell git rev-list --tags --max-count=1) VERSION := $(shell git describe --tags ${TAG}) .PHONY: build check fmt lint test test-race vet test-cover-html help install proto ui compose-up-dev .DEFAULT_GOAL := build -PROTON_COMMIT := "1c6e24b3b0fd693d80546370c66a1e5f05e3ab61" +PROTON_COMMIT := "fecbdc10ba9cfb77b27cdca1722d23460eabbf04" ui: @echo " > generating ui build" diff --git a/billing/checkout/service.go b/billing/checkout/service.go index 9e85df47a..102ad4b1b 100644 --- a/billing/checkout/service.go +++ b/billing/checkout/service.go @@ -169,7 +169,7 @@ func (s *Service) backgroundSync(ctx context.Context) { } for _, customer := range customers { - if customer.DeletedAt != nil || customer.IsOffline() { + if !customer.IsActive() || customer.IsOffline() { continue } if err := s.SyncWithProvider(ctx, customer.ID); err != nil { @@ -639,6 +639,7 @@ func (s *Service) ensureSubscription(ctx context.Context, ch Checkout) (string, // create subscription md := metadata.Build(ch.Metadata) md[CheckoutIDMetadataKey] = ch.ID + md[subscription.ProviderTestResource] = !stripeSubscription.Livemode sub, err := s.subscriptionService.Create(ctx, subscription.Subscription{ ID: uuid.New().String(), ProviderID: subProviderID, @@ -875,9 +876,10 @@ func (s *Service) Apply(ctx context.Context, ch Checkout) (*subscription.Subscri CustomerID: billingCustomer.ID, PlanID: plan.ID, Metadata: map[string]any{ - "org_id": billingCustomer.OrgID, - "delegated": "true", - "checkout_id": ch.ID, + "org_id": billingCustomer.OrgID, + "delegated": "true", + "checkout_id": ch.ID, + subscription.ProviderTestResource: !stripeSubscription.Livemode, }, State: string(stripeSubscription.Status), TrialEndsAt: utils.AsTimeFromEpoch(stripeSubscription.TrialEnd), diff --git a/billing/customer/customer.go b/billing/customer/customer.go index d05a5b35b..bd859a13f 100644 --- a/billing/customer/customer.go +++ b/billing/customer/customer.go @@ -56,6 +56,10 @@ func (c Customer) IsOffline() bool { return c.ProviderID == "" } +func (c Customer) IsActive() bool { + return c.State == ActiveState && c.DeletedAt == nil +} + type Address struct { City string `json:"city"` Country string `json:"country"` diff --git a/billing/customer/service.go b/billing/customer/service.go index 3b718f522..066c9f868 100644 --- a/billing/customer/service.go +++ b/billing/customer/service.go @@ -387,54 +387,57 @@ func (s *Service) SyncWithProvider(ctx context.Context, customr Customer) error shouldUpdate = true } } - if stripeCustomer.TaxIDs != nil { - var taxData []Tax - for _, taxID := range stripeCustomer.TaxIDs.Data { - taxData = append(taxData, Tax{ - ID: taxID.Value, - Type: string(taxID.Type), - }) + if customr.IsActive() { + // don't update for disabled state + if stripeCustomer.TaxIDs != nil { + var taxData []Tax + for _, taxID := range stripeCustomer.TaxIDs.Data { + taxData = append(taxData, Tax{ + ID: taxID.Value, + Type: string(taxID.Type), + }) + } + if !slices.EqualFunc(customr.TaxData, taxData, func(a Tax, b Tax) bool { + return a.ID == b.ID && a.Type == b.Type + }) { + customr.TaxData = taxData + shouldUpdate = true + } } - if !slices.EqualFunc(customr.TaxData, taxData, func(a Tax, b Tax) bool { - return a.ID == b.ID && a.Type == b.Type - }) { - customr.TaxData = taxData + if stripeCustomer.Phone != customr.Phone { + customr.Phone = stripeCustomer.Phone shouldUpdate = true } - } - if stripeCustomer.Phone != customr.Phone { - customr.Phone = stripeCustomer.Phone - shouldUpdate = true - } - if stripeCustomer.Email != customr.Email { - customr.Email = stripeCustomer.Email - shouldUpdate = true - } - if stripeCustomer.Name != customr.Name { - customr.Name = stripeCustomer.Name - shouldUpdate = true - } - if stripeCustomer.Currency != "" && string(stripeCustomer.Currency) != customr.Currency { - customr.Currency = string(stripeCustomer.Currency) - shouldUpdate = true - } - if stripeCustomer.Address != nil { - if stripeCustomer.Address.City != customr.Address.City || - stripeCustomer.Address.Country != customr.Address.Country || - stripeCustomer.Address.Line1 != customr.Address.Line1 || - stripeCustomer.Address.Line2 != customr.Address.Line2 || - stripeCustomer.Address.PostalCode != customr.Address.PostalCode || - stripeCustomer.Address.State != customr.Address.State { - customr.Address = Address{ - City: stripeCustomer.Address.City, - Country: stripeCustomer.Address.Country, - Line1: stripeCustomer.Address.Line1, - Line2: stripeCustomer.Address.Line2, - PostalCode: stripeCustomer.Address.PostalCode, - State: stripeCustomer.Address.State, - } + if stripeCustomer.Email != customr.Email { + customr.Email = stripeCustomer.Email + shouldUpdate = true + } + if stripeCustomer.Name != customr.Name { + customr.Name = stripeCustomer.Name + shouldUpdate = true + } + if stripeCustomer.Currency != "" && string(stripeCustomer.Currency) != customr.Currency { + customr.Currency = string(stripeCustomer.Currency) shouldUpdate = true } + if stripeCustomer.Address != nil { + if stripeCustomer.Address.City != customr.Address.City || + stripeCustomer.Address.Country != customr.Address.Country || + stripeCustomer.Address.Line1 != customr.Address.Line1 || + stripeCustomer.Address.Line2 != customr.Address.Line2 || + stripeCustomer.Address.PostalCode != customr.Address.PostalCode || + stripeCustomer.Address.State != customr.Address.State { + customr.Address = Address{ + City: stripeCustomer.Address.City, + Country: stripeCustomer.Address.Country, + Line1: stripeCustomer.Address.Line1, + Line2: stripeCustomer.Address.Line2, + PostalCode: stripeCustomer.Address.PostalCode, + State: stripeCustomer.Address.State, + } + shouldUpdate = true + } + } } if shouldUpdate { if _, err := s.repository.UpdateByID(ctx, customr); err != nil { diff --git a/billing/invoice/service.go b/billing/invoice/service.go index 2ab680b5a..84df67ba9 100644 --- a/billing/invoice/service.go +++ b/billing/invoice/service.go @@ -84,7 +84,7 @@ func (s *Service) backgroundSync(ctx context.Context) { return } for _, customer := range customers { - if customer.DeletedAt != nil || customer.ProviderID == "" { + if !customer.IsActive() || customer.ProviderID == "" { continue } if err := s.SyncWithProvider(ctx, customer); err != nil { diff --git a/billing/subscription/service.go b/billing/subscription/service.go index fc2085afc..c664f5ede 100644 --- a/billing/subscription/service.go +++ b/billing/subscription/service.go @@ -31,8 +31,8 @@ import ( ) const ( - SyncDelay = time.Second * 60 - + SyncDelay = time.Second * 60 + ProviderTestResource = "test_resource" InitiatorIDMetadataKey = "initiated_by" ) @@ -147,7 +147,7 @@ func (s *Service) backgroundSync(ctx context.Context) { } for _, customer := range customers { - if customer.DeletedAt != nil || customer.IsOffline() { + if !customer.IsActive() || customer.IsOffline() { continue } if err := s.SyncWithProvider(ctx, customer); err != nil { @@ -188,9 +188,26 @@ func (s *Service) SyncWithProvider(ctx context.Context, customr customer.Custome var subErrs []error for _, sub := range subs { + if sub.IsCanceled() { + continue + } + stripeSubscription, stripeSchedule, err := s.createOrGetSchedule(ctx, sub) if err != nil { - subErrs = append(subErrs, fmt.Errorf("%s: %w", err, ErrSubscriptionOnProviderNotFound)) + if errors.Is(err, ErrSubscriptionOnProviderNotFound) { + // if it's a test resource, mark it as canceled + if val, ok := sub.Metadata[ProviderTestResource].(bool); ok && val { + sub.State = StateCanceled.String() + sub.CanceledAt = time.Now().UTC() + if _, err := s.repository.UpdateByID(ctx, sub); err != nil { + subErrs = append(subErrs, err) + } + } else { + subErrs = append(subErrs, err) + } + } else { + subErrs = append(subErrs, err) + } continue } @@ -392,6 +409,10 @@ func (s *Service) createOrGetSchedule(ctx context.Context, sub Subscription) (*s }, }) if err != nil { + // check if it's a subscription not found err + if stripeErr, ok := err.(*stripe.Error); ok && stripeErr.Code == stripe.ErrorCodeResourceMissing { + return nil, nil, ErrSubscriptionOnProviderNotFound + } return nil, nil, fmt.Errorf("failed to get subscription from billing provider: %w", err) } diff --git a/billing/subscription/subscription.go b/billing/subscription/subscription.go index 2b601b4e1..6b0222f24 100644 --- a/billing/subscription/subscription.go +++ b/billing/subscription/subscription.go @@ -28,6 +28,7 @@ const ( StateActive State = "active" StateTrialing State = "trialing" StatePastDue State = "past_due" + StateCanceled State = "canceled" ) type PhaseReason string @@ -83,6 +84,10 @@ func (s Subscription) IsActive() bool { return State(s.State) == StateActive || State(s.State) == StateTrialing } +func (s Subscription) IsCanceled() bool { + return State(s.State) == StateCanceled || !s.DeletedAt.IsZero() || !s.CanceledAt.IsZero() +} + type Filter struct { CustomerID string ProviderID string diff --git a/proto/apidocs.swagger.yaml b/proto/apidocs.swagger.yaml index 36686969d..4ef132fc1 100644 --- a/proto/apidocs.swagger.yaml +++ b/proto/apidocs.swagger.yaml @@ -9891,7 +9891,7 @@ definitions: `NullValue` is a singleton enumeration to represent the null value for the `Value` type union. - The JSON representation for `NullValue` is JSON `null`. + The JSON representation for `NullValue` is JSON `null`. - NULL_VALUE: Null value. rpcStatus: diff --git a/proto/v1beta1/models.pb.go b/proto/v1beta1/models.pb.go index eece1f884..07b2d69ea 100644 --- a/proto/v1beta1/models.pb.go +++ b/proto/v1beta1/models.pb.go @@ -5915,7 +5915,7 @@ var file_raystack_frontier_v1beta1_models_proto_rawDesc = []byte{ 0x28, 0x09, 0x42, 0x19, 0xfa, 0x42, 0x16, 0x72, 0x14, 0x52, 0x06, 0x63, 0x72, 0x65, 0x64, 0x69, 0x74, 0x52, 0x07, 0x66, 0x65, 0x61, 0x74, 0x75, 0x72, 0x65, 0xd0, 0x01, 0x01, 0x52, 0x04, 0x74, 0x79, 0x70, 0x65, 0x12, 0x1f, 0x0a, 0x06, 0x61, 0x6d, 0x6f, 0x75, 0x6e, 0x74, 0x18, 0x06, 0x20, - 0x01, 0x28, 0x03, 0x42, 0x07, 0xfa, 0x42, 0x04, 0x22, 0x02, 0x28, 0x00, 0x52, 0x06, 0x61, 0x6d, + 0x01, 0x28, 0x03, 0x42, 0x07, 0xfa, 0x42, 0x04, 0x22, 0x02, 0x20, 0x00, 0x52, 0x06, 0x61, 0x6d, 0x6f, 0x75, 0x6e, 0x74, 0x12, 0x17, 0x0a, 0x07, 0x75, 0x73, 0x65, 0x72, 0x5f, 0x69, 0x64, 0x18, 0x07, 0x20, 0x01, 0x28, 0x09, 0x52, 0x06, 0x75, 0x73, 0x65, 0x72, 0x49, 0x64, 0x12, 0x33, 0x0a, 0x08, 0x6d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x18, 0x14, 0x20, 0x01, 0x28, 0x0b, 0x32, diff --git a/proto/v1beta1/models.pb.validate.go b/proto/v1beta1/models.pb.validate.go index 5e19febf2..1f3f72719 100644 --- a/proto/v1beta1/models.pb.validate.go +++ b/proto/v1beta1/models.pb.validate.go @@ -6839,10 +6839,10 @@ func (m *Usage) validate(all bool) error { } - if m.GetAmount() < 0 { + if m.GetAmount() <= 0 { err := UsageValidationError{ field: "Amount", - reason: "value must be greater than or equal to 0", + reason: "value must be greater than 0", } if !all { return err