-
Notifications
You must be signed in to change notification settings - Fork 1
enh: Dynamically implement config to allow transfer from negative balance #195
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Changes from all commits
77c8542
8d27f1a
374ca28
2de3104
25cb0aa
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 | ||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -274,6 +274,14 @@ public function store(Request $request): JsonResponse | |||||||||||||||||||||||||||||||||
| $data = $validationResult['data']; | ||||||||||||||||||||||||||||||||||
| $user = $request->user(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| $wallet = \App\Models\Wallet::find($data['wallet_id']); | ||||||||||||||||||||||||||||||||||
| $allowNegative = $user->getConfigValue('allow-negative-balance', false); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| // Only check for expense types; income is always allowed | ||||||||||||||||||||||||||||||||||
| if ($data['type'] === 'expense' && $wallet->balance < $data['amount'] && !$allowNegative) { | ||||||||||||||||||||||||||||||||||
| return $this->failure(__('Insufficient balance in wallet'), 422); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if (! empty($data['client_id'])) { | ||||||||||||||||||||||||||||||||||
| $existingTransaction = Transaction::findByClientId($data['client_id'], $user); | ||||||||||||||||||||||||||||||||||
| if ($existingTransaction) { | ||||||||||||||||||||||||||||||||||
|
|
@@ -370,6 +378,8 @@ public function store(Request $request): JsonResponse | |||||||||||||||||||||||||||||||||
| )->delay($recurring_transaction->next_scheduled_at); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return $transaction; | ||||||||||||||||||||||||||||||||||
| }); | ||||||||||||||||||||||||||||||||||
| } catch (HttpException $e) { | ||||||||||||||||||||||||||||||||||
|
|
@@ -382,7 +392,9 @@ public function store(Request $request): JsonResponse | |||||||||||||||||||||||||||||||||
| return $this->failure(__('Failed to create transaction'), 500, [$e->getMessage()]); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return $this->success($transaction, statusCode: 201); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return $this->transferWithNegativeBalance($transaction); | ||||||||||||||||||||||||||||||||||
|
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. Clean up commented-out code before merging. The previous success response is now redundant as it is handled by the new helper method.
Suggested change
|
||||||||||||||||||||||||||||||||||
| // return $this->success($transaction, statusCode: 201); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| #[OA\Delete( | ||||||||||||||||||||||||||||||||||
|
|
@@ -824,4 +836,20 @@ private function validateResourceOwnership(array $data, array $categories = []): | |||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||||
| * Helper to format the success response and inject negative balance warnings. | ||||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||||
| private function transferWithNegativeBalance(Transaction $transaction): JsonResponse | ||||||||||||||||||||||||||||||||||
|
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. The method name
Suggested change
|
||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||
| $transaction->load('wallet'); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| $responseData = $transaction->toArray(); | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| if ($transaction->wallet->balance < 0) { | ||||||||||||||||||||||||||||||||||
| $responseData['warning'] = __('Transaction recorded, but your wallet is now in a negative balance.'); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||
| return $this->success($responseData, statusCode: 201); | ||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
|
Comment on lines
+839
to
+854
Contributor
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.
Suggested change
Not needed. We can't create a new function for every config or flolw |
||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -65,6 +65,20 @@ class User extends Authenticatable | |||||
| 'password' => 'hashed', | ||||||
| ]; | ||||||
|
|
||||||
| /** | ||||||
| * The "booted" method of the model. | ||||||
| */ | ||||||
| protected static function booted() | ||||||
| { | ||||||
| static::created(function ($user) { | ||||||
|
|
||||||
| $user->setConfigValue( | ||||||
| 'allow-negative-balance', | ||||||
| false, | ||||||
| \Whilesmart\ModelConfiguration\Enums\ConfigValueType::Boolean | ||||||
|
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. Inconsistent use of fully qualified class name for the Enum. Since it is imported in other files, consider importing it here for readability.
Suggested change
|
||||||
| ); | ||||||
| }); | ||||||
| } | ||||||
|
Comment on lines
+68
to
+81
Contributor
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. Do we set other configs like this? If not use events! use the |
||||||
| public function categories(): HasMany | ||||||
| { | ||||||
| return $this->hasMany(Category::class); | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change | ||||||
|---|---|---|---|---|---|---|---|---|
|
|
@@ -26,7 +26,11 @@ public function transfer( | |||||||
| ?string $datetime = null, | ||||||||
| array $transactionClientIds = [] | ||||||||
| ) { | ||||||||
| if ($fromWallet->balance < $amountToSend) { | ||||||||
| //check the user allows negative balance | ||||||||
|
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. The variable
Suggested change
|
||||||||
| $allowNegativeBalance = $user->getConfigValue('allow-negative-balance'); | ||||||||
|
|
||||||||
| //only throw the exception if the balance is low and they havent enabled negative transfers | ||||||||
| if ($fromWallet->balance < $amountToSend && !$allowNegativeBalance) { | ||||||||
|
Comment on lines
+29
to
+33
Contributor
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. You should only read the config is balance < amount. Saves us a trip to the database when not needed. |
||||||||
| throw new InvalidArgumentException(__('Insufficient balance in source wallet')); | ||||||||
| } | ||||||||
|
|
||||||||
|
|
||||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
| @@ -0,0 +1,33 @@ | ||||
| <?php | ||||
|
|
||||
| use Illuminate\Database\Migrations\Migration; | ||||
| use Illuminate\Database\Schema\Blueprint; | ||||
| use Illuminate\Support\Facades\Schema; | ||||
|
|
||||
| return new class () extends Migration { | ||||
| /** | ||||
| * Run the migrations. | ||||
| */ | ||||
| public function up(): void | ||||
| { | ||||
| Schema::create('configurations', function (Blueprint $table) { | ||||
| $table->id(); | ||||
| $table->string('key'); | ||||
| $table->json('value'); | ||||
| $table->string('configurable_type'); | ||||
| $table->unsignedBigInteger('configurable_id'); | ||||
| $table->string('type')->default('string'); | ||||
| $table->timestamps(); | ||||
|
|
||||
| $table->unique(['configurable_id', 'configurable_type', 'key']); | ||||
| }); | ||||
| } | ||||
|
|
||||
| /** | ||||
| * Reverse the migrations. | ||||
| */ | ||||
| public function down(): void | ||||
| { | ||||
| Schema::dropIfExists('configurations'); | ||||
| } | ||||
| }; | ||||
|
Contributor
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.
Suggested change
The configurations table is supplied by the configurations package this is not needed. TIP: Configurations have been working before now on other things (how do you think they worked without this table?) |
||||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,25 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| <?php | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| namespace Database\Seeders; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| use App\Models\User; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| use Illuminate\Database\Seeder; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| use Whilesmart\ModelConfiguration\Enums\ConfigValueType; | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| class ConfigurationSeeder extends Seeder | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| public function run(): void | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| $users = User::all(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| foreach ($users as $user) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+13
to
+15
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. Using
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||
| // Ensure the user has "Allow Negative Balance" set to false by default | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| $user->setConfigValue( | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| 'allow-negative-balance', | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| false, | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ConfigValueType::Boolean | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| ); | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+1
to
+25
Contributor
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.
Suggested change
Again not needed. |
||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -14,6 +14,7 @@ public function run(): void | |||
| $this->call([ | ||||
| UserSeeder::class, | ||||
| TransactionSeeder::class, | ||||
| ConfigurationSeeder::class, | ||||
|
Contributor
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.
Suggested change
|
||||
| ]); | ||||
| } | ||||
| } | ||||
| Original file line number | Diff line number | Diff line change | ||
|---|---|---|---|---|
|
|
@@ -26,11 +26,13 @@ | |||
| <env name="CACHE_DRIVER" value="array"/> | ||||
| <env name="DB_DATABASE" value="trakli_test"/> | ||||
| <env name="DB_HOST" value="mysql-test"/> | ||||
|
|
||||
|
Contributor
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.
Suggested change
no whitespace changes |
||||
| <env name="MAIL_MAILER" value="array"/> | ||||
| <env name="PULSE_ENABLED" value="false"/> | ||||
| <env name="QUEUE_CONNECTION" value="database"/> | ||||
| <env name="SESSION_DRIVER" value="array"/> | ||||
| <env name="TELESCOPE_ENABLED" value="false"/> | ||||
| <env name="FIREBASE_CREDENTIALS" value=""/> | ||||
|
|
||||
|
Contributor
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. no whitespace changes.. do not modify files for no reason |
||||
| </php> | ||||
| </phpunit> | ||||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,69 @@ | ||||||
| <?php | ||||||
|
|
||||||
| namespace Tests\Feature; | ||||||
|
|
||||||
| use App\Models\User; | ||||||
| use App\Models\Wallet; | ||||||
| use Illuminate\Foundation\Testing\RefreshDatabase; | ||||||
| use Tests\TestCase; | ||||||
| use Whilesmart\ModelConfiguration\Enums\ConfigValueType; | ||||||
|
|
||||||
| class NegativeBalanceTest extends TestCase | ||||||
| { | ||||||
| use RefreshDatabase; | ||||||
|
|
||||||
| /** | ||||||
| * Test that a transaction fails if balance is insufficient and config is OFF. | ||||||
| */ | ||||||
| public function test_transaction_fails_when_insufficient_funds_and_config_disabled() | ||||||
| { | ||||||
| $user = User::factory()->create(); | ||||||
| $wallet = Wallet::factory()->create(['user_id' => $user->id, 'balance' => 10.00]); | ||||||
|
|
||||||
| // Explicitly set allow-negative-balance to false | ||||||
| $user->setConfigValue('allow-negative-balance', false, ConfigValueType::Boolean); | ||||||
|
|
||||||
| $payload = [ | ||||||
| 'amount' => 50.00, // More than the 10.00 balance | ||||||
| 'type' => 'expense', | ||||||
| 'wallet_id' => $wallet->id, | ||||||
| 'description' => 'Overdraft attempt', | ||||||
| ]; | ||||||
|
|
||||||
| $response = $this->actingAs($user, 'sanctum')->postJson('/api/v1/transactions', $payload); | ||||||
|
|
||||||
| // It should fail because the TransferService (or Controller) throws an error | ||||||
| $response->assertStatus(422); | ||||||
| $this->assertEquals(10.00, $wallet->fresh()->balance); | ||||||
| } | ||||||
|
|
||||||
| /** | ||||||
| * Test that a transaction succeeds if balance is insufficient but config is ON. | ||||||
| */ | ||||||
| public function test_transaction_succeeds_when_insufficient_funds_and_config_enabled() | ||||||
| { | ||||||
| $user = User::factory()->create(); | ||||||
| $wallet = Wallet::factory()->create(['user_id' => $user->id, 'balance' => 10.00]); | ||||||
|
|
||||||
| // Enable negative balance for this user | ||||||
| $user->setConfigValue('allow-negative-balance', true, ConfigValueType::Boolean); | ||||||
|
|
||||||
| $payload = [ | ||||||
| 'amount' => 50.00, | ||||||
| 'type' => 'expense', | ||||||
| 'wallet_id' => $wallet->id, | ||||||
| 'description' => 'Allowed overdraft', | ||||||
| ]; | ||||||
|
|
||||||
| $response = $this->actingAs($user, 'sanctum')->postJson('/api/v1/transactions', $payload); | ||||||
|
|
||||||
| // Assert success | ||||||
| $response->assertStatus(201); | ||||||
|
|
||||||
| // Assert the warning exists in the response JSON | ||||||
| // $response->assertJsonPath('warning', __('Your account is now in a negative balance.')); | ||||||
|
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. This assertion is commented out. It should be enabled to verify the warning message is actually delivered to the client.
Suggested change
|
||||||
|
|
||||||
| // Assert the database reflects a negative balance (-40) | ||||||
| $this->assertEquals(-40.00, (float)$wallet->fresh()->balance); | ||||||
| } | ||||||
| } | ||||||
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.
Retrieving the wallet without ensuring it belongs to the authenticated user is a security risk. A user could pass a
wallet_idthey don't own to bypass validation or probe for valid IDs.