Shopify Copilot Tax Matching Agent: human-in-the-loop review + tests (stack 3/3) - #10409
Shopify Copilot Tax Matching Agent: human-in-the-loop review + tests (stack 3/3)#10409Onat Buyukakkus (onbuyuka) wants to merge 8 commits into
Conversation
af14356 to
f9241d6
Compare
f9241d6 to
fa460a8
Compare
fa460a8 to
640c7a5
Compare
The merge-base changed after approval.
…ce and tests Adds the review UI on top of the matching engine: the Tax Match Review page and order tax-lines part, the order / sales-order / shop-card page extensions with the review drill-through and Discovered uptake, and the stateless notification codeunit. Grants the review pages in the permission set and adds the Architecture and TestMatrix documentation. Adds the review-specific tests (rate-conflict review flow and the human-in-the- loop notification / undo-approval behaviour) to the test app whose engine suites and unit tests shipped in the previous layer. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86eff15e-fcd8-43b1-9984-5aca2b37e9f7
…faces, review-page tuning - Add the standard MIT copyright header to all new review-UI and review-test .al files, matching the rest of the Shopify connector. - On the base Sales Order page, affix the added control (ShpfyTaxMatchApplied) and brand the caption/action as "Shopify Tax Match Applied" / "Review Shopify Tax Match" so users see the data originates from Shopify. - Narrow the review tax-lines part helper SetTaxLineFilter to internal so it is not a published contract. - SetLoadFields on the review page's tax-line and order-line scans (SnapshotTaxLines, HasPendingEdits, RevertTaxLineEdits, BuildTaxLineFilter). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86eff15e-fcd8-43b1-9984-5aca2b37e9f7
Follows the Shpfy affix rename of the base-table members through the review-UI layer: the Sales Order page control now binds Rec."Shpfy Tax Match Applied", the review page and the HITL/rate-conflict tests reference "Shpfy Created by Agent" and "Shpfy Verified", and Architecture.md/TestMatrix.md field references are updated. Captions are unchanged, so the concept prose still matches the UI. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86eff15e-fcd8-43b1-9984-5aca2b37e9f7
Reflect the review feedback in Architecture.md: the per-tax-line (0000UN0) and per-tax-area (0000UN1) events are now Normal traces rather than Used uptake, and the guardrail-unavailable event (0000UNV) is Error rather than Warning. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86eff15e-fcd8-43b1-9984-5aca2b37e9f7
Reflect the scope changes in Architecture.md: 0000UMG/UMH/UMJ and 0000UMM/UMN/UMO are now ExtensionPublisher-scoped, and 0000UMM logs the HTTP status code as a custom dimension instead of the echoed AOAI error text. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86eff15e-fcd8-43b1-9984-5aca2b37e9f7
Log the 0000UMU "tax review opened" usage event only when the review page actually opens; emit a distinct 0000UN9 event when the drill-through falls back to the raw Shopify order, so the metric no longer counts opens that did not happen. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 86eff15e-fcd8-43b1-9984-5aca2b37e9f7
feaee22 to
0c07abf
Compare
Agentic PR Review - Round 1Recommendation: Accept with SuggestionsWhat this PR doesThis PR adds the Shopify Tax Matching Agent human-review layer: order and sales-order entry points, stateless review notifications, a Tax Match Review card with editable tax-line jurisdiction assignments, a tax-lines list part with BC-vs-Shopify rate guidance, approval/undo actions, permissions, documentation, and HITL/rate-conflict test coverage. SuggestionsS1 - Persist the rebuilt Tax Area when approving edited jurisdictions Risk assessment and necessityRisk: Moderate. The new UI is mostly additive, but it gates sales document creation for held orders and allows humans to change tax jurisdictions. Persisting the approved tax area is important because this page is now the handoff between AI matching and document creation. Necessity: The HITL layer is needed so low-confidence, incomplete, or rate-conflict tax matches can be reviewed before the connector creates the sales document.
|
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 896b49e4-84bf-4541-9997-acb614af2ed6
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 896b49e4-84bf-4541-9997-acb614af2ed6
| FilterBuilder.Append(Format(ParentId)); | ||
| end; | ||
|
|
||
| local procedure ApproveReview() |
There was a problem hiding this comment.
ShpfyTMAReview.Page.al's ApproveReview procedure directly orchestrates multi-step business logic in the page object: it re-applies tax matching (TMAMatcher.ReapplyFromAssignedLines), rebuilds the Tax Area (TaxAreaBuilder.FindOrCreateTaxArea), writes the activity log, flips the reviewed flag, and marks jurisdictions verified, all inline in a page procedure with no codeunit wrapping the sequence. This couples the approval workflow to the UI layer, making it untestable without a page object and harder to reuse from another entry point (e.g. a future bulk-approve action or API).
Recommendation:
- extract the approval sequence into a codeunit procedure that the page calls, keeping the page responsible only for confirmation/UI refresh.
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
| OrderHeader."Tax Exempt" := TaxExempt; | ||
| end; | ||
|
|
||
| local procedure CreateConflictScenario(var OrderHeader: Record "Shpfy Order Header"; Shop: Record "Shpfy Shop"; ShopifyRate: Decimal; ExistingBcRate: Decimal) |
There was a problem hiding this comment.
The new tests build most fixtures by hand with Init/Insert helpers and invented keys (CreateSalesHeader, CreateConflictScenario, CreateShippingScenario, EnsureItem, EnsureJurisdiction, CreateShop) instead of routing setup through the existing Shpfy TMA Test Library or the standard test library codeunits. That bypasses number-series/validation behavior and duplicates prerequisite setup knowledge, so these tests will become brittle as the Shopify or base-app schemas evolve.
Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
| { | ||
| Caption = 'Tax Matching Agent'; | ||
|
|
||
| field("Tax Matching Agent Enabled"; Rec."Tax Matching Agent Enabled") |
There was a problem hiding this comment.
This pageextension adds five new field controls to "Shpfy Shop Card" (owned by the separate "Shopify Connector" app) with unaffixed member names ("Tax Matching Agent Enabled", "Auto Create Tax Jurisdictions", "Auto Create Tax Areas", "Tax Area Naming Pattern", and "Tax Match Review Mode"). AppSource requires extension members added to another app's page to carry the reserved Shpfy affix individually; qualifying only the pageextension object itself is not enough.
Suggested fix (apply manually — could not be anchored as a one-click suggestion):
field(ShpfyTaxMatchingAgentEnabled; Rec."Tax Matching Agent Enabled")
{
ApplicationArea = All;
}
field(ShpfyAutoCreateTaxJurisdictions; Rec."Auto Create Tax Jurisdictions")
{
ApplicationArea = All;
Enabled = Rec."Tax Matching Agent Enabled";
}
field(ShpfyAutoCreateTaxAreas; Rec."Auto Create Tax Areas")
{
ApplicationArea = All;
Enabled = Rec."Tax Matching Agent Enabled";
}
field(ShpfyTaxAreaNamingPattern; Rec."Tax Area Naming Pattern")
{
ApplicationArea = All;
Enabled = Rec."Tax Matching Agent Enabled" and Rec."Auto Create Tax Areas";
}
field(ShpfyTaxMatchReviewMode; Rec."Tax Match Review Mode")
{
ApplicationArea = All;
Enabled = Rec."Tax Matching Agent Enabled";
}Knowledge:
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
| Rec.SetRange("Parent Id", 0); | ||
| end; | ||
|
|
||
| trigger OnAfterGetRecord() |
There was a problem hiding this comment.
ShpfyTMAOrderTaxLinesPart.OnAfterGetRecord resolves line context and BC rate for every rendered row. That path does Order Line or Shipping Charge lookups, Order Header and Shop Gets, and TryGetEffectiveItemRate's Tax Detail read per row, so the review page scales as N+1 database calls with the number of tax lines. Preload or cache the order context and rate data for the page instead of re-querying per row.
Knowledge:
- microsoft/knowledge/performance/avoid-get-inside-loop-on-large-table.md
- microsoft/knowledge/performance/use-setloadfields-for-partial-records.md
👍 useful · ❤️ especially valuable · 👎 wrong - reply with why · AL review agent v1.35.4
Part 3 of 3 of a stacked PR that splits #7453 (AB#445769). Stacks on #10408 — review/merge #10407 then #10408 first.
This layer — human-in-the-loop review experience + review tests
Together, the three layers reproduce #7453 exactly (verified: this branch's tree is byte-for-byte identical to the original PR head).
Stack (bottom-up)
main(Shopify connector: persist order Tax Area Code, Tax Liable, and Tax Exempt (stack 1/3) #10407)