guide changes

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
+}

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")
+	}
+}

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
+}

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")
+	}
+}

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
+}

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)
+	}
+}

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)
+}