Conversation
WalkthroughUpdates CurrencyService to call apilayer API with timeouts, add structured error handling and logging, validate responses, and provide fallback defaults (e.g., 83.00 for USDINR). Both single-rate and all-rates fetch methods were revised to parse quotes, handle missing API keys, and standardize fallback returns. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant S as CurrencyService
participant A as apilayer API
participant L as Logger
rect rgb(240,245,255)
note right of S: fetchExchangeRateInINR (USD→INR)
C->>S: fetchExchangeRateInINR()
alt API key missing
S->>L: warn("Missing API key")
S-->>C: defaultRate(83.00)
else API key present
S->>A: GET /currency_data/live?source=USD¤cies=INR
alt Network/API error
S->>L: error(details)
S-->>C: defaultRate(83.00)
else Response OK
S->>S: validate structure (quotes.USDINR)
alt Valid
S-->>C: quotes.USDINR
else Invalid
S->>L: error("Invalid response structure")
S-->>C: defaultRate(83.00)
end
end
end
end
rect rgb(240,255,240)
note right of S: fetchAllExchangeRateInINR (USD source)
C->>S: fetchAllExchangeRateInINR()
alt API key missing
S->>L: warn("Missing API key")
S-->>C: defaultQuotes({USDINR: 83.00, ...})
else API key present
S->>A: GET /currency_data/live?source=USD
alt Error
S->>L: error(details)
S-->>C: defaultQuotes({USDINR: 83.00, ...})
else OK
S->>S: validate quotes map
alt Valid
S-->>C: quotes
else Invalid
S->>L: error("Invalid response structure")
S-->>C: defaultQuotes({USDINR: 83.00, ...})
end
end
end
end
Best Practices Flags
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Employee portal
|
||||||||||||||||||||||||||||
| Project |
Employee portal
|
| Branch Review |
refs/pull/3776/merge
|
| Run status |
|
| Run duration | 00m 22s |
| Commit |
|
| Committer | Pankaj Kandpal |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
4
|
| View all changes introduced in this branch ↗︎ | |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Modules/Invoice/Services/CurrencyService.php (1)
1-123: Fix code formatting issues flagged by PHP CS Fixer.The CI pipeline detected multiple formatting violations that need to be addressed. Run the following command to automatically fix them:
php-cs-fixer fix Modules/Invoice/Services/CurrencyService.phpOr review and apply fixes manually for:
ordered_class_elementstrailing_comma_in_multilinenot_operator_with_successor_spaceblank_line_before_statementordered_importsbinary_operator_spacesBased on pipeline failures.
🧹 Nitpick comments (1)
Modules/Invoice/Services/CurrencyService.php (1)
55-84: Extract duplicate default rate retrieval and response validation.The error handling approach is solid, but there are opportunities to reduce duplication:
- The default rate retrieval
round(config('services.currencylayer.default_rate', 83.00), 2)appears multiple times across both fetch methods.- Response structure validation could be extracted into a helper method.
- Generic
\Exceptionat Line 71 is immediately caught by the\Throwablehandler—consider using a more specific exception type or removing the intermediate throw.Consider extracting common logic:
private function getDefaultRate(): float { return round(config('services.currencylayer.default_rate', 83.00), 2); } private function validateResponse(array $data, array $requiredKeys): void { foreach ($requiredKeys as $key) { if (empty($data[$key])) { throw new \RuntimeException("Missing required key: {$key}"); } } }Then simplify the fetch methods:
-if (!config('services.currencylayer.access_key')) { - Log::warning('CurrencyLayer API key missing. Using default rate.'); - return round(config('services.currencylayer.default_rate', 83.00), 2); +if (!config('services.currencylayer.access_key')) { + Log::warning('CurrencyLayer API key missing. Using default rate.'); + return $this->getDefaultRate(); } try { // ... API call ... $data = json_decode($response->getBody()->getContents(), true); - if (empty($data) || empty($data['quotes']['USDINR'])) { - throw new \Exception('Invalid API response structure.'); - } + $this->validateResponse($data, ['quotes']); + if (empty($data['quotes']['USDINR'])) { + throw new \RuntimeException('Invalid API response: USDINR rate missing.'); + } return round($data['quotes']['USDINR'], 2); } catch (ConnectException $e) { // ... logging ... } catch (RequestException $e) { // ... logging ... } catch (\Throwable $e) { Log::error('Unexpected error fetching exchange rate: ' . $e->getMessage()); } -return round(config('services.currencylayer.default_rate', 83.00), 2); +return $this->getDefaultRate();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Modules/Invoice/Services/CurrencyService.php(4 hunks)
🧰 Additional context used
🪛 GitHub Actions: Coding Standards
Modules/Invoice/Services/CurrencyService.php
[error] 1-1: PHP CS Fixer failed on dry-run. Detectable issues in CurrencyService.php: ordered_class_elements, trailing_comma_in_multiline, not_operator_with_successor_space, blank_line_before_statement, ordered_imports, binary_operator_spaces. Apply fixes (e.g., run 'php-cs-fixer fix' or follow the diff) to resolve.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: portal
- GitHub Check: portal
🔇 Additional comments (3)
Modules/Invoice/Services/CurrencyService.php (3)
6-9: LGTM!The new imports are necessary for the enhanced error handling and logging functionality.
23-32: Good addition of timeout configurations.The connection and request timeouts (5s and 10s respectively) are sensible defaults that prevent indefinite blocking on external API calls.
21-21: Visibility change is non-breaking — no external calls toCurrencyService::setClient;CalendarMeetingServicedefines its ownsetClient.
| if (!config('services.currencylayer.access_key')) { | ||
| Log::warning('CurrencyLayer API key missing. Using default rate.'); | ||
| return [ | ||
| 'USDINR' => round(config('services.currencylayer.default_rate', 83.00), 2) | ||
| ]; | ||
| } | ||
|
|
||
| $response = $this->client->get('currency_data/live', [ | ||
| 'query' => [ | ||
| 'access_key' => config('services.currencylayer.access_key'), | ||
| ], | ||
| ]); | ||
|
|
||
| $data = json_decode($response->getBody()->getContents(), true); | ||
| try { | ||
| $response = $this->client->get('currency_data/live', [ | ||
| 'query' => [ | ||
| 'source' => 'USD', | ||
| ], | ||
| ]); | ||
|
|
||
| $data = json_decode($response->getBody()->getContents(), true); | ||
|
|
||
| if (empty($data) || empty($data['quotes'])) { | ||
| throw new \Exception('Invalid API response structure.'); | ||
| } | ||
|
|
||
| return $data['quotes']; | ||
| } catch (ConnectException $e) { | ||
| Log::error('Currency API connection failed: ' . $e->getMessage()); | ||
| } catch (RequestException $e) { | ||
| Log::error('Currency API request error: ' . $e->getMessage()); | ||
| } catch (\Throwable $e) { | ||
| Log::error('Unexpected error fetching all exchange rates: ' . $e->getMessage()); | ||
| } | ||
|
|
||
| return $data['quotes']; | ||
| // fallback if API fails | ||
| return [ | ||
| 'USDINR' => round(config('services.currencylayer.default_rate', 83.00), 2) | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Eliminate duplicate error handling across both fetch methods.
The fetchAllExchangeRateInINR method duplicates the entire error-handling pattern from fetchExchangeRateInINR. This violates the DRY principle and makes maintenance harder.
Extract the common API call logic into a reusable method:
private function makeApiCall(string $endpoint, array $queryParams, callable $processor)
{
if (!config('services.currencylayer.access_key')) {
Log::warning('CurrencyLayer API key missing. Using default rate.');
return $this->getDefaultRate();
}
try {
$response = $this->client->get($endpoint, ['query' => $queryParams]);
$data = json_decode($response->getBody()->getContents(), true);
if (empty($data)) {
throw new \RuntimeException('Empty API response.');
}
return $processor($data);
} catch (ConnectException $e) {
Log::error('Currency API connection failed: ' . $e->getMessage());
} catch (RequestException $e) {
Log::error('Currency API request error: ' . $e->getMessage());
} catch (\Throwable $e) {
Log::error('Unexpected error in API call: ' . $e->getMessage());
}
return null; // or throw exception for caller to handle fallback
}Then refactor both methods to use it:
private function fetchExchangeRateInINR()
{
$result = $this->makeApiCall(
'currency_data/live',
['currencies' => 'INR', 'source' => 'USD'],
function ($data) {
if (empty($data['quotes']['USDINR'])) {
throw new \RuntimeException('USDINR rate missing from response.');
}
return round($data['quotes']['USDINR'], 2);
}
);
return $result ?? $this->getDefaultRate();
}
private function fetchAllExchangeRateInINR()
{
$result = $this->makeApiCall(
'currency_data/live',
['source' => 'USD'],
function ($data) {
if (empty($data['quotes'])) {
throw new \RuntimeException('Quotes missing from response.');
}
return $data['quotes'];
}
);
return $result ?? ['USDINR' => $this->getDefaultRate()];
}🤖 Prompt for AI Agents
In Modules/Invoice/Services/CurrencyService.php around lines 89 to 122, the
error-handling and API-call logic in fetchAllExchangeRateInINR duplicates
fetchExchangeRateInINR; extract common behavior into a private helper (e.g.,
makeApiCall) that handles api-key check, performs the Guzzle GET with given
endpoint and query, decodes payload, validates basic structure, logs
ConnectException/RequestException/Throwable uniformly, and returns decoded data
or null on failure; add a getDefaultRate helper that returns the rounded default
USDINR from config; then refactor both fetchExchangeRateInINR and
fetchAllExchangeRateInINR to call makeApiCall with a small processor callback
that extracts/validates the specific fields (USDINR or quotes) and return
processor result or fallback to getDefaultRate (or array with USDINR) when null.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
Modules/Invoice/Services/CurrencyService.php (2)
61-84: DRY violation: error handling duplicated across methods.The error handling pattern in this method is nearly identical to
fetchAllExchangeRateInINR(lines 97-121). This duplication was already flagged in a previous review, and the suggestion to extract common API call logic into a reusable helper method remains valid.Please refer to the previous review comment suggesting extraction of common API call logic into a
makeApiCallhelper method. This would eliminate the duplication and make the code more maintainable.
89-95: Same issues as fetchExchangeRateInINR.This segment has the same default rate duplication issue flagged in lines 55-59. After extracting
getDefaultRate(), update this section to use it as well.
🧹 Nitpick comments (1)
Modules/Invoice/Services/CurrencyService.php (1)
72-72: Consider using a custom exception type.Throwing generic
\Exceptionmakes it harder to distinguish between different failure scenarios. Consider creating a custom exception class for currency service errors.Create a custom exception class:
namespace Modules\Invoice\Exceptions; class CurrencyServiceException extends \Exception { }Then use it instead of generic
\Exception:- throw new \Exception('Invalid API response structure.'); + throw new CurrencyServiceException('Invalid API response structure.');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Modules/Invoice/Services/CurrencyService.php(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: portal
- GitHub Check: portal
🔇 Additional comments (3)
Modules/Invoice/Services/CurrencyService.php (3)
6-9: LGTM!The imports are appropriate for the error handling implementation and are used correctly throughout the file.
23-32: Good addition of timeouts to prevent hanging requests.The timeout configuration (10s request, 5s connection) prevents the application from blocking indefinitely when the API is slow or unresponsive. These values are reasonable for external API calls.
106-108: Better validation approach than fetchExchangeRateInINR.This validation correctly checks
$data['quotes']without accessing nested keys directly, avoiding the PHP notice issue present on Line 71. However, consider using a custom exception type here as well (see comment on Line 72).
| if (! config('services.currencylayer.access_key')) { | ||
| return round(config('services.currencylayer.default_rate'), 2); | ||
| } | ||
| Log::warning('CurrencyLayer API key missing. Using default rate.'); | ||
|
|
||
| $response = $this->client->get('currency_data/live', [ | ||
| 'query' => [ | ||
| 'access_key' => config('services.currencylayer.access_key'), | ||
| 'currencies' => 'INR', | ||
| ], | ||
| ]); | ||
| return round(config('services.currencylayer.default_rate', 83.00), 2); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Extract default rate to a helper method.
The default rate calculation round(config('services.currencylayer.default_rate', 83.00), 2) is duplicated across lines 58, 84, 93, and 120. This violates the DRY principle.
Extract this to a private helper method:
+ private function getDefaultRate()
+ {
+ return round(config('services.currencylayer.default_rate', 83.00), 2);
+ }
+
private function fetchExchangeRateInINR()
{
if (! config('services.currencylayer.access_key')) {
Log::warning('CurrencyLayer API key missing. Using default rate.');
- return round(config('services.currencylayer.default_rate', 83.00), 2);
+ return $this->getDefaultRate();
}Then replace all other occurrences (lines 84, 93, 120) with $this->getDefaultRate().
🤖 Prompt for AI Agents
In Modules/Invoice/Services/CurrencyService.php around lines 55-59 (and also
replace duplicates at lines 84, 93, and 120), extract the duplicated default
rate expression round(config('services.currencylayer.default_rate', 83.00), 2)
into a private helper method named getDefaultRate() that returns that rounded
value, then replace each occurrence with $this->getDefaultRate(); ensure the
helper is private, placed in the class, and all four sites use that method so
the logic is centralized.
| if (empty($data) || empty($data['quotes']['USDINR'])) { | ||
| throw new \Exception('Invalid API response structure.'); | ||
| } |
There was a problem hiding this comment.
Fix potential PHP notice in validation logic.
The expression empty($data['quotes']['USDINR']) on Line 71 will trigger a PHP notice if $data['quotes'] doesn't exist. The validation should check the structure step-by-step.
Apply this diff to fix the validation:
- if (empty($data) || empty($data['quotes']['USDINR'])) {
+ if (empty($data) || !isset($data['quotes']['USDINR']) || empty($data['quotes']['USDINR'])) {
throw new \Exception('Invalid API response structure.');
}Alternatively, validate in stages:
- if (empty($data) || empty($data['quotes']['USDINR'])) {
+ if (empty($data)) {
+ throw new \Exception('Empty API response.');
+ }
+
+ if (!isset($data['quotes']['USDINR']) || empty($data['quotes']['USDINR'])) {
throw new \Exception('Invalid API response structure.');
}🤖 Prompt for AI Agents
In Modules/Invoice/Services/CurrencyService.php around lines 71 to 73, the
current validation uses empty($data['quotes']['USDINR']) which can raise a PHP
notice if $data['quotes'] is undefined; update the check to validate the
structure in stages — first ensure $data is not empty and is an array, then
ensure $data['quotes'] exists and is an array (or use isset) before checking
$data['quotes']['USDINR'], and throw the same exception if any of those checks
fail.
1 participant
Due to the refactoring of currenlylayer API, the regular invoice workflows in portal are failing. As the part of a temporary fix, I have enabled sufficient try catch blogs, for better logging and handling of errors received in API. Even when the API fails, it won't crash the entire page
Summary by CodeRabbit
Bug Fixes
Chores