diff --git a/app/Api/V1/Requests/Models/Bill/UpdateRequest.php b/app/Api/V1/Requests/Models/Bill/UpdateRequest.php index 47e00d4934..3f01053bba 100644 --- a/app/Api/V1/Requests/Models/Bill/UpdateRequest.php +++ b/app/Api/V1/Requests/Models/Bill/UpdateRequest.php @@ -46,24 +46,23 @@ class UpdateRequest extends FormRequest */ public function getAll(): array { - $active = true; - if (null !== $this->get('active')) { - $active = $this->boolean('active'); - } - - return [ - 'name' => $this->string('name'), - 'amount_min' => $this->string('amount_min'), - 'amount_max' => $this->string('amount_max'), - 'currency_id' => $this->integer('currency_id'), - 'currency_code' => $this->string('currency_code'), - 'date' => $this->date('date'), - 'repeat_freq' => $this->string('repeat_freq'), - 'skip' => $this->integer('skip'), - 'active' => $active, - 'order' => $this->integer('order'), - 'notes' => $this->nlString('notes'), + $fields = [ + 'name' => ['name', 'string'], + 'amount_min' => ['amount_min', 'string'], + 'amount_max' => ['amount_max', 'string'], + 'currency_id' => ['currency_id', 'integer'], + 'currency_code' => ['currency_code', 'string'], + 'date' => ['date', 'date'], + 'repeat_freq' => ['repeat_freq', 'string'], + 'skip' => ['skip', 'integer'], + 'active' => ['active', 'boolean'], + 'order' => ['order', 'integer'], + 'notes' => ['notes', 'nlString'], + 'object_group_id' => ['object_group_id', 'integer'], + 'object_group_title' => ['object_group_title', 'string'], ]; + + return $this->getAllData($fields); } /** @@ -73,7 +72,8 @@ class UpdateRequest extends FormRequest */ public function rules(): array { - $bill = $this->route()->parameter('bill'); + $bill = $this->route()->parameter('bill'); + return [ 'name' => sprintf('between:1,255|uniqueObjectForUser:bills,name,%d', $bill->id), 'amount_min' => 'numeric|gt:0', @@ -100,10 +100,12 @@ class UpdateRequest extends FormRequest $validator->after( static function (Validator $validator) { $data = $validator->getData(); - $min = (float) ($data['amount_min'] ?? 0); - $max = (float) ($data['amount_max'] ?? 0); - if ($min > $max) { - $validator->errors()->add('amount_min', (string) trans('validation.amount_min_over_max')); + if (array_key_exists('amount_min', $data) && array_key_exists('amount_max', $data)) { + $min = (float)($data['amount_min'] ?? 0); + $max = (float)($data['amount_max'] ?? 0); + if ($min > $max) { + $validator->errors()->add('amount_min', (string)trans('validation.amount_min_over_max')); + } } } ); diff --git a/app/Services/Internal/Update/BillUpdateService.php b/app/Services/Internal/Update/BillUpdateService.php index 4db3e78c28..ffbbbbd855 100644 --- a/app/Services/Internal/Update/BillUpdateService.php +++ b/app/Services/Internal/Update/BillUpdateService.php @@ -23,12 +23,10 @@ declare(strict_types=1); namespace FireflyIII\Services\Internal\Update; -use DB; use FireflyIII\Factory\TransactionCurrencyFactory; use FireflyIII\Models\Bill; use FireflyIII\Models\Rule; use FireflyIII\Models\RuleTrigger; -use FireflyIII\Models\TransactionCurrency; use FireflyIII\Repositories\Bill\BillRepositoryInterface; use FireflyIII\Repositories\ObjectGroup\CreatesObjectGroups; use FireflyIII\Services\Internal\Support\BillServiceTrait; @@ -55,25 +53,21 @@ class BillUpdateService public function update(Bill $bill, array $data): Bill { $this->user = $bill->user; - /** @var TransactionCurrencyFactory $factory */ - $factory = app(TransactionCurrencyFactory::class); - /** @var TransactionCurrency $currency */ - $currency = $factory->find($data['currency_id'] ?? null, $data['currency_code'] ?? null); - if (null === $currency) { - // use default currency: - $currency = app('amount')->getDefaultCurrencyByUser($bill->user); + if (array_key_exists('currency_id', $data) || array_key_exists('currency_code', $data)) { + $factory = app(TransactionCurrencyFactory::class); + $currency = $factory->find($data['currency_id'] ?? null, $data['currency_code'] ?? null) ?? app('amount')->getDefaultCurrencyByUser($bill->user); + + // enable the currency if it isn't. + $currency->enabled = true; + $currency->save(); + $bill->transaction_currency_id = $currency->id; + $bill->save(); } - - // enable the currency if it isn't. - $currency->enabled = true; - $currency->save(); - - // new values - $data['transaction_currency_name'] = $currency->name; - $bill = $this->updateBillProperties($bill, $data); - $bill->transaction_currency_id = $currency->id; + // update bill properties: + $bill = $this->updateBillProperties($bill, $data); $bill->save(); + $bill->refresh(); // old values $oldData = [ 'name' => $bill->name, @@ -84,53 +78,60 @@ class BillUpdateService // update note: - if (isset($data['notes'])) { + if (array_key_exists('notes', $data)) { $this->updateNote($bill, (string)$data['notes']); } // update order. - // update the order of the piggy bank: - $oldOrder = (int)$bill->order; - $newOrder = (int)($data['order'] ?? $oldOrder); - if ($oldOrder !== $newOrder) { - $this->updateOrder($bill, $oldOrder, $newOrder); + if (array_key_exists('order', $data)) { + // update the order of the piggy bank: + $oldOrder = (int)$bill->order; + $newOrder = (int)($data['order'] ?? $oldOrder); + if ($oldOrder !== $newOrder) { + $this->updateOrder($bill, $oldOrder, $newOrder); + } } // update rule actions. - $this->updateBillActions($bill, $oldData['name'], $data['name']); - $this->updateBillTriggers($bill, $oldData, $data); + if (array_key_exists('name', $data)) { + $this->updateBillActions($bill, $oldData['name'], $data['name']); + $this->updateBillTriggers($bill, $oldData, $data); + } // update using name: - $objectGroupTitle = $data['object_group'] ?? ''; - if ('' !== $objectGroupTitle) { - $objectGroup = $this->findOrCreateObjectGroup($objectGroupTitle); - if (null !== $objectGroup) { - $bill->objectGroups()->sync([$objectGroup->id]); + if (array_key_exists('object_group_title', $data)) { + $objectGroupTitle = $data['object_group_title'] ?? ''; + if ('' !== $objectGroupTitle) { + $objectGroup = $this->findOrCreateObjectGroup($objectGroupTitle); + if (null !== $objectGroup) { + $bill->objectGroups()->sync([$objectGroup->id]); + $bill->save(); + } + + return $bill; + } + // remove if name is empty. Should be overruled by ID. + if ('' === $objectGroupTitle) { + $bill->objectGroups()->sync([]); $bill->save(); } - - return $bill; - } - // remove if name is empty. Should be overruled by ID. - if ('' === $objectGroupTitle) { - $bill->objectGroups()->sync([]); - $bill->save(); } + if (array_key_exists('object_group_id', $data)) { + // try also with ID: + $objectGroupId = (int)($data['object_group_id'] ?? 0); + if (0 !== $objectGroupId) { + $objectGroup = $this->findObjectGroupById($objectGroupId); + if (null !== $objectGroup) { + $bill->objectGroups()->sync([$objectGroup->id]); + $bill->save(); + } - // try also with ID: - $objectGroupId = (int)($data['object_group_id'] ?? 0); - if (0 !== $objectGroupId) { - $objectGroup = $this->findObjectGroupById($objectGroupId); - if (null !== $objectGroup) { - $bill->objectGroups()->sync([$objectGroup->id]); + return $bill; + } + if (0 === $objectGroupId) { + $bill->objectGroups()->sync([]); $bill->save(); } - - return $bill; - } - if (0 === $objectGroupId) { - $bill->objectGroups()->sync([]); - $bill->save(); } return $bill; @@ -144,7 +145,6 @@ class BillUpdateService */ private function updateBillProperties(Bill $bill, array $data): Bill { - if (isset($data['name']) && '' !== (string)$data['name']) { $bill->name = $data['name']; } @@ -161,10 +161,10 @@ class BillUpdateService if (isset($data['repeat_freq']) && '' !== (string)$data['repeat_freq']) { $bill->repeat_freq = $data['repeat_freq']; } - if (isset($data['skip']) && '' !== (string)$data['skip']) { + if (array_key_exists('skip', $data)) { $bill->skip = $data['skip']; } - if (isset($data['active']) && is_bool($data['active'])) { + if (array_key_exists('active', $data)) { $bill->active = $data['active']; } @@ -185,14 +185,14 @@ class BillUpdateService if ($newOrder > $oldOrder) { $this->user->bills()->where('order', '<=', $newOrder)->where('order', '>', $oldOrder) ->where('bills.id', '!=', $bill->id) - ->decrement('bills.order',1); + ->decrement('bills.order', 1); $bill->order = $newOrder; $bill->save(); } if ($newOrder < $oldOrder) { $this->user->bills()->where('order', '>=', $newOrder)->where('order', '<', $oldOrder) ->where('bills.id', '!=', $bill->id) - ->increment('bills.order',1); + ->increment('bills.order', 1); $bill->order = $newOrder; $bill->save(); } diff --git a/tests/Api/Models/Account/StoreControllerTest.php b/tests/Api/Models/Account/StoreControllerTest.php index 4490fb285b..12bf912a59 100644 --- a/tests/Api/Models/Account/StoreControllerTest.php +++ b/tests/Api/Models/Account/StoreControllerTest.php @@ -50,9 +50,9 @@ class StoreControllerTest extends TestCase /** * @param array $submission * - * @dataProvider storeDataProvider + * @ data Provider storeDataProvider * - * @ data Provider emptyDataProvider + * @dataProvider emptyDataProvider */ public function testStore(array $submission): void { diff --git a/tests/Api/Models/AvailableBudget/StoreControllerTest.php b/tests/Api/Models/AvailableBudget/StoreControllerTest.php index 66c4032bc1..188bdc5082 100644 --- a/tests/Api/Models/AvailableBudget/StoreControllerTest.php +++ b/tests/Api/Models/AvailableBudget/StoreControllerTest.php @@ -51,8 +51,8 @@ class StoreControllerTest extends TestCase /** * @param array $submission * - * @dataProvider storeDataProvider - * @ data Provider emptyDataProvider + * @ data Provider storeDataProvider + * @dataProvider emptyDataProvider */ public function testStore(array $submission): void { diff --git a/tests/Api/Models/Bill/StoreControllerTest.php b/tests/Api/Models/Bill/StoreControllerTest.php index 92da32a0d3..195d1d8282 100644 --- a/tests/Api/Models/Bill/StoreControllerTest.php +++ b/tests/Api/Models/Bill/StoreControllerTest.php @@ -51,8 +51,8 @@ class StoreControllerTest extends TestCase /** * @param array $submission * - * @dataProvider storeDataProvider - * @ data Provider emptyDataProvider + * @ data Provider storeDataProvider + * @dataProvider emptyDataProvider */ public function testStore(array $submission): void { diff --git a/tests/Api/Models/Bill/UpdateControllerTest.php b/tests/Api/Models/Bill/UpdateControllerTest.php new file mode 100644 index 0000000000..814d1b9226 --- /dev/null +++ b/tests/Api/Models/Bill/UpdateControllerTest.php @@ -0,0 +1,192 @@ +. + */ + +namespace Tests\Api\Models\Bill; + + +use Faker\Factory; +use Laravel\Passport\Passport; +use Log; +use Tests\TestCase; +use Tests\Traits\CollectsValues; +use Tests\Traits\RandomValues; +use Tests\Traits\TestHelpers; + +/** + * Class UpdateControllerTest + */ +class UpdateControllerTest extends TestCase +{ + use RandomValues, TestHelpers, CollectsValues; + + /** + * + */ + public function setUp(): void + { + parent::setUp(); + Passport::actingAs($this->user()); + Log::info(sprintf('Now in %s.', get_class($this))); + } + + + /** + * @dataProvider updateDataProvider + */ + public function testUpdate(array $submission): void + { + $ignore = [ + 'created_at', + 'updated_at', + ]; + $route = route('api.v1.bills.update', [$submission['id']]); + + $this->updateAndCompare($route, $submission, $ignore); + } + + + /** + * @return array + */ + public function updateDataProvider(): array + { + $submissions = []; + $all = $this->updateDataSet(); + foreach ($all as $name => $data) { + $submissions[] = [$data]; + } + + return $submissions; + } + + + /** + * @return array + */ + public function updateDataSet(): array + { + $faker = Factory::create(); + $currencies = [ + 1 => 'EUR', + 2 => 'HUF', + 3 => 'GBP', + 4 => 'UAH', + ]; + $repeatFreqs = ['yearly', 'weekly', 'monthly']; + $repeatFreq = $repeatFreqs[rand(0, count($repeatFreqs) - 1)]; + $objectGroupId = $faker->numberBetween(1, 2); + $objectGroupName = sprintf('Object group %d', $objectGroupId); + $rand = rand(1, 4); + $set = [ + 'name' => [ + 'id' => 1, + 'fields' => [ + 'name' => ['test_value' => join(' ', $faker->words(4))], + ], + 'extra_ignore' => [], + ], + 'amount_min' => [ + 'id' => 1, + 'fields' => [ + 'amount_min' => ['test_value' => number_format($faker->randomFloat(2, 10, 50), 2)], + ], + 'extra_ignore' => [], + ], + 'amount_max' => [ + 'id' => 1, + 'fields' => [ + 'amount_max' => ['test_value' => number_format($faker->randomFloat(2, 60, 90), 2)], + ], + 'extra_ignore' => [], + ], + 'date' => [ + 'id' => 1, + 'fields' => [ + 'date' => ['test_value' => $faker->dateTimeBetween('-1 year', 'now')->format('Y-m-d')], + ], + 'extra_ignore' => [], + ], + + 'repeat_freq' => [ + 'id' => 1, + 'fields' => [ + 'repeat_freq' => ['test_value' => $repeatFreq], + ], + 'extra_ignore' => [], + ], + 'skip' => [ + 'id' => 1, + 'fields' => [ + 'skip' => ['test_value' => $faker->numberBetween(1, 10)], + ], + 'extra_ignore' => [], + ], + + 'active' => [ + 'id' => 1, + 'fields' => [ + 'active' => ['test_value' => $faker->boolean], + ], + 'extra_ignore' => [], + ], + 'notes' => [ + 'id' => 1, + 'fields' => [ + 'notes' => ['test_value' => join(' ', $faker->words(5))], + ], + 'extra_ignore' => [], + ], + 'object_group_id' => [ + 'id' => 1, + 'fields' => [ + 'object_group_id' => ['test_value' => (string)$objectGroupId], + ], + 'extra_ignore' => ['object_group_order', 'object_group_title'], + ], + 'object_group_title' => [ + 'id' => 1, + 'fields' => [ + 'object_group_title' => ['test_value' => $objectGroupName], + ], + 'extra_ignore' => ['object_group_order', 'object_group_id'], + ], + 'currency_id' => [ + 'id' => 1, + 'fields' => [ + 'currency_id' => ['test_value' => (string)$rand], + ], + 'extra_ignore' => ['currency_code', 'currency_symbol'], + ], + 'currency_code' => [ + 'id' => 1, + 'fields' => [ + 'currency_code' => ['test_value' => $currencies[$rand]], + ], + 'extra_ignore' => ['currency_id', 'currency_symbol'], + ], + + ]; + + return $set; + } + + +} \ No newline at end of file