HP-2512 Cleaned up BillingRegistryService#102
Conversation
|
Warning Rate limit exceeded@VadymHrechukha has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 25 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis change consolidates tariff and behavior registry interfaces into a single Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant BillingRegistryService
Client->>BillingRegistryService: getTariffTypeDefinitionByTariffName(tariffName)
BillingRegistryService->>BillingRegistryService: Lookup tariff type by name using equalsName()
alt Not found
BillingRegistryService-->>Client: Throw TariffTypeDefinitionNotFoundException
else Found
BillingRegistryService-->>Client: Return TariffTypeDefinitionInterface
end
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/product/Application/BillingRegistryService.php(1 hunks)src/product/Application/BillingRegistryServiceInterface.php(1 hunks)src/product/Application/BillingRegistryTariffServiceInterface.php(1 hunks)src/product/Application/BillingRegistryTariffServiseInterface.php(0 hunks)tests/unit/product/Application/BillingRegistryTariffServiceTest.php(2 hunks)
💤 Files with no reviewable changes (1)
- src/product/Application/BillingRegistryTariffServiseInterface.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/unit/product/Application/BillingRegistryTariffServiceTest.php (2)
src/product/Application/BillingRegistryTariffServiceInterface.php (1)
getTariffTypeDefinitionByName(15-15)src/product/Application/BillingRegistryService.php (1)
getTariffTypeDefinitionByName(52-61)
src/product/Application/BillingRegistryTariffServiceInterface.php (2)
src/product/Exception/TariffTypeDefinitionNotFoundException.php (1)
TariffTypeDefinitionNotFoundException(7-10)src/product/Application/BillingRegistryService.php (1)
getTariffTypeDefinitionByName(52-61)
🪛 GitHub Actions: phpunit-tests
tests/unit/product/Application/BillingRegistryTariffServiceTest.php
[error] 58-58: Error: Call to undefined method hiqdev\php\billing\product\Application\BillingRegistryService::hasBehaviour().
🔇 Additional comments (4)
tests/unit/product/Application/BillingRegistryTariffServiceTest.php (1)
41-41: LGTM! Method name updated correctly.The test correctly uses the renamed method
getTariffTypeDefinitionByName.src/product/Application/BillingRegistryServiceInterface.php (1)
10-10: LGTM! Interface name typo corrected.The interface now correctly extends
BillingRegistryTariffServiceInterfaceinstead of the misspelled version.src/product/Application/BillingRegistryTariffServiceInterface.php (1)
1-16: LGTM! Well-structured interface with proper documentation.The new interface is clean, focused, and properly documented with exception handling. The PHPDoc correctly indicates the thrown exception type.
src/product/Application/BillingRegistryService.php (1)
52-61: LGTM! Improved method signature and error handling.The changes enhance the API design:
- Non-nullable return type makes the contract clearer
- Exception throwing provides better error handling than returning null
- Using
equalsName()method is more appropriate than direct string comparison
tests/unit/product/Application/BillingRegistryTariffServiceTest.php
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/product/invoice/Representation.php (1)
35-38: Consider adding null safety check.The
getType()method might be called beforesetType()is invoked, which could result in accessing an uninitialized property.Consider adding validation to ensure the type is set before returning it:
public function getType(): TypeInterface { + if (!isset($this->type)) { + throw new InvalidRepresentationException('Type must be set before accessing it.'); + } return $this->type; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/product/invoice/Representation.php(1 hunks)tests/unit/product/Application/BillingRegistryServiceTest.php(2 hunks)tests/unit/product/invoice/TestRepresentation.php(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/unit/product/invoice/TestRepresentation.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
tests/unit/product/Application/BillingRegistryServiceTest.php (1)
tests/unit/product/invoice/TestRepresentation.php (1)
TestRepresentation(7-9)
src/product/invoice/Representation.php (1)
src/product/invoice/InvalidRepresentationException.php (1)
InvalidRepresentationException(7-10)
🪛 GitHub Actions: phpunit-tests
tests/unit/product/Application/BillingRegistryServiceTest.php
[error] 54-54: hiqdev\php\billing\product\invoice\DuplicateRepresentationException: Duplicate 'TestRepresentation:dummy' representation causing test failure in testGetRepresentationsByInterfaceReturnsAllRepresentations.
🔇 Additional comments (3)
src/product/invoice/Representation.php (3)
7-11: Excellent documentation explaining the design decision.The comment clearly explains why the class is abstract and guides developers to create their own implementations to distinguish between multiple representations.
16-21: Robust SQL validation with proper error handling.The constructor correctly validates that the SQL string is not empty or whitespace-only, throwing an appropriate exception with a clear error message.
28-33: Proper fluent interface implementation.The
setTypemethod correctly returnsRepresentationInterfaceto maintain the fluent interface pattern, allowing method chaining.
| ->attach(new TestRepresentation("Invoice")) | ||
| ->attach(new TestRepresentation("Payment Request")) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix the duplicate representation issue causing test failure.
The pipeline failure indicates a DuplicateRepresentationException with "TestRepresentation:dummy", but the test creates instances with different parameters ("Invoice" and "Payment Request"). This suggests the representation identification mechanism doesn't use the constructor parameter as expected.
Please investigate how representations are identified internally. Run this script to examine the TestRepresentation implementation and related identification logic:
🏁 Script executed:
#!/bin/bash
# Description: Examine TestRepresentation implementation and representation identification mechanism
# Check TestRepresentation implementation
echo "=== TestRepresentation class implementation ==="
cat tests/unit/product/invoice/TestRepresentation.php
# Look for any identification or registry mechanisms
echo -e "\n=== Searching for representation identification logic ==="
rg -A 10 -B 5 "attach.*Representation" src/
# Check for any default identification in parent classes
echo -e "\n=== Searching for representation identification patterns ==="
rg -A 5 -B 5 "representation.*dummy|dummy.*representation" src/ tests/Length of output: 1975
🏁 Script executed:
#!/bin/bash
# Inspect core representation logic and uniqueness enforcement
echo "=== Representation implementation ==="
rg -n "class Representation" -A200 src/product/invoice/Representation.php
echo -e "\n=== UniquenessGuard implementation ==="
rg -n "class UniquenessGuard" -A200 src/product/invoice/UniquenessGuard.phpLength of output: 1116
🏁 Script executed:
#!/bin/bash
# 1. Find where uniqueness is enforced in the codebase
echo "=== ensureUnique occurrences ==="
rg -n "ensureUnique" -A5 src
# 2. Locate the DuplicateRepresentationException class
echo -e "\n=== DuplicateRepresentationException declaration ==="
rg -n "class DuplicateRepresentationException" -A20 src
# 3. List all guard-like classes to find UniquenessGuard equivalent
echo -e "\n=== Guard-like classes in src ==="
rg -n "class .*Guard" -n src
# 4. Inspect the test around the failing lines
echo -e "\n=== BillingRegistryServiceTest (first 200 lines) ==="
sed -n '1,200p' tests/unit/product/Application/BillingRegistryServiceTest.phpLength of output: 8781
Include SQL in representation key to avoid false duplicates
The uniqueness guard currently generates keys using only the representation class and type, so both new TestRepresentation("Invoice") and new TestRepresentation("Payment Request") yield the same "TestRepresentation:dummy" key. To fix this, extend the key to include the SQL string:
• File needing update:
- src/product/invoice/RepresentationUniquenessGuard.php
• Proposed diff:
--- a/src/product/invoice/RepresentationUniquenessGuard.php
+++ b/src/product/invoice/RepresentationUniquenessGuard.php
@@ -23,7 +23,12 @@ class RepresentationUniquenessGuard
private function generateKey(RepresentationInterface $representation): string
{
- return \get_class($representation) . ':' . $representation->getType()->getName();
+ // include SQL so different queries aren’t treated as duplicates
+ return \sprintf(
+ '%s:%s:%s',
+ \get_class($representation),
+ $representation->getType()->getName(),
+ $representation->getSql()
+ );
}
}This change ensures each representation’s SQL is factored into uniqueness, resolving the DuplicateRepresentationException in tests.
🧰 Tools
🪛 GitHub Actions: phpunit-tests
[error] 54-54: hiqdev\php\billing\product\invoice\DuplicateRepresentationException: Duplicate 'TestRepresentation:dummy' representation causing test failure in testGetRepresentationsByInterfaceReturnsAllRepresentations.
🤖 Prompt for AI Agents
In src/product/invoice/RepresentationUniquenessGuard.php around the logic
generating uniqueness keys, the current key only includes the representation
class and type, causing different TestRepresentation instances with distinct SQL
strings to be seen as duplicates. Modify the key generation method to append the
SQL string from each representation to the uniqueness key, ensuring that
representations with different SQL are uniquely identified and preventing
DuplicateRepresentationException in tests.
… we don't have BillingRegistryTariffService class
…nse and only confusing - BillingRegistryBehaviorServiceInterface - BillingRegistryTariffServiceInterface
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/product/Application/BillingRegistryServiceInterface.php (1)
44-45: Minor formatting: Remove extra blank line.There's an unnecessary extra blank line that can be removed for consistency.
- - +
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/product/Application/BillingRegistryBehaviorServiceInterface.php(0 hunks)src/product/Application/BillingRegistryService.php(2 hunks)src/product/Application/BillingRegistryServiceInterface.php(2 hunks)tests/unit/product/Application/BillingRegistryServiceTest.php(6 hunks)tests/unit/product/Application/BillingRegistryTariffServiceTest.php(0 hunks)
💤 Files with no reviewable changes (2)
- tests/unit/product/Application/BillingRegistryTariffServiceTest.php
- src/product/Application/BillingRegistryBehaviorServiceInterface.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/product/Application/BillingRegistryServiceTest.php
- src/product/Application/BillingRegistryService.php
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/product/Application/BillingRegistryServiceInterface.php (3)
src/product/Exception/TariffTypeDefinitionNotFoundException.php (1)
TariffTypeDefinitionNotFoundException(7-10)src/product/quantity/FractionQuantityData.php (1)
FractionQuantityData(7-14)src/product/Application/BillingRegistryService.php (4)
getTariffTypeDefinitionByName(57-66)getBehavior(85-114)getBehaviors(129-146)findPriceTypeDefinitionsByBehavior(161-168)
🔇 Additional comments (5)
src/product/Application/BillingRegistryServiceInterface.php (5)
10-10: LGTM: Import correctly added for new exception.The import for
TariffTypeDefinitionNotFoundExceptionis properly added to support the newgetTariffTypeDefinitionByNamemethod.
29-34: LGTM: Method signature matches implementation.The
getTariffTypeDefinitionByNamemethod is correctly declared with proper documentation and exception handling that matches the implementation shown in the relevant code snippets.
36-43: LGTM: Well-documented behavior retrieval method.The
getBehaviormethod declaration is correct with comprehensive documentation and proper exception declarations that align with the implementation.
46-52: LGTM: Generator return type properly documented.The
getBehaviorsmethod correctly uses Generator return type with proper generic type annotation and clear documentation.
54-60: LGTM: Price type definition finder properly declared.The
findPriceTypeDefinitionsByBehaviormethod is correctly declared with appropriate Generator return type and clear documentation.
…TypeName() method
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/product/Application/BillingRegistryServiceInterface.php (1)
23-28: Fix typo in deprecation comment.The deprecation comment references
getPriceTypeDefinitionByTypeName()but the actual method name isgetPriceTypeDefinitionByPriceTypeName().- * @deprecated - please use getPriceTypeDefinitionByTypeName() method instead + * @deprecated - please use getPriceTypeDefinitionByPriceTypeName() method insteadsrc/product/Application/BillingRegistryService.php (1)
96-99: Consider relocating private method for better organization.The
convertStringTypeToTypemethod is positioned between public methods, which disrupts the logical flow. Consider moving it after all public methods or grouping private methods together.} + public function getBehaviors(string $behaviorClassWrapper): \Generator + { + // ... existing implementation + } + + public function getAggregate(string $type): AggregateInterface + { + // ... existing implementation + } + + public function getPriceTypeDefinitionByPriceTypeName(string $typeName): PriceTypeDefinitionInterface + { + // ... existing implementation + } + + public function findPriceTypeDefinitionsByBehavior(string $behaviorClassWrapper): \Generator + { + // ... existing implementation + } + private function convertStringTypeToType(string $type): TypeInterface { return Type::anyId($type); } - - private function findBehaviorInPriceType(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/product/Application/BillingRegistryService.php(5 hunks)src/product/Application/BillingRegistryServiceInterface.php(1 hunks)src/product/Exception/PriceTypeDefinitionNotFoundException.php(1 hunks)tests/unit/product/Application/BillingRegistryServiceTest.php(6 hunks)tests/unit/product/Domain/Model/DummyTariffType.php(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- tests/unit/product/Domain/Model/DummyTariffType.php
- src/product/Exception/PriceTypeDefinitionNotFoundException.php
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unit/product/Application/BillingRegistryServiceTest.php
🧰 Additional context used
🧬 Code Graph Analysis (2)
src/product/Application/BillingRegistryServiceInterface.php (4)
src/product/Exception/TariffTypeDefinitionNotFoundException.php (1)
TariffTypeDefinitionNotFoundException(7-10)src/product/Application/BillingRegistryService.php (7)
getRepresentationsByType(25-52)getAggregate(133-136)getPriceTypeDefinitionByPriceTypeName(138-152)getTariffTypeDefinitionByTariffName(54-63)getBehavior(65-94)getBehaviors(114-131)findPriceTypeDefinitionsByBehavior(154-161)src/product/price/PriceTypeDefinitionInterface.php (2)
getAggregate(64-64)type(49-49)src/product/price/PriceTypeDefinition.php (2)
getAggregate(213-220)type(161-164)
src/product/Application/BillingRegistryService.php (12)
src/product/Exception/PriceTypeDefinitionNotFoundException.php (1)
PriceTypeDefinitionNotFoundException(7-10)src/product/Application/BillingRegistryServiceInterface.php (3)
getTariffTypeDefinitionByTariffName(37-37)getPriceTypeDefinitionByPriceTypeName(30-30)getAggregate(28-28)src/product/BillingRegistryInterface.php (2)
getTariffTypeDefinitions(19-19)priceTypes(14-14)src/product/BillingRegistry.php (2)
getTariffTypeDefinitions(21-24)priceTypes(26-33)src/product/TariffTypeDefinitionInterface.php (1)
tariffType(18-18)src/product/TariffTypeDefinition.php (1)
tariffType(38-41)src/product/Domain/Model/TariffTypeInterface.php (1)
equalsName(13-13)tests/unit/product/Domain/Model/TariffType.php (1)
equalsName(14-17)src/product/Exception/TariffTypeDefinitionNotFoundException.php (1)
TariffTypeDefinitionNotFoundException(7-10)src/product/price/PriceTypeDefinitionInterface.php (3)
type(49-49)getAggregate(64-64)hasType(51-51)src/product/price/PriceTypeDefinition.php (3)
type(161-164)getAggregate(213-220)hasType(166-169)src/type/Type.php (2)
Type(18-103)anyId(99-102)
🔇 Additional comments (15)
src/product/Application/BillingRegistryServiceInterface.php (6)
30-63: LGTM: Well-designed consolidated interface.The consolidation of tariff and behavior registry methods into a single interface is clean and well-documented. The consistent use of specific exception types and comprehensive PHPDoc comments improve the API's usability and maintainability.
10-10: LGTM!The import is correctly added to support the new method that throws
TariffTypeDefinitionNotFoundException.
32-37: LGTM!The method documentation is comprehensive and correctly includes the
@throwsannotation.
39-46: LGTM!Excellent documentation with comprehensive parameter descriptions and proper exception annotations.
49-55: LGTM!Well-documented method with proper Generator type annotation and clear purpose description.
57-63: LGTM!Consistent documentation style with proper Generator type annotation and clear description.
src/product/Application/BillingRegistryService.php (9)
31-40: LGTM: Code formatting improvement.The multi-line formatting of the conditional statement improves readability while maintaining the same logic.
54-63: LGTM: Improved error handling with specific exceptions.The method name change to
getTariffTypeDefinitionByTariffNameand the switch from returning null to throwingTariffTypeDefinitionNotFoundExceptionprovides better error handling and aligns with the new interface contract.
135-152: LGTM: Clean delegation and robust error handling.The
getAggregatemethod properly delegates to the newgetPriceTypeDefinitionByPriceTypeNamemethod, maintaining backward compatibility. The new method implementation correctly uses type conversion and provides clear error messages when price type definitions are not found.
10-10: LGTM!The import is correctly added to support the new exception handling in
getPriceTypeDefinitionByPriceTypeNamemethod.
31-38: LGTM!Good formatting improvement that enhances readability while preserving the original logic.
54-63: LGTM!Excellent improvement to throw exceptions instead of returning null, which provides better type safety and makes error handling more explicit.
96-99: LGTM!Clean and simple implementation using the appropriate
Type::anyId()static method for string-to-type conversion.
135-135: LGTM!Good refactoring that eliminates code duplication by delegating to the new method while preserving functionality.
138-152: LGTM!Well-structured implementation that follows a clear pattern: convert input, iterate through options, and throw meaningful exceptions when not found.
2 participants
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
Chores