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

Add line item level allowance charge #18

Merged
merged 1 commit into from
Dec 18, 2024

Conversation

phillipp
Copy link
Contributor

The standard allows for AllowanceCharge on the line item level. This is now added and must be an array, because there can be multiple AllowanceCharges (think multiple discounts).

The base_amount is optional in the standard, so we don't require it anymore.

@phillipp phillipp force-pushed the line-item-allowancecharge branch from 50e2bbd to 7c4621f Compare December 17, 2024 10:14
@phillipp
Copy link
Contributor Author

@corny Many of the bugs I fixed were simple adjustments to the XML that weren't detected because only a single top-level document is run through the validator. The fact that both specs and the validator exist can lead to either not using the validator or overlooking it entirely.

Additionally, it's a bit tedious that XML files always have to be edited for testing. Some of the XML files in spec/fixtures/scraps don't even match the spec, making it impossible to verify overall correctness properly.

My suggestion would be to restructure the testing setup so that the validator runs in daemon mode, and all tests build a complete document. This document would then be tested against the web interface. Request/response pairs would be recorded using VCR, making tests fast and eliminating the need for a local server when only changing code.

This would ensure that all generated XML is truly valid. What do you think?

@corny
Copy link
Member

corny commented Dec 17, 2024

I prefer to start the validator in a service container that must be running during the specs.

@phillipp
Copy link
Contributor Author

@corny was just asking what yourt preference was, this pull is not affected by this and can be merged ;-)

@corny corny merged commit 16aa58b into digineo:main Dec 18, 2024
4 checks passed
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.

2 participants