Repository navigation
Fix #11390: Use ISO country codes in FatturaPA export - #11662
Marco Antonio Mauro (marcus905) wants to merge 19 commits into
Conversation
Good Sense Reviewer - Round 1Recommendation: Accept with SuggestionsWhat this PR doesThis change resolves FatturaPA country values through the related Country/Region record and writes its two-character ISO Code. It also reports missing or invalid ISO setup before XML generation. The seven changed XML values address the reported key-versus-ISO mismatch, and the customer regression test exercises the main scenario. One related domestic-country decision still compares record keys, so equivalent records with the same ISO code can still produce foreign-recipient XML behavior. Problem-solution fitFit: Partial The main serialization defect is fixed consistently across the identified XML values. The remaining key-based domestic-country comparisons and incomplete coverage leave related cases unresolved. SuggestionsS1 (🟠 Moderate): Use ISO codes for domestic-country decisions S2 (🟠 Moderate): Update self-billing vendor test setup S3 (🟠 Moderate): Cover the other changed country sources Risk assessment and necessityRisk: The production change is narrow and does not change schemas or public APIs. The main regression surface is FatturaPA and self-billing export when Country/Region keys differ from ISO codes or test data has no ISO Code. Necessity: The change is required because configurable Business Central record keys can produce country values that FatturaPA does not accept. Resolving the ISO Code is the correct approach and avoids requiring tenants to rename existing country records.
|
Good Sense Reviewer - Round 2Recommendation: Request ChangesWhat this PR doesThis round changes domestic-country decisions to compare ISO codes, updates self-billing test setup, and adds coverage for company, tax representative, and transmission intermediary sources. These changes address the three previous suggestions. The normal invoice path now validates the configured country sources before export. The self-billing vendor path still reaches the shared getter without the same length validation, so a nonblank one-character ISO value can be serialized. Status of previous suggestions
New observations (commits since round 1)S4 (🔴 High): Validate ISO length in the shared getter Risk assessment and necessityRisk: The main invoice path is now consistent and covered. The remaining risk is limited to malformed Country/Region data that bypassed field validation, but self-billing can then generate an invalid FatturaPA file and persist that the VAT entry was exported. Necessity: Resolving configurable Country/Region keys to ISO alpha-2 values is required for valid FatturaPA output. Enforcing the two-character invariant in every export path is needed to complete the fix.
|
|
Thanks for the follow-up review. S4 is now addressed.
I also added a regression test with a one-character ISO value inserted without field validation to verify that the shared getter rejects it with the existing FatturaPA-specific validation error.
|
Good Sense Reviewer - Round 3Recommendation: AcceptWhat this PR doesThis round makes the shared FatturaPA country helper require an ISO code of exactly two characters and adds a regression test for malformed nonblank data. The check now covers every caller of the shared helper, including self-billing. An invalid value stops XML generation before the VAT entry can be marked as exported, while the normal invoice validation continues to report the setup error before generation. Status of previous suggestions
New observations (commits since round 2)None - the changes address S4 without introducing a new issue. Risk assessment and necessityRisk: The change is narrow, uses an established two-character ISO validation pattern, and changes no schema, public API, or event contract. The main invoice and self-billing paths now reject malformed country data before producing an invalid FatturaPA document. Necessity: The validation completes the ISO-country fix and prevents invalid electronic documents when stored setup data bypasses normal field validation.
|
| Initialize(); | ||
|
|
||
| // [GIVEN] A Country/Region with a one-character ISO Code inserted without field validation | ||
| CountryRegion.Init(); |
There was a problem hiding this comment.
The test hand-rolls a Country/Region fixture with Init, an invented key, and Insert instead of using Library - ERM. Build the record through LibraryERM.CreateCountryRegion, then deliberately assign the malformed ISO value so the test retains its negative-case intent while relying on the maintained fixture setup.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
LibraryERM.CreateCountryRegion(CountryRegion);
CountryRegion."ISO Code" := 'U';
CountryRegion.Modify();Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6
| Initialize(); | ||
|
|
||
| // [GIVEN] A foreign Country/Region without an ISO Code | ||
| CountryRegion.Init(); |
There was a problem hiding this comment.
The test hand-rolls a Country/Region fixture with Init, an invented key, and Insert instead of using Library - ERM. Use LibraryERM.CreateCountryRegion so the prerequisite remains valid as the Country/Region schema evolves; the library-created record can still exercise the missing-ISO scenario.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
LibraryERM.CreateCountryRegion(CountryRegion);Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6
|
Thanks for the additional review! Both testing findings have now been addressed. The affected tests use The negative scenarios remain unchanged:
I rebuilt and reran Both affected regression tests passed. The eight unchanged failures are caused by the existing No production code was modified in this follow-up. Thanks again! |
Good Sense Reviewer - Round 4Recommendation: AcceptWhat this PR doesThe FatturaPA exporter resolves Country/Region keys to two-character ISO codes. This round only replaces manual Country/Region creation in two negative tests with Both tests still assign their invalid values after creating the record: one uses Status of previous suggestions
New observations (commits since round 3)None - the fixture changes preserve the intended invalid-ISO scenarios. Risk assessment and necessityRisk: The latest commit changes test setup only. The production ISO lookup and validation are unchanged, with no new schema or event-contract changes. This was a static review; no build or tests were run. Necessity: The ISO-country fix remains necessary because configurable Country/Region keys can otherwise produce invalid electronic invoices. Using the standard fixtures keeps the regression tests consistent with the test library without weakening their checks.
|
| "No.", CreateAndPostSalesInvoice(DocumentRecRef, CreatePaymentMethod(), CreatePaymentTerms(), CustomerNo)); | ||
|
|
||
| // [WHEN] The document is exported to FatturaPA | ||
| asserterror ElectronicDocumentFormat.SendElectronically( |
There was a problem hiding this comment.
The new negative test uses asserterror without asserting the error raised by the export call. Although the page handler checks that the expected validation message is displayed, an unrelated error can still satisfy asserterror; add an assertion for the outer error (or use the error-message library to assert the logged error) so the test cannot pass for the wrong failure.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.51.6
|
Thanks for the additional review! The latest testing finding has now been addressed. The negative ISO validation test now verifies both:
I verified the behavior against the standard I rebuilt and reran No production code was modified. Thanks! |
Good Sense Reviewer - Round 5Recommendation: AcceptWhat this PR doesThe FatturaPA exporter resolves configurable Country/Region keys to two-character ISO codes and validates the country setup. This round only adds an assertion for the expected empty rollback error in the missing-ISO export test; production code is unchanged. This round adds an exact empty-error assertion after the expected SendElectronically failure. The existing page handler checks the specific ISO validation message, while ShowErrorMessages(true) raises Error('') for the rollback, so the assertions cover different parts of the failure contract. Production code is unchanged in this round and all four previous fixes remain present. Problem-solution fitFit: Strong Resolving the separate ISO Code addresses the key-versus-country mismatch without renaming country records. The latest assertion accurately checks the existing validation rollback contract and strengthens the negative export regression test without changing its scenario. Status of previous suggestions
New observations (commits since round 4)None - no new issues found in the changes since the previous round. Risk assessment and necessityRisk: The latest delta changes one negative test only, with no production, schema, API, event, or posting change. The overall ISO-country serialization and domestic-country checks remain the relevant regression surface; runtime execution was not independently reproduced. Necessity: Without the ISO lookup, legitimate country keys that differ from ISO alpha-2 values can still produce rejected electronic invoices; renaming those keys is a disruptive workaround. The added assertion is useful because an unqualified expected error can otherwise conceal an unrelated export or handler failure.
|
Resolve the transmitter IdPaese/IdCodice overlap by retaining the upstream GetCompanyTransmitterId calculation and the PR's ISO-based country serialization. Preserve subsequent main changes, including the current XSD test path configuration. Assert the standard empty Dialog rollback explicitly in the missing ISO country negative export test. The error-message page handler keeps verifying the exact FatturaPA validation diagnostic.
|
Thanks for the review. Addressed in The negative ISO-country export test now asserts both I also merged the current upstream Validation: Codeunit 144200 executed 50 tests; 42 passed, including all five ISO-country regression scenarios. The 8 remaining failures are unchanged test-environment issues (Error Messages TestPage failures). The PR is now conflict-free and ready for renewed CI and review. |
What & why
The Italian FatturaPA exporter currently writes Business Central
Country/Region Codevalues directly into FatturaPA elements such asIdPaeseandNazione.However,
Country/Region Codeis the primary key of the Business CentralCountry/Regionrecord and is not required to be the ISO 3166-1 alpha-2 code.For example, a tenant can validly configure:
The current exporter can therefore emit:
instead of the FatturaPA-compatible ISO code:
This change resolves FatturaPA country values through the related
Country/Regionrecord and uses itsISO Codeinstead of assuming that the Business Central primary key is already the required ISO value.The change:
Country/Region."ISO Code"when generating FatturaPA country-code elements;Country/Region Codeprimary key and existing address data without requiring tenants to rename their country records.Linked work
Fixes #11390
Fixes AB#656997
How I validated this
What I tested and the outcome
Added regression coverage in
FatturaPATest.Codeunit.alfor the following scenarios:ISO Code;IdPaeseandNazione;Existing foreign-customer FatturaPA tests were also updated so their generated Country/Region test data contains a valid ISO code.
The localized Italian Base Application and test applications build successfully.
FatturaPA Testcodeunit 144200 was executed locally:Library - Error Message.LoadErrorMessagesbecause TestPage field ID2074815070is not present on pageError Messages. These failures are unrelated to the changes in this PR and are unchanged from the existing test baseline.Risk & compatibility
No standard field or table schema is changed.
Country/Region Coderemains the Business Central primary key and is not required to be changed to an ISO value. FatturaPA serialization now resolves the correspondingCountry/Region."ISO Code"when a two-character country code is required.Existing tenants that already use ISO alpha-2 values as their Country/Region keys continue to produce the same XML as long as the corresponding
ISO Codeis configured consistently.Country/Region records used by FatturaPA must now contain a valid two-character
ISO Code. Missing or invalid setup is reported before XML generation instead of allowing an invalid country code to be serialized.