From e1aa63487abd3ed03a8720a556e7cc94dac52258 Mon Sep 17 00:00:00 2001 From: James Cole Date: Fri, 5 Jun 2015 12:18:20 +0200 Subject: [PATCH] Optimized a validator. [skip ci] --- .../Account/AccountRepository.php | 98 ++++++------ app/Validation/FireflyValidator.php | 146 +++++++++++++----- tests/controllers/AccountControllerTest.php | 7 +- tests/repositories/AccountRepositoryTest.php | 15 +- 4 files changed, 173 insertions(+), 93 deletions(-) diff --git a/app/Repositories/Account/AccountRepository.php b/app/Repositories/Account/AccountRepository.php index 7b16735c03..dd413a236e 100644 --- a/app/Repositories/Account/AccountRepository.php +++ b/app/Repositories/Account/AccountRepository.php @@ -65,7 +65,7 @@ class AccountRepository implements AccountRepositoryInterface public function getAccounts(array $types) { $result = Auth::user()->accounts()->with( - ['accountmeta' => function(HasMany $query) { + ['accountmeta' => function (HasMany $query) { $query->where('name', 'accountRole'); }] )->accountTypeIn($types)->orderBy('accounts.name', 'ASC')->get(['accounts.*'])->sortBy('name'); @@ -80,15 +80,15 @@ class AccountRepository implements AccountRepositoryInterface public function getCreditCards() { return Auth::user()->accounts() - ->hasMetaValue('accountRole', 'ccAsset') - ->hasMetaValue('ccType', 'monthlyFull') - ->get( - [ - 'accounts.*', - 'ccType.data as ccType', - 'accountRole.data as accountRole' - ] - ); + ->hasMetaValue('accountRole', 'ccAsset') + ->hasMetaValue('ccType', 'monthlyFull') + ->get( + [ + 'accounts.*', + 'ccType.data as ccType', + 'accountRole.data as accountRole' + ] + ); } /** @@ -151,18 +151,18 @@ class AccountRepository implements AccountRepositoryInterface } $set = Auth::user() - ->transactionjournals() - ->with(['transactions']) - ->leftJoin('transactions', 'transactions.transaction_journal_id', '=', 'transaction_journals.id') - ->leftJoin('accounts', 'accounts.id', '=', 'transactions.account_id')->where('accounts.id', $account->id) - ->leftJoin('transaction_currencies', 'transaction_currencies.id', '=', 'transaction_journals.transaction_currency_id') - ->leftJoin('transaction_types', 'transaction_types.id', '=', 'transaction_journals.transaction_type_id') - ->before($end) - ->after($start) - ->orderBy('transaction_journals.date', 'DESC') - ->orderBy('transaction_journals.id', 'DESC') - ->take(10) - ->get(['transaction_journals.*', 'transaction_currencies.symbol', 'transaction_types.type']); + ->transactionjournals() + ->with(['transactions']) + ->leftJoin('transactions', 'transactions.transaction_journal_id', '=', 'transaction_journals.id') + ->leftJoin('accounts', 'accounts.id', '=', 'transactions.account_id')->where('accounts.id', $account->id) + ->leftJoin('transaction_currencies', 'transaction_currencies.id', '=', 'transaction_journals.transaction_currency_id') + ->leftJoin('transaction_types', 'transaction_types.id', '=', 'transaction_journals.transaction_type_id') + ->before($end) + ->after($start) + ->orderBy('transaction_journals.date', 'DESC') + ->orderBy('transaction_journals.id', 'DESC') + ->take(10) + ->get(['transaction_journals.*', 'transaction_currencies.symbol', 'transaction_types.type']); $cache->store($set); return $set; @@ -178,13 +178,13 @@ class AccountRepository implements AccountRepositoryInterface { $offset = ($page - 1) * 50; $query = Auth::user() - ->transactionJournals() - ->withRelevantData() - ->leftJoin('transactions', 'transactions.transaction_journal_id', '=', 'transaction_journals.id') - ->where('transactions.account_id', $account->id) - ->orderBy('transaction_journals.date', 'DESC') - ->orderBy('transaction_journals.order', 'ASC') - ->orderBy('transaction_journals.id', 'DESC'); + ->transactionJournals() + ->withRelevantData() + ->leftJoin('transactions', 'transactions.transaction_journal_id', '=', 'transaction_journals.id') + ->where('transactions.account_id', $account->id) + ->orderBy('transaction_journals.date', 'DESC') + ->orderBy('transaction_journals.order', 'ASC') + ->orderBy('transaction_journals.id', 'DESC'); $count = $query->count(); $set = $query->take(50)->offset($offset)->get(['transaction_journals.*']); @@ -242,7 +242,7 @@ class AccountRepository implements AccountRepositoryInterface } $accounts->each( - function(Account $account) use ($start, $end) { + function (Account $account) use ($start, $end) { $account->startBalance = Steam::balance($account, $start, true); $account->endBalance = Steam::balance($account, $end, true); $account->piggyBalance = 0; @@ -282,7 +282,7 @@ class AccountRepository implements AccountRepositoryInterface $end = clone Session::get('end', new Carbon); $accounts->each( - function(Account $account) use ($start, $end) { + function (Account $account) use ($start, $end) { $account->startBalance = Steam::balance($account, $start); $account->endBalance = Steam::balance($account, $end); @@ -320,22 +320,22 @@ class AccountRepository implements AccountRepositoryInterface */ public function getTransfersInRange(Account $account, Carbon $start, Carbon $end) { - $set = TransactionJournal::whereIn( - 'id', function(Builder $q) use ($account, $start, $end) { + $set = TransactionJournal::whereIn( + 'id', function (Builder $q) use ($account, $start, $end) { $q->select('transaction_journals.id') - ->from('transactions') - ->leftJoin('transaction_journals', 'transaction_journals.id', '=', 'transactions.transaction_journal_id') - ->leftJoin('transaction_types', 'transaction_types.id', '=', 'transaction_journals.transaction_type_id') - ->where('transactions.account_id', $account->id) - ->where('transaction_journals.user_id', Auth::user()->id) - ->where('transaction_journals.date', '>=', $start->format('Y-m-d')) - ->where('transaction_journals.date', '<=', $end->format('Y-m-d')) - ->where('transaction_types.type', 'Transfer'); + ->from('transactions') + ->leftJoin('transaction_journals', 'transaction_journals.id', '=', 'transactions.transaction_journal_id') + ->leftJoin('transaction_types', 'transaction_types.id', '=', 'transaction_journals.transaction_type_id') + ->where('transactions.account_id', $account->id) + ->where('transaction_journals.user_id', Auth::user()->id) + ->where('transaction_journals.date', '>=', $start->format('Y-m-d')) + ->where('transaction_journals.date', '<=', $end->format('Y-m-d')) + ->where('transaction_types.type', 'Transfer'); } )->get(); $filtered = $set->filter( - function(TransactionJournal $journal) use ($account) { + function (TransactionJournal $journal) use ($account) { if ($journal->destination_account->id == $account->id) { return $journal; } @@ -374,10 +374,10 @@ class AccountRepository implements AccountRepositoryInterface public function openingBalanceTransaction(Account $account) { return TransactionJournal - ::orderBy('transaction_journals.date', 'ASC') - ->accountIs($account) - ->orderBy('created_at', 'ASC') - ->first(['transaction_journals.*']); + ::orderBy('transaction_journals.date', 'ASC') + ->accountIs($account) + ->orderBy('created_at', 'ASC') + ->first(['transaction_journals.*']); } /** @@ -401,7 +401,7 @@ class AccountRepository implements AccountRepositoryInterface 'name' => $data['name'] . ' initial balance', 'active' => false, ]; - $opposing = $this->storeAccount($opposingData); + $opposing = $this->storeAccount($opposingData); $this->storeInitialBalance($newAccount, $opposing, $data); } @@ -452,7 +452,7 @@ class AccountRepository implements AccountRepositoryInterface 'active' => false, 'virtualBalance' => 0, ]; - $opposing = $this->storeAccount($opposingData); + $opposing = $this->storeAccount($opposingData); $this->storeInitialBalance($account, $opposing, $data); } @@ -486,7 +486,7 @@ class AccountRepository implements AccountRepositoryInterface if (!$newAccount->isValid()) { // does the account already exist? - $searchData = [ + $searchData = [ 'user_id' => $data['user'], 'account_type_id' => $accountType->id, 'virtual_balance' => $data['virtualBalance'], diff --git a/app/Validation/FireflyValidator.php b/app/Validation/FireflyValidator.php index 59de869bd8..b1d1e18af3 100644 --- a/app/Validation/FireflyValidator.php +++ b/app/Validation/FireflyValidator.php @@ -9,9 +9,8 @@ use Crypt; use DB; use FireflyIII\Models\Account; use FireflyIII\Models\AccountType; -use Illuminate\Contracts\Encryption\DecryptException; +use FireflyIII\User; use Illuminate\Validation\Validator; -use Log; use Navigation; use Symfony\Component\Translation\TranslatorInterface; @@ -93,54 +92,129 @@ class FireflyValidator extends Validator */ public function validateUniqueAccountForUser($attribute, $value, $parameters) { - $type = null; - - /** - * Switch on different cases on which this method can respond: - */ - $hasWhat = isset($this->data['what']); - $hasAccountTypeId = isset($this->data['account_type_id']) && isset($this->data['name']); - $hasAccountId = isset($this->data['id']); - $ignoreId = 0; - - - if ($hasWhat) { - $search = Config::get('firefly.accountTypeByIdentifier.' . $this->data['what']); - $type = AccountType::whereType($search)->first(); - // this field can be used to find the exact type, and continue. + // because a user does not have to be logged in (tests and what-not). + if (!Auth::check()) { + return $this->validateAccountAnonymously(); } - if ($hasAccountTypeId) { + if (isset($this->data['what'])) { + return $this->validateByAccountTypeString($value, $parameters); + } + + if (isset($this->data['account_type_id'])) { + return $this->validateByAccountTypeId($value, $parameters); + } + + + var_dump($attribute); + var_dump($value); + var_dump($parameters); + var_dump($this->data); + + exit; + + + // try to determin type of account: + if (!empty($this->data['what'])) { + $search = Config::get('firefly.accountTypeByIdentifier.' . $this->data['what']); + $type = AccountType::whereType($search)->first(); + } else { $type = AccountType::find($this->data['account_type_id']); } - if ($hasAccountId) { - /** @var Account $account */ - $account = Account::find($this->data['id']); - $ignoreId = intval($this->data['id']); - $type = AccountType::find($account->account_type_id); - unset($account); + // ignore itself, if parameter is given: + if (isset($parameters[0])) { + $ignoreId = $parameters[0]; + } else { + $ignoreId = 0; } - /** - * Try to decrypt data just in case: - */ - try { - $value = Crypt::decrypt($value); - } catch (DecryptException $e) { - // if it fails, probably not encrypted. + // reset to basic check, see what happens: + $userId = Auth::user()->id; + $ignoreId = intval($this->data['id']); + + $set = Account::where('account_type_id', $type->id)->where('id', '!=', $ignoreId)->where('user_id', $userId)->get(); + /** @var Account $entry */ + foreach ($set as $entry) { + if ($entry->name == $value) { + return false; + } } + return true; - if (is_null($type)) { - Log::error('Could not determine type of account to validate.'); + } + /** + * @return bool + */ + protected function validateAccountAnonymously() + { + if (!isset($this->data['user_id'])) { return false; } - // get all accounts with this type, and find the name. - $userId = Auth::check() ? Auth::user()->id : 0; - $set = Account::where('account_type_id', $type->id)->where('id', '!=', $ignoreId)->where('user_id', $userId)->get(); + $user = User::find($this->data['user_id']); + $type = AccountType::find($this->data['account_type_id'])->first(); + $value = $this->data['name']; + + // decrypt if necessary + if (intval($this->data['encrypted']) === 1) { + $value = Crypt::decrypt($this->data['name']); + } + + + $set = $user->accounts()->where('account_type_id', $type->id)->get(); + /** @var Account $entry */ + foreach ($set as $entry) { + if ($entry->name == $value) { + return false; + } + } + + return true; + } + + /** + * @param $value + * @param $parameters + * + * @return bool + */ + protected function validateByAccountTypeString($value, $parameters) + { + $search = Config::get('firefly.accountTypeByIdentifier.' . $this->data['what']); + $type = AccountType::whereType($search)->first(); + $ignore = isset($parameters[0]) ? intval($parameters[0]) : 0; + + $set = Auth::user()->accounts()->where('account_type_id', $type->id)->where('id', '!=', $ignore)->get(); + /** @var Account $entry */ + foreach ($set as $entry) { + if ($entry->name == $value) { + return false; + } + } + + return true; + } + + /** + * @param $value + * @param $parameters + * + * @return bool + */ + protected function validateByAccountTypeId($value, $parameters) + { + $type = AccountType::find($this->data['account_type_id'])->first(); + $ignore = isset($parameters[0]) ? intval($parameters[0]) : 0; + + // if is encrypted, decrypt: + if (intval($this->data['encrypted']) === 1) { + $value = Crypt::decrypt($value); + } + + $set = Auth::user()->accounts()->where('account_type_id', $type->id)->where('id', '!=', $ignore)->get(); /** @var Account $entry */ foreach ($set as $entry) { if ($entry->name == $value) { diff --git a/tests/controllers/AccountControllerTest.php b/tests/controllers/AccountControllerTest.php index d1b548e774..86d5be5128 100644 --- a/tests/controllers/AccountControllerTest.php +++ b/tests/controllers/AccountControllerTest.php @@ -21,6 +21,7 @@ class AccountControllerTest extends TestCase public function setUp() { parent::setUp(); + $this->createAccount(); } @@ -49,8 +50,12 @@ class AccountControllerTest extends TestCase */ public function createAccount() { + $user = FactoryMuffin::create('FireflyIII\User'); + $this->be($user); if (is_null($this->account)) { - $this->account = FactoryMuffin::create('FireflyIII\Models\Account'); + $this->account = FactoryMuffin::create('FireflyIII\Models\Account'); + $this->account->user_id = $user->id; + $this->account->save(); } } diff --git a/tests/repositories/AccountRepositoryTest.php b/tests/repositories/AccountRepositoryTest.php index c2bbaf16cf..876a5c4286 100644 --- a/tests/repositories/AccountRepositoryTest.php +++ b/tests/repositories/AccountRepositoryTest.php @@ -603,6 +603,8 @@ class AccountRepositoryTest extends TestCase } /** + * This function should give a big fat error, but it doesnt. + * * @covers FireflyIII\Repositories\Account\AccountRepository::store * @covers FireflyIII\Repositories\Account\AccountRepository::storeAccount * @covers FireflyIII\Repositories\Account\AccountRepository::storeMetadata @@ -610,12 +612,12 @@ class AccountRepositoryTest extends TestCase */ public function testStoreWithExistingAccount() { - $account = FactoryMuffin::create('FireflyIII\Models\Account'); - FactoryMuffin::create('FireflyIII\Models\AccountType'); - FactoryMuffin::create('FireflyIII\Models\AccountType'); - FactoryMuffin::create('FireflyIII\Models\TransactionType'); - FactoryMuffin::create('FireflyIII\Models\TransactionType'); - FactoryMuffin::create('FireflyIII\Models\TransactionType'); + $account = FactoryMuffin::create('FireflyIII\Models\Account'); // expense + FactoryMuffin::create('FireflyIII\Models\AccountType'); // revenue + FactoryMuffin::create('FireflyIII\Models\AccountType'); // asset + FactoryMuffin::create('FireflyIII\Models\TransactionType'); // withdrawal + FactoryMuffin::create('FireflyIII\Models\TransactionType'); // deposit + FactoryMuffin::create('FireflyIII\Models\TransactionType'); // transfer $currency = FactoryMuffin::create('FireflyIII\Models\TransactionCurrency'); $this->be($account->user); @@ -632,7 +634,6 @@ class AccountRepositoryTest extends TestCase 'openingBalanceDate' => '2015-01-01', ]; - $newAccount = $this->object->store($data); $this->assertEquals($account->name, $newAccount->name);