From e1186b48eca8bde2204428c8084263c9ebcc8e44 Mon Sep 17 00:00:00 2001 From: James Cole Date: Mon, 30 Dec 2019 20:44:52 +0100 Subject: [PATCH] Fix account name validator and make sure the location CRUD works in the API for accounts. --- app/Api/V1/Controllers/TagController.php | 11 +-- app/Api/V1/Requests/AccountStoreRequest.php | 3 + app/Api/V1/Requests/AccountUpdateRequest.php | 13 +++ app/Api/V1/Requests/Request.php | 9 +- .../{TagRequest.php => TagStoreRequest.php} | 26 ++---- app/Api/V1/Requests/TagUpdateRequest.php | 82 +++++++++++++++++++ app/Factory/AccountFactory.php | 2 +- .../Internal/Update/AccountUpdateService.php | 34 +++++--- app/Validation/FireflyValidator.php | 42 +++++++--- 9 files changed, 170 insertions(+), 52 deletions(-) rename app/Api/V1/Requests/{TagRequest.php => TagStoreRequest.php} (79%) create mode 100644 app/Api/V1/Requests/TagUpdateRequest.php diff --git a/app/Api/V1/Controllers/TagController.php b/app/Api/V1/Controllers/TagController.php index ecbc99c026..8e17589977 100644 --- a/app/Api/V1/Controllers/TagController.php +++ b/app/Api/V1/Controllers/TagController.php @@ -25,7 +25,8 @@ namespace FireflyIII\Api\V1\Controllers; use Carbon\Carbon; use FireflyIII\Api\V1\Requests\DateRequest; -use FireflyIII\Api\V1\Requests\TagRequest; +use FireflyIII\Api\V1\Requests\TagUpdateRequest; +use FireflyIII\Api\V1\Requests\TagStoreRequest; use FireflyIII\Helpers\Collector\GroupCollectorInterface; use FireflyIII\Models\Tag; use FireflyIII\Repositories\Tag\TagRepositoryInterface; @@ -161,11 +162,11 @@ class TagController extends Controller /** * Store new object. * - * @param TagRequest $request + * @param TagStoreRequest $request * * @return JsonResponse */ - public function store(TagRequest $request): JsonResponse + public function store(TagStoreRequest $request): JsonResponse { $rule = $this->repository->store($request->getAll()); $manager = $this->getManager(); @@ -234,12 +235,12 @@ class TagController extends Controller /** * Update a rule. * - * @param TagRequest $request + * @param TagUpdateRequest $request * @param Tag $tag * * @return JsonResponse */ - public function update(TagRequest $request, Tag $tag): JsonResponse + public function update(TagUpdateRequest $request, Tag $tag): JsonResponse { $rule = $this->repository->update($tag, $request->getAll()); $manager = $this->getManager(); diff --git a/app/Api/V1/Requests/AccountStoreRequest.php b/app/Api/V1/Requests/AccountStoreRequest.php index f43798ed96..74c3bc5f96 100644 --- a/app/Api/V1/Requests/AccountStoreRequest.php +++ b/app/Api/V1/Requests/AccountStoreRequest.php @@ -77,6 +77,9 @@ class AccountStoreRequest extends Request 'interest' => 'required_if:type,liability|between:0,100|numeric', 'interest_period' => 'required_if:type,liability|in:daily,monthly,yearly', 'notes' => 'min:0|max:65536', + 'latitude' => 'numeric|min:-90|max:90|nullable|required_with:longitude', + 'longitude' => 'numeric|min:-180|max:180|nullable|required_with:latitude', + 'zoom_level' => 'numeric|min:0|max:80|nullable|required_with:latitude', ]; return $rules; diff --git a/app/Api/V1/Requests/AccountUpdateRequest.php b/app/Api/V1/Requests/AccountUpdateRequest.php index f62f057b11..2d7a782c7e 100644 --- a/app/Api/V1/Requests/AccountUpdateRequest.php +++ b/app/Api/V1/Requests/AccountUpdateRequest.php @@ -58,6 +58,11 @@ class AccountUpdateRequest extends Request if (null !== $this->get('include_net_worth')) { $includeNetWorth = $this->boolean('include_net_worth'); } + $updateLocation = false; + + if ($this->has('longitude') && $this->has('latitude') && $this->has('zoom_level')) { + $updateLocation = true; + } $data = [ 'name' => $this->nullableString('name'), @@ -79,6 +84,10 @@ class AccountUpdateRequest extends Request 'notes' => $this->nullableNlString('notes'), 'interest' => $this->nullableString('interest'), 'interest_period' => $this->nullableString('interest_period'), + 'has_location' => $updateLocation, + 'longitude' => '' === $this->string('longitude') ? null : $this->string('longitude'), + 'latitude' => '' === $this->string('latitude') ? null : $this->string('latitude'), + 'zoom_level' => '' === $this->string('zoom_level') ? null : $this->integer('zoom_level'), ]; if ('liability' === $data['account_type']) { @@ -102,6 +111,7 @@ class AccountUpdateRequest extends Request $accountRoles = implode(',', config('firefly.accountRoles')); $types = implode(',', array_keys(config('firefly.subTitlesByIdentifier'))); $ccPaymentTypes = implode(',', array_keys(config('firefly.ccTypes'))); + $rules = [ 'name' => sprintf('min:1|uniqueAccountForUser:%d', $account->id), 'type' => sprintf('in:%s', $types), @@ -124,6 +134,9 @@ class AccountUpdateRequest extends Request 'interest' => 'required_if:type,liability|between:0,100|numeric', 'interest_period' => 'required_if:type,liability|in:daily,monthly,yearly', 'notes' => 'min:0|max:65536', + 'latitude' => 'numeric|min:-90|max:90|nullable|required_with:longitude', + 'longitude' => 'numeric|min:-180|max:180|nullable|required_with:latitude', + 'zoom_level' => 'numeric|min:0|max:80|nullable|required_with:latitude', ]; return $rules; diff --git a/app/Api/V1/Requests/Request.php b/app/Api/V1/Requests/Request.php index 76beaf4e3c..738f2f1a2b 100644 --- a/app/Api/V1/Requests/Request.php +++ b/app/Api/V1/Requests/Request.php @@ -41,13 +41,16 @@ class Request extends FireflyIIIRequest { $active = true; $includeNetWorth = true; + $hasLocation = false; if (null !== $this->get('active')) { $active = $this->boolean('active'); } if (null !== $this->get('include_net_worth')) { $includeNetWorth = $this->boolean('include_net_worth'); } - + if ($this->has('longitude') && $this->has('latitude') && $this->has('zoom_level')) { + $hasLocation = true; + } $data = [ 'name' => $this->string('name'), 'active' => $active, @@ -68,6 +71,10 @@ class Request extends FireflyIIIRequest 'notes' => $this->nlString('notes'), 'interest' => $this->string('interest'), 'interest_period' => $this->string('interest_period'), + 'has_location' => $hasLocation, + 'longitude' => $this->string('longitude'), + 'latitude' => $this->string('latitude'), + 'zoom_level' => $this->integer('zoom_level'), ]; if ('liability' === $data['account_type']) { diff --git a/app/Api/V1/Requests/TagRequest.php b/app/Api/V1/Requests/TagStoreRequest.php similarity index 79% rename from app/Api/V1/Requests/TagRequest.php rename to app/Api/V1/Requests/TagStoreRequest.php index 075e884b62..472fffa6eb 100644 --- a/app/Api/V1/Requests/TagRequest.php +++ b/app/Api/V1/Requests/TagStoreRequest.php @@ -1,7 +1,6 @@ $this->string('tag'), 'date' => $this->date('date'), 'description' => $this->string('description'), @@ -62,8 +60,6 @@ class TagRequest extends Request 'longitude' => '' === $this->string('longitude') ? null : $this->string('longitude'), 'zoom_level' => $this->integer('zoom_level'), ]; - - return $data; } /** @@ -73,7 +69,7 @@ class TagRequest extends Request */ public function rules(): array { - $rules = [ + return [ 'tag' => 'required|min:1|uniqueObjectForUser:tags,tag', 'description' => 'min:1|nullable', 'date' => 'date|nullable', @@ -81,17 +77,5 @@ class TagRequest extends Request 'longitude' => 'numeric|min:-180|max:180|nullable|required_with:latitude', 'zoom_level' => 'numeric|min:0|max:80|nullable', ]; - switch ($this->method()) { - default: - break; - case 'PUT': - case 'PATCH': - /** @var Tag $tag */ - $tag = $this->route()->parameter('tagOrId'); - $rules['tag'] = 'required|min:1|uniqueObjectForUser:tags,tag,' . $tag->id; - break; - } - - return $rules; } } diff --git a/app/Api/V1/Requests/TagUpdateRequest.php b/app/Api/V1/Requests/TagUpdateRequest.php new file mode 100644 index 0000000000..c833aeef4a --- /dev/null +++ b/app/Api/V1/Requests/TagUpdateRequest.php @@ -0,0 +1,82 @@ +. + */ + +declare(strict_types=1); + +namespace FireflyIII\Api\V1\Requests; + +/** + * Class TagUpdateRequest + * + * @codeCoverageIgnore + * + */ +class TagUpdateRequest extends Request +{ + + /** + * Authorize logged in users. + * + * @return bool + */ + public function authorize(): bool + { + // Only allow authenticated users + return auth()->check(); + } + + /** + * Get all data from the request. + * + * @return array + */ + public function getAll(): array + { + return [ + 'tag' => $this->string('tag'), + 'date' => $this->date('date'), + 'description' => $this->string('description'), + 'latitude' => '' === $this->string('latitude') ? null : $this->string('latitude'), + 'longitude' => '' === $this->string('longitude') ? null : $this->string('longitude'), + 'zoom_level' => $this->integer('zoom_level'), + ]; + } + + /** + * The rules that the incoming request must be matched against. + * + * @return array + */ + public function rules(): array + { + $tag = $this->route()->parameter('tagOrId'); + + return [ + 'tag' => 'required|min:1|uniqueObjectForUser:tags,tag,' . $tag->id, + 'description' => 'min:1|nullable', + 'date' => 'date|nullable', + 'latitude' => 'numeric|min:-90|max:90|nullable|required_with:longitude', + 'longitude' => 'numeric|min:-180|max:180|nullable|required_with:latitude', + 'zoom_level' => 'numeric|min:0|max:80|nullable', + ]; + } +} diff --git a/app/Factory/AccountFactory.php b/app/Factory/AccountFactory.php index cc1ce17b89..91c37a0da9 100644 --- a/app/Factory/AccountFactory.php +++ b/app/Factory/AccountFactory.php @@ -134,7 +134,7 @@ class AccountFactory $this->updateNote($return, $data['notes'] ?? ''); // store location - if (true === ($data['has_location'] ?? true) && null !== $return) { + if (true === ($data['has_location'] ?? false) && null !== $return) { $location = new Location; $location->latitude = $data['latitude'] ?? 52.3167; $location->longitude = $data['longitude'] ?? 5.55; diff --git a/app/Services/Internal/Update/AccountUpdateService.php b/app/Services/Internal/Update/AccountUpdateService.php index a92528d523..e32d0c1cd6 100644 --- a/app/Services/Internal/Update/AccountUpdateService.php +++ b/app/Services/Internal/Update/AccountUpdateService.php @@ -101,20 +101,28 @@ class AccountUpdateService $this->updateMetaData($account, $data); // update, delete or create location: - $hasLocation = $data['has_location'] ?? false; - if (false === $hasLocation) { - $account->locations()->delete(); - } - if (true === $hasLocation) { - $location = $this->accountRepository->getLocation($account); - if (null === $location) { - $location = new Location; - $location->locatable()->associate($account); + $updateLocation = $data['has_location'] ?? false; + + // location must be updated? + if (true === $updateLocation) { + // if all set to NULL, delete + if(null === $data['latitude'] && null === $data['longitude'] && null === $data['zoom_level']) { + $account->locations()->delete(); + } + + // otherwise, update or create. + if(!(null === $data['latitude'] && null === $data['longitude'] && null === $data['zoom_level'])) { + $location = $this->accountRepository->getLocation($account); + if (null === $location) { + $location = new Location; + $location->locatable()->associate($account); + } + + $location->latitude = $data['latitude'] ?? config('firefly.default_location.latitude'); + $location->longitude = $data['longitude'] ?? config('firefly.default_location.longitude'); + $location->zoom_level = $data['zoom_level'] ?? config('firefly.default_location.zoom_level'); + $location->save(); } - $location->latitude = $data['latitude'] ?? config('firefly.default_location.latitude'); - $location->longitude = $data['longitude'] ?? config('firefly.default_location.longitude'); - $location->zoom_level = $data['zoom_level'] ?? config('firefly.default_location.zoom_level'); - $location->save(); } // has valid initial balance (IB) data? diff --git a/app/Validation/FireflyValidator.php b/app/Validation/FireflyValidator.php index f8f0792e06..60492ad5f6 100644 --- a/app/Validation/FireflyValidator.php +++ b/app/Validation/FireflyValidator.php @@ -376,20 +376,23 @@ class FireflyValidator extends Validator public function validateUniqueAccountForUser($attribute, $value, $parameters): bool { // because a user does not have to be logged in (tests and what-not). - if (!auth()->check()) { return $this->validateAccountAnonymously(); } if (isset($this->data['what'])) { + return $this->validateByAccountTypeString($value, $parameters, $this->data['what']); } if (isset($this->data['type'])) { return $this->validateByAccountTypeString($value, $parameters, $this->data['type']); } - if (isset($this->data['account_type_id'])) { return $this->validateByAccountTypeId($value, $parameters); } + $parameterId = $parameters[0] ?? null; + if (null !== $parameterId) { + return $this->validateByParameterId((int)$parameterId, $value); + } if (isset($this->data['id'])) { return $this->validateByAccountId($value); } @@ -545,16 +548,33 @@ class FireflyValidator extends Validator $ignore = $existingAccount->id; /** @var Collection $set */ - $set = auth()->user()->accounts()->where('account_type_id', $type->id)->where('id', '!=', $ignore)->get(); - // TODO no longer need to loop like this - /** @var Account $entry */ - foreach ($set as $entry) { - if ($entry->name === $value) { - return false; - } - } + $entry = auth()->user()->accounts()->where('account_type_id', $type->id)->where('id', '!=', $ignore) + ->where('name', $value) + ->first(); - return true; + return null === $entry; + } + + + /** + * @param $value + * + * @return bool + */ + private function validateByParameterId(int $accountId, $value): bool + { + /** @var Account $existingAccount */ + $existingAccount = Account::find($accountId); + + $type = $existingAccount->accountType; + $ignore = $existingAccount->id; + + /** @var Collection $set */ + $entry = auth()->user()->accounts()->where('account_type_id', $type->id)->where('id', '!=', $ignore) + ->where('name', $value) + ->first(); + + return null === $entry; } /**