Problem/Motivation

The same duplication audit as #3618835: Give the copied manager form, signed token, domains form, storage schema and row accessors one owner each found that BookingManager keeps private copies of knowledge that services it already injects own. This is why the class is 2,644 lines, but the size is the symptom: a capacity rule written four times is a rule three of whose copies can be missed.

A tariff's effective quota on a slot is stated four times and owned nowhere. A slot tariff row overrides the resource tariff's quota, and where there is no such row the resource tariff's own quota stands. That one sentence is written out in four places: BookingConsumption::quotaFor() and CapacityLockAnchorScopes::quotaFor() are byte-identical, docblock included; BookingManager::limitFor() is the same expression behind a NULL-tariff branch; and BookingManager states it again inline where it walks a page of slots. So a correction to the fallback has to be found in three files to be complete, and this is the number every capacity decision is taken against.

Two things nearby look like the same rule and are not, which is worth saying so they are not folded in by mistake. The allotment is resolved by both BookingManager::allotmentFor() and yoyaku_placement's PinnedPlaceHold::allotmentFor(), but not the same way: the second guards on hasField('allotment') and falls back to the raw target_id where the first calls getAllotmentId(). Reconciling those two is a decision about behavior rather than a move. And the two price reads in yoyaku_payment's PaymentPolicyResolver take the override alone, with no fallback at all, because the tariff's own price is applied further on inside policyFor(). They are a different shape and are not part of this.

What the booking rows have taken is assembled six times. sumConsuming(), sumConsumingGrouped() and sumConsumingByTariffAndAllotment() in BookingManager, then CapacityLockAnchorCounts, and then twice more in yoyaku_placement, in PlacementLockAnchorCounts and PinnedPlaceHold. Every one of them needs the same three facts, the booking table, the SUM(quantity) measure and the filter on CONSUMING_STATES, and they differ only in what they group by. So a fact about what a consuming line is has to be found in six places across three modules, and two of those six are summing something the engine already sums.

Two private methods that only forward. BookingManager::forTariff() and BookingScheduleResolver::consumingLines() each wrap one call to an injected service under the same name it already has, so they add nothing but a second place to look. bookingStorage() and transactionStorage() read the same way at a glance but are not the same thing: they have four callers and two, so removing them would repeat a storage name four times and twice, which is the duplication this is about. They stay.

Proposed resolution

Give the effective quota one owner on SlotTariffLookup, which already owns the forTariff() lookup three of the four sites call first. Two entry points, because the callers differ in what they hold: one taking the slot and the tariff, for a caller that has to look the row up, and one taking an already-loaded row and the tariff, for the caller that walks a page of slots with the rows preloaded. The second is what keeps this from costing a lookup per tariff per slot, which is the way a change like this silently makes things worse. The rule itself lives in one of them and the other calls it. Keep the return type honest about a quota that is absent on both sides, since an unset quota means unbounded and not nought.

Give the sum one owner stating those three facts once, with the groupings the engine needs on it. A module bounding a scope of its own groups these rows by a column the engine has no business naming, so the statement itself is reachable for a reader to narrow and group, rather than restated. Then nothing outside that owner says what a consuming line is.

Drop the forwarding methods and call the collaborator.

Remaining tasks

Every step has to hold or improve the query count, proven with Database::startLog() rather than argued, because collapsing four inline reads into one shared method is exactly the kind of change that can add a lookup per call. The existing performance test classes cover the paths involved and stay the check on this.

Whether to reconcile the two allotment resolutions is left open here rather than assumed, since the two differ in what they do when a tariff has no allotment field at all.

Decomposing the class along the seams this leaves, into reading availability, keeping the capacity accounting, and the booking lifecycle, is a follow-up rather than part of this: what is here is the reuse the title names, and it is what makes those seams visible. BookingManagerInterface and its 15 methods do not move either way.

User interface changes

None.

API changes

None to the engine's own interface. SlotTariffLookup gains the effective-quota read in two forms, and the sum's owner is new; both are additions.

Data model changes

None.

AI-Generated: Yes (Claude Code was used to run the duplication audit behind this issue, to help draft this summary, and to write the code and tests on the merge request. The linters and the named test classes were run before it was pushed; a maintainer reviews it before it merges.)

Issue fork yoyaku-3618847

Command icon 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

mably created an issue. See original summary.

mably’s picture

Issue summary: View changes

mably’s picture

Issue summary: View changes
Status: Active » Needs review

  • mably committed be16d8f7 on 1.x
    task: #3618847 Let BookingManager read through the services it already...
mably’s picture

Status: Needs review » Fixed

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.