Release: an order that changed under you, and a guard that does not write what it was given - #823
Merged
Merged
Conversation
A handset sends the WHOLE order when it saves, and the till keeps only what arrives. That is not a mistake: it is how a cancelled dish gets cancelled. It is also how a floor with several handsets loses food. Waiter A adds a biryani at 19:00. Waiter B saves at 19:01 from a screen opened at 18:58, so B's list has no biryani in it, and the till removes one the kitchen has already cooked. The bill goes out short. Nobody is told: not A, not B, not the kitchen, and not the shop, which has now given away a plate of biryani and has nothing that says so. So a caller may say which version of the order it was looking at, and a save written against an older one is refused with `order_changed` and the moment it actually changed. The client reloads and decides again, because only a person knows whether that biryani was meant to go. SILENCE STILL MEANS YES. A caller that sends no seen_at is served exactly as before. Handsets already in shops are older than this code, and refusing their saves would turn a bug that loses a dish into one that takes no orders. An unreadable timestamp is not a conflict either: that is a caller this check cannot help, and blocking it helps nobody. An order nobody has ever updated is judged by when it was created, so a first edit is guarded too rather than waved through. Six tests against a real mongod. Three of them fail without the guard; the other three are there to catch the guard overreaching, and pass either way.
CodeQL flagged the log line in the middleware I added last, and it was right. The comment above it said "the path and the count, never the value" and the code truncated the key paths to eighty characters, which I treated as safe. Truncating is not neutralising. A key is a string the caller chose. A newline inside one ends the log entry and starts another, so an attacker writes a second line into the file somebody reads to find out what happened - indented, plausible, and entirely theirs. Control characters do the same to a terminal reading it. Capping the length does nothing about either. So everything outside ordinary printable characters is replaced before it is written, and the request method and path go through the same function: they are a verb somebody sent and a URL somebody typed. Replaced rather than dropped, so the length still says something was there, and still capped so one key cannot fill a log.
The refusal went back as 404, which says the order is not there. It is there, the caller is welcome, and the request is simply out of date. 409 also gives the client something better to key on than the spelling of a message, so the handset's handling stays correct when somebody rewords it later.
An order that changed under you is not saved over
…hat-it-was-given A guard that does not write what it was given
Owner: "chickent briyani link to chicken 65 or mojito or coke. but coke should not suggest the briyani." Cross-selling has two halves. The shop's own goes_with was always directional: putting a chicken 65 under a biryani does not put a biryani under the chicken 65. The learned half was not. salesSignals counts every pair in BOTH directions, deliberately - it measures which dishes travel together, and that is a symmetric fact. A suggestion is not symmetric. Somebody holding a biryani may well want a drink; nobody holding a drink is one nudge away from a biryani, and being offered one reads as a shop trying to sell rather than a shop helping. So a coke that had sold beside a biryani four hundred times suggested it back, and no amount of filling in goes_with fixes that - the explicit pairings are consulted first, and the learned ones fill the slots underneath. THE RULE: a suggestion may not cost more than the dish it is suggested under. Price, because it needs nothing set up - no categories to maintain, no list of what counts as a main - which is what makes it work on a menu nobody has tidied. A dish cheaper than the anchor is an accompaniment to it whatever either of them is called. What the shop said is never filtered. A shop that took the trouble to pair a dish has overruled the statistics on purpose, most often to STOP a pairing the numbers keep producing - which is where this area came from: "for checken briyani its suggessting french fries. not good combination." Absent prices filter nothing, in both directions. A menu that cannot price itself suggests what it always suggested rather than suddenly suggesting nothing, and an unpriced candidate is kept rather than dropped: absent is "not said", never "expensive". Also guards a dish being its own learned pairing. salesSignals cannot produce that today, but the write path guards it on the explicit half, and a rule that holds only because of what a caller happens to do stops holding when the caller changes.
A shop defines option sets in settings - extra cheese, half plate, medium spicy - each with a price, and ticks which dishes carry them. The sale model has stored the answers since V2 and says so in its own words: "the price delta already inside the line price the client sent". The client does the arithmetic. So a client that cannot SEE the options cannot charge for them, and the handset could not see them at all: the storefront builds an explicit field list and modifier_group_ids was not in it. A waiter typed "extra cheese" as a note, the kitchen made it, and the bill said nothing. The shop has been giving cheese away with no record that it did. Sent WHOLE rather than as ids. A handset keeps its menu and sells from it on a dead network; ids would mean a second request to a settings endpoint, and the one time that request matters is the time it cannot be made. The ids never reach a client at all, so nobody is tempted to build that second request later. min and max travel with the group, because "pick one" and "pick any" are not the same screen. A group the shop deleted is dropped rather than drawn as an empty box a waiter has to tap past, and so is one whose options have all been removed. A shop whose option sets cannot be read still sells food: losing the extras is a smaller harm than losing the menu, and the warning says which happened. Seven tests. Six of them fail without this.
It lifted pairingsFor out of the repository by its exact signature, so adding the price argument made it match nothing and fail with "the shop can no longer say what goes with a dish" - the assertion firing not because the behaviour went, but because the anchor moved. The signature is now a named constant used for both the lookup and the header rewrite, so the next change to it breaks in one place. While it is open, it carries the rule too: a coke offers nothing back to the biryani, a biryani still offers both its drinks, a pairing the shop typed by hand is never filtered, and a dish with no price is read as "not said" rather than as expensive. Checked by deleting the price filter and watching this file fail on "a coke offered the biryani back".
Two files arrived wrapped at 80 columns. The api package sets 100, so its format:check fails on develop and every branch cut from it inherits a red run that has nothing to do with what it changed - the same thing that happened once already. Whitespace only: two statements rejoined onto one line.
Sending the option sets to a handset is half a feature. This path prices
every ordinary line from the shop's catalogue and discards whatever price
a client sends:
const sellingPrice = partnerVenues.priceFor(dynamic ? asked : catalogue, ...)
So a phone that added the cheese into its line price would have watched
the till quietly replace it with the plain price. The extras would have
been cooked and not charged, exactly as before, with more code in the way
and a screen that now claimed otherwise.
The till does the arithmetic instead, which is the right answer anyway: a
client that could name the price of cheese could also name a discount
nobody agreed to. A handset says WHICH options the table asked for; the
money comes from the shop's own documents.
Added BEFORE tax and discount, because extra cheese on a pizza is part of
the pizza. It is taxed at the pizza's rate and a percentage discount
applies to the whole plate. Adding it afterwards would tax the dish and
not the cheese, which is a quiet way to file a wrong return.
An option the shop does not offer on that dish is refused by name, not
ignored: a phone asking for it is holding a stale menu or asking for
something the kitchen will not make.
HOW MANY is left to the screen that asked, because a shop that tightens
min and max after a phone cached its menu should not have that phone's
orders refused at the till. Duplicates are folded and a ceiling is
honoured, since charging twice for one tick is a different kind of wrong.
An option that costs nothing still travels. "Medium" is not a money
question, it is a cooking question, and dropping it would send the pass a
dish with no answer on it.
Nine tests.
Owner, on Azure Coastal Kitchen: "you need to fill the details of menu. description and nutrition, veg or non veg, other all details needs to be filled one by one. very detailed." Every one of those fields already exists on an item and already has a box on the item screen. What did not exist was a way to fill in ninety of them without opening ninety forms: the item import carries the price-and-stock columns and nothing else, and sets description to empty on the way past. So a menu can only be detailed by hand. utils/dish-columns is the translation between a column in a file and a field on a dish. It is its own file because the rules it enforces are the item form's rules - an import that can write what the form would have refused is not an import, it is a back door, and the form filters food tags against the tickable list precisely so that a client asking for heart_healthy stores nothing. THREE RULES. A column the file does not carry changes nothing. This is the one that matters to the other 89 shops: everybody re-imports the 18-column export to change prices, and if a missing column meant "make it empty", the first price update after a menu was written would erase the menu. So the dish detail is spread in from what the row actually has, not listed. A column that is there and empty clears the field, because otherwise there is no way to take a value back out through the door it went in. Nothing is dropped in silence. A tag that is not a tag, a diet word nobody knows, a pairing naming a dish that is not on the menu: each comes back in the message the import already shows, three then a count. NUMBERS IN A FILE ARE AN ESTIMATE UNLESS THE FILE SAYS KITCHEN. An empty nutrition_source is what turns the calorie figure and the health claims on, so it cannot be a default: a spreadsheet drafted from standard recipes would otherwise arrive claiming a kitchen had checked every figure, on a live menu. Nutrition merges per nutrient, so a file with only a calories column leaves the protein somebody typed alone. Pairings arrive as NAMES and resolve after every row is written, because the file is usually the whole menu and a row pairs with dishes that did not exist yet when it was read. A name the file carries matches whatever the case; one that is only in the database is looked up once, through safe-search, so "Chicken 65 (Boneless)" is a name and not a pattern. Same three caps as the form: never itself, never twice, six at most. The export is deliberately not widened. It would put 18 more columns in front of every retail shop that will never set a spice level. The round trip works without it, because of the first rule. 69 tests. The diet list now has one definition rather than two.
The handset can see the options it is meant to charge for
…as-prettier-leaves-it
Three files wrapped at 80 where the api package sets 100, and one generated list that a new server message never got added to. `languages/server/_english.json` is written by tests/tools/i18n-server-text.js and is what every translation pack is checked against. A message added to the server without regenerating it makes the i18n suite fail on develop and on every branch cut from it. `order_changed` arrived with the order-conflict work. Whitespace and one generated line. No behaviour. The two causes are worth naming because both are silent locally: prettier run from the repo root skips api/ entirely (the root .prettierignore), so `cd api && npm run format` is the one that catches the first, and the second has a tool that writes the answer.
…ke-does-not-suggest-a-biryani
…import-carries-the-dish
The import carries the dish, not only its price
…ryani A coke does not suggest a biryani
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Promotes develop to main. Two merged pull requests.
Merge with a merge commit, never a squash.
Correcting my own first draft of this note: I listed #818 and #819 here, and they are not in it - they were merged straight into
mainand are already live. Only these two are new.#821 - an order that changed under you is not saved over (another session)
A handset sends the whole order when it saves and the till keeps only what arrives, which is how a cancelled dish gets cancelled - and also how one handset silently discards what another just added. It now answers 409 rather than writing over it.
#822 - a guard that does not write what it was given
The CodeQL check on release #816 flagged my own log line, and it was right. I truncated caller-chosen key paths to 80 characters and called it safe. Truncating is not neutralising: a newline in a key forges a second log entry, indented and plausible, in the file somebody reads to find out what happened.
Control characters are now replaced before anything is written, and
req.methodandreq.pathgo through the same function - I had been interpolating both raw.What to watch
The API deploy. Neither of these touches the frontend.
Verified before opening
API suite and root suite green on each branch before merge;
scripts/check-locally.jspasses all seven ondevelop.