bugs on variant id

repositories/catalogueRepository.go

@@ -229,6 +229,7 @@ type CatalogueRepository interface {
 	GetProducts(brand, category, keyword string, pageno, pagesize int) ([]models.CatalogueProduct, int64, error)
 	GetProductBySKU(brand, sku string) (*models.CatalogueProduct, error)
 	GetProductByID(brand string, id int64) (*models.CatalogueProduct, error)
+	GetProductByImageID(brand, imageID string) (*models.CatalogueProduct, error)
 }
 
 type catalogueRepository struct {
@@ -615,6 +616,47 @@ func (r *catalogueRepository) GetProductBySKU(brand, sku string) (*models.Catalo
 	return &product, nil
 }
 
+// GetProductByImageID resolves a catalogue row by the id the ingest pipeline
+// calls canonical.
+//
+// The pipeline reports what it wrote as a manifest of `image_id` values, and
+// its own note is blunt about why: image_id is "the primary key every other
+// product is deduplicated on", and a product name differing by one character
+// is a different product. Importing into a shop needs `catalogueid` — the row
+// id — so without this the console would have to match the manifest on NAME,
+// which silently creates duplicates instead of updating.
+//
+// Alongside GetProductBySKU rather than replacing it: a sheet may leave the sku
+// column blank, in which case the pipeline mints one and the sku is not a key
+// the sender recognises. image_id is derived from brand, name and size and is
+// stable across re-uploads.
+func (r *catalogueRepository) GetProductByImageID(brand, imageID string) (*models.CatalogueProduct, error) {
+	if r.db == nil {
+		return nil, ErrCatalogueDBUnavailable
+	}
+
+	table, err := r.tableForBrand(brand)
+	if err != nil {
+		return nil, err
+	}
+
+	var row catalogueProductRow
+	query := fmt.Sprintf(
+		`SELECT %s FROM %s WHERE image_id = ? LIMIT 1`,
+		columnsForTable(table), table,
+	)
+	result := r.db.Raw(query, strings.TrimSpace(imageID)).Scan(&row)
+	if result.Error != nil {
+		return nil, result.Error
+	}
+	if result.RowsAffected == 0 {
+		return nil, nil
+	}
+
+	product := row.toModel(strings.ToLower(brand))
+	return &product, nil
+}
+
 func (r *catalogueRepository) GetProductByID(brand string, id int64) (*models.CatalogueProduct, error) {
 	if r.db == nil {
 		return nil, ErrCatalogueDBUnavailable

repositories/productRepository.go

@@ -51,6 +51,7 @@ type ProductRepository interface {
 	GetTenantCategories(tenantid int) ([]models.TenantCategory, error)
 	UpdateProductPricing(productid int, retailprice, productcost, taxpercent float64) error
 	UpdateProductCategory(productid, categoryid, subcategoryid int) error
+	UpdateProductVariant(productid, variantid int) error
 }
 
 type productRepository struct {
@@ -945,7 +946,26 @@ func (r *productRepository) GetProductByVariant(tenantid, variantid, locationid,
 	case productid > 0:
 		q = q.Where("p.tenantid = ? AND p.productid = ?", tenantid, productid)
 	default:
-		q = q.Where("p.tenantid = ? AND p.variants = ?", tenantid, variantid)
+		// Neither a group nor a product was named, so there is nothing to
+		// return — and returning nothing is the point.
+		//
+		// This branch used to run `WHERE p.variants = 0`, which is not "no
+		// match": EVERY ungrouped product carries variants 0, so asking for
+		// variant 0 handed back the tenant's entire ungrouped catalogue as if
+		// those products were variants of one another. Measured on tenant 1135:
+		// six unrelated products — a chocolate bar, a chewing gum and an apple —
+		// returned as each other's variants.
+		//
+		// That is what stops an order being placed. The app sends the tapped
+		// product's `variants` value, which is 0 for almost every product, and
+		// receives six things it must choose between. There is no correct choice
+		// to make, so the screen cannot proceed.
+		//
+		// An empty result is the honest answer to a question that named nothing.
+		// The controller rejects this case outright with a message naming
+		// `productid`; this is the second line of defence, so no future caller
+		// can reach the match-everything behaviour by another route.
+		return data, nil
 	}
 
 	err := q.
@@ -1261,6 +1281,37 @@ func (r *productRepository) UpdateProductCategory(productid, categoryid, subcate
 	return r.db.Table("products").Where("productid = ?", productid).Updates(updates).Error
 }
 
+// UpdateProductVariant puts an existing product into a variant group, or takes
+// it out of one.
+//
+// It exists because nothing else could. `products.variants` is the grouping the
+// ordering screen reads — it is what makes three pack sizes one choice rather
+// than three unrelated products — and until now it could only ever be set at
+// CREATE time, by a caller that already knew the group id:
+//
+//   products/create           writes whatever the body carries, variants included
+//   importcatalogueproduct    never sets it, so every imported product is 0
+//   UpdateProduct             writes productlocations.status only, despite the name
+//
+// Since importing from the catalogue is how products actually arrive, every
+// product this console creates is ungrouped and there was no call that could
+// change that. Groups could be created (createproductvariant) and never used.
+//
+// Zero is allowed here, unlike UpdateProductCategory: ungrouping a product is a
+// real thing to want, and 0 is what ungrouped means. The guard that matters for
+// this column lives in the read path, which refuses to treat 0 as a group.
+func (r *productRepository) UpdateProductVariant(productid, variantid int) error {
+	if productid <= 0 {
+		return fmt.Errorf("productid is required")
+	}
+	if variantid < 0 {
+		variantid = 0
+	}
+	return r.db.Table("products").
+		Where("productid = ?", productid).
+		Update("variants", variantid).Error
+}
+
 func (r *productRepository) UpdateProductPricing(productid int, retailprice, productcost, taxpercent float64) error {
 	return r.db.Table("products").
 		Where("productid = ?", productid).