Skip to content

Commit d052d3c

Browse files
turipturip
committed
fix: review comments
1 parent 0b63d60 commit d052d3c

5 files changed

Lines changed: 216 additions & 4 deletions

File tree

‎.agents/skills/billing/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -172,7 +172,7 @@ The billing line-engine contract lives in `openmeter/billing/lineengine.go`. Bil
172172
**Hook sequence and ownership**:
173173
- `BuildStandardInvoiceLines`
174174
- called while converting gathering lines into a new standard invoice in `service/gatheringinvoicependinglines.go`
175-
- must return standard lines reusing the exact same line IDs as the input gathering lines
175+
- must return standard lines reusing the same line IDs as the input gathering lines
176176
- `OnStandardInvoiceCreated`
177177
- called after the standard invoice and standard lines have been persisted
178178
- may mutate and return replacement lines

‎openmeter/billing/charges/service/invoicable_test.go‎

Lines changed: 9 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -222,10 +222,12 @@ func (s *InvoicableChargesTestSuite) TestFlatFeePartialCreditRealizations() {
222222
defer s.FlatFeeTestHandler.Reset()
223223

224224
authorizedCallback := newCountedLedgerTransactionCallback[flatfee.Charge]()
225+
// Use non-fatal assertions inside handler callbacks so failures are reported
226+
// on the callback's testing context without aborting the parent test flow.
225227
s.FlatFeeTestHandler.onPaymentAuthorized = authorizedCallback.Handler(s.T(), func(t *testing.T, charge flatfee.Charge) {
226-
s.Require().NotNil(charge.Realizations.AccruedUsage)
227-
s.Nil(charge.Realizations.Payment)
228-
s.Equal(flatfee.StatusActive, charge.Status)
228+
assert.NotNil(t, charge.Realizations.AccruedUsage)
229+
assert.Nil(t, charge.Realizations.Payment)
230+
assert.Equal(t, flatfee.StatusActive, charge.Status)
229231
})
230232

231233
invoiceUsageAccruedCallback := newCountedLedgerTransactionCallback[flatfee.OnInvoiceUsageAccruedInput]()
@@ -265,13 +267,17 @@ func (s *InvoicableChargesTestSuite) TestFlatFeePartialCreditRealizations() {
265267
defer s.FlatFeeTestHandler.Reset()
266268

267269
authorizedCallback := newCountedLedgerTransactionCallback[flatfee.Charge]()
270+
// Use non-fatal assertions inside handler callbacks so failures are reported
271+
// on the callback's testing context without aborting the parent test flow.
268272
s.FlatFeeTestHandler.onPaymentAuthorized = authorizedCallback.Handler(s.T(), func(t *testing.T, charge flatfee.Charge) {
269273
assert.Nil(t, charge.Realizations.Payment)
270274
assert.NotNil(t, charge.Realizations.AccruedUsage)
271275
assert.Equal(t, flatfee.StatusActive, charge.Status)
272276
})
273277

274278
settledCallback := newCountedLedgerTransactionCallback[flatfee.Charge]()
279+
// Use non-fatal assertions inside handler callbacks so failures are reported
280+
// on the callback's testing context without aborting the parent test flow.
275281
s.FlatFeeTestHandler.onPaymentSettled = settledCallback.Handler(s.T(), func(t *testing.T, charge flatfee.Charge) {
276282
assert.NotNil(t, charge.Realizations.Payment)
277283
assert.NotNil(t, charge.Realizations.Payment.Authorized)

‎openmeter/billing/charges/service/invoice.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,9 @@ func (s *service) handleStandardInvoiceUpdate(ctx context.Context, invoice billi
5959
creditPurchase: s.creditPurchaseService.PostInvoicePaymentAuthorized,
6060
})
6161

62+
// Temporary hack to handle the case where the invoice is authorized and settled in one go
63+
// This should be removed once we transition to the line-engine based invocation of hooks in all
64+
// charge types.
6265
case billing.StandardInvoiceStatusPaymentProcessingBookingAuthorizedAndSettled:
6366
return s.handleChargeEvent(ctx, invoice, processorByType{
6467
flatFee: s.flatFeeService.PostInvoicePaymentAuthorized,

‎openmeter/server/server_test.go‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1870,6 +1870,10 @@ func (n NoopBillingService) ApproveInvoice(ctx context.Context, input billing.Ap
18701870
return billing.StandardInvoice{}, nil
18711871
}
18721872

1873+
func (n NoopBillingService) PaymentAuthorized(ctx context.Context, input billing.PaymentAuthorizedInput) (billing.StandardInvoice, error) {
1874+
return billing.StandardInvoice{}, nil
1875+
}
1876+
18731877
func (n NoopBillingService) RetryInvoice(ctx context.Context, input billing.RetryInvoiceInput) (billing.StandardInvoice, error) {
18741878
return billing.StandardInvoice{}, nil
18751879
}

‎test/billing/lineengine_test.go‎

Lines changed: 199 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -998,3 +998,202 @@ func (s *LineEngineTestSuite) TestOnPaymentSettledFailureTransitionsToRetryableP
998998
s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status)
999999
})
10001000
}
1001+
1002+
func (s *LineEngineTestSuite) TestOnPaymentSettledIsCalledAfterAuthorization() {
1003+
var (
1004+
ctx = s.T().Context()
1005+
namespace = s.GetUniqueNamespace("ns-line-engine-on-payment-settled-after-authorization")
1006+
mockEngine = &mockCollectionCompletedLineEngine{engineType: ombilling.LineEngineTypeChargeCreditPurchase}
1007+
customInvoicingApp = s.SetupCustomInvoicing(namespace).App
1008+
invoice ombilling.StandardInvoice
1009+
collectionAt time.Time
1010+
onPaymentSettledCnt int
1011+
)
1012+
1013+
clockBase := lo.Must(time.Parse(time.RFC3339, "2024-09-02T12:13:14Z"))
1014+
clock.SetTime(clockBase)
1015+
defer clock.ResetTime()
1016+
defer func() { _ = s.MeterAdapter.ReplaceMeters(ctx, []meter.Meter{}) }()
1017+
defer s.MockStreamingConnector.Reset()
1018+
s.registerMockLineEngine(s.T(), mockEngine)
1019+
defer s.unregisterLineEngine(s.T(), mockEngine)
1020+
1021+
s.Run("Given a draft invoice waiting for collection with a payment-settled hook after authorization", func() {
1022+
defer mockEngine.Reset()
1023+
1024+
mockEngine.buildStandardInvoiceLines = func(_ context.Context, input ombilling.BuildStandardInvoiceLinesInput) (ombilling.StandardLines, error) {
1025+
return mustAsNewStandardLines(input), nil
1026+
}
1027+
1028+
invoice, collectionAt = s.createMeteredDraftInvoiceWaitingForCollectionForApp(
1029+
ctx,
1030+
namespace,
1031+
customInvoicingApp.GetID(),
1032+
mockEngine.GetLineEngineType(),
1033+
"UBP - payment settled hook after authorization",
1034+
)
1035+
})
1036+
1037+
s.Run("When the invoice is collected, issued, marked authorized, and then marked paid", func() {
1038+
defer mockEngine.Reset()
1039+
1040+
mockEngine.onCollectionCompleted = func(_ context.Context, input ombilling.OnCollectionCompletedInput) (ombilling.StandardLines, error) {
1041+
lines := input.Lines
1042+
for _, stdLine := range lines {
1043+
if stdLine.UsageBased == nil {
1044+
stdLine.UsageBased = &ombilling.UsageBasedLine{}
1045+
}
1046+
1047+
stdLine.UsageBased.Quantity = lo.ToPtr(alpacadecimal.NewFromInt(7))
1048+
stdLine.UsageBased.MeteredQuantity = lo.ToPtr(alpacadecimal.NewFromInt(7))
1049+
stdLine.UsageBased.PreLinePeriodQuantity = lo.ToPtr(alpacadecimal.Zero)
1050+
stdLine.UsageBased.MeteredPreLinePeriodQuantity = lo.ToPtr(alpacadecimal.Zero)
1051+
}
1052+
1053+
return lines, nil
1054+
}
1055+
1056+
mockEngine.onInvoiceIssued = func(_ context.Context, input ombilling.OnInvoiceIssuedInput) error {
1057+
s.Equal(invoice.ID, input.Invoice.ID)
1058+
return nil
1059+
}
1060+
1061+
mockEngine.onPaymentSettled = func(_ context.Context, input ombilling.OnPaymentSettledInput) error {
1062+
onPaymentSettledCnt++
1063+
s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingBookingSettled, input.Invoice.Status)
1064+
s.Equal(invoice.ID, input.Invoice.ID)
1065+
s.Len(input.Lines, 1)
1066+
s.Equal(invoice.ID, input.Lines[0].InvoiceID)
1067+
s.Equal(mockEngine.GetLineEngineType(), input.Lines[0].Engine)
1068+
return nil
1069+
}
1070+
1071+
clock.SetTime(collectionAt.Add(time.Minute))
1072+
1073+
var err error
1074+
invoice, err = s.BillingService.AdvanceInvoice(ctx, invoice.GetInvoiceID())
1075+
s.Require().NoError(err)
1076+
s.Equal(ombilling.StandardInvoiceStatusDraftWaitingAutoApproval, invoice.Status)
1077+
1078+
invoice, err = s.BillingService.ApproveInvoice(ctx, invoice.GetInvoiceID())
1079+
s.Require().NoError(err)
1080+
s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingPending, invoice.Status)
1081+
1082+
invoice = s.markInvoiceAuthorized(ctx, invoice.GetInvoiceID())
1083+
s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingAuthorized, invoice.Status)
1084+
1085+
invoice = s.markInvoicePaid(ctx, invoice.GetInvoiceID())
1086+
s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status)
1087+
})
1088+
1089+
s.Run("Then the payment-settled hook is called once", func() {
1090+
s.Equal(1, onPaymentSettledCnt)
1091+
s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status)
1092+
})
1093+
}
1094+
1095+
func (s *LineEngineTestSuite) TestOnPaymentSettledFailureAfterAuthorizationTransitionsToRetryablePaymentState() {
1096+
var (
1097+
ctx = s.T().Context()
1098+
namespace = s.GetUniqueNamespace("ns-line-engine-on-payment-settled-failed-after-authorization")
1099+
mockEngine = &mockCollectionCompletedLineEngine{engineType: ombilling.LineEngineTypeChargeCreditPurchase}
1100+
customInvoicingApp = s.SetupCustomInvoicing(namespace).App
1101+
invoice ombilling.StandardInvoice
1102+
collectionAt time.Time
1103+
onPaymentSettledCnt int
1104+
)
1105+
1106+
clockBase := lo.Must(time.Parse(time.RFC3339, "2024-09-02T12:13:14Z"))
1107+
clock.SetTime(clockBase)
1108+
defer clock.ResetTime()
1109+
defer func() { _ = s.MeterAdapter.ReplaceMeters(ctx, []meter.Meter{}) }()
1110+
defer s.MockStreamingConnector.Reset()
1111+
s.registerMockLineEngine(s.T(), mockEngine)
1112+
defer s.unregisterLineEngine(s.T(), mockEngine)
1113+
1114+
s.Run("Given a draft invoice waiting for collection with a failing payment-settled hook after authorization", func() {
1115+
defer mockEngine.Reset()
1116+
1117+
mockEngine.buildStandardInvoiceLines = func(_ context.Context, input ombilling.BuildStandardInvoiceLinesInput) (ombilling.StandardLines, error) {
1118+
return mustAsNewStandardLines(input), nil
1119+
}
1120+
1121+
invoice, collectionAt = s.createMeteredDraftInvoiceWaitingForCollectionForApp(
1122+
ctx,
1123+
namespace,
1124+
customInvoicingApp.GetID(),
1125+
mockEngine.GetLineEngineType(),
1126+
"UBP - payment settled hook failed after authorization",
1127+
)
1128+
})
1129+
1130+
s.Run("When the invoice is collected, issued, marked authorized, and payment settlement hits the failing hook", func() {
1131+
defer mockEngine.Reset()
1132+
1133+
mockEngine.onCollectionCompleted = func(_ context.Context, input ombilling.OnCollectionCompletedInput) (ombilling.StandardLines, error) {
1134+
lines := input.Lines
1135+
for _, stdLine := range lines {
1136+
if stdLine.UsageBased == nil {
1137+
stdLine.UsageBased = &ombilling.UsageBasedLine{}
1138+
}
1139+
1140+
stdLine.UsageBased.Quantity = lo.ToPtr(alpacadecimal.NewFromInt(7))
1141+
stdLine.UsageBased.MeteredQuantity = lo.ToPtr(alpacadecimal.NewFromInt(7))
1142+
stdLine.UsageBased.PreLinePeriodQuantity = lo.ToPtr(alpacadecimal.Zero)
1143+
stdLine.UsageBased.MeteredPreLinePeriodQuantity = lo.ToPtr(alpacadecimal.Zero)
1144+
}
1145+
1146+
return lines, nil
1147+
}
1148+
1149+
mockEngine.onInvoiceIssued = func(_ context.Context, input ombilling.OnInvoiceIssuedInput) error {
1150+
s.Equal(invoice.ID, input.Invoice.ID)
1151+
return nil
1152+
}
1153+
1154+
mockEngine.onPaymentSettled = func(_ context.Context, input ombilling.OnPaymentSettledInput) error {
1155+
onPaymentSettledCnt++
1156+
s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingBookingSettled, input.Invoice.Status)
1157+
s.Equal(invoice.ID, input.Invoice.ID)
1158+
return errors.New("simulated payment settled failure")
1159+
}
1160+
1161+
clock.SetTime(collectionAt.Add(time.Minute))
1162+
1163+
var err error
1164+
invoice, err = s.BillingService.AdvanceInvoice(ctx, invoice.GetInvoiceID())
1165+
s.Require().NoError(err)
1166+
s.Equal(ombilling.StandardInvoiceStatusDraftWaitingAutoApproval, invoice.Status)
1167+
1168+
invoice, err = s.BillingService.ApproveInvoice(ctx, invoice.GetInvoiceID())
1169+
s.Require().NoError(err)
1170+
s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingPending, invoice.Status)
1171+
1172+
invoice = s.markInvoiceAuthorized(ctx, invoice.GetInvoiceID())
1173+
s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingAuthorized, invoice.Status)
1174+
1175+
invoice = s.markInvoicePaid(ctx, invoice.GetInvoiceID())
1176+
s.Equal(ombilling.StandardInvoiceStatusPaymentProcessingBookingSettledFailed, invoice.Status)
1177+
s.True(invoice.StatusDetails.Failed)
1178+
s.NotNil(invoice.StatusDetails.AvailableActions.Retry)
1179+
s.NotEmpty(invoice.ValidationIssues)
1180+
s.Equal(ombilling.ValidationIssueSeverityCritical, invoice.ValidationIssues[0].Severity)
1181+
s.Equal("simulated payment settled failure", invoice.ValidationIssues[0].Message)
1182+
})
1183+
1184+
s.Run("Then retry succeeds without restarting payment processing", func() {
1185+
defer mockEngine.Reset()
1186+
1187+
mockEngine.onPaymentSettled = func(_ context.Context, input ombilling.OnPaymentSettledInput) error {
1188+
onPaymentSettledCnt++
1189+
s.Equal(invoice.ID, input.Invoice.ID)
1190+
return nil
1191+
}
1192+
1193+
var err error
1194+
invoice, err = s.BillingService.RetryInvoice(ctx, invoice.GetInvoiceID())
1195+
s.Require().NoError(err)
1196+
s.Equal(2, onPaymentSettledCnt)
1197+
s.Equal(ombilling.StandardInvoiceStatusPaid, invoice.Status)
1198+
})
1199+
}

0 commit comments

Comments
 (0)