Skip to content
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.

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

Issue #458 Allow Multiple payment details for invoice #459

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Frgetg
Copy link

@Frgetg Frgetg commented Aug 30, 2024

No description provided.

@Frgetg Frgetg changed the title Isue #458 Allow Multiple payment details for invoice Issue #458 Allow Multiple payment details for invoice Aug 30, 2024
@jstaerk
Copy link
Collaborator

jstaerk commented Sep 20, 2024

@Frgetg Good idea my issue is that you are changing public methods which would require a new major version of the software, can't you just add something like addPaymentTerms instead of changing setPaymentTerms to an array?

@Frgetg
Copy link
Author

Frgetg commented Oct 14, 2024

I can change the setter, yes, however, the getter will always have to return the whole array, or the XML serialization wont access all PaymentTerms.

And both a getPaymentTerms which always returns only the first one, and getAllPaymentTerms which returns everything feels rather hacky and error prone.

@marco-lennartz
Copy link
Contributor

@jstaerk how would you like to deal this? In the extended profile are 0...n SpecifiedTradePaymentTerms allowed. So the payment terms must be an array or a list. As @Frgetg stated handling a getter with only one PaymentTerm would be very error prone.

@jstaerk
Copy link
Collaborator

jstaerk commented Jan 6, 2025

a getter with only one PaymentTerm would be very error prone
if that getter already exists as a public method the maximum you can

  • either wait for the next major version, maybe in some years OR
  • add an alternative
  • continue behaving like previously with what is implemented
  • the maximum you could do is depreate the old getter

@Frgetg Frgetg force-pushed the issues/458-multiple-payment-details branch from 1abdded to 509b034 Compare January 18, 2025 14:13
@Frgetg Frgetg force-pushed the issues/458-multiple-payment-details branch from 509b034 to 42e8f80 Compare January 18, 2025 14:27
@Frgetg
Copy link
Author

Frgetg commented Jan 18, 2025

Hello @jstaerk

I adapted the patchset according to your guidelines

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants