RomanNum3ral
/
whisper-money
mirror of https://github.com/whisper-money/whisper-money.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

refactor(accounts): split store/update in the account controller (#776) 

## Why

`Settings\AccountController` held two of the remaining complexity
offenders — `store` at **16** and `update` at **11** — and the two
clones jscpd reports inside the file:

```
AccountController.php [75:9 - 83:47]  ↔  [201:9 - 209:47]   (the nine real-estate fields)
AccountController.php [115:9 - 125:35] ↔ [217:9 - 227:35]   (the three loan fields)
```

Both actions were doing four jobs at once: map the request onto columns,
create or update the type-specific detail row, backfill balance history,
and answer.

## What changed

**The shared mapping** — `accountAttributes()`, `realEstateAttributes()`
and `loanAttributes()`, used by both actions. That is both clones gone.

**`store`'s type-specific work** moves out:

| new method | job |
|---|---|
| `createRealEstateDetail()` | the detail row + its balance history |
| `createLoanDetail()` | the detail row + its balance history |
| `linkToRealEstateAccount()` | points the property back at its new
mortgage |
| `backfillHistoricalBalances()` | "last twelve months now, older on the
queue" — both paths ended in this same dance |

**`update`'s loan branch** (`syncLoanDetail()`) returns the missing
fields instead of `return to_route(...)->withErrors(...)` from inside a
nested `else`, so the redirect decision stays in the action.

## Metrics

| | before | after |
|---|---|---|
| methods over complexity 10 | 30 | 28 |
| `store` | 16 | 5 |
| `update` | 11 | 4 |
| clones in this file | 2 | 0 |

## Testing

`AccountControllerTest`, `LoanTest`, `RealEstateTest`,
`RealEstateAvailabilityTest`, `AccountBalanceControllerTest`,
`AccountUserCurrencyServiceTest` — 148 tests green. PHPStan clean.

Three of the branches I moved had no coverage, so this adds it:

- creating a loan that started four years ago **queues**
`GenerateHistoricalLoanBalancesJob` and still writes the recent balances
inline (so the chart is not empty while the queue catches up)
- one that started two months ago queues **nothing** — the `isBefore`
condition that is the whole point of `backfillHistoricalBalances`
- adding loan details with only one of the three required fields comes
back with errors on the other two and writes no row. Worth noting these
errors come from the controller, not the request: `loanDetailRules()`
marks all three `nullable`.

The mortgage back-link was already covered by `LoanTest`.
This commit is contained in:
Víctor Falcón 2026-08-11 15:58:47 +02:00 committed by GitHub
parent 1553325c4f
commit 6803f2cf0e
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 253 additions and 135 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

319
app/Http/Controllers/Settings/AccountController.php
View File

@ -54,15 +54,7 @@ class AccountController extends Controller
$validated = $request->validated();
$balance = $validated['balance'] ?? null;
$accountData = collect($validated)->only([
'name', 'bank_id', 'currency_code', 'type',
])->toArray();
$account = $user->accounts()->create([
...$accountData,
'encrypted' => false,
'name_iv' => null,
]);
$account = $user->accounts()->create($this->accountAttributes($validated));
if ($balance !== null) {
$account->balances()->create([
@ -71,102 +63,13 @@ class AccountController extends Controller
]);
}
// Create real estate detail if account type is real_estate
if ($account->type === AccountType::RealEstate) {
$realEstateData = collect($validated)->only([
'property_type', 'address', 'purchase_price', 'purchase_date',
'area_value', 'area_unit', 'linked_loan_account_id', 'notes',
'revaluation_percentage',
])->filter(fn ($value) => $value !== null)->toArray();
if (! empty($realEstateData)) {
$account->realEstateDetail()->create($realEstateData);
}
// Generate historical balances when purchase data and current value are provided
if ($balance !== null && isset($validated['purchase_price'], $validated['purchase_date'])) {
$purchaseDate = Carbon::parse($validated['purchase_date']);
$twelveMonthsAgo = Carbon::today()->subMonths(12)->startOfMonth();
// Generate the last 12 months synchronously
$balanceGenerator->generateHistoricalBalances(
$account,
$validated['purchase_price'],
$purchaseDate,
$balance,
from: $twelveMonthsAgo,
);
// Dispatch older balances asynchronously if the purchase predates the sync window
if ($purchaseDate->isBefore($twelveMonthsAgo)) {
GenerateHistoricalRealEstateBalancesJob::dispatch(
$account,
$validated['purchase_price'],
$purchaseDate,
$balance,
$purchaseDate,
$twelveMonthsAgo->copy()->subDay(),
);
}
}
$this->createRealEstateDetail($account, $validated, $balance, $balanceGenerator);
}
// Create loan detail if account type is loan and loan fields are provided
if ($account->type === AccountType::Loan) {
$loanData = collect($validated)->only([
'annual_interest_rate', 'loan_term_months', 'original_amount',
])->filter(fn ($value) => $value !== null)->toArray();
$loanStartDate = $validated['loan_start_date'] ?? null;
if ($loanStartDate) {
$loanData['start_date'] = $loanStartDate;
}
if (! empty($loanData) && isset($loanData['annual_interest_rate'], $loanData['loan_term_months'], $loanData['original_amount'])) {
if (! isset($loanData['start_date'])) {
$loanData['start_date'] = now()->toDateString();
}
$loanDetail = $account->loanDetail()->create($loanData);
if ($balance !== null) {
$startDate = Carbon::parse($loanDetail->start_date);
$twelveMonthsAgo = Carbon::today()->subMonths(12)->startOfMonth();
$loanBalanceGenerator->generateHistoricalBalances(
$account,
(int) $loanDetail->original_amount,
$startDate,
$balance,
from: $twelveMonthsAgo,
);
if ($startDate->isBefore($twelveMonthsAgo)) {
GenerateHistoricalLoanBalancesJob::dispatch(
$account,
(int) $loanDetail->original_amount,
$startDate,
$balance,
$startDate,
$twelveMonthsAgo->copy()->subDay(),
);
}
}
}
$linkedRealEstateAccountId = $validated['linked_real_estate_account_id'] ?? null;
if ($linkedRealEstateAccountId !== null) {
$realEstateAccount = $user->accounts()
->whereKey($linkedRealEstateAccountId)
->where('type', AccountType::RealEstate->value)
->with('realEstateDetail')
->first();
$realEstateAccount?->realEstateDetail?->update([
'linked_loan_account_id' => $account->id,
]);
}
$this->createLoanDetail($account, $validated, $balance, $loanBalanceGenerator);
$this->linkToRealEstateAccount($user, $account, $validated['linked_real_estate_account_id'] ?? null);
}
$accountUserCurrencyService->syncFromFirstAccount($account);
@ -187,26 +90,12 @@ class AccountController extends Controller
$validated = $request->validated();
$accountData = collect($validated)->only([
'name', 'bank_id', 'currency_code', 'type',
'ownership_percentage', 'ownership_applies_to_balance',
])->toArray();
$account->update($this->accountAttributes($validated));
$account->update([
...$accountData,
'encrypted' => false,
'name_iv' => null,
]);
// Update or create real estate detail if account type is real_estate
if ($account->type === AccountType::RealEstate) {
$realEstateData = collect($validated)->only([
'property_type', 'address', 'purchase_price', 'purchase_date',
'area_value', 'area_unit', 'linked_loan_account_id', 'notes',
'revaluation_percentage',
])->filter(fn ($value) => $value !== null)->toArray();
$realEstateData = $this->realEstateAttributes($validated);
if (! empty($realEstateData)) {
if ($realEstateData !== []) {
$account->realEstateDetail()->updateOrCreate(
['account_id' => $account->id],
$realEstateData,
@ -215,10 +104,10 @@ class AccountController extends Controller
}
if ($account->type === AccountType::Loan) {
$incompleteLoan = $this->syncLoanDetail($account, $validated);
$errors = $this->syncLoanDetail($account, $validated);
if ($incompleteLoan !== null) {
return $incompleteLoan;
if ($errors !== []) {
return to_route('accounts.index')->withErrors($errors);
}
}
@ -226,54 +115,214 @@ class AccountController extends Controller
}
/**
* Update or create the account's loan detail from the validated payload.
*
* Returns a redirect carrying the missing-field errors when a loan detail
* has to be created but the payload is incomplete, and null otherwise.
* The account's own columns. Encryption is gone, so every write clears the
* legacy flags rather than leaving stale ones behind.
*
* @param array<string, mixed> $validated
* @return array<string, mixed>
*/
private function syncLoanDetail(Account $account, array $validated): ?RedirectResponse
private function accountAttributes(array $validated): array
{
return [
...collect($validated)->only([
'name', 'bank_id', 'currency_code', 'type',
'ownership_percentage', 'ownership_applies_to_balance',
])->toArray(),
'encrypted' => false,
'name_iv' => null,
];
}
/**
* @param array<string, mixed> $validated
* @return array<string, mixed>
*/
private function realEstateAttributes(array $validated): array
{
return collect($validated)->only([
'property_type', 'address', 'purchase_price', 'purchase_date',
'area_value', 'area_unit', 'linked_loan_account_id', 'notes',
'revaluation_percentage',
])->filter(fn ($value) => $value !== null)->toArray();
}
/**
* The loan's own columns, defaulting the start date to what the user sent.
*
* @param array<string, mixed> $validated
* @return array<string, mixed>
*/
private function loanAttributes(array $validated): array
{
$loanData = collect($validated)->only([
'annual_interest_rate', 'loan_term_months', 'original_amount',
])->filter(fn ($value) => $value !== null)->toArray();
$loanStartDate = $validated['loan_start_date'] ?? null;
if ($loanStartDate) {
$loanData['start_date'] = $loanStartDate;
}
if (empty($loanData)) {
return null;
return $loanData;
}
/**
* @param array<string, mixed> $validated
*/
private function createRealEstateDetail(Account $account, array $validated, ?int $balance, RealEstateBalanceGeneratorService $balanceGenerator): void
{
$realEstateData = $this->realEstateAttributes($validated);
if ($realEstateData !== []) {
$account->realEstateDetail()->create($realEstateData);
}
// Historical balances need both ends of the line: what it was bought for
// and what it is worth now.
if ($balance === null || ! isset($validated['purchase_price'], $validated['purchase_date'])) {
return;
}
$this->backfillHistoricalBalances(
Carbon::parse($validated['purchase_date']),
fn (Carbon $from) => $balanceGenerator->generateHistoricalBalances(
$account,
$validated['purchase_price'],
Carbon::parse($validated['purchase_date']),
$balance,
from: $from,
),
fn (Carbon $until) => GenerateHistoricalRealEstateBalancesJob::dispatch(
$account,
$validated['purchase_price'],
Carbon::parse($validated['purchase_date']),
$balance,
Carbon::parse($validated['purchase_date']),
$until,
),
);
}
/**
* @param array<string, mixed> $validated
*/
private function createLoanDetail(Account $account, array $validated, ?int $balance, LoanBalanceGeneratorService $loanBalanceGenerator): void
{
$loanData = $this->loanAttributes($validated);
if (! isset($loanData['annual_interest_rate'], $loanData['loan_term_months'], $loanData['original_amount'])) {
return;
}
$loanData['start_date'] ??= now()->toDateString();
$loanDetail = $account->loanDetail()->create($loanData);
if ($balance === null) {
return;
}
$startDate = Carbon::parse($loanDetail->start_date);
$this->backfillHistoricalBalances(
$startDate,
fn (Carbon $from) => $loanBalanceGenerator->generateHistoricalBalances(
$account,
(int) $loanDetail->original_amount,
$startDate,
$balance,
from: $from,
),
fn (Carbon $until) => GenerateHistoricalLoanBalancesJob::dispatch(
$account,
(int) $loanDetail->original_amount,
$startDate,
$balance,
$startDate,
$until,
),
);
}
/**
* Fill in the balance history for an account that existed before we knew about
* it: the last twelve months now, so the chart is populated on the next
* render, and anything older on the queue.
*
* @param callable(Carbon): mixed $generateRecent receives the month to start from
* @param callable(Carbon): mixed $queueOlder receives the day the recent window starts
*/
private function backfillHistoricalBalances(Carbon $startDate, callable $generateRecent, callable $queueOlder): void
{
$twelveMonthsAgo = Carbon::today()->subMonths(12)->startOfMonth();
$generateRecent($twelveMonthsAgo);
if ($startDate->isBefore($twelveMonthsAgo)) {
$queueOlder($twelveMonthsAgo->copy()->subDay());
}
}
/**
* Point the property this mortgage belongs to back at the loan account, so the
* two show up together on the property's chart.
*/
private function linkToRealEstateAccount(User $user, Account $loanAccount, ?string $realEstateAccountId): void
{
if ($realEstateAccountId === null) {
return;
}
$user->accounts()
->whereKey($realEstateAccountId)
->where('type', AccountType::RealEstate->value)
->with('realEstateDetail')
->first()
?->realEstateDetail
?->update(['linked_loan_account_id' => $loanAccount->id]);
}
/**
* Update the loan's details, or create them when the account did not have any
* yet. Returns the fields still missing when there is not enough to create a
* loan with, so the caller can send them back to the form.
*
* @param array<string, mixed> $validated
* @return array<string, string>
*/
private function syncLoanDetail(Account $account, array $validated): array
{
$loanData = $this->loanAttributes($validated);
if ($loanData === []) {
return [];
}
$existingLoanDetail = $account->loanDetail;
if ($existingLoanDetail) {
if ($existingLoanDetail !== null) {
$existingLoanDetail->update($loanData);
return null;
return [];
}
if (isset($loanData['annual_interest_rate'], $loanData['loan_term_months'], $loanData['original_amount'])) {
$loanData['start_date'] ??= now()->toDateString();
$account->loanDetail()->create($loanData);
return null;
return [];
}
$required = ['annual_interest_rate', 'loan_term_months', 'original_amount'];
$errors = [];
foreach ($required as $field) {
foreach (['annual_interest_rate', 'loan_term_months', 'original_amount'] as $field) {
if (! isset($loanData[$field])) {
$errors[$field] = __('This field is required.');
}
}
return to_route('accounts.index')->withErrors($errors);
return $errors;
}
/**

69
tests/Feature/LoanTest.php
View File

@ -1,6 +1,7 @@
<?php
use App\Enums\AccountType;
use App\Jobs\GenerateHistoricalLoanBalancesJob;
use App\Models\Account;
use App\Models\AccountBalance;
use App\Models\Bank;
@ -8,6 +9,7 @@ use App\Models\LoanDetail;
use App\Models\RealEstateDetail;
use App\Models\User;
use App\Services\LoanAmortizationService;
use Illuminate\Support\Facades\Queue;
use function Pest\Laravel\actingAs;
use function Pest\Laravel\artisan;
@ -1019,3 +1021,70 @@ it('preserves loan detail when account is soft deleted', function () {
assertDatabaseHas('loan_details', ['id' => $detail->id]);
});
it('generates the recent balances inline and queues only the ones older than a year', function () {
Queue::fake();
actingAs($this->user);
$this->post(route('accounts.store'), [
'name' => 'Old Mortgage',
'bank_id' => $this->bank->id,
'currency_code' => 'USD',
'type' => AccountType::Loan->value,
'annual_interest_rate' => 3.5,
'loan_term_months' => 360,
'original_amount' => 20000000,
// Well before the twelve-month window the controller fills in inline.
'loan_start_date' => now()->subYears(4)->toDateString(),
'balance' => 15000000,
])->assertRedirect();
Queue::assertPushed(GenerateHistoricalLoanBalancesJob::class);
$account = Account::query()->where('name', 'Old Mortgage')->sole();
// The last twelve months are written straight away, so the chart is not empty
// while the queue catches up.
expect(AccountBalance::query()->where('account_id', $account->id)->count())
->toBeGreaterThan(1);
});
it('does not queue anything when the loan starts inside the twelve-month window', function () {
Queue::fake();
actingAs($this->user);
$this->post(route('accounts.store'), [
'name' => 'Recent Mortgage',
'bank_id' => $this->bank->id,
'currency_code' => 'USD',
'type' => AccountType::Loan->value,
'annual_interest_rate' => 3.5,
'loan_term_months' => 360,
'original_amount' => 20000000,
'loan_start_date' => now()->subMonths(2)->toDateString(),
'balance' => 19000000,
])->assertRedirect();
Queue::assertNotPushed(GenerateHistoricalLoanBalancesJob::class);
});
it('reports the missing fields when adding loan details to an account that has none', function () {
actingAs($this->user);
$account = Account::factory()->create([
'user_id' => $this->user->id,
'type' => AccountType::Loan,
'currency_code' => 'USD',
]);
// Only one of the three fields a loan detail needs.
$this->patch(route('accounts.update', $account), [
'name' => $account->name,
'bank_id' => $this->bank->id,
'currency_code' => 'USD',
'type' => AccountType::Loan->value,
'annual_interest_rate' => 3.5,
])->assertSessionHasErrors(['loan_term_months', 'original_amount']);
assertDatabaseMissing('loan_details', ['account_id' => $account->id]);
});