Skip to content

The shop rule calls the door the way the door is written - #806

Closed
sridharkalaibala wants to merge 1 commit into
developfrom
fix/the-shop-rule-calls-the-right-door
Closed

sridharkalaibala wants to merge 1 commit into
developfrom
fix/the-shop-rule-calls-the-right-door

Conversation

@sridharkalaibala

Copy link
Copy Markdown
Contributor

The auto accept / auto cancel rule from #795 has never decided anything. Found while
checking the code-scanning alert on #805.

decideOnOrder takes { saleId, decision, reason }. The sweep called it as
decideOnOrder(saleId, decision, reason), so saleId was undefined, the guard
answered "Enter must correct order id", and every decision was refused before it began.

Why nothing looked wrong

The read-back I added for an unrelated reason saw the order had not moved and quietly
moved on. So no alarm was falsely silenced and no order was wrongly cancelled - the
rule simply did nothing at all, which is indistinguishable from a shop that never
configured one. That is the only good news here.

The tests passed throughout because the fake repository took three positional arguments
too.
A fake that speaks a language the real thing does not is a test proving the caller
agrees with itself.

How it is pinned now

Two tests, both against the real module rather than a fake:

  • one spies on the actual decideOnOrder and asserts it is handed one object carrying
    saleId;
  • one calls the real function the old way and asserts it refuses - which is exactly
    what it did, in silence, on every order, for as long as the feature existed.

Verified by putting the old call back and watching six tests go red.

Same family as the ai.availability / ai.available typo: pin call sites against real
exports.
A feature test proves the code works and can never prove anybody calls it
correctly.

Checks

  • api: 379 suites, 10,892 tests
  • npm run check: 7/7

The auto accept / auto cancel rule has never decided anything.

decideOnOrder takes { saleId, decision, reason }. The sweep called it as
decideOnOrder(saleId, decision, reason), so saleId was undefined, the
guard answered "Enter must correct order id", and every decision was
refused before it began.

Nothing looked wrong from any angle. The read-back added for an unrelated
reason saw the order had not moved and quietly moved on, so no alarm was
falsely silenced and no order was wrongly cancelled - the rule simply did
nothing at all, which is indistinguishable from a shop that never
configured one.

The tests passed throughout because the fake repository took three
positional arguments too. A fake that speaks a language the real thing
does not is a test proving the caller agrees with itself.

So the contract is pinned against the real module: one test spies on the
actual decideOnOrder and asserts it is handed ONE object carrying saleId,
and another calls the real function the old way and asserts it refuses -
which is exactly what it did, in silence, on every order, for as long as
the feature existed. Both fail if the old call comes back; verified by
putting it back and watching six tests go red.

Same family as the AI availability typo: pin call sites against real
exports, because a feature test proves the code works and can never prove
anybody calls it correctly.
@sridharkalaibala

Copy link
Copy Markdown
Contributor Author

Closing as redundant, not as wrong.

#807 landed the same fix from another session while this was open, and covers it slightly better: it also carries by: 'rule' through to decideOnOrder, and it pins the call shape in the source as well as the behaviour.

Verified on develop just now rather than assumed:

  • the sweep sends an object - repo.decideOnOrder({ saleId, decision, reason, by: 'rule' })
  • the real method still refuses the old positional call: false | Enter must correct order id
  • unanswered-orders.test.js 19/19, including "the real method refuses the call this used to make"

So the rule now actually decides orders, which it never did. Nothing from this branch is needed; rebasing it would only replay a duplicate through a conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant