[halfbyte]

@ahmedm10 @neuged could you two please take a look at this PR and tell me if this variant of your two approaches would satisfy your needs? I had to wrestle with this a bit, tried various combinations and think that this, while being slightly convoluted, is probably the best alternative.

[ahmedm10]

Thanks for pulling the two approaches together! This works for us: the electronic address keeps its API, and TradeContact covers what we need for the seller contact (name, phone, email). Separating
exclude_electronic_address from exclude_tax is a nice clean-up too.

Two things I'd fix before merging, plus a few small notes.

1. assert_valid_xml can never fail (test/test_helper.rb)

assert [], errors

assert only checks that its first argument is truthy, and [] is truthy in Ruby, so this passes whatever the validator returns:

assert [], ["boom"]   # => passes

Since all validation checks in invoice_test.rb now go through this helper, the suite currently doesn't verify validity at all. Suggestion:

assert_empty errors, errors.join("\n")

2. exclude_contact still lets person_name through (lib/secretariat/trade_party.rb)

if !exclude_contact && trade_contact.present?
  trade_contact.to_xml(xml, version: version)
elsif person_name.present?
  ...

With exclude_contact: true the elsif branch still writes a DefinedTradeContact. With make_de_invoice (buyer has person_name: 'Max Mustermann') the ShipToTradeParty contains the contact, and the
Factur-X EN 16931 Schematron reports "Element 'ram:DefinedTradeContact' is marked as not used in the given context". That was already the case before this PR, but since exclude_contact is meant for exactly
this, maybe guard both branches:

unless exclude_contact
  if trade_contact.present?
    trade_contact.to_xml(xml, version: version)
  elsif person_name.present?
    xml['ram'].DefinedTradeContact do
      xml['ram'].PersonName person_name
    end
  end
end

(Finding 1 is why the tests don't catch this one.)

Smaller notes, take them or leave them

• test_electronic_addresses_of_seller_and_buyer used to check against the Schematron; it now uses validator: :schema (XSD only). The Schematron is what proves the address is fine in seller and buyer and
  absent from the ship-to party, so I'd keep validator: :schematron there. make_eu_invoice passes it cleanly.
• There's no test yet for what TradeContact writes: DepartmentName, TelephoneUniversalCommunication, EmailURIUniversalCommunication and their order. One assert_match on a seller with all four fields
  would pin it down.
• TradeContact#to_xml checks truthiness (if person_name), so an empty string writes an empty element. The rest of the gem uses present? (needs using ObjectExtensions in the new file).
• When both trade_contact and person_name are set, trade_contact wins silently. Maybe worth a short comment or README line.
• ELECTRONIC_ADDRESS_SCHEME_CODES isn't used anywhere yet. Fine as a convenience for callers, just mentioning it.

[halfbyte]

@ahmedm10 OMG, thanks for your remarks, that's embarassing. I think I'll do the correct thing and separate out the test helper changes into a separate PR with better checking and then rebase this PR and look through your notes. Thanks for the quick answer. I'll hopefully get to this latest next wednesday.

[neuged]

This works for us as well 👍

We won't be able to switch back to upstream quite yet after this, because we also would need PR #58, but that is another matter.

Thank you for putting this together!

[halfbyte]

@neuged Yeah, please give me some time for this, I went through a rough patch recently and didn't find the energy and time to work on this. I'm slowly working my way through the pile, but some of this actually needs my brain. Thanks for your patience and thanks to you both for your feedback.

[neuged]

No problem at all! Thank you!