-
Notifications
You must be signed in to change notification settings - Fork 0
Money validator #1
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,42 @@ | ||
name: Run Tests | ||
|
||
on: | ||
push: | ||
branches: | ||
- main | ||
pull_request: | ||
branches: | ||
- main | ||
jobs: | ||
test: | ||
runs-on: ubuntu-latest | ||
|
||
steps: | ||
- name: Checkout code | ||
uses: actions/checkout@v3 | ||
|
||
- name: Set up PHP | ||
uses: shivammathur/setup-php@v2 | ||
with: | ||
php-version: 8.2 | ||
extensions: mysqli | ||
|
||
- name: Install Composer dependencies | ||
working-directory: ./code | ||
run: composer install --prefer-dist --no-progress | ||
|
||
- name: Copy .env.example to .env | ||
working-directory: ./code | ||
run: cp .env.example .env | ||
|
||
- name: Generate app key | ||
working-directory: ./code | ||
run: php artisan key:generate | ||
|
||
- name: Run database migrations | ||
working-directory: ./code | ||
run: php artisan migrate | ||
|
||
- name: Run tests | ||
working-directory: ./code | ||
run: vendor/bin/phpunit |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\DTO; | ||
|
||
use Money\Currency; | ||
use Money\Money; | ||
|
||
final readonly class Request | ||
{ | ||
private function __construct(private Money $amount) | ||
{ | ||
} | ||
|
||
public static function create(int $amountInCents, string $currency): self | ||
{ | ||
return new self(new Money($amountInCents, new Currency(strtoupper($currency)))); | ||
} | ||
|
||
public function getAmount(): Money | ||
{ | ||
return $this->amount; | ||
} | ||
|
||
public function getCurrency(): string | ||
{ | ||
return $this->amount->getCurrency()->getCode(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\DTO; | ||
|
||
use Money\Currency; | ||
use Money\Money; | ||
|
||
final readonly class Transaction | ||
{ | ||
private function __construct(private Money $amount) | ||
{ | ||
} | ||
|
||
public static function create(int $amountInCents, string $currency): self | ||
{ | ||
return new self(new Money($amountInCents, new Currency(strtoupper($currency)))); | ||
} | ||
|
||
public function getAmount(): Money | ||
{ | ||
return $this->amount; | ||
} | ||
|
||
public function getCurrency(): string | ||
{ | ||
return $this->amount->getCurrency()->getCode(); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,36 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Http\Controllers; | ||
|
||
use App\DTO\Request; | ||
use App\DTO\Transaction; | ||
use App\Services\RequestMoneyValidator; | ||
use Illuminate\Http\JsonResponse; | ||
use Illuminate\Routing\Controller; | ||
use InvalidArgumentException; | ||
|
||
final class TransactionController extends Controller | ||
{ | ||
public function __construct(private readonly RequestMoneyValidator $validator) | ||
{ | ||
} | ||
|
||
public function validateTransaction(): JsonResponse | ||
{ | ||
try { | ||
$userRequest = Request::create(10000, 'USD'); | ||
$transaction = Transaction::create(9000, 'USD'); | ||
|
||
$isValid = $this->validator->validate($userRequest, $transaction); | ||
|
||
return response()->json(['isValid' => $isValid]); | ||
} catch (InvalidArgumentException $e) { | ||
return response()->json([ | ||
'error' => 'Validation failed', | ||
'message' => $e->getMessage() | ||
], 400); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Providers; | ||
|
||
use App\Services\RequestMoneyValidator; | ||
use Illuminate\Support\ServiceProvider; | ||
|
||
class AppServiceProvider extends ServiceProvider | ||
{ | ||
/** | ||
* Register any application services. | ||
*/ | ||
public function register(): void | ||
{ | ||
$this->app->singleton(RequestMoneyValidator::class, function ($app) { | ||
return new RequestMoneyValidator( | ||
deviation: config('app.transaction_deviation') | ||
); | ||
}); | ||
} | ||
|
||
/** | ||
* Bootstrap any application services. | ||
*/ | ||
public function boot(): void | ||
{ | ||
// | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,40 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace App\Services; | ||
|
||
use App\DTO\Request; | ||
use App\DTO\Transaction; | ||
use InvalidArgumentException; | ||
|
||
final readonly class RequestMoneyValidator | ||
{ | ||
public function __construct(private int $deviation) | ||
{ | ||
} | ||
|
||
public function validate(Request $request, Transaction $transaction): bool | ||
{ | ||
if (!filter_var( | ||
$this->deviation, | ||
FILTER_VALIDATE_INT, | ||
['options' => ['min_range' => 0, 'max_range' => 100]] | ||
)) { | ||
throw new InvalidArgumentException('Deviation must be between 0 and 100.'); | ||
} | ||
|
||
if ($request->getCurrency() !== $transaction->getCurrency()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. minor: есть Money::assertSameCurrency() |
||
return false; | ||
} | ||
|
||
$deviationPercentage = (string) ($this->deviation / 100); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. зачем нужен каст в строку - (string) ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. хороший вопрос :) тут каст не нужен |
||
$deviationAmount = $transaction->getAmount()->multiply($deviationPercentage); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. почему решил вычислять отклонение от суммы Транзакции, а не Реквеста? Имеет ли смысл вычислить отклонение в обе стороны - от Транзакции и от Реквеста? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Сумма транзакции это, по сути, "истина", а запрос должен быть проверен, чтобы отклонения были в допустимых пределах относительно этой "истины". На счет отклонений в обе стороны - действительно это имеет смысл, если на стороне запроса была ошибка. Т.к. не было конкретных условий вычисления отклонений с обеих сторон, то оставил так |
||
|
||
$minAmount = $transaction->getAmount()->subtract($deviationAmount); | ||
$maxAmount = $transaction->getAmount()->add($deviationAmount); | ||
|
||
return $request->getAmount()->greaterThanOrEqual($minAmount) | ||
&& $request->getAmount()->lessThanOrEqual($maxAmount); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -123,5 +123,5 @@ | |
'store' => env('APP_MAINTENANCE_STORE', 'database'), | ||
], | ||
|
||
'transaction_deviation' => env('TRANSACTION_DEVIATION', 5), | ||
'transaction_deviation' => (int) env('TRANSACTION_DEVIATION', 5), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. разве не может быть отклонение дробным числом - почему оно должно быть целым int? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. конечно может, использовал инт для упрощения решения, т.к. это тестовая задача |
||
]; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,10 @@ | ||
<?php | ||
|
||
use App\Http\Controllers\TransactionController; | ||
use Illuminate\Support\Facades\Route; | ||
|
||
Route::get('/', function () { | ||
return view('welcome'); | ||
}); | ||
|
||
Route::get('/validate-transaction', [TransactionController::class, 'validateTransaction']); |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Tests\Feature; | ||
|
||
use Tests\TestCase; | ||
|
||
class TransactionControllerTest extends TestCase | ||
{ | ||
public function test_validate_transaction_success(): void | ||
{ | ||
$response = $this->getJson('/validate-transaction'); | ||
|
||
$response->assertStatus(200) | ||
->assertJson(['isValid' => false]); | ||
} | ||
|
||
public function test_validate_transaction_invalid_deviation(): void | ||
{ | ||
config(['app.transaction_deviation' => 150]); | ||
|
||
$response = $this->getJson('/validate-transaction'); | ||
|
||
$response->assertStatus(400) | ||
->assertJson([ | ||
'error' => 'Validation failed', | ||
'message' => 'Deviation must be between 0 and 100.' | ||
]); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,74 @@ | ||
<?php | ||
|
||
declare(strict_types=1); | ||
|
||
namespace Tests\Unit; | ||
|
||
use App\DTO\Request; | ||
use App\DTO\Transaction; | ||
use App\Services\RequestMoneyValidator; | ||
use InvalidArgumentException; | ||
use Tests\TestCase; | ||
|
||
class RequestMoneyValidatorTest extends TestCase | ||
{ | ||
public function test_validate_currency_mismatch(): void | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. семантика, но по PSR-1 методы должны называться в camelCase формате - https://www.php-fig.org/psr/psr-1/[#1](#1)-overview. Код сниффер не пропустит такой нейминг There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. вот тут я не знаю честно говоря почему так написал))тут прям самому смешно, может потому что делал на Ларавел. - нет ответа у меня. На Симфони всегда пишу по ПСР через camelCase |
||
{ | ||
$request = Request::create(10000, 'USD'); | ||
$transaction = Transaction::create(10000, 'EUR'); | ||
$validator = new RequestMoneyValidator(10); | ||
|
||
$this->assertFalse($validator->validate($request, $transaction)); | ||
} | ||
|
||
public function test_validate_within_deviation(): void | ||
{ | ||
$request = Request::create(9500, 'USD'); | ||
$transaction = Transaction::create(9000, 'USD'); | ||
|
||
$validator = new RequestMoneyValidator(10); | ||
|
||
$this->assertTrue($validator->validate($request, $transaction)); | ||
} | ||
|
||
public function test_validate_outside_deviation(): void | ||
{ | ||
$request = Request::create(10000, 'USD'); | ||
$transaction = Transaction::create(9000, 'USD'); | ||
|
||
$validator = new RequestMoneyValidator(5); | ||
|
||
$this->assertFalse($validator->validate($request, $transaction)); | ||
} | ||
|
||
public function test_validate_exact_amount(): void | ||
{ | ||
$request = Request::create(10000, 'USD'); | ||
$transaction = Transaction::create(10000, 'USD'); | ||
$validator = new RequestMoneyValidator(10); | ||
|
||
$this->assertTrue($validator->validate($request, $transaction)); | ||
} | ||
|
||
public function test_validate_small_deviation(): void | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. в тестах рассмотрены кейзы для девиейшн 1%, 5% и 10%. А что если будет 0, null, 99, 100, ... ?? А что если в Транзакции или Реквесте будут отрицательные или пустые значения? Мало, мало тестов... Еще мне кажется здесь очень бы подошел дата провадер: https://blog.martinhujer.cz/how-to-use-data-providers-in-phpunit/ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. да, согласен, поскольку тестов и правда не много, то решил не использовать дата провайдер, хотя мысли об этом были |
||
{ | ||
$request = Request::create(9950, 'USD'); | ||
$transaction = Transaction::create(10000, 'USD'); | ||
|
||
$validator = new RequestMoneyValidator(1); | ||
|
||
$this->assertTrue($validator->validate($request, $transaction)); | ||
} | ||
|
||
public function test_negative_deviation_throws_exception(): void | ||
{ | ||
$this->expectException(InvalidArgumentException::class); | ||
$this->expectExceptionMessage('Deviation must be between 0 and 100.'); | ||
|
||
$request = Request::create(10000, 'USD'); | ||
$transaction = Transaction::create(9000, 'USD'); | ||
$validator = new RequestMoneyValidator(-5); | ||
|
||
$validator->validate($request, $transaction); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
почему валидация здесь, а не в конструкторе?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
только из соображений того, что я ловлю эксепшн в контроллере чтобы избежать 500 и выдать контролируемый эксепшн. Было бы и правда логичнее делать проверку в конструкторе, но тогда будет 500 при инициализации объекта