Treat empty tags as absent when parsing

[CarlosMari]

Two details that were being dropped between UBL and GOBL, both found building
the Danish OIOUBL converter against Erhvervsstyrelsen's official sample corpus.
Each one turns a perfectly good source document into GOBL that fails GOBL's own
validation.

• An empty tag is not an empty value. A self-closing <cbc:Telephone/>
  unmarshals to a pointer to an empty string, not nil, so goblParty built a
  Telephone with a blank Number — a required field. The document then failed
  validation for a party that simply had no phone number.
• Free-text delivery terms carry no identity. DeliveryTerms with no ID
  produced an org.Identity with no code, which also fails validation.

Scope

Deliberately small: both are parse-side fixes to an EN 16931 mapping.

Several larger changes were considered and dropped after measuring them:

• Moving the delivery location's SchemeID from Identity.Label to the
  iso-scheme-id extension. It changed behaviour for every UBL consumer, forced
  all 12 golden fixtures to be regenerated, and cost DELIVERY_Advanced_Invoice_02
  from the corpus, because a scheme written as an extension is not read back on
  the way out.
• Reading the delivery party's PostalAddress. UBL-CR-394 says a UBL invoice
  should not carry one, so this moved to gobl.dk.oioubl, which relocates it to
  the delivery location before parsing.
• Omitting an empty <cac:DeliveryLocation/>, and emitting a location that
  states only an identifier (BT-71). Both are real — Peppol and OIOUBL each
  reject the empty element (F-INV239) — but neither shape arises from a corpus
  document, and outbound senders are expected to state a delivery address.

Verification

• Build, vet, lint, tests: green. No golden fixtures change, so this is
  behaviour-preserving for every existing test.
• Corpus round-trip xml → gobl → xml → gobl → xml with schematron at every
  stage: 100/142 clean, unchanged. Of the 456 OIOUBL documents there, 121 reach
  the parser and 114 convert to a valid envelope; the rest are Orders, Reminders
  and other document types this converter does not handle.

[codecov-commenter]

Codecov Report

❌ Patch coverage is 90.90909% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 81.01%. Comparing base (7939f25) to head (865e45d).

Files with missing lines	Patch %	Lines
delivery_parse.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #116      +/-   ##
==========================================
+ Coverage   80.97%   81.01%   +0.03%     
==========================================
  Files          28       28              
  Lines        2145     2149       +4     
==========================================
+ Hits         1737     1741       +4     
  Misses        269      269              
  Partials      139      139              

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Copilot]

Pull request overview

Adjusts UBL → GOBL party contact parsing so that present-but-empty contact tags don’t produce invalid GOBL Telephone/Email entries with required fields left blank, avoiding downstream validation failures on otherwise-valid source documents.

Changes:

• Treat empty <cbc:Telephone/> values as absent by only creating org.Telephone when the cleaned number is non-empty.
• Treat empty <cbc:ElectronicMail/> values as absent by only creating org.Email when the cleaned address is non-empty.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

party_parse.go:64

• This change fixes a real-world parsing edge case (self-closing/empty <cbc:Telephone/> and <cbc:ElectronicMail/>), but there is no test asserting that empty tags result in no Telephones/Emails entries. Adding a fixture (or targeted parsing test) that includes empty contact tags would prevent regressions and validate the behavior described in the PR.

	if party.Contact != nil {
		// A present-but-empty tag (e.g. <cbc:Telephone/>) means no value was
		// given, not a blank one; only add the entry once there's something to clean.
		if party.Contact.Telephone != nil {
			number := cleanString(*party.Contact.Telephone)
			if number != "" {
				p.Telephones = []*org.Telephone{

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (3)

party_parse.go:63

• The new logic treats empty cbc:Telephone/ and cbc:ElectronicMail/ as absent, but there doesn’t appear to be test coverage for self-closing/empty contact tags. Adding a regression test would help prevent reintroducing blank required fields (e.g., by parsing an XML invoice that contains empty Telephone/ElectronicMail elements and asserting no telephones/emails are produced).

		// A present-but-empty tag (e.g. <cbc:Telephone/>) means no value was
		// given, not a blank one; only add the entry once there's something to clean.
		if party.Contact.Telephone != nil {
			number := cleanString(*party.Contact.Telephone)
			if number != "" {

delivery_parse.go:49

• DeliveryLocation.ID.SchemeID is being mapped into org.Identity.Type. Elsewhere in the parser, SchemeID is preserved via the ISO scheme-id extension (e.g., party_parse.go:166-170, 272-275) rather than using the Identity.Type field (which is also used for other semantics like tax scheme). Using Ext for SchemeID here would keep delivery identities consistent with the rest of the codebase and JSON outputs ("ext": {"iso-scheme-id": ...}) rather than switching to "type".

					Code: cbc.Code(del.DeliveryLocation.ID.Value),
				}
				if del.DeliveryLocation.ID.SchemeID != nil {
					id.Label = *del.DeliveryLocation.ID.SchemeID
				}

delivery.go:49

• The PR description focuses on empty Contact/Telephone and ElectronicMail tags, but this change set also modifies delivery emission/parsing behavior (DeliveryLocation suppression, DeliveryTerms identity handling) and updates multiple golden JSON fixtures accordingly. Please update the PR description to cover the delivery/identity changes (or split into separate PRs) so reviewers can validate intent and downstream impact more easily.

	// The location carries the identifier (BT-71) and the address (BG-15). It
	// is written only when there is something to put in it: an empty
	// DeliveryLocation is rejected by some profiles, and the identifier stands
	// on its own even when no party is named.
	loc := new(Location)

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (2)

delivery.go:53

• newAddress always returns a non-nil *PostalAddress when Receiver.Addresses is non-empty, even if none of the fields are set. That means this code can still emit an empty <cac:Address/> (and therefore a non-empty DeliveryLocation) despite the intent/comment to only write the location when there is something to include. This can happen if the inbound UBL had a present-but-empty postal address tag that parsed into an all-zero org.Address.

	loc := new(Location)
	if del.Receiver != nil {
		out.DeliveryParty = newDeliveryParty(del.Receiver)
		loc.Address = newAddress(del.Receiver.Addresses, ctx)
	}

party_parse.go:74

• This change fixes a real-world parsing edge case (present-but-empty contact tags), but there’s no test covering it. Adding a fixture with <cbc:Telephone/> / <cbc:ElectronicMail/> (or equivalent) and asserting that Telephones/Emails are absent would prevent regressions and document the expected behavior.

	if party.Contact != nil {
		// A present-but-empty tag (e.g. <cbc:Telephone/>) means no value was
		// given, not a blank one; only add the entry once there's something to clean.
		if party.Contact.Telephone != nil {
			number := cleanString(*party.Contact.Telephone)
			if number != "" {
				p.Telephones = []*org.Telephone{
					{
						Number: number,
					},
				}
			}
		}
		if party.Contact.ElectronicMail != nil {
			address := cleanString(*party.Contact.ElectronicMail)
			if address != "" {
				p.Emails = []*org.Email{

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

party_parse.go:63

• Add a regression test for the new behavior where present-but-empty cbc:Telephone/ and cbc:ElectronicMail/ tags are treated as absent; currently this logic change in goblParty isn't covered by the existing party parsing tests, so a fixture with empty tags could slip back in without failing CI.

		// A present-but-empty tag (e.g. <cbc:Telephone/>) means no value was
		// given, not a blank one; only add the entry once there's something to clean.
		if party.Contact.Telephone != nil {
			number := cleanString(*party.Contact.Telephone)
			if number != "" {

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

party_parse.go:63

• This change alters parsing behavior for empty cbc:Telephone/ / cbc:ElectronicMail/ tags, but there’s no regression test asserting that an empty tag results in no Telephone/Email entry (nil/len==0). Adding a focused test (e.g., a fixture with self-closing contact tags, or a ubl-package test that calls goblParty) would prevent reintroducing the original validation failure.

		if party.Contact.Telephone != nil {
			number := cleanString(*party.Contact.Telephone)
			if number != "" {
				p.Telephones = []*org.Telephone{
					{

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

delivery.go:52

• newAddress always returns a non-nil *PostalAddress when len(addresses) > 0, even if it contains no populated fields. That means loc.Address can become a non-nil but empty struct, and the loc.ID != nil || loc.Address != nil check will still emit an empty <cac:DeliveryLocation><cac:Address/></cac:DeliveryLocation>, despite the intent to avoid empty elements that fail profile validation.

	loc := new(Location)
	if del.Receiver != nil {
		out.DeliveryParty = newDeliveryParty(del.Receiver)
		loc.Address = newAddress(del.Receiver.Addresses, ctx)
	}

party_parse.go:75

• This change is meant to handle real-world UBL where <cbc:Telephone/> or <cbc:ElectronicMail/> is present but empty. party_parse_test.go currently only asserts non-empty phone/email cases; adding a regression test covering empty contact tags would help ensure this behavior doesn’t regress and that GOBL validation failures are prevented.

		if party.Contact.Telephone != nil {
			number := cleanString(*party.Contact.Telephone)
			if number != "" {
				p.Telephones = []*org.Telephone{
					{
						Number: number,
					},
				}
			}
		}
		if party.Contact.ElectronicMail != nil {
			address := cleanString(*party.Contact.ElectronicMail)
			if address != "" {
				p.Emails = []*org.Email{
					{
						Address: address,
					},

[Copilot]

Pull request overview

Copilot reviewed 6 out of 18 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

party_parse.go:110

• parseAddress always returns a non-nil *org.Address for any non-nil *PostalAddress, even when the XML element is present-but-empty. As written, if a := parseAddress(...); a != nil will therefore always be true, which means an empty <cac:PostalAddress/> will populate p.Addresses with an empty address and also prevent the p.Name == "" && len(p.Addresses) == 0 guard from returning nil (contradicting the “Nothing worth carrying” intent). Treat the parsed address as absent unless it contains at least one meaningful field.

	if party.PostalAddress != nil {
		if a := parseAddress(party.PostalAddress); a != nil {
			p.Addresses = []*org.Address{a}
		}
	}

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

delivery_test.go:71

• This slice literal is declared as []*org.Identity but includes a non-pointer composite literal ({}), which will not compile. Use a pointer literal for the empty identity so the test exercises the intended "unusable identity" case.

		inv.Delivery.Identities = append([]*org.Identity{nil, {}}, inv.Delivery.Identities...)

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

delivery_parse.go:72

• This block overwrites d.Identities that may already have been set from del.DeliveryLocation.ID above, which drops the delivery location identifier (BT-71) whenever DeliveryTerms.ID is present. To avoid data loss, append the delivery-terms identity instead of replacing the slice.

	if ui.DeliveryTerms != nil && ui.DeliveryTerms.ID != "" {
		d.Identities = []*org.Identity{
			{
				Code: cbc.Code(ui.DeliveryTerms.ID),
			},

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

delivery.go:54

• newDelivery only reads del.Identities[0] and unconditionally dereferences it. If the first identity is nil (possible with a []*org.Identity slice) this will panic; if the first identity has an empty code it will still emit a non-nil loc.ID with an empty value, causing an empty <cbc:ID> and potentially failing validation / dropping a later usable identity. Scan for the first non-nil identity with a non-empty code before setting loc.ID.

	if len(del.Identities) > 0 {
		loc.ID = &IDType{Value: del.Identities[0].Code.String()}
	}

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

delivery.go:62

• newDeliveryLocation indexes del.Identities[0] without checking for nil/empty identities. This can panic when the first identity is nil, and it can also emit an invalid empty <cbc:ID> when Code.String() is empty, or drop a later usable identity. Prefer scanning for the first non-nil identity with a non-empty code before setting loc.ID.

	if len(del.Identities) > 0 {
		loc.ID = &IDType{Value: del.Identities[0].Code.String()}
	}

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

delivery.go:65

• newDeliveryLocation unconditionally indexes del.Identities[0] and calls .Code.String(). Since Identities is a slice of pointers, this can panic when the first entry is nil, and it can also emit an invalid empty <cbc:ID> when the first identity’s code is empty. Scan for the first non-nil identity with a non-empty code before setting loc.ID.

	if len(del.Identities) > 0 {
		loc.ID = &IDType{Value: del.Identities[0].Code.String()}
	}

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

delivery_parse.go:72

• This block overwrites any d.Identities already set from del.DeliveryLocation.ID earlier in the function, so an invoice that includes both a delivery location ID and delivery terms ID will silently drop the location identifier. Since newDelivery later reads del.Identities[0] into DeliveryLocation.ID (delivery.go:51-53), this can also change round-trip behavior. Consider appending the delivery-terms identity instead of replacing the slice.

	if ui.DeliveryTerms != nil && ui.DeliveryTerms.ID != "" {
		d.Identities = []*org.Identity{
			{
				Code: cbc.Code(ui.DeliveryTerms.ID),
			},