From 485f31239d3cab7d81c35e10120fdaedac77aa42 Mon Sep 17 00:00:00 2001 From: abhishek Date: Tue, 1 Sep 2026 12:01:43 +0530 Subject: [PATCH] guide changes --- controllers/tenantController.go | 132 ++++++++++++++++++++++- models/tenant.go | 17 ++- repositories/tenantRepository.go | 173 ++++++++++++++++++++++++++++++- routes/tenantroutes.go | 36 +++++++ services/ownProfile.go | 54 ++++++++++ services/ownProfile_test.go | 54 ++++++++++ services/staffAssignment.go | 29 ++++++ services/staffAssignment_test.go | 54 ++++++++++ services/tenantProfile.go | 75 ++++++++++++++ services/tenantProfile_test.go | 100 ++++++++++++++++++ services/tenantService.go | 27 +++++ 11 files changed, 746 insertions(+), 5 deletions(-) create mode 100644 services/ownProfile.go create mode 100644 services/ownProfile_test.go create mode 100644 services/staffAssignment.go create mode 100644 services/staffAssignment_test.go create mode 100644 services/tenantProfile.go create mode 100644 services/tenantProfile_test.go diff --git a/controllers/tenantController.go b/controllers/tenantController.go index d2016a1..0a47ad0 100644 --- a/controllers/tenantController.go +++ b/controllers/tenantController.go @@ -444,7 +444,7 @@ func (ctl *TenantController) CreateTenantUser(c *fiber.Ctx) error { func (ctl *TenantController) GetTenantInfo(c *fiber.Ctx) error { log.Printf("[DEBUG] GetTenantInfo OriginalURL: %s, Headers: %v", c.OriginalURL(), c.GetReqHeaders()) - + // Parse tenant ID tidStr := c.Query("tenantid") if tidStr == "" { @@ -580,3 +580,133 @@ func (ctl *TenantController) GetTenantByKeyword(c *fiber.Ctx) error { "details": data, }) } + +// AssignStaff moves one of a merchant's people to a branch, or takes them off. +// +// `locationid` 0 unassigns, and is a real instruction rather than a missing +// value — somebody can leave a shop before the next one opens, and the console +// needs a way to say that which is not "delete the account". +// +// The tenant comes from the request and every check is scoped to it in the +// query, so a userid belonging to another business matches nothing and the call +// fails rather than moving a stranger's staff. +func (ctl *TenantController) AssignStaff(c *fiber.Ctx) error { + var input struct { + Tenantid int `json:"tenantid"` + Userid int `json:"userid"` + Locationid int `json:"locationid"` + Unassign bool `json:"unassign"` + } + if err := c.BodyParser(&input); err != nil { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, "message": "Invalid input", + }) + } + if input.Tenantid <= 0 || input.Userid <= 0 { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, + "message": "tenantid and userid are required", + }) + } + + // The rule lives in services.ResolveAssignment so it can be tested without + // a request: a zero locationid must never be read as "unassign", because a + // dropped field looks exactly like one. + location, err := services.ResolveAssignment(input.Locationid, input.Unassign) + if err != nil { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, "message": err.Error(), + }) + } + + if err := ctl.tenantService.AssignStaffToBranch(input.Tenantid, input.Userid, location); err != nil { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, "message": err.Error(), + }) + } + return c.JSON(fiber.Map{"code": 200, "status": true, "message": "Success"}) +} + +// UpdateTenantProfile lets a merchant change their own business record. +// +// The body is read as a free-form map rather than into `models.Tenants`, +// deliberately. Binding to the struct would make every column on the table a +// candidate for writing and leave "which of these may a merchant set?" answered +// by whichever fields a form happened to send. The allowlist in +// services.TenantProfileUpdate answers it in one place instead, and everything +// absent from a request is left alone rather than blanked. +func (ctl *TenantController) UpdateTenantProfile(c *fiber.Ctx) error { + fields := map[string]any{} + if err := c.BodyParser(&fields); err != nil { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, "message": "Invalid input", + }) + } + + // The row to write is named by `tenantid`, and it is the one value in the + // body that is never a value to write. + tenantID := 0 + switch id := fields["tenantid"].(type) { + case float64: + tenantID = int(id) + case int: + tenantID = id + } + if tenantID <= 0 { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, + "message": "tenantid is required", + }) + } + + if err := ctl.tenantService.UpdateTenantProfile(tenantID, fields); err != nil { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, "message": err.Error(), + }) + } + return c.JSON(fiber.Map{"code": 200, "status": true, "message": "Success"}) +} + +// UpdateOwnProfile lets somebody change their own name, mobile or email. +// +// Read as a map rather than into `models.User` for the same reason as the shop +// profile: `app_users` keeps identity next to authorisation, so binding to the +// struct would make `roleid`, `locationid`, `status`, `password` and `pin` +// candidates for writing. The allowlist in services.OwnProfileUpdate answers +// "what may a person change about themselves?" in one place. +// +// Scoped by userid AND tenantid — the existing `users/update` checks only the +// userid, which is why the store user's account page has been read-only rather +// than editable. +func (ctl *TenantController) UpdateOwnProfile(c *fiber.Ctx) error { + fields := map[string]any{} + if err := c.BodyParser(&fields); err != nil { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, "message": "Invalid input", + }) + } + + readID := func(key string) int { + switch id := fields[key].(type) { + case float64: + return int(id) + case int: + return id + } + return 0 + } + userID, tenantID := readID("userid"), readID("tenantid") + if userID <= 0 || tenantID <= 0 { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, + "message": "userid and tenantid are both required", + }) + } + + if err := ctl.tenantService.UpdateOwnProfile(userID, tenantID, fields); err != nil { + return c.Status(http.StatusBadRequest).JSON(fiber.Map{ + "code": http.StatusBadRequest, "status": false, "message": err.Error(), + }) + } + return c.JSON(fiber.Map{"code": 200, "status": true, "message": "Success"}) +} diff --git a/models/tenant.go b/models/tenant.go index 88934f0..1f1ba2b 100644 --- a/models/tenant.go +++ b/models/tenant.go @@ -68,6 +68,21 @@ type Tenantlocations struct { Deliverymins int `json:"deliverymins"` Cancelsecs int `json:"cancelsecs"` Status string `json:"status" gorm:"default:Active"` + + // Who will run this outlet, when the caller already has somebody in mind. + // + // `gorm:"-"` because it is not a column — it names an existing `app_users` + // row to bind to the new branch instead of spawning a fresh login. + // + // Spawning was the only option, and it produced an account named after the + // SHOP, on the SHOP's email address, one per outlet. Two people behind the + // same counter shared one credential and nothing recorded which of them did + // anything. Naming a person here is what lets a merchant decide who runs a + // branch, and hire that person before the branch exists. + // + // Zero keeps the old behaviour exactly, so every existing caller is + // unaffected. + Operatorid int `json:"operatorid" gorm:"-"` } type Tenantslot struct { @@ -120,7 +135,7 @@ type Tenantpricing struct { } type StaffInfo struct { - Userid int `json:"userid"` + Userid int `json:"userid"` // What the role is called, so a console does not have to map ids itself. // `app_roles` holds six rows for four back-office roles and most accounts // carry an id absent from it, so any mapping written client-side is wrong. diff --git a/repositories/tenantRepository.go b/repositories/tenantRepository.go index 7d58a4a..c7bc24c 100644 --- a/repositories/tenantRepository.go +++ b/repositories/tenantRepository.go @@ -22,8 +22,11 @@ type TenantRepository interface { UpdateLocation(input models.Tenantlocations) error CreateLocation(data models.Tenantlocations) error DeleteLocation(locationid int, tenantid int) error + UpdateTenantProfile(tenantID int, fields map[string]any) error + UpdateOwnProfile(userID, tenantID int, fields map[string]any) error GetStaffs(tid int) ([]models.StaffInfo, error) CreateStaff(user models.User) error + AssignStaffToBranch(tenantID, userID, locationID int) error UpdateStaff(user models.User) error CreateTenantLocation(data models.Tenantlocations) (models.Tenantlocations, error) UpdateTenantLocation(data models.Tenantlocations) error @@ -312,6 +315,18 @@ func (r *tenantRepository) CreateLocation(data models.Tenantlocations) error { return nil } +// GetStaffs lists a merchant's people, INCLUDING the ones not yet given a +// branch. +// +// The join was INNER, which excluded exactly the state this list exists to +// show. A person hired before their outlet opens — or moved off a branch, or +// created and not yet placed — has `locationid` 0, matches no `tenantlocations` +// row, and vanished from the only screen that could assign them one. They could +// sign in (and were met with "No store assigned"), they simply could not be +// seen by the person able to fix it. +// +// LEFT, and unassigned first: they are the ones needing an action, and a list +// sorted by branch buries them under everybody already settled. func (r *tenantRepository) GetStaffs(tid int) ([]models.StaffInfo, error) { var data []models.StaffInfo @@ -323,10 +338,11 @@ func (r *tenantRepository) GetStaffs(tid int) ([]models.StaffInfo, error) { b.locationname, COALESCE(c.rolename,'') AS rolename FROM app_users a - INNER JOIN tenantlocations b ON a.locationid = b.locationid + LEFT JOIN tenantlocations b ON a.locationid = b.locationid LEFT JOIN app_roles c ON c.roleid = a.roleid WHERE a.tenantid = ? - AND COALESCE(a.roleid, 0) NOT IN (7, 8)` + AND COALESCE(a.roleid, 0) NOT IN (7, 8) + ORDER BY a.locationid NULLS FIRST, a.userid DESC` if err := r.db.Raw(q1, tid).Scan(&data).Error; err != nil { return nil, err @@ -390,6 +406,21 @@ func (r *tenantRepository) CreateTenantLocation(data models.Tenantlocations) (mo data.Status = "Active" } + // An outlet nobody can sign in to is a dead end, and a silent one — it + // appears in every list and every branch picker, and the first person to + // notice is whoever is standing in the shop. + // + // So a branch must arrive with an operator, one way or the other: an + // existing person named in Operatorid, or an email to spawn a login from. + // Neither used to be checked, and a create with a blank email produced an + // account whose authname was the empty string — a row that can never + // authenticate. + if data.Operatorid <= 0 && strings.TrimSpace(data.Email) == "" { + tx.Rollback() + return models.Tenantlocations{}, errors.New( + "a branch needs somebody to run it: name an existing user in operatorid, or give an email to create a login from") + } + // Step 1: Insert into tenantlocations. GORM writes the DB-assigned // locationid back onto data, which callers need to build the store's // QR code (payload is just {tenantid, locationid}) right after onboarding. @@ -398,7 +429,42 @@ func (r *tenantRepository) CreateTenantLocation(data models.Tenantlocations) (mo return models.Tenantlocations{}, err } - // Step 2: Insert into app_users + // Step 2a: bind an existing person, when one was named. + // + // Scoped to this tenant in the WHERE clause rather than checked first: a + // userid belonging to another merchant then matches no row, and the branch + // is refused rather than handed to a stranger. Doing it as one guarded + // UPDATE also means the check and the write cannot drift apart under a + // concurrent reassignment. + if data.Operatorid > 0 { + res := tx.Table("app_users"). + Where("userid = ? AND tenantid = ? AND COALESCE(roleid, 0) NOT IN (7, 8)", + data.Operatorid, data.Tenantid). + Updates(map[string]any{"locationid": data.Locationid}) + if res.Error != nil { + tx.Rollback() + return models.Tenantlocations{}, res.Error + } + if res.RowsAffected == 0 { + // Either the person does not exist, belongs to another merchant, or + // is a till account. All three are the same answer to the caller, + // and none of them should leave a branch standing. + tx.Rollback() + return models.Tenantlocations{}, fmt.Errorf( + "user %d cannot run this branch — they belong to another business, do not exist, or are a till account", + data.Operatorid) + } + if err := tx.Commit().Error; err != nil { + return models.Tenantlocations{}, err + } + return data, nil + } + + // Step 2b: no person named — spawn a login, as before. + // + // Kept so every existing caller behaves exactly as it did. The account it + // makes is named after the shop and sits on the shop's email, which is why + // naming a real person above is the better path where the caller has one. user.Authname = data.Email user.Firstname = data.Locationname user.Email = data.Email @@ -754,3 +820,104 @@ func (r *tenantRepository) GetTenantByKeyword(keyword string) ([]models.TenantSe return data, nil } + +// AssignStaffToBranch moves one of a merchant's people to a branch, or takes +// them off one. +// +// `locationid` of 0 unassigns — a real state, not a missing value. Somebody +// leaves a shop before the next one opens, and the alternative to holding them +// unassigned is deleting the account and losing who did what. +// +// Both the person and the branch are checked against the tenant IN THE QUERY +// rather than beforehand. A userid from another merchant then matches no row +// and the call fails, instead of one business quietly moving another's staff — +// and the check cannot drift from the write under a concurrent edit. +// +// Till accounts (roleids 7 and 8) are excluded for the same reason GetStaffs +// hides them: they are POS people with no back-office screen, and their branch +// is managed by the till console, not here. +func (r *tenantRepository) AssignStaffToBranch(tenantID, userID, locationID int) error { + if locationID > 0 { + var owned int64 + if err := r.db.Raw( + `SELECT COUNT(1) FROM tenantlocations WHERE locationid = ? AND tenantid = ?`, + locationID, tenantID).Scan(&owned).Error; err != nil { + return err + } + if owned == 0 { + return fmt.Errorf("branch %d does not belong to this business", locationID) + } + } + + res := r.db.Table("app_users"). + Where("userid = ? AND tenantid = ? AND COALESCE(roleid, 0) NOT IN (7, 8)", + userID, tenantID). + Updates(map[string]any{"locationid": locationID}) + if res.Error != nil { + return res.Error + } + if res.RowsAffected == 0 { + return fmt.Errorf( + "user %d is not one of this business's people", userID) + } + return nil +} + +// UpdateTenantProfile writes a merchant's own business record. +// +// The FIRST write path this table has ever had. Every field on `tenants` was +// set once at onboarding by a Nearle Admin and could never be changed again, by +// anybody — which is why, across 200 merchants, 18 had a shop photograph and +// none had a licence number. +// +// `fields` has already been reduced to the merchant-editable columns by +// services.TenantProfileUpdate. This deliberately does not take a struct: GORM +// would then decide what to write from which values happen to be non-zero, and +// the set of columns a merchant may touch would be implied by a form rather +// than stated anywhere. +func (r *tenantRepository) UpdateTenantProfile(tenantID int, fields map[string]any) error { + if tenantID <= 0 { + return errors.New("tenantid is required") + } + if len(fields) == 0 { + return errors.New("nothing to update") + } + res := r.db.Table("tenants").Where("tenantid = ?", tenantID).Updates(fields) + if res.Error != nil { + return res.Error + } + if res.RowsAffected == 0 { + return fmt.Errorf("no business with tenantid %d", tenantID) + } + return nil +} + +// UpdateOwnProfile writes the fields a person owns about themselves. +// +// Scoped by userid AND tenantid together, in the WHERE clause. `UpdateStaff` +// checks only the userid, so a request naming somebody else's account is +// carried out — which is survivable while the only caller is an admin screen, +// and is not once a person can edit their own profile. +// +// `fields` has already been reduced by services.OwnProfileUpdate to identity +// columns. Role, branch, tenant, status, password and PIN are not in it: this +// table keeps who-you-are next to what-you-may-do, and only the first half +// belongs to the person. +func (r *tenantRepository) UpdateOwnProfile(userID, tenantID int, fields map[string]any) error { + if userID <= 0 || tenantID <= 0 { + return errors.New("userid and tenantid are both required") + } + if len(fields) == 0 { + return errors.New("nothing to update") + } + res := r.db.Table("app_users"). + Where("userid = ? AND tenantid = ?", userID, tenantID). + Updates(fields) + if res.Error != nil { + return res.Error + } + if res.RowsAffected == 0 { + return fmt.Errorf("no account %d in this business", userID) + } + return nil +} diff --git a/routes/tenantroutes.go b/routes/tenantroutes.go index b9ea753..b0f6e56 100644 --- a/routes/tenantroutes.go +++ b/routes/tenantroutes.go @@ -22,6 +22,37 @@ func RegisterTenantRoutes(api fiber.Router, f *facade.Facade) { tenant.Put("/updatetenantlocation", f.TenantController.UpdateTenantLocation) tenant.Post("/createtenantuser", f.TenantController.CreateTenantUser) + // One business, by id. + // + // Also /mob-only until now, so the console's only way to read its own + // merchant was `getalltenants` — 262 rows, paginated, and a shop on page + // two was simply not found. A profile screen cannot be built on that. + tenant.Get("/gettenantinfo", f.TenantController.GetTenantInfo) + + // A merchant editing their own business record. The first write path the + // `tenants` table has ever had — see UpdateTenantProfile for what may be + // set, and what deliberately may not. + tenant.Put("/updatetenant", f.TenantController.UpdateTenantProfile) + + // Somebody editing their own name, mobile or email. Scoped to their own + // account AND their own business — `users/update` checks neither, which is + // why the store user's account page could only ever be read-only. + tenant.Put("/updateownprofile", f.TenantController.UpdateOwnProfile) + + // A merchant's people, and where each of them works. + // + // These existed only under /mob, which is why the console has never had a + // screen for them: back-office staff were reachable exclusively through the + // customer app's door. The /mob registrations stay — something may be + // calling them — but this is where they belong. + tenant.Get("/getstaffs", f.TenantController.GetStaffs) + tenant.Post("/createstaff", f.TenantController.CreateStaff) + + // Placing a person at a branch, or taking them off one. Separate from + // createstaff because hiring and posting are different decisions, and the + // second happens repeatedly over an account's life. + tenant.Put("/assignstaff", f.TenantController.AssignStaff) + tenant = api.Group("/v1/mob/tenants") tenant.Get("/gettenantslot", f.TenantController.GetTenantSlot) @@ -34,6 +65,11 @@ func RegisterTenantRoutes(api fiber.Router, f *facade.Facade) { tenant.Post("/createlocation", f.TenantController.CreateLocation) tenant.Get("/getstaffs", f.TenantController.GetStaffs) tenant.Post("/createstaff", f.TenantController.CreateStaff) + + // Placing a person at a branch, or taking them off one. Separate from + // createstaff because hiring and posting are different decisions, and the + // second happens again and again over an account's life. + tenant.Put("/assignstaff", f.TenantController.AssignStaff) tenant.Post("/createtenantuser", f.TenantController.CreateTenantUser) tenant.Get("/gettenantinfo", f.TenantController.GetTenantInfo) diff --git a/services/ownProfile.go b/services/ownProfile.go new file mode 100644 index 0000000..fbbed8b --- /dev/null +++ b/services/ownProfile.go @@ -0,0 +1,54 @@ +package services + +import ( + "errors" + "strings" +) + +// What a person may change about their OWN account. +// +// Deliberately short, and short for a reason. `app_users` carries the columns +// that decide what somebody is allowed to do — `roleid`, `locationid`, +// `tenantid`, `status`, `password`, `pin` — beside the ones that merely say who +// they are. `PUT /users/update` writes whatever struct it is handed and checks +// only `userid`, so before this a self-service profile form would have let a +// branch user promote themselves, move to another shop, or reactivate a +// disabled account. +// +// Identity here, authorisation elsewhere. Moving somebody between branches is +// AssignStaffToBranch, and it is the store admin's call — which is the whole +// point of the hiring order: who runs a shop is decided by the merchant, not by +// the person who works there. +var editableOwnFields = map[string]bool{ + "firstname": true, + "lastname": true, + "contactno": true, + "email": true, +} + +// OwnProfileUpdate reduces a request to the fields a person owns about +// themselves. +// +// Errors when nothing survives rather than reporting a successful write of +// nothing: somebody who changed only their role would otherwise be told it +// saved. +func OwnProfileUpdate(fields map[string]any) (map[string]any, error) { + clean := make(map[string]any, len(fields)) + for key, value := range fields { + lower := strings.ToLower(strings.TrimSpace(key)) + if !editableOwnFields[lower] { + continue + } + // Blank means "not supplied". A profile form posts every field it + // renders, so honouring blanks would let one save wipe a mobile number + // the person never touched. + if text, ok := value.(string); ok && strings.TrimSpace(text) == "" { + continue + } + clean[lower] = value + } + if len(clean) == 0 { + return nil, errors.New("nothing to update — no editable field was supplied") + } + return clean, nil +} diff --git a/services/ownProfile_test.go b/services/ownProfile_test.go new file mode 100644 index 0000000..a03165a --- /dev/null +++ b/services/ownProfile_test.go @@ -0,0 +1,54 @@ +package services + +import "testing" + +/* +`app_users` keeps identity and authorisation in one table, so a self-service +profile form is one careless `Updates(&struct)` away from letting a branch user +promote themselves. + +`PUT /users/update` already writes whatever it is handed and checks only +`userid` — no tenant, no role guard — which is precisely why the store user's +account page has been read-only rather than editable. +*/ + +func TestAPersonCannotPromoteOrMoveThemselves(t *testing.T) { + clean, err := OwnProfileUpdate(map[string]any{ + "firstname": "Suriya", + "roleid": 1, + "locationid": 1166, + "tenantid": 9, + "status": "Active", + "password": "hunter2", + "pin": 1234, + }) + if err != nil { + t.Fatalf("OwnProfileUpdate: %v", err) + } + for _, forbidden := range []string{"roleid", "locationid", "tenantid", "status", "password", "pin"} { + if _, present := clean[forbidden]; present { + t.Errorf("%q survived the allowlist", forbidden) + } + } + if clean["firstname"] != "Suriya" { + t.Errorf("the legitimate change was dropped: %+v", clean) + } +} + +func TestARequestOfNothingButPrivilegeIsRefused(t *testing.T) { + if _, err := OwnProfileUpdate(map[string]any{"roleid": 1, "status": "Active"}); err == nil { + t.Fatal("a request that changes only privilege was accepted") + } +} + +// A form sends every field it renders. If blank meant erase, saving one change +// would wipe the mobile number nobody touched. +func TestABlankDoesNotEraseAField(t *testing.T) { + clean, err := OwnProfileUpdate(map[string]any{"firstname": "Suriya", "contactno": " "}) + if err != nil { + t.Fatalf("OwnProfileUpdate: %v", err) + } + if _, present := clean["contactno"]; present { + t.Error("a whitespace-only value was treated as a change") + } +} diff --git a/services/staffAssignment.go b/services/staffAssignment.go new file mode 100644 index 0000000..cb256d4 --- /dev/null +++ b/services/staffAssignment.go @@ -0,0 +1,29 @@ +package services + +import "errors" + +// Where a staff assignment request is actually asking to put somebody. +// +// Its own function because the dangerous case is a quiet one. Unassigning is a +// real instruction — people leave a shop before the next one opens, and the +// alternative to holding them unassigned is deleting the account and losing who +// did what — but it looks exactly like a `locationid` that failed to arrive. +// A JSON body missing the field, a form that posted a blank, a client that +// dropped it: all of them present as 0. +// +// So 0 alone never unassigns. `unassign` has to be sent, deliberately, and a +// request that names no branch and does not ask to unassign is refused rather +// than guessed at. +func ResolveAssignment(locationID int, unassign bool) (int, error) { + if unassign { + // An explicit request wins even if a branch was also sent — the caller + // said what they wanted, and honouring the leftover id instead would be + // the same silent guess in reverse. + return 0, nil + } + if locationID <= 0 { + return 0, errors.New( + "give a locationid, or send unassign:true to take them off a branch") + } + return locationID, nil +} diff --git a/services/staffAssignment_test.go b/services/staffAssignment_test.go new file mode 100644 index 0000000..e0bbef0 --- /dev/null +++ b/services/staffAssignment_test.go @@ -0,0 +1,54 @@ +package services + +import "testing" + +/* +Assignment has one failure mode worth defending against, and it is silent. + +Taking somebody off a branch is a legitimate thing to do, and it is expressed as +`locationid` 0. But a body that lost the field, a form that posted a blank, or a +client that dropped it all arrive as 0 too — so inferring "unassign" from the +zero would let a network hiccup quietly turn somebody out of their shop. The +person would keep signing in and keep being told "No store assigned", and +nothing would say why. +*/ + +func TestUnassigningMustBeAskedForExplicitly(t *testing.T) { + if _, err := ResolveAssignment(0, false); err == nil { + t.Fatal("a missing locationid was treated as a request to unassign") + } +} + +func TestAnExplicitUnassignIsHonoured(t *testing.T) { + location, err := ResolveAssignment(0, true) + if err != nil { + t.Fatalf("an explicit unassign was refused: %v", err) + } + if location != 0 { + t.Fatalf("want 0, got %d", location) + } +} + +// The caller said what they wanted. Preferring a leftover id would be the same +// silent guess, running the other way. +func TestUnassignBeatsALeftoverBranchId(t *testing.T) { + location, err := ResolveAssignment(1172, true) + if err != nil || location != 0 { + t.Fatalf("want 0 with no error, got %d / %v", location, err) + } +} + +func TestANamedBranchPassesThrough(t *testing.T) { + location, err := ResolveAssignment(1172, false) + if err != nil || location != 1172 { + t.Fatalf("want 1172 with no error, got %d / %v", location, err) + } +} + +// A negative id is not a branch and is not an unassign — it is a bug upstream, +// and it should stop here rather than be rounded into either. +func TestANegativeBranchIdIsRefused(t *testing.T) { + if _, err := ResolveAssignment(-3, false); err == nil { + t.Fatal("a negative locationid was accepted") + } +} diff --git a/services/tenantProfile.go b/services/tenantProfile.go new file mode 100644 index 0000000..bcf4128 --- /dev/null +++ b/services/tenantProfile.go @@ -0,0 +1,75 @@ +package services + +import ( + "errors" + "strings" +) + +// What a merchant is allowed to change about their own business. +// +// An allowlist, and it has to be one. The obvious implementation — hand the +// parsed body to GORM's `Updates` — would let anyone who can reach the endpoint +// set `approved`, `status`, `partnerid`, `partneruserid`, `moduleid`, +// `configid` or `tenanttoken` on their own record: approve themselves onto the +// platform, move themselves under another partner, or reassign their billing. +// None of those belong to the merchant, and none of them are things a UI would +// ever send, which is exactly what makes the omission easy to miss. +// +// So the fields are named here, once, and the repository writes nothing it is +// not given. Anything absent from this map is untouched rather than blanked — +// a profile form that renders four fields must not erase the twenty it did not. +// +// `tenantid` is deliberately absent too: it identifies the row, it is never a +// value to be written. +var editableTenantFields = map[string]bool{ + // What a shopper sees. + "tenantname": true, + "tenantimage": true, + "tenantinfo": true, + + // How to reach the business. + "primaryemail": true, + "primarycontact": true, + "companyname": true, + + // Where it is. + "address": true, + "suburb": true, + "city": true, + "state": true, + "postcode": true, + "latitude": true, + "longitude": true, + + // Legal and trading terms. + "registrationno": true, + "licenseno": true, + "minorder": true, + "subcategoryid": true, +} + +// TenantProfileUpdate reduces a request to the fields a merchant may set. +// +// Returns an error rather than an empty map when nothing survives: a write that +// changes nothing and reports success is indistinguishable from one that +// worked, and the caller would go on believing their licence number was saved. +func TenantProfileUpdate(fields map[string]any) (map[string]any, error) { + clean := make(map[string]any, len(fields)) + for key, value := range fields { + lower := strings.ToLower(strings.TrimSpace(key)) + if !editableTenantFields[lower] { + continue + } + // A blank string is "not supplied", not "erase it". The profile form + // sends every field it renders on every save, so honouring blanks would + // let a half-filled form wipe an address somebody typed last month. + if text, ok := value.(string); ok && strings.TrimSpace(text) == "" { + continue + } + clean[lower] = value + } + if len(clean) == 0 { + return nil, errors.New("nothing to update — no editable field was supplied") + } + return clean, nil +} diff --git a/services/tenantProfile_test.go b/services/tenantProfile_test.go new file mode 100644 index 0000000..65787d3 --- /dev/null +++ b/services/tenantProfile_test.go @@ -0,0 +1,100 @@ +package services + +import "testing" + +/* +The tenants table had no write path at all, so a merchant could never change +their own shop's photo, licence number or contact — measured across 200 tenants: +18 had an image, none had a licence. + +Adding one is where the risk is. `tenants` carries platform-controlled columns +next to merchant-owned ones — `approved`, `status`, `partnerid`, `partneruserid`, +`moduleid`, `configid`, `tenanttoken` — and the obvious implementation, handing +the parsed body to GORM, would let anyone reaching the endpoint approve +themselves onto the platform or move themselves under a different partner. A UI +would never send those fields, which is what makes the hole easy to leave open. +*/ + +func TestAMerchantCannotApproveThemselves(t *testing.T) { + clean, err := TenantProfileUpdate(map[string]any{ + "tenantimage": "https://example.com/shop.jpg", + "approved": 1, + "status": "Active", + }) + if err != nil { + t.Fatalf("TenantProfileUpdate: %v", err) + } + for _, forbidden := range []string{"approved", "status"} { + if _, present := clean[forbidden]; present { + t.Errorf("%q survived the allowlist", forbidden) + } + } + if clean["tenantimage"] != "https://example.com/shop.jpg" { + t.Errorf("the legitimate field was dropped: %+v", clean) + } +} + +func TestOwnershipAndBillingColumnsAreNotEditable(t *testing.T) { + _, err := TenantProfileUpdate(map[string]any{ + "partnerid": 9, + "partneruserid": 9, + "moduleid": 3, + "configid": 2, + "tenanttoken": "stolen", + "tenantid": 1, + }) + // Every field was refused, so nothing is left to write — and that must be + // an error, not a silent success. + if err == nil { + t.Fatal("a request of nothing but forbidden fields was accepted") + } +} + +/* +A profile form sends every field it renders on every save. If a blank meant +"erase", opening the form and saving one change would wipe everything the form +does not show — an address typed last month, a licence added by somebody else. +*/ +func TestABlankFieldIsNotAnInstructionToErase(t *testing.T) { + clean, err := TenantProfileUpdate(map[string]any{ + "licenseno": "12345678901234", + "address": " ", + "city": "", + }) + if err != nil { + t.Fatalf("TenantProfileUpdate: %v", err) + } + if _, present := clean["address"]; present { + t.Error("a whitespace-only value was treated as a change") + } + if _, present := clean["city"]; present { + t.Error("an empty value was treated as a change") + } + if clean["licenseno"] != "12345678901234" { + t.Errorf("the real change was lost: %+v", clean) + } +} + +// Zero is a real minimum order and a real subcategory id is not a string, so +// the blank rule must apply to text only. +func TestANumericZeroIsStillAChange(t *testing.T) { + clean, err := TenantProfileUpdate(map[string]any{"minorder": 0}) + if err != nil { + t.Fatalf("TenantProfileUpdate: %v", err) + } + if value, present := clean["minorder"]; !present || value != 0 { + t.Errorf("a minimum order of zero was dropped: %+v", clean) + } +} + +// Case and stray whitespace in a key are a client's problem, not a reason to +// silently ignore a field the merchant filled in. +func TestFieldNamesAreMatchedLeniently(t *testing.T) { + clean, err := TenantProfileUpdate(map[string]any{" TenantImage ": "x.jpg"}) + if err != nil { + t.Fatalf("TenantProfileUpdate: %v", err) + } + if clean["tenantimage"] != "x.jpg" { + t.Errorf("want the field normalised onto its column, got %+v", clean) + } +} diff --git a/services/tenantService.go b/services/tenantService.go index 19e4226..c51ad6e 100644 --- a/services/tenantService.go +++ b/services/tenantService.go @@ -21,8 +21,11 @@ type TenantService interface { UpdateLocation(input models.Tenantlocations) error CreateLocation(data models.Tenantlocations) error DeleteLocation(locationid int, tenantid int) error + UpdateTenantProfile(tenantID int, fields map[string]any) error + UpdateOwnProfile(userID, tenantID int, fields map[string]any) error GetStaffs(tid int) ([]models.StaffInfo, error) CreateStaff(user models.User) error + AssignStaffToBranch(tenantID, userID, locationID int) error UpdateStaff(user models.User) error CreateTenantLocation(data models.Tenantlocations) map[string]interface{} UpdateTenantLocation(data models.Tenantlocations) map[string]interface{} @@ -229,3 +232,27 @@ func sameAddress(data models.Tenants) bool { strings.EqualFold(strings.TrimSpace(outlet.City), strings.TrimSpace(data.City)) && strings.EqualFold(strings.TrimSpace(outlet.Postcode), strings.TrimSpace(data.Postcode)) } + +func (s *tenantService) AssignStaffToBranch(tenantID, userID, locationID int) error { + return s.repo.AssignStaffToBranch(tenantID, userID, locationID) +} + +// UpdateTenantProfile filters the request down to what a merchant owns, then +// writes it. The allowlist lives in tenantProfile.go with the reasoning. +func (s *tenantService) UpdateTenantProfile(tenantID int, fields map[string]any) error { + clean, err := TenantProfileUpdate(fields) + if err != nil { + return err + } + return s.repo.UpdateTenantProfile(tenantID, clean) +} + +// UpdateOwnProfile filters a self-service edit down to identity fields, then +// writes it scoped to the person's own business. See ownProfile.go. +func (s *tenantService) UpdateOwnProfile(userID, tenantID int, fields map[string]any) error { + clean, err := OwnProfileUpdate(fields) + if err != nil { + return err + } + return s.repo.UpdateOwnProfile(userID, tenantID, clean) +}