Release: promote develop to main (one place decides what a dish costs)

frontend/static/script/js/modules/js/kot.js

@@ -692,44 +692,12 @@ PosnicPro.kot = {
 
                 items.forEach(function(item) {
                     var itemId = item.item_id || '';
+                    /* One sum, in _priceFrom. This path keeps its own inputs:
+                       item_price as a fallback and an untyped tax read as
+                       exclusive - see the note on _priceFrom. */
                     var sellingPrice = parseFloat(item.selling_price || item.item_price || 0);
                     var tax = parseFloat(item.tax || 0);
-                    var taxType = item.tax_type || 'exclusive';
-                    var discountAmount = parseFloat(item.discount_amount || 0);
-                    var discountPercentage = parseFloat(item.discount_percentage || 0);
-                    var finalPrice = 0;
-
-                    // Calculate final price with discounts and taxes
-                    var taxPrice = (sellingPrice * tax) / (100 + tax);
-                    var inclusive_price = sellingPrice - taxPrice;
-
-                    if (discountAmount > 0 && tax > 0) {
-                        var discountValue = (taxType === 'exclusive') ? sellingPrice - discountAmount : inclusive_price - discountAmount;
-                        finalPrice = discountValue + (tax / 100) * discountValue;
-                    } else if (discountPercentage > 0 && tax > 0) {
-                        var discountValue = 0;
-                        var taxValue = 0;
-                        if (taxType === 'exclusive') {
-                            discountValue = (sellingPrice * (discountPercentage / 100));
-                            taxValue = sellingPrice - discountValue;
-                        } else {
-                            discountValue = (inclusive_price * (discountPercentage / 100));
-                            taxValue = inclusive_price - discountValue;
-                        }
-                        finalPrice = taxValue + (tax / 100) * taxValue;
-                    } else if (discountAmount > 0) {
-                        finalPrice = sellingPrice - discountAmount;
-                    } else if (discountPercentage > 0) {
-                        finalPrice = sellingPrice - (sellingPrice * (discountPercentage / 100));
-                    } else if (tax > 0) {
-                        if (taxType === 'exclusive') {
-                            finalPrice = sellingPrice + (sellingPrice * tax / 100);
-                        } else {
-                            finalPrice = inclusive_price + (inclusive_price / 100) * tax;
-                        }
-                    } else {
-                        finalPrice = sellingPrice;
-                    }
+                    var finalPrice = PosnicPro.kot._priceOfOrderLine(item);
 
                     console.log('Item:', item.item_name, '- Selling Price:', sellingPrice, '- Tax:', tax, '- Final Price:', finalPrice);
 
@@ -877,14 +845,24 @@ PosnicPro.kot = {
      * this file, in the view and add-from-view paths. They are left alone
      * rather than refactored blind; folding them in is worth its own change.
      */
-    _priceOf: function (data) {
-        var sellingPrice = parseFloat(data.selling_price || 0);
-        var discountAmount = parseFloat(data.discount_amount || 0);
-        var discountPercentage = parseFloat(data.discount_percentage || 0);
-        var tax = parseFloat(data.tax || 0);
-        var taxType = data.tax_type || 'inclusive';
+    /*
+     * THE SUM ITSELF, in one place, over values somebody else resolved.
+     *
+     * There were THREE copies of this in this file and they had drifted, which
+     * is the whole reason it is worth moving. The search read `selling_price`
+     * and treated an untyped tax as INCLUSIVE; the two paths that re-read an
+     * order read `selling_price || item_price` and treated it as EXCLUSIVE. On
+     * an item carrying tax with no tax_type, those two return different money
+     * for the same dish.
+     *
+     * So the arithmetic is shared and THE DIFFERENCES ARE KEPT, passed in by
+     * each caller. Folding the defaults together would quietly reprice every
+     * untyped item on one screen or the other, and that is a decision about
+     * money for the owner to make on purpose - not a tidy-up to slip into a
+     * refactor.
+     */
+    _priceFrom: function (sellingPrice, tax, taxType, discountAmount, discountPercentage) {
         var finalPrice = 0;
-
         var taxPrice = (sellingPrice * tax) / (100 + tax);
         var inclusive_price = sellingPrice - taxPrice;
         var discountValue = 0;
@@ -915,10 +893,42 @@ PosnicPro.kot = {
         } else {
             finalPrice = sellingPrice;
         }
+        return finalPrice;
+    },
 
+    /*
+     * What a CATALOGUE row costs - the search results and the Browse grid.
+     * Reads `selling_price`, and an untyped tax is inclusive, which is what
+     * this path has always done.
+     */
+    _priceOf: function (data) {
+        var sellingPrice = parseFloat(data.selling_price || 0);
+        var finalPrice = PosnicPro.kot._priceFrom(
+            sellingPrice,
+            parseFloat(data.tax || 0),
+            data.tax_type || 'inclusive',
+            parseFloat(data.discount_amount || 0),
+            parseFloat(data.discount_percentage || 0)
+        );
         return { priceDisplay: finalPrice.toFixed(2), basePrice: sellingPrice.toFixed(2) };
     },
 
+    /*
+     * What an ORDER LINE costs - the two paths that re-read an order already
+     * sitting on a table. They fall back to `item_price` for a line saved
+     * before a catalogue price existed, and an untyped tax is exclusive.
+     */
+    _priceOfOrderLine: function (item) {
+        return PosnicPro.kot._priceFrom(
+            parseFloat(item.selling_price || item.item_price || 0),
+            parseFloat(item.tax || 0),
+            item.tax_type || 'exclusive',
+            parseFloat(item.discount_amount || 0),
+            parseFloat(item.discount_percentage || 0)
+        );
+    },
+
+
     /** Text into markup, because a dish name is somebody's typing. */
     _escape: function (text) {
         return String(text == null ? '' : text).replace(/[&<>"']/g, function (c) {
@@ -1205,42 +1215,10 @@ PosnicPro.kot = {
                             var item = existingItemIds[itemId];
 
                             // Calculate final price with discounts and taxes
-                            var sellingPrice = parseFloat(item.selling_price || item.item_price || 0);
-                            var tax = parseFloat(item.tax || 0);
-                            var taxType = item.tax_type || 'exclusive';
-                            var discountAmount = parseFloat(item.discount_amount || 0);
-                            var discountPercentage = parseFloat(item.discount_percentage || 0);
-
-                            var taxPrice = (sellingPrice * tax) / (100 + tax);
-                            var inclusive_price = sellingPrice - taxPrice;
-
-                            if (discountAmount > 0 && tax > 0) {
-                                var discountValue = (taxType === 'exclusive') ? sellingPrice - discountAmount : inclusive_price - discountAmount;
-                                finalPrice = discountValue + (tax / 100) * discountValue;
-                            } else if (discountPercentage > 0 && tax > 0) {
-                                var discountValue = 0;
-                                var taxValue = 0;
-                                if (taxType === 'exclusive') {
-                                    discountValue = (sellingPrice * (discountPercentage / 100));
-                                    taxValue = sellingPrice - discountValue;
-                                } else {
-                                    discountValue = (inclusive_price * (discountPercentage / 100));
-                                    taxValue = inclusive_price - discountValue;
-                                }
-                                finalPrice = taxValue + (tax / 100) * taxValue;
-                            } else if (discountAmount > 0) {
-                                finalPrice = sellingPrice - discountAmount;
-                            } else if (discountPercentage > 0) {
-                                finalPrice = sellingPrice - (sellingPrice * (discountPercentage / 100));
-                            } else if (tax > 0) {
-                                if (taxType === 'exclusive') {
-                                    finalPrice = sellingPrice + (sellingPrice * tax / 100);
-                                } else {
-                                    finalPrice = inclusive_price + (inclusive_price / 100) * tax;
-                                }
-                            } else {
-                                finalPrice = sellingPrice;
-                            }
+                            /* One sum, in _priceFrom. This path keeps its own inputs:
+                               item_price as a fallback and an untyped tax read as
+                               exclusive - see the note on _priceFrom. */
+                            finalPrice = PosnicPro.kot._priceOfOrderLine(item);
 
                             console.log('Existing Item - Final Price:', finalPrice, '- Qty:', qty);
                         } else {

tests/mobile-app-reachability.test.js

@@ -113,10 +113,46 @@ test('the kiosk mobile login hands back a credential and a shop key', () => {
 
   assert.match(handler, /token: jwtToken/,
     'the app has no credential to present to the routes behind protectOrKioskKey');
-  assert.match(handler, /signLegacyToken\(recordsFiltered, req\)/,
+  /*
+   * The RULE, not the call's exact spelling.
+   *
+   * This pinned `signLegacyToken(recordsFiltered, req)` as a literal, and so
+   * went red the moment a handset token was given its own lifetime:
+   *
+   *     signLegacyToken(recordsFiltered, req, undefined, handsetSeconds)
+   *
+   * Nothing about that weakened the credential - it is still signed from the
+   * user who signed in - but develop carried a failing suite for it, which is
+   * how a red test stops meaning anything. What has to hold is that the user
+   * and the request are what the token is signed FROM; how long it then lasts
+   * is a separate decision, and adding arguments must not fail this.
+   */
+  assert.match(handler, /signLegacyToken\(\s*recordsFiltered\s*,\s*req/,
     'the token must name the user who signed in, not a shared device key');
-  assert.match(handler, /expiresIn: jwtLifetimeSeconds\(\)/,
-    'a client that has to guess its own expiry refreshes far too often, or too late');
+  /*
+   * The client is told the SAME lifetime the token was signed with.
+   *
+   * This pinned `expiresIn: jwtLifetimeSeconds()` as a literal, and went red
+   * when a handset was given its own, longer lifetime:
+   *
+   *     const handsetSeconds = handsetLifetimeSeconds();
+   *     signLegacyToken(recordsFiltered, req, undefined, handsetSeconds);
+   *     expiresIn: handsetSeconds,
+   *
+   * Which is better than what it replaced, and the test failed it anyway. The
+   * property worth holding is not WHICH clock the lifetime comes from - a
+   * handset on a floor all day should not expire like a till - but that the
+   * number reported is the number the token carries. A client told a
+   * different figure refreshes too often or, worse, too late.
+   *
+   * So the fourth argument to signLegacyToken is read out of the source, and
+   * expiresIn must report that same identifier.
+   */
+  const signedWith = handler.match(/signLegacyToken\([^)]*?,\s*[^,)]*,\s*([A-Za-z_$][\w$]*)\s*\)/);
+  assert.ok(signedWith, 'the token is signed without a lifetime, so nothing can be reported');
+  assert.match(handler, new RegExp('expiresIn: ' + signedWith[1] + '\\b'),
+    'the client is told an expiry that is not the one the token carries: signed with '
+      + signedWith[1]);
   assert.match(handler, /shopKey,/,
     'without a shop key the app cannot tell that a LAN server holds the same shop');
   assert.doesNotMatch(handler, /license: *recordsFiltered\.license/,

tests/one-place-decides-what-a-dish-costs.test.js

@@ -0,0 +1,154 @@
+'use strict';
+
+/*
+ * ONE PLACE DECIDES WHAT A DISH COSTS.
+ *
+ * kot.js carried THREE copies of the same discount-and-tax arithmetic: the
+ * search results, and the two paths that re-read an order already sitting on
+ * a table. Three copies of a price calculation is how a dish ends up costing
+ * two different amounts on two screens, and nobody notices until a customer
+ * is charged twice differently for the same thing.
+ *
+ * THEY HAD ALREADY DRIFTED, which is the part worth knowing:
+ *
+ *   the search path      reads `selling_price`, and an item carrying tax with
+ *                        no tax_type is treated as INCLUSIVE
+ *   the order paths      read `selling_price || item_price`, and the same
+ *                        item is treated as EXCLUSIVE
+ *
+ * On a taxed item with no tax_type those two return different money. So the
+ * sum is now shared (`_priceFrom`) and the differences are KEPT, passed in by
+ * each caller through `_priceOf` and `_priceOfOrderLine`. Folding the
+ * defaults together would quietly reprice every untyped item on one screen or
+ * the other, and that is a decision about money for the owner to make on
+ * purpose rather than something to slip into a refactor.
+ *
+ * These tests run the real functions against the numbers the old copies
+ * produced, because "I moved some code and it looks the same" is not a thing
+ * anybody should take on trust where money is concerned.
+ */
+
+const test = require('node:test');
+const assert = require('node:assert');
+const fs = require('node:fs');
+const path = require('node:path');
+
+const ROOT = path.join(__dirname, '..');
+const SRC = fs.readFileSync(
+  path.join(ROOT, 'frontend', 'static', 'script', 'js', 'modules', 'js', 'kot.js'),
+  'utf8'
+);
+const NL = String.fromCharCode(10);
+
+/** The three pricing methods, lifted out and runnable. */
+function pricing() {
+  const names = ['_priceFrom', '_priceOf', '_priceOfOrderLine'];
+  const bodies = names.map((name) => {
+    const at = SRC.indexOf('    ' + name + ': function');
+    assert.ok(at > 0, name + ' is gone from kot.js');
+    /* To the line that closes the method at this indent. */
+    const end = SRC.indexOf(NL + '    },', at);
+    assert.ok(end > at, name + ' has no visible end');
+    return SRC.slice(at, end + NL.length + 6);
+  });
+  const src = 'var PosnicPro = { kot: {' + NL + bodies.join(NL) + NL + '} };' + NL + 'return PosnicPro.kot;';
+  /* eslint-disable-next-line no-new-func -- running the real arithmetic is the point */
+  return new Function(src)();
+}
+
+const kot = pricing();
+
+/** What every old copy did, kept here as the thing being compared against. */
+function theOldWay(sellingPrice, tax, taxType, discountAmount, discountPercentage) {
+  let finalPrice = 0;
+  const taxPrice = (sellingPrice * tax) / (100 + tax);
+  const inclusive = sellingPrice - taxPrice;
+  if (discountAmount > 0 && tax > 0) {
+    const d = taxType === 'exclusive' ? sellingPrice - discountAmount : inclusive - discountAmount;
+    finalPrice = d + (tax / 100) * d;
+  } else if (discountPercentage > 0 && tax > 0) {
+    const base = taxType === 'exclusive' ? sellingPrice : inclusive;
+    const v = base - base * (discountPercentage / 100);
+    finalPrice = v + (tax / 100) * v;
+  } else if (discountAmount > 0) {
+    finalPrice = sellingPrice - discountAmount;
+  } else if (discountPercentage > 0) {
+    finalPrice = sellingPrice - sellingPrice * (discountPercentage / 100);
+  } else if (tax > 0) {
+    finalPrice = taxType === 'exclusive'
+      ? sellingPrice + (sellingPrice * tax) / 100
+      : inclusive + (inclusive / 100) * tax;
+  } else {
+    finalPrice = sellingPrice;
+  }
+  return finalPrice;
+}
+
+const CASES = [
+  /* price, tax, taxType, discount amount, discount % */
+  [220, 0, 'inclusive', 0, 0],
+  [220, 5, 'inclusive', 0, 0],
+  [220, 5, 'exclusive', 0, 0],
+  [220, 18, 'exclusive', 20, 0],
+  [220, 18, 'inclusive', 20, 0],
+  [220, 12, 'exclusive', 0, 10],
+  [220, 12, 'inclusive', 0, 10],
+  [220, 0, 'inclusive', 50, 0],
+  [220, 0, 'inclusive', 0, 25],
+  [0, 18, 'exclusive', 0, 0],
+];
+
+test('the shared sum returns exactly what the three copies returned', () => {
+  for (const [p, t, tt, da, dp] of CASES) {
+    assert.strictEqual(
+      kot._priceFrom(p, t, tt, da, dp).toFixed(4),
+      theOldWay(p, t, tt, da, dp).toFixed(4),
+      'moved the money for ' + JSON.stringify([p, t, tt, da, dp])
+    );
+  }
+});
+
+test('a catalogue row still treats an untyped tax as inclusive', () => {
+  /* The search and Browse path. Changing this would reprice every untyped
+     item in the search results. */
+  const got = kot._priceOf({ selling_price: 220, tax: 5 });
+  assert.strictEqual(got.priceDisplay, theOldWay(220, 5, 'inclusive', 0, 0).toFixed(2));
+  assert.strictEqual(got.basePrice, '220.00');
+});
+
+test('an order line still treats an untyped tax as exclusive', () => {
+  /* The two order-reading paths. This is the drift that was found, and it is
+     kept deliberately - see the header. */
+  assert.strictEqual(
+    kot._priceOfOrderLine({ selling_price: 220, tax: 5 }).toFixed(2),
+    theOldWay(220, 5, 'exclusive', 0, 0).toFixed(2)
+  );
+});
+
+test('the two defaults really do disagree, so nobody merges them by accident', () => {
+  /*
+   * Stated as a test rather than only as a comment. If somebody makes these
+   * agree, this fails and asks them to mean it - which is the point, because
+   * it changes what customers are charged.
+   */
+  const catalogue = Number(kot._priceOf({ selling_price: 220, tax: 5 }).priceDisplay);
+  const orderLine = Number(kot._priceOfOrderLine({ selling_price: 220, tax: 5 }).toFixed(2));
+  assert.notStrictEqual(catalogue, orderLine,
+    'the inclusive/exclusive defaults now agree; if that was deliberate, say so here');
+});
+
+test('an order line falls back to item_price when there is no catalogue price', () => {
+  /* A line saved before a catalogue price existed. The search path has never
+     done this and still does not. */
+  assert.strictEqual(kot._priceOfOrderLine({ item_price: 180 }).toFixed(2), '180.00');
+  assert.strictEqual(kot._priceOf({ item_price: 180 }).priceDisplay, '0.00');
+});
+
+test('only one copy of the arithmetic is left in the file', () => {
+  /*
+   * The reason for the whole change. Counted on the one line every copy had -
+   * if a fourth appears, this says so before it can drift.
+   */
+  const copies = (SRC.match(/inclusive_price = sellingPrice - taxPrice/g) || []).length;
+  assert.strictEqual(copies, 1, 'there are ' + copies + ' copies of the pricing sum in kot.js');
+});