-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
[billing] Implement ReconcileInvoices #12712
Conversation
c5fb522
to
7d9e602
Compare
invoice, err := c.GetUpcomingInvoice(ctx, customer.ID) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to find upcoming invoice for customer %s: %w", customer.ID, err) | ||
} | ||
|
||
_, err = c.UpdateInvoiceMetadata(ctx, invoice.ID, map[string]string{ | ||
ReportIDMetadataKey: summary.ReportID, | ||
}) | ||
if err != nil { | ||
return nil, fmt.Errorf("failed to udpate invoice %s metadata with report ID: %w", invoice.ID, err) | ||
} | ||
|
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.
Dropping this part as
- It already doesn't work as we can't update an UpcomingInvoice in Stripe with metadata
- We're no longer planning to use the metadata for sharing the report
7d9e602
to
5a2c563
Compare
5eb6411
to
33d67f5
Compare
5a2c563
to
559b00a
Compare
559b00a
to
34ef9cb
Compare
|
||
creditSummaryForTeams[id] = stripe.CreditSummary{ | ||
Credits: int64(math.Ceil(balance.CreditCents.ToCredits())), | ||
ReportID: "no-report", |
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.
This will be removed later, once we remove the other usage of the ReportID, for now it doesn't hurt us in any way as we do not use it during the UsageUpdate invoked below.
} | ||
} | ||
|
||
err = s.stripeClient.UpdateUsage(ctx, creditSummaryForTeams) |
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 I read correctly here, we might fail to update if the very first query is for a costCenter without stripe account, no?
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.
We fail if we fail to lookup customers from stripe. But we then iterate over the stripe customers which actually exist rather than over all the attribution IDs so in this case it would not fail on the first which doesn't have a stripe account.
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.
LGTM. My concern regarding generalization to attributionIds (or accounts, or costcenters) can be discussed and tackled in a follow-up PR.
return nil, status.Errorf(codes.Internal, "Failed to reconcile invoices.") | ||
} | ||
|
||
creditSummaryForTeams := map[string]stripe.CreditSummary{} |
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.
Can we generalize and talk about attributeId instead of teams?
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.
Yes, this requires a couple of changes. For one we need to start setting the attributionId: <value>
on stripe customers rather than the teamId
we set now
entity, id := balance.AttributionID.Values() | ||
|
||
// TODO: Support updating of user attribution IDs | ||
if entity != db.AttributionEntity_Team { |
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.
why do we need this exclusion here?
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.
Legacy of the focus on team based UBP first.
Description
Implements reconciliation of usage balances against Stripe.
Related Issue(s)
How to test
Release Notes
Documentation
Werft options: