search

stephanstapel / ZUGFeRD-csharp

C# assembly for creating and reading ZUGFeRD invoices
Apache License 2.0
214 stars 114 forks source link

Change SpecifiedTradePaymentTerms from type to List<> #370

Closed jochenkirstaetter closed 1 month ago

jochenkirstaetter commented 1 month ago

Following the conversation in issue #223, I modified the PaymentTerms from single type to List<>. Minor, necessary changes have been made to a few tests accessing the previous PaymentTerms property directly.

Added

Changed

During the conversation it turned out that the cardinality of SpecifiedTradePaymentTerms as well as its properties depend mainly on the ZUGFeRD.Profile classification. The Writer/Reader types have been amended / extended accordingly given best knowledge of the specifications.

jochenkirstaetter commented 1 month ago

Did a couple of online validations and the results looked good. Let me know whether this is acceptable and if there is more work to do, eg. handle Profile differently?

Cheers, JoKi

stephanstapel commented 1 month ago

@jochenkirstaetter : thanks a lot for the PR, I have added a few comments above.

jochenkirstaetter commented 1 month ago

@stephanstapel you're most welcome, thanks. I commented the marked areas with my thoughts and decision process. Looking forward to your input.

Cheers, JoKi

jochenkirstaetter commented 1 month ago

Managed to find some valuable information in the Factur-X Specification 1.01.07, chapter 7.6 The Profile EXTENDED

image

I hope that's helpful for this issue as well as a couple of other Extended changes.

stephanstapel commented 1 month ago

Managed to find some valuable information in the Factur-X Specification 1.01.07, chapter 7.6 The Profile EXTENDED

image

I hope that's helpful for this issue as well as a couple of other Extended changes.

Thanks. I'm just wondering if we need to follow all of these....

jochenkirstaetter commented 1 month ago

@stephanstapel I'm too new to this material. Right now, I would leave it to on-demand. In case there are package consumers needing a specific requirement in Extended profile, like eg. those very PaymentTerms ;-), then there shall be an issue filed and the necessary changes could be implemented then.

Following YAGNI principle and to avoid further havoc in the package. Maybe it's just me thinking too simple.

stephanstapel commented 1 month ago

@jochenkirstaetter, Could you review this code:

https://github.com/stephanstapel/ZUGFeRD-csharp/blob/e42fc77fa64df1324d1b9feeb5c018aa715d9f8b/ZUGFeRD/EnumExtensions.cs

Thanks a lot in advance!

jochenkirstaetter commented 1 month ago

Yes, sure, will do. Going to review it in about an hour or so. A bit occupied right now.

jochenkirstaetter commented 1 month ago

Hi @stephanstapel

@jochenkirstaetter, Could you review this code:

Looks good to me. There are a few enums that would have special handling, eg. CountryCode - Kosovo or GlobalIDSchemeIdentifiers, but that shall be fine. In such situation either don't use the generic extension methods or provide an additional method that would wrap the generic behaviour and extend it with the specialised one.

Nonetheless, that shall remove some redundant source code.