Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions app/Console/Commands/ProcessRecurringTransactions.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
<?php

namespace App\Console\Commands;

use App\Services\RecurringTransactionService;
use Illuminate\Console\Command;

class ProcessRecurringTransactions extends Command
{
/**
* The name and signature of the console command.
*/
protected $signature = 'transactions:process-recurring';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';


/**
* The console command description.
*/
protected $description = 'Process and schedule recurring transactions that are due';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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';


/**
* Execute the console command.
*/
public function handle(RecurringTransactionService $service): void
{
$this->info('Starting to process recurring transactions ...');

try {
// call the service to process all transactions that are due
$service->processAllDueTransactions();
$this->info('Recurring transactions processing completed.');
} catch (\Exception $e) {
$this->error('Error processing recurring transactions: '.$e->getMessage());
}
Comment on lines +31 to +33
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);
}

}
}
1 change: 1 addition & 0 deletions app/Console/Kernel.php
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ class Kernel extends ConsoleKernel
protected function schedule(Schedule $schedule): void
{
// $schedule->command('inspire')->hourly();
$schedule->command('transactions:process-recurring')->daily();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

}

/**
Expand Down
11 changes: 5 additions & 6 deletions app/Http/Controllers/API/v1/TransactionController.php
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,8 @@ public function store(Request $request): JsonResponse
// schedule next task.
$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,
Comment on lines -288 to -289
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// id: (int) $recurring_transaction->id,
(int) $recurring_transaction->ruleId
)->delay($next_date);
}

Expand Down Expand Up @@ -574,9 +573,9 @@ public function update(Request $request, $id): JsonResponse
// schedule next task.
$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);
// id: (int) $recurring_transaction->id
(int) $recurring_transaction->ruleId
)->delay($next_date);
}
}

Expand Down
65 changes: 12 additions & 53 deletions app/Jobs/RecurrentTransactionJob.php
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

namespace App\Jobs;

use App\Models\RecurringTransactionRule;
use App\Services\RecurringTransactionService;
use Illuminate\Bus\Queueable;
use Illuminate\Contracts\Queue\ShouldQueue;
use Illuminate\Foundation\Bus\Dispatchable;
Expand All @@ -13,69 +13,28 @@ class RecurrentTransactionJob implements ShouldQueue
{
use Dispatchable, InteractsWithQueue, Queueable, SerializesModels;

private int $id;

private string $recurrence_period;

private int $recurrence_interval;
public int $ruleId;

/**
* Create a new job instance.
* The job only needs the ID of the rule to process.
*/
public function __construct(int $id, string $recurrence_period, int $recurrence_interval)
public function __construct(int $ruleId)
{
$this->id = $id;
$this->recurrence_period = $recurrence_period;
$this->recurrence_interval = $recurrence_interval;
$this->ruleId = $ruleId;
}

/**
* Execute the job.
* The job's only purpose is to tell the service to do the work.
*/
public function handle(): void
public function handle(RecurringTransactionService $service): void
{
try {
logger()->info('Handling recurrent transaction');
// 1. Check if the user might have cancelled this recurring transaction
$recurring_transaction = RecurringTransactionRule::find($this->id);
if ($recurring_transaction) {
// 2. Cancel this job if the recurrence period or recurrence interval has changed or has exceeded the end date
if (
($this->recurrence_period == $recurring_transaction->recurrence_period)
&&
($this->recurrence_interval == $recurring_transaction->recurrence_interval)
&&
($recurring_transaction->recurrence_ends_at >= now() || is_null($recurring_transaction->recurrence_ends_at))
) {
logger()->info('Running recurrent transaction with id '.$this->id);
$new_transaction = $recurring_transaction->transaction->replicate();
$new_transaction->created_at = now();
$new_transaction->updated_at = now();
$new_transaction->save();
logger()->info('Handling recurrent transaction for rule ID: '.$this->ruleId);

if ($recurring_transaction->transaction->categories->isNotEmpty()) {
$new_transaction->categories()->sync($recurring_transaction->transaction->categories->pluck('id'));
}

// 3. schedule next transaction
if ($recurring_transaction->recurrence_ends_at > now() || is_null($recurring_transaction->recurrence_ends_at)) {
$next_date = get_next_transaction_schedule_date($recurring_transaction);
logger()->info('Next transaction date for '.$this->id.": {$next_date}");
RecurrentTransactionJob::dispatch(
id: (int) $recurring_transaction->id,
recurrence_period: $recurring_transaction->recurrence_period,
recurrence_interval: $recurring_transaction->recurrence_interval
)->delay($next_date);
}
} else {
logger()->info('Recurrent transaction details for '.$this->id.' have changed, skipping');
}
} else {
logger()->info('Recurrent transaction for '.$this->id.' not found');
}
try {
// Call the service to do all the work.
$service->generateNextTransaction($this->ruleId);
} catch (\Exception $e) {
logger()->error('Exception occurred');
logger()->error($e->getMessage());
logger()->error('Error processing job for rule '.$this->ruleId.': '.$e->getMessage());
}
}
}
104 changes: 104 additions & 0 deletions app/Services/RecurringTransactionService.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
<?php

namespace App\Services;

use App\Models\RecurringTransactionRule;
use App\Models\Transaction;
use Carbon\Carbon;

class RecurringTransactionService
{
/**
* This method is called by the job. It finds a specific recurring rule
* and processes it.
*/
public function generateNextTransaction(int $ruleId): void
{
// 1. Find the specific rule in the database by its ID.
// It's linked to the original transaction.
$rule = RecurringTransactionRule::with('transaction')->find($ruleId);

// 2. If the rule doesn't exist, we stop here.
if (! $rule) {
logger()->info('Recurring transaction rule '.$ruleId.' not found.');

return;
}

// 3. Make sure the rule is still active and hasn't expired.
if ($rule->recurrence_ends_at && $rule->recurrence_ends_at < now()) {
logger()->info('Rule '.$ruleId.' has expired. Skipping.');

return;
}

try {
// 4. Create a brand new transaction based on the rule.
$this->createTransactionFromRule($rule);

// 5. Calculate the next time this transaction should happen.
$nextRunDate = $this->calculateNextRunDate($rule);

// 6. Tell Laravel to run the job again at that future date.
dispatch(new \App\Jobs\RecurrentTransactionJob($rule->id))->delay($nextRunDate);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

logger()->info('Scheduled next transaction for '.$rule->id.' at '.$nextRunDate);

} catch (\Exception $e) {
logger()->error('Error processing rule '.$ruleId.': '.$e->getMessage());
}
}

/**
* This method is called by the command. It finds ALL rules that are due
* and dispatches a separate job for each one.
*/
public function processAllDueTransactions(): void
{
// 1. Find all rules that are due to be run.
$rules = RecurringTransactionRule::where('recurrence_ends_at', '>=', now())
->orWhereNull('recurrence_ends_at')
->get();
Comment on lines +57 to +60
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();


// 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));
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

}
Comment on lines +58 to +67
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
}
}
}

}

/**
* Creates a new transaction record from a recurring rule.
*/
private function createTransactionFromRule(RecurringTransactionRule $rule): Transaction
{
// Get the original transaction data.
$originalTransaction = $rule->transaction;

// Make a copy of the original transaction.
$newTransaction = $originalTransaction->replicate();

// Update the copy with today's date and time.
$newTransaction->created_at = now();
$newTransaction->updated_at = now();

Comment on lines +83 to +84
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// Save the new copy to the database and synced.
$newTransaction->save();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

$newTransaction->markAsSynced();

if ($originalTransaction->categories->isNotEmpty()) {
$newTransaction->categories()->sync($originalTransaction->categories->pluck('id'));
}

return $newTransaction;
}

/**
* Calculates the date of the next transaction.
*/
private function calculateNextRunDate(RecurringTransactionRule $rule): Carbon
{
// Use a function that exists in your project to calculate the next date.
return get_next_transaction_schedule_date($rule);
}
}
12 changes: 5 additions & 7 deletions docker-compose.yml
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
services:
app:
build:
context: .
dockerfile: Dockerfile
args:
HOST_UID: '${HOST_UID}'
HOST_GID: '${HOST_GID}'
image: shinsenter/laravel:php8.2
environment:
APP_UID: '${APP_UID:-1001}'
APP_GID: '${APP_GID:-1001}'
ports:
- '${APP_PORT:-8000}:80'
tty: true
Expand Down Expand Up @@ -61,4 +59,4 @@ networks:
driver: bridge
volumes:
sailmysql:
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

driver: local
driver: local