Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Quantity allocation refactoring and other fixes #64

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

osteel
Copy link
Owner

@osteel osteel commented Sep 13, 2023

Summary

This PR revises the way same-day and 30-day quantities are allocated to acquisitions and refactors the DisposalProcessor and ReversionFinder services, as well as the SharePoolingAsset aggregate root.

Explanation

Quantity allocations

Since disposals are what's driving the allocation of same-day and 30-day quantities, these are now (de)allocated to/from the corresponding acquisitions upon applying SharePoolingAssetDisposedOf and SharePoolingAssetDisposalReverted events, and only then.

This means allocated quantities are now always correct, even upon replaying the events.

DisposalBuilder

The DisposalProcessor service has now been renamed DisposalBuilder. Its only job is to return a SharePoolingAssetDisposal object with the right cost basis and quantity allocations. It now receives a copy of the aggregate's transactions, meaning it doesn't update the acquisitions it contains anymore.

That's now done upon applying the disposal events, as per the previous section.

SharePoolingAsset

The aggregate root now only stores reverted disposals as unprocessed, as these need to keep their position in the array of transactions until they are replayed.

Be it an acquisition or a disposal, the concerned disposals are reverted first, then the acquisition or disposal is processed, then the reverted disposals are replayed.

This simplifies the logic within DisposalBuilder and ReversionFinder.

Checklist

  • I have provided a summary and an explanation
  • I have reviewed the PR myself and left comments to provide context
  • I have covered the changes with tests as appropriate
  • I have made sure static analysis and other checks are successful

Comment on lines -149 to -166
// Also deduct same-day disposals with available same-day quantity that haven't been processed yet
$sameDayDisposals = $transactions->disposalsMadeOn($acquisition->date)
->unprocessed()
->withAvailableSameDayQuantity();

foreach ($sameDayDisposals as $disposal) {
$sameDayQuantityToApply = Quantity::minimum($disposal->availableSameDayQuantity(), $thirtyDayQuantityToApply);
$disposal->sameDayQuantityAllocation->allocateQuantity($sameDayQuantityToApply, $acquisition);
$acquisition->increaseSameDayQuantity($sameDayQuantityToApply);
$thirtyDayQuantityToApply = $thirtyDayQuantityToApply->minus($sameDayQuantityToApply);
if ($thirtyDayQuantityToApply->isZero()) {
break;
}
}

if ($thirtyDayQuantityToApply->isZero()) {
continue;
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Disposals are not being added as unprocessed anymore, so this section is now redundant

Comment on lines +22 to +33
public static function applyDisposal(
SharePoolingAssetDisposal $disposal,
SharePoolingAssetTransactions $transactions,
): void {
foreach (self::getAcquisitions($disposal->sameDayQuantityAllocation, $transactions) as $acquisition) {
$acquisition->increaseSameDayQuantity($disposal->sameDayQuantityAllocation->quantityAllocatedTo($acquisition));
}

foreach (self::getAcquisitions($disposal->thirtyDayQuantityAllocation, $transactions) as $acquisition) {
$acquisition->increaseThirtyDayQuantity($disposal->thirtyDayQuantityAllocation->quantityAllocatedTo($acquisition));
}
}
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ An acquisition's same-day and 30-day quantities are now only adjusted upon applying a disposal's event, using this method

Comment on lines -26 to +45
try {
$acquisition->decreaseSameDayQuantity($disposal->sameDayQuantityAllocation->quantityAllocatedTo($acquisition));
} catch (SharePoolingAssetAcquisitionException) {
// @TODO When re-acquiring within 30 days an asset that was disposed of on the same day it was acquired,
// decreasing the same-day quantity of the concerned acquisitions fails, because at the time the latter
// were recorded within the SharePoolingAssetAcquired events that had no same-day quantity yet
}
$acquisition->decreaseSameDayQuantity($disposal->sameDayQuantityAllocation->quantityAllocatedTo($acquisition));
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ Since an acquisition's quantities are now adjusted upon applying a disposal's event, these quantities are always right and this hack is not necessary anymore

Comment on lines +161 to +162
// Restore quantities deducted from acquisitions whose quantities were allocated to the disposal
QuantityAdjuster::revertDisposal($event->disposal, $this->transactions);
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ This is now done upon applying the reversion event, meaning the quantities are always right

@osteel osteel force-pushed the feature/quantity-allocation-refactoring branch from bdfa8a2 to ecc1a3d Compare September 14, 2023 10:37
@osteel osteel force-pushed the feature/quantity-allocation-refactoring branch from ecc1a3d to 4b29ccf Compare September 14, 2023 11:00
Comment on lines 66 to +82
public function increaseSameDayQuantity(Quantity $quantity): self
{
if ($quantity->isGreaterThan($availableQuantity = $this->section104PoolQuantity())) {
throw SharePoolingAssetAcquisitionException::insufficientSameDayQuantityToIncrease($quantity, $availableQuantity);
}

$this->sameDayQuantity = $this->sameDayQuantity->plus($quantity);

return $this;
}

/**
* Increase the same-day quantity and adjust the 30-day quantity accordingly.
*
* @return Quantity the added quantity
*/
public function increaseSameDayQuantityUpToAvailableQuantity(Quantity $quantity): Quantity
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ There are now two separate methods – increaseSameDayQuantity and increaseSameDayQuantityUpToAvailableQuantity.

The former will throw an exception if the specified quantity exceeds the currently unallocated same-day quantity and is used by the QuantityAdjuster service (see further down).

The latter will allocate as much as possible the specified quantity, also reallocating some or all of the 30-day quantity to the same-day quantity as necessary.

@osteel osteel marked this pull request as ready for review September 14, 2023 11:15
@@ -74,14 +89,14 @@ public function increaseSameDayQuantity(Quantity $quantity): self
$quantityToDeduct = Quantity::minimum($quantityToAdd, $this->thirtyDayQuantity);
$this->thirtyDayQuantity = $this->thirtyDayQuantity->minus($quantityToDeduct);

return $this;
return $quantityToAdd;
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ℹ️ increaseSameDayQuantityUpToAvailableQuantity and increaseThirtyDayQuantityUpToAvailableQuantity (below) now return the quantity that has actually been applied, instead of duplicating that logic in DisposalBuilder

@osteel osteel merged commit 1991562 into main Sep 14, 2023
4 checks passed
@osteel osteel deleted the feature/quantity-allocation-refactoring branch September 14, 2023 11:18
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