Feat: invoice basic implementation #23
Conversation
- Invoice object - Credit object - Discount object
WalkthroughAdds three domain classes: Credit (value object) with status constants, credit consumption logic, and (de)serialization; Discount (value object) with fixed/percentage types, non‑negative validation, calculation, and (de)serialization; and Invoice (aggregate) that manages amounts, discounts, credits, lifecycle methods (applyDiscounts, applyCredits, finalize), normalization from arrays, and (de)serialization. Also adds PHPUnit test suites for Credit, Discount, and Invoice covering construction, getters/setters, calculations, application flows, status transitions, edge cases, and (de)serialization. Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas to focus review on:
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (15)
src/Pay/Discount/Discount.php (2)
98-107: Add return type declaration for consistencyThe
toArraymethod lacks a return type declaration while other methods have them.-public function toArray() +public function toArray(): array { return [ 'id' => $this->id, 'amount' => $this->amount, 'value' => $this->value, 'description' => $this->description, 'type' => $this->type, ]; }
109-120: Add parameter and return type declarationsThe
fromArraymethod lacks type declarations for both parameter and return type.-public static function fromArray($data) +public static function fromArray(array $data): self { $discount = new self( $data['id'] ?? $data['$id'] ?? '', $data['value'] ?? 0, $data['amount'] ?? 0, $data['description'] ?? '', $data['type'] ?? self::TYPE_FIXED, ); return $discount; }src/Pay/Credit/Credit.php (4)
16-17: PHPDoc type hint inconsistencyThe PHPDoc comment indicates
@param int $creditsUsedbut the constructor acceptsfloat $creditsUsed./** * @param string $id * @param float $credits - * @param int $creditsUsed + * @param float $creditsUsed * @param string $status */
86-88: Potential floating-point comparison issueUsing strict equality (
===) with floating-point numbers can be unreliable due to precision issues.Consider using a small epsilon for comparison:
-if ($this->credits === 0) { +if ($this->credits < 0.0001) { $this->status = self::STATUS_APPLIED; }
93-98: Missing parameter type declarationThe
setStatusmethod parameter lacks a type declaration while other setters have them.-public function setStatus($status): static +public function setStatus(string $status): static { $this->status = $status; return $this; }
100-103: Potential floating-point comparison issueSimilar to the previous issue, using strict equality with floats can be problematic.
public function isFullyUsed(): bool { - return $this->credits === 0 || $this->status === self::STATUS_APPLIED; + return $this->credits < 0.0001 || $this->status === self::STATUS_APPLIED; }tests/Pay/Credit/CreditTest.php (2)
82-87: Remove unnecessary conditional logic in testThe test includes defensive code that shouldn't be necessary if the implementation is correct. Tests should assert expected behavior, not work around potential bugs.
// Check if status is applied when credits are zero -// If the implementation doesn't change the status, we need to manually call markAsApplied -if ($this->credit->getStatus() !== Credit::STATUS_APPLIED) { - $this->credit->markAsApplied(); -} $this->assertEquals(Credit::STATUS_APPLIED, $this->credit->getStatus());
98-104: Remove unnecessary conditional logic in testSame issue as above - the test should assert the expected behavior directly.
// Check if status is applied when credits are zero -// If the implementation doesn't change the status, we need to manually call markAsApplied -if ($this->credit->getStatus() !== Credit::STATUS_APPLIED) { - $this->credit->markAsApplied(); -} $this->assertEquals(Credit::STATUS_APPLIED, $this->credit->getStatus());tests/Pay/Invoice/InvoiceTest.php (1)
333-339: Remove unnecessary conditional logic in testSimilar to the Credit tests, this defensive code shouldn't be needed if the implementation is correct.
-// The implementation may use different logic for setting status -// Adjust based on the actual implementation -if ($this->invoice->getStatus() !== Invoice::STATUS_SUCCEEDED) { - $this->invoice->markAsSucceeded(); -} - $this->assertEquals(Invoice::STATUS_SUCCEEDED, $this->invoice->getStatus());tests/Pay/Discount/DiscountTest.php (1)
109-120: Incorrect handling of negative invoice amountsThe test wraps the discount calculation with
max(0, ...)which hides the actual behavior. The implementation should handle negative amounts properly, and the test should verify that behavior directly.public function testCalculateDiscountWithNegativeInvoiceAmount(): void { $invoiceAmount = -50.0; - // Assuming the implementation should handle negative amounts safely - // Adjust based on the expected behavior in your application - $fixedDiscountAmount = max(0, $this->fixedDiscount->calculateDiscount($invoiceAmount)); - $percentageDiscountAmount = max(0, $this->percentageDiscount->calculateDiscount($invoiceAmount)); + $fixedDiscountAmount = $this->fixedDiscount->calculateDiscount($invoiceAmount); + $percentageDiscountAmount = $this->percentageDiscount->calculateDiscount($invoiceAmount); $this->assertEquals(0, $fixedDiscountAmount); - $this->assertEquals(0, $percentageDiscountAmount); + // Percentage of negative amount would be negative + $this->assertEquals(-5.0, $percentageDiscountAmount); }Note: The implementation should probably return 0 for negative amounts, but the test should verify the actual behavior.
src/Pay/Invoice/Invoice.php (5)
141-141: Unnecessary array checkThe parameter is already type-hinted as
array, so checkingis_array($discounts)is redundant.public function setDiscounts(array $discounts): static { // Handle both arrays of Discount objects and arrays of arrays - if (is_array($discounts)) { $discountObjects = []; foreach ($discounts as $discount) { if ($discount instanceof Discount) { $discountObjects[] = $discount; } elseif (is_array($discount)) { // Convert array to Discount object for backward compatibility $discountObjects[] = new Discount( $discount['id'] ?? uniqid('discount_'), $discount['value'] ?? 0, $discount['amount'] ?? 0, $discount['description'] ?? '', $discount['type'] ?? Discount::TYPE_FIXED ); } else { throw new \InvalidArgumentException('Discount must be either a Discount object or an array'); } } $this->discounts = $discountObjects; - } else { - throw new \InvalidArgumentException('Discounts must be an array'); - } return $this; }
234-237: Add parameter type declaration and return typeThe
isBelowMinimumAmountmethod lacks type declarations.-public function isBelowMinimumAmount($minimumAmount = 0.50) +public function isBelowMinimumAmount(float $minimumAmount = 0.50): bool { return $this->grossAmount < $minimumAmount; }
239-242: Potential floating-point comparison issueUsing
==for float comparison can be unreliable.public function isZeroAmount(): bool { - return $this->grossAmount == 0; + return abs($this->grossAmount) < 0.0001; }
311-312: Unreachable/unnecessary code in applyCreditsThe condition at Line 311 checks if
$amount == 0and breaks, but Line 319-321 has an unnecessary check forisZeroAmount()which would never be true at that point, and the continue statement in a loop that's about to check the same condition is redundant.foreach ($this->credits as $credit) { if ($amount == 0) { break; } $creditToUse = $credit->useCredits($amount); $amount = $amount - $creditToUse; $totalCreditsUsed += $creditToUse; $creditsIds[] = $credit->getId(); - if ($this->isZeroAmount()) { - continue; - } }Also applies to: 319-321, 338-340
429-431: Array keys may not be preserved after filteringUsing
array_filterwithoutarray_valuesmay result in non-sequential array keys, which could cause issues if the array is expected to have sequential numeric keys.public function removeDiscountById(string $id): static { - $this->discounts = array_filter($this->discounts, function ($discount) use ($id) { + $this->discounts = array_values(array_filter($this->discounts, function ($discount) use ($id) { return $discount->getId() !== $id; - }); + })); return $this; }public function removeCreditById(string $id): static { - $this->credits = array_filter($this->credits, function ($credit) use ($id) { + $this->credits = array_values(array_filter($this->credits, function ($credit) use ($id) { return $credit->getId() !== $id; - }); + })); return $this; }Also applies to: 438-440
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/Pay/Credit/Credit.php(1 hunks)src/Pay/Discount/Discount.php(1 hunks)src/Pay/Invoice/Invoice.php(1 hunks)tests/Pay/Credit/CreditTest.php(1 hunks)tests/Pay/Discount/DiscountTest.php(1 hunks)tests/Pay/Invoice/InvoiceTest.php(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
tests/Pay/Credit/CreditTest.php (2)
src/Pay/Credit/Credit.php (3)
Credit(5-123)markAsApplied(28-33)useCredits(76-91)src/Pay/Invoice/Invoice.php (9)
getId(57-60)getCredits(264-267)getCreditsUsed(174-177)getStatus(72-75)setCredits(269-285)setCreditsUsed(179-184)setStatus(198-203)toArray(445-461)fromArray(463-492)
tests/Pay/Discount/DiscountTest.php (2)
src/Pay/Discount/Discount.php (14)
Discount(5-121)getId(22-25)getValue(70-73)getAmount(34-37)getDescription(46-49)getType(58-61)setId(27-32)setValue(75-80)setAmount(39-44)setDescription(51-56)setType(63-68)calculateDiscount(82-91)toArray(98-107)fromArray(109-120)src/Pay/Invoice/Invoice.php (4)
getId(57-60)getAmount(62-65)toArray(445-461)fromArray(463-492)
src/Pay/Discount/Discount.php (2)
src/Pay/Invoice/Invoice.php (3)
__construct(38-55)toArray(445-461)fromArray(463-492)src/Pay/Credit/Credit.php (3)
__construct(19-21)toArray(114-122)fromArray(105-112)
tests/Pay/Invoice/InvoiceTest.php (3)
src/Pay/Credit/Credit.php (1)
Credit(5-123)src/Pay/Discount/Discount.php (3)
Discount(5-121)toArray(98-107)fromArray(109-120)src/Pay/Invoice/Invoice.php (28)
Invoice(8-493)getGrossAmount(85-88)getTaxAmount(97-100)getVatAmount(109-112)setGrossAmount(90-95)setTaxAmount(102-107)setVatAmount(114-119)setStatus(198-203)markAsPaid(80-83)markAsDue(205-210)markAsSucceeded(215-220)markAsCancelled(222-227)setDiscounts(138-165)setCredits(269-285)applyDiscounts(332-353)applyCredits(304-330)finalize(355-383)toArray(445-461)fromArray(463-492)hasDiscounts(385-388)hasCredits(390-393)findDiscountById(405-414)findCreditById(416-425)removeDiscountById(427-434)removeCreditById(436-443)isNegativeAmount(229-232)isZeroAmount(239-242)isBelowMinimumAmount(234-237)
src/Pay/Credit/Credit.php (2)
src/Pay/Invoice/Invoice.php (10)
__construct(38-55)getStatus(72-75)getId(57-60)getCredits(264-267)setCredits(269-285)getCreditsUsed(174-177)setCreditsUsed(179-184)setStatus(198-203)fromArray(463-492)toArray(445-461)src/Pay/Discount/Discount.php (4)
__construct(18-20)getId(22-25)fromArray(109-120)toArray(98-107)
src/Pay/Invoice/Invoice.php (2)
src/Pay/Credit/Credit.php (12)
Credit(5-123)__construct(19-21)setCredits(52-57)getId(35-38)getStatus(23-26)getCreditsUsed(59-62)setCreditsUsed(64-69)setStatus(93-98)toArray(114-122)getCredits(47-50)fromArray(105-112)useCredits(76-91)src/Pay/Discount/Discount.php (7)
Discount(5-121)__construct(18-20)getId(22-25)getAmount(34-37)toArray(98-107)fromArray(109-120)calculateDiscount(82-91)
🔇 Additional comments (1)
src/Pay/Discount/Discount.php (1)
93-96: Operator precedence issue in validation logicThe condition has incorrect operator precedence. The
||operator will make this return true whenever$this->type === self::TYPE_PERCENTAGEregardless of the value check.Add parentheses to fix the logical grouping:
public function isValid(): bool { - return $this->amount > 0 && $this->type === self::TYPE_FIXED || $this->value > 0 && $this->type === self::TYPE_PERCENTAGE; + return ($this->amount > 0 && $this->type === self::TYPE_FIXED) || ($this->value > 0 && $this->type === self::TYPE_PERCENTAGE); }Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
src/Pay/Discount/Discount.php (2)
109-117: Add return type declaration for consistency.The method is missing an explicit return type declaration.
Apply this diff:
- public function toArray() + public function toArray(): array {
119-138: Add type declarations for consistency.The method is missing explicit type declarations for the parameter and return value.
Apply this diff:
- public static function fromArray($data) + public static function fromArray(array $data): self {src/Pay/Invoice/Invoice.php (5)
228-231: Add type hint and use strict comparison.The parameter lacks a type declaration, and the comparison should be strict for consistency.
Apply this diff:
- public function isBelowMinimumAmount($minimumAmount = 0.50) + public function isBelowMinimumAmount(float $minimumAmount = 0.50): bool { - return $this->grossAmount < $minimumAmount; + return $this->grossAmount < $minimumAmount; }Note: The comparison operator
<is already strict; consider also adding the explicit return type.
233-236: Use strict comparison for zero check.Loose comparison (
==) with floats can lead to unexpected behavior. Since amounts are rounded to 2 decimals, use strict comparison.Apply this diff:
public function isZeroAmount(): bool { - return $this->grossAmount == 0; + return $this->grossAmount === 0.0; }
305-331: Use strict comparison and remove redundant check.Two issues:
- Line 312 uses loose comparison (
==) which should be strict (===)- Lines 320-322: The
isZeroAmount()check is redundant since Line 312-314 already breaks when amount reaches 0Apply this diff:
foreach ($this->credits as $credit) { - if ($amount == 0) { + if ($amount === 0.0) { break; } $creditToUse = $credit->useCredits($amount); $amount = $amount - $creditToUse; $totalCreditsUsed += $creditToUse; $creditsIds[] = $credit->getId(); - if ($this->isZeroAmount()) { - continue; - } }
333-354: Use strict comparison for consistency.Line 339 uses loose comparison which should be strict.
Apply this diff:
foreach ($discounts as $discount) { - if ($amount == 0) { + if ($amount === 0.0) { break; }
428-444: Consider re-indexing arrays after filtering.Both methods use
array_filterwhich preserves keys, potentially leaving gaps in the numeric indices. While this works withforeachiteration, it may cause issues if code assumes sequential indices.If sequential indices are needed, apply this diff:
public function removeDiscountById(string $id): static { - $this->discounts = array_filter($this->discounts, function ($discount) use ($id) { + $this->discounts = array_values(array_filter($this->discounts, function ($discount) use ($id) { return $discount->getId() !== $id; - }); + })); return $this; } public function removeCreditById(string $id): static { - $this->credits = array_filter($this->credits, function ($credit) use ($id) { + $this->credits = array_values(array_filter($this->credits, function ($credit) use ($id) { return $credit->getId() !== $id; - }); + })); return $this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Pay/Discount/Discount.php(1 hunks)src/Pay/Invoice/Invoice.php(1 hunks)tests/Pay/Discount/DiscountTest.php(1 hunks)tests/Pay/Invoice/InvoiceTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/Pay/Invoice/InvoiceTest.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Pay/Discount/DiscountTest.php (1)
src/Pay/Discount/Discount.php (12)
Discount(5-139)getId(28-31)getValue(43-46)getDescription(64-67)getType(76-79)setId(33-38)setValue(53-62)setDescription(69-74)setType(81-86)calculateDiscount(94-107)toArray(109-117)fromArray(119-138)
src/Pay/Invoice/Invoice.php (2)
src/Pay/Credit/Credit.php (12)
Credit(5-123)__construct(19-21)setCredits(52-57)getId(35-38)getStatus(23-26)fromArray(105-112)getCreditsUsed(59-62)setCreditsUsed(64-69)setStatus(93-98)toArray(114-122)getCredits(47-50)useCredits(76-91)src/Pay/Discount/Discount.php (6)
Discount(5-139)__construct(17-26)getId(28-31)fromArray(119-138)calculateDiscount(94-107)toArray(109-117)
src/Pay/Discount/Discount.php (2)
src/Pay/Invoice/Invoice.php (4)
__construct(38-55)getId(57-60)toArray(446-462)fromArray(464-493)src/Pay/Credit/Credit.php (5)
__construct(19-21)getId(35-38)setId(40-45)toArray(114-122)fromArray(105-112)
🔇 Additional comments (13)
src/Pay/Discount/Discount.php (2)
17-26: LGTM: Constructor with proper validation.The constructor is well-documented and includes appropriate validation to prevent negative discount values.
94-107: LGTM: Discount calculation logic is correct.The method properly handles both discount types, caps fixed discounts at the amount, and guards against non-positive amounts.
tests/Pay/Discount/DiscountTest.php (4)
22-63: LGTM: Well-structured test setup and basic validation.The setUp method and basic tests appropriately cover constructor and getter/setter behavior.
65-90: LGTM: Comprehensive discount calculation tests.The tests effectively validate both fixed and percentage discount calculations, including the capping behavior for fixed discounts.
116-162: LGTM: Thorough serialization tests.The tests comprehensively cover array conversion in both directions, including minimal data scenarios with proper defaults.
164-222: LGTM: Comprehensive validation tests.The tests thoroughly validate error handling for negative and null values across constructor, setter, and deserialization methods.
src/Pay/Invoice/Invoice.php (7)
38-55: LGTM: Well-structured constructor.The constructor properly uses promoted properties and ensures discounts/credits are converted to objects via the setters.
57-75: LGTM: Standard getters.The getters are straightforward and correctly return the property values.
77-131: LGTM: Fluent setters with proper return types.The setter methods follow a consistent fluent interface pattern.
138-159: LGTM: Robust discount setter with type handling.The method properly handles both Discount objects and arrays, with appropriate validation and error messages.
192-221: LGTM: Clean status transition methods.The status management methods are straightforward and follow a consistent pattern.
238-253: LGTM: Correct sequential discount calculation.The method properly applies discounts in sequence, reducing the base amount after each discount.
270-303: LGTM: Consistent credit handling.The credit management methods follow the same pattern as discounts with proper validation and conversion.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/Pay/Discount/Discount.php (1)
135-140: Validate discount type in setter to prevent silent failures.The
setTypemethod accepts any string without validation. If an invalid type is provided,calculateDiscount()will silently return 0, making it difficult to detect configuration errors during development or runtime.Apply this diff to add validation:
public function setType(string $type): static { + if ($type !== self::TYPE_FIXED && $type !== self::TYPE_PERCENTAGE) { + throw new \InvalidArgumentException( + 'Discount type must be either TYPE_FIXED or TYPE_PERCENTAGE' + ); + } + $this->type = $type; return $this; }Based on past review comments that flagged this issue.
🧹 Nitpick comments (1)
src/Pay/Invoice/Invoice.php (1)
249-270: Remove redundant array type check.Line 252 checks
is_array($discounts), but since the parameter is already typed asarray, this condition will always be true. The else block at lines 265-267 is unreachable dead code.Apply this diff to simplify:
public function setDiscounts(array $discounts): static { - // Handle both arrays of Discount objects and arrays of arrays - if (is_array($discounts)) { - $discountObjects = []; - foreach ($discounts as $discount) { - if ($discount instanceof Discount) { - $discountObjects[] = $discount; - } elseif (is_array($discount)) { - // Convert array to Discount object using fromArray for backward compatibility - $discountObjects[] = Discount::fromArray($discount); - } else { - throw new \InvalidArgumentException('Discount must be either a Discount object or an array'); - } + $discountObjects = []; + foreach ($discounts as $discount) { + if ($discount instanceof Discount) { + $discountObjects[] = $discount; + } elseif (is_array($discount)) { + // Convert array to Discount object using fromArray for backward compatibility + $discountObjects[] = Discount::fromArray($discount); + } else { + throw new \InvalidArgumentException('Discount must be either a Discount object or an array'); } - $this->discounts = $discountObjects; - } else { - throw new \InvalidArgumentException('Discounts must be an array'); } + $this->discounts = $discountObjects; return $this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/Pay/Credit/Credit.php(1 hunks)src/Pay/Discount/Discount.php(1 hunks)src/Pay/Invoice/Invoice.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Pay/Credit/Credit.php
🧰 Additional context used
🧬 Code graph analysis (2)
src/Pay/Discount/Discount.php (2)
src/Pay/Credit/Credit.php (3)
__construct(36-38)toArray(215-223)fromArray(200-208)src/Pay/Invoice/Invoice.php (3)
__construct(68-85)toArray(749-766)fromArray(774-805)
src/Pay/Invoice/Invoice.php (2)
src/Pay/Credit/Credit.php (12)
Credit(11-224)__construct(36-38)setCredits(101-106)getId(67-70)getStatus(45-48)fromArray(200-208)getCreditsUsed(113-116)setCreditsUsed(124-129)setStatus(177-182)toArray(215-223)getCredits(90-93)useCredits(150-169)src/Pay/Discount/Discount.php (7)
Discount(11-209)__construct(33-42)getId(49-52)fromArray(189-208)toArray(171-179)getType(124-127)calculateDiscount(151-164)
🔇 Additional comments (3)
src/Pay/Discount/Discount.php (1)
151-164: Consider validating against negative amounts.While the method returns 0 for
$amount <= 0, it doesn't explicitly prevent or warn about negative amounts being passed. Negative amounts could indicate a logic error upstream, and silently returning 0 may mask bugs.Consider either:
- Adding explicit validation to throw an exception for negative amounts
- Adding a comment explaining why negative amounts are treated as zero
This would help catch potential bugs where negative amounts are passed unintentionally. Based on past review comments.
src/Pay/Invoice/Invoice.php (2)
565-603: Discount application logic looks solid.The method correctly:
- Sorts fixed discounts before percentage discounts
- Applies discounts sequentially to the running amount
- Stops when amount reaches zero
- Rounds final amounts appropriately
616-643: Finalization workflow is well-structured.The method properly sequences:
- Initialize grossAmount from base amount
- Apply discounts
- Round and add tax/VAT
- Apply credits
- Set appropriate status based on final amount
The status transitions (zero → succeeded, below minimum → cancelled, otherwise → due) provide clear business logic.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/Pay/Invoice/InvoiceTest.php (1)
322-340: Remove conditional logic from test—assert implementation behavior directly.Lines 335-337 contain conditional logic that checks whether the status is
SUCCEEDEDand manually marks it if not. Tests should assert expected behavior, not conditionally adjust state. This pattern suggests either:
- Uncertainty about the implementation's behavior
- The implementation is non-deterministic
- The test is working around an inconsistency
Based on the
finalize()implementation in Invoice.php (lines 631-632), whenisZeroAmount()returns true, the status should be set toSUCCEEDED.Remove the conditional and assert the expected behavior directly:
$this->invoice->finalize(); $this->assertEquals(0.0, $this->invoice->getGrossAmount()); - - // The implementation may use different logic for setting status - // Adjust based on the actual implementation - if ($this->invoice->getStatus() !== Invoice::STATUS_SUCCEEDED) { - $this->invoice->markAsSucceeded(); - } - $this->assertEquals(Invoice::STATUS_SUCCEEDED, $this->invoice->getStatus());src/Pay/Invoice/Invoice.php (2)
249-270: Minor: Redundant type check on already-typed parameter.Line 252 checks
is_array($discounts), but the parameter is already type-hinted asarrayin the method signature (line 249). This check will always be true and the else block (lines 265-267) is unreachable.Consider removing the redundant check and the unreachable else block:
public function setDiscounts(array $discounts): static { - // Handle both arrays of Discount objects and arrays of arrays - if (is_array($discounts)) { - $discountObjects = []; - foreach ($discounts as $discount) { - if ($discount instanceof Discount) { - $discountObjects[] = $discount; - } elseif (is_array($discount)) { - // Convert array to Discount object using fromArray for backward compatibility - $discountObjects[] = Discount::fromArray($discount); - } else { - throw new \InvalidArgumentException('Discount must be either a Discount object or an array'); - } + $discountObjects = []; + foreach ($discounts as $discount) { + if ($discount instanceof Discount) { + $discountObjects[] = $discount; + } elseif (is_array($discount)) { + $discountObjects[] = Discount::fromArray($discount); + } else { + throw new \InvalidArgumentException('Discount must be either a Discount object or an array'); } - $this->discounts = $discountObjects; - } else { - throw new \InvalidArgumentException('Discounts must be an array'); } + $this->discounts = $discountObjects; return $this; }
472-488: Similar redundant check and slightly misleading comment.Like
setDiscounts, this method has a redundant type check since$creditsis already typed asarray. Additionally, the comment on line 474 says "Validate that all items are Credit objects" but the code actually normalizes arrays to Credit objects rather than just validating.Consider simplifying the structure and clarifying the comment:
public function setCredits(array $credits): static { - // Validate that all items are Credit objects + // Normalize to Credit objects $creditObjects = []; foreach ($credits as $credit) { if ($credit instanceof Credit) { $creditObjects[] = $credit; } elseif (is_array($credit)) { $creditObjects[] = Credit::fromArray($credit); } else { - throw new \InvalidArgumentException('All items in credits array must be Credit objects or arrays with id and credits keys'); + throw new \InvalidArgumentException('Credit must be either a Credit object or an array'); } } $this->credits = $creditObjects; return $this; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Pay/Discount/Discount.php(1 hunks)src/Pay/Invoice/Invoice.php(1 hunks)tests/Pay/Discount/DiscountTest.php(1 hunks)tests/Pay/Invoice/InvoiceTest.php(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Pay/Discount/Discount.php
🧰 Additional context used
🧬 Code graph analysis (3)
tests/Pay/Invoice/InvoiceTest.php (3)
src/Pay/Credit/Credit.php (1)
Credit(11-224)src/Pay/Discount/Discount.php (4)
Discount(11-213)getId(49-52)toArray(175-183)fromArray(193-212)src/Pay/Invoice/Invoice.php (43)
Invoice(14-803)getId(92-95)getAmount(102-105)getCurrency(112-115)getStatus(122-125)getGrossAmount(142-145)getTaxAmount(165-168)getVatAmount(188-191)getCreditsUsed(290-293)getAddress(211-214)getDiscounts(234-237)getCredits(457-460)setGrossAmount(153-158)setTaxAmount(176-181)setVatAmount(199-204)setAddress(222-227)setCreditsUsed(301-306)setStatus(337-342)markAsPaid(132-135)markAsDue(349-354)markAsSucceeded(361-366)markAsCancelled(373-378)addDiscount(278-283)addCredit(496-501)getTotalAvailableCredits(508-516)setDiscounts(249-270)setCredits(472-488)applyDiscounts(562-600)getDiscountTotal(419-422)applyCredits(526-549)finalize(613-640)setDiscountTotal(430-435)toArray(746-763)fromArray(771-802)hasDiscounts(647-650)hasCredits(657-660)findDiscountById(683-692)findCreditById(700-709)removeDiscountById(717-724)removeCreditById(732-739)isNegativeAmount(385-388)isZeroAmount(406-409)isBelowMinimumAmount(396-399)
src/Pay/Invoice/Invoice.php (2)
src/Pay/Credit/Credit.php (12)
Credit(11-224)__construct(36-38)setCredits(101-106)getId(67-70)getStatus(45-48)fromArray(200-208)getCreditsUsed(113-116)setCreditsUsed(124-129)setStatus(177-182)toArray(215-223)getCredits(90-93)useCredits(150-169)src/Pay/Discount/Discount.php (7)
Discount(11-213)__construct(33-42)getId(49-52)fromArray(193-212)toArray(175-183)getType(124-127)calculateDiscount(155-168)
tests/Pay/Discount/DiscountTest.php (2)
src/Pay/Discount/Discount.php (12)
Discount(11-213)getId(49-52)getValue(72-75)getDescription(101-104)getType(124-127)setId(60-65)setValue(85-94)setDescription(112-117)setType(135-144)calculateDiscount(155-168)toArray(175-183)fromArray(193-212)src/Pay/Invoice/Invoice.php (3)
getId(92-95)toArray(746-763)fromArray(771-802)
🔇 Additional comments (5)
tests/Pay/Invoice/InvoiceTest.php (1)
1-72: Comprehensive test coverage with clear assertions.The test suite provides excellent coverage of the Invoice class functionality, including:
- Constructor variations with and without discounts/credits
- Getter/setter validation
- Status transitions
- Discount and credit application with proper ordering
- Finalization workflow with edge cases
- Serialization/deserialization round-trips
- Utility methods for finding and removing items
The tests are well-organized, use clear naming conventions, and properly exercise both the happy path and edge cases.
Also applies to: 93-320, 342-477
tests/Pay/Discount/DiscountTest.php (1)
1-223: Excellent test coverage for the Discount value object.The test suite thoroughly validates all aspects of the Discount class:
- Constructor validation and initialization
- Getter/setter behavior with type enforcement
- Discount calculation logic for both TYPE_FIXED and TYPE_PERCENTAGE
- Edge cases including zero amounts, negative amounts, and capping behavior
- Serialization/deserialization with full and minimal data sets
- Validation of negative and null values with proper exception messages
The tests are clear, focused, and properly assert expected behavior without masking implementation details.
src/Pay/Invoice/Invoice.php (3)
1-227: Well-structured foundation with clear status constants and fluent API.The class header, constants, constructor, and basic accessors are well-designed:
- Comprehensive status constants with clear documentation
- Constructor properly normalizes discounts and credits via setters
- Consistent fluent interface pattern throughout
- Appropriate use of promoted properties for concise initialization
526-600: Credit and discount application logic is correct and well-implemented.Both
applyCreditsandapplyDiscountsimplement proper financial calculation flows:applyCredits (526-549):
- Correctly iterates through credits sequentially
- Uses Credit's useCredits method for proper state management
- Tracks used credits and IDs for audit trail
- Breaks early when amount reaches zero
- Properly rounds to 2 decimal places
applyDiscounts (562-600):
- Correctly sorts discounts (fixed first, then percentage) to ensure accurate calculations
- Sequential application prevents incorrect compounding
- Skips zero-value discounts
- Properly rounds both amount and discount total
613-802: Finalization flow and serialization are well-orchestrated.The remaining implementation is solid:
finalize (613-640):
- Proper sequence: base amount → discounts → tax/VAT → credits
- Appropriate status determination based on final amount
- Consistent rounding throughout
Utility methods (647-739):
- Simple, readable implementations for has/find/remove operations
- Linear search is appropriate for typical invoice sizes
Serialization (746-802):
- toArray properly delegates to nested objects
- fromArray correctly reconstructs object graph using Discount::fromArray and Credit::fromArray
- Sensible defaults for optional fields
Summary by CodeRabbit
New Features
Tests