HP-2883: Implement Business Central bills import service#114
Merged
SilverFire merged 9 commits intoMay 13, 2026
Conversation
Contributor
|
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:
📝 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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}
Contributor
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).
Comment on lines
+290
to
+297
| /** | ||
| * @param BillReversesId|null $reversesId | ||
| * @return void | ||
| */ | ||
| public function setReversesId($reversesId): void | ||
| { | ||
| $this->reversesId = $reversesId; | ||
| } |
Contributor
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.
…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
Contributor
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
SilverFire
approved these changes
May 13, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit