Fix empty fields in payment output - #57
Conversation
Prevent blank XML elements from being emitted when source data has no meaningful values for credit transfer, card, mandate, and payment terms fields. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #57 +/- ##
==========================================
+ Coverage 76.12% 77.09% +0.97%
==========================================
Files 26 26
Lines 1889 1895 +6
==========================================
+ Hits 1438 1461 +23
+ Misses 329 312 -17
Partials 122 122 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR aims to reduce empty/meaningless XML output in generated UBL payment structures by omitting optional elements/fields when the corresponding GOBL payment data is empty.
Changes:
- Return
nilfromnewCreditTransferAccountwhen no account fields are set, avoiding empty financial account output. - Make card PAN and direct-debit mandate ID conditional on non-empty source values (and move direct-debit payer account under
PaymentMandate). - Only append
PaymentTermsnotes whenTerms.Notesis non-empty.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| mandate := &PaymentMandate{} | ||
| if instr.DirectDebit.Ref != "" { | ||
| mandate.ID = IDType{Value: instr.DirectDebit.Ref} | ||
| } |
There was a problem hiding this comment.
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.
| 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} | ||
| } | ||
| if instr.DirectDebit.Account != "" { | ||
| ui.PaymentMeans[0].PayerFinancialAccount = &FinancialAccount{ | ||
| mandate.PayerFinancialAccount = &FinancialAccount{ | ||
| ID: &instr.DirectDebit.Account, | ||
| } | ||
| } | ||
| ui.PaymentMeans[0].PaymentMandate = mandate | ||
| } |
There was a problem hiding this comment.
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.
| ui.PaymentMeans[0].CardAccount.HolderName = &instr.Card.Holder | ||
| card.HolderName = &instr.Card.Holder | ||
| } | ||
| ui.PaymentMeans[0].CardAccount = card |
There was a problem hiding this comment.
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.
| ui.PaymentMeans[0].CardAccount = card | |
| if card.PrimaryAccountNumberID != nil || card.HolderName != nil { | |
| ui.PaymentMeans[0].CardAccount = card | |
| } |
| if pfa.ID == nil && pfa.Name == nil && pfa.FinancialInstitutionBranch == nil { | ||
| return nil | ||
| } |
There was a problem hiding this comment.
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.
| } else if pymt.Terms.Notes != "" { | ||
| ui.PaymentTerms = append(ui.PaymentTerms, PaymentTerms{ | ||
| Note: []string{pymt.Terms.Notes}, | ||
| }) |
There was a problem hiding this comment.
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.
Summary
newCreditTransferAccountnow returnsnilinstead of an emptyFinancialAccountwhen no fields are setCardAccount.PrimaryAccountNumberIDonly set whenLast4is non-emptyPaymentMandate.IDonly set whenDirectDebit.Refis non-emptyPaymentTermsnote only appended whenTerms.Notesis non-empty