From a2b3c140229d53c09d6cabf78dda3a8c638f498e Mon Sep 17 00:00:00 2001 From: Gerhard Scheikl Date: Fri, 15 May 2026 15:24:43 +0200 Subject: [PATCH] fix(invoice): detect pickup via missing shippingAddress (real signal for built-in 'Shop location' rate) Reproduced against real dev order #1032: the built-in 'Shop location' shipping rate sets neither a pickup keyword nor deliveryCategory: shippingLine: { title: 'Shop location', code: 'Shop location', source: 'shopify', deliveryCategory: null } shippingAddress: null requiresShipping: true So neither v2 (string regex on title/code) nor v3 (deliveryCategory) caught it. The robust signal is 'requiresShipping && shippingAddress == null': Shopify rejects checkout for a normal shipping order without an address, so this combination is conclusive proof of pickup. - Query Order.requiresShipping (only needs read_orders). - detectPickup() now treats missing-address-but-requires-shipping as the primary signal; deliveryCategory + title/code regex remain as fallbacks for Local-Pickup-app installs and custom rates. - New fixture buildShopLocationPickupOrder() in render-sample.ts mirrors order #1032 exactly so we never regress on this shape. --- app/services/invoice/composeInvoice.ts | 54 ++++++++++--------- .../invoice/loadDraftOrderForOffer.server.ts | 1 + .../invoice/loadOrderForInvoice.server.ts | 8 +++ scripts/render-sample.ts | 38 +++++++++++++ 4 files changed, 75 insertions(+), 26 deletions(-) diff --git a/app/services/invoice/composeInvoice.ts b/app/services/invoice/composeInvoice.ts index 9983f71..f03c95e 100644 --- a/app/services/invoice/composeInvoice.ts +++ b/app/services/invoice/composeInvoice.ts @@ -487,50 +487,52 @@ function mapTracking(order: RawOrderForInvoice): TrackingInfo[] { } /** - * Detects whether the order is a "local pickup" order using two signals: + * Detects whether the order is a "local pickup" order using three signals + * (any one is enough). All rely only on the `read_orders` scope. * - * 1. Primary: `shippingLine.deliveryCategory` (e.g. `"pickup"`, - * `"local_pickup"`). This is what Shopify's Local Pickup app and any - * properly-categorised custom pickup rate set, and only requires the - * `read_orders` scope. - * 2. Fallback: regex on `shippingLine.{source,code,title,carrierIdentifier}` - * for merchants who model pickup as a custom shipping rate without a - * pickup category (e.g. titled "Abholung im Lager"). + * 1. **No shipping address** despite `requiresShipping == true`. Shopify + * never lets a regular ship-to-customer order check out without one, + * so this combination is a textbook pickup. This is the *only* signal + * for the built-in "Shop location" rate, which leaves + * `deliveryCategory` null and the title/code as the bare location + * name (e.g. "Shop location" / "Lager Graz"). + * 2. `shippingLine.deliveryCategory` contains "pickup"/"local_pickup". + * Set by some Local Pickup integrations. + * 3. Regex on `shippingLine.{source,code,title,carrierIdentifier}` for + * custom rates titled "Abholung"/"Pickup". * * (We deliberately do NOT query `Order.fulfillmentOrders.deliveryMethod`: * that field requires the `read_merchant_managed_fulfillment_orders` scope, * which would force every install to re-grant permissions.) * - * When pickup is detected, the location name is taken from - * `shippingLine.title` — for Shopify Local Pickup the title IS the chosen - * location name (e.g. "Lager Graz"). + * Location name is taken from `shippingLine.title` — for the Shopify + * Local Pickup app and the built-in "Shop location" rate, the title IS + * the chosen location name. * * Returns the pickup descriptor or `null` when the order is a normal - * shipping order. Callers should not render the pickup-location address as - * a separate "delivery address". + * shipping order. Callers should not render the pickup-location address + * as a separate "delivery address". */ function detectPickup( order: RawOrderForInvoice, ): { locationName: string | null } | null { const sl = order.shippingLine; - if (!sl) return null; - // Primary signal: shippingLine.deliveryCategory is "pickup" / "local_pickup" - // for any pickup-like fulfillment (set by Shopify's Local Pickup app and by - // custom apps that use the proper category). Doesn't require any extra scope. - const dc = (sl.deliveryCategory ?? "").toLowerCase(); + // Strongest signal: shipping is required but there's no shipping address. + // Shopify rejects checkout otherwise, so this is conclusive. + const noShipAddrButRequired = + order.requiresShipping && order.shippingAddress == null; + // Secondary: explicit pickup category from Local Pickup apps. + const dc = (sl?.deliveryCategory ?? "").toLowerCase(); const isPickupCategory = dc.includes("pickup") || dc.includes("pick_up") || dc.includes("pick-up"); - // Fallback: string heuristic on title/code/source/carrier — covers - // merchants who model pickup as a custom shipping rate without category. - const haystack = [sl.source, sl.code, sl.title, sl.carrierIdentifier] + // Tertiary: regex on title/code/source/carrier — covers merchants who + // model pickup as a custom shipping rate. + const haystack = [sl?.source, sl?.code, sl?.title, sl?.carrierIdentifier] .filter(Boolean) .join(" ") .toLowerCase(); const isPickupString = /pick[\s-]?up|abholung|abhol\b/.test(haystack); - if (!isPickupCategory && !isPickupString) return null; - // For Shopify Local Pickup, `title` is the location name itself - // (e.g. "Lager Graz"). For custom-rate pickup, it's a generic description - // — still better than nothing as a hint. - return { locationName: sl.title?.trim() || null }; + if (!noShipAddrButRequired && !isPickupCategory && !isPickupString) return null; + return { locationName: sl?.title?.trim() || null }; } /** diff --git a/app/services/invoice/loadDraftOrderForOffer.server.ts b/app/services/invoice/loadDraftOrderForOffer.server.ts index e01f00f..4e9e75c 100644 --- a/app/services/invoice/loadDraftOrderForOffer.server.ts +++ b/app/services/invoice/loadDraftOrderForOffer.server.ts @@ -161,6 +161,7 @@ export async function loadDraftOrderForOffer( currencyCode: draft.currencyCode, displayFinancialStatus: null, paymentGatewayNames: [], + requiresShipping: false, shippingLine: null, fulfillments: [], discountCodes: [], diff --git a/app/services/invoice/loadOrderForInvoice.server.ts b/app/services/invoice/loadOrderForInvoice.server.ts index 6dbb4c6..7591966 100644 --- a/app/services/invoice/loadOrderForInvoice.server.ts +++ b/app/services/invoice/loadOrderForInvoice.server.ts @@ -13,6 +13,11 @@ export interface RawOrderForInvoice { currencyCode: string; displayFinancialStatus: string | null; paymentGatewayNames: string[]; + /** True when the order contains at least one shippable line item. For + * pickup orders this is `true` but `shippingAddress` is `null` — that + * combination is the most reliable pickup signal we have without + * hitting `read_merchant_managed_fulfillment_orders`. */ + requiresShipping: boolean; customer: { firstName: string | null; lastName: string | null; @@ -118,6 +123,7 @@ const QUERY = `#graphql displayFinancialStatus paymentGatewayNames taxesIncluded + requiresShipping customer { firstName lastName @@ -229,6 +235,7 @@ interface RawAdminResponse { displayFinancialStatus: string | null; paymentGatewayNames: string[] | null; taxesIncluded: boolean; + requiresShipping: boolean | null; customer: { firstName: string | null; lastName: string | null; @@ -286,6 +293,7 @@ export async function loadOrderForInvoice( displayFinancialStatus: order.displayFinancialStatus, paymentGatewayNames: order.paymentGatewayNames ?? [], taxesIncluded: order.taxesIncluded, + requiresShipping: order.requiresShipping ?? false, customer: order.customer, billingAddress: order.billingAddress, shippingAddress: order.shippingAddress, diff --git a/scripts/render-sample.ts b/scripts/render-sample.ts index f59df52..7f502a4 100644 --- a/scripts/render-sample.ts +++ b/scripts/render-sample.ts @@ -136,6 +136,7 @@ function buildAtB2BOrder(): RawOrderForInvoice { displayFinancialStatus: "PENDING", paymentGatewayNames: ["manual"], taxesIncluded: false, + requiresShipping: true, discountCodes: [], customer: { firstName: "Lukas", @@ -329,6 +330,28 @@ function buildCategoryOnlyPickupOrder(): RawOrderForInvoice { return o; } +/** Pickup variant matching a REAL observed order on + * linumiq-dev.myshopify.com (#1032): Shopify's built-in "Shop location" + * rate. NO "pickup" string anywhere, deliveryCategory is `null`, and + * shippingAddress is also `null` — detection must rely on + * `requiresShipping && shippingAddress == null`. */ +function buildShopLocationPickupOrder(): RawOrderForInvoice { + const o = buildAtB2BOrder(); + o.shippingLine = { + title: "Shop location", + code: "Shop location", + source: "shopify", + carrierIdentifier: null, + deliveryCategory: null, + originalPriceSet: { shopMoney: { amount: "0.00", currencyCode: "EUR" } }, + discountedPriceSet: { shopMoney: { amount: "0.00", currencyCode: "EUR" } }, + taxLines: [], + }; + o.shippingAddress = null; + o.requiresShipping = true; + return o; +} + // ------------------------------------------------------------------ // Run assertions // ------------------------------------------------------------------ @@ -655,6 +678,21 @@ async function main() { assert("shippingMethod cleared in category-only pickup", categoryPickupVm.shippingMethod == null); + // Real-world "Shop location" pickup (matches dev order #1032): no + // "pickup" keyword anywhere, deliveryCategory null, shippingAddress null. + // The only signal is `requiresShipping && !shippingAddress`. + const shopLocPickupVm = composeInvoice({ + order: buildShopLocationPickupOrder(), + settings: settings as never, + invoiceNumber: "RE-1034", + }); + assert("isPickup detected from missing shippingAddress (Shop location rate)", + shopLocPickupVm.isPickup); + assertEq("pickupLocationName from shippingLine.title for Shop location", + shopLocPickupVm.pickupLocationName, "Shop location"); + assert("shippingMethod cleared for Shop location pickup", + shopLocPickupVm.shippingMethod == null); + // Fallback: when footerNoteEn is empty, English uses the German note. console.log("• Footer note fallback (en → de when EN empty)"); const settingsNoEn = { ...(settings as object), footerNoteEn: "" } as never;