Restore the key's own reason and pin the plan-side identifying key
Two things the plan's own text asked back. First, the idempotency-key comment lost its substance when Task 5 rewrote it: the charged amount and the treatment are in the key because at a rate of nought the two Prices are the same amount and one key would have Stripe hand back a single object for both — a maintainer who simplified that key would only rediscover why from a unique-constraint violation. Restored alongside the newer sentence about what actually stops a second Price (adoption, not this key), so the module and plan sides read alike again. Spelled out "twenty-four hours" in both places in the same comment, matching the sibling. Second, nothing pinned identifying: ['plan_price_id'] — the one decision that makes the plan side different from the module side, because a family's Product carries a Price for every version, term and treatment, so plan_family alone cannot say which row a Price belongs to. Added a test that plants a Price proving only the family, confirmed it fails when identifying is loosened to ['plan_family'], and reverted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>feature/betriebsmodus^2
parent
cdda38c6b2
commit
dfbd3b5c1f
|
|
@ -172,11 +172,18 @@ final class PlanPrices
|
|||
currency: (string) $price->currency,
|
||||
interval: $interval,
|
||||
metadata: $metadata,
|
||||
// Says "I have already sent this call", nothing more — the
|
||||
// metadata is folded in by IdempotencyKey inside the client, so
|
||||
// changing the format above can never again refuse the call for
|
||||
// a day. What stops a second Price for one figure is the
|
||||
// adoption step above; Stripe forgets a key after 24 hours.
|
||||
// Says "I have already sent this call", nothing more. The
|
||||
// CHARGED amount is part of the key, so a run after a rate
|
||||
// change cannot replay the Price minted at the old figure. So
|
||||
// is the treatment, and it has to be: at a rate of nought the
|
||||
// two Prices are the same amount, and one key would have
|
||||
// Stripe hand back the same object for both — which the
|
||||
// register, where a Price id is unique, would refuse to
|
||||
// record twice. The metadata is folded in by IdempotencyKey
|
||||
// inside the client, so changing the format above can never
|
||||
// again refuse the call for a day. What stops a second Price
|
||||
// for one figure is the adoption step above, not this key —
|
||||
// Stripe forgets a key after twenty-four hours.
|
||||
idempotencyKey: "clupilot-price-{$price->id}-{$charged}"
|
||||
.($treatment->reverseCharge ? '-rc' : ''),
|
||||
);
|
||||
|
|
|
|||
|
|
@ -401,3 +401,52 @@ it('does not take a package price belonging to another catalogue row', function
|
|||
// exactly the failure a wrong plan_price_id must never cause.
|
||||
->and($this->stripe->archived)->not->toContain('price_other_row');
|
||||
});
|
||||
|
||||
it('does not adopt a package price that only proves the family, not this row', function () {
|
||||
// The whole reason the plan side needs `identifying: ['plan_price_id']`
|
||||
// rather than the module side's simpler key: one Product carries a Price
|
||||
// for every version, term AND treatment of a family, so a Price that
|
||||
// proves only the FAMILY could be any of them. If `identifying` were ever
|
||||
// loosened to ['plan_family'], a Price like the one planted below would
|
||||
// wrongly confirm — nothing else here would catch it.
|
||||
app(Kernel::class)->call('stripe:sync-catalogue');
|
||||
|
||||
$row = PlanPrice::query()->firstOrFail();
|
||||
$product = (string) $row->version->family->stripe_product_id;
|
||||
$charged = PlanPrices::chargedCents($row, TaxTreatment::domestic());
|
||||
$interval = $row->term === Subscription::TERM_YEARLY ? 'year' : 'month';
|
||||
|
||||
// This row's OWN domestic Price is a legitimate orphan the moment its
|
||||
// register row is gone — same reason as the two tests above — and would
|
||||
// otherwise be correctly re-adopted before the planted Price below is
|
||||
// ever weighed, masking the very rejection this test means to prove.
|
||||
$this->stripe->archivePrice((string) StripePlanPrice::query()
|
||||
->where('plan_price_id', $row->id)
|
||||
->where('reverse_charge', false)
|
||||
->value('stripe_price_id'));
|
||||
|
||||
StripePlanPrice::query()->where('plan_price_id', $row->id)->delete();
|
||||
|
||||
// Right product, right money, right currency, right interval, and the
|
||||
// right FAMILY — the one thing missing is plan_price_id, which is the
|
||||
// only key that says which of the family's many rows a Price belongs to.
|
||||
$this->stripe->plantPrice('price_family_only', $product, $charged, (string) $row->currency,
|
||||
$interval, ['plan_family' => $row->version->family->key]);
|
||||
|
||||
// Same reason as the two tests above: with the sync's own idempotency
|
||||
// key still in force, createPrice() would replay this row's own previous
|
||||
// Price rather than genuinely minting, leaving nothing for the
|
||||
// assertions below to catch.
|
||||
$this->stripe->keys = [];
|
||||
|
||||
$before = count($this->stripe->prices);
|
||||
|
||||
$id = app(PlanPrices::class)->ensure($row->refresh(), TaxTreatment::domestic());
|
||||
|
||||
expect($id)->not->toBe('price_family_only')
|
||||
// A fresh Price was minted rather than the family-only Price being
|
||||
// handed out — this is the assertion `identifying: ['plan_price_id']`
|
||||
// exists to keep true; swap it for ['plan_family'] and this fails.
|
||||
->and(count($this->stripe->prices))->toBe($before + 1)
|
||||
->and($this->stripe->archived)->not->toContain('price_family_only');
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue