HP-2883: Implement Business Central bills import service#114
HP-2883: Implement Business Central bills import service#114VadymHrechukha wants to merge 7 commits intohiqdev:masterfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdded three new bill value objects (BillSource, BillTxn, BillReversesId), extended Bill and BillInterface to store/expose optional source/txn/reversesId, added setter/getter for reversesId, and made multiple constructor parameters explicitly nullable; adjusted SinglePrice parameter order and updated related usages/tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (1)
src/bill/Bill.php (1)
260-268: Asymmetric mutation API: setter only forreversesId, not forsourceandtxn
setReversesId()allows post-construction mutation of$reversesId, but$sourceand$txnhave no equivalent setters. If mutable post-construction state is intentional only for the reversal use-case, a short inline comment explaining this would prevent confusion for future maintainers. Otherwise, consider making the API consistent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/bill/Bill.php` around lines 260 - 268, The class exposes mutation for $reversesId via setReversesId but has getters getSource() and getTxn() without corresponding setters, creating an asymmetric API; either add setters setSource(BillSource $source): void and setTxn(BillTxn $txn): void (or nullable variants) to mirror setReversesId, or if immutability for source/txn is intentional, add a concise inline comment near the properties and the getSource()/getTxn() methods explaining that only reversesId is mutable post-construction and why (referencing $reversesId, setReversesId, $source, getSource, $txn, getTxn).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/bill/Bill.php`:
- Around line 290-297: The setter setReversesId is missing the nullable type
hint documented in the docblock; change its signature to accept ?BillReversesId
(i.e. public function setReversesId(?BillReversesId $reversesId): void) so it
matches the constructor's BillReversesId $reversesId = null intent and prevents
arbitrary values, and ensure any necessary import/use for BillReversesId is
present and the docblock stays consistent.
In `@src/bill/BillReversesId.php`:
- Around line 9-19: The getId(): int return type is incompatible with the
protected $id (int|null) and the public __construct($id = null); either make the
return nullable by changing getId(): ?int to reflect the property, or prevent
null construction by making __construct private and add a static factory like
fromInt(int $id): self that sets $this->id; locate the class BillReversesId, the
$id property, the __construct and getId methods and apply one of these two fixes
consistently so types no longer conflict.
---
Nitpick comments:
In `@src/bill/Bill.php`:
- Around line 260-268: The class exposes mutation for $reversesId via
setReversesId but has getters getSource() and getTxn() without corresponding
setters, creating an asymmetric API; either add setters setSource(BillSource
$source): void and setTxn(BillTxn $txn): void (or nullable variants) to mirror
setReversesId, or if immutability for source/txn is intentional, add a concise
inline comment near the properties and the getSource()/getTxn() methods
explaining that only reversesId is mutable post-construction and why
(referencing $reversesId, setReversesId, $source, getSource, $txn, getTxn).
| /** | ||
| * @param BillReversesId|null $reversesId | ||
| * @return void | ||
| */ | ||
| public function setReversesId($reversesId): void | ||
| { | ||
| $this->reversesId = $reversesId; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
setReversesId parameter is untyped — add ?BillReversesId hint
The @param BillReversesId|null docblock documents the intended type, but the actual parameter declaration is untyped. This is inconsistent with the constructor signature (line 92: BillReversesId $reversesId = null) and silently accepts any value at runtime.
♻️ Proposed fix
- public function setReversesId($reversesId): void
+ public function setReversesId(?BillReversesId $reversesId): void
{
$this->reversesId = $reversesId;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bill/Bill.php` around lines 290 - 297, The setter setReversesId is
missing the nullable type hint documented in the docblock; change its signature
to accept ?BillReversesId (i.e. public function setReversesId(?BillReversesId
$reversesId): void) so it matches the constructor's BillReversesId $reversesId =
null intent and prevents arbitrary values, and ensure any necessary import/use
for BillReversesId is present and the docblock stays consistent.
| /** @var int|null */ | ||
| protected $id; | ||
|
|
||
| public function __construct($id = null) | ||
| { | ||
| $this->id = $id; | ||
| } | ||
|
|
||
| public function getId(): int | ||
| { | ||
| return $this->id; |
There was a problem hiding this comment.
getId(): int will throw TypeError when $id is null
The property is documented as int|null and the constructor defaults $id to null, so any instance created via new BillReversesId() or new BillReversesId(null) will throw a TypeError when getId() is called under declare(strict_types=1). The docblock and return type are contradictory.
Either make the return type nullable to match the property, or restrict the constructor so a null instance cannot be created:
🐛 Option A — fix the return type to match the nullable property
- public function getId(): int
+ public function getId(): ?int
{
return $this->id;
}🐛 Option B — restrict construction to the typed factory only
- public function __construct($id = null)
+ private function __construct(int $id)
{
$this->id = $id;
}
public function getId(): int
{
return $this->id;
}With a private constructor, callers are forced through fromInt(int $id) and a null state becomes impossible.
📝 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.
| /** @var int|null */ | |
| protected $id; | |
| public function __construct($id = null) | |
| { | |
| $this->id = $id; | |
| } | |
| public function getId(): int | |
| { | |
| return $this->id; | |
| /** `@var` int|null */ | |
| protected $id; | |
| public function __construct($id = null) | |
| { | |
| $this->id = $id; | |
| } | |
| public function getId(): ?int | |
| { | |
| return $this->id; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/bill/BillReversesId.php` around lines 9 - 19, The getId(): int return
type is incompatible with the protected $id (int|null) and the public
__construct($id = null); either make the return nullable by changing getId():
?int to reflect the property, or prevent null construction by making __construct
private and add a static factory like fromInt(int $id): self that sets
$this->id; locate the class BillReversesId, the $id property, the __construct
and getId methods and apply one of these two fixes consistently so types no
longer conflict.
…ed, the explicit nullable type must be used instead" deprecation warning in Bill
…ed, the explicit nullable type must be used instead" deprecation warning in batch of classes
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/plan/Plan.php (1)
63-63: Consider adding explicit type hint for$parent_idparameter.The
$parent_idparameter lacks a type hint, but the property on line 35 is declared as?int. For consistency with the other nullable parameters being updated in this PR, consider adding the type hint:♻️ Suggested improvement
- $parent_id = null + ?int $parent_id = null🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/plan/Plan.php` at line 63, The $parent_id parameter in the Plan class signature is missing a type hint while the corresponding property is declared as ?int; update the method/constructor declaration that currently has "$parent_id = null" to explicitly type it as "?int $parent_id = null" (in the Plan class where $parent_id is declared) so it matches the property type and other nullable parameters in this PR; ensure any callers remain compatible with the new nullable int hint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/plan/Plan.php`:
- Line 63: The $parent_id parameter in the Plan class signature is missing a
type hint while the corresponding property is declared as ?int; update the
method/constructor declaration that currently has "$parent_id = null" to
explicitly type it as "?int $parent_id = null" (in the Plan class where
$parent_id is declared) so it matches the property type and other nullable
parameters in this PR; ensure any callers remain compatible with the new
nullable int hint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 30e87f7b-ed9d-424f-ae55-a3b51ee48fda
📒 Files selected for processing (6)
src/action/AbstractAction.phpsrc/charge/Charge.phpsrc/customer/Customer.phpsrc/plan/Plan.phpsrc/price/AbstractPrice.phpsrc/product/price/PriceTypeDefinitionCollection.php
✅ Files skipped from review due to trivial changes (3)
- src/price/AbstractPrice.php
- src/action/AbstractAction.php
- src/product/price/PriceTypeDefinitionCollection.php
…ed, the explicit nullable type must be used instead" deprecation warning in SinglePrice
…ed, the explicit nullable type must be used instead" deprecation warning in BillSource
…ameter $price is implicitly treated as a required parameter" deprecation warning in SinglePrice
1 participant
Summary by CodeRabbit