diff --git a/services/productService.go b/services/productService.go index 3119b4b..aedae61 100644 --- a/services/productService.go +++ b/services/productService.go @@ -197,6 +197,9 @@ func (s *productService) GetProductsBySubcategory(params models.ProductFilter) ( return nil, err } + products = InStockOnly(products, params.LocationID) + + var details []models.SubcategoryProductResponse var uncategorized []models.Products diff --git a/services/productVisibility.go b/services/productVisibility.go new file mode 100644 index 0000000..36a6323 --- /dev/null +++ b/services/productVisibility.go @@ -0,0 +1,53 @@ +package services + +import "nearle/models" + +// InStockOnly drops the products an outlet cannot actually sell. +// +// Being on a shelf is not the same as being in stock. Suriya Store's Peelamedu +// branch listed three products in the customer app of which exactly one had any +// stock, so a shopper could add a chocolate bar to their basket and order +// something the shop did not have. The rule the business wants is the simple +// one: if we have the stock, it can be ordered — otherwise it is not on offer. +// +// ── Why here and not in the SQL ────────────────────────────────────────────── +// +// The obvious implementation is another correlated subquery in GetProducts' +// WHERE clause. This is better for a reason that outlives the convenience: +// `Productstock` is the number the app DISPLAYS, computed by that query as the +// live SUM(in)-SUM(out) balance. Filtering on the same value that is shown makes +// it impossible for the two to disagree. A second copy of the balance +// expression in a WHERE clause is a copy that can drift from the one in the +// SELECT, and the failure it produces — a product listed as "3 in stock" that +// the filter considers empty, or worse the reverse — is very hard to see. +// +// It also filters on the ledger rather than productlocations.status. That flag +// is derived and admits drift: SyncProductLocationStatus repairs it on the next +// ledger entry, so between an order and that entry it can still read +// "available" for an empty shelf. The ledger cannot drift from itself. +// +// ── The locationID guard ───────────────────────────────────────────────────── +// +// Not defensive padding. GetProducts computes the stock balance scoped to +// params.LocationID, and with no outlet the subquery matches nothing, so EVERY +// product comes back with Productstock 0. Filtering that would turn an unscoped +// browse into an empty catalogue rather than an unfiltered one. +// +// The console is deliberately unaffected: its inventory screens read +// GetLocationProducts, a different query, and they must keep showing the empty +// lines — restocking them is the entire point of that screen. +func InStockOnly(products []models.Products, locationID int) []models.Products { + if locationID <= 0 { + return products + } + + // Rebuilt rather than filtered in place: the caller's slice is the + // repository's own result and nothing here should be writing through it. + inStock := make([]models.Products, 0, len(products)) + for _, p := range products { + if p.Productstock > 0 { + inStock = append(inStock, p) + } + } + return inStock +} diff --git a/services/productVisibility_test.go b/services/productVisibility_test.go index 91a99cf..f9a0d3d 100644 --- a/services/productVisibility_test.go +++ b/services/productVisibility_test.go @@ -234,3 +234,122 @@ func TestReimportWithoutACategoryLeavesTheExistingOneAlone(t *testing.T) { t.Errorf("a category-less import overwrote an existing category") } } + +/* +"If we have the stock, we can order it" — the customer app's browse rule. + +The products below are Suriya Store's Peelamedu shelf as it actually stood: +three products listed, one with stock. Before this filter the app offered all +three, so a shopper could order two chocolate bars the shop did not have. +*/ + +func names(products []models.Products) []string { + out := make([]string, 0, len(products)) + for _, p := range products { + out = append(out, p.Productname) + } + return out +} + +var peelamedu = []models.Products{ + {Productid: 7084, Productname: "Cadbury 5 Star 9.8 g", Productstock: 0}, + {Productid: 7085, Productname: "Cadbury 5 Star 18g", Productstock: 0}, + {Productid: 7086, Productname: "Cadbury Dairy Milk 100g", Productstock: 25}, +} + +func TestOnlyProductsWithStockAreOffered(t *testing.T) { + got := InStockOnly(peelamedu, 1170) + + if len(got) != 1 { + t.Fatalf("offered %d products, want 1: %v", len(got), names(got)) + } + if got[0].Productid != 7086 { + t.Errorf("offered %d, want 7086 (the only one in stock)", got[0].Productid) + } +} + +// An outlet with nothing in stock offers nothing — it does not fall back to +// showing the shelf. An empty shop is the honest answer. +func TestAnOutletWithNoStockOffersNothing(t *testing.T) { + empty := []models.Products{ + {Productid: 7084, Productstock: 0}, + {Productid: 7085, Productstock: 0}, + } + if got := InStockOnly(empty, 1172); len(got) != 0 { + t.Errorf("offered %d products from an empty shelf", len(got)) + } +} + +// Without an outlet, GetProducts scopes its balance subquery to locationid 0, +// which matches nothing and returns every product with Productstock 0. Filtering +// there would empty the catalogue instead of leaving it unscoped. +func TestAnUnscopedBrowseIsNotFiltered(t *testing.T) { + if got := InStockOnly(peelamedu, 0); len(got) != 3 { + t.Errorf("unscoped browse returned %d, want all 3 unfiltered", len(got)) + } +} + +// A negative balance is not stock. The ledger can go below zero when an "out" +// exceeds what was received, and `> 0` must not read that as sellable. +func TestANegativeBalanceIsNotStock(t *testing.T) { + oversold := []models.Products{{Productid: 7084, Productstock: -3}} + if got := InStockOnly(oversold, 1170); len(got) != 0 { + t.Errorf("an oversold product was offered for sale") + } +} + +func TestFilteringDoesNotDisturbTheCallersSlice(t *testing.T) { + source := []models.Products{ + {Productid: 1, Productstock: 0}, + {Productid: 2, Productstock: 5}, + } + _ = InStockOnly(source, 1170) + if len(source) != 2 || source[0].Productid != 1 || source[1].Productid != 2 { + t.Errorf("the input slice was modified: %+v", source) + } +} + +func (f *fakeProductRepo) GetSubcategories(categoryID int) ([]models.Subcategory, error) { + return nil, nil +} + +// Returns (nil, nil) so GetProductsBySubcategory falls through to its plain +// {"details": ...} shape. The tenant header block is not what this test is about. +func (f *fakeProductRepo) GetTenantInfo(tenantID, applocationID int) (map[string]interface{}, error) { + return nil, nil +} + +func (f *fakeProductRepo) GetProducts(params models.ProductFilter) ([]models.Products, error) { + f.calls = append(f.calls, "GetProducts") + return peelamedu, nil +} + +// The filter has to be WIRED, not merely written. +// +// The tests above exercise InStockOnly directly, which would keep passing if +// nobody called it. This goes through the endpoint the customer app actually +// hits and asserts on what a shopper would be shown. +func TestTheBrowseEndpointOffersOnlyStockedProducts(t *testing.T) { + repo := newFakeRepo() + svc := NewProductService(repo, &fakeCatalogueService{}) + + result, err := svc.GetProductsBySubcategory(models.ProductFilter{ + CategoryID: 2, TenantID: 1135, LocationID: 1170, + }) + if err != nil { + t.Fatalf("GetProductsBySubcategory: %v", err) + } + + groups, _ := result["details"].([]models.SubcategoryProductResponse) + var offered []models.Products + for _, g := range groups { + offered = append(offered, g.Products...) + } + + if len(offered) != 1 { + t.Fatalf("the app was offered %d products, want 1: %v", len(offered), names(offered)) + } + if offered[0].Productid != 7086 { + t.Errorf("offered product %d, want 7086 — the only one Peelamedu has", offered[0].Productid) + } +}