Skip to content

Fix empty fields in payment output #57

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.

Sign up for GitHub

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

Open
alvarolivie wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Fix empty fields in payment output #57

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
56db125
Fix empty fields being generated in payment output
alvarolivie Mar 4, 2026
79609de
Add tests for payment and totals changes
alvarolivie Mar 5, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 15 additions & 8 deletions payment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,28 +131,32 @@ func (ui *Invoice) addPaymentInstructions(instr *pay.Instructions) error {
ui.PaymentMeans[0].PayeeFinancialAccount = newCreditTransferAccount(instr.CreditTransfer[0])
}
if instr.DirectDebit != nil {
ui.PaymentMeans[0].PaymentMandate = &PaymentMandate{
ID: IDType{Value: instr.DirectDebit.Ref},
mandate := &PaymentMandate{}
if instr.DirectDebit.Ref != "" {
mandate.ID = IDType{Value: instr.DirectDebit.Ref}
}
Comment on lines +134 to 137

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PaymentMandate.ID is a non-pointer IDType, so it will still be marshaled as <cbc:ID> with an empty value even when instr.DirectDebit.Ref == "". The conditional assignment here doesn’t actually prevent an empty ID from appearing in the XML; consider making PaymentMandate.ID a *IDType (or implementing custom marshaling) so it can be omitted when unset, and update the parsing side accordingly.

Copilot uses AI. Check for mistakes.
if instr.DirectDebit.Account != "" {
ui.PaymentMeans[0].PayerFinancialAccount = &FinancialAccount{
mandate.PayerFinancialAccount = &FinancialAccount{
ID: &instr.DirectDebit.Account,
}
}
ui.PaymentMeans[0].PaymentMandate = mandate
}
Comment on lines 133 to 144

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PaymentMandate is always allocated and assigned whenever instr.DirectDebit != nil, even if both Ref and Account are empty. That will still emit a <cac:PaymentMandate> element (and currently an empty <cbc:ID>), which seems to contradict the goal of avoiding empty output fields. Consider only setting ui.PaymentMeans[0].PaymentMandate when at least one meaningful DirectDebit field is present, otherwise leave it nil.

Copilot uses AI. Check for mistakes.
if instr.Card != nil {
ui.PaymentMeans[0].CardAccount = &CardAccount{
PrimaryAccountNumberID: &instr.Card.Last4,
card := &CardAccount{}
if instr.Card.Last4 != "" {
card.PrimaryAccountNumberID = &instr.Card.Last4
}
if instr.Card.Holder != "" {
ui.PaymentMeans[0].CardAccount.HolderName = &instr.Card.Holder
card.HolderName = &instr.Card.Holder
}
ui.PaymentMeans[0].CardAccount = card

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CardAccount is always allocated and assigned whenever instr.Card != nil, even when both Last4 and Holder are empty. This can still produce an empty <cac:CardAccount/> element in the XML. To fully avoid empty payment output, consider only setting ui.PaymentMeans[0].CardAccount when at least one card field is non-empty.

Suggested change
ui.PaymentMeans[0].CardAccount = card
if card.PrimaryAccountNumberID != nil || card.HolderName != nil {
ui.PaymentMeans[0].CardAccount = card
}

Copilot uses AI. Check for mistakes.
}
return nil
}

func newCreditTransferAccount(ct *pay.CreditTransfer) *FinancialAccount {
pfa := new(FinancialAccount)
pfa := &FinancialAccount{}
if ct.IBAN != "" {
pfa.ID = &ct.IBAN
} else if ct.Number != "" {
Expand All @@ -164,6 +168,9 @@ func newCreditTransferAccount(ct *pay.CreditTransfer) *FinancialAccount {
if ct.BIC != "" {
pfa.FinancialInstitutionBranch = &Branch{ID: &ct.BIC}
}
if pfa.ID == nil && pfa.Name == nil && pfa.FinancialInstitutionBranch == nil {
return nil
}
Comment on lines +171 to +173

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This adds new behavior where newCreditTransferAccount returns nil when no fields are set. There are existing payment conversion tests, but none assert the nil/omission behavior for an empty credit transfer; adding a unit test/fixture covering an empty pay.CreditTransfer would help prevent regressions in the generated XML.

Copilot uses AI. Check for mistakes.
return pfa
}

Expand Down Expand Up @@ -193,7 +200,7 @@ func (ui *Invoice) addPaymentTerms(inv *bill.Invoice, pymt *bill.PaymentDetails)
}
} else if len(pymt.Terms.DueDates) == 1 && ui.CreditNoteTypeCode == "" {
ui.DueDate = formatDate(*pymt.Terms.DueDates[0].Date)
} else {
} else if pymt.Terms.Notes != "" {
ui.PaymentTerms = append(ui.PaymentTerms, PaymentTerms{
Note: []string{pymt.Terms.Notes},
})
Comment on lines +203 to 206

Copilot AI Mar 4, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PaymentTerms are now appended only when pymt.Terms.Notes is non-empty. Since this changes output behavior (previously an empty <cbc:Note/> could be emitted), it would be good to add a conversion test that covers the case where due dates are absent and Terms.Notes is empty to ensure ui.PaymentTerms stays empty.

Copilot uses AI. Check for mistakes.
Expand Down
94 changes: 94 additions & 0 deletions payment_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

ubl "github.com/invopop/gobl.ubl"
"github.com/invopop/gobl/bill"
"github.com/invopop/gobl/pay"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
)
Expand Down Expand Up @@ -36,6 +37,99 @@ func TestNewPayment(t *testing.T) {
assert.Equal(t, "DNBANOKK", *doc.PaymentMeans[0].PayeeFinancialAccount.FinancialInstitutionBranch.ID)
})

t.Run("credit transfer with no account fields omits financial account", func(t *testing.T) {
env, err := loadTestEnvelope("invoice-minimal.json")
require.NoError(t, err)

inv, ok := env.Extract().(*bill.Invoice)
require.True(t, ok)

inv.Payment.Instructions.CreditTransfer = []*pay.CreditTransfer{{}}

doc, err := ubl.ConvertInvoice(env)
require.NoError(t, err)
assert.Nil(t, doc.PaymentMeans[0].PayeeFinancialAccount)
})

t.Run("card with empty last4 omits PAN", func(t *testing.T) {
env, err := loadTestEnvelope("invoice-minimal.json")
require.NoError(t, err)

inv, ok := env.Extract().(*bill.Invoice)
require.True(t, ok)

inv.Payment.Instructions.Card = &pay.Card{Holder: "John Doe"}

doc, err := ubl.ConvertInvoice(env)
require.NoError(t, err)
require.NotNil(t, doc.PaymentMeans[0].CardAccount)
assert.Nil(t, doc.PaymentMeans[0].CardAccount.PrimaryAccountNumberID)
assert.Equal(t, "John Doe", *doc.PaymentMeans[0].CardAccount.HolderName)
})

t.Run("direct debit with empty ref omits mandate ID", func(t *testing.T) {
env, err := loadTestEnvelope("invoice-minimal.json")
require.NoError(t, err)

inv, ok := env.Extract().(*bill.Invoice)
require.True(t, ok)

inv.Payment.Instructions.DirectDebit = &pay.DirectDebit{Account: "DE89370400440532013000"}

doc, err := ubl.ConvertInvoice(env)
require.NoError(t, err)
require.NotNil(t, doc.PaymentMeans[0].PaymentMandate)
assert.Empty(t, doc.PaymentMeans[0].PaymentMandate.ID.Value)
assert.Equal(t, "DE89370400440532013000", *doc.PaymentMeans[0].PaymentMandate.PayerFinancialAccount.ID)
})

t.Run("instruction detail mapped to payment means name", func(t *testing.T) {
env, err := loadTestEnvelope("invoice-minimal.json")
require.NoError(t, err)

inv, ok := env.Extract().(*bill.Invoice)
require.True(t, ok)

inv.Payment.Instructions.Detail = "Bank transfer"

doc, err := ubl.ConvertInvoice(env)
require.NoError(t, err)
require.NotNil(t, doc.PaymentMeans[0].PaymentMeansCode.Name)
assert.Equal(t, "Bank transfer", *doc.PaymentMeans[0].PaymentMeansCode.Name)
})

t.Run("payment terms with empty notes omits payment terms", func(t *testing.T) {
env, err := loadTestEnvelope("invoice-minimal.json")
require.NoError(t, err)

inv, ok := env.Extract().(*bill.Invoice)
require.True(t, ok)

inv.Payment.Terms.Notes = ""

doc, err := ubl.ConvertInvoice(env)
require.NoError(t, err)
assert.Empty(t, doc.PaymentTerms)
})

t.Run("BT-90 creditor ID on supplier when no payee", func(t *testing.T) {
env, err := loadTestEnvelope("invoice-minimal.json")
require.NoError(t, err)

inv, ok := env.Extract().(*bill.Invoice)
require.True(t, ok)

inv.Payment.Instructions.DirectDebit = &pay.DirectDebit{Creditor: "DE98ZZZ09999999999"}

doc, err := ubl.ConvertInvoice(env)
require.NoError(t, err)
require.Nil(t, doc.PayeeParty)
ids := doc.AccountingSupplierParty.Party.PartyIdentification
require.NotEmpty(t, ids)
assert.Equal(t, "DE98ZZZ09999999999", ids[len(ids)-1].ID.Value)
assert.Equal(t, "SEPA", *ids[len(ids)-1].ID.SchemeID)
})

t.Run("document type extension", func(t *testing.T) {
env, err := loadTestEnvelope("invoice-minimal.json")
require.NoError(t, err)
Expand Down
19 changes: 19 additions & 0 deletions totals_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,4 +24,23 @@ func TestNewTotals(t *testing.T) {
assert.Equal(t, "VAT", doc.TaxTotal[0].TaxSubtotal[0].TaxCategory.TaxScheme.ID)
assert.Equal(t, "21.0", *doc.TaxTotal[0].TaxSubtotal[0].TaxCategory.Percent)
})

t.Run("exemption reason from legal note with reverse-charge tag", func(t *testing.T) {
doc, err := testInvoiceFrom("peppol/peppol-reverse-charge.json")
require.NoError(t, err)

subtotal := doc.TaxTotal[0].TaxSubtotal[0]
require.NotNil(t, subtotal.TaxCategory.TaxExemptionReason)
assert.Equal(t, "Reverse Charge / Umkehr der Steuerschuld.", *subtotal.TaxCategory.TaxExemptionReason)
require.NotNil(t, subtotal.TaxCategory.TaxExemptionReasonCode)
assert.Equal(t, "VATEX-EU-AE", *subtotal.TaxCategory.TaxExemptionReasonCode)
})

t.Run("no exemption reason without reverse-charge tag", func(t *testing.T) {
doc, err := testInvoiceFrom("peppol/peppol-1.json")
require.NoError(t, err)

subtotal := doc.TaxTotal[0].TaxSubtotal[0]
assert.Nil(t, subtotal.TaxCategory.TaxExemptionReason)
})
}