Feat: invoice basic implementation #23
Conversation
- Invoice object - Credit object - Discount object
WalkthroughAdds three new classes: Credit, Discount, and Invoice, implementing credit/discount modeling, application, and invoice lifecycle with serialization. Introduces corresponding PHPUnit test suites validating construction, getters/setters, calculations, application order, status transitions, and (de)serialization for all new entities. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant I as Invoice
participant D as Discount
participant R as Credit
rect rgba(230,240,255,0.6)
note right of I: Initialization
C->>I: new Invoice(id, amount, ...discounts, credits)
I->>I: setDiscounts()/setCredits()<br/>(normalize arrays to objects)
end
rect rgba(235,255,235,0.6)
note over I: Finalize
C->>I: finalize()
I->>I: gross = round(amount, 2)
loop for each Discount
I->>D: calculateDiscount(gross)
D-->>I: discountAmount
I->>I: gross -= discountAmount
end
I->>I: gross += round(taxAmount) + round(vatAmount)
alt Credits available
loop for each Credit
I->>R: useCredits(gross)
R-->>I: used
I->>I: gross -= used<br/>track creditsUsed, credit IDs
end
end
alt gross == 0
I->>I: status = SUCCEEDED
else gross < minimum
I->>I: status = CANCELLED
else
I->>I: status = DUE
end
I-->>C: Invoice (updated)
end
sequenceDiagram
autonumber
participant I as Invoice
participant R as Credit[*]
I->>I: applyCredits()
loop credits
I->>R: useCredits(gross)
R-->>I: usedAmount
I->>I: gross -= usedAmount<br/>creditsUsed += usedAmount<br/>collect credit ID
end
I-->>I: updated gross, creditsUsed, credit IDs
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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.
| public function calculateDiscount(float $amount): float | ||
| { | ||
| if ($this->type === self::TYPE_FIXED) { | ||
| return min($this->amount, $amount); | ||
| } elseif ($this->type === self::TYPE_PERCENTAGE) { | ||
| return ($this->value / 100) * $amount; | ||
| } | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Fixed discount calculation uses $this->amount instead of $this->value
The calculation for TYPE_FIXED discounts uses $this->amount (Line 85) while TYPE_PERCENTAGE uses $this->value (Line 87). This inconsistency is confusing and error-prone.
For fixed discounts, both value and amount are typically set to the same value (as seen in the tests), but conceptually value should be the discount value to apply, while amount might serve a different purpose (e.g., tracking total discount given).
Consider using $this->value consistently for both discount types:
public function calculateDiscount(float $amount): float
{
if ($this->type === self::TYPE_FIXED) {
- return min($this->amount, $amount);
+ return min($this->value, $amount);
} elseif ($this->type === self::TYPE_PERCENTAGE) {
return ($this->value / 100) * $amount;
}
return 0;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function calculateDiscount(float $amount): float | |
| { | |
| if ($this->type === self::TYPE_FIXED) { | |
| return min($this->amount, $amount); | |
| } elseif ($this->type === self::TYPE_PERCENTAGE) { | |
| return ($this->value / 100) * $amount; | |
| } | |
| return 0; | |
| } | |
| public function calculateDiscount(float $amount): float | |
| { | |
| if ($this->type === self::TYPE_FIXED) { | |
| return min($this->value, $amount); | |
| } elseif ($this->type === self::TYPE_PERCENTAGE) { | |
| return ($this->value / 100) * $amount; | |
| } | |
| return 0; | |
| } |
🤖 Prompt for AI Agents
In src/Pay/Discount/Discount.php around lines 82 to 91, the TYPE_FIXED branch
uses $this->amount instead of $this->value which is inconsistent with the
TYPE_PERCENTAGE branch; change the fixed-discount calculation to use
$this->value (e.g., return min($this->value, $amount)) so both discount types
use the intended discount value property while preserving the cap at the
transaction amount.
| public function testFromArray(): void | ||
| { | ||
| // Data should match the order expected by fromArray method | ||
| // Check actual fromArray implementation | ||
| $this->markTestSkipped('fromArray method implementation is incorrect in the Discount class and needs to be fixed'); | ||
|
|
||
| $data = [ | ||
| 'id' => 'discount-789', | ||
| 'amount' => 30.0, | ||
| 'value' => 30.0, | ||
| 'description' => 'From Array Discount', | ||
| 'type' => Discount::TYPE_FIXED, | ||
| ]; | ||
|
|
||
| $discount = Discount::fromArray($data); | ||
|
|
||
| $this->assertEquals($data['id'], $discount->getId()); | ||
| $this->assertEquals($data['value'], $discount->getValue()); | ||
| $this->assertEquals($data['amount'], $discount->getAmount()); | ||
| $this->assertEquals($data['description'], $discount->getDescription()); | ||
| $this->assertEquals($data['type'], $discount->getType()); | ||
| } |
There was a problem hiding this comment.
Re-enable the skipped fromArray tests
These tests are marked as skipped but they appear to test valid functionality. If there's an issue with the implementation, it should be fixed rather than skipping the tests.
Remove the markTestSkipped calls and ensure the tests pass. The fromArray implementation in Discount.php appears correct based on the code review.
🤖 Prompt for AI Agents
In tests/Pay/Discount/DiscountTest.php around lines 140 to 161, remove the call
to $this->markTestSkipped(...) so the fromArray test runs, then verify and if
failing fix Discount::fromArray to accept the provided associative keys
('id','amount','value','description','type') and map them to the Discount
object's properties in the same order/assertions used by the test (id, value,
amount, description, type); update fromArray to handle missing keys safely or
throw a clear exception, run the test suite and adjust mapping until the
assertions pass.
| public function testFromArrayWithMinimalData(): void | ||
| { | ||
| // Data should match the order expected by fromArray method | ||
| // Check actual fromArray implementation | ||
| $this->markTestSkipped('fromArray method implementation is incorrect in the Discount class and needs to be fixed'); | ||
|
|
||
| $data = [ | ||
| 'id' => 'discount-789', | ||
| 'value' => 30.0, | ||
| ]; | ||
|
|
||
| $discount = Discount::fromArray($data); | ||
|
|
||
| $this->assertEquals($data['id'], $discount->getId()); | ||
| $this->assertEquals($data['value'], $discount->getValue()); | ||
| $this->assertEquals(0, $discount->getAmount()); | ||
| $this->assertEquals('', $discount->getDescription()); | ||
| $this->assertEquals(Discount::TYPE_FIXED, $discount->getType()); | ||
| } |
There was a problem hiding this comment.
Re-enable the skipped fromArray test
Same issue as above - this test should be enabled.
Remove the markTestSkipped call and ensure the test passes.
🤖 Prompt for AI Agents
In tests/Pay/Discount/DiscountTest.php around lines 163 to 181, remove the
markTestSkipped call and re-enable the test; to make it pass, update the
Discount::fromArray implementation to accept an associative array with optional
keys and provide sensible defaults: set id and value from the array, default
amount to 0, description to empty string, and type to Discount::TYPE_FIXED when
not provided; ensure fromArray uses key lookups (not positional order) and
returns a Discount object with getters matching those defaults so the assertions
succeed.
|
|
||
| public function testConstructor(): void | ||
| { | ||
| $this->assertEquals($this->invoiceId, $this->invoice->getid()); |
There was a problem hiding this comment.
Typo in method name
The method call should be getId() with capital 'I', not getid().
-$this->assertEquals($this->invoiceId, $this->invoice->getid());
+$this->assertEquals($this->invoiceId, $this->invoice->getId());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->assertEquals($this->invoiceId, $this->invoice->getid()); | |
| $this->assertEquals($this->invoiceId, $this->invoice->getId()); |
🤖 Prompt for AI Agents
In tests/Pay/Invoice/InvoiceTest.php around line 62, the test calls
$this->invoice->getid() which is a typo; change the method call to
$this->invoice->getId() to match the actual method name; update the assertion to
use getId() (capital I) and run the tests to verify.
| [$this->credit] | ||
| ); | ||
|
|
||
| $this->assertEquals($this->invoiceId, $invoice->getid()); |
There was a problem hiding this comment.
Typo in method name
Same typo as above.
-$this->assertEquals($this->invoiceId, $invoice->getid());
+$this->assertEquals($this->invoiceId, $invoice->getId());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->assertEquals($this->invoiceId, $invoice->getid()); | |
| $this->assertEquals($this->invoiceId, $invoice->getId()); |
🤖 Prompt for AI Agents
In tests/Pay/Invoice/InvoiceTest.php around line 87, the call uses getid()
(lowercase 'i') which is a typo; replace it with the correct method name getId()
to match the invoice object's real method, and scan/update any other test lines
with the same typo to use getId() consistently.
|
|
||
| $invoice = Invoice::fromArray($data); | ||
|
|
||
| $this->assertEquals('invoice-array', $invoice->getid()); |
There was a problem hiding this comment.
Typo in method name
Same typo issue.
-$this->assertEquals('invoice-array', $invoice->getid());
+$this->assertEquals('invoice-array', $invoice->getId());📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $this->assertEquals('invoice-array', $invoice->getid()); | |
| $this->assertEquals('invoice-array', $invoice->getId()); |
🤖 Prompt for AI Agents
In tests/Pay/Invoice/InvoiceTest.php around line 415, the test calls
$invoice->getid() which has a lowercase 'i' typo; update the assertion to call
the correctly cased accessor $invoice->getId() (or the actual public method name
on the Invoice class) so the test uses the proper method name and will invoke
the intended getter.
Summary by CodeRabbit
New Features
Tests