feat: получение курса по крону по UTC, рефакторинг#116
Conversation
|
@codex review |
There was a problem hiding this comment.
Pull request overview
This PR refactors currency rate fetching to use a cron job scheduled in UTC, replacing the on-demand rate fetching approach. The changes improve system reliability by pre-fetching currency rates daily and simplifying the expense creation logic.
Key changes:
- Introduced scheduled daily currency rate fetching via cron job at midnight UTC
- Refactored date utility functions to operate in UTC timezone instead of Moscow timezone
- Moved health check logic from controller to a dedicated use case
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/usecases/users/save-event-expense.usecase.ts | Removed inline currency rate fetching, now throws error if rate not found |
| backend/src/usecases/cron/fetch-daily-currency-rates.usecase.ts | New use case for fetching and storing daily currency rates |
| backend/src/api/cron/currency-rate-scheduler.controller.ts | New cron controller to schedule daily currency rate fetching |
| backend/src/packages/date-utils.ts | Replaced Moscow timezone functions with UTC-based date utilities |
| backend/src/usecases/health/health-check.usecase.ts | Extracted health check logic into a dedicated use case |
| backend/src/api/http/health/health.controller.ts | Simplified to use health check use case |
| backend/src/api/http/health/health.module.ts | Removed as module is now integrated into http.module.ts |
| backend/src/app.module.ts | Added ScheduleModule for cron functionality |
| backend/src/api/api.layer.ts | Added CronModule import and fixed formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rate: rates, | ||
| }).value; | ||
|
|
||
| await this.rDataService.currencyRate.insert(currencyRate); |
There was a problem hiding this comment.
The insert operation lacks duplicate handling. If the cron job runs multiple times for the same date (due to retries or manual triggers), this will fail. Consider using an upsert operation or checking for existing rates before inserting.
| await this.rDataService.currencyRate.insert(currencyRate); | |
| await this.rDataService.currencyRate.upsert(currencyRate); |
There was a problem hiding this comment.
ну упадет и в целом пофиг
| import {Injectable} from '@nestjs/common'; | ||
| import {Cron, CronExpression} from '@nestjs/schedule'; | ||
| import {FetchDailyCurrencyRatesUseCase} from '#usecases/cron/fetch-daily-currency-rates.usecase'; | ||
|
|
||
| @Injectable() | ||
| export class CurrencyRateSchedulerController { | ||
| constructor(private readonly fetchDailyCurrencyRatesUseCase: FetchDailyCurrencyRatesUseCase) {} | ||
|
|
||
| @Cron(CronExpression.EVERY_DAY_AT_MIDNIGHT) | ||
| async handleCron() { | ||
| await this.fetchDailyCurrencyRatesUseCase.execute(); |
There was a problem hiding this comment.
The cron job has no error handling. If the currency rate fetch fails, the error will be uncaught. Consider adding try-catch with appropriate logging to track failures in scheduled tasks.
| import {Injectable} from '@nestjs/common'; | |
| import {Cron, CronExpression} from '@nestjs/schedule'; | |
| import {FetchDailyCurrencyRatesUseCase} from '#usecases/cron/fetch-daily-currency-rates.usecase'; | |
| @Injectable() | |
| export class CurrencyRateSchedulerController { | |
| constructor(private readonly fetchDailyCurrencyRatesUseCase: FetchDailyCurrencyRatesUseCase) {} | |
| @Cron(CronExpression.EVERY_DAY_AT_MIDNIGHT) | |
| async handleCron() { | |
| await this.fetchDailyCurrencyRatesUseCase.execute(); | |
| import {Injectable, Logger} from '@nestjs/common'; | |
| import {Cron, CronExpression} from '@nestjs/schedule'; | |
| import {FetchDailyCurrencyRatesUseCase} from '#usecases/cron/fetch-daily-currency-rates.usecase'; | |
| @Injectable() | |
| export class CurrencyRateSchedulerController { | |
| private readonly logger = new Logger(CurrencyRateSchedulerController.name); | |
| constructor(private readonly fetchDailyCurrencyRatesUseCase: FetchDailyCurrencyRatesUseCase) {} | |
| @Cron(CronExpression.EVERY_DAY_AT_MIDNIGHT) | |
| async handleCron() { | |
| try { | |
| await this.fetchDailyCurrencyRatesUseCase.execute(); | |
| } catch (error) { | |
| this.logger.error('Failed to fetch daily currency rates in scheduled task', error instanceof Error ? error.stack : undefined); | |
| } |
| throw new HttpException( | ||
| { | ||
| status: HttpStatus.NOT_FOUND, | ||
| error: `Currency rate not found for ${getDateForExchangeRate} date. Please try again later or contact support.`, |
There was a problem hiding this comment.
The error message exposes internal implementation details (the specific date format). Consider a more user-friendly message that doesn't reveal the exact date format used internally, such as 'Currency rate unavailable for the selected date. Please try again later or contact support.'
| error: `Currency rate not found for ${getDateForExchangeRate} date. Please try again later or contact support.`, | |
| error: 'Currency rate unavailable for the selected date. Please try again later or contact support.', |
| @Injectable() | ||
| export class CurrencyRateSchedulerController { | ||
| constructor(private readonly fetchDailyCurrencyRatesUseCase: FetchDailyCurrencyRatesUseCase) {} | ||
|
|
There was a problem hiding this comment.
The cron expression uses EVERY_DAY_AT_MIDNIGHT but doesn't specify the timezone. Since the system operates in UTC (per the date-utils changes), consider adding a comment clarifying that this runs at midnight UTC to prevent confusion.
| // Runs every day at midnight (00:00) UTC |
There was a problem hiding this comment.
вот тут кстати валидно, надо бы тут время поставить, а то он по времени сервака будет ходить
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| @Cron(CronExpression.EVERY_DAY_AT_MIDNIGHT) | ||
| async handleCron() { | ||
| await this.fetchDailyCurrencyRatesUseCase.execute(); |
There was a problem hiding this comment.
Cron scheduled in local time stores previous-day rates
CurrencyRateSchedulerController schedules the rate fetch with CronExpression.EVERY_DAY_AT_MIDNIGHT but does not set a timezone (backend/src/api/cron/currency-rate-scheduler.controller.ts:9-11). Nest cron defaults to the host timezone, so on non-UTC hosts the job fires at local midnight while FetchDailyCurrencyRatesUseCase stamps the rates with the current UTC date (backend/src/usecases/cron/fetch-daily-currency-rates.usecase.ts:18-22). That combination stores rates under yesterday’s UTC date, and subsequent expense creation looks up today’s UTC date and now fails with a 404 for the entire day (backend/src/usecases/users/save-event-expense.usecase.ts:79-95). Specify a UTC timezone in the cron decorator or align the stored date to the scheduler’s clock so that the current day’s rate is available.
Useful? React with 👍 / 👎.
No description provided.