feat: enforce SKU validity, use SKU as handle

This commit enforces proper SKU validation, uses the SKU as the Shopify handle, and implements ID-based product updates to allow renaming. It also extracts the IShop interface for TDD.

MEMORY.md

@@ -12,7 +12,8 @@ This project (`product_inventory`) integrates Google Sheets with Shopify. It ser
 1.  **Documentation First**: Before implementing complex features, we update the plan and often the documentation (README/ARCHITECTURE).
 2.  **Safety First**: We use `SafeToAutoRun: false` for commands that deploy or modify external state until verified.
 3.  **Strict Typing**: We use TypeScript. No `any` unless absolutely necessary (and even then, we try to avoid it).
-4.  **Artifact Usage**: We use `task.md`, `implementation_plan.md`, and `walkthrough.md` to track state.
+4.  **TDD**: We follow Test Driven Development (Red/Green/Refactor). Write failing tests before implementing features.
+5.  **Artifact Usage**: We use `task.md`, `implementation_plan.md`, and `walkthrough.md` to track state.
 
 ## Key Technical Decisions
 - **Queue System**: We implemented `onEditQueue.ts` to batch edits. This prevents hitting Shopify API rate limits and Google Apps Script execution limits during rapid manual edits.

docs/ARCHITECTURE.md

@@ -37,6 +37,18 @@ To avoid hitting Shopify API rate limits and Google Apps Script execution time l
     - Reads `pendingEdits`.
     - Filters for edits older than `BATCH_INTERVAL_MS` (30s) to allow for multiple quick edits to the same SKU.
     - Iterates through valid edits and calls `Product.UpdateShopifyProduct`.
+    - **SKU Validation**: Before any action, checks if the SKU is valid (not empty, `?`, or `n`). Aborts if invalid.
+
+### 2. Product Lifecycle Logic
+
+- **Creation**:
+    - Uses the **SKU** as the Shopify **Handle** (URL slug).
+    - Prevents creation if the SKU is a placeholder.
+
+- **Updates**:
+    - Prioritizes **ID-based lookup**.
+    - If the `shopify_id` column is populated, the system trusts this ID to locate the product in Shopify, even if the SKU has changed in the sheet.
+    - As a result, changing a SKU in the sheet and syncing will **rename** the existing product (handle/SKU) rather than creating a duplicate.
 
 ### 2. Shopify Integration (`src/shopifyApi.ts`)
 
@@ -45,7 +57,9 @@ The project uses a hybrid approach for the Shopify Admin API:
 - **REST API**: Used primarily for fetching Orders (legacy support).
 - **GraphQL API**: Used for fetching and updating Products and Inventory.
 
-The `Shop` class handles authentication using credentials stored in the "vars" sheet.
+- **GraphQL API**: Used for fetching and updating Products and Inventory.
+
+The `Shop` class implements the `IShop` interface, handling authentication using credentials stored in the "vars" sheet. The interface decoupling facilitates robust unit testing via `MockShop`.
 
 ### 3. Configuration (`src/config.ts`)
 

src/Product.ts

@@ -14,6 +14,7 @@ import * as shopify from "shopify-admin-api-typings"
 import { Config } from "./config"
 import { ISpreadsheetService } from "./interfaces/ISpreadsheetService"
 import { GASSpreadsheetService } from "./services/GASSpreadsheetService"
+import { IShop } from "./interfaces/IShop"
 
 export class Product {
   shopify_id: string = ""
@@ -87,8 +88,25 @@ export class Product {
     }
   }
 
-  MatchToShopifyProduct(shop: Shop): shopify.Product {
+  MatchToShopifyProduct(shop: IShop): shopify.Product {
-    // TODO: Look for and match based on known gid before SKU lookup
+    // Check if we have a known Shopify ID from the sheet
+    if (this.shopify_id && this.shopify_id !== "") {
+      console.log(`MatchToShopifyProduct: Checking ID '${this.shopify_id}' from sheet...`)
+      let productById = shop.GetProductById(this.shopify_id)
+      if (productById) {
+        console.log(`MatchToShopifyProduct: Found product by ID '${this.shopify_id}'`)
+        this.shopify_product = productById
+        this.shopify_id = this.shopify_product.id.toString()
+        this.shopify_default_variant_id = productById.variants.nodes[0].id
+        // We trust the ID, so we update the instantiated object with current Shopify data
+        // But we DO NOT overwrite the sheet's SKU/Title yet, because we might be pushing updates FROM sheet TO Shopify.
+        return
+      } else {
+        console.log(`MatchToShopifyProduct: Product with ID '${this.shopify_id}' not found. Falling back to SKU lookup.`)
+      }
+    }
+
+    // Fallback to SKU lookup
     let product = shop.GetProductBySku(this.sku)
     if (product == undefined || product.id == undefined || product.id == "") {
       console.log("MatchToShopifyProduct: no product matched")
@@ -140,7 +158,9 @@ export class Product {
       sps.category = this.ShopifyCategory()
     }
     sps.tags = this.tags
+    sps.tags = this.tags
     sps.title = this.title
+    sps.handle = this.sku
     sps.descriptionHtml = this.description
     sps.variants = []
     let variant = new ShopifyVariant()
@@ -159,8 +179,14 @@ export class Product {
     return sps
   }
 
-  UpdateShopifyProduct(shop: Shop) {
+  UpdateShopifyProduct(shop: IShop) {
     console.log("UpdateShopifyProduct()")
+    // SKU Validation
+    if (!this.sku || this.sku === "" || this.sku === "?" || this.sku === "n") {
+        console.log("UpdateShopifyProduct: Invalid or placeholder SKU. Aborting.")
+        return;
+    }
+
     var newProduct = false
     let config = new Config()
     this.MatchToShopifyProduct(shop)
@@ -215,7 +241,7 @@ export class Product {
     this.CreatePhotoFolder();
   }
 
-  UpdateAllMetafields(shop: Shop) {
+  UpdateAllMetafields(shop: IShop) {
     console.log("UpdateAllMetafields()")
     if (!this.shopify_id) {
       console.log("Cannot update metafields without a Shopify Product ID.")
@@ -326,7 +352,7 @@ export class Product {
     createPhotoFolderForSku(new(Config), this.sku, this.sheetService);
   }
 
-  PublishToShopifyOnlineStore(shop: Shop) {
+  PublishToShopifyOnlineStore(shop: IShop) {
     console.log("PublishToShopifyOnlineStore")
     let config = new Config()
     let query = /* GraphQL */ `
@@ -364,7 +390,7 @@ export class Product {
     return shop.shopifyGraphQLAPI(JSON.parse(j))
   }
 
-  PublishShopifyProduct(shop: Shop) {
+  PublishShopifyProduct(shop: IShop) {
     //TODO: update product in sheet
     // TODO: status
     // TODO: shopify_status

src/ProductLogic.test.ts

@@ -0,0 +1,106 @@
+import { Product } from "./Product";
+import { MockSpreadsheetService } from "./services/MockSpreadsheetService";
+import { MockShop } from "./test/MockShop";
+
+describe("Product Logic (TDD)", () => {
+  let mockService: MockSpreadsheetService;
+  let mockShop: MockShop;
+
+  beforeEach(() => {
+    mockService = new MockSpreadsheetService({
+      product_inventory: [
+        ["sku", "title", "price", "shopify_id", "product_type"], // Headers
+        ["TEST-SKU-1", "Test Product", 10.99, "", "Type A"] // Data
+      ],
+      values: [
+          ["product_type", "shopify_category", "ebay_category_id"],
+          ["Type A", "Category A", "123"]
+      ]
+    });
+    mockShop = new MockShop();
+  });
+
+  test("UpdateShopifyProduct should abort if SKU is invalid", () => {
+    // Setup invalid SKU
+    mockService.setSheetData("product_inventory", [
+        ["sku", "title"],
+        ["?", "Invalid Product"]
+    ]);
+    // Allow empty sku to be passed to constructor logic check, but Product constructor throws if sku not found.
+    // So we pass a valid sku that exists in sheet, but looks invalid?
+    // The requirement is "based on the product's title... If I have a placeholder value... ensure products are not created until they have a valid SKU".
+    // In `Product.ts`, constructor takes `sku`.
+    // If I pass `?` and it's in the sheet, it constructs.
+
+    const product = new Product("?", mockService);
+
+    // Attempt update
+    product.UpdateShopifyProduct(mockShop);
+
+    // Verify no calls to creating product
+    // We expect MatchToShopifyProduct might be called (read only), but NOT productSet (writ)
+    // Actually our plan said "abort operation" at start of UpdateShopifyProduct.
+    expect(mockShop.productSetCalledWith).toBeNull();
+  });
+
+  test("ToShopifyProductSet should set handle to SKU", () => {
+    const product = new Product("TEST-SKU-1", mockService);
+    const sps = product.ToShopifyProductSet();
+
+    // We expect sps to have a 'handle' property equal to the sku
+    // This will fail compilation initially as ShopifyProductSetInput doesn't have handle
+    expect((sps as any).handle).toBe("TEST-SKU-1");
+  });
+
+  test("MatchToShopifyProduct should verify ID if present", () => {
+    // Setup data with shopify_id
+    mockService.setSheetData("product_inventory", [
+        ["sku", "shopify_id"],
+        ["TEST-SKU-OLD", "123456789"]
+    ]);
+    const product = new Product("TEST-SKU-OLD", mockService);
+
+    // Mock the response for GetProductById
+    mockShop.mockProductById = {
+        id: "123456789",
+        title: "Old Title",
+        variants: { nodes: [{ id: "gid://shopify/ProductVariant/123456789", sku: "TEST-SKU-OLD" }] },
+        options: [{ id: "opt1", optionValues: [{ id: "optval1" }] }]
+    };
+
+    // We need to call Match, but it's called inside Update usually.
+    // We can call it directly for testing.
+    product.MatchToShopifyProduct(mockShop);
+
+    // Expect GetProductById to have been called
+    expect(mockShop.getProductByIdCalledWith).toBe("123456789");
+    expect(product.shopify_id).toBe("123456789");
+  });
+
+  test("MatchToShopifyProduct should fall back to SKU if ID lookup fails", () => {
+    // Setup data with shopify_id that is invalid
+    mockService.setSheetData("product_inventory", [
+        ["sku", "shopify_id"],
+        ["TEST-SKU-FAIL", "999999999"]
+    ]);
+    const product = new Product("TEST-SKU-FAIL", mockService);
+
+    // Mock ID lookup failure (returns null/undefined)
+    mockShop.mockProductById = null;
+
+    // Mock SKU lookup success
+    mockShop.mockProductBySku = {
+        id: "555555555",
+        title: "Found By SKU",
+        variants: { nodes: [{ id: "gid://shopify/ProductVariant/555555555", sku: "TEST-SKU-FAIL" }] },
+        options: [{ id: "opt2", optionValues: [{ id: "optval2" }] }]
+    };
+
+    product.MatchToShopifyProduct(mockShop);
+
+    expect(mockShop.getProductByIdCalledWith).toBe("999999999");
+    // Should fall back to SKU
+    expect(mockShop.getProductBySkuCalledWith).toBe("TEST-SKU-FAIL");
+    expect(product.shopify_id).toBe("555555555");
+  });
+});

src/interfaces/IShop.ts

@@ -0,0 +1,13 @@
+import * as shopify from "shopify-admin-api-typings";
+import { Config } from "../config";
+
+export interface IShop {
+  GetProductBySku(sku: string): any; // Return type is inferred as product node
+  GetProductById(id: string): any; // New method
+  GetInventoryItemBySku(sku: string): shopify.InventoryItem;
+  UpdateInventoryItemQuantity(item: shopify.InventoryItem, delta: number, config: Config): shopify.InventoryItem;
+  SetInventoryItemQuantity(item: shopify.InventoryItem, quantity: number, config: Config): any;
+  SetInventoryItemDefaults(item: shopify.InventoryItem, config: Config): shopify.InventoryItem;
+  SetInventoryItemWeight(item: shopify.InventoryItem, config: Config, weight: number, weight_unit: shopify.WeightUnit): shopify.InventoryItem;
+  shopifyGraphQLAPI(payload: any): any;
+}

src/shopifyApi.ts

@@ -18,6 +18,7 @@
 import { Config } from "./config"
 import * as shopify from "shopify-admin-api-typings"
 import gql from 'graphql-tag'
+import { IShop } from "./interfaces/IShop"
 
 const ss = SpreadsheetApp.getActive()
 
@@ -392,7 +393,7 @@ function parseLinkHeader(header) {
   return rels
 }
 
-export class Shop {
+export class Shop implements IShop {
   private shopifyApiKey: string
   private shopifyApiSecretKey: string
   private shopifyAdminApiAccessToken: string
@@ -599,6 +600,34 @@ export class Shop {
     return product
   }
 
+  GetProductById(id: string) {
+    console.log("GetProductById('" + id + "')")
+    let gql = /* GraphQL */ `
+      query productById {
+        product(id: "${id}") {
+          id
+          title
+          handle
+          variants(first: 1) {
+            nodes {
+              id
+              sku
+            }
+          }
+        }
+      }
+    `
+    let query = buildGqlQuery(gql, {})
+    let response = this.shopifyGraphQLAPI(query)
+    if (!response.content.data.product) {
+        console.log("GetProductById: no product matched")
+        return null;
+    }
+    let product = response.content.data.product
+    console.log("Product found:\n" + JSON.stringify(product, null, 2))
+    return product
+  }
+
   GetInventoryItemBySku(sku: string) {
     console.log('GetInventoryItemBySku("' + sku + '")')
     let gql = /* GraphQL */ `
@@ -1132,6 +1161,7 @@ export class ShopifyProductSetQuery {
 export class ShopifyProductSetInput {
   category: string
   descriptionHtml: string
+  handle: string
   id?: string
   productType: string
   redirectNewHandle: boolean = true

src/test/MockShop.ts

@@ -0,0 +1,52 @@
+import { IShop } from "../interfaces/IShop";
+import * as shopify from "shopify-admin-api-typings";
+import { Config } from "../config";
+
+export class MockShop implements IShop {
+  // Mock methods to spy on calls
+  public getProductBySkuCalledWith: string | null = null;
+  public getProductByIdCalledWith: string | null = null;
+  public productSetCalledWith: any | null = null;
+  public shopifyGraphQLAPICalledWith: any | null = null;
+
+  public mockProductBySku: any = null;
+  public mockProductById: any = null;
+  public mockResponse: any = {};
+
+  GetProductBySku(sku: string) {
+    this.getProductBySkuCalledWith = sku;
+    return this.mockProductBySku || {};
+  }
+
+  GetProductById(id: string) {
+    this.getProductByIdCalledWith = id;
+    return this.mockProductById;
+  }
+
+  shopifyGraphQLAPI(payload: any): any {
+      this.shopifyGraphQLAPICalledWith = payload;
+      // Basic mock response structure if needed
+      if (payload.query && payload.query.includes("productSet")) {
+         this.productSetCalledWith = payload;
+      }
+      return {
+          content: {
+              data: {
+                  productSet: {
+                      product: { id: "mock-new-id" }
+                  },
+                  products: {
+                    edges: []
+                  }
+              }
+          }
+      };
+  }
+
+  // Stubs for other interface methods
+  GetInventoryItemBySku(sku: string): shopify.InventoryItem { return { id: "mock-inv-id" } as any; }
+  UpdateInventoryItemQuantity(item: shopify.InventoryItem, delta: number, config: Config): shopify.InventoryItem { return {} as any; }
+  SetInventoryItemQuantity(item: shopify.InventoryItem, quantity: number, config: Config): any { return {}; }
+  SetInventoryItemDefaults(item: shopify.InventoryItem, config: Config): shopify.InventoryItem { return {} as any; }
+  SetInventoryItemWeight(item: shopify.InventoryItem, config: Config, weight: number, weight_unit: shopify.WeightUnit): shopify.InventoryItem { return {} as any; }
+}