feat: support for myself party type by iMercyvlogs · Pull Request #185 · trakli/webservice

iMercyvlogs · 2026-03-12T12:02:29Z

added support for party type myself.... Transactions "From" myself (Income) are handled as transfers if configured so. Target issue: #169

sourceant · 2026-03-12T12:03:04Z

Code Review Summary

This PR introduces support for 'myself' party types, allowing transactions involving the user themselves to be automatically converted into transfers if configured. This involves changes to the Transaction controller, Party model, and User model configuration.

🚀 Key Improvements

Encapsulated validation logic into validateRequestData helper.
Added a TransferService dependency to handle the conversion logic.
Included a comprehensive feature test MyselfTransferTest.php.

💡 Minor Suggestions

Remove excessive whitespace in TransactionController and ConfigurationSeeder.
Use $data instead of $request inside the helper methods for consistency.

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2026-03-12T12:03:05Z

-        private RecurringTransactionService $recurringTransactionService
+        private RecurringTransactionService $recurringTransactionService,
+        private TransferService $transferService
    ) {


Retrieving the Party model directly in the controller and performing business logic here violates the Single Responsibility Principle. This logic should be encapsulated within the TransactionService or a specific HandleMyselfPartyAction. Additionally, using \App\Models\Party::find ignores the user scope; it should be scoped to the authenticated user for security.

Suggested change

) {

$partyId = $data['party_id'] ?? null;

if (config('app.convert_myself_to_transfer') && $partyId) {

$party = $user->parties()->find($partyId);

if ($party?->is_myself && $request->has('from_wallet_id')) {

$transfer = $this->transferService->transfer(

amountToSend: (float) $data['amount'],

fromWallet: $user->wallets()->findOrFail($request['from_wallet_id']),

amountToReceive: (float) $data['amount'],

toWallet: $user->wallets()->findOrFail($data['wallet_id']),

user: $user,

exchangeRate: 1.0,

datetime: $data['datetime'] ?? null,

transactionClientIds: ['income_transaction_client_id' => $data['client_id'] ?? null]

);

return $this->success($transfer, statusCode: 201);

}

}

sourceant

Review complete. See the overview comment for a summary.

nfebe

Thank for this! I took a quick look and have one comment for now :)

sourceant

Review complete. See the overview comment for a summary.

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2026-03-13T05:55:02Z

+
+    }
+
+    private function isMyselfTransfer($data, $user, $request): bool


The method signature in the implementation includes $request, but the call site on line 263 only passes two arguments. This will cause a TypeError if line 862 is reached.

Suggested change

private function isMyselfTransfer($data, $user, $request): bool

private function isMyselfTransfer($data, $user): bool

sourceant

Review complete. See the overview comment for a summary.

kofimokome

Your tests are failing

nfebe

We use whilesmart/eloquent-model-configs read the docs on and understand how to better set the configs
Inline I recommend the names of configs to set. is-myself set on party and create-transfers-for-myself-transactions. Its a boolean and is true by default. Which means if a user doesn't turn this off setting applies.
Many of the changes in phpunit, testcase.php, seem unrelated.
composer lock should not change

nfebe · 2026-03-14T06:41:35Z

+            'files.*' => 'file|mimes:jpg,jpeg,png,pdf|max:1240',
+
+            'from_wallet_id' => 'required_if:convert_myself_to_transfer,true|integer|exists:wallets,id',
+        ]);


Suggested change

]);

Create a request instead and there should likely be a dedicated commit for that

sorry, I didn't quite understand this

check the Laravel documentation here => https://laravel.com/docs/13.x/validation#form-request-validation

You haven't updated the codes to use Laravel Requests

sourceant

Review complete. No specific code suggestions were generated. See the overview comment for a summary.

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2026-03-16T14:16:49Z

+        ]);
+    }
+
+    private function isMyselfTransfer($data, $user, $request): bool


Type hinting the parameters improves code clarity and helps static analysis tools catch errors.

Suggested change

private function isMyselfTransfer($data, $user, $request): bool

private function isMyselfTransfer(array $data, \App\Models\User $user, Request $request): bool

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2026-03-16T14:22:54Z

+        private RecurringTransactionService $recurringTransactionService,
+        private TransferService $transferService
    ) {
    }


This method performs a raw query on �App\Models\Configuration within the controller. This logic is repeated and should be moved to the User model or a ConfigurationService to improve maintainability and allow for caching.

Suggested change

}

private function isMyselfTransfer(array $data, User $user, Request $request): bool

{

$myselfPartyId = $user->getMyselfPartyId();

$isMyself = isset($data['party_id']) && (string)$data['party_id'] === (string)$myselfPartyId;

return config('app.convert_myself_to_transfer') &&

$isMyself &&

$request->has('from_wallet_id');

}

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2026-03-16T14:42:11Z

        $data = $validationResult['data'];
        $user = $request->user();

+        //check if party_id is the user's "myself" party


There is significant whitespace and some redundant logic flow here. The logic for 'myself' party conversion should ideally be handled inside the service layer or a dedicated action to keep the controller lean.

Suggested change

//check if party_id is the user's "myself" party

if ($this->isMyselfTransfer($data, $user, $request)) {

$transfer = $this->handleMyselfTransfer($data, $user, $request);

return $this->success($transfer, statusCode: 201);

}

sourceant

Review complete. See the overview comment for a summary.

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2026-04-13T02:46:52Z

+        private RecurringTransactionService $recurringTransactionService,
+        private TransferService $transferService
    ) {
    }


The isMyselfTransfer logic is still performing an extra database query via $user->parties()->find($partyId) for every transaction storage request. Since party_id is already validated to exist in the request, this could be optimized or cached. Additionally, the logic for determining if a party is 'myself' should reside on the Party model itself.

Suggested change

}

private function isMyselfTransfer(array $data, User $user, Request $request): bool

{

if (!$user->getConfigValue('create-transfers-for-myself-transactions') || !$request->has('from_wallet_id')) {

return false;

}

$partyId = $data['party_id'] ?? null;

if (!$partyId) {

return false;

}

$party = $user->parties()->find($partyId);

return $party && $party->getConfigValue('is-myself');

}

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2026-04-13T03:41:17Z

+            'files' => 'nullable|array',
+            'files.*' => 'file|mimes:jpg,jpeg,png,pdf|max:1240',
+
+            'from_wallet_id' => 'required_if:convert_myself_to_transfer,true|integer|exists:wallets,id',


The validation rule required_if:convert_myself_to_transfer,true uses a hardcoded string key that isn't part of the request payload. It should likely check the user's config or handle this requirement logically, as convert_myself_to_transfer isn't a field in the request.

Suggested change

'from_wallet_id' => 'required_if:convert_myself_to_transfer,true|integer|exists:wallets,id',

'from_wallet_id' => 'nullable|integer|exists:wallets,id',

…exity to 91" This reverts commit de2b185.

This reverts commit ae50a3b.

sourceant

Review complete. No specific code suggestions were generated. See the overview comment for a summary.

kofimokome · 2026-04-28T20:24:27Z

+            'files.*' => 'file|mimes:jpg,jpeg,png,pdf|max:1240',
+
+            'from_wallet_id' => 'required_if:convert_myself_to_transfer,true|integer|exists:wallets,id',
+        ]);


check the Laravel documentation here => https://laravel.com/docs/13.x/validation#form-request-validation

nfebe

No need to publish the configurations table and use FormRequest like Kofi mentioned.

sourceant

Review complete. See the overview comment for a summary.

sourceant · 2026-04-30T07:24:18Z

+
+        $isMyselfParty = $party && $party->getConfigValue('is-myself');
+
+        return $featureEnabled && $isMyselfParty && $request->has('from_wallet_id');


The check $request->has('from_wallet_id') is redundant because the validation rules (line 851) already enforce required_if:convert_myself_to_transfer,true. However, if convert_myself_to_transfer is false but the key exists, this logic might still trigger unexpectedly if the other conditions meet. It's safer to rely strictly on the validated data.

Suggested change

return $featureEnabled && $isMyselfParty && $request->has('from_wallet_id');

return $featureEnabled && $isMyselfParty && isset($data['from_wallet_id']);

sourceant · 2026-04-30T07:24:18Z

+    public function run(): void
+    {
+        //enable feature for all existing users by default, can be turned off by user if they want
+        foreach (\App\Models\User::all() as $user) {


Iterating through all users and executing a query for each in a seeder is inefficient for large datasets. Consider using a bulk insert or a more performant update strategy.

Suggested change

foreach (\App\Models\User::all() as $user) {

\App\Models\User::all()->each(function ($user) {

$user->setConfigValue('create-transfers-for-myself-transactions', true, \Whilesmart\ModelConfiguration\Enums\ConfigValueType::Boolean);

});

sourceant

Review complete. No specific code suggestions were generated. See the overview comment for a summary.

github-actions · 2026-05-04T06:59:18Z

Coverage Report
PR coverage: 71.88%
Baseline: 71.71%
Change: ✅+0.2%

kofimokome · 2026-05-05T16:29:22Z

+            'files.*' => 'file|mimes:jpg,jpeg,png,pdf|max:1240',
+
+            'from_wallet_id' => 'required_if:convert_myself_to_transfer,true|integer|exists:wallets,id',
+        ]);


You haven't updated the codes to use Laravel Requests

iMercyvlogs · 2026-05-08T13:23:37Z

@kofimokome Notice that in the code, prior to being assigned the task of supporting myself party type the validation was done inlline through a custom validateRequest() method inherited from ApiController. So I simply moved the existing logic into a new helper method(validateRequestData()) with aim of reducing the store() method's complexity. Are you now saying I should change the validation strategy to rather use Laravel's built-in form request classes? In such a case, wouldn't I need to change all instances within the code base where validation was done?

kofimokome · 2026-05-11T09:05:23Z

I strongly disagree with your last point. You would still need to use a form request.

You can decide to do it in another PR.

nfebe

@iMercyvlogs Use FormRequest that is the correct way to do it, if not there is not need to refactor this into a function.

Plus using the FormRequest will increase pattern usage of forms which is a good thing as opposed to writing class-based validators like this.

iMercyvlogs · 2026-05-12T12:48:49Z

@kofimokome @nfebe #232

nfebe

is_myself is not limited to transfers. That is already clear for transfers as users cannot transfer to anything else except a wallet they own.

So the is-myself config on parties make sense but it has/should have wider applications beyond transfers, it could also have a user specific config to enable the behavior.

Lets keep this open until I have time to look deeper and also research what will be best for UX.

enh: Myself party changed to transfer

e94acd2

iMercyvlogs requested review from kofimokome and nfebe and removed request for kofimokome March 12, 2026 12:02

sourceant Bot reviewed Mar 12, 2026

View reviewed changes

chore: Resolve conflicts

8abd89c

sourceant Bot suggested changes Mar 12, 2026

View reviewed changes

Comment thread app/Http/Controllers/API/v1/TransactionController.php Outdated

Comment thread database/migrations/2024_07_20_125104_create_parties_table.php Outdated

Comment thread tests/TestCase.php Outdated

nfebe requested changes Mar 12, 2026

View reviewed changes

Comment thread database/migrations/2024_07_20_125104_create_parties_table.php Outdated

enh: Myself party strictly by config no db change

7e2d3c4

sourceant Bot reviewed Mar 13, 2026

View reviewed changes

Comment thread app/Http/Controllers/API/v1/TransactionController.php Outdated

Comment thread app/Http/Controllers/API/v1/TransactionController.php

iMercyvlogs requested a review from kofimokome March 13, 2026 04:43

chore: Add helper functions in store 2 pass md test

81cc8e3

sourceant Bot suggested changes Mar 13, 2026

View reviewed changes

chore: Fix code standards

37bc40f

sourceant Bot reviewed Mar 13, 2026

View reviewed changes

Comment thread app/Http/Controllers/API/v1/TransactionController.php Outdated

kofimokome requested changes Mar 13, 2026

View reviewed changes

nfebe requested changes Mar 14, 2026

View reviewed changes

kofimokome requested changes Mar 14, 2026

View reviewed changes

Comment thread phpunit.xml Outdated

chore: Added argument controller method to pass tests

2e777e1

sourceant Bot approved these changes Mar 16, 2026

View reviewed changes

chore: Change result to jsonresponse and take more user types

5e166a2

sourceant Bot reviewed Mar 16, 2026

View reviewed changes

chore: Fix php code standards

bbb2cc3

sourceant Bot reviewed Mar 16, 2026

View reviewed changes

chore: Revert testcase and phpunit changes

58b2098

sourceant Bot reviewed Mar 16, 2026

View reviewed changes

iMercyvlogs added 2 commits March 16, 2026 14:14

enh: Import and use config package

b933206

chore: Coding standard tests pass

8000581

sourceant Bot suggested changes Mar 16, 2026

View reviewed changes

Comment thread app/Http/Controllers/API/v1/TransactionController.php Outdated

Comment thread database/seeders/ConfigurationSeeder.php Outdated

sourceant Bot reviewed Apr 13, 2026

View reviewed changes

chore: Helper functions to reduce cyclo complexity

de2b185

sourceant Bot suggested changes Apr 13, 2026

View reviewed changes

iMercyvlogs added 2 commits April 13, 2026 00:07

Revert "chore: Undo previous changes since they rather increase compl…

c83223c

…exity to 91" This reverts commit de2b185.

Revert "chore: Undo changes since they rather increase complexity to 90"

146607b

This reverts commit ae50a3b.

sourceant Bot approved these changes Apr 13, 2026

View reviewed changes

fix: Suppress cyclocomplexity issue

eb32e9c

iMercyvlogs requested a review from kofimokome April 28, 2026 20:22

kofimokome requested changes Apr 28, 2026

View reviewed changes

nfebe requested changes Apr 28, 2026

View reviewed changes

Comment thread database/migrations/2025_06_30_120453_create_configurations_table.php Outdated

enh: Add convertmyselftotransfer in payload

63af365

sourceant Bot reviewed Apr 30, 2026

View reviewed changes

chore: Find party only when id is not null

d1588b9

sourceant Bot approved these changes Apr 30, 2026

View reviewed changes

iMercyvlogs added 5 commits April 30, 2026 07:53

enh: Return transaction instead of transfer

a3c24f2

chore: Remove config migration table

378bab1

chore: Merge conflicts

4362f7f

chore: Reduce line characters to pass code standards test

4f5dddb

chore: Merge conflict

55c059b

iMercyvlogs requested review from kofimokome and nfebe May 4, 2026 07:01

kofimokome requested changes May 5, 2026

View reviewed changes

iMercyvlogs requested a review from kofimokome May 10, 2026 22:13

kofimokome approved these changes May 11, 2026

View reviewed changes

nfebe requested changes May 11, 2026

View reviewed changes

nfebe reviewed Jun 2, 2026

View reviewed changes

-    ) {
+        $partyId = $data['party_id'] ?? null;
+        if (config('app.convert_myself_to_transfer') && $partyId) {
+            $party = $user->parties()->find($partyId);
+            if ($party?->is_myself && $request->has('from_wallet_id')) {
+                $transfer = $this->transferService->transfer(
+                    amountToSend: (float) $data['amount'],
+                    fromWallet: $user->wallets()->findOrFail($request['from_wallet_id']),
+                    amountToReceive: (float) $data['amount'],
+                    toWallet: $user->wallets()->findOrFail($data['wallet_id']),
+                    user: $user,
+                    exchangeRate: 1.0,
+                    datetime: $data['datetime'] ?? null,
+                    transactionClientIds: ['income_transaction_client_id' => $data['client_id'] ?? null]
+                );
+                return $this->success($transfer, statusCode: 201);
+            }
+        }


		}

		private function isMyselfTransfer($data, $user, $request): bool

	private function isMyselfTransfer($data, $user, $request): bool
	private function isMyselfTransfer(array $data, \App\Models\User $user, Request $request): bool

-    }
+    private function isMyselfTransfer(array $data, User $user, Request $request): bool
+    {
+        $myselfPartyId = $user->getMyselfPartyId();
+        $isMyself = isset($data['party_id']) && (string)$data['party_id'] === (string)$myselfPartyId;
+        return config('app.convert_myself_to_transfer') &&
+            $isMyself &&
+            $request->has('from_wallet_id');
+    }

-    }
+    private function isMyselfTransfer(array $data, User $user, Request $request): bool
+    {
+        if (!$user->getConfigValue('create-transfers-for-myself-transactions') || !$request->has('from_wallet_id')) {
+            return false;
+        }
+        $partyId = $data['party_id'] ?? null;
+        if (!$partyId) {
+            return false;
+        }
+        $party = $user->parties()->find($partyId);
+        return $party && $party->getConfigValue('is-myself');
+    }

	'from_wallet_id' => 'required_if:convert_myself_to_transfer,true\|integer\|exists:wallets,id',
	'from_wallet_id' => 'nullable\|integer\|exists:wallets,id',


		$isMyselfParty = $party && $party->getConfigValue('is-myself');

		return $featureEnabled && $isMyselfParty && $request->has('from_wallet_id');

-        foreach (\App\Models\User::all() as $user) {
+        \App\Models\User::all()->each(function ($user) {
+            $user->setConfigValue('create-transfers-for-myself-transactions', true, \Whilesmart\ModelConfiguration\Enums\ConfigValueType::Boolean);
+        });

Conversation

iMercyvlogs commented Mar 12, 2026

Uh oh!

sourceant Bot commented Mar 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review Summary

🚀 Key Improvements

💡 Minor Suggestions

Uh oh!

sourceant Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourceant Bot Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

sourceant Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nfebe left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sourceant Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sourceant Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sourceant Bot Mar 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sourceant Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kofimokome left a comment

Choose a reason for hiding this comment

Uh oh!

nfebe left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nfebe Mar 14, 2026

Choose a reason for hiding this comment

Uh oh!

iMercyvlogs Apr 13, 2026

Choose a reason for hiding this comment

Uh oh!

kofimokome Apr 28, 2026

Choose a reason for hiding this comment

Uh oh!

kofimokome May 5, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sourceant Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourceant Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourceant Bot Mar 16, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

sourceant Bot left a comment

Choose a reason for hiding this comment

Uh oh!

sourceant Bot commented Mar 12, 2026 •

edited

Loading

nfebe left a comment •

edited

Loading

kofimokome commented May 11, 2026 •

edited

Loading