enh(app): Ensure client id is attached to models #35
Conversation
- Sync client_generated_id with categories, parties and wallets when provided - Write tests to ensure that, create operations are still possible when `client_id` is provided Signed-off-by: nfebe <fenn25.fn@gmail.com>
- Improve date-time validation - Correct tests Signed-off-by: nfebe <fenn25.fn@gmail.com>
nfebe
force-pushed
the
fix/accept-client-id-on-create
branch
from
7e49da9 to
bb6b6d2
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
app/Http/Controllers/API/v1/TransferController.php (1)
57-68: 💡 Verification agent🧩 Analysis chain
Consider adding client_id handling logic similar to other controllers
While the validation now includes
client_id, the transfer creation logic doesn't appear to have been updated to store the client_id on the model, unlike other controllers mentioned in the PR. This may lead to inconsistent behavior.Run this script to verify how other controllers implement client_id handling:
🏁 Script executed:
#!/bin/bash # Check how other controllers implement client_id handling rg -A 15 "client_id.*=.*request" --type php app/Http/ControllersLength of output: 66
🏁 Script executed:
#!/bin/bash # Look for direct assignments of client_id from the request in controllers rg -n "\$request->client_id" --type php app/Http/Controllers # Fallback: any occurrence of 'client_id' in controller code rg -n "client_id" --type php app/Http/ControllersLength of output: 2846
🏁 Script executed:
#!/bin/bash file="app/Http/Controllers/API/v1/TransferController.php" # Find the line number of the store method store_line=$(rg -n "public function store" --max-count 1 "$file" | cut -d: -f1) # Calculate context range start=$((store_line > 5 ? store_line - 5 : 1)) end=$((store_line + 60)) # Print the context around the store method sed -n "${start},${end}p" "$file"Length of output: 2420
Ensure client_id Is Persisted in TransferController
The
store()method validatesclient_idbut never assigns it to the created transfer. To stay consistent with other controllers (WalletController,TransactionController, etc.), add logic to persistclient_idon the transfer model. For example, inapp/Http/Controllers/API/v1/TransferController.php, inside thestore()method—after you pull in$databut before or within the DB transaction—do something like:$data = $validationResult['data']; + + // Persist client-generated ID if provided + $clientId = $data['client_id'] ?? null; $transfer = DB::transaction(function () use ($data, $fromWallet, $toWallet, $amountToReceive, $user, $exchangeRate, $clientId) { - return $this->transferService->transfer( - amountToSend: $data['amount'], - fromWallet: $fromWallet, - amountToReceive: $amountToReceive, - toWallet: $toWallet, - user: $user, - exchangeRate: $exchangeRate - ); + $transfer = $this->transferService->transfer( + amountToSend: $data['amount'], + fromWallet: $fromWallet, + amountToReceive: $amountToReceive, + toWallet: $toWallet, + user: $user, + exchangeRate: $exchangeRate + ); + if ($clientId) { + $transfer->setClientGeneratedId($clientId); + $transfer->save(); + } + return $transfer; });• File:
app/Http/Controllers/API/v1/TransferController.php
• Method:store()
• Location: just after assigning$data(around line 60)This will mirror the pattern in your other controllers and ensure
client_idis stored correctly.app/Http/Controllers/API/v1/PartyController.php (1)
22-23:⚠️ Potential issueIncorrect OpenAPI annotation
There's a misplaced
OA\Propertywithin the parameters array of the OpenAPI annotation for the GET endpoint.This should be a
OA\Parameterinstead, or it should be moved to the appropriate location. Fix:- new OA\Property(property: 'client_id', type: 'string', format: 'uuid', - description: 'Unique identifier for your local client'), + new OA\Parameter( + name: 'client_id', + in: 'query', + required: false, + schema: new OA\Schema(type: 'string', format: 'uuid'), + description: 'Unique identifier for your local client' + ),
🧹 Nitpick comments (7)
app/Rules/Iso8601Date.php (1)
17-21: Consider expanding ISO 8601 validation pattern for more format variations.The current regex only supports the UTC timezone indicator 'Z' but doesn't handle timezone offsets like "+01:00" or "-05:00" which are valid in ISO 8601.
- $pattern = '/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d{3})?Z$/'; + $pattern = '/^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}(?:\.\d{3})?(Z|[+-]\d{2}:\d{2})$/';tests/Feature/GroupTest.php (1)
49-59: Enhance test verification for client_id handling.The test confirms the group is created successfully, but doesn't explicitly verify that the client_id is properly stored.
$response->assertStatus(201); $this->assertDatabaseHas('groups', ['id' => $response->json('data.id')]); + $this->assertDatabaseHas('groups', [ + 'id' => $response->json('data.id'), + 'client_generated_id' => '123e4567-e89b-12d3-a456-426614174000' + ]);tests/Feature/WalletTest.php (1)
40-40: Minor typo in test descriptionThere's a small typo in the description field: "descriptoin" should be "description".
- 'description' => 'test descriptoin', + 'description' => 'test description',tests/Feature/PartyTest.php (1)
40-40: Same typo in test descriptionThere's the same typo in the description field: "descriptoin" should be "description".
- 'description' => 'test descriptoin', + 'description' => 'test description',tests/Feature/CategoriesTest.php (1)
36-58: Good test coverage for client ID functionalityThis test properly verifies that the API can handle category creation with a client ID, checking both the response structure and database persistence.
Two suggestions for enhancement:
- Consider adding an assertion to verify that the client ID is actually stored correctly:
$this->assertDatabaseHas('categories', ['id' => $response->json('data.id')]); +$this->assertDatabaseHas('categories', [ + 'id' => $response->json('data.id'), + 'client_generated_id' => '123e4567-e89b-12d3-a456-426614174000' +]);
- For completeness, add a test case for creating income categories with client ID as well.
app/Http/Controllers/API/v1/WalletController.php (1)
115-124: Enhanced wallet creation with client ID supportThe implementation properly handles client IDs and synchronization state. However, there's an inconsistency in how client_id is checked.
Consider using
!empty($validatedData['client_id'])instead ofisset($request['client_id'])for more consistent and robust checking:- if (isset($request['client_id'])) { - $wallet->setClientGeneratedId($request['client_id']); + if (!empty($validatedData['client_id'])) { + $wallet->setClientGeneratedId($validatedData['client_id']);This would be more consistent with how it's implemented in PartyController.
app/Http/Controllers/API/v1/PartyController.php (1)
117-127: Consider extracting common controller logic to a traitThis client ID and sync state handling pattern is repeated across multiple controllers (Party, Wallet, etc.) with similar implementation.
Consider extracting this common logic to a trait to reduce code duplication and maintain consistency. For example:
trait HandlesSyncableResources { protected function createSyncableResource($user, $model, $validatedData) { try { $resource = $user->{$model}()->create($validatedData); if (!empty($validatedData['client_id'])) { $resource->setClientGeneratedId($validatedData['client_id']); } $resource->markAsSynced(); return $resource; } catch (\Exception $e) { return false; } } }This would simplify your controller code and ensure consistent behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
app/Http/Controllers/API/v1/CategoryController.php(3 hunks)app/Http/Controllers/API/v1/GroupController.php(2 hunks)app/Http/Controllers/API/v1/PartyController.php(3 hunks)app/Http/Controllers/API/v1/TransactionController.php(4 hunks)app/Http/Controllers/API/v1/TransferController.php(2 hunks)app/Http/Controllers/API/v1/WalletController.php(2 hunks)app/Rules/Iso8601Date.php(1 hunks)app/helpers.php(1 hunks)composer.json(1 hunks)tests/Feature/CategoriesTest.php(1 hunks)tests/Feature/GroupTest.php(1 hunks)tests/Feature/PartyTest.php(1 hunks)tests/Feature/TransactionsTest.php(6 hunks)tests/Feature/WalletTest.php(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
app/Http/Controllers/API/v1/TransferController.php (1)
app/Rules/Iso8601Date.php (1)
Iso8601Date(8-23)
tests/Feature/CategoriesTest.php (1)
app/Models/User.php (1)
User(12-79)
app/Http/Controllers/API/v1/WalletController.php (4)
app/Rules/Iso8601Date.php (1)
Iso8601Date(8-23)app/Models/User.php (1)
wallets(65-68)app/Traits/Syncable.php (2)
setClientGeneratedId(20-28)markAsSynced(40-43)app/Http/Controllers/API/ApiController.php (1)
failure(107-114)
🔇 Additional comments (26)
composer.json (1)
30-33: Good addition of global helper autoloading.The addition of
"files": ["app/helpers.php"]follows Laravel best practices for autoloading global helper functions. This enables the new date formatting function to be available application-wide without requiring explicit imports.app/helpers.php (1)
5-17: Well-implemented helper function for ISO 8601 date format conversion.The
format_iso8601_to_sqlfunction is implemented with proper null handling, error catching, and uses Carbon for robust date parsing.app/Http/Controllers/API/v1/TransferController.php (2)
6-6: Import added for new ISO 8601 date validation ruleThe addition of the Iso8601Date import aligns with the PR's implementation of consistent date format handling across controllers.
63-63: Validation rule updated to use custom ISO 8601 formatThe validation for
created_athas been improved to use the customIso8601Datevalidation rule, which properly validates ISO 8601 formatted dates (e.g., "2025-04-30T15:17:54.120Z"). This change is consistent with similar updates in other controllers.tests/Feature/WalletTest.php (1)
32-45: Good test for wallet creation with client_idThis test appropriately verifies the new functionality of adding a client-generated ID to a wallet. It correctly tests both the successful creation of the wallet and confirms the record exists in the database.
tests/Feature/PartyTest.php (1)
36-45: Well-structured test for party creation with client_idThis test properly verifies the new functionality of attaching a client-generated ID to a party. The test structure follows the same pattern as other similar tests in the codebase.
tests/Feature/TransactionsTest.php (3)
41-41: Date format updated to ISO 8601The date format has been correctly updated to use ISO 8601 format, which aligns with the validation changes in the controllers.
58-71: Good test for transaction creation with client_idThis test effectively verifies the new functionality for creating transactions with a client-generated ID. The test structure is consistent with other similar tests in the suite.
99-99: Consistent ISO 8601 date format updatesAll date strings in the test file have been updated to use the new ISO 8601 format, which ensures consistency across the test suite and matches the new validation requirements.
Also applies to: 121-121, 134-134, 184-184
app/Http/Controllers/API/v1/GroupController.php (3)
6-7: Good addition of dependenciesThe inclusion of both
Groupmodel andIso8601Daterule directly clarifies the code's dependencies, which is good for maintainability.
105-105: Improved date validation with custom ruleReplacing the strict date format with the custom
Iso8601Datevalidation rule provides better flexibility for handling date formats while maintaining validation.
113-123:✅ Verification successful
Well-structured error handling and client ID support
The implementation correctly wraps the group creation in a try-catch block, handles the client ID when provided, and marks the record as synced. The error handling returns appropriate status codes and messages.
However, we should verify that the
markAsSyncedmethod exists on the Group model:
🏁 Script executed:
#!/bin/bash # Check if Group model has the necessary methods rg -A 2 "function (setClientGeneratedId|markAsSynced)" app/Models/Group.phpLength of output: 77
To locate where (or if) these methods are defined on the Group model (including traits), let’s dump its imports/traits and search the entire repo:
🏁 Script executed:
#!/bin/bash # Show the top of the Group model (namespaces, uses, traits) sed -n '1,200p' app/Models/Group.php # Search whole repo for method definitions rg -n "function setClientGeneratedId" . rg -n "function markAsSynced" .Length of output: 1660
Approve error handling and syncing logic
Verified that
app/Models/Group.phpuses theSyncabletrait (inapp/Traits/Syncable.php), which defines bothsetClientGeneratedId()andmarkAsSynced(). The try-catch block, client ID handling, and sync marking are all correct.app/Http/Controllers/API/v1/CategoryController.php (3)
6-7: Good addition of dependenciesThe inclusion of both
Categorymodel andIso8601Daterule directly clarifies the code's dependencies, which is good for maintainability.
96-96: Improved date validation with custom ruleReplacing the strict date format with the custom
Iso8601Datevalidation rule provides better flexibility for handling date formats while maintaining validation.
111-121:✅ Verification successful
Well-structured error handling and client ID support
The implementation correctly wraps the category creation in a try-catch block, handles the client ID when provided, and marks the record as synced. The error handling returns appropriate status codes and messages.
Let's verify that both methods exist on the Category model:
🏁 Script executed:
#!/bin/bash # Check if Category model has the necessary methods rg -A 2 "function (setClientGeneratedId|markAsSynced)" app/Models/Category.phpLength of output: 80
🏁 Script executed:
#!/bin/bash # Searching for method definitions across the app directory rg -n "function setClientGeneratedId" -n app rg -n "function markAsSynced" -n appLength of output: 230
Well-structured error handling and client ID support
I’ve confirmed that both
setClientGeneratedIdandmarkAsSyncedare defined inapp/Traits/Syncable.phpand applied to theCategorymodel. The try-catch block, client ID assignment, and sync marking are all implemented correctly.app/Http/Controllers/API/v1/TransactionController.php (6)
7-7: Good addition of dependencyAdding the
Iso8601Daterule clarifies the code's validation dependencies.
108-110: Improved date validation with custom ruleUsing the custom
Iso8601Datevalidation rule for both datetime fields improves flexibility while maintaining proper validation.
282-283: Consistent date validation across methodsUsing the same validation approach for dates in both store and update methods ensures consistency.
Also applies to: 289-290
298-304: Proper date format conversion for updatesThe implementation correctly converts ISO 8601 dates to SQL format for updates as well, maintaining consistency with the store method.
123-129:✅ Verification successful
Proper date format conversion
Properly converting ISO 8601 dates to SQL format ensures compatibility with the database storage.
Let's verify the
format_iso8601_to_sqlhelper function exists and works correctly:
🏁 Script executed:
#!/bin/bash # Check the helper function implementation grep -A 10 "function format_iso8601_to_sql" app/helpers.phpLength of output: 346
Verified ISO 8601 to SQL date conversion
I’ve confirmed that the
format_iso8601_to_sqlhelper inapp/helpers.php
– Correctly handlesnull/empty input
– UsesCarbon::parse(...)->format('Y-m-d H:i:s')with exception safetyNo further changes needed; approving the date‐format conversion.
137-156:✅ Verification successful
Comprehensive error handling and client ID support
The implementation correctly:
- Wraps transaction creation in a try-catch block
- Sets the client ID when provided
- Marks the transaction as synced
- Handles categories synchronization
- Includes error logging and returns appropriate error responses
This is a robust implementation with proper error handling.
Let's verify the Transaction model has the necessary methods:
🏁 Script executed:
#!/bin/bash # Check if Transaction model has the necessary methods rg -A 2 "function (setClientGeneratedId|markAsSynced)" app/Models/Transaction.phpLength of output: 83
🏁 Script executed:
#!/bin/bash # Search the entire repository for the methods in question rg -n "function setClientGeneratedId" . rg -n "function markAsSynced" . rg -n "setClientGeneratedId" . rg -n "markAsSynced" .Length of output: 1636
🏁 Script executed:
#!/bin/bash # Verify that the Transaction model uses the Syncable trait rg -n "use .*Syncable" app/Models/Transaction.php rg -n "class Transaction" -A5 app/Models/Transaction.phpLength of output: 349
Comprehensive error handling and client ID support – Verified
All required methods are present and correctly applied:
- app/Models/Transaction.php uses the Syncable trait, which defines setClientGeneratedId() and markAsSynced().
- The controller wraps creation in a try-catch, sets client ID, marks as synced, syncs categories, logs errors, and returns a proper failure response.
No changes needed; this is a robust implementation.
app/Http/Controllers/API/v1/WalletController.php (2)
6-7: Appropriate imports addedAdded imports for
Walletmodel andIso8601Datevalidation rule to support the new implementation.
110-110: Improved date validationReplacing the fixed date format with the custom
Iso8601Datevalidation rule provides better support for standard ISO 8601 formatted dates.app/Http/Controllers/API/v1/PartyController.php (3)
7-7: Appropriate import addedAdded import for
Iso8601Datevalidation rule to support the new implementation.
108-108: Improved date validationReplacing the fixed date format with the custom
Iso8601Datevalidation rule provides better support for standard ISO 8601 formatted dates.
118-127: Well-implemented party creation with client ID supportThe implementation properly handles client IDs and synchronization state with appropriate error handling. The use of
!empty($validatedData['client_id'])is a good practice for checking the presence of optional values.
client_idis provided