shifts

controllers/posController.go

@@ -771,6 +771,20 @@ func posClaimError(c *fiber.Ctx, err error) error {
 // once the console can hold a session.
 
 // posWebScope reads and checks the tenant and outlet a console request names.
+// posTenantScope is the guard for things that belong to a whole business
+// rather than to one of its shops — shift windows, so far.
+//
+// No ownership query, because there is nothing to own: `middleware.WebAuth`
+// pins the tenant from the signed session and refuses a request naming another
+// one, so reaching here with a tenant id at all means it is this caller's.
+// Naming an outlet is what needs checking, and that is `posWebScope` below.
+func (ctl *PosController) posTenantScope(tenantID int) error {
+	if tenantID <= 0 {
+		return fmt.Errorf("tenantid is required")
+	}
+	return nil
+}
+
 func (ctl *PosController) posWebScope(tenantID, locationID int) error {
 	if tenantID <= 0 {
 		return fmt.Errorf("tenantid is required")
@@ -921,9 +935,18 @@ func (ctl *PosController) WebListStaffShifts(c *fiber.Ctx) error {
 	tenantID, _ := strconv.Atoi(strings.TrimSpace(c.Query("tenantid")))
 	locationID, _ := strconv.Atoi(strings.TrimSpace(c.Query("locationid")))
 
-	if err := ctl.posWebScope(tenantID, locationID); err != nil {
+	// Tenant-scoped, because a shift belongs to the business rather than to one
+	// of its shops. An outlet may still be named to narrow the list, and is
+	// checked for ownership when it is — omitting it is not a way to read
+	// somebody else's, because the tenant comes from the signed session.
+	if err := ctl.posTenantScope(tenantID); err != nil {
 		return posBadRequest(c, err)
 	}
+	if locationID > 0 {
+		if err := ctl.posWebScope(tenantID, locationID); err != nil {
+			return posBadRequest(c, err)
+		}
+	}
 
 	shifts, err := ctl.posService.ListStaffShifts(tenantID, locationID,
 		strings.EqualFold(c.Query("include_inactive"), "true"))
@@ -944,9 +967,19 @@ func (ctl *PosController) WebCreateStaffShift(c *fiber.Ctx) error {
 		return posBadRequest(c, fmt.Errorf("invalid request body"))
 	}
 
-	if err := ctl.posWebScope(req.Tenantid, req.Locationid); err != nil {
+	// A shift with no outlet belongs to the tenant and every branch it owns,
+	// which is the ordinary case — a business that works 07:00–15:00 works
+	// those hours at every shop, and entering them per outlet is how the third
+	// branch quietly ends up on 07:00–15:30. An outlet is named only when one
+	// shop really does differ, and is checked for ownership then.
+	if err := ctl.posTenantScope(req.Tenantid); err != nil {
 		return posBadRequest(c, err)
 	}
+	if req.Locationid > 0 {
+		if err := ctl.posWebScope(req.Tenantid, req.Locationid); err != nil {
+			return posBadRequest(c, err)
+		}
+	}
 
 	shift, err := ctl.posService.CreateStaffShift(req.Tenantid, req.Locationid, req)
 	if err != nil {

repositories/posShiftRepository.go

@@ -36,21 +36,42 @@ func normaliseShiftTime(raw string) (string, error) {
 	return t, nil
 }
 
-// ListStaffShifts returns an outlet's shifts, newest last so a picker reads in
-// the order they were created rather than alphabetically by name.
+// ListStaffShifts returns the shifts a tenant's staff can be put on, newest
+// last so a picker reads in the order they were created rather than
+// alphabetically by name.
+//
+// ── Why the outlet is optional ──────────────────────────────────────────────
+//
+// A shift is a fact about how a BUSINESS runs, not about one shop: a tenant
+// that works 07:00–15:00 and 15:00–23:00 works those hours at every branch it
+// owns, and making somebody re-enter them per outlet guarantees the third
+// branch gets 07:00–15:30 and nobody notices. `locationid = 0` is a shift that
+// belongs to the whole tenant.
+//
+// Branch-specific rows are still honoured, because they already exist and a
+// tenant may genuinely run one outlet differently. Asking for an outlet returns
+// the tenant's shifts AND that outlet's own; asking for none returns everything
+// the tenant has.
 func (r *posRepository) ListStaffShifts(tenantID, locationID int, includeInactive bool) ([]models.StaffShifts, error) {
-	if tenantID <= 0 || locationID <= 0 {
-		return nil, fmt.Errorf("tenantid and locationid are required")
+	if tenantID <= 0 {
+		return nil, fmt.Errorf("tenantid is required")
 	}
 
 	shifts := make([]models.StaffShifts, 0)
-	query := `SELECT * FROM staffshifts WHERE tenantid = ? AND locationid = ?`
+	args := []any{tenantID}
+
+	query := `SELECT * FROM staffshifts WHERE tenantid = ?`
+	if locationID > 0 {
+		// The tenant-wide ones and this outlet's, never another outlet's.
+		query += ` AND (COALESCE(locationid, 0) = 0 OR locationid = ?)`
+		args = append(args, locationID)
+	}
 	if !includeInactive {
 		query += ` AND LOWER(COALESCE(status,'active')) <> 'inactive'`
 	}
 	query += ` ORDER BY staffshiftid`
 
-	if err := r.db.Raw(query, tenantID, locationID).Scan(&shifts).Error; err != nil {
+	if err := r.db.Raw(query, args...).Scan(&shifts).Error; err != nil {
 		return nil, err
 	}
 	return shifts, nil
@@ -58,8 +79,14 @@ func (r *posRepository) ListStaffShifts(tenantID, locationID int, includeInactiv
 
 // CreateStaffShift adds a window at one outlet.
 func (r *posRepository) CreateStaffShift(tenantID, locationID int, req models.StaffShifts) (*models.StaffShifts, error) {
-	if tenantID <= 0 || locationID <= 0 {
-		return nil, fmt.Errorf("tenantid and locationid are required")
+	if tenantID <= 0 {
+		return nil, fmt.Errorf("tenantid is required")
+	}
+	// `locationid = 0` is deliberate and is now the ordinary case: the shift
+	// belongs to the tenant and every branch it owns can use it. An outlet is
+	// only named when one shop really does run different hours.
+	if locationID < 0 {
+		locationID = 0
 	}
 
 	name := strings.TrimSpace(req.Name)

repositories/posShift_test.go

@@ -0,0 +1,54 @@
+package repositories
+
+import (
+	"strings"
+	"testing"
+)
+
+/*
+A shift belongs to a business, not to one of its shops.
+
+Both halves of this used to demand an outlet, so the same two windows had to be
+re-entered at every branch a tenant owns — which is how the third branch quietly
+gets 07:00–15:30 and nobody notices until a cashier is filed under hours that do
+not exist. A tenant that works 07:00–15:00 works those hours everywhere.
+
+`locationid = 0` is now the ordinary case. A named outlet still works, because
+one shop may genuinely run differently and those rows already exist.
+*/
+
+func TestATimeIsAcceptedInBothFormsTheClientsSend(t *testing.T) {
+	// `<input type="time">` gives "07:00"; some browsers and every hand-typed
+	// value give "07:00:00"; `ridershifts` stores the seconds form already.
+	for _, tc := range []struct{ in, want string }{
+		{"07:00", "07:00"},
+		{"07:00:00", "07:00"},
+		{" 23:59 ", "23:59"},
+		{"00:00", "00:00"},
+	} {
+		got, err := normaliseShiftTime(tc.in)
+		if err != nil {
+			t.Fatalf("%q: %v", tc.in, err)
+		}
+		if got != tc.want {
+			t.Fatalf("%q became %q, want %q", tc.in, got, tc.want)
+		}
+	}
+}
+
+func TestSomethingThatIsNotATimeOfDayIsRefused(t *testing.T) {
+	for _, bad := range []string{"", "  ", "7:00", "24:00", "07:60", "morning", "07", "7pm"} {
+		if _, err := normaliseShiftTime(bad); err == nil {
+			t.Fatalf("%q was accepted as a time of day", bad)
+		}
+	}
+}
+
+func TestTheTimeErrorSaysWhatShapeIsWanted(t *testing.T) {
+	// "invalid" tells somebody nothing. The message names the format, because
+	// the most common wrong answer is a valid time in the wrong notation.
+	_, err := normaliseShiftTime("7pm")
+	if err == nil || !strings.Contains(err.Error(), "HH:MM") {
+		t.Fatalf("the refusal does not say what to type: %v", err)
+	}
+}