Repository navigation
به‌روزرسانی درایور باجت - #367
saeeditbaz wants to merge 2 commits into
Conversation
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #367 +/- ##
=========================================
Coverage ? 80.79%
Complexity ? 1137
=========================================
Files ? 66
Lines ? 5716
Branches ? 0
=========================================
Hits ? 4618
Misses ? 1098
Partials ? 0 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| private ?string $cachedToken = null; | ||
|
|
||
| private int $tokenExpiresAt = 0; |
There was a problem hiding this comment.
this is not the state of driver but the state of token so we shouldn't keep it here!
for people using event-loop technologies in PHP, this line becomes a shared state and causes a race condition.
please keep the tokens expiration state either as a local variable or create a Token class for it:
readonly class Token
{
public function __construct(
public string $value,
public int $expiresAt,
) {
}
}| { | ||
| protected Client $client; | ||
|
|
||
| private ?string $cachedToken = null; |
There was a problem hiding this comment.
this is also not needed! we should pass the token from caller to the callee. btw I can see there is only one call to token method and it is also a force token refresh request because it is called with a true parameter as ->token(true) , so please remove this line.
| if ($credit > $amount || $cash !== $amount - $credit) { | ||
| throw new RuntimeException('Bajet paid amount does not match the invoice.'); | ||
| } | ||
| } elseif ($status === '') { |
There was a problem hiding this comment.
When the response has a recognized success status (e.g. "success") but no amount, creditAmount, or cashAmount at all, this branch does nothing — verify() returns a receipt with zero amount corroboration against the invoice. Only the empty-status case is rejected here; a non-empty, recognized-success status with no amount data at all sails through. Was that intentional for some provider response shape, or should we require at least one amount field whenever we don't have a split?
| $orderId = $this->text($result['orderId'] ?? null); | ||
| $expectedOrder = $this->invoice->getDetail('orderId'); | ||
| if ($expectedOrder !== null && $orderId !== $this->text($expectedOrder)) { | ||
| if (array_key_exists('orderId', $result) && $expectedOrder !== null |
There was a problem hiding this comment.
This changed from fail-closed to fail-open: previously a missing orderId in the response was treated as an empty string and rejected against $expectedOrder. Now, if the response simply omits orderId, the order-match check is skipped entirely rather than failing. Combined with the status-only path in verify() above, a minimal response like {"status":"success","referenceId":"ref-1"} would verify with no order or amount corroboration at all — worth double-checking this is the intended trust boundary for the official API.
| private function httpsUrl(mixed $value): string | ||
| { | ||
| $url = $this->text($value); | ||
| $url = $this->text($value); |
There was a problem hiding this comment.
Indentation regression: this line (and the return $url; a few lines below) picked up 4 extra spaces of indentation, so they're now misaligned with the rest of the method body. Looks unintentional — probably worth a formatter pass before merge.
| } | ||
| if (($inquiry['finalStatus'] ?? null) !== 'SUCCESS') { | ||
| throw new RuntimeException('Bajet transaction is not ready for verification.'); | ||
| if ($status !== '' && !in_array($status, ['success', 'successful', 'completed'], true)) { |
There was a problem hiding this comment.
When both status and finalStatus are missing, $status is "" and this check is skipped. So a response with only creditAmount/cashAmount (no status at all) is treated as paid, and testOfficialVerificationShapesDoNotRequireInquiryOrOrderId even pins it (status => "", creditAmount => 10000).
Amounts that add up only say the numbers are consistent, not that the payment happened. Can we require an explicit success status here, or point to the official response sample that really has no status?
| } | ||
|
|
||
| /** Amount is in invoice currency; reuse the same trackId when reconciling a refund. */ | ||
| public function refund(?int $amount = null, ?string $trackId = null): array |
There was a problem hiding this comment.
These new public methods (reverse, refund, refundInquiry, isRefundEnabled) are outside DriverInterface, so callers can only reach them by knowing the concrete class. Same thing we talked about in #179: please put them behind a small contract (e.g. Refundable) so other drivers can share it and callers can do $driver instanceof Refundable.
|
|
||
| return $this->text($result['token'] ?? null); | ||
| $token = $this->text($result['token'] ?? null); | ||
| $ttl = min(840, $this->integer($result['expiresIn'] ?? 840)); |
There was a problem hiding this comment.
Nit: 840 is a magic number, and the token is treated as valid until the very last second. Give it a name (e.g. MAX_TOKEN_TTL) and maybe keep a few seconds of margin. This fits well in the Token class suggested above.
| throw new RuntimeException('Unable to contact Bajet. Check transaction status before retrying.'); | ||
| } | ||
|
|
||
| if ($refreshOnUnauthorized && in_array($response->getStatusCode(), [401, 403], true)) { |
There was a problem hiding this comment.
A 403 usually means "not allowed", not "token expired". For order, reverse and refund we would refresh the token and send the same money request again. Should we retry only on 401, or at least only for requests that are safe to repeat? The PR says refunds are never auto-repeated, and this path is a small exception to it.
| $refundCheck = $operation === 'terminal/check-refund'; | ||
| if ($response->getStatusCode() < 200 || $response->getStatusCode() >= 300 | ||
| || (isset($body['status']) && is_numeric($body['status']) && (int) $body['status'] >= 400) | ||
| || (array_key_exists('success', $body) ? $body['success'] !== true : !$refundCheck)) { |
There was a problem hiding this comment.
This condition is hard to read: a success key, a numeric status, and a special case for terminal/check-refund, all in one expression. And the list at lines 294-296 repeats the same operation names. A small private method like isSuccessful(array $body, string $operation): bool plus one constant for the "no result envelope" operations would make it much easier to follow and test.
به‌روزرسانی درایور باجت
verifyو پشتیبانی از قالب‌های مختلف پاسخ موفق.401یا403.reverse،refund،refundInquiryو بررسی فعال بودن استرداد باisRefundEnabled.trackIdثابت برای پیگیری استرداد و جلوگیری از تکرار خودکار درخواست در خطاهای ارتباطی.نتیجه بررسی
۱۰۷ تست درایور باجت با پاسخ‌های شبیه‌سازی‌شده موفق بوده است. بررسی PHPStan نیز بدون خطا انجام شده است.