Skip to content

HP-2166/Store_Action_Usage_Interval_in_the_action_table #81

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
SilverFire merged 5 commits into from
Oct 30, 2024
Merged

HP-2166/Store_Action_Usage_Interval_in_the_action_table #81

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
89560f9
HP-2166/Store_Action_Usage_Interval_in_the_action_table
Oct 25, 2024
df66a14
HP-2166/Fixed behat
Oct 25, 2024
96c368d
HP-2166/Fixed after review
Oct 28, 2024
ab5edab
HP-2166/Added exclude value of fraction
Oct 30, 2024
93e0657
HP-2166/Changed property name
Oct 30, 2024
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
23 changes: 21 additions & 2 deletions src/action/AbstractAction.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ abstract class AbstractAction implements \JsonSerializable, ActionInterface
/** @var ActionInterface */
protected $parent;

protected float $fractionOfMonth;

/**
* @param SaleInterface $sale
* @param ActionInterface $parent
Expand All @@ -68,7 +70,8 @@ public function __construct(
DateTimeImmutable $time,
SaleInterface $sale = null,
ActionState $state = null,
ActionInterface $parent = null
ActionInterface $parent = null,
float $fractionOfMonth = 0.0
) {
$this->id = $id;
$this->type = $type;
Expand All @@ -79,6 +82,7 @@ public function __construct(
$this->sale = $sale;
$this->state = $state;
$this->parent = $parent;
$this->fractionOfMonth = $fractionOfMonth;
}

/**
Expand Down Expand Up @@ -218,6 +222,10 @@ public function setSale(SaleInterface $sale)
$this->sale = $sale;
}

public function getFractionOfMonth(): float
{
return $this->fractionOfMonth;
}
/**
* {@inheritdoc}
*/
Expand All @@ -232,6 +240,17 @@ public function getUsageInterval(): UsageInterval
return UsageInterval::wholeMonth($this->getTime());
}

return UsageInterval::withinMonth($this->getTime(), $this->getSale()->getTime(), $this->getSale()->getCloseTime());
if ($this->getFractionOfMonth() > 0) {
return UsageInterval::withMonthAndFraction(
$this->getTime(),
$this->getSale()->getTime(),
$this->getFractionOfMonth()
);
}
return UsageInterval::withinMonth(
$this->getTime(),
$this->getSale()->getTime(),
$this->getSale()->getCloseTime()
);
}
}
2 changes: 2 additions & 0 deletions src/action/ActionInterface.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,4 +82,6 @@ public function hasSale();
public function setSale(SaleInterface $sale);

public function getUsageInterval(): UsageInterval;

public function getFractionOfMonth(): float;
}
39 changes: 39 additions & 0 deletions src/action/UsageInterval.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,45 @@ public static function withinMonth(
);
}

/**
* Calculates the usage interval for the given month for the given start date and fraction of month value.
*
* @param DateTimeImmutable $month the month to calculate the usage interval for
* @param DateTimeImmutable $start the start date of the sale
* @param float $fractionOfMonth the fraction of manth
* @return static
*/
public static function withMonthAndFraction(
DateTimeImmutable $month,
DateTimeImmutable $start,
float $fractionOfMonth
): self {
if ($fractionOfMonth < 0 || $fractionOfMonth > 1) {
throw new InvalidArgumentException('Fraction of month must be between 0 and 1');
}
$month = self::toMonth($month);
$nextMonth = $month->modify('+1 month');

if ($start >= $nextMonth) {
$start = $month;
}

$effectiveSince = max($start, $month);

if ($fractionOfMonth === 1.0) {
$effectiveTill = $month->modify('+1 month');
} else {
$monthDays = (int) $month->format('t');
$days = (int) round($monthDays * $fractionOfMonth);
$effectiveTill = $effectiveSince->modify(sprintf('+%d days', $days));
}

return new self(
$effectiveSince,
$effectiveTill,
);
}

public function start(): DateTimeImmutable
{
return $this->start;
Expand Down
17 changes: 15 additions & 2 deletions tests/behat/bootstrap/FeatureContext.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,14 +188,27 @@ public function actionIs(string $target, string $type, float $amount, string $un
$type = Type::anyId($type);
$target = new Target(Target::ANY, $target);
$time = new DateTimeImmutable($date);
$fractionOfMonth = 1;
if ($this->sale->getCloseTime() instanceof DateTimeImmutable) {
$amount = $amount * $this->getFractionOfMonth(
$fractionOfMonth = $this->getFractionOfMonth(
$time, $time, $this->sale->getCloseTime()
);
$amount = $amount * $fractionOfMonth;
}
$quantity = Quantity::create($unit, $amount);

$this->action = new Action(null, $type, $target, $quantity, $this->customer, $time);
$this->action = new Action(
null,
$type,
$target,
$quantity,
$this->customer,
$time,
null,
null,
null,
$fractionOfMonth
);
Comment on lines +200 to +211
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Refactor Action constructor to use builder pattern or named arguments

The constructor call has several issues:

  1. Long parameter list (10 parameters)
  2. Multiple consecutive null parameters
  3. No named arguments despite PHP 8.0+ support

Consider these improvements:

  1. Use named arguments (PHP 8.0+):
-        $this->action = new Action(
-            null,
-            $type,
-            $target,
-            $quantity,
-            $this->customer,
-            $time,
-            null,
-            null,
-            null,
-            $fractionOfMonth
-        );
+        $this->action = new Action(
+            id: null,
+            type: $type,
+            target: $target,
+            quantity: $quantity,
+            customer: $this->customer,
+            time: $time,
+            parent: null,
+            sale: null,
+            bill: null,
+            fractionOfMonth: $fractionOfMonth
+        );
  1. Or better, implement a builder pattern:
class ActionBuilder {
    private ?string $id = null;
    private ?Type $type = null;
    // ... other properties

    public function withType(Type $type): self {
        $this->type = $type;
        return $this;
    }
    // ... other with* methods

    public function build(): Action {
        return new Action(
            $this->id,
            $this->type,
            // ... other properties
        );
    }
}

// Usage:
$this->action = (new ActionBuilder())
    ->withType($type)
    ->withTarget($target)
    ->withQuantity($quantity)
    ->withCustomer($this->customer)
    ->withTime($time)
    ->withFractionOfMonth($fractionOfMonth)
    ->build();

}

private function getFractionOfMonth(DateTimeImmutable $month, DateTimeImmutable $startTime, DateTimeImmutable $endTime): float
Expand Down
114 changes: 114 additions & 0 deletions tests/unit/action/UsageIntervalTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,4 +120,118 @@ public function provideWithinMonth()
]
];
}

/**
* @dataProvider provideWithMonthAndFraction
*/
public function testWithMonthAndFraction(array $constructor, array $expectations): void
{
$month = new DateTimeImmutable($constructor['month']);
$start = new DateTimeImmutable($constructor['start']);

if (isset($expectations['expectedException'])) {
$this->expectException($expectations['expectedException']);
$this->expectExceptionMessage($expectations['expectedExceptionMessage']);
}

$interval = UsageInterval::withMonthAndFraction($month, $start, $constructor['fraction']);

$this->assertEquals($expectations['start'], $interval->start()->format("Y-m-d H:i:s"));
$this->assertEquals($expectations['end'], $interval->end()->format("Y-m-d H:i:s"));
$this->assertSame($expectations['ratioOfMonth'], $interval->ratioOfMonth());
$this->assertSame($expectations['seconds'], $interval->seconds());
$this->assertSame($expectations['secondsInMonth'], $interval->secondsInMonth());
}

public function provideWithMonthAndFraction()
{
yield 'For a start and end dates outside the month, the interval is the whole month' => [
['month' => '2023-02-01 00:00:00', 'start' => '2023-01-01 00:00:00', 'fraction' => 1],
[
'start' => '2023-02-01 00:00:00',
'end' => '2023-03-01 00:00:00',
'ratioOfMonth' => 1.0,
'seconds' => 2_419_200,
'secondsInMonth' => 2_419_200,
]
];

yield 'When start date is greater than a month, the interval is a fraction of month' => [
['month' => '2023-02-01 00:00:00', 'start' => '2023-02-15 00:00:00', 'fraction' => 0.5],
[
'start' => '2023-02-15 00:00:00',
'end' => '2023-03-01 00:00:00',
'ratioOfMonth' => 0.5,
'seconds' => 1_209_600,
'secondsInMonth' => 2_419_200,
]
];

yield 'When end date is less than a month, the interval is a fraction of month' => [
['month' => '2023-02-01 00:00:00', 'start' => '2021-10-02 19:01:10', 'fraction' => 0.5],
[
'start' => '2023-02-01 00:00:00',
'end' => '2023-02-15 00:00:00',
'ratioOfMonth' => 0.5,
'seconds' => 1_209_600,
'secondsInMonth' => 2_419_200,
]
];

yield 'When start and end dates are within a month, the interval is a fraction of month' => [
['month' => '2023-02-01 00:00:00', 'start' => '2023-02-15 00:00:00', 'fraction' => 0.17857142857142858],
[
'start' => '2023-02-15 00:00:00',
'end' => '2023-02-20 00:00:00',
'ratioOfMonth' => 0.17857142857142858,
'seconds' => 432_000,
'secondsInMonth' => 2_419_200,
]
];

yield 'When start date is greater than current month, the interval is zero' => [
['month' => '2023-02-01 00:00:00', 'start' => '2023-03-15 00:00:00', 'fraction' => 0],
[
'start' => '2023-02-01 00:00:00',
'end' => '2023-02-01 00:00:00',
'ratioOfMonth' => 0.0,
'seconds' => 0,
'secondsInMonth' => 2_419_200,
]
];

yield 'When end date is less than current month, the interval is zero' => [
['month' => '2023-02-01 00:00:00', 'start' => '2021-10-02 19:01:10', 'fraction' => 0],
[
'start' => '2023-02-01 00:00:00',
'end' => '2023-02-01 00:00:00',
'ratioOfMonth' => 0.0,
'seconds' => 0,
'secondsInMonth' => 2_419_200,
]
];
}

/**
* @dataProvider provideInvalidFractionOfMonthValues
*/
public function testWithMonthAndFractionInvalidValues(float $fractionOfMonth): void
{
$month = new DateTimeImmutable('2023-01-01');
$start = new DateTimeImmutable('2023-01-15');

$this->expectException(\InvalidArgumentException::class);
$this->expectExceptionMessage('Fraction of month must be between 0 and 1');

UsageInterval::withMonthAndFraction($month, $start, $fractionOfMonth);
}

public function provideInvalidFractionOfMonthValues(): array
{
return [
[-0.1],
[1.1],
[2.0],
];
}
Comment on lines +215 to +236
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot Oct 30, 2024

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Enhance invalid value testing.

Good implementation of invalid value testing as requested. Consider these additions:

  1. Test more edge cases:
    • Non-numeric values (NaN, INF)
    • String values that PHP would attempt to convert
    • Extremely large/small values

Add these test cases to provideInvalidFractionOfMonthValues:

     public function provideInvalidFractionOfMonthValues(): array
     {
         return [
             [-0.1],
             [1.1],
             [2.0],
+            [NAN],
+            [INF],
+            [-INF],
+            [PHP_FLOAT_MAX],
+            [PHP_FLOAT_MIN],
         ];
     }

Also, consider testing the exact error message format:

-        $this->expectExceptionMessage('Fraction of month must be between 0 and 1');
+        $this->expectExceptionMessage(sprintf(
+            'Fraction of month must be between 0 and 1, got: %f',
+            $fractionOfMonth
+        ));

Committable suggestion was skipped due to low confidence.

}