diff --git a/README.md b/README.md index b5b1165..99f9520 100644 --- a/README.md +++ b/README.md @@ -120,8 +120,12 @@ current example of all seven. - **One MQTT client id per replica.** A second connection with the same id evicts the first. Never run a local process with the production `MQTT_URL`. -- **`scratch/` tools read production** when run with `.env.production`. - They are read-only by construction; keep them that way. +- **`scratch/` tools read production** when run with `.env.production`, and + most are read-only. Two are not — `termbackfill` and + `cataloguefactsbackfill` repair rows that no endpoint can reach. Both + default to a dry run that prints every change and write only when passed + `apply`, and both print the SQL to undo themselves afterwards. A new tool + that writes follows that shape or it does not write. ## Operations cheat-sheet (Kubernetes, namespace `nearle`) diff --git a/controllers/productController.go b/controllers/productController.go index e22dda4..3c816bb 100644 --- a/controllers/productController.go +++ b/controllers/productController.go @@ -191,7 +191,14 @@ func (ctl *ProductController) CreateProduct(c *fiber.Ctx) error { }) } - if err := ctl.productService.CreateProduct(product); err != nil { + // The created row, not the parsed body. + // + // This returned the struct it had just parsed off the request, which by + // definition carried `productid: 0` — the id is assigned by the database a + // moment later and was never read back. Every caller that needed the id + // went and looked the product up again by SKU. + created, err := ctl.productService.CreateProduct(product) + if err != nil { return c.JSON(fiber.Map{ "code": http.StatusInternalServerError, "message": "Failed to create product", @@ -203,7 +210,7 @@ func (ctl *ProductController) CreateProduct(c *fiber.Ctx) error { "code": http.StatusCreated, "message": "Product created successfully", "status": true, - "data": product, + "data": created, }) } diff --git a/controllers/tenantController.go b/controllers/tenantController.go index 7bdcb10..cfbbf66 100644 --- a/controllers/tenantController.go +++ b/controllers/tenantController.go @@ -44,6 +44,25 @@ func (ctl *TenantController) SearchTenant(c *fiber.Ctx) error { func (ctl *TenantController) GetAllTenants(c *fiber.Ctx) error { pageno, _ := strconv.Atoi(c.Query("pageno")) pagesize, _ := strconv.Atoi(c.Query("pagesize")) + + // Paging is defaulted, not required. + // + // The repository builds LIMIT/OFFSET from these directly, so a caller that + // omitted either — or sent pageno=0 — got an empty result reported as + // `code 200, status true, message "Success"`. "There are no tenants on the + // platform" and "you forgot a query parameter" are very different answers + // and this endpoint gave the first for the second. + // + // Defaulted rather than rejected with a 400: every existing caller that + // works today keeps working, and a platform list with no paging asked for + // has an obvious right answer — the first page. + if pageno < 1 { + pageno = 1 + } + if pagesize < 1 { + pagesize = 50 + } + status := c.Query("status") aid, _ := strconv.Atoi(c.Query("applocationid")) tenanttype := c.Query("tenanttype") diff --git a/init/README.md b/init/README.md index 356af44..a0baf74 100644 --- a/init/README.md +++ b/init/README.md @@ -39,11 +39,41 @@ inside a git repository. `.gitignore` excludes `*.sql` here for that reason. ## Getting something to test against -An empty schema boots but has no tenants, so there is nothing to sign in as. -Two options: +`nearledb/02-seed.sql` is committed and applied automatically, so a fresh +volume already has a merchant to sign into. It invents one rather than copying +one, which is why it can live here at all. -- **Onboard a tenant through the console** once it is pointed at localhost. - That exercises the real path and is usually what you want. -- **Copy a few rows** you actually need — a tenant, its locations, its - app_users — with `pg_dump --data-only --table=...`. Check what you are - copying: `app_users.password` is stored in clear. +| Account | Password | Opens | +|---|---|---| +| `super@nearle.invalid` | `localdev` | Nearle Admin — the platform workspace | +| `admin@testmart.invalid` | `localdev` | Store Admin — all of Testmart's branches | +| `main@testmart.invalid` | `localdev` | Store user — Testmart Main only | + +It also seeds the role ladder, three aisles under category 2, and a second +merchant (`Halfmart`) deliberately left in the broken `categoryid = 0` shape as +a permanent regression fixture. The sequences are moved past the seeded ids at +the end, so the first row you create locally does not come back as id 1. + +If you need something it does not cover: + +- **Onboard a tenant through the console.** That exercises the real path and is + usually what you want. +- **Copy a few rows** you actually need with `pg_dump --data-only --table=...`. + Check what you are copying: `app_users.password` is stored in clear. + +## The catalogue database + +`cataloguedb/02-seed.sql` is committed too, and also entirely invented. The +real catalogue is another team's scrape of real retailers and a dump of it does +not belong on a laptop. + +Without it the catalogue database exists but holds no catalogue: every +`brand_*` table is missing, `getbrands` answers 500, and the global catalogue +screen, the import flow and `importcatalogueproduct` cannot be exercised at +all. The seed gives you two brands: + +- `brand_testbrand` — every column the reader knows about, four products, one + of them deliberately with no images. +- `brand_sparsebrand` — only `id`, `product_name` and a price, to keep the + degraded-but-still-listed path covered. Brands are discovered by table name, + so adding another is just another `brand_*` table. diff --git a/init/cataloguedb/02-seed.sql b/init/cataloguedb/02-seed.sql new file mode 100644 index 0000000..c0c80a6 --- /dev/null +++ b/init/cataloguedb/02-seed.sql @@ -0,0 +1,109 @@ +-- A synthetic global catalogue to develop against. +-- +-- INVENTED DATA, exactly like `nearledb/02-seed.sql` and for the same reason: +-- the real catalogue is somebody else's scrape of real retailers, and a dump of +-- it does not belong on a laptop inside a git repository. +-- +-- ── Why this file has to exist ────────────────────────────────────────────── +-- +-- `init/cataloguedb/` was empty, so a local stack had a catalogue DATABASE with +-- no catalogue in it. Every `brand_*` table was missing, `getbrands` answered +-- 500, and the whole catalogue-import path — the global catalogue screen, the +-- import flow, `importcatalogueproduct` — could not be exercised locally at +-- all. It is a documented feature with its own integration doc and it had no +-- local coverage whatsoever. +-- +-- ── The shape ─────────────────────────────────────────────────────────────── +-- +-- Brands are discovered from `information_schema` by table name, so a table +-- called `brand_` IS a brand; there is no registry to add it to. +-- `catalogueCoreColumns` requires only `id` and `product_name` — everything +-- else is selected when present and replaced with NULL when absent, so a +-- partial table degrades rather than disappearing. These two are written full +-- so that the degraded path is a deliberate test, not the only thing available: +-- `brand_testbrand` has every column, and `brand_sparsebrand` deliberately has +-- only the core two plus a price, to exercise `columnsFor`. +-- +-- `image_id` is the durable key across re-scrapes — catalogue ids are not +-- stable and `models.Products.Imageid` is what the import stores — so every +-- product here has one and they are distinct. + +CREATE EXTENSION IF NOT EXISTS vector; + +-- ── A brand with the full column set ──────────────────────────────────────── +CREATE TABLE IF NOT EXISTS brand_testbrand ( + id BIGSERIAL PRIMARY KEY, + product_name TEXT NOT NULL, + title TEXT, + description TEXT, + category TEXT, + image_id TEXT, + size TEXT, + variant_key TEXT, + product_sku TEXT, + sku_source TEXT, + -- A RANGE, not a price. The global catalogue carries what retailers were + -- seen charging; the shop sets its own price at import time, which is why + -- the console collects one before an import can be enabled. + price_range TEXT, + providers TEXT[], + fssai_license TEXT, + highlights TEXT[], + nutrients TEXT[], + search_query TEXT, + image_url TEXT, + image_urls TEXT[], + created_at TIMESTAMPTZ DEFAULT NOW(), + updated_at TIMESTAMPTZ DEFAULT NOW() +); + +INSERT INTO brand_testbrand + (product_name, title, description, category, image_id, size, variant_key, + product_sku, sku_source, price_range, providers, fssai_license, + highlights, nutrients, search_query, image_url, image_urls) +VALUES + ('Testbrand Basmati Rice 5kg', 'Testbrand Basmati Rice', 'Long grain basmati, aged twelve months.', + 'Rice & Grains', 'IMG-TB-RICE-5K', '5 kg', 'rice-5kg', 'TB-RICE-5K', 'scrape', + '380-420', ARRAY['bigbasket','amazon'], '12345678901234', + ARRAY['Aged 12 months','Extra long grain'], ARRAY['Energy 350kcal','Protein 7g'], + 'basmati rice 5kg', 'https://placehold.co/300x300?text=Rice5kg', + ARRAY['https://placehold.co/300x300?text=Rice5kg','https://placehold.co/300x300?text=Rice5kg-back']), + + ('Testbrand Basmati Rice 1kg', 'Testbrand Basmati Rice', 'Long grain basmati, aged twelve months.', + 'Rice & Grains', 'IMG-TB-RICE-1K', '1 kg', 'rice-1kg', 'TB-RICE-1K', 'scrape', + '85-99', ARRAY['bigbasket'], '12345678901234', + ARRAY['Aged 12 months'], ARRAY['Energy 350kcal','Protein 7g'], + 'basmati rice 1kg', 'https://placehold.co/300x300?text=Rice1kg', + ARRAY['https://placehold.co/300x300?text=Rice1kg']), + + ('Testbrand Sunflower Oil 1L', 'Testbrand Sunflower Oil', 'Refined sunflower oil, light and neutral.', + 'Oils & Ghee', 'IMG-TB-OIL-1L', '1 L', 'oil-1l', 'TB-OIL-1L', 'scrape', + '150-185', ARRAY['bigbasket','jiomart'], '99999999999999', + ARRAY['Vitamin E','Light frying'], ARRAY['Energy 900kcal','Fat 100g'], + 'sunflower oil 1 litre', 'https://placehold.co/300x300?text=Oil1L', + ARRAY['https://placehold.co/300x300?text=Oil1L']), + + -- No images at all. `ImportCatalogueProduct` only sets `productimages` when + -- the product has photos, so this row is the one that proves an import still + -- works when it does not — the case that used to hit the jsonb empty-string + -- failure in `products`. + ('Testbrand Salt 1kg', 'Testbrand Iodised Salt', 'Free-flowing iodised salt.', + 'Everyday', 'IMG-TB-SALT-1K', '1 kg', 'salt-1kg', 'TB-SALT-1K', 'scrape', + '20-28', ARRAY['jiomart'], NULL, + NULL, NULL, 'iodised salt 1kg', NULL, NULL); + +-- ── A brand with only the core columns ────────────────────────────────────── +-- +-- Discovery used to demand all eighteen columns, which made a table like this +-- INVISIBLE rather than merely thin — 16 of 35 live brands were unreachable +-- from this side for exactly that reason. Keeping one here means the +-- degraded-but-listed path is covered by the seed and stays covered. +CREATE TABLE IF NOT EXISTS brand_sparsebrand ( + id BIGSERIAL PRIMARY KEY, + product_name TEXT NOT NULL, + price_range TEXT +); + +INSERT INTO brand_sparsebrand (product_name, price_range) VALUES + ('Sparsebrand Biscuits 100g', '20-30'), + ('Sparsebrand Tea 250g', '110-140'); diff --git a/init/nearledb/02-seed.sql b/init/nearledb/02-seed.sql index 3bc9a25..cc7cf7b 100644 --- a/init/nearledb/02-seed.sql +++ b/init/nearledb/02-seed.sql @@ -161,4 +161,72 @@ INSERT INTO productstocks ( (9504, 9001, 9102, 9301, NOW(), 'in', 12, 'Active') ON CONFLICT (productstockid) DO NOTHING; +-- ── The platform operator ─────────────────────────────────────────────────── +-- +-- Without this there is nobody who can open the Nearle Admin workspace, which +-- is the one this console was built for first. `resolveRole` checks +-- `issuperadmin` BEFORE roleid — deliberately, because the flag is derived by +-- the server and a roleid is just a number in a row — so no amount of role 1 +-- gets you in without it, and every local session landed in Store Admin +-- instead. The accounts above are one per role and this was the role they were +-- missing. +-- +-- Not attached to either merchant in spirit, only in columns: a platform +-- operator has to carry a tenantid because the column is not nullable, and +-- nothing in the admin workspace reads it. +INSERT INTO app_users ( + userid, authname, firstname, lastname, email, dialcode, contactno, + configid, roleid, password, tenantid, locationid, applocationid, + status, issuperadmin +) VALUES + (9299, 'super@nearle.invalid', 'Nearle', 'Operator', 'super@nearle.invalid', + '+91', '9000009999', 1, 1, 'localdev', 9001, 9101, 9001, 'Active', true) +ON CONFLICT (userid) DO NOTHING; + +-- ── The role ladder ───────────────────────────────────────────────────────── +-- +-- `getstaffs` LEFT JOINs app_roles for `rolename`, so an empty table is not an +-- error — every person on Users & access simply reads "—" where their role +-- should be. The ids are the ones the rest of the system already assumes: +-- 1 and 3 reach Store Admin, 4 is a branch manager, 7 and 8 are till accounts +-- and are excluded from every back-office query by the backend itself. +INSERT INTO app_roles (roleid, rolename, configid) VALUES + (1, 'Super admin', 1), + (3, 'Admin', 1), + (4, 'Manager', 1), + (7, 'Supervisor', 1), + (8, 'Cashier', 1) +ON CONFLICT (roleid) DO NOTHING; + +-- ── Aisles under the category the customer app browses ────────────────────── +-- +-- categoryid 2 is the only category the app lists, and the aisle a shopper +-- reads is the SUBCATEGORY. With none of these the sheet importer has nothing +-- to resolve a row's category against, so every imported product falls back to +-- subcategoryid 0 and lands under "Uncategorized". +INSERT INTO productsubcategories (subcatid, categoryid, tenantid, subcatname, status, sortorder) +VALUES + (9601, 2, 9001, 'Rice & Grains', 'Active', 1), + (9602, 2, 9001, 'Oils & Ghee', 'Active', 2), + (9603, 2, 9001, 'Snacks', 'Active', 3) +ON CONFLICT (subcatid) DO NOTHING; + +-- ── Move the sequences past the seeded ids ────────────────────────────────── +-- +-- Everything above inserts an explicit id, which does NOT advance the sequence +-- behind that column. So the first tenant, outlet or product created against a +-- fresh local database came back as id 1 — harmless here, but it means local +-- ids look nothing like the ones the same code produces in production, and a +-- seed that ever collides with a sequence value fails on a duplicate key. +-- +-- `GREATEST(..., 1)` because setval refuses a value below the sequence minimum, +-- and a table the seed does not touch is legitimately empty. +SELECT setval('tenants_tenantid_seq', GREATEST((SELECT COALESCE(MAX(tenantid),0) FROM tenants), 1)); +SELECT setval('tenantlocations_locationid_seq', GREATEST((SELECT COALESCE(MAX(locationid),0) FROM tenantlocations), 1)); +SELECT setval('app_users_userid_seq', GREATEST((SELECT COALESCE(MAX(userid),0) FROM app_users), 1)); +SELECT setval('products_productid_seq', GREATEST((SELECT COALESCE(MAX(productid),0) FROM products), 1)); +SELECT setval('productlocations_productlocationid_seq', GREATEST((SELECT COALESCE(MAX(productlocationid),0) FROM productlocations), 1)); +SELECT setval('productstocks_productstockid_seq', GREATEST((SELECT COALESCE(MAX(productstockid),0) FROM productstocks), 1)); +SELECT setval('customers_customerid_seq', GREATEST((SELECT COALESCE(MAX(customerid),0) FROM customers), 1)); + COMMIT; diff --git a/main.go b/main.go index 290b1a7..b2b8f70 100644 --- a/main.go +++ b/main.go @@ -79,6 +79,38 @@ func main() { log.Println("⚠️ could not add products.productimages, extra photos will not be stored:", err) } + // What the global catalogue knew about this product, kept. + // + // The import copies eight of the catalogue's eighteen fields onto the + // tenant's product and left the other ten behind — among them the FSSAI + // licence, the nutrition lines, the highlights, the provider list, the + // price range and the variant key. The console needs exactly those to + // decide what to charge, so `ProductDrawer` went back to the catalogue for + // them on every open. + // + // That lookup is not a substitute for storing them. A tenant's product is a + // SNAPSHOT and outlives its source row: the catalogue is re-scraped, a + // variant is retired, and the licence number and the nutrition panel for a + // product the shop is still selling are gone with no way back. Measured + // locally by retiring one row — the product survived, everything the drawer + // shows about it did not. + // + // One jsonb column rather than six typed ones, and rather than the + // `productspecs` table that has sat unused since the schema was written. + // The value is a snapshot of somebody else's record, read as a whole and + // displayed as a whole — it is never joined, aggregated or filtered — and + // the catalogue grows fields faster than this side can add migrations. + // Postgres can still reach inside it (`cataloguefacts->>'fssai_license'`) + // on the day somebody needs to. `productimages` beside it made the same + // call for the same reason. + // + // Not fatal on failure, exactly like the column above: a product without + // its catalogue facts is the product we have today. + if err := db.DB.Exec( + `ALTER TABLE products ADD COLUMN IF NOT EXISTS cataloguefacts jsonb`).Error; err != nil { + log.Println("⚠️ could not add products.cataloguefacts, catalogue detail will not survive a re-scrape:", err) + } + // When a product became visible to a store, and the only thing that decides // whether it is. // @@ -173,6 +205,60 @@ func main() { log.Println("productvariants.variantid given a key generator (one time)") } + // Key generators for the two partner tables, for exactly the reason above. + // + // `partnerinfo.partnerid` and `partnerlocations.partnerlocationid` are both + // NOT NULL with no default and no identity, so GORM — which sends nothing + // for a key it expects the database to mint — had every insert refused with + // a not-null violation. `createpartner` therefore could not write a partner + // OR its regions: the endpoint exists, the form exists, and the row could + // never land. The five partners on the platform were all inserted by hand, + // which is the symptom rather than a choice. + // + // This matters more than one broken button. `GetPartners` now separates the + // partners registered through this console from the ones another product + // left in the shared `partnerinfo` by joining `partnerlocations` — and only + // a successful create writes that table. Without a key generator no partner + // can ever be registered, so nothing would ever have a link row and the + // Rider partners page would be empty forever. + // + // Both sequences start above the ids already there, so the hand-inserted + // rows keep theirs. + for _, key := range []struct{ table, column string }{ + {"partnerinfo", "partnerid"}, + {"partnerlocations", "partnerlocationid"}, + } { + var keyed int64 + if err := db.DB.Raw(` + SELECT COUNT(1) FROM information_schema.columns + WHERE table_name = ? AND column_name = ? + AND (column_default IS NOT NULL OR is_identity = 'YES')`, + key.table, key.column).Scan(&keyed).Error; err != nil { + log.Fatalf("could not check %s.%s: %v", key.table, key.column, err) + } + if keyed > 0 { + continue + } + + seq := key.table + "_" + key.column + "_seq" + if err := db.DB.Exec(fmt.Sprintf( + `CREATE SEQUENCE IF NOT EXISTS %s START WITH 1 OWNED BY %s.%s`, + seq, key.table, key.column)).Error; err != nil { + log.Fatalf("could not create %s: %v", seq, err) + } + if err := db.DB.Exec(fmt.Sprintf( + `SELECT setval('%s', COALESCE((SELECT MAX(%s) FROM %s), 0) + 1, false)`, + seq, key.column, key.table)).Error; err != nil { + log.Fatalf("could not position %s: %v", seq, err) + } + if err := db.DB.Exec(fmt.Sprintf( + `ALTER TABLE %s ALTER COLUMN %s SET DEFAULT nextval('%s')`, + key.table, key.column, seq)).Error; err != nil { + log.Fatalf("could not default %s.%s: %v", key.table, key.column, err) + } + log.Printf("%s.%s given a key generator (one time)", key.table, key.column) + } + // The catalogue's own stable key for an imported product. // // `catalogueid` was never able to be this. The catalogue is rebuilt by diff --git a/models/partner.go b/models/partner.go index c4d862c..0707a88 100644 --- a/models/partner.go +++ b/models/partner.go @@ -296,10 +296,17 @@ type NewPartner struct { Where they work — ONE district, not a set. `Applocationid` is the home region and goes on the partner row itself, - because `GetPartners` filters on it and the rider app reads it. - `Applocationids` is every region they cover and goes to - `partnerlocations` — one partner routinely serves several cities, and - that is the whole reason the link table exists. + because the rider app reads it. The same region is also written to + `partnerlocations`, which is the table that may hold SEVERAL — a partner + routinely serves more than one city, and that is why the link table + exists. Nothing populates more than one today: `regionsOf` returns this + single field and the console's form offers one district, never a set. + + `GetPartners` reads the link table rather than this field, for two + reasons. It is the column allowed to grow, so a partner who covers a + second city will be found there without another change. And + `partnerinfo` is shared with another product that writes no link rows, + so having one is what marks a partner as ours. */ Applocationid int `json:"applocationid"` /* diff --git a/models/product.go b/models/product.go index 78dff8e..88bb49c 100644 --- a/models/product.go +++ b/models/product.go @@ -155,6 +155,23 @@ type Products struct { // `catalogueProductColumns` casts its text[] columns to text. Productimages string `json:"productimages,omitempty" gorm:"column:productimages;type:jsonb"` + // The catalogue's own record of this product, as it stood at import. + // + // Holds the fields the snapshot does not have columns for — fssai_license, + // highlights, nutrients, providers, price_range, variant_key, title, + // sku_source, search_query — so the console can show them without asking + // the catalogue again. It asked on every drawer open, and got nothing back + // the moment a re-scrape retired the source row, taking a licence number + // and a nutrition panel off a product the shop was still selling. + // + // Empty for anything that did not come from the catalogue: a sheet-imported + // product has no such record, and the drawer falls back to the live lookup + // for those exactly as before. + // + // A string for the same reason `Productimages` is one — GORM's raw + // scan-into-struct silently drops slice- and map-kind destination fields. + Cataloguefacts string `json:"cataloguefacts,omitempty" gorm:"column:cataloguefacts;type:jsonb"` + Productdesc string `json:"productdesc,omitempty"` Productsku string `json:"productsku,omitempty"` Brandid int `json:"brandid,omitempty"` @@ -225,6 +242,28 @@ type Locationproducts struct { Productimage string `json:"productimage,omitempty"` Productdesc string `json:"productdesc,omitempty"` Productsku string `json:"productsku,omitempty"` + + // Three columns this read used to leave in the table. + // + // All three are stored on `products` and none of them reached the store + // catalogue screen, because this struct had no field to scan them into — + // so the console could not use what the import had gone to the trouble of + // saving: + // + // Imageid the catalogue's durable key, and what HealthScorePanel + // joins on. Absent, the panel reads it as "this product + // never came from the catalogue" and renders nothing — for + // EVERY product, including ones that plainly did. + // Productimages the rest of a product's photos. `imagesOf()` parses this + // and always got undefined, so the gallery fell back to + // the single `productimage` and the extra images — 90 of + // nestle's 123 products have them — were never shown. + // Cataloguefacts the licence, nutrition, highlights, providers and price + // range kept at import so they survive a re-scrape. + Imageid string `json:"imageid,omitempty"` + Productimages string `json:"productimages,omitempty"` + Cataloguefacts string `json:"cataloguefacts,omitempty"` + Brandid int `json:"brandid,omitempty"` Productbrand string `json:"productbrand,omitempty"` Productunit string `json:"productunit"` diff --git a/models/tenant.go b/models/tenant.go index 2088f17..44cb310 100644 --- a/models/tenant.go +++ b/models/tenant.go @@ -71,6 +71,14 @@ type Tenantinfo struct { Allocationid int `json:"allocationid"` Allocationtype string `json:"allocationtype"` Allocationmode int `json:"allocationmode"` + + // How many outlets this merchant has. + // + // Only `GetAllTenants` fills this; it is 0 everywhere else, which is why it + // is last and optional rather than part of the record proper. The console's + // store list previously derived it by counting duplicate rows, and this + // endpoint has never returned duplicates — see the note on the query. + Branchcount int `json:"branchcount"` } type Tenantlocations struct { @@ -190,6 +198,10 @@ type StaffInfo struct { Tenantid int `json:"tenantid"` Locationid int `json:"locationid"` Locationname string `json:"locationname"` + // Whether this login still works, straight off `app_users.status`. + // Without it every row on the console's Users & access screen read + // "Unknown", because the field was never selected or sent. + Status string `json:"status"` } type Tenantuser struct { diff --git a/repositories/partnerRepository.go b/repositories/partnerRepository.go index c9de69f..1794915 100644 --- a/repositories/partnerRepository.go +++ b/repositories/partnerRepository.go @@ -87,30 +87,65 @@ func (r *partnerRepository) GetPartners(aid, pid, uid int) ([]models.Partnerinfo var q1 string var args []interface{} + // Every variant joins partnerlocations, and that join is the whole point. + // + // ── It is what separates our partners from somebody else's ────────────── + // + // `partnerinfo` is shared. It has no column saying which product a row + // belongs to — no configid, no appid — so a partner created by another app + // on this database is indistinguishable from ours by its own fields, and + // this read used to return every Active row on the platform. The console + // made that worse rather than better: it asks `getapplocations` for EVERY + // region and then fetches partners region by region, so the applocationid + // filter below never narrowed anything. + // + // `partnerlocations` is the difference. Only `CreatePartner` writes it — + // one row per region, in the same transaction as the partner — so a row in + // that table means "registered through this console". The partners that + // predate it were inserted by hand and have none, which is why two of them + // are called "Test". + // + // ── The region filter reads the link table, not the home region ───────── + // + // `partnerinfo.applocationid` is the HOME region — CreatePartner writes + // `regions[0]` there — while partnerlocations holds every region covered. + // Today those are always the same one region, because `regionsOf` returns a + // single district and the console's form offers one ("never a set"). So + // this is not a behaviour change yet; it is the filter being applied to the + // column that is allowed to grow. The moment a partner covers two cities, + // filtering on the home region would hide them from the second, and the + // link table is the whole reason that table exists. + // + // DISTINCT for that same future: a partner covering three regions has three + // rows in the join and is still one partner. Only partnerinfo columns are + // selected, so there is nothing per-region for it to fail to collapse. + const columns = `select distinct p.partnerid,p.applocationid,p.partnertypeid,p.partnername, + p.primarycontact,p.primaryemail,p.contactno,p.address,p.suburb,p.state,p.city,p.partnerimage + from partnerinfo p + inner join partnerlocations l on l.partnerid = p.partnerid + where p.status='Active'` + if pid != 0 { - q1 = `select partnerid,applocationid,partnertypeid,partnername,primarycontact,primaryemail, - contactno,address,suburb,state,city,partnerimage - from partnerinfo where status='Active' and partnerid=?` + // Scoped the same way on purpose: asking for a partner by id must not + // be a way round the separation above. + q1 = columns + ` and p.partnerid=?` args = append(args, pid) } else if aid != 0 { - q1 = `select partnerid,applocationid,partnertypeid,partnername,primarycontact,primaryemail, - contactno,address,suburb,state,city,partnerimage - from partnerinfo where status='Active' and applocationid=?` + q1 = columns + ` and l.applocationid=?` args = append(args, aid) } else { - q1 = `select partnerid,applocationid,partnertypeid,partnername,primarycontact,primaryemail, - contactno,address,suburb,state,city,partnerimage - from partnerinfo where status='Active'` + q1 = columns } + q1 += ` order by p.partnername, p.partnerid` + err := r.db.Raw(q1, args...).Find(&data).Error if err != nil { return nil, err } - print(q1) return data, nil } @@ -615,13 +650,17 @@ them are named "Test". Where a partner works is recorded twice, on purpose and not by accident: - partnerinfo.applocationid their home region — `GetPartners` filters on it - and the rider app reads it + partnerinfo.applocationid their home region — the rider app reads it partnerlocations every region they cover Both are kept in step here. Writing only the first would confine a partner to -one city, and writing only the second would hide them from every existing -query. */ +one city, and writing only the second would hide them from the rider app. + +`GetPartners` reads the SECOND: it joins partnerlocations, which both scopes a +region query to every city a partner actually covers and — because only this +function writes that table — separates partners registered here from the ones +another product put in the shared `partnerinfo`. So the link rows are not +bookkeeping; they are what makes a partner ours. */ // CreatePartner onboards a delivery partner and records the regions they cover. func (r *partnerRepository) CreatePartner(input models.NewPartner) (int, error) { diff --git a/repositories/productRepository.go b/repositories/productRepository.go index 2083b02..378e057 100644 --- a/repositories/productRepository.go +++ b/repositories/productRepository.go @@ -26,7 +26,6 @@ type ProductRepository interface { 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 GetStockStatement(tenantID, locationID, subcategoryID, pageno, pagesize int, keyword string) ([]models.Productstockstatement, error) @@ -393,19 +392,32 @@ func (r *productRepository) UpdateProductStatus(productIDs []int, status string) Update("productstatus", status).Error } -func (r *productRepository) CreateProduct(product models.Products) error { - tx := r.db.Begin() - - if err := tx.Create(&product).Error; err != nil { - tx.Rollback() - return err +// normaliseProductJSON makes a product safe to INSERT. +// +// `products.productimages` is jsonb and `models.Products.Productimages` is a +// plain string, so a caller that never set it hands GORM the zero value — and +// GORM puts that empty string in the INSERT rather than omitting the column. +// Postgres answers "invalid input syntax for type json (SQLSTATE 22P02)" and +// the whole row is rejected, over a field nobody asked for. +// +// That was not a corner case: the console's sheet importer sends no +// productimages at all, so EVERY product it created failed with a 500, and +// ImportCatalogueProduct leaves the field empty for any catalogue product that +// has no photos. An empty ARRAY is the honest value — there are no extra +// images — and it is what `catalogueUploadService` already does for its own +// jsonb column, for the same reason. +// +// Applied at the one create path, which is the last point before the SQL, and +// the constraint being satisfied is the database's. +func normaliseProductJSON(product *models.Products) { + if strings.TrimSpace(product.Productimages) == "" { + product.Productimages = "[]" } - - if err := tx.Commit().Error; err != nil { - return err + // An OBJECT, not an array: this one holds named catalogue fields, and `{}` + // is what a reader parsing it expects to find when there are none. + if strings.TrimSpace(product.Cataloguefacts) == "" { + product.Cataloguefacts = "{}" } - - return nil } func (r *productRepository) UpdateProduct(product models.Products) error { @@ -1316,10 +1328,22 @@ func (r *productRepository) FindTenantProductByCatalogueRef(tenantid int, brand return &product, nil } -// CreateProductReturningID inserts a new product snapshot and returns its -// generated productid. Kept separate from CreateProduct so existing callers -// of CreateProduct are unaffected. +// CreateProductReturningID inserts a product and returns its generated +// productid. +// +// This is now the only way to create one. There used to be a second method, +// `CreateProduct`, that did the same INSERT and threw the id away — it took +// the struct by value, so GORM wrote the generated id onto a copy that went +// out of scope, and `POST /products/create` answered `productid: 0` for every +// product it had just created. The console worked around it by creating, then +// re-reading the whole tenant catalogue, then matching back by SKU. +// +// The two were kept apart so that "existing callers are unaffected", but the +// only caller of the id-less one was the endpoint that needed the id most. +// One create path also means the jsonb guard above has one place to live. func (r *productRepository) CreateProductReturningID(product models.Products) (int, error) { + normaliseProductJSON(&product) + if err := r.db.Create(&product).Error; err != nil { return 0, err } diff --git a/repositories/tenantRepository.go b/repositories/tenantRepository.go index 709bc64..9e029f7 100644 --- a/repositories/tenantRepository.go +++ b/repositories/tenantRepository.go @@ -85,7 +85,22 @@ func (r *tenantRepository) GetAllTenants(pageno, pagesize, aid int, status, tena var data []models.Tenantinfo - base := `SELECT * FROM tenants a WHERE 1 = 1` + // `branchcount` is selected here because there is nowhere else to get it. + // + // This returns one row per TENANT — there is no join to tenantlocations at + // all — but the console's store list read it as one row per + // tenant-location pair and counted the duplicates, so every merchant on the + // platform showed exactly one branch, and the "Branches" and "Avg branches" + // tiles above the list were the tenant count wearing another name. The + // tenant's own detail page, which reads gettenantlocations, disagreed with + // the list it was opened from. + // + // A correlated subquery rather than a LEFT JOIN + GROUP BY: the row shape + // stays exactly as it was, so nothing else that reads this endpoint has to + // change, and every filter below still applies to `a` alone. + base := `SELECT a.*, + (SELECT COUNT(*) FROM tenantlocations tl WHERE tl.tenantid = a.tenantid) AS branchcount + FROM tenants a WHERE 1 = 1` var ( conds []string @@ -337,7 +352,12 @@ func (r *tenantRepository) GetStaffs(tid int) ([]models.StaffInfo, error) { a.state,a.postcode,a.userfcmtoken,a.pin,a.applocationid, a.roleid,a.partnerid,a.tenantid,a.locationid, b.locationname, - COALESCE(c.rolename,'') AS rolename + COALESCE(c.rolename,'') AS rolename, + -- Whether the account still works. Absent from this SELECT + -- until now, so Users & access had nothing to read and showed + -- every person on the platform as "Unknown" — an admin could not + -- tell a working login from one that had been switched off. + COALESCE(a.status,'') AS status FROM app_users a LEFT JOIN tenantlocations b ON a.locationid = b.locationid LEFT JOIN app_roles c ON c.roleid = a.roleid @@ -625,6 +645,51 @@ func (r *tenantRepository) CreateTenantUser(data models.Tenants) (bool, error) { var custloc models.Customerlocations var tcust models.Tenantcustomers + // A tenant with configid 0 is unreachable, and it takes its customer row + // with it. + // + // Step 3 below already forces `user.Configid = 1`, with a comment + // explaining that AppLogin only ever queries configid 1 and a zero makes + // the account permanently unfindable. The same zero was left to flow into + // `tenants` itself and into the `customers` row copied from it at step 4, + // where nothing corrected it — so a caller that omits configid (the console + // sends it; the mobile route and anything else need not) created a business + // and a customer that no scoped read can see. + // + // Defaulted rather than rejected: 1 is the only value any caller has ever + // meant here, and refusing the create would break callers that work today. + if data.Configid == 0 { + data.Configid = 1 + } + + // Give the primary outlet the scaffolding the tenant already has. + // + // The outlet itself is created by GORM, as the `Tenantlocations` + // association on the struct below — the console nests a full object in the + // request and step 1 saves it with the tenant. What it does NOT do is fill + // anything the caller left out, and two of those columns matter: + // + // applocationid — `orderRepository.go` calls it "authoritative" and has + // no fallback anywhere for a 0. + // moduleid — same file: "tenantlocations carries 0 for + // moduleid/partnerid at outlets whose live orders + // nonetheless use non-zero values", worked around there + // by copying scaffolding off the most recent real order. + // A shop commissioned a minute ago has no such order. + // + // Neither column has a database default, and no onboarding form asks for + // them — they describe the platform, not the shop. The tenant's own values + // are the right answer and are already right here. + // + // Filled before the insert rather than corrected after it, so there is one + // write and no window where the row exists with a zero in it. + if data.Tenantlocations.Applocationid == 0 { + data.Tenantlocations.Applocationid = data.Applocationid + } + if data.Tenantlocations.Moduleid == 0 { + data.Tenantlocations.Moduleid = data.Moduleid + } + tx := r.db.Begin() // Step 1: Insert into tenants diff --git a/repositories/userRepository.go b/repositories/userRepository.go index 60a4cf7..c99b5e6 100644 --- a/repositories/userRepository.go +++ b/repositories/userRepository.go @@ -255,6 +255,30 @@ func (r *userRepository) GetTenantUserById(userid int) models.TenantUserInfo { } func (r *userRepository) CreateUser(user models.User) (int, error) { + // Inherit the delivery region from the tenant when the caller did not name + // one. + // + // `app_users.applocationid` has no column default, and no console form + // collects it — it is a platform region, not something a merchant picks + // per person. So every back-office account created through this path landed + // with 0, which is not a region: `orderRepository.go` calls the equivalent + // column on tenantlocations "authoritative" and has no fallback for a zero, + // and 43 of 75 live branches are already in that state. + // + // A lookup rather than a default value, because the right answer is + // whichever region the business trades in. Failure is not fatal: the + // account is still worth creating, and a 0 here is exactly what would have + // been written anyway. + if user.Applocationid == 0 && user.Tenantid > 0 { + var inherited int + if err := r.db.Raw( + `SELECT COALESCE(applocationid, 0) FROM tenants WHERE tenantid = ?`, + user.Tenantid, + ).Scan(&inherited).Error; err == nil && inherited > 0 { + user.Applocationid = inherited + } + } + tx := r.db.Begin() if err := tx.Table("app_users").Create(&user).Error; err != nil { diff --git a/scratch/cataloguefactsbackfill/main.go b/scratch/cataloguefactsbackfill/main.go new file mode 100644 index 0000000..0a30aa3 --- /dev/null +++ b/scratch/cataloguefactsbackfill/main.go @@ -0,0 +1,402 @@ +// Backfills products.cataloguefacts for products imported before the column existed. +// +// The catalogue import copied eight of the catalogue's eighteen fields onto a +// tenant's product and left the other ten behind — the FSSAI licence, nutrients, +// highlights, providers, the typical price range, the variant key. The console +// covered for it by asking the catalogue again on every drawer open, and that +// stops working the moment a re-scrape retires the source row: a tenant's +// product is a SNAPSHOT and outlives it, so a licence number came off a product +// the shop was still selling with no way back. +// +// The import keeps them now. Every product imported BEFORE that does not have +// them, and no amount of new code fixes a row that was written last month — so +// this reads each one's catalogue entry while it is still there and stores it. +// +// go run ./scratch/cataloguefactsbackfill # dry run — shows every change +// go run ./scratch/cataloguefactsbackfill apply # writes, then prints the undo +// +// ── What it will and will not touch ───────────────────────────────────────── +// +// Only products with an `imageid` and a NULL `cataloguefacts`. That is the +// whole safety story: +// +// - NULL means nothing was ever written. A product whose facts are already +// stored — including one stored as `{}` because the catalogue genuinely had +// nothing to say — is never overwritten, so re-running this is a no-op +// rather than a second opinion. +// - No `imageid` means it never came from the catalogue. Sheet-imported +// products have no entry to read and are left alone. +// - A catalogue row that has already been retired cannot be recovered by +// anything, here or later. Those are counted and named rather than written +// as empty, because `{}` would claim the catalogue said nothing when the +// truth is that nobody asked in time. +// +// Brand tables are discovered rather than assumed, and their columns are +// checked one by one before being selected: the catalogue is another team's +// scrape, brands appear between runs, and a table missing `nutrients` is a +// perfectly good catalogue of products. Demanding the full column set is the +// exact mistake that once made 16 of 35 live brands invisible to this side. +package main + +import ( + "encoding/json" + "fmt" + "log" + "os" + "sort" + "strings" + + "github.com/joho/godotenv" + "gorm.io/driver/postgres" + "gorm.io/gorm" + "gorm.io/gorm/logger" + + "nearle/models" +) + +// The columns worth keeping, in the order the drawer reads them. Scalars and +// arrays are separated because an array comes back as a Postgres text[] literal +// and has to be parsed before it can be re-encoded as JSON. +var scalarFacts = []string{ + "title", "category", "variant_key", "sku_source", + "price_range", "fssai_license", "search_query", +} + +var arrayFacts = []string{"providers", "highlights", "nutrients"} + +type product struct { + Productid int + Productbrand string + Imageid string + Productname string + Tenantid int +} + +func main() { + apply := len(os.Args) > 1 && os.Args[1] == "apply" + + _ = godotenv.Load() + + main, err := open("DB_HOST", "DB_PORT", "DB_USER", "DB_PASSWORD", "DB_NAME") + if err != nil { + log.Fatal("nearledb: ", err) + } + cat, err := open("CATALOGUE_DB_HOST", "CATALOGUE_DB_PORT", "CATALOGUE_DB_USER", + "CATALOGUE_DB_PASSWORD", "CATALOGUE_DB_NAME") + if err != nil { + log.Fatal("cataloguedb: ", err) + } + + // The column has to exist before there is anything to fill. Checked rather + // than assumed so this says so plainly instead of failing inside a query. + var hasColumn int + main.Raw(`SELECT COUNT(*) FROM information_schema.columns + WHERE table_name = 'products' AND column_name = 'cataloguefacts'`).Scan(&hasColumn) + if hasColumn == 0 { + log.Fatal("products.cataloguefacts does not exist — start the API once to run the migration, then re-run this") + } + + var candidates []product + main.Raw(`SELECT productid, tenantid, COALESCE(productbrand,'') AS productbrand, + COALESCE(imageid,'') AS imageid, COALESCE(productname,'') AS productname + FROM products + WHERE COALESCE(imageid,'') <> '' AND cataloguefacts IS NULL + ORDER BY productbrand, productid`).Scan(&candidates) + + var ( + total int + alreadyDone int + noImageid int + ) + main.Raw(`SELECT COUNT(*) FROM products`).Scan(&total) + main.Raw(`SELECT COUNT(*) FROM products WHERE cataloguefacts IS NOT NULL`).Scan(&alreadyDone) + main.Raw(`SELECT COUNT(*) FROM products WHERE COALESCE(imageid,'') = ''`).Scan(&noImageid) + + fmt.Printf("products on the platform : %d\n", total) + fmt.Printf(" never came from the catalogue : %d (no imageid — left alone)\n", noImageid) + fmt.Printf(" facts already stored : %d (never overwritten)\n", alreadyDone) + fmt.Printf(" to backfill : %d\n\n", len(candidates)) + + if len(candidates) == 0 { + fmt.Println("nothing to do.") + return + } + + // One column check per brand table, not per product: the shape is a + // property of the table and a per-row check would be thousands of + // information_schema reads to learn the same thing. + columnsByTable := map[string][]string{} + missingTable := map[string]bool{} + + type update struct { + product product + facts string + } + var ( + updates []update + retired []product + unknown []product + emptyOnly []product + ) + + for _, p := range candidates { + table := brandTable(p.Productbrand) + if table == "" { + unknown = append(unknown, p) + continue + } + if missingTable[table] { + retired = append(retired, p) + continue + } + + cols, known := columnsByTable[table] + if !known { + cols = factColumnsOf(cat, table) + if cols == nil { + missingTable[table] = true + retired = append(retired, p) + continue + } + columnsByTable[table] = cols + } + + facts, found := factsFor(cat, table, cols, p.Imageid) + if !found { + retired = append(retired, p) + continue + } + if len(facts) == 0 { + // The row is there and had nothing in these columns. Worth writing + // `{}` — it is the true answer and it stops the console asking the + // catalogue again on every open. + emptyOnly = append(emptyOnly, p) + } + + encoded, err := json.Marshal(facts) + if err != nil { + log.Printf("could not encode facts for product %d: %v", p.Productid, err) + continue + } + updates = append(updates, update{product: p, facts: string(encoded)}) + } + + fmt.Printf("%-9s %-14s %-22s %-34s %s\n", "product", "brand", "imageid", "name", "facts recovered") + for _, u := range updates { + var keys []string + var got map[string]any + _ = json.Unmarshal([]byte(u.facts), &got) + for k := range got { + keys = append(keys, k) + } + sort.Strings(keys) + summary := strings.Join(keys, ",") + if summary == "" { + summary = "(catalogue row has none)" + } + fmt.Printf("%-9d %-14s %-22s %-34s %s\n", + u.product.Productid, trim(u.product.Productbrand, 14), trim(u.product.Imageid, 22), + trim(u.product.Productname, 34), summary) + } + + if len(retired) > 0 { + fmt.Printf("\n!! %d product(s) cannot be recovered — their catalogue row is gone:\n", len(retired)) + for _, p := range retired { + fmt.Printf(" %-9d %-14s %-22s %s\n", p.Productid, trim(p.Productbrand, 14), + trim(p.Imageid, 22), trim(p.Productname, 40)) + } + fmt.Println(" These are left NULL. The console falls back to the live lookup for them,") + fmt.Println(" which will also find nothing — the detail was lost before this ran.") + } + + if len(unknown) > 0 { + fmt.Printf("\n!! %d product(s) carry a brand with no table in the catalogue:\n", len(unknown)) + for _, p := range unknown { + fmt.Printf(" %-9d %-14s %s\n", p.Productid, trim(p.Productbrand, 14), trim(p.Productname, 40)) + } + } + + fmt.Printf("\nwill write %d product(s)", len(updates)) + if len(emptyOnly) > 0 { + fmt.Printf(", %d of them as `{}` because the catalogue row carries none of these fields", len(emptyOnly)) + } + fmt.Printf("; leaving %d NULL\n", len(retired)+len(unknown)) + + if len(updates) == 0 { + return + } + if !apply { + fmt.Println("\ndry run — nothing written. re-run with `apply` to write.") + return + } + + // One row at a time, each guarded by `cataloguefacts IS NULL` again. + // Between the read above and this write another import could have stored + // the real thing, and this must never be the one that overwrites it. + written := 0 + ids := make([]int, 0, len(updates)) + for _, u := range updates { + res := main.Exec(`UPDATE products SET cataloguefacts = ?::jsonb + WHERE productid = ? AND cataloguefacts IS NULL`, + u.facts, u.product.Productid) + if res.Error != nil { + log.Printf("product %d: %v", u.product.Productid, res.Error) + continue + } + if res.RowsAffected > 0 { + written++ + ids = append(ids, u.product.Productid) + } + } + + fmt.Printf("\nwrote %d product(s)\n", written) + + var stillNull int + main.Raw(`SELECT COUNT(*) FROM products + WHERE COALESCE(imageid,'') <> '' AND cataloguefacts IS NULL`).Scan(&stillNull) + fmt.Printf("catalogue-linked products still without facts: %d\n", stillNull) + + if len(ids) > 0 { + fmt.Printf("\nundo:\n UPDATE products SET cataloguefacts = NULL WHERE productid IN (%s);\n", + joinInts(ids)) + } +} + +func open(hostKey, portKey, userKey, passKey, nameKey string) (*gorm.DB, error) { + dsn := fmt.Sprintf("host=%s port=%s user=%s password=%s dbname=%s sslmode=disable", + os.Getenv(hostKey), os.Getenv(portKey), os.Getenv(userKey), + os.Getenv(passKey), os.Getenv(nameKey)) + return gorm.Open(postgres.Open(dsn), &gorm.Config{Logger: logger.Default.LogMode(logger.Silent)}) +} + +// brandTable mirrors the repository's rule: a brand IS a `brand_` table. +// +// Lowercased and stripped of anything that is not a letter, digit or +// underscore. The table name cannot be parameterized in SQL, so this is the +// one place it is built and it refuses to build anything else. +func brandTable(brand string) string { + cleaned := strings.Map(func(r rune) rune { + switch { + case r >= 'a' && r <= 'z', r >= '0' && r <= '9', r == '_': + return r + case r >= 'A' && r <= 'Z': + return r + 32 + } + return -1 + }, strings.TrimSpace(brand)) + + if cleaned == "" { + return "" + } + return "brand_" + cleaned +} + +// factColumnsOf returns which of the fact columns this brand table actually +// has, or nil when the table is not there at all. +func factColumnsOf(db *gorm.DB, table string) []string { + var have []string + db.Raw(`SELECT column_name FROM information_schema.columns + WHERE table_schema = 'public' AND table_name = ?`, table).Scan(&have) + if len(have) == 0 { + return nil + } + + present := map[string]bool{} + for _, c := range have { + present[c] = true + } + // image_id is how a product is found at all. Without it the table cannot + // answer the question, whatever else it holds. + if !present["image_id"] { + return nil + } + + var keep []string + for _, c := range append(append([]string{}, scalarFacts...), arrayFacts...) { + if present[c] { + keep = append(keep, c) + } + } + return keep +} + +// factsFor reads one catalogue row and returns only what it actually stated. +// +// An empty field is omitted rather than stored as "" or [], so a reader can +// tell "the catalogue did not say" from "the catalogue said none" — the drawer +// prints a row per fact and an empty string would print an empty row. +func factsFor(db *gorm.DB, table string, cols []string, imageID string) (map[string]any, bool) { + if len(cols) == 0 { + return map[string]any{}, true + } + + selects := make([]string, 0, len(cols)) + for _, c := range cols { + if isArrayFact(c) { + selects = append(selects, c+"::text AS "+c) + continue + } + selects = append(selects, c) + } + + row := map[string]any{} + res := db.Raw(`SELECT `+strings.Join(selects, ", ")+` FROM `+table+ + ` WHERE image_id = ? LIMIT 1`, imageID).Scan(&row) + if res.Error != nil || res.RowsAffected == 0 { + return nil, false + } + + facts := map[string]any{} + for _, c := range cols { + raw, ok := row[c] + if !ok || raw == nil { + continue + } + text := strings.TrimSpace(fmt.Sprintf("%v", raw)) + if text == "" { + continue + } + if isArrayFact(c) { + values := models.ParsePGArray(text) + kept := make([]string, 0, len(values)) + for _, v := range values { + if t := strings.TrimSpace(v); t != "" { + kept = append(kept, t) + } + } + if len(kept) > 0 { + facts[c] = kept + } + continue + } + facts[c] = text + } + return facts, true +} + +func isArrayFact(name string) bool { + for _, c := range arrayFacts { + if c == name { + return true + } + } + return false +} + +func trim(s string, n int) string { + if len(s) <= n { + return s + } + if n <= 1 { + return s[:n] + } + return s[:n-1] + "…" +} + +func joinInts(ids []int) string { + parts := make([]string, len(ids)) + for i, id := range ids { + parts[i] = fmt.Sprint(id) + } + return strings.Join(parts, ",") +} diff --git a/services/productService.go b/services/productService.go index 57b6c42..23291fc 100644 --- a/services/productService.go +++ b/services/productService.go @@ -4,9 +4,11 @@ import ( "encoding/json" "fmt" "log" + "strings" + "time" + "nearle/models" "nearle/repositories" - "time" ) type ProductService interface { @@ -22,7 +24,7 @@ type ProductService interface { RemoveProductVariant(tenantid, variantid int) error VariantChildIDs(tenantid int) (map[int]bool, error) CreateProductStock(stocks []models.Productstock) error - CreateProduct(product models.Products) error + CreateProduct(product models.Products) (models.Products, error) UpdateProduct(product models.Products) error DeleteProduct(productID int) error GetStockStatement(tenantID, locationID, subcategoryID, pageno, pagesize int, keyword string) ([]models.Productstockstatement, error) @@ -169,8 +171,27 @@ func (s *productService) UpdateProductStatus(productIDs []int, status string) er return s.repo.UpdateProductStatus(productIDs, status) } -func (s *productService) CreateProduct(product models.Products) error { - return s.repo.CreateProduct(product) +// CreateProduct stores one product and hands it back with its id filled in. +// +// It used to return only an error, and the id was lost on the way out: the +// repository took the struct by value, GORM wrote the generated productid onto +// that copy, and the copy was discarded — so the endpoint answered +// `productid: 0` for a row that certainly had one. +// +// The caller needs it. A product is not sellable until it has been priced at an +// outlet and stocked there, and both of those calls are keyed on productid, so +// every importer had to create, re-read the tenant's whole catalogue, and match +// its own rows back by SKU to carry on — which is also why creating two +// products with the same SKU quietly attached the second one's stock to the +// first. +func (s *productService) CreateProduct(product models.Products) (models.Products, error) { + id, err := s.repo.CreateProductReturningID(product) + if err != nil { + return models.Products{}, err + } + + product.Productid = id + return product, nil } func (s *productService) UpdateProduct(product models.Products) error { @@ -308,6 +329,54 @@ func (s *productService) DeleteProductLocation(tenantid, locationid, productid i return s.repo.DeleteProductLocation(tenantid, locationid, productid) } +// catalogueFactsOf collects the catalogue fields the product table has no +// column for, so an import keeps them instead of leaving them behind. +// +// Only what the catalogue actually stated: an empty field is omitted rather +// than written as `""` or `[]`, so a reader can tell "the catalogue did not say" +// from "the catalogue said none". The drawer prints a row per fact and an empty +// string would print an empty row. +// +// The keys are the catalogue's own wire names. They are what the console +// already reads off a live catalogue row, so the same rendering works against +// either source without a translation layer in between. +func catalogueFactsOf(p *models.CatalogueProduct) map[string]any { + facts := map[string]any{} + if p == nil { + return facts + } + + put := func(key, value string) { + if v := strings.TrimSpace(value); v != "" { + facts[key] = v + } + } + putList := func(key string, values []string) { + kept := make([]string, 0, len(values)) + for _, v := range values { + if t := strings.TrimSpace(v); t != "" { + kept = append(kept, t) + } + } + if len(kept) > 0 { + facts[key] = kept + } + } + + put("title", p.Title) + put("category", p.Category) + put("variant_key", p.VariantKey) + put("sku_source", p.SKUSource) + put("price_range", p.PriceRange) + put("fssai_license", p.FSSAILicense) + put("search_query", p.SearchQuery) + putList("providers", p.Providers) + putList("highlights", p.Highlights) + putList("nutrients", p.Nutrients) + + return facts +} + // ImportCatalogueProduct bridges a global catalogue product (CatalogueDB) into // a tenant's own store catalogue: it snapshots the catalogue product into the // tenant's `products` table on first import (keyed on brand+catalogueid so @@ -417,6 +486,27 @@ func (s *productService) ImportCatalogueProduct(reqs []models.ImportCataloguePro Taxpercent: req.Taxpercent, Approve: 1, } + // Everything the snapshot has no column for, kept as the catalogue + // stated it. + // + // Ten of the catalogue's eighteen fields used to stop here. Two of + // them SHOULD — `category` is remapped to the platform's own + // categoryid, and `price_range` is replaced by the price the shop + // sets — but they are kept anyway, because what other retailers + // charge is the most useful thing on the drawer when somebody is + // deciding what to charge, and the catalogue's own category is how + // a mis-filed product gets noticed. + // + // Encoding failure is swallowed, like the images below: the product + // is worth creating without its facts, and refusing an import over + // a nutrition line would be the wrong trade. + if encoded, err := json.Marshal(catalogueFactsOf(catalogueProduct)); err == nil { + snapshot.Cataloguefacts = string(encoded) + } else { + log.Printf("import: could not encode catalogue facts for %s/%d: %v", + req.Brand, req.Catalogueid, err) + } + if len(catalogueProduct.Images) > 0 { // The first stays where every reader already looks for it. snapshot.Productimage = catalogueProduct.Images[0] diff --git a/services/productVisibility_test.go b/services/productVisibility_test.go index b3d8b74..158e5ec 100644 --- a/services/productVisibility_test.go +++ b/services/productVisibility_test.go @@ -1,6 +1,7 @@ package services import ( + "errors" "testing" "nearle/models" @@ -47,6 +48,10 @@ type fakeProductRepo struct { publishedRefs []models.ProductLocationRef created []models.Products categorySet map[int][2]int // productid -> {categoryid, subcategoryid} + + // Set to make the insert fail, for the tests that check a failed create + // does not hand back a half-made product. + createErr error } func newFakeRepo() *fakeProductRepo { @@ -119,6 +124,9 @@ func (f *fakeProductRepo) UpdateProductCategory(productid, categoryid, subcatego // re-import branch, so this only became reachable when publishing did. func (f *fakeProductRepo) CreateProductReturningID(product models.Products) (int, error) { f.calls = append(f.calls, "CreateProductReturningID") + if f.createErr != nil { + return 0, f.createErr + } f.created = append(f.created, product) return 9001, nil } @@ -767,3 +775,72 @@ func TestPricingFilterDoesNotDisturbTheCallersSlice(t *testing.T) { t.Error("the caller's slice was modified") } } + +/* ── Creating a product hands back its id ─────────────────────────────────── + * + * `POST /products/create` answered `productid: 0` for every product it created: + * the repository took the struct by value, GORM wrote the generated id onto + * that copy, and the copy went out of scope. The endpoint is the only way to + * create a product, and a product cannot be priced or stocked without its id, + * so every caller had to re-read the tenant's whole catalogue and find its own + * row again by SKU — a column nothing enforces, in an importer that creates + * duplicates by design. + */ + +func TestCreateProductReturnsTheIdTheDatabaseAssigned(t *testing.T) { + repo := &fakeProductRepo{} + svc := NewProductService(repo, &fakeCatalogueService{}) + + created, err := svc.CreateProduct(models.Products{ + Tenantid: 9001, + Productname: "Test Rice 5kg", + Productsku: "TM-RICE-5K", + }) + if err != nil { + t.Fatalf("CreateProduct: %v", err) + } + + // 9001 is what the fake's CreateProductReturningID returns. The point is + // that it reaches the caller at all — it used to be dropped. + if created.Productid != 9001 { + t.Errorf("productid = %d, want 9001 — the id was lost on the way out", created.Productid) + } + + // The rest of the product survives the round trip, because the response is + // what the console shows and what it prices and stocks against. + if created.Productsku != "TM-RICE-5K" || created.Productname != "Test Rice 5kg" { + t.Errorf("the product came back altered: %+v", created) + } +} + +func TestCreateProductGoesThroughTheOneCreatePath(t *testing.T) { + // There were two repository methods doing this same INSERT, one of which + // discarded the id. Only one remains, and this is what pins that: a second + // path would have to be added here to be used at all. + repo := &fakeProductRepo{} + svc := NewProductService(repo, &fakeCatalogueService{}) + + if _, err := svc.CreateProduct(models.Products{Tenantid: 9001}); err != nil { + t.Fatalf("CreateProduct: %v", err) + } + + if len(repo.calls) != 1 || repo.calls[0] != "CreateProductReturningID" { + t.Errorf("want exactly one call to CreateProductReturningID, got %v", repo.calls) + } +} + +func TestAFailedCreateReturnsNoProduct(t *testing.T) { + // The caller prices and stocks against what comes back, so a half-made + // product with a zero id would be worse than an error — it would send a + // price and a stock movement to product 0. + repo := &fakeProductRepo{createErr: errors.New("duplicate key")} + svc := NewProductService(repo, &fakeCatalogueService{}) + + created, err := svc.CreateProduct(models.Products{Tenantid: 9001, Productsku: "DUP"}) + if err == nil { + t.Fatal("a failed insert was reported as a success") + } + if created.Productid != 0 || created.Productsku != "" { + t.Errorf("a product was returned for a failed create: %+v", created) + } +} diff --git a/services/stockrequestService.go b/services/stockrequestService.go index 497fb9e..567da62 100644 --- a/services/stockrequestService.go +++ b/services/stockrequestService.go @@ -1,6 +1,8 @@ package services import ( + "errors" + "nearle/models" "nearle/repositories" "time" @@ -22,6 +24,21 @@ func NewStockRequestService(repo repositories.StockRequestRepository, productSer } func (s *stockRequestService) CreateStockRequest(req *models.StockRequest) error { + // A request for nothing is not a request. + // + // Nothing downstream rejected it, so a branch could raise a request for + // zero units and it sat in the admin's queue looking exactly like a real + // one — and approving it moved no stock, which reads as the ledger being + // broken rather than the request being empty. A negative would move stock + // the wrong way on receipt, since UpdateStockRequest writes Qty straight + // into the ledger as an 'in'. + // + // Returned as an ordinary error: the controller already reports per-item + // reasons, so one bad row in a batch is named and the rest still land. + if req.Qty <= 0 { + return errors.New("quantity must be more than zero") + } + return s.repo.CreateStockRequest(req) }