[main] bug 644746 - Add configurable EPO Service Setup for gov.cz domain migration - #9857
[main] bug 644746 - Add configurable EPO Service Setup for gov.cz domain migration#9857DavidHolusa wants to merge 5 commits into
Conversation
- Introduced new table and page for EPO Service Setup. - Updated EPO API Submission and VAT Report Submit codeunits to utilize new service setup. - Enhanced permission sets to include access to EPO Service Setup.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
- Replace TestField with ErrorInfo including navigation action to EPO Service Setup page - Extract GetOrInitEPOServiceSetup() helper procedure to avoid code duplication - Add customer consent confirmation when enabling EPO service - Move DataClassification to table level (SystemMetadata) and remove redundant field-level declarations - Move Label variables from local procedure vars to global var section (page and codeunit level) - Fix alphabetical ordering in permission set
Agentic PR Review - Round 1Recommendation: Request ChangesWhat this PR doesThis PR moves the Czech EPO open-form endpoint from a hardcoded URL into a new SuggestionsS1 - Setup defaults are not reliable S2 - Missing regression test for first use Risk assessment and necessityRisk: The regression surface is the CZ VAT report EPO submission flow. Existing tenants could move from a working hardcoded URL to an error about a missing or disabled setup record, or to a blank endpoint if Service Connections creates the record first. The procedure rename is internal, and no BaseApp event publisher change was found in this diff. Necessity: The work item describes a real government domain migration, so changing the endpoint and allowing configuration is justified. The scope is acceptable for the bug, but it must preserve first-use behavior for existing customers and create a valid default setup record on every entry point.
|
| var | ||
| EPOServiceSetupRecordRef: RecordRef; | ||
| begin | ||
| GetOrInitEPOServiceSetup(); |
There was a problem hiding this comment.
HandleEPOServiceConnection calls GetOrInitEPOServiceSetup(), which inserts the setup record from inside the OnRegisterServiceConnection subscriber when no row exists. This event runs while the Service Connections list is being populated, so simply rendering that page now performs a database write and can fail for sessions without insert permission on EPO Service Setup CZL. Keep the subscriber read-only: treat a missing setup record as disabled, or move first-time initialization to an explicit setup flow/page instead of the subscriber.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.26.4
|
|
||
| fields | ||
| { | ||
| field(1; "Primary Key"; Code[10]) |
There was a problem hiding this comment.
The new "EPO Service Setup CZL" table declares table-level DataClassification = SystemMetadata and relies on it cascading to the Normal fields "Primary Key", "Limit Response Time", and Enabled instead of setting an explicit field-level DataClassification on each. AppSourceCop AS0016 requires an explicit field-level DataClassification on every Normal field; relying on the table default is non-compliant even though the field "Open Form Endpoint" itself correctly declares CustomerContent explicitly.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.26.4
| OpenFormUriTok: Label 'epo_podani?otevriFormular=1', Locked = true; | ||
| ShowEPOServiceSetupLbl: Label 'Show EPO Service Setup'; | ||
|
|
||
| var |
There was a problem hiding this comment.
src/Apps/CZ/CoreLocalizationPack/app/Src/Codeunits/EPOAPIMgtCZL.Codeunit.al contains a second, empty var block immediately before TryGetFormUrl(). Remove the redundant block so the object has a single meaningful declaration section; leaving an empty declaration block adds noise and makes the code look accidentally incomplete.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.26.4
| ServiceConnection.InsertServiceConnection( | ||
| ServiceConnection, EPOServiceSetupRecordRef.RecordId, EPOServiceSetupCZL.TableCaption(), EPOServiceSetupCZL."Open Form Endpoint", Page::"EPO Service Setup CZL"); | ||
| end; | ||
| } No newline at end of file |
There was a problem hiding this comment.
src/Apps/CZ/CoreLocalizationPack/app/Src/Codeunits/EPOAPIMgtCZL.Codeunit.al is checked in without a trailing newline. Add the final newline so the file follows the usual source-text convention and avoids needless end-of-file diff noise in later edits.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.26.4
|
|
||
| RestClient.Initialize(); | ||
| RestClient.SetTimeOut(Timeout); | ||
| HttpResponseMessage := RestClient.Post(RequestUri, RequestHttpContent); |
There was a problem hiding this comment.
The new EPO API wrapper makes an outbound RestClient.Post call and raises an error on non-success responses, but the failure path emits no Session.LogMessage/Session.LogError telemetry. That leaves production EPO outages and HTTP failures invisible in telemetry; add a stable failure event before raising the error, including the endpoint/result details and an appropriate scope/verbosity.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.26.4
| GetOrInitEPOServiceSetup(); | ||
|
|
||
| HttpContent := HttpContent.Create(Content); | ||
| TrySend(EPOServiceSetupCZL."Open Form Endpoint", EPOServiceSetupCZL."Limit Response Time", HttpContent, ResponseHttpContent); |
There was a problem hiding this comment.
The new EPO POST flow assumes every successful response is XML with a top-level URL node: it parses GetContent().AsXmlDocument() and immediately dereferences SelectSingleNode('URL', ...) without checking the payload shape first. Because the endpoint is now configurable, a misconfigured URL or an upstream HTML/XML error page can fail here with an opaque XML/node error instead of a clear service-response message. Validate the response format and the presence of the URL element before dereferencing it, and raise an explicit error when the endpoint returns an unexpected payload.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.26.4
Agentic PR Review - Round 2Recommendation: Request ChangesWhat this PR doesThis PR moves the Czech EPO endpoint to a configurable setup record and registers it as a Service Connection for AB#644746. The new commits try to default the setup and add consent when the service is enabled, but the first-use path is still not safe: the codeunit creates a blank setup record without running the insert trigger, and the submit flow now ignores a failed try function. That means the default endpoint can still be missing, and a failed submission can continue into archive and hyperlink code. Status of previous suggestions
New observations (commits since round 1)S3 - Extra var section breaks the object S4 - Try function result is ignored Risk assessment and necessityRisk: This area submits Czech VAT reports to an external tax service and archives both the request and response, so wrong error handling or a blank endpoint can affect statutory reporting. The new Service Connection path also runs outside the setup page, so it must not create a half-initialized setup record. Necessity: The gov.cz endpoint migration is a valid and important bug fix, and the linked Bug explains the external URL change. The scope is justified, but the current implementation still needs changes before it is safe to merge.
|
3 participants
What & why
The Czech Financial Administration is migrating from the adisspr.mfcr.cz domain to mojedane.gov.cz. The EPO (Electronic Filing Office) endpoint URL was previously hardcoded in the EPO API Submission CZL codeunit. This change introduces a configurable EPO Service Setup CZL table and page so the endpoint URL can be managed by administrators without a code change. The default URL is updated to the new https://mojedane.gov.cz/dpr/ domain. The EPO service is also registered as a Service Connection for discoverability. A typo in GetHttpResonse() → GetHttpResponse() is fixed as well.
Linked work
Fixes AB#644746
How I validated this
What I tested and the outcome
Opened the new EPO Service Setup page; verified the default URL is set to https://mojedane.gov.cz/dpr/epo_podani?otevriFormular=1 on first insert.
Toggled the Enabled flag on/off; confirmed the endpoint field becomes non-editable when enabled and the warning text is shown.
Used Set URL to Default action to restore the URL after manual edits.
Verified the EPO service appears in the Service Connections page with the correct status and URL.
Submitted a VAT report via EPO and confirmed the form URL is returned correctly from the new endpoint.
No tests added — test coverage for EPO submission will be addressed in a follow-up.
Risk & compatibility
Breaking change: The GetHttpResonse() method was renamed to GetHttpResponse() (typo fix). Since the codeunit has Access = Internal, this does not affect external consumers.
Data migration: On first access the EPO Service Setup CZL record is auto-initialized with the new default URL, so no upgrade codeunit is needed.
URL transition: The Czech Financial Administration will maintain both old and new URLs during a transitional period. Existing installations that upgrade will automatically use the new default URL on first setup record creation.