Skip to content

[Shopify] Refresh cached plan before order sync#8212

Open
onbuyuka wants to merge 5 commits into
mainfrom
bugs/635878-shopify-refresh-plan-before-order-sync
Open

[Shopify] Refresh cached plan before order sync#8212
onbuyuka wants to merge 5 commits into
mainfrom
bugs/635878-shopify-refresh-plan-before-order-sync

Conversation

@onbuyuka
Copy link
Copy Markdown
Contributor

@onbuyuka onbuyuka commented May 19, 2026

Summary

Bug 635878: When a merchant downgrades their Shopify plan from Plus / Advanced to a standard plan, the connector keeps requesting the staffMember { id } field in the order GraphQL query. The lower plan does not grant read_users scope, so Shopify returns ACCESS_DENIED and order sync is fully broken until the user manually toggles the Enabled field off and on again on the Shop Card.

Shop."Advanced Shopify Plan" (field 207) is the cached flag that gates the staff member field. It was only refreshed when the user toggled Enabled or when a scope change was detected on Shop Card open — never before an order sync.

Fix

Auto-refresh the cached plan via the existing Shop.GetShopSettings() helper (one cheap GraphQL query for shop { plan { ... } weightUnit }) before both order-import entry points:

  1. Bulk sync — Report 30104 Shpfy Sync Orders from Shopify (also the scheduled auto-sync path). One refresh call per shop per batch run.
  2. Single-order reimportShpfyImportOrder.ReimportExistingOrderConfirmIfConflicting (manual reimport action on the Order Header page).

The refresh is deliberately not placed in ShpfyImportOrder.SetShop() because that runs per order; for a 100-order batch it would mean 100 extra GraphQL round-trips.

Tests

Two regression tests added to the existing ShpfyOrdersAPITest.Codeunit.al (139608):

  • TestGetShopSettingsClearsStaleAdvancedShopifyPlanFlag — sets stale "Advanced Shopify Plan" = true, calls GetShopSettings, asserts the flag becomes false.
  • TestSyncOrdersFromShopifyReportRefreshesAdvancedShopifyPlanFlag — sets stale flag, runs Report 30104, asserts the persisted Shop record has the flag refreshed to false.

Existing OrdersAPIHttpHandler extended with a one-shot PlanRefreshExpected flag returning a downgraded-plan JSON for the first request, then falling through to the prior empty-data behavior.

Both tests verified locally.

Fixes AB#635878

Auto-refresh Shop."Advanced Shopify Plan" via the existing
Shop.GetShopSettings() helper before both order-import entry points so
that a Shopify plan downgrade doesn't leave the connector requesting
the staffMember field that the new plan can no longer grant access to
(ACCESS_DENIED on read_users scope).

Entry points covered:
- Bulk sync: report 30104 "Shpfy Sync Orders from Shopify" (also the
  scheduled auto-sync path). One refresh call per shop per batch run.
- Single-order reimport: ShpfyImportOrder
  .ReimportExistingOrderConfirmIfConflicting.

Tests added to ShpfyOrdersAPITest.Codeunit.al:
- TestGetShopSettingsClearsStaleAdvancedShopifyPlanFlag
- TestSyncOrdersFromShopifyReportRefreshesAdvancedShopifyPlanFlag

Fixes AB#635878

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@onbuyuka onbuyuka requested a review from a team as a code owner May 19, 2026 17:40
@github-actions github-actions Bot added the AL: Apps (W1) Add-on apps for W1 label May 19, 2026
@github-actions github-actions Bot modified the milestone: Version 29.0 May 19, 2026
Comment thread src/Apps/W1/Shopify/Test/Order Handling/ShpfyOrdersAPITest.Codeunit.al Outdated
Address PR review feedback: reset PlanRefreshExpected /
PlanRefreshCallCount at the start of Initialize() so an assertion
failure in one test cannot leak the plan-refresh flag into the next
test and corrupt unrelated HTTP calls.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@onbuyuka onbuyuka enabled auto-merge (squash) May 19, 2026 18:34
@github-actions
Copy link
Copy Markdown
Contributor

$\textbf{🟠\ High\ Severity\ —\ Performance} \quad \color{gray}{\texttt{\small Iteration\ 1}}$

Unhandled API failure can abort entire report

Shop.GetShopSettings() makes a live HTTP call to the Shopify API inside OnAfterGetRecord. If the API is unavailable, rate-limited, or returns an error, the exception propagates unhandled and aborts the entire report run, potentially leaving no orders imported.

Recommendation:

  • Wrap the call in a if not TryGetShopSettings() then guard or use Codeunit.Run with error handling so that a transient API failure degrades gracefully rather than aborting the report.
if not TryGetShopSettings(Shop) then
    // log or skip; continue with existing cached settings
else
    Shop.Modify(false);

Line mapping was unavailable, so this was posted as an issue comment.

👍 useful · ❤️ especially valuable · 👎 wrong - reply with why

…eunit.al

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
onbuyuka and others added 2 commits May 19, 2026 22:47
CI lint fails with AA0214 ("record should be modified before saving")
because the analyzer cannot see through GetShopSettings() to detect
the field assignments it performs on the record. This mirrors the
established suppression pattern used elsewhere in the codebase (e.g.,
ShpfyProductCollectionTest, Subscription Billing tables, EDocument
line matching) for the same false-positive scenario.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Commit 8f0d961 (a suggestion accepted via the GitHub UI) placed a
LibraryAssert call between two procedure bodies, breaking the file
syntax. Moving the intended tightened assertion ("exactly one"
plan-refresh query, matching the GetShopSettings test) into its
correct location inside the report test, replacing the previous
">= 1" check.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AL: Apps (W1) Add-on apps for W1

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant