allow past a zero credit balance when overage is on - #30
Merged
Conversation
Mirrors check_credit_balance_condition in rulesengine-rust, one for one, including the seven tests. The two engines must agree while both are in use — SCHY-515 was opened over exactly this kind of divergence. Company gains a per-credit CreditOverageEnabled map, defaulting nil so a caller that does not send it keeps the existing hard stop; indexing a nil map yields false, which is the behaviour we want. The check short-circuits before the precedence chain rather than joining it. All four of those branches compare against the balance, and with overage on the balance is no longer the question — including the caller-supplied credit cost branch, which would otherwise re-impose the gate. With no cap in the current design there is nothing else to compare against. This library is slated for removal by schematichq/api#7525, which moves the API onto the wasm engine. Changing it anyway because that PR is not close to merging and SCHX-582 has no working enforcement without it.
cbrady
marked this pull request as ready for review
September 1, 2026 15:23
dontlaugh
reviewed
Sep 1, 2026
| // an overage rate rather than being denied, so the balance stops gating the | ||
| // check. Absent or false is the historical behaviour, so a caller that does | ||
| // not send the field keeps hard-stopping at zero. | ||
| CreditOverageEnabled map[string]bool `json:"credit_overage_enabled"` |
Contributor
There was a problem hiding this comment.
If we need to have a map, document what the [string] is here.
But do we need a map here? Could we have a slice of credit ids instead? It's a narrower set of states.
Contributor
Author
There was a problem hiding this comment.
i'll add the documentation, but i'm going to keep it as a map because to match the rust rulesengine.
Reverting the overage check left this subtest passing: createTestFlag randomizes DefaultValue, and CheckFlag falls back to it when no rule matches, so the assertion was reading a coin flip rather than the rule. It was flaky for the same reason. Pinning the default false means it can only pass when the rule genuinely matches -- reverting the check now fails all three positive subtests instead of one. Also names the map key as a billing credit ID, per review.
dontlaugh
approved these changes
Sep 1, 2026
cbrady
added a commit
that referenced
this pull request
Sep 3, 2026
Overage has three states per credit — off, on-and-uncapped, on-and-capped — and the enabled-set plus cap-map spelling could express combinations that mean nothing: a cap on a credit that is not enabled, or enabled with no entry either side. Nothing prevented the two disagreeing. One map keyed by credit makes those unrepresentable: absent is off, a nil value is uncapped, and a set value is the cap. The check is one lookup rather than two. This replaces credit_overage_enabled from #30 rather than adding alongside it. Nothing consumes that field yet, so there is no reason to ship both and deprecate one later.
cbrady
added a commit
that referenced
this pull request
Sep 3, 2026
Overage has three states per credit — off, on-and-uncapped, on-and-capped — and the enabled-set plus cap-map spelling could express combinations that mean nothing: a cap on a credit that is not enabled, or enabled with no entry either side. Nothing prevented the two disagreeing. One map keyed by credit makes those unrepresentable: absent is off, a nil value is uncapped, and a set value is the cap. The check is one lookup rather than two. This replaces credit_overage_enabled from #30 rather than adding alongside it. Nothing consumes that field yet, so there is no reason to ship both and deprecate one later.
cbrady
added a commit
that referenced
this pull request
Sep 4, 2026
* deny once the overage cap is spent Overage previously failed open with no floor: enabled meant the balance stopped gating the check entirely. The cap moves the floor to -cap rather than removing it, so a company consumes past zero until it has accrued the configured limit and is then denied, the way an exhausted balance denies with overage off. An absent cap stays uncapped, which is the behaviour that shipped first. * carry overage as one nullable map instead of two Overage has three states per credit — off, on-and-uncapped, on-and-capped — and the enabled-set plus cap-map spelling could express combinations that mean nothing: a cap on a credit that is not enabled, or enabled with no entry either side. Nothing prevented the two disagreeing. One map keyed by credit makes those unrepresentable: absent is off, a nil value is uncapped, and a set value is the cap. The check is one lookup rather than two. This replaces credit_overage_enabled from #30 rather than adding alongside it. Nothing consumes that field yet, so there is no reason to ship both and deprecate one later. * rename a test param off a predeclared identifier golangci-lint's predeclared linter flags 'cap' shadowing the builtin. * measure the cap against the cost, not the balance alone The overage branch returned before the cost/quantity precedence below it, so the cap was compared against the balance as it stood rather than as it would stand after the call. That enforced the cap only to within one call's cost: a company 5 credits short of a 100-credit cap passed a call costing 50 and landed 45 past it. Cost is now resolved first and overage shifts the floor it is measured against: balance + allowance >= cost. Uncapped still returns before the comparison, and with no overage the allowance is zero, which is exactly the balance >= cost check that has always applied. Adds the capped-plus-cost cases that were missing — every existing overage test used the legacy single-unit path, which is why this got through — and the serialization round-trip proving absent, present-and-null, and present-and-set stay distinct in both directions.
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
2 participants
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.
Adds a per-credit
CreditOverageEnabledmap toCompanyand lets the creditcondition allow past a zero balance when it is set, for SCHX-582: credit grants
hard-stop at zero today, which cuts off invoice-billed customers on net terms
who have no card for auto top-up to charge.
Mirrors
check_credit_balance_conditioninrulesengine-rustone for one,including the tests, since the two engines have to agree while both are in use.
The check short-circuits before the precedence chain rather than joining it —
all four of those branches compare against the balance, and with overage on the
balance is no longer the question.
Note this library is slated for removal by SchematicHQ/api#7525, which moves the
API onto the wasm engine. Changing it anyway because that PR is not close to
merging and SCHX-582 has no working enforcement without it.