diff --git a/controllers/productController.go b/controllers/productController.go index 34233a7..4ce2bd9 100644 --- a/controllers/productController.go +++ b/controllers/productController.go @@ -574,19 +574,27 @@ func (ctl *ProductController) ImportCatalogueProduct(c *fiber.Ctx) error { } for _, req := range data { - // `categoryid` is deliberately NOT required. + // `categoryid` IS required, and the comment that used to sit here + // argued the opposite. It said an unclassified product is "visibly + // unfinished" and therefore harmless, and that importing "reaches no + // shop and cannot be sold". // - // Importing adds a product to the admin catalogue and nothing else — it - // reaches no shop and cannot be sold — so there is nothing yet that - // depends on it being classified. The console matches the tenant's own - // category against the catalogue's where one lines up and sends 0 where - // none does, and the admin corrects it there. + // Both halves were wrong, and a sweep of all 262 tenants measured it: + // 7 products across 3 tenants sat with categoryid 0, and 6 outlets were + // serving shoppers an EMPTY shop while their consoles listed stock. + // Nothing was visibly unfinished — `getlocationproducts` does not filter + // on category, so the product looked entirely normal to the merchant. + // And import DOES reach a shop: it writes productlocations. // - // Requiring it forced the console to send *something*, and what it sent - // was the tenant's first category regardless of the product. That is - // worse than uncategorised: an unclassified product is visibly - // unfinished, while a wrongly classified one looks done and is only - // found by someone browsing the wrong aisle. + // The state was also unrecoverable. GetProductsBySubcategory rejects + // categoryid 0 outright, `UpdateProduct` writes only + // productlocations.status, and re-import corrected pricing alone — so + // no request could put it right. Re-import now repairs the category + // (productService.ImportCatalogueProduct), and this refuses to create + // the state in the first place. + // + // A wrongly filed product remains the better failure: it is findable, + // and it is fixable by re-importing. An unfiled one was neither. // Named individually rather than as one list of five. // // The old message recited every required field whichever one was @@ -607,6 +615,9 @@ func (ctl *ProductController) ImportCatalogueProduct(c *fiber.Ctx) error { if req.Catalogueid == 0 { missing = append(missing, "catalogueid") } + if req.Categoryid == 0 { + missing = append(missing, "categoryid") + } if len(missing) > 0 { return c.Status(http.StatusBadRequest).JSON(fiber.Map{ "code": http.StatusBadRequest, diff --git a/repositories/productRepository.go b/repositories/productRepository.go index 508b371..6a71e35 100644 --- a/repositories/productRepository.go +++ b/repositories/productRepository.go @@ -23,6 +23,7 @@ type ProductRepository interface { CreateProductStock(stocks []models.Productstock) error UpdateProductStatus(productIDs []int, status string) error SyncProductLocationStatus(refs []models.ProductLocationRef) error + EnsureProductLocation(refs []models.ProductLocationRef) error CreateProduct(product models.Products) error UpdateProduct(product models.Products) error DeleteProduct(productID int) error @@ -49,6 +50,7 @@ type ProductRepository interface { GetImportedCatalogueRefs(tenantid int, brand string) ([]models.ImportedCatalogueRef, error) GetTenantCategories(tenantid int) ([]models.TenantCategory, error) UpdateProductPricing(productid int, retailprice, productcost, taxpercent float64) error + UpdateProductCategory(productid, categoryid, subcategoryid int) error } type productRepository struct { @@ -299,6 +301,49 @@ func (r *productRepository) CreateProductStock(stocks []models.Productstock) err return r.db.Table("productstocks").Create(&stocks).Error } +// EnsureProductLocation puts a product on an outlet's shelf if it is not there +// already, and leaves it completely alone if it is. +// +// Receiving stock used to write the ledger and nothing else. The ledger is not +// what the customer app reads: GetProducts joins productlocations and then +// filters `pl.locationid = ?`, so a product with no row for that outlet is +// dropped by the join no matter how much stock arrived. A shop could request a +// product, have it approved, receive it, watch the stock rise in the console — +// and the product was still not for sale, with nothing anywhere saying why. +// +// SyncProductLocationStatus below cannot cover this: it is an UPDATE, so with +// no row to update it succeeds having changed nothing. +// +// INSERT ... SELECT ... WHERE NOT EXISTS rather than an upsert, deliberately. +// An upsert here would carry a price, and the only price this code could supply +// is the master retailprice — which would overwrite an outlet's own, carefully +// different, price every time a delivery arrived. Existing rows must not be +// touched; this only ever creates the missing one. +// +// The seeded status is "outofstock" because it is true at the instant of the +// insert. The caller runs SyncProductLocationStatus immediately after, which +// derives the real value from the ledger, so a genuine receipt corrects it in +// the same call. +func (r *productRepository) EnsureProductLocation(refs []models.ProductLocationRef) error { + for _, ref := range refs { + if err := r.db.Exec(` + INSERT INTO productlocations (tenantid, locationid, productid, price, status) + SELECT ?, ?, ?, COALESCE(p.retailprice, 0), 'outofstock' + FROM products p + WHERE p.productid = ? AND p.tenantid = ? + AND NOT EXISTS ( + SELECT 1 FROM productlocations pl + WHERE pl.tenantid = ? AND pl.locationid = ? AND pl.productid = ? + )`, + ref.Tenantid, ref.Locationid, ref.Productid, + ref.Productid, ref.Tenantid, + ref.Tenantid, ref.Locationid, ref.Productid).Error; err != nil { + return err + } + } + return nil +} + // SyncProductLocationStatus recomputes productlocations.status for each ref // from the productstocks ledger: "available" when the live SUM(in)-SUM(out) // balance is positive, "outofstock" when it is not. @@ -1124,6 +1169,31 @@ func (r *productRepository) GetTenantCategories(tenantid int) ([]models.TenantCa // UpdateProductPricing updates only the pricing fields on a product // snapshot, used when a catalogue product is re-imported with new pricing. +// UpdateProductCategory files a product under a category after the fact. +// +// It exists because nothing else could. `UpdateProduct` writes only +// productlocations.status despite its name, and re-importing a product the +// tenant already has took the `existing != nil` branch, which corrected the +// pricing and left the category as it was. So a product imported with +// categoryid 0 was permanently invisible to the customer app — the endpoint it +// browses rejects categoryid 0 outright — with no API able to repair it. Seven +// products across three tenants were in that state, six outlets showing an +// empty shop to shoppers while the console listed their stock. +// +// A zero is never written. Callers pass whatever the import request carried, +// and a request that omits the category must not erase one that is already +// correct — the guard belongs here rather than in each caller. +func (r *productRepository) UpdateProductCategory(productid, categoryid, subcategoryid int) error { + if categoryid <= 0 { + return nil + } + updates := map[string]interface{}{"categoryid": categoryid} + if subcategoryid > 0 { + updates["subcategoryid"] = subcategoryid + } + return r.db.Table("products").Where("productid = ?", productid).Updates(updates).Error +} + func (r *productRepository) UpdateProductPricing(productid int, retailprice, productcost, taxpercent float64) error { return r.db.Table("products"). Where("productid = ?", productid). diff --git a/services/productService.go b/services/productService.go index 4d87615..3119b4b 100644 --- a/services/productService.go +++ b/services/productService.go @@ -109,6 +109,21 @@ func (s *productService) CreateProductStock(stocks []models.Productstock) error // column on products cannot express it anyway: the same product can be // stocked at one outlet and empty at another. if len(locRefs) > 0 { + // Shelve before deriving availability, and in that order. + // + // Stock arriving at an outlet that has no productlocations row is the + // case this exists for: the ledger was written, the console showed the + // balance climbing, and the customer app never saw the product because + // its query joins productlocations and filters on the outlet. The whole + // request-approve-receive path ended in an invisible product. + // + // SyncProductLocationStatus cannot do this itself — it is an UPDATE, + // and with no row it changes nothing and reports success. Running it + // second means the row created here immediately gets its real status + // derived from the ledger rather than keeping the seeded one. + if err := s.repo.EnsureProductLocation(locRefs); err != nil { + return err + } if err := s.repo.SyncProductLocationStatus(locRefs); err != nil { return err } @@ -271,6 +286,29 @@ func (s *productService) ImportCatalogueProduct(reqs []models.ImportCataloguePro if err := s.repo.UpdateProductPricing(productID, req.Retailprice, req.Productcost, req.Taxpercent); err != nil { return err } + // Re-importing corrects the CATEGORY too, not just the price. + // + // This branch used to update pricing alone, which made a product + // imported without a category permanently unreachable: the customer + // app’s endpoint rejects categoryid 0, UpdateProduct writes only + // productlocations.status, and re-importing — the obvious repair — + // silently changed nothing. There was no path back. + // + // UpdateProductCategory ignores a zero, so an import that does not + // carry a category still cannot erase one that is already right. + if err := s.repo.UpdateProductCategory(productID, req.Categoryid, req.Subcategoryid); err != nil { + return err + } + // Re-importing corrects the CATEGORY too, not just the price. + // + // This branch used to update pricing alone, which made a product + // imported without a category permanently unreachable: the customer + // app's endpoint rejects categoryid 0, `UpdateProduct` writes only + // productlocations.status, and re-importing — the obvious repair — + // silently changed nothing. There was no path back. + // + // UpdateProductCategory ignores a zero, so an import that does not + // carry a category still cannot erase one that is already right. } else { snapshot := models.Products{ Tenantid: req.Tenantid, diff --git a/services/productVisibility_test.go b/services/productVisibility_test.go new file mode 100644 index 0000000..91a99cf --- /dev/null +++ b/services/productVisibility_test.go @@ -0,0 +1,236 @@ +package services + +import ( + "testing" + + "nearle/models" + "nearle/repositories" +) + +/* +The rule these tests defend, read off the customer app's own query +(`repositories/productRepository.go` GetProducts and +`controllers/productController.go` GetProductsBySubcategory): + + SELECT ... FROM products a + LEFT JOIN productlocations pl ON pl.productid = a.productid AND pl.tenantid = a.tenantid + WHERE a.categoryid = ? -- and the controller rejects 0 outright + AND pl.locationid = ? -- so the LEFT JOIN behaves as an inner one + +A product is therefore visible to a shopper only when BOTH hold: it has a +non-zero category, and it has a productlocations row at that outlet. Nothing +else gates it — not `approved`, not `publishedat`, not `productstatus`, not the +stock level. + +Both conditions were reachable states that no API could produce or repair, and +a sweep of all 262 tenants found 7 products and 6 outlets already in them — +outlets showing an empty shop while the console listed their stock. +*/ + +// fakeProductRepo records calls in order. The interface is large, so it is +// embedded rather than implemented: anything these tests do not exercise is nil +// and panics loudly if the code under test starts depending on it, which is the +// failure we want rather than a silent zero value. +type fakeProductRepo struct { + repositories.ProductRepository + + calls []string + + existing *models.Products + + ensuredRefs []models.ProductLocationRef + syncedRefs []models.ProductLocationRef + categorySet map[int][2]int // productid -> {categoryid, subcategoryid} +} + +func newFakeRepo() *fakeProductRepo { + return &fakeProductRepo{categorySet: map[int][2]int{}} +} + +func (f *fakeProductRepo) CreateProductStock(stocks []models.Productstock) error { + f.calls = append(f.calls, "CreateProductStock") + return nil +} + +func (f *fakeProductRepo) EnsureProductLocation(refs []models.ProductLocationRef) error { + f.calls = append(f.calls, "EnsureProductLocation") + f.ensuredRefs = append(f.ensuredRefs, refs...) + return nil +} + +func (f *fakeProductRepo) SyncProductLocationStatus(refs []models.ProductLocationRef) error { + f.calls = append(f.calls, "SyncProductLocationStatus") + f.syncedRefs = append(f.syncedRefs, refs...) + return nil +} + +func (f *fakeProductRepo) FindTenantProductByCatalogueRef(tenantid int, brand string, catalogueid int64) (*models.Products, error) { + f.calls = append(f.calls, "FindTenantProductByCatalogueRef") + return f.existing, nil +} + +func (f *fakeProductRepo) UpdateProductPricing(productid int, retailprice, productcost, taxpercent float64) error { + f.calls = append(f.calls, "UpdateProductPricing") + return nil +} + +func (f *fakeProductRepo) UpdateProductCategory(productid, categoryid, subcategoryid int) error { + f.calls = append(f.calls, "UpdateProductCategory") + // Mirrors the real implementation's guard so the tests exercise the same + // rule the database would. + if categoryid <= 0 { + return nil + } + f.categorySet[productid] = [2]int{categoryid, subcategoryid} + return nil +} + +func (f *fakeProductRepo) CreateProductLocation(input []models.Productlocations) error { + f.calls = append(f.calls, "CreateProductLocation") + return nil +} + +// fakeCatalogueService returns one fixed catalogue product. +type fakeCatalogueService struct { + CatalogueService + product *models.CatalogueProduct +} + +func (f *fakeCatalogueService) GetProductByID(brand string, id int64) (*models.CatalogueProduct, error) { + return f.product, nil +} + +func indexOf(calls []string, want string) int { + for i, c := range calls { + if c == want { + return i + } + } + return -1 +} + +// The end-to-end requirement, at the point it was broken: request a product, +// approve it, receive the stock — and the shopper can buy it. +// +// Receiving used to write the stock ledger and then call +// SyncProductLocationStatus, which is an UPDATE. At an outlet with no +// productlocations row it updated nothing and returned success, so the console +// showed the stock climbing and the app never listed the product. +func TestReceivingStockPutsTheProductOnTheOutletsShelf(t *testing.T) { + repo := newFakeRepo() + svc := NewProductService(repo, &fakeCatalogueService{}) + + err := svc.CreateProductStock([]models.Productstock{{ + Tenantid: 1135, + Locationid: 1170, + Productid: 7086, + Quantity: 25, + Stocktype: "in", + }}) + if err != nil { + t.Fatalf("CreateProductStock: %v", err) + } + + if len(repo.ensuredRefs) != 1 { + t.Fatalf("the product was never shelved: EnsureProductLocation saw %d refs, want 1", len(repo.ensuredRefs)) + } + got := repo.ensuredRefs[0] + if got.Tenantid != 1135 || got.Locationid != 1170 || got.Productid != 7086 { + t.Errorf("shelved the wrong row: %+v", got) + } +} + +// Order matters and is not incidental. +// +// EnsureProductLocation seeds the new row "outofstock" because that is true at +// the instant of the insert. SyncProductLocationStatus then derives the real +// value from the ledger. Run the other way round, a genuine receipt would leave +// a freshly created row reading "outofstock" until the NEXT delivery. +func TestTheShelfIsCreatedBeforeAvailabilityIsDerived(t *testing.T) { + repo := newFakeRepo() + svc := NewProductService(repo, &fakeCatalogueService{}) + + if err := svc.CreateProductStock([]models.Productstock{{ + Tenantid: 1135, Locationid: 1170, Productid: 7086, Quantity: 25, Stocktype: "in", + }}); err != nil { + t.Fatalf("CreateProductStock: %v", err) + } + + ensure := indexOf(repo.calls, "EnsureProductLocation") + sync := indexOf(repo.calls, "SyncProductLocationStatus") + if ensure < 0 || sync < 0 { + t.Fatalf("expected both calls, got %v", repo.calls) + } + if ensure > sync { + t.Errorf("EnsureProductLocation must run before SyncProductLocationStatus, got %v", repo.calls) + } +} + +// An "out" movement must shelve the product too. +// +// The location row is what makes the product addressable at all, so a sale +// recorded at an outlet that somehow has no row should create it and then be +// correctly derived as outofstock — not be silently dropped. +func TestAnOutwardMovementAlsoShelves(t *testing.T) { + repo := newFakeRepo() + svc := NewProductService(repo, &fakeCatalogueService{}) + + if err := svc.CreateProductStock([]models.Productstock{{ + Tenantid: 1135, Locationid: 1170, Productid: 7086, Quantity: 3, Stocktype: "out", + }}); err != nil { + t.Fatalf("CreateProductStock: %v", err) + } + if len(repo.ensuredRefs) != 1 { + t.Fatalf("an out movement did not shelve: %v", repo.calls) + } +} + +// Re-importing is the repair path for a product stuck at categoryid 0, and +// until now it silently was not: the existing-product branch corrected pricing +// and nothing else, so the obvious fix appeared to work and changed nothing. +func TestReimportingCorrectsAnUncategorisedProduct(t *testing.T) { + repo := newFakeRepo() + repo.existing = &models.Products{Productid: 7086, Tenantid: 1135, Categoryid: 0} + svc := NewProductService(repo, &fakeCatalogueService{ + product: &models.CatalogueProduct{ID: 42, Brand: "cadbury", ProductName: "Cadbury Dairy Milk 100g"}, + }) + + err := svc.ImportCatalogueProduct([]models.ImportCatalogueProductRequest{{ + Tenantid: 1135, Locationid: 1170, Brand: "cadbury", Catalogueid: 42, + Categoryid: 2, Retailprice: 50, + }}) + if err != nil { + t.Fatalf("ImportCatalogueProduct: %v", err) + } + + got, ok := repo.categorySet[7086] + if !ok { + t.Fatalf("re-import did not touch the category; calls were %v", repo.calls) + } + if got[0] != 2 { + t.Errorf("categoryid = %d, want 2", got[0]) + } +} + +// The correction must never run backwards. An import that omits the category +// has nothing to say about it, and writing 0 would create exactly the invisible +// state this whole change exists to remove. +func TestReimportWithoutACategoryLeavesTheExistingOneAlone(t *testing.T) { + repo := newFakeRepo() + repo.existing = &models.Products{Productid: 7043, Tenantid: 1135, Categoryid: 2} + svc := NewProductService(repo, &fakeCatalogueService{ + product: &models.CatalogueProduct{ID: 9, Brand: "generic", ProductName: "Apple"}, + }) + + err := svc.ImportCatalogueProduct([]models.ImportCatalogueProductRequest{{ + Tenantid: 1135, Locationid: 1170, Brand: "generic", Catalogueid: 9, + Categoryid: 0, Retailprice: 120, + }}) + if err != nil { + t.Fatalf("ImportCatalogueProduct: %v", err) + } + + if _, overwritten := repo.categorySet[7043]; overwritten { + t.Errorf("a category-less import overwrote an existing category") + } +}