Problem/Motivation
A review mapped every behaviour in src/, the module file and the install file to the test that asserts it. Most of the money paths are well covered; these are not, ranked by what a regression would cost:
- The expiry sweep for self-scheduling subscriptions (the non-charge-based branch of the cron hook, and
loadExpiredSubscriptions()). A regression leaves a lapsed subscriber with paid roles forever, or deactivates active rows. Only the charge-based branch is tested. - Webhook replay:
resolveCharge()on a charge already succeeded or failed. Replays are routine; a regression returns 500 to the provider forever or advances the anchor twice. - Registration while a billing API is down: the user-insert hook's fail-open is only exercised with the failure flag set after the user exists.
upgrade()with nothing to prorate (cheaper plan, or anchor in the past): untested whether the plan still swaps and no charge is made.- The entitlement-role backfill's healing path never executes in any test; the update-path test only asserts that nobody gained a role.
- The engine's data guards: missing currency or interval, and a plan whose connector is not charge-based.
syncPlans()per-connector exception isolation and the unknown-connector error.
Seven assertions are weaker than the behaviour they guard: four anchor checks that pass even when the anchor does not move (CycleEngineTest twice, DunningTest twice), a tax test that accepts a pending result as success, a reversal test that counts events where the names matter, a usage-rating test that checks for any charge rather than the amount, a retry-refusal test that checks only the call count, and a maintenance test that checks status alone where roles and dunning fields matter.
Two further gaps — the first period charge when the connector throws, and the duplicates resolver on a charge-based loser — are closed by the charge-outcomes issue's tests and are not repeated here.
Proposed resolution
Kernel tests for the seven gaps:
- Expiry sweep: a self-scheduling plan with a role; one subscription expired yesterday, one expiring tomorrow; run cron; the first is off with the role gone, the second untouched.
- Replay: resolve a pending charge as succeeded twice, then as failed; one anchor advance, one recorded charge, still succeeded, not past due.
- Registration: set the solo connector's throw flag before creating the user; the user saves and an error is logged.
- Upgrade without proration: cheaper plan with a future anchor, and pricier plan with a past anchor; NULL returned, plan swapped, no connector call.
- Backfill: an active subscriber whose role was removed by hand is healed; a blocked user is not; the summary counts match.
- Guards: no currency, and a charge-based plan on a self-scheduling connector; no charge record, no connector call.
syncPlans(): one connector throwing does not stop the others; an unknown connector ID throws.
And the six assertions tightened to the exact value, state or event list.
Remaining tasks
- Write the tests; no code change expected unless a test finds one.
User interface changes
None.
API changes
None.
Data model changes
None.
Issue fork subscription_manager-3629969
Show commands
Start within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
Comments
Comment #2
colanDesign pass, after re-reading each listed path against 1.0.x as it stands following #3629968: Treat a charge exception as an unknown outcome, reap stale submitted charges, never bill or revive a subscription that is no longer live, and stop back-billing suspended time and #3629966: Return JSON as JSON, replace the raw prices map in api/my-subscription with a PlanPrice, and burn post-purchase tokens atomically. The seven gaps are still gaps; the code for each behaves as the summary expects, so no production change is foreseen. Notes on placement and on a few details the summary did not settle, then the decisions.
The seven gaps
statusoff and saves; role revocation follows from thegranted_rolessnapshot on save. A newExpirySweepTestseeds a self-scheduling plan with a role and three subscriptions: expired yesterday, expiring tomorrow, and already off with a past expiry (the loader filters on status, so it must stay untouched and unlogged). It runs the realcronservice and asserts status, role and the notice log line.resolveCharge()ignores a charge that is notsubmittedwith a warning. InCycleEngineTest: a pending period charge resolved as succeeded twice, then as failed; one anchor advance, the record still succeeded, not past due, one settled-charge registration with the tax service, and the warning logged twice.createUser(); the user exists afterwards, no subscription was created, and the hook's error is logged. InMultiConnectorSyncTest, next to the existing flag tests.upgrade()returnsNULLwhen the prorated amount is zero or negative but still swaps plan, basis and roles and clears a pending downgrade. InCycleEngineTest: a cheaper plan with a future anchor and a pricier plan with a past anchor;NULLreturned, plan and roles swapped, no charge record, no connector call, anchor untouched.subscription_manager_backfill_missing_entitlement_roles()is procedural but takes no request state, so a Kernel test calls it directly. A newEntitlementBackfillTest: an active subscriber stripped of the plan's role by hand is healed; one who already has it is scanned but not healed; a blocked subscriber and an inactive subscription are skipped; the summary is exactly['scanned' => 2, 'healed' => 1, 'roles_added' => 1]. The$uidargument narrows to one user. The update-path test keeps its "nobody gained a role" assertion for the fixture.test_connector: the same, with the "not charge-based" error. InCycleEngineTest.syncPlans(). The test connectors'syncPlans()are empty, so two small additions to the test modules:TestConnectorBase::syncPlans()records its plugin ID in state, andSoloConnector::syncPlans()throws under the existing remote-throw flag. The test asserts every other connector synced when the solo one threw, the error names the connector, and an unknown ID throwsInvalidArgumentExceptionbefore any connector runs.The weak assertions
Re-reading finds four loose anchor checks, not three: two in
CycleEngineTest(zero-amount with recording off; cron end to end) and two inDunningTest(retry success; proration retry then period). All four become the exact next anchor fromcomputeNextAnchor(). The others, as the summary has them: the tax proration test asserts the charge succeeded and its taxed amount rather than that a charge exists; the reversal idempotency test asserts the event name list; the usage-rating zero-base test asserts the connector received one charge of exactly2.00; the retry-refusal test also asserts the record is still succeeded, the subscription not past due, the attempt and anchor unchanged; the duplicates resolution test also asserts the loser's role is gone and the winner's intact.Decisions to ratify
ExpirySweepTestandEntitlementBackfillTest; the other gaps join the existing class for their subject.syncPlans(): a sync recorder in the shared test base and the solo connector throwing under its existing flag.Comment #3
colanImplemented in the MR, in three commits, following the four decisions ratified in #2. No production code changed: every path behaved as the summary expected, so decision 4's fallback was not needed.
The seven gaps
ExpirySweepTestruns the realcronservice over three self-scheduling subscriptions: expired yesterday (ends up off, the owner's role gone), expiring tomorrow (untouched, role kept), already off with a past expiry (untouched). Exactly one "Expired subscription" notice is logged, naming the lapsed one.CycleEngineTest::testResolutionReplayIsIgnored(): a pending period charge resolved as succeeded twice and then as failed. One anchor advance to the exact next anchor, the record still succeeded with no failure message, not past due, one charge record, one connector call, and the "Ignoring resolution" warning logged twice.MultiConnectorSyncTest::testRegistrationSurvivesConnectorOutage(): the solo connector's throw flag set beforecreateUser(). The account exists, no subscription was created, and the hook's error is logged once at error level with the user ID and the connector's message.testUpgradeWithoutProrationSwapsPlanOnly(): a cheaper plan mid-cycle and a pricier plan with the anchor passed. Both returnNULL, swap plan, basis and roles, leave the anchor, and open no charge record and no connector call.EntitlementBackfillTestcallssubscription_manager_backfill_missing_entitlement_roles()directly. The role is stripped at the database level, since a user save re-unions the roles of active subscriptions (which is how the loss happened in the wild). A stripped active subscriber is healed, an intact one is scanned but not healed, a blocked account and an inactive subscription are skipped, and the summary is exactly['scanned' => 2, 'healed' => 1, 'roles_added' => 1]. A second test shows the$uidargument narrowing the scan.testMissingCurrencyIsSkipped()andtestNonChargeBasedConnectorIsSkipped(): error logged with the exact message, no charge record, no connector call, anchor untouched.syncPlans()—testSyncPlansIsolatesConnectorFailures()asserts every connector extending the recording test base synced while the solo connector threw, and the error is logged againstsolo_connector;testSyncPlansUnknownConnectorThrows()asserts theInvalidArgumentExceptionand that no connector ran.The weak assertions
The four loose anchor checks (two in
CycleEngineTest, two inDunningTest) expect the exact next anchor fromcomputeNextAnchor(). The retry-refusal test also asserts the record is still succeeded with attempt 1, the subscription not past due with zero retry attempts, the anchor unchanged, and no event fired. The tax proration test asserts the charge succeeded and the connector was asked for the taxed amount. The reversal replay test asserts the event names. The usage-rating zero-base test asserts the connector received one charge of exactly 2.00 CAD. The duplicates resolution test gives both plans a role and asserts the shared owner ends with exactly the winner's role, and the winner keeps its snapshot.One thing learned on the way, for the record: a deactivated subscription keeps its
granted_rolessnapshot (only a plan change clears it). That is by design, so the resolution test asserts the owner's roles rather than the loser's snapshot.Test modules
TestConnectorBase::syncPlans()records its plugin ID in state;SoloConnector::syncPlans()throws under its existing remote-throw flag.Suite
222 Kernel tests (ten added), 6 Functional tests untouched; phpcs clean. No change record: nothing outside
tests/changed.Comment #9
colan