HP-2166/Store_Action_Usage_Interval_in_the_action_table#81
HP-2166/Store_Action_Usage_Interval_in_the_action_table#81SilverFire merged 5 commits intohiqdev:masterfrom ValeriyShnurovoy:HP-2166/Store_Action_Usage_Interval_in_the_action_table
Conversation
WalkthroughThe changes involve enhancements to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant AbstractAction
participant UsageInterval
User->>AbstractAction: Create instance with fractionOfMonth
AbstractAction->>UsageInterval: Call withMonthAndFraction(month, start, fractionOfMonth)
UsageInterval->>UsageInterval: Calculate effective end date based on fractionOfMonth
UsageInterval-->>AbstractAction: Return calculated end date
AbstractAction-->>User: Provide action with updated usage interval
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
src/action/ActionInterface.php (1)
86-86: Add PHPDoc for the new method.Please add documentation explaining the purpose and expected return value range of
getFractionOfMonth(). This will help other developers understand how this value affects billing calculations.+ /** + * Returns the fraction of month this action represents. + * @return float Value between 0 and 1 representing the portion of a month + */ public function getFractionOfMonth(): float;src/action/UsageInterval.php (1)
75-84: Verify billing calculations with partial month usage.The introduction of partial month calculations could impact billing amounts. Ensure that:
- Billing calculations correctly handle partial month usage
- Edge cases (e.g., very small fractions) don't cause rounding issues
- The change is backward compatible with existing billing records
Consider adding integration tests that verify:
- Billing amounts for partial months
- Rounding behavior for various fraction values
- Compatibility with historical billing data
tests/unit/action/UsageIntervalTest.php (1)
Line range hint
83-89: Consider rounding the fraction to reduce floating-point precision issuesThe fraction
0.17857142857142858(5/28 days) is using high precision which might lead to floating-point comparison issues in tests. Consider rounding to a reasonable number of decimal places (e.g., 4-6) for more reliable test assertions.Example modification:
- ['month' => '2023-02-01 00:00:00', 'start' => '2023-02-15 00:00:00', 'end' => '2023-02-20 00:00:00', 'fraction' => 0.17857142857142858], + ['month' => '2023-02-01 00:00:00', 'start' => '2023-02-15 00:00:00', 'end' => '2023-02-20 00:00:00', 'fraction' => 0.1786], [ 'start' => '2023-02-15 00:00:00', 'end' => '2023-02-20 00:00:00', - 'ratioOfMonth' => 0.17857142857142858, + 'ratioOfMonth' => 0.1786, 'seconds' => 432_000, 'secondsInMonth' => 2_419_200, ]src/action/AbstractAction.php (3)
58-59: Add property documentation and consider naming consistency.The new property should include PHPDoc documentation explaining its purpose and valid range. Also, consider using camelCase to match the naming convention of other properties in the class.
+ /** + * @var float Fraction of month (0.0 to 1.0) representing partial month usage + */ - protected float $fraction_of_month; + protected float $fractionOfMonth;
225-228: Add method documentation.Add PHPDoc comment to explain the method's purpose and return value meaning.
+ /** + * Returns the fraction of month (0.0 to 1.0) representing partial month usage. + * + * @return float The fraction of month between 0.0 and 1.0 + */ public function getFractionOfMonth(): float { return $this->fraction_of_month; }
Line range hint
58-248: Consider system-wide impact of fractional billing.The introduction of partial month usage tracking is a significant change that could affect:
- Billing calculations and proration
- Reporting and analytics
- API responses and documentation
- Database schema (storing float values)
Please ensure:
- Documentation is updated to reflect this new billing capability
- Related billing calculation components handle fractional months correctly
- Database schema can properly store and query float values
- API documentation is updated if this affects response formats
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/action/AbstractAction.php (5 hunks)
- src/action/ActionInterface.php (1 hunks)
- src/action/UsageInterval.php (2 hunks)
- tests/unit/action/UsageIntervalTest.php (8 hunks)
🔇 Additional comments (5)
src/action/ActionInterface.php (1)
86-86: Verify the impact of fractional month calculations.The introduction of fractional month calculations could significantly affect billing logic. Let's verify the implementation across the codebase.
✅ Verification successful
Based on the results, I can now provide a final response:
The fractional month calculation is well-integrated and has existing test coverage
The
getFractionOfMonth()method is:
- Defined in
ActionInterface.php- Implemented in
AbstractAction.phpwith proper fraction storage- Used in billing calculations through
FeatureContext.phpwith time-based calculations- Integrated with SQL functionality (days2quantity)
The implementation appears to be consistent with existing billing logic and has proper test coverage through Behat tests. No concerning side effects were found in the billing calculation system.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze the usage of fractional month calculations # Check for existing fraction calculations in billing logic echo "Checking for existing fraction calculations:" rg -l "fraction|partial|month" --type php # Look for potential billing calculation methods that might need updates echo -e "\nChecking for billing calculation methods:" ast-grep --pattern 'function calculate$$$($$$) { $$$ }' # Check for test coverage of the new functionality echo -e "\nChecking for related test files:" fd -e php test -e php spec | rg -i "fraction|interval|billing"Length of output: 1429
Script:
#!/bin/bash # Let's analyze the implementation and usage of getFractionOfMonth # Check the implementation of getFractionOfMonth echo "Checking getFractionOfMonth implementations:" ast-grep --pattern 'function getFractionOfMonth() { $$$ }' # Check for direct usage of getFractionOfMonth echo -e "\nChecking direct usage of getFractionOfMonth:" rg "getFractionOfMonth" -A 3 --type php # Check billing calculation logic in PriceChargesEstimator echo -e "\nChecking billing calculation in PriceChargesEstimator:" rg "class PriceChargesEstimator" -A 20 --type php # Check test coverage specifically for fraction calculations echo -e "\nChecking fraction-related test cases:" rg "function test.*Fraction" -A 3 --type phpLength of output: 3044
src/action/UsageInterval.php (1)
76-84: 🛠️ Refactor suggestionImprove robustness of interval calculation.
The current implementation has several potential issues:
- Using
strtotimeis less reliable than native DateTime methods- Manual interval string construction could be error-prone
- Potential timezone issues with timestamp calculations
Consider using DateTimeImmutable's native methods:
if ($fractionOfMonth === 1.0) { $calcEnd = $month->modify('+1 month'); } else { - $startTime = strtotime(($month->format('D, d M Y H:i:s O'))); - $finishTime = strtotime($nextMonth->format('D, d M Y H:i:s O')); - $interval = 'PT' . (($finishTime - $startTime) * $fractionOfMonth) . 'S'; - $calcEnd = $effectiveSince->add(new \DateInterval($interval)); + $monthDays = (int)$month->format('t'); + $secondsInMonth = $monthDays * 24 * 60 * 60; + $intervalSeconds = (int)round($secondsInMonth * $fractionOfMonth); + $calcEnd = $effectiveSince->add(new \DateInterval("PT{$intervalSeconds}S")); }Let's verify if this calculation is used consistently across the codebase:
tests/unit/action/UsageIntervalTest.php (2)
38-38: LGTM: Method signature update properly testedThe test correctly incorporates the new
fractionparameter in theUsageInterval::withinMonthcall, maintaining proper test coverage for the updated method signature.
Line range hint
50-61: LGTM: Comprehensive test coverage for fractionsThe test cases thoroughly cover various scenarios including:
- Full month usage (fraction = 1)
- Half month usage (fraction = 0.5)
- Custom fractional usage
- Edge cases with dates outside the month
Also applies to: 72-83
src/action/AbstractAction.php (1)
243-248: Verify UsageInterval class compatibility.The changes look correct, but we should verify that the
UsageInterval::withinMonthmethod properly handles the new fraction parameter.✅ Verification successful
UsageInterval class is properly updated to handle the fraction parameter
The verification confirms that:
- The
UsageInterval::withinMonthmethod is correctly defined with thefloat $fractionOfMonthparameter- The method is properly tested in
UsageIntervalTestwith test cases validating the fraction parameter- The implementation and test structure show the parameter is integrated into the class's functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if UsageInterval class has been updated to handle the fraction parameter ast-grep --pattern 'class UsageInterval { $$$ public static function withinMonth($_, $_, $_, float $fractionOfMonth) { $$$ } $$$ }'Length of output: 145
Script:
#!/bin/bash # Let's try a broader search for the UsageInterval class and its withinMonth method rg -A 10 "class UsageInterval" # Also search for withinMonth method declaration specifically rg -A 5 "function withinMonth" # Search for any usage of withinMonth to understand the expected parameters rg "withinMonth\("Length of output: 2334
Script:
#!/bin/bash # Let's check the implementation of withinMonth method to verify it handles the fraction parameter correctly rg -B 2 -A 15 "function withinMonth" src/action/UsageInterval.php # Also check the test cases to understand the expected behavior rg -B 2 -A 15 "testWithinMonth" tests/unit/action/UsageIntervalTest.phpLength of output: 1640
src/action/UsageInterval.php
Outdated
| ?DateTimeImmutable $end, | ||
| float $fractionOfMonth |
There was a problem hiding this comment.
Update method documentation and add parameter validation.
The new $fractionOfMonth parameter needs to be documented in the PHPDoc block. Also, consider adding validation to ensure the fraction is between 0 and 1.
Apply this diff:
/**
* Calculates the usage interval for the given month for the given start and end sale dates.
*
* @param DateTimeImmutable $month the month to calculate the usage interval for
* @param DateTimeImmutable $start the start date of the sale
* @param DateTimeImmutable|null $end the end date of the sale or null if the sale is active
+ * @param float $fractionOfMonth fraction of the month to calculate usage for (between 0 and 1)
* @return static
+ * @throws InvalidArgumentException if fraction is not between 0 and 1
*/
public static function withinMonth(
DateTimeImmutable $month,
DateTimeImmutable $start,
?DateTimeImmutable $end,
float $fractionOfMonth
): self {
+ if ($fractionOfMonth <= 0 || $fractionOfMonth > 1) {
+ throw new InvalidArgumentException('Fraction of month must be between 0 and 1');
+ }📝 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.
| ?DateTimeImmutable $end, | |
| float $fractionOfMonth | |
| /** | |
| * Calculates the usage interval for the given month for the given start and end sale dates. | |
| * | |
| * @param DateTimeImmutable $month the month to calculate the usage interval for | |
| * @param DateTimeImmutable $start the start date of the sale | |
| * @param DateTimeImmutable|null $end the end date of the sale or null if the sale is active | |
| * @param float $fractionOfMonth fraction of the month to calculate usage for (between 0 and 1) | |
| * @return static | |
| * @throws InvalidArgumentException if fraction is not between 0 and 1 | |
| */ | |
| public static function withinMonth( | |
| DateTimeImmutable $month, | |
| DateTimeImmutable $start, | |
| ?DateTimeImmutable $end, | |
| float $fractionOfMonth | |
| ): self { | |
| if ($fractionOfMonth <= 0 || $fractionOfMonth > 1) { | |
| throw new InvalidArgumentException('Fraction of month must be between 0 and 1'); | |
| } |
src/action/AbstractAction.php
Outdated
| ActionInterface $parent = null, | ||
| $fractionOfMonth = 0.0 |
There was a problem hiding this comment.
Add type hint and validate fraction range.
The fraction parameter should be type-hinted and validated to ensure it's within the valid range of 0.0 to 1.0.
public function __construct(
$id,
TypeInterface $type,
TargetInterface $target,
QuantityInterface $quantity,
CustomerInterface $customer,
DateTimeImmutable $time,
SaleInterface $sale = null,
ActionState $state = null,
ActionInterface $parent = null,
- $fractionOfMonth = 0.0
+ float $fractionOfMonth = 0.0
) {
+ if ($fractionOfMonth < 0.0 || $fractionOfMonth > 1.0) {
+ throw new \InvalidArgumentException('Fraction of month must be between 0.0 and 1.0');
+ }
$this->id = $id;
$this->type = $type;Also applies to: 85-85
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/behat/bootstrap/FeatureContext.php (1 hunks)
🔇 Additional comments (1)
tests/behat/bootstrap/FeatureContext.php (1)
191-196: Verify fraction calculation parameters and add validationThe fraction calculation logic has potential issues:
- The
getFractionOfMonthcall passes the same$timeparameter twice, which might not be the intended behavior:$fractionOfMonth = $this->getFractionOfMonth( $time, $time, $this->sale->getCloseTime() );
- Consider adding validation to ensure the fraction is positive and non-zero before applying it to the amount.
Let's verify the usage of
getFractionOfMonthacross the codebase:
| $this->action = new Action( | ||
| null, | ||
| $type, | ||
| $target, | ||
| $quantity, | ||
| $this->customer, | ||
| $time, | ||
| null, | ||
| null, | ||
| null, | ||
| $fractionOfMonth | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor Action constructor to use builder pattern or named arguments
The constructor call has several issues:
- Long parameter list (10 parameters)
- Multiple consecutive null parameters
- No named arguments despite PHP 8.0+ support
Consider these improvements:
- 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
+ );- 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();
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
src/action/UsageInterval.php (2)
85-92: Improve method documentation.The PHPDoc block needs several improvements:
- Fix the typo in
@param float $fractionOfMonthdescription ("manth" → "month")- Document the valid range for
$fractionOfMonth- Add
@throwsdocumentationApply this diff:
/** * 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 + * @param float $fractionOfMonth the fraction of month (must be between 0 and 1) * @return static + * @throws \InvalidArgumentException if fraction is not between 0 and 1 */
93-120: Consider addressing the constructor constraint concern.Responding to @SilverFire's comment about constructor constraints: The current implementation shifts the responsibility of fraction calculation to the caller. Consider providing both options:
- Keep the existing
withinMonthfor backward compatibility- Add a new method that calculates the fraction internally
Consider adding a method that calculates the fraction internally:
public static function withAutoFraction( DateTimeImmutable $month, DateTimeImmutable $start, ?DateTimeImmutable $end ): self { if ($end === null) { return self::withMonthAndFraction($month, $start, 1.0); } $monthStart = self::toMonth($month); $monthEnd = $monthStart->modify('+1 month'); $effectiveEnd = min($end, $monthEnd); $fraction = (float) $effectiveEnd->diff($monthStart)->days / (float) $monthStart->format('t'); return self::withMonthAndFraction($month, $start, $fraction); }This provides flexibility while maintaining the original functionality.
src/action/AbstractAction.php (1)
58-58: Add PHPDoc for the new property.Please add PHPDoc to explain the purpose of this property and document its valid range (0.0 to 1.0).
+ /** + * Represents the fraction of a month this action applies to. + * Value must be between 0.0 and 1.0, where: + * - 0.0 means no fraction (default) + * - 1.0 means whole month + * @var float + */ protected float $fractionOfMonth;tests/unit/action/UsageIntervalTest.php (2)
124-144: Add PHPDoc param types and parameter type hints.The test method could benefit from improved type safety and documentation:
/** * @dataProvider provideWithMonthAndFraction + * @param array{month: string, start: string, fraction: float} $constructor + * @param array{start?: string, end?: string, ratioOfMonth?: float, seconds?: int, secondsInMonth?: int, expectedException?: class-string<\Throwable>, expectedExceptionMessage?: string} $expectations */ - public function testWithMonthAndFraction(array $constructor, array $expectations): void + public function testWithMonthAndFraction(array $constructor, array $expectations): void
146-213: Add test cases for fraction edge cases.While the current test cases cover the main scenarios, consider adding the following edge cases for more robust testing:
- Negative fractions (should throw an exception)
- Fractions greater than 1 (should throw an exception or be capped)
- Invalid fraction values (NaN, Inf)
- Fraction precision handling (e.g., very small fractions)
Example test case to add:
yield 'When fraction is negative, an exception is thrown' => [ ['month' => '2023-02-01 00:00:00', 'start' => '2023-02-01 00:00:00', 'fraction' => -0.5], [ 'expectedException' => InvalidArgumentException::class, 'expectedExceptionMessage' => 'Fraction must be between 0 and 1', ] ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- src/action/AbstractAction.php (5 hunks)
- src/action/UsageInterval.php (1 hunks)
- tests/unit/action/UsageIntervalTest.php (1 hunks)
🔇 Additional comments (3)
src/action/AbstractAction.php (2)
74-74:⚠️ Potential issueAdd range validation for fractionOfMonth.
As mentioned in the previous review, please add validation to ensure the fraction is within the valid range.
public function __construct( // ... other parameters ... float $fractionOfMonth = 0.0 ) { + if ($fractionOfMonth < 0.0 || $fractionOfMonth > 1.0) { + throw new \InvalidArgumentException('Fraction of month must be between 0.0 and 1.0'); + } // ... other assignments ... $this->fractionOfMonth = $fractionOfMonth; }Also applies to: 85-85
243-254: LGTM! Verify UsageInterval method existence.The logic for handling fractional months is well-structured. However, let's verify that the
UsageInterval::withMonthAndFractionmethod exists and accepts these parameters.✅ Verification successful
✓ UsageInterval methods verified and correctly implemented
The
UsageInterval::withMonthAndFractionmethod exists and accepts the correct parameters:
DateTimeImmutable $monthDateTimeImmutable $startfloat $fractionOfMonthThe implementation properly handles fractional month calculations and is consistent with the usage in
AbstractAction.php.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify the existence of withMonthAndFraction method in UsageInterval class ast-grep --pattern 'class UsageInterval { $$$ public static function withMonthAndFraction($$$) { $$$ } $$$ }'Length of output: 123
Script:
#!/bin/bash # Let's try to find the UsageInterval class first fd UsageIntervalLength of output: 87
Script:
#!/bin/bash # Search for any static method calls to withMonthAndFraction rg "UsageInterval::withMonthAndFraction" -A 2Length of output: 589
Script:
#!/bin/bash # Search for any static method calls to withinMonth for comparison rg "UsageInterval::withinMonth" -A 2Length of output: 530
Script:
#!/bin/bash # Let's check the implementation of UsageInterval class cat src/action/UsageInterval.phpLength of output: 4581
tests/unit/action/UsageIntervalTest.php (1)
123-213: Implementation looks good overall!The new test method and its data provider are well-structured and provide good coverage for the
UsageInterval::withMonthAndFractionfunctionality. The implementation:
- Follows existing test patterns
- Covers key scenarios
- Maintains consistency with other tests
- Properly validates the behavior described in the PR
src/action/UsageInterval.php
Outdated
| $startTime = strtotime(($month->format('D, d M Y H:i:s O'))); | ||
| $finishTime = strtotime($nextMonth->format('D, d M Y H:i:s O')); | ||
| $interval = 'PT' . (($finishTime - $startTime) * $fractionOfMonth) . 'S'; | ||
| $effectiveTill = $effectiveSince->add(new \DateInterval($interval)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor interval calculation to use DateTimeImmutable methods.
The current implementation uses strtotime() and string manipulation, which is less reliable than native DateTimeImmutable methods. Also, the interval calculation could be simplified.
Consider this alternative implementation:
- $startTime = strtotime(($month->format('D, d M Y H:i:s O')));
- $finishTime = strtotime($nextMonth->format('D, d M Y H:i:s O'));
- $interval = 'PT' . (($finishTime - $startTime) * $fractionOfMonth) . 'S';
- $effectiveTill = $effectiveSince->add(new \DateInterval($interval));
+ $monthDays = (int) $month->format('t');
+ $days = (int) round($monthDays * $fractionOfMonth);
+ $effectiveTill = $effectiveSince->modify(sprintf('+%d days', $days));📝 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.
| $startTime = strtotime(($month->format('D, d M Y H:i:s O'))); | |
| $finishTime = strtotime($nextMonth->format('D, d M Y H:i:s O')); | |
| $interval = 'PT' . (($finishTime - $startTime) * $fractionOfMonth) . 'S'; | |
| $effectiveTill = $effectiveSince->add(new \DateInterval($interval)); | |
| } | |
| $monthDays = (int) $month->format('t'); | |
| $days = (int) round($monthDays * $fractionOfMonth); | |
| $effectiveTill = $effectiveSince->modify(sprintf('+%d days', $days)); | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
tests/unit/action/UsageIntervalTest.php (1)
124-213: Consider adding more edge cases and improving test descriptions.
The test coverage is comprehensive and well-structured. However, consider these improvements:
-
Add more edge cases for fraction values:
- Test with very small fraction (e.g., 0.000001)
- Test with fraction close to 1 (e.g., 0.999999)
-
Make test case descriptions more specific:
- Include the fraction value in the description
- Clarify the expected behavior in the description
Example of improved test case description:
- yield 'When start and end dates are within a month, the interval is a fraction of month' => [
+ yield 'With fraction 0.17857, interval spans 5 days (Feb 15-20) within the month' => [📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- src/action/UsageInterval.php (1 hunks)
- tests/unit/action/UsageIntervalTest.php (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/action/UsageInterval.php
🧰 Additional context used
📓 Learnings (1)
tests/unit/action/UsageIntervalTest.php (1)
Learnt from: SilverFire
PR: hiqdev/php-billing#81
File: src/action/UsageInterval.php:93-106
Timestamp: 2024-10-29T10:44:02.367Z
Learning: Tests should cover invalid parameter values, such as for the `$fractionOfMonth` parameter in `withMonthAndFraction` method.
| /** | ||
| * @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], | ||
| ]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance invalid value testing.
Good implementation of invalid value testing as requested. Consider these additions:
- 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/action/AbstractAction.php (2)
58-59: Add PHPDoc for the fractionOfMonth property.
Please add PHPDoc explaining the purpose of this property and its valid range (0.0 to 1.0).
+ /**
+ * Represents the fraction of a month this action applies to.
+ * Value must be between 0.0 (none) and 1.0 (full month).
+ * @var float
+ */
protected float $fractionOfMonth;243-249: Consider edge case handling for fraction comparison.
The condition $this->getFractionOfMonth() > 0 might miss edge cases. Consider using a small epsilon value for floating-point comparison.
- if ($this->getFractionOfMonth() > 0) {
+ if ($this->getFractionOfMonth() > 0.000001) { // PHP_FLOAT_EPSILON or similar📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- src/action/AbstractAction.php (5 hunks)
🔇 Additional comments (2)
src/action/AbstractAction.php (2)
225-228: LGTM!
The getter method is well-implemented with proper return type declaration.
243-254: Verify null safety in the usage interval calculation.
Please ensure that $this->getSale() cannot be null when accessing its methods. Consider adding explicit null checks or documenting the assumptions about sale presence.
2 participants
Summary by CodeRabbit
Release Notes
New Features
UsageIntervalclass with a method to calculate intervals based on a fraction of the month.Bug Fixes
Tests