Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/action/UsageInterval.php
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,10 @@
use DateInterval;
use DateTimeImmutable;
use InvalidArgumentException;
use JsonSerializable;

/** @readonly */
final class UsageInterval
final class UsageInterval implements JsonSerializable
{
/** @readonly */
private DateTimeImmutable $start;
Expand Down Expand Up @@ -189,4 +190,9 @@ public function extend(self $other): self
$newEnd,
);
}

public function jsonSerialize(): array
{
return array_filter(get_object_vars($this));
}
Comment on lines +194 to +197
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve JSON serialization to handle DateTime objects and control property exposure.

The current implementation using get_object_vars might expose internal state and doesn't properly handle DateTime objects.

Consider explicitly defining which properties to serialize and their format:

     public function jsonSerialize(): array
     {
-        return array_filter(get_object_vars($this));
+        return [
+            'month' => [
+                'date' => $this->month->format('Y-m-d\TH:i:s\Z')
+            ],
+            'start' => [
+                'date' => $this->start->format('Y-m-d\TH:i:s\Z')
+            ],
+            'end' => [
+                'date' => $this->end->format('Y-m-d\TH:i:s\Z')
+            ]
+        ];
     }

}
2 changes: 2 additions & 0 deletions src/bill/BillCreationDto.php
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,6 @@ class BillCreationDto
public $charges;

public $state;

public $usageInterval;
}
22 changes: 21 additions & 1 deletion src/bill/BillFactory.php
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@

namespace hiqdev\php\billing\bill;

use DateTimeImmutable;
use hiqdev\billing\hiapi\action\UsageIntervalHydrator;
use hiqdev\php\billing\action\UsageInterval;

/**
* Default bill factory.
*
Expand All @@ -23,7 +27,7 @@ class BillFactory implements BillFactoryInterface
*/
public function create(BillCreationDto $dto)
{
return new Bill(
$bill = new Bill(
$dto->id,
$dto->type,
$dto->time,
Expand All @@ -35,5 +39,21 @@ public function create(BillCreationDto $dto)
$dto->charges ?: [],
$dto->state
);
if (!empty($dto->usageInterval)) {
if ($dto->usageInterval instanceof UsageInterval) {
$interval = $dto->usageInterval;
} else {
$month = $dto->usageInterval['month']['date'];
$start = $dto->usageInterval['start']['date'];
$end = $dto->usageInterval['end']['date'];;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Remove extra semicolon.

There's a double semicolon at the end of the line.

-                $end = $dto->usageInterval['end']['date'];;
+                $end = $dto->usageInterval['end']['date'];
📝 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.

Suggested change
$end = $dto->usageInterval['end']['date'];;
$end = $dto->usageInterval['end']['date'];

$interval = UsageInterval::withinMonth(
new DateTimeImmutable($month),
new DateTimeImmutable($start),
new DateTimeImmutable($end)
);
}
$bill->setUsageInterval($interval);
}
Comment on lines +42 to +56
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add error handling for date parsing and array access.

The current implementation lacks error handling for:

  1. Invalid date strings that could cause DateTimeImmutable constructor to throw an exception
  2. Missing array keys that could cause undefined index errors

Consider wrapping the date parsing in a try-catch block and validating array structure:

         if (!empty($dto->usageInterval)) {
             if ($dto->usageInterval instanceof UsageInterval) {
                 $interval = $dto->usageInterval;
             } else {
+                if (!isset($dto->usageInterval['month']['date']) ||
+                    !isset($dto->usageInterval['start']['date']) ||
+                    !isset($dto->usageInterval['end']['date'])) {
+                    throw new InvalidArgumentException('Invalid usage interval structure');
+                }
                 $month = $dto->usageInterval['month']['date'];
                 $start = $dto->usageInterval['start']['date'];
                 $end = $dto->usageInterval['end']['date'];
+                try {
                     $interval = UsageInterval::withinMonth(
                         new DateTimeImmutable($month),
                         new DateTimeImmutable($start),
                         new DateTimeImmutable($end)
                     );
+                } catch (\Exception $e) {
+                    throw new InvalidArgumentException('Invalid date format in usage interval', 0, $e);
+                }
             }
             $bill->setUsageInterval($interval);
         }
📝 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.

Suggested change
if (!empty($dto->usageInterval)) {
if ($dto->usageInterval instanceof UsageInterval) {
$interval = $dto->usageInterval;
} else {
$month = $dto->usageInterval['month']['date'];
$start = $dto->usageInterval['start']['date'];
$end = $dto->usageInterval['end']['date'];;
$interval = UsageInterval::withinMonth(
new DateTimeImmutable($month),
new DateTimeImmutable($start),
new DateTimeImmutable($end)
);
}
$bill->setUsageInterval($interval);
}
if (!empty($dto->usageInterval)) {
if ($dto->usageInterval instanceof UsageInterval) {
$interval = $dto->usageInterval;
} else {
if (!isset($dto->usageInterval['month']['date']) ||
!isset($dto->usageInterval['start']['date']) ||
!isset($dto->usageInterval['end']['date'])) {
throw new InvalidArgumentException('Invalid usage interval structure');
}
$month = $dto->usageInterval['month']['date'];
$start = $dto->usageInterval['start']['date'];
$end = $dto->usageInterval['end']['date'];;
try {
$interval = UsageInterval::withinMonth(
new DateTimeImmutable($month),
new DateTimeImmutable($start),
new DateTimeImmutable($end)
);
} catch (\Exception $e) {
throw new InvalidArgumentException('Invalid date format in usage interval', 0, $e);
}
}
$bill->setUsageInterval($interval);
}

return $bill;
}
}
4 changes: 3 additions & 1 deletion src/tools/Merger.php
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ public function mergeBill(BillInterface $first, BillInterface $other): BillInter
{
$charges = $this->mergeCharges(array_merge($first->getCharges(), $other->getCharges()));

return new Bill(
$bill = new Bill(
$this->mergeId($first, $other),
$first->getType(),
$first->getTime(),
Expand All @@ -57,6 +57,8 @@ public function mergeBill(BillInterface $first, BillInterface $other): BillInter
$first->getPlan(),
$charges
);
$bill->setUsageInterval($first->getUsageInterval());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Consider merging usage intervals instead of just copying from first bill.

The current implementation copies the usage interval from the first bill without considering the other bill's interval. This might lead to loss of information if the bills have different intervals.

Consider extending the intervals:

-        $bill->setUsageInterval($first->getUsageInterval());
+        $firstInterval = $first->getUsageInterval();
+        $otherInterval = $other->getUsageInterval();
+        if ($firstInterval !== null && $otherInterval !== null) {
+            $bill->setUsageInterval($firstInterval->extend($otherInterval));
+        } elseif ($firstInterval !== null) {
+            $bill->setUsageInterval($firstInterval);
+        } elseif ($otherInterval !== null) {
+            $bill->setUsageInterval($otherInterval);
+        }
📝 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.

Suggested change
$bill->setUsageInterval($first->getUsageInterval());
$firstInterval = $first->getUsageInterval();
$otherInterval = $other->getUsageInterval();
if ($firstInterval !== null && $otherInterval !== null) {
$bill->setUsageInterval($firstInterval->extend($otherInterval));
} elseif ($firstInterval !== null) {
$bill->setUsageInterval($firstInterval);
} elseif ($otherInterval !== null) {
$bill->setUsageInterval($otherInterval);
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
$bill->setUsageInterval($first->getUsageInterval());
$bill->setUsageInterval($first->getUsageInterval()->extend($other->getUsageInterval());

Здається, має бути так

return $bill;
}

/**
Expand Down
30 changes: 30 additions & 0 deletions tests/behat/bootstrap/BillingContext.php
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,36 @@ public function billWithTime($type, $sum, $currency, $quantity, $unit, $target,
}
}

/**
* @Given /bill interval +for (\S+) is +(\S+) (\S+) per (\S+) (\S+) for target (.+?) at (\S+) between (\S+) and (\S+)?$/
*/
public function billInterval($type, $sum, $currency, $quantity, $unit, $target, $time, $since, $till)
{
$this->builder->flushEntitiesCacheByType('bill');

$quantity = $this->prepareQuantity($quantity);
$sum = $this->prepareSum($sum, $quantity);
$time = $this->prepareTime($time);
$bill = $this->findBill([
'type' => $type,
'target' => $target,
'sum' => "$sum $currency",
'quantity' => "$quantity $unit",
'time' => $time,
]);
Assert::assertSame($type, $bill->getType()->getName(), "Bill type mismatch: expected $type, got {$bill->getType()->getName()}");
Assert::assertSame($target, $bill->getTarget()->getFullName(), "Bill target mismatch: expected $target, got {$bill->getTarget()->getFullName()}");
Assert::assertEquals(bcmul($sum, 100), $bill->getSum()->getAmount(), "Bill sum mismatch: expected $sum, got {$bill->getSum()->getAmount()}");
Assert::assertSame($currency, $bill->getSum()->getCurrency()->getCode(), "Bill currency mismatch: expected $currency, got {$bill->getSum()->getCurrency()->getCode()}");
Assert::assertEquals((float)$quantity, (float)$bill->getQuantity()->getQuantity(), "Bill quantity mismatch: expected $quantity, got {$bill->getQuantity()->getQuantity()}");
Assert::assertEquals(strtolower($unit), strtolower($bill->getQuantity()->getUnit()->getName()), "Bill unit mismatch: expected $unit, got {$bill->getQuantity()->getUnit()->getName()}");
Assert::assertEquals(new DateTimeImmutable($time), $bill->getTime(), "Bill time mismatch: expected $time, got {$bill->getTime()->format(DATE_ATOM)}");
$billStart = $bill->getUsageInterval()->start();
$billEnd = $bill->getUsageInterval()->end();
Assert::assertEquals(new DateTimeImmutable($since), $billStart, "Bill since time mismatch: expected $since, got {$billStart->format(DATE_ATOM)}");
Assert::assertEquals(new DateTimeImmutable($till), $billEnd, "Bill till time mismatch: expected $till, got {$billEnd->format(DATE_ATOM)}");
}

public function findBill(array $params): BillInterface
{
$bills = $this->builder->findBills($params);
Expand Down