Skip to content

Preserve a fractional quantity when updating a limit period - #93

Open
thomasbergernz wants to merge 1 commit into
waldur:developfrom
thomasbergernz:fix/fractional-limit-period-quantity-truncation
Open

thomasbergernz wants to merge 1 commit into
waldur:developfrom
thomasbergernz:fix/fractional-limit-period-quantity-truncation

Conversation

@thomasbergernz

Copy link
Copy Markdown

Problem

_update_invoice_item truncates the previous period's quantity:

old_quantity = int(old_period["quantity"])

So a limit change on a component previously set to a fractional value — say 0.1 TiB — is billed as though the old limit had been nothing. The old sub-period is re-serialised with quantity: 0, and its prorated share of the billing period is lost from invoice_item.quantity.

Unlike the set_limits 500 fixed in 595327d, this one is silent: the request succeeds and the invoice is quietly wrong, which makes it the harder of the two to notice.

Change

old_quantity = float(old_period["quantity"])
if old_quantity.is_integer():
    old_quantity = int(old_quantity)

Kept JSON-native rather than coerced to Decimal, because the value goes straight back into details["resource_limit_periods"] via serialize_resource_limit_period and a Decimal is not JSON encodable. Whole numbers stay int, so existing payloads are byte-identical and integer-only deployments see no change at all.

Relationship to 595327d

595327d (Coerce fractional limits to Decimal in total-period billing [#349]) fixed the float - Decimal TypeError on the TOTAL path — thank you, that came from #91. This is its sibling on the monthly/quarterly/annual path, which that commit did not touch. Both are needed before a fractional limit is safe end to end.

For reference, the fix is on develop but not in 8.1.3-rc.8, which still carries both this truncation and the pre-fix subtraction.

Verified

Built on 8.1.2 with this change plus the coercion, and run in a test deployment. Changing a storage_project limit from 0.1 to 0.5 on a resource that already had invoice items produced the correct pair of items — 0.1 @ 420 = 42.00 and a delta of 0.4 @ 420 = 168.00, netting 210.00 for 0.5 TiB — where before it raised, and with only the coercion the old period would have contributed 0.

Licensing: I agree to license this contribution under the MIT license.

int(old_period["quantity"]) truncates a fractional value to 0, so a limit
change on a component previously set to, say, 0.1 is billed as though the old
limit had been nothing: the old period is re-serialised with quantity 0 and the
prorated total for that sub-period is lost.

Keep the value JSON-native rather than coercing to Decimal. It is written
straight back into details["resource_limit_periods"] by
serialize_resource_limit_period, and a Decimal is not JSON encodable. Whole
numbers stay int, so existing payloads are byte-identical.

This is the sibling of the coercion added in 595327d for the total-period
diff; that one fixed the TypeError, this one fixes a silent wrong value on the
monthly/quarterly/annual path.
@thomasbergernz

Copy link
Copy Markdown
Author

Context for sequencing: this is the billing-correctness half. The two PRs that make a fractional limit reachable by a customer are #94 (API) and waldur/waldur-homeport#105 (UI). This one and 595327d should land first — otherwise widening the inputs turns a hard error into a quietly wrong invoice.

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