feat: support for myself party type - #185
iMercyvlogs wants to merge 1 commit into
Conversation
Code Review Summary✨ Adds a 🚀 Key Improvements
📉 Regressions
💡 Minor Suggestions
🚨 Critical Issues
|
![sourceant[bot]](https://avatars.githubusercontent.com/in/1161781?s=60&v=4){kind=small}
| private RecurringTransactionService $recurringTransactionService | ||
| private RecurringTransactionService $recurringTransactionService, | ||
| private TransferService $transferService | ||
| ) { |
There was a problem hiding this comment.
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.
| ) { | |
| $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); | |
| } | |
| } |
nfebe
left a comment
nfebe
left a comment
There was a problem hiding this comment.
Thank for this! I took a quick look and have one comment for now :)
|
|
||
| } | ||
|
|
||
| private function isMyselfTransfer($data, $user, $request): bool |
There was a problem hiding this comment.
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.
| private function isMyselfTransfer($data, $user, $request): bool | |
| private function isMyselfTransfer($data, $user): bool |
kofimokome
left a comment
kofimokome
left a comment
There was a problem hiding this comment.
Your tests are failing
There was a problem hiding this comment.
-
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-myselfset on party andcreate-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
| 'files.*' => 'file|mimes:jpg,jpeg,png,pdf|max:1240', | ||
|
|
||
| 'from_wallet_id' => 'required_if:convert_myself_to_transfer,true|integer|exists:wallets,id', | ||
| ]); |
There was a problem hiding this comment.
| ]); |
Create a request instead and there should likely be a dedicated commit for that
There was a problem hiding this comment.
sorry, I didn't quite understand this
There was a problem hiding this comment.
check the Laravel documentation here => https://laravel.com/docs/13.x/validation#form-request-validation
There was a problem hiding this comment.
You haven't updated the codes to use Laravel Requests
| ]); | ||
| } | ||
|
|
||
| private function isMyselfTransfer($data, $user, $request): bool |
There was a problem hiding this comment.
Type hinting the parameters improves code clarity and helps static analysis tools catch errors.
| private function isMyselfTransfer($data, $user, $request): bool | |
| private function isMyselfTransfer(array $data, \App\Models\User $user, Request $request): bool |
| private RecurringTransactionService $recurringTransactionService, | ||
| private TransferService $transferService | ||
| ) { | ||
| } |
There was a problem hiding this comment.
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.
| } | |
| 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'); | |
| } |
| $data = $validationResult['data']; | ||
| $user = $request->user(); | ||
|
|
||
| //check if party_id is the user's "myself" party |
There was a problem hiding this comment.
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.
| //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); | |
| } |
| )] | ||
| class Party extends Model | ||
| { | ||
| use configurable; |
There was a problem hiding this comment.
The trait name should follow PascalCase convention for consistency, although PHP is case-insensitive for traits.
| use configurable; | |
| use Configurable; |
| { | ||
| return $this->getConfigValue('avatar'); | ||
| } | ||
| protected static function booted() |
There was a problem hiding this comment.
Using the booted static method for logic like this can be fragile during bulk imports or migrations. It is generally better to handle default user settings via a Service/Action class when a user is registered, or use database defaults where possible.
| protected static function booted() | |
| // Consider moving this logic to a User Registration Service or an Observer. |
|
@iMercyvlogs you have two unresolved changes(suggestions) from nfebe. |
| private RecurringTransactionService $recurringTransactionService, | ||
| private TransferService $transferService | ||
| ) { | ||
| } |
There was a problem hiding this comment.
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.
| } | |
| 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'); | |
| } |
| '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', |
There was a problem hiding this comment.
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.
| 'from_wallet_id' => 'required_if:convert_myself_to_transfer,true|integer|exists:wallets,id', | |
| 'from_wallet_id' => 'nullable|integer|exists:wallets,id', |
| 'files.*' => 'file|mimes:jpg,jpeg,png,pdf|max:1240', | ||
|
|
||
| 'from_wallet_id' => 'required_if:convert_myself_to_transfer,true|integer|exists:wallets,id', | ||
| ]); |
There was a problem hiding this comment.
check the Laravel documentation here => https://laravel.com/docs/13.x/validation#form-request-validation
nfebe
left a comment
nfebe
left a comment
There was a problem hiding this comment.
No need to publish the configurations table and use FormRequest like Kofi mentioned.
|
|
||
| $isMyselfParty = $party && $party->getConfigValue('is-myself'); | ||
|
|
||
| return $featureEnabled && $isMyselfParty && $request->has('from_wallet_id'); |
There was a problem hiding this comment.
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.
| return $featureEnabled && $isMyselfParty && $request->has('from_wallet_id'); | |
| return $featureEnabled && $isMyselfParty && isset($data['from_wallet_id']); |
| 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) { |
There was a problem hiding this comment.
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.
| 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); | |
| }); |
|
Coverage Report |
| 'files.*' => 'file|mimes:jpg,jpeg,png,pdf|max:1240', | ||
|
|
||
| 'from_wallet_id' => 'required_if:convert_myself_to_transfer,true|integer|exists:wallets,id', | ||
| ]); |
There was a problem hiding this comment.
You haven't updated the codes to use Laravel Requests
|
@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? |
|
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
left a comment
nfebe
left a comment
There was a problem hiding this comment.
@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.
nfebe
left a comment
nfebe
left a comment
There was a problem hiding this comment.
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.
Money moved between your own wallets was being recorded as income or spending, which inflated both. A party can now stand for the account holder, and a transaction against one moves the money instead. The request can decline the conversion, and so can a setting.
55c059b to
aa7aa89
Compare
| private TransactionWriter $transactionWriter, | ||
| private TransferService $transferService | ||
| ) { | ||
| } |
There was a problem hiding this comment.
Read the flag from the validated $data instead of the raw $request so the method depends only on validated input (and the Request parameter can be dropped). Also, the truthiness chain only recognises a few concrete representations of a boolean config value; normalising with filter_var(..., FILTER_VALIDATE_BOOLEAN) keeps the 'unset means on' default while correctly handling 'on'/'yes'/'off'/'0'. array_key_exists is used (rather than isset) so an explicit false is still honoured.
| } | |
| + if (array_key_exists('convert_myself_to_transfer', $data)) { | |
| + return (bool) $data['convert_myself_to_transfer']; | |
| + } | |
| + | |
| + // Unset means on, the way the notification preferences read. | |
| + $preference = $user->getConfigValue(ConfigurationKeys::TRANSFER_MYSELF_TRANSACTIONS); | |
| + | |
| + return $preference === null || filter_var($preference, FILTER_VALIDATE_BOOLEAN); |
|
|
||
| private function transferBetweenOwnWallets(array $data, User $user): JsonResponse | ||
| { | ||
| if (empty($data['from_wallet_id'])) { |
There was a problem hiding this comment.
Guard against from_wallet_id equal to the destination wallet_id. As written, a self-transfer calls TransferService::transfer with the same wallet for both legs, which nets to zero but writes an expense and an income leg against one wallet and can distort balance reconciliation. Reject it explicitly with a 422.
| if (empty($data['from_wallet_id'])) { | |
| + if (empty($data['from_wallet_id'])) { | |
| + return $this->failure(__('Server failed to validate request.'), 422, [ | |
| + 'from_wallet_id' => [__('Name the wallet the money leaves, or send convert_myself_to_transfer as false.')], | |
| + ]); | |
| + } | |
| + | |
| + if ((string) $data['from_wallet_id'] === (string) $data['wallet_id']) { | |
| + return $this->failure(__('Server failed to validate request.'), 422, [ | |
| + 'from_wallet_id' => [__('The wallet the money leaves must be different from the wallet it arrives at.')], | |
| + ]); | |
| + } |
| $partyId = $data['party_id'] ?? null; | ||
|
|
There was a problem hiding this comment.
The conversion is documented as applying to income transactions only, but the guard does not look at type. An expense sent against the 'myself' party takes this branch as well, producing a transfer whose fromWallet is from_wallet_id and toWallet is wallet_id — but for expenses wallet_id is the source wallet, so the transfer direction is inverted and an income leg is returned for an expense request.
| $partyId = $data['party_id'] ?? null; | |
| $partyId = $data['party_id'] ?? null; | |
| if ($partyId === null | |
| || ($data['type'] ?? '') !== 'income' | |
| || $user->parties()->whereKey($partyId)->value('type') !== 'myself') { | |
| return false; | |
| } |
| private TransactionWriter $transactionWriter, | ||
| private TransferService $transferService | ||
| ) { | ||
| } |
There was a problem hiding this comment.
from_wallet_id is validated with an unscoped exists:wallets,id. A wallet that does not exist returns the {success, message, errors} payload below, but a wallet that exists and belongs to another user passes validation and then throws a ModelNotFoundException from findOrFail, returning a framework 404 with a different body. Resolve the wallet once and report both cases through the same failure shape, keeping a single validator instead of adding a second one.
| } | |
| $sourceWallet = $user->wallets()->whereKey($data['from_wallet_id'] ?? null)->first(); | |
| if ($sourceWallet === null) { | |
| return $this->failure(__('Server failed to validate request.'), 422, [ | |
| 'from_wallet_id' => [__('Name a wallet that belongs to you the money leaves, or send convert_myself_to_transfer as false.')], | |
| ]); | |
| } | |
| $transfer = $this->transferService->transfer( | |
| amountToSend: (float) $data['amount'], | |
| fromWallet: $sourceWallet, |
| 'icon_type' => 'required_with:icon|string|in:icon,image,emoji', | ||
| // @phpcs:ignore | ||
| 'type' => 'sometimes|string|in:individual,organization,business,partnership,non_profit,government_agency,educational_institution,healthcare_provider', | ||
| 'type' => 'sometimes|string|in:myself,individual,organization,business,partnership,non_profit,government_agency,educational_institution,healthcare_provider', |
There was a problem hiding this comment.
The valid party types are declared as a duplicated inline literal here (store()) and again identically in update() at line 306. This PR had to touch both lists to add myself; the next type added to only one list will cause POST /parties and PUT /parties to disagree on what a valid type is. Extract the list once and reference it from both validations.
| 'type' => 'sometimes|string|in:myself,individual,organization,business,partnership,non_profit,government_agency,educational_institution,healthcare_provider', | |
| 'type' => 'sometimes|string|'.self::PARTY_TYPES, |
| $partyId = $data['party_id'] ?? null; | ||
|
|
There was a problem hiding this comment.
movesMoneyBetweenOwnWallets() never inspects $data['type'], yet it always builds the transfer as fromWallet = from_wallet_id and toWallet = wallet_id. For an income transaction that is correct (money lands in wallet_id). For an expense transaction wallet_id is the source wallet, so the same inputs invert the money flow — an expense-to-self would push money into wallet_id and out of from_wallet_id. The PR description scopes this feature to income ("Transactions 'From' myself (Income)"), and the tests only cover income, so the expense path is both untested and wrong. Guard the conversion on the transaction type.
| $partyId = $data['party_id'] ?? null; | |
| $partyId = $data['party_id'] ?? null; | |
| // Only money arriving is re-interpreted as a transfer. Doing the same for | |
| // an expense would move money into wallet_id and out of from_wallet_id. | |
| if (($data['type'] ?? null) !== 'income') { | |
| return false; | |
| } | |
| if ($partyId === null || $user->parties()->whereKey($partyId)->value('type') !== 'myself') { | |
| return false; | |
| } |
|
Closing. Recording what came out of finishing this so it is not rediscovered. A party type cannot drive the conversion, because it carries no direction. A transfer needs two wallets and a direction; a transaction has one wallet and a sign. "This party is me" supplies neither, so the endpoint has to guess, and it guesses from The wallet is untouched and nothing is recorded, although the request already said which wallet the money left. Supplying It also duplicates something that already works. Two bugs worth knowing about, independent of the design.
Unrelated, noticed while here: |
3 participants
added support for party type myself.... Transactions "From" myself (Income) are handled as transfers if configured so. Target issue: #169