feat: add recurring transaction service

[iMercyvlogs]

-added recurring transaction service file

• Updated job file for recurring transactions to only call service
• Created command file in Console/commands folder
• In transaction controller, made store() and update() methods to only recieve recurring transactions' id, not their recurrence and expiry dates

[sourceant]

Code Review Summary

✨ This pull request introduces a robust system for handling recurring transactions, separating the core logic into a dedicated RecurringTransactionService and simplifying the RecurrentTransactionJob. The console command transactions:process-recurring is added to initiate the processing of due transactions. The TransactionController is updated to correctly dispatch the simplified job.

While the architecture is strong, a critical bug exists in the daily processing logic that needs immediate attention to ensure correct and efficient scheduling.

🚀 Key Improvements

• Centralized Recurring Transaction Logic: Moving the complex logic into app/Services/RecurringTransactionService.php significantly improves modularity and maintainability.
• Simplified RecurrentTransactionJob: The job now acts as a thin wrapper, delegating the heavy lifting to the service, adhering to the 'thin jobs, fat services' principle.
• Standardized Job Dispatching: The TransactionController now dispatches the simplified job consistently, reducing potential for errors.

💡 Minor Suggestions

• Exception Handling in Console Command: Improve error handling in app/Console/Commands/ProcessRecurringTransactions.php to catch \Throwable and log detailed exceptions.
• Specific Schedule Time: Assign a precise execution time to the transactions:process-recurring command in app/Console/Kernel.php for better resource management.
• Add next_scheduled_at to RecurringTransactionRule: Implement a next_scheduled_at column in the recurring_transaction_rules database table to enable accurate tracking and querying of due transactions.
• File End Newline: Ensure docker-compose.yml ends with a newline character for best practice compliance.

🚨 Critical Issues

• Flawed processAllDueTransactions Logic: The RecurringTransactionService::processAllDueTransactions method incorrectly dispatches jobs for all active recurring rules daily. This can lead to redundant job processing and premature transaction generation. This requires adding a next_scheduled_at field to RecurringTransactionRule and modifying the query to only process genuinely due rules.

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

The command name transactions:process-recurrent in the schedule is inconsistent with the actual command signature transactions:process-recurring. This will prevent the scheduled command from running.

Suggested change

	$schedule->command('transactions:process-recurrent')->daily();
	$schedule->command('transactions:process-recurring')->daily();

[sourceant]

The condition !is_null($rule->recurrence_ends_at) is redundant here because now() cannot be compared with null in this context, and null < now() would evaluate to false. A more concise and idiomatic PHP way to write this check is to simply treat the Carbon object (or null) as a boolean.

Suggested change

	if ($rule->recurrence_ends_at < now() && !is_null($rule->recurrence_ends_at)) {
	if ($rule->recurrence_ends_at && $rule->recurrence_ends_at < now()) {

[sourceant]

New transactions created by the recurring service should also be marked as synced, similar to how initial transactions are handled in the store method. This ensures consistency in the transaction's sync status.

Suggested change

	$newTransaction->save();
	$newTransaction->save();
	$newTransaction->markAsSynced();

[sourceant]

These lines are commented out, indicating they are no longer needed due to the RecurrentTransactionJob refactoring. They should be removed for clarity.

[sourceant]

The job dispatch call is inconsistent with the store method (it uses positional arguments instead of named id:). Also, the commented-out lines should be removed for cleaner code.

Suggested change

	$recurring_transaction->id) -> delay($next_date);
	// id: (int) $recurring_transaction->id,
	// recurrence_period: $recurring_transaction->recurrence_period,
	// recurrence_interval: $recurring_transaction->recurrence_interval)->delay($next_date);
	}
	RecurrentTransactionJob::dispatch(
	id: (int) $recurring_transaction->id
	)->delay($next_date);

[kofimokome]

@iMercyvlogs your changes do not pass the tests. Please ensure the tests succeed

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

For consistency and improved readability, using the static ::dispatch method directly on the RecurrentTransactionJob class is generally preferred over the global dispatch() helper, especially when the job is already imported via its namespace. This makes it clearer which job is being dispatched.

Suggested change

	dispatch(new \App\Jobs\RecurrentTransactionJob($rule->id))->delay($nextRunDate);
	\App\Jobs\RecurrentTransactionJob::dispatch($rule->id)->delay($nextRunDate);

[sourceant]

Similar to the generateNextTransaction method, using the static ::dispatch method on the RecurrentTransactionJob class provides better readability and explicit linkage to the job class.

Suggested change

	dispatch(new \App\Jobs\RecurrentTransactionJob($rule->id));
	\App\Jobs\RecurrentTransactionJob::dispatch($rule->id);

[sourceant]

The doc blocks for $signature and $description are standard Laravel practice but could be more descriptive than just /** */. While not strictly necessary, providing a brief explanation for each property can enhance clarity for future maintainers.

Suggested change

	*
	*/
	* The name and signature of the console command.
	* @var string

[sourceant]

Similar to the $signature doc block, adding @var string for $description property improves type clarity.

Suggested change

	*
	*/
	* The console command description.
	* @var string

[sourceant]

When dispatching a job, it's generally good practice to explicitly define parameters with their names to improve readability and prevent issues if the constructor order changes. The removal of recurrence_period and recurrence_interval is a positive change aligning with the job's new responsibility, but the id could be named more clearly as ruleId to match the RecurrentTransactionJob constructor.

Suggested change

	$next_date = get_next_transaction_schedule_date($recurring_transaction);
	RecurrentTransactionJob::dispatch(
	id: (int) $recurring_transaction->id,
	recurrence_period: $recurring_transaction->recurrence_period,
	recurrence_interval: $recurring_transaction->recurrence_interval,
	)->delay($next_date);
	}
	RecurrentTransactionJob::dispatch(ruleId: (int) $recurring_transaction->id)->delay($next_date);

[sourceant]

Similar to the store method, explicitly naming the id parameter as ruleId when dispatching the job enhances clarity and consistency with the job's constructor signature.

Suggested change

	RecurrentTransactionJob::dispatch(
	id: (int) $recurring_transaction->id,
	recurrence_period: $recurring_transaction->recurrence_period,
	recurrence_interval: $recurring_transaction->recurrence_interval)->delay($next_date);
	id: (int) $recurring_transaction->id
	)->delay($next_date);
	}
	RecurrentTransactionJob::dispatch(ruleId: (int) $recurring_transaction->id)->delay($next_date);

[sourceant]

Relying on a global function like get_next_transaction_schedule_date can make testing harder and reduces code discoverability. It would be better to encapsulate this logic within a dedicated helper class, a trait, or even a static method within a relevant class, then inject or use that dependency. For example, a ScheduleCalculator service or a static method RecurringTransactionRule::calculateNextRunDate($rule). For now, assuming it's a known helper, but for future scaling, consider proper encapsulation.

Suggested change

	// Use a function that exists in your project to calculate the next date.
	// I have assumed it's called get_next_transaction_schedule_date
	return get_next_transaction_schedule_date($rule);
	// Consider encapsulating `get_next_transaction_schedule_date` within a dedicated class or service.
	return get_next_transaction_schedule_date($rule);

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

The doc block for $signature is incomplete. It should typically describe the purpose of the signature itself, rather than being left empty or with only comments about its existence. This is a minor style improvement for consistency.

Suggested change

	protected $signature = 'transactions:process-recurring';
	protected $signature = 'transactions:process-recurring';

[sourceant]

Similar to the $signature doc block, the doc block for $description is incomplete. While the description property itself contains the information, the doc block should conform to standards for consistency and clarity.

Suggested change

	protected $description = 'Process and schedule recurring transactions that are due';
	protected $description = 'Process and schedule recurring transactions that are due';

[sourceant]

The docker-compose.yml file was missing a newline at the end of the file, which can sometimes cause issues with certain tools or version control systems. Adding a newline ensures the file is properly formatted.

Suggested change

	sailmysql:
	driver: local

[sourceant]

Catching a generic \Exception is generally discouraged as it can mask unexpected errors. It's better to catch more specific exception types relevant to the processAllDueTransactions method. If a specific exception cannot be identified, it's often more appropriate to log the full exception details including stack trace for debugging, and rethrow or handle gracefully. Given the nature of a console command, a full log is typically preferred over just the message.

Suggested change

	} catch (\Exception $e) {
	$this->error('Error processing recurring transactions: ' . $e->getMessage());
	}
	} catch (\Throwable $e) {
	$this->error('Error processing recurring transactions: ' . $e->getMessage());
	logger()->error('Recurring transaction command failed', ['exception' => $e]);
	}

[sourceant]

The expiration check $rule->recurrence_ends_at < now() uses now() which includes time. For recurrence periods (daily, weekly, monthly, yearly), it's often more robust and less prone to edge-case issues to compare dates at the start of the day. Using Carbon::today() or ensuring the recurrence_ends_at is stored as a date-only value and compared accordingly can prevent transactions from being skipped if now() crosses the boundary mid-day on the end date.

Suggested change

	if ($rule->recurrence_ends_at && $rule->recurrence_ends_at < now() ) {
	logger()->info('Rule ' . $ruleId . ' has expired. Skipping.');
	return;
	if ($rule->recurrence_ends_at && $rule->recurrence_ends_at->startOfDay()->lessThan(now()->startOfDay()) ) {

[sourceant]

This processAllDueTransactions method has a critical logical flaw. It currently dispatches RecurrentTransactionJob without delay for all non-expired recurring rules. This means every time the transactions:process-recurring command runs (e.g., daily), it will immediately create a new transaction for every active recurring rule, regardless of its recurrence_period and whether it's actually due. This will lead to many incorrect, premature transactions.

To fix this, the method must first determine if a rule is actually due to run now before dispatching a job for immediate execution. This requires querying or filtering rules based on their next_occurrence_at (or similar logic derived from get_next_transaction_schedule_date relative to now()). Without a next_occurrence_at field on the RecurringTransactionRule model, the system lacks proper state management for when a rule was last processed or is next due. For this suggestion, I'm assuming get_next_transaction_schedule_date can correctly calculate if a transaction should have already occurred or is occurring now.

Suggested change

	$rules = RecurringTransactionRule::where('recurrence_ends_at', '>=', now())
	->orWhereNull('recurrence_ends_at')
	->get();
	// 2. For each rule, dispatch a job to handle it.
	foreach ($rules as $rule) {
	// Dispatch a job for each rule. This is better than doing all the
	// work at once, especially for many transactions.
	dispatch(new \App\Jobs\RecurrentTransactionJob($rule->id));
	}
	public function processAllDueTransactions(): void
	{
	// 1. Find all rules that are due to be run.
	// This query should be refined to only select rules whose next occurrence date
	// (as determined by get_next_transaction_schedule_date) is on or before today.
	// This requires an assumption about get_next_transaction_schedule_date's behavior
	// or a 'next_occurrence_at' field on the rule.
	$rules = RecurringTransactionRule::with('transaction')
	->where(function ($query) {
	$query->where('recurrence_ends_at', '>=', now()->startOfDay())
	->orWhereNull('recurrence_ends_at');
	})
	->get();
	// 2. For each rule, dispatch a job to handle it *only if it is actually due*.
	foreach ($rules as $rule) {
	// Calculate the specific next date this transaction *should* run.
	$nextExpectedRunDate = get_next_transaction_schedule_date($rule);
	// Dispatch a job only if the transaction is due today or in the past,
	// ensuring we don't process future transactions prematurely.
	if ($nextExpectedRunDate && $nextExpectedRunDate->lessThanOrEqualTo(now())) {
	dispatch(new \App\Jobs\[RecurrentTransactionJob($rule->id));
	// Ideally, here you would also update a `last_processed_at` or `next_occurrence_at` on the rule
	// to prevent repeated processing within the same due period.
	}
	}
	}

[sourceant]

The current replication process for recurring transactions doesn't reset the client_id field. Since client_id is described as a "Unique identifier for your local client", it implies uniqueness. Replicating it could lead to duplicate client_id values, which might cause issues if a client-side system expects these IDs to be unique across all transactions, or if a unique constraint exists in the database. A new recurring transaction should likely generate its own client_id if one is needed, or the client_id should be explicitly set to null to avoid duplication.

Suggested change

	$newTransaction->updated_at = now();
	// Make a copy of the original transaction.
	$newTransaction = $originalTransaction->replicate(["client_id"]);
	// Update the copy with today's date and time.
	$newTransaction->created_at = now();
	$newTransaction->updated_at = now();
	$newTransaction->client_id = null; // Ensure new recurring transactions get a fresh client ID if needed, or it's re-assigned later.

[sourceant]

Review complete. See the overview comment for a summary.

[sourceant]

For daily scheduled tasks, it's often best practice to specify an exact time rather than just daily() to ensure it runs during off-peak hours and at a predictable time. This helps avoid potential overlaps with other system processes or heavy user traffic.

Suggested change

	$schedule->command('transactions:process-recurring')->daily();
	$schedule->command('transactions:process-recurring')->dailyAt('03:00'); // Example: Run daily at 3:00 AM

[sourceant]

It's a best practice for text files, including YAML configuration files, to end with a newline character. This ensures proper parsing by some tools and prevents issues when concatenating files.

Suggested change

	sailmysql:
	driver: local

[sourceant]

It's generally recommended to catch \Throwable instead of \Exception to also catch Error exceptions, which are unrecoverable application errors. Additionally, logging the full exception stack trace is crucial for effective debugging, aligning with existing logging patterns in the TransactionController and RecurrentTransactionJob.

Suggested change

	} catch (\Exception $e) {
	$this->error('Error processing recurring transactions: '.$e->getMessage());
	}
	} catch (\Throwable $e) {
	$this->error('Error processing recurring transactions: '.$e->getMessage());
	logger()->error('Error processing recurring transactions', ['exception' => $e]);
	}

[sourceant]

The current processAllDueTransactions method dispatches a job for all active recurring rules daily, regardless of their actual due date. This creates inefficiency and redundancy, as individual jobs (dispatched by generateNextTransaction) are already self-scheduling their next occurrence.

To accurately 'process and schedule recurring transactions that are due' (as per the console command description), the RecurringTransactionRule model must include a next_scheduled_at (or last_processed_at) timestamp column. This timestamp would be updated by generateNextTransaction after it successfully schedules the next recurrence.

The processAllDueTransactions method should then query for rules where next_scheduled_at is in the past or null (for rules that have never been scheduled). This ensures only genuinely due or missed rules are processed by the daily command, avoiding duplicate job dispatching and premature transaction generation.

Below is a conceptual code suggestion assuming the next_scheduled_at column exists. Implementing this change requires a database migration to add next_scheduled_at to the recurring_transaction_rules table.

Suggested change

	// 1. Find all rules that are due to be run.
	$rules = RecurringTransactionRule::where('recurrence_ends_at', '>=', now())
	->orWhereNull('recurrence_ends_at')
	->get();
	$rules = RecurringTransactionRule::where(function ($query) {
	$query->where('next_scheduled_at', '<=', now())
	->orWhereNull('next_scheduled_at'); // For rules never scheduled before or missed
	})
	->where(function ($query) { // Ensure rules haven't expired
	$query->where('recurrence_ends_at', '>=', now())
	->orWhereNull('recurrence_ends_at');
	})
	->get();

[kofimokome]

@iMercyvlogs the recurrence_period and recurrence_interval were sent to the job to determine if the rule is still the same by the time the job runs. It could be possible for a user to change the recurrence period or frequency by the time the next job runs. That is why we check if the recurrence period and interval are the same before running the job.

Please test again and ensure the next job (that was already scheduled) does not run if the interval and period changes and let me know if it your new changes handle this scenario.

[iMercyvlogs]

It doesn't

[iMercyvlogs]

The update() method in the controller takes the recurrence_period, recurrence_interval, recurrence_ends_at information and checks to see if the recurrence is still valid... if it is, that's when the RecurrenceTransactionJob is triggered with the target rule's ID.