Skip to content
This repository was archived by the owner on Oct 23, 2024. It is now read-only.

Money validator #1

Closed
wants to merge 4 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions .github/workflows/test.yml
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
30 changes: 30 additions & 0 deletions code/app/DTO/Request.php
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();
}
}
30 changes: 30 additions & 0 deletions code/app/DTO/Transaction.php
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();
}
}
36 changes: 36 additions & 0 deletions code/app/Http/Controllers/TransactionController.php
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);
}
}
}
31 changes: 31 additions & 0 deletions code/app/Providers/AppServiceProvider.php
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
{
//
}
}
40 changes: 40 additions & 0 deletions code/app/Services/RequestMoneyValidator.php
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.');

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

почему валидация здесь, а не в конструкторе?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

только из соображений того, что я ловлю эксепшн в контроллере чтобы избежать 500 и выдать контролируемый эксепшн. Было бы и правда логичнее делать проверку в конструкторе, но тогда будет 500 при инициализации объекта

}

if ($request->getCurrency() !== $transaction->getCurrency()) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: есть Money::assertSameCurrency()

return false;
}

$deviationPercentage = (string) ($this->deviation / 100);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

зачем нужен каст в строку - (string) ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

хороший вопрос :) тут каст не нужен

$deviationAmount = $transaction->getAmount()->multiply($deviationPercentage);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

почему решил вычислять отклонение от суммы Транзакции, а не Реквеста? Имеет ли смысл вычислить отклонение в обе стороны - от Транзакции и от Реквеста?

Copy link
Owner Author

Choose a reason for hiding this comment

The 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);
}
}
2 changes: 1 addition & 1 deletion code/config/app.php
Original file line number Diff line number Diff line change
Expand Up @@ -123,5 +123,5 @@
'store' => env('APP_MAINTENANCE_STORE', 'database'),
],

'transaction_deviation' => env('TRANSACTION_DEVIATION', 5),
'transaction_deviation' => (int) env('TRANSACTION_DEVIATION', 5),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

разве не может быть отклонение дробным числом - почему оно должно быть целым int?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

конечно может, использовал инт для упрощения решения, т.к. это тестовая задача

];
10 changes: 10 additions & 0 deletions code/routes/web.php
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']);
19 changes: 0 additions & 19 deletions code/tests/Feature/ExampleTest.php

This file was deleted.

31 changes: 31 additions & 0 deletions code/tests/Feature/TransactionControllerTest.php
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.'
]);
}
}
74 changes: 74 additions & 0 deletions code/tests/Unit/RequestMoneyValidatorTest.php
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

Choose a reason for hiding this comment

The 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. Код сниффер не пропустит такой нейминг

Copy link
Owner Author

Choose a reason for hiding this comment

The 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

Choose a reason for hiding this comment

The 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/

Copy link
Owner Author

Choose a reason for hiding this comment

The 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);
}
}
Loading