refactor: Use form requests for transactions

[iMercyvlogs]

Use FormRequest in store and update methods in the transactionController

[sourceant]

Code Review Summary

✨ This PR moves request validation for the transaction endpoints out of TransactionController and into dedicated form requests, introducing a shared ApiFormRequest base class. store() and update() now type-hint StoreTransactionRequest / UpdateTransactionRequest and read $request->validated() instead of calling the controller's validateRequest() helper and manually branching on isValidated; the removed validation arrays are relocated verbatim into the new request classes. ApiFormRequest centralizes two behaviors for API requests: casting textual booleans (e.g. "false") for attributes whose rules declare boolean, and converting validation failures into a JSON error envelope (success/message/errors) with a 422 when the failure is attributable to a named field and 400 otherwise. The pre-existing import-related requests (FileImportApiRequest, FixFailedImportsRequest, ImportAnalyzeRequest, ImportConfirmRequest) were reparented onto this base class, and feature tests were added for named-field failures, nested-field failures, and textual booleans. The review focuses on the field-matching logic used to choose between the 422 and 400 responses, which does not handle nested/wildcard error keys.

🚀 Key Improvements

• Validation rules for store and update are extracted from app/Http/Controllers/API/v1/TransactionController.php into app/Http/Requests/StoreTransactionRequest.php and app/Http/Requests/UpdateTransactionRequest.php, removing duplicated controller plumbing and the custom rules (ValidateClientId, Iso8601DateTime, file mime/size limits) being re-declared inline.
• ApiFormRequest::validationData() casts textual booleans for attributes whose rules declare boolean, while leaving unrecognized values untouched so the boolean rule still rejects them instead of silently coercing to false.
• ApiFormRequest::failedValidation() gives all API form requests a consistent JSON failure envelope (success, message, errors) along with a status code chosen from whether the failure maps to a declared field.
• Tests in tests/Feature/TransactionsTest.php cover a missing named field, a nested categories.0 failure, and acceptance of the textual boolean 'false'.

📉 Regressions

• app/Http/Requests/ApiFormRequest.php:42 — the 422-vs-400 decision keys off array_keys($this->rules()), which only contains declared attributes (categories, categories.*). MessageBag stores errors under flat concrete keys such as categories.0, so a failure that occurs only on a nested/wildcard rule is treated as unnamed, returned as 400 with "Server unable to process request.", and contradicts the new test_validation_failure_on_a_nested_field_is_reported_under_its_own_key test, which expects 422.
• app/Http/Requests/ApiFormRequest.php:43 — MessageBag::hasAny() matches keys exactly, so declared wildcard rules like categories.* never match a concrete error key, weakening the named-field error contract for any request extending this base class, including the import requests reparented onto it.

🚨 Critical Issues

• MessageBag stores messages under their flat, dotted attribute name (the same keys Laravel emits in JSON validation payloads, e.g. categories.0). array_keys($this->rules()) only contains the declared keys — categories and categories.* — never categories.0. Therefore hasAny(...) returns false for a failure that only occurs on a nested/wildcard rule, and the request falls through to the 400 branch. The new test test_validation_failure_on_a_nested_field_is_reported_under_its_own_key posts ['categories' => [999999]], whose only error key is categories.0, yet asserts 422 — with the current check that request returns 400 and the test fails. Match nested keys against the declared top-level attributes as well.
• MessageBag::hasAny() compares keys exactly, so declared wildcard rules such as categories.* never match a concrete error key such as categories.0. Nested-field failures will therefore be classified as unrecognised and returned as 400 / "Server unable to process request." instead of 422, contradicting the added test test_validation_failure_on_a_nested_field_is_reported_under_its_own_key and the API's named-field contract. Expand the check so any error key matching a declared rule (including via Str::is wildcards) counts as named.

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

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

[github-actions]

Coverage Report
PR coverage: 73.51%
Baseline: 73.48%
Change: ✅+0%

[nfebe]

Pull request title not properly named, seems to have is_myself changes leaking in.

[nfebe]

@iMercyvlogs the pull request title does not follow the patterns we use

[kofimokome]

This is good. However, failure responses have a structure. See

$this->failure()

It should be such that If the form request validation fails, the response structure should follow the same structure as $this->failure()

[nfebe]

Merged dev and corrected three things the branch had drifted on. The refactor itself is unchanged: store() and update() take a form request and read $request->validated().

The rules had gone stale. dev has since added intent and capped categories at max:1. The extracted rules were missing both, so merging as-is would have dropped validation that exists today. They now match dev line for line, checked by diffing the two rule sets.

The branch carried rules from another PR. convert_myself_to_transfer and from_wallet_id belong to #185 and exist nowhere on dev. Removed.

ApiFormRequest dropped two behaviours that ApiController::validateRequest() has. It casts textual booleans before validating, so is_recurring: "false" is read as a boolean rather than rejected, and it answers 400 with a different message when no error key matches a rule key. Both are restored, so the refactor changes no response the API can produce.

Three tests cover it: a named-field failure returns 422, a nested-field failure reports under categories.0, and a textual boolean is accepted.

One correction to something said in review: the 400 path is not reachable through a wildcard rule. MessageBag::has() matches wildcard keys, so categories.* does match an error on categories.0 and the response is 422. ApiFormRequest still implements both paths so nothing depends on that holding.

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

expectsBoolean() casts non-array rules to string unconditionally. validationData() runs for every subclass of ApiFormRequest (including the four import requests, whose rules() we cannot see here). If any rule is declared as a bare object/Closure (e.g. 'field' => new MyRule() or 'field' => function () {}), (string) $rule throws Object of class ... could not be converted to string, crashing the request before validation even runs. Custom ValidationRule implementations generally do not define __toString(). Guard the cast so only string rules are exploded.

Suggested change

	$rules = is_array($rule) ? $rule : explode('|', (string) $rule);
	+ $rules = is_array($rule) ? $rule : (is_string($rule) ? explode('|', $rule) : []);

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

This duplicates the API validation-failure envelope that already lives on the base controller (ApiController::validateRequest() / failure()), which produced the same two messages and was consumed by the removed controller code via $validationResult['isValidated'] / $this->failure(...). Two independent implementations of the same contract can now diverge (message text, 422/400 status, payload shape), and because TransactionController still extends ApiController, other endpoints keep using the old path. Extract the envelope into one reusable method so both call sites share it instead of each owning a copy.

Suggested change

	{
	protected function failedValidation(Validator $validator): void
	{
	throw new HttpResponseException($this->validationErrorResponse(
	$validator->errors(),
	array_keys($this->rules())
	));
	}
	/**
	* Single source of truth for the API validation-error envelope.
	*
	* ApiController::failure()/validateRequest() previously produced these same
	* messages and status codes; they should delegate here so the API exposes
	* one contract instead of two that can drift apart.
	*/
	protected function validationErrorResponse($errors, array $knownAttributes)
	{
	$named = $errors->hasAny($knownAttributes);
	return response()->json([
	'success' => false,
	'message' => $named
	? __('Server failed to validate request.')
	: __('Server unable to process request.'),
	'errors' => $errors->toArray(),
	], $named ? 422 : 400);
	}

[sourceant-local]

This repository is not connected to any of your workspaces. Please connect it at https://app.sourceant.ai to get reviews on it.

Superseded: the branch was rebased onto dev and the code these reviews point at no longer exists.

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

MessageBag stores messages under their flat, dotted attribute name (the same keys Laravel emits in JSON validation payloads, e.g. categories.0). array_keys($this->rules()) only contains the declared keys — categories and categories.* — never categories.0. Therefore hasAny(...) returns false for a failure that only occurs on a nested/wildcard rule, and the request falls through to the 400 branch. The new test test_validation_failure_on_a_nested_field_is_reported_under_its_own_key posts ['categories' => [999999]], whose only error key is categories.0, yet asserts 422 — with the current check that request returns 400 and the test fails. Match nested keys against the declared top-level attributes as well.

Suggested change

	$errors = $validator->errors();
	$errors = $validator->errors();
	$rules = array_keys($this->rules());
	$named = $errors->hasAny($rules)
	|| collect($errors->keys())->contains(
	fn (string $key) => in_array(explode('.', $key)[0], $rules, true)
	);

[sourceant]

MessageBag::hasAny() compares keys exactly, so declared wildcard rules such as categories.* never match a concrete error key such as categories.0. Nested-field failures will therefore be classified as unrecognised and returned as 400 / "Server unable to process request." instead of 422, contradicting the added test test_validation_failure_on_a_nested_field_is_reported_under_its_own_key and the API's named-field contract. Expand the check so any error key matching a declared rule (including via Str::is wildcards) counts as named.

Suggested change

	$named = $errors->hasAny(array_keys($this->rules()));
	$ruleKeys = array_keys($this->rules());
	$named = $errors->hasAny($ruleKeys)
	|| collect($errors->keys())->contains(
	fn ($errorKey) => collect($ruleKeys)->contains(
	fn ($ruleKey) => \Illuminate\Support\Str::is($ruleKey, $errorKey)
	)
	);