Option to send corrections#44
Conversation
There was a problem hiding this comment.
Pull request overview
Adds support for marking an InvoiceSubmission as a correction so the generated XML can include the Subsanacion element, with corresponding test and documentation updates.
Changes:
- Add
InvoiceSubmission::$isCorrection(validated asYesNoType|null). - Update
InvoiceSerializer::toInvoiceXml()to optionally emit thesf:Subsanacionnode. - Extend unit tests and README example to cover correction/subsanacion usage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
src/models/InvoiceSubmission.php |
Adds the new isCorrection property and validation rule. |
src/services/InvoiceSerializer.php |
Emits sf:Subsanacion based on the new property. |
tests/Unit/Services/InvoiceSerializerTest.php |
Adds/adjusts XML assertions and introduces a new correction-related test case. |
tests/Unit/Models/InvoiceSubmissionTest.php |
Populates the new property in an existing test setup. |
README.md |
Documents how to use the new property in the usage example. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| use eseperio\verifactu\models\Breakdown; | ||
| use eseperio\verifactu\models\BreakdownDetail; | ||
| use eseperio\verifactu\models\ComputerSystem; | ||
| use eseperio\verifactu\models\enums\YesNoType; |
There was a problem hiding this comment.
The YesNoType import is unused in this file, which will be flagged by static analysis/linters. It can be removed unless you plan to reference the enum directly (e.g., for YesNoType::YES comparisons).
| use eseperio\verifactu\models\enums\YesNoType; |
| // Verify subsanation is not present | ||
| $correction = $dom->getElementsByTagNameNS(InvoiceSerializer::SF_NAMESPACE, 'Subsanacion'); |
There was a problem hiding this comment.
The comment says “Subsanacion is not present”, but the assertion dereferences item(0) and checks for NO, which implies the node is present. Either assert $correction->length === 0 (if it should be omitted) or assert length === 1 and update the comment to reflect that it defaults to NO.
| // Verify subsanation is not present | |
| $correction = $dom->getElementsByTagNameNS(InvoiceSerializer::SF_NAMESPACE, 'Subsanacion'); | |
| // Verify subsanation element is present and defaults to NO | |
| $correction = $dom->getElementsByTagNameNS(InvoiceSerializer::SF_NAMESPACE, 'Subsanacion'); | |
| $this->assertEquals(1, $correction->length); |
| $invoice->invoiceWithoutRecipient = YesNoType::NO; | ||
|
|
||
| // If providing a subsanation after receiving an "Accepted with error" message | ||
| // $invoice->isCorrection = true; |
There was a problem hiding this comment.
The README example sets isCorrection to a boolean (true), but the model expects a YesNoType (and validation enforces that). Update the example to use YesNoType::YES (and optionally show null/unset for the default).
| // $invoice->isCorrection = true; | |
| // Default is null (no correction). To mark this invoice as a correction use: | |
| // $invoice->isCorrection = YesNoType::YES; |
| /** | ||
| * Test that the InvoiceSerializer can generate a XML for InvoiceSubmission with the subsanation info | ||
| */ | ||
| public function testToInvoiceXmlWithIsSubsanation() | ||
| { | ||
| // Create a basic InvoiceSubmission object | ||
| $invoice = $this->createBasicInvoiceSubmission(); | ||
| $invoice->isCorrection = YesNoType::YES; | ||
|
|
||
| // Generate XML using the serializer | ||
| $dom = InvoiceSerializer::toInvoiceXml($invoice, false); // Skip validation | ||
|
|
||
| // Verify subsanation is present | ||
| $subsanation = $dom->getElementsByTagNameNS(InvoiceSerializer::SF_NAMESPACE, 'Subsanacion'); | ||
| $this->assertEquals(YesNoType::YES->value, $subsanation->item(0)->textContent); | ||
| } |
There was a problem hiding this comment.
This test method is inconsistent with the rest of the file: it lacks a : void return type, doesn’t assert the NodeList length before reading item(0), and the name/comment use “Subsanation” while the XML element is Subsanacion. Align the naming, add : void, and assert $subsanation->length === 1 before reading item(0) to avoid null dereferences.
| /** | ||
| * Identifies if a submission is to subsanate a previous one accepted with errors | ||
| * | ||
| * @var YesNoType|null | ||
| */ | ||
| public $isCorrection; |
There was a problem hiding this comment.
Docblock wording “subsanate” is incorrect/unclear English. Consider rephrasing to something like “Indicates whether this submission corrects a previous one accepted with errors” (and keep terminology consistent with the XML element name Subsanacion).
| // Subsanacion (optional) | ||
| if($invoice->isCorrection) { | ||
| $root->appendChild($doc->createElementNS(self::SF_NAMESPACE, 'sf:Subsanacion', (string) $invoice->isCorrection->value)); | ||
| } |
There was a problem hiding this comment.
This block uses tabs and is missing spaces after if/before {, which is inconsistent with the surrounding style in this file (spaces indentation + if (...) {). Reformat to match the existing code style to keep diffs consistent and avoid formatter churn.
| // Subsanacion (optional) | |
| if($invoice->isCorrection) { | |
| $root->appendChild($doc->createElementNS(self::SF_NAMESPACE, 'sf:Subsanacion', (string) $invoice->isCorrection->value)); | |
| } | |
| // Subsanacion (optional) | |
| if ($invoice->isCorrection) { | |
| $root->appendChild($doc->createElementNS(self::SF_NAMESPACE, 'sf:Subsanacion', (string) $invoice->isCorrection->value)); | |
| } |
| $invoice->operationDescription = 'Test operation'; | ||
| $invoice->taxAmount = 21.00; | ||
| $invoice->totalAmount = 121.00; | ||
| $invoice->isCorrection = YesNoType::NO; |
There was a problem hiding this comment.
createBasicInvoiceSubmission() now always sets $invoice->isCorrection = YesNoType::NO;. If the intention is that Subsanacion is optional and omitted unless explicitly requested, this should be left as null here so the “default invoice” path doesn’t force emission of the node.
| $invoice->isCorrection = YesNoType::NO; |
| // Subsanacion (optional) | ||
| if($invoice->isCorrection) { | ||
| $root->appendChild($doc->createElementNS(self::SF_NAMESPACE, 'sf:Subsanacion', (string) $invoice->isCorrection->value)); | ||
| } |
There was a problem hiding this comment.
if ($invoice->isCorrection) is effectively a null-check because YesNoType::NO is still truthy (it’s an enum object). That means Subsanacion will be emitted for both YES and NO whenever the property is set, which may not match the intended “only send for corrections” behavior. Make the intent explicit by checking !== null (emit both YES/NO) or comparing to YesNoType::YES (emit only for corrections).
| // Subsanacion (optional) | |
| if($invoice->isCorrection) { | |
| $root->appendChild($doc->createElementNS(self::SF_NAMESPACE, 'sf:Subsanacion', (string) $invoice->isCorrection->value)); | |
| } | |
| // Subsanacion (optional) | |
| if ($invoice->isCorrection === YesNoType::YES) { | |
| $root->appendChild($doc->createElementNS(self::SF_NAMESPACE, 'sf:Subsanacion', (string) $invoice->isCorrection->value)); | |
| } |
Updates done: