Fix HTTP client exception handling, validation, consolidate documentation, and update AI guidelines #354

Copilot · 2026-01-11T13:50:28Z

PHPUnit Test Fixes - Complete Exception Handling ✅

All requirements implemented with proper exception flow, documentation, logging, and comprehensive format handler support.

Exception Handling Architecture

Flow:

ApiClient.request()
  → Laravel HTTP call
  → ->throw() on 4xx/5xx
  → HttpClientExceptionHandler catches
  → Logs error details  
  → Re-throws exception
  → DocumentsClient catches selectively
  → Returns response for 422/429/500/404
  → Throws for 401/403 and others
  → PeppolService checks response.successful()
  → Throws if needed for business logic

Recent Updates (Addressing Code Review Feedback)

AI Guidelines Updated for Complete Peppol Format Handler Coverage ✅

Enhanced .junie/guidelines.md:

Added comprehensive list of all 11 Peppol format handlers with country associations
Documented format selection logic (customer preference → mandatory → recommended → fallback)
Added logging behavior documentation (Info vs Warning levels for different scenarios)
Clarified country-specific format requirements and mandatory formats

Enhanced .github/copilot-instructions.md:

Listed all 11 supported Peppol format handlers with descriptions
Added country associations for each format (e.g., FatturaPA for Italy, Facturae for Spain)
Noted comprehensive PHPUnit test coverage for all handlers
Documented automatic fallback logic in FormatHandlerFactory

These updates ensure AI assistants (Junie, Copilot) have complete information about:

All available format handlers and their purposes
Country-specific e-invoicing requirements
Format selection strategy and fallback behavior
Logging levels for different scenarios

Documentation Consolidation for Production Users ✅

Enhanced Main README:

Consolidated Quick Start, Installation, and Setup guides into comprehensive main README
Added table of contents with emoji navigation for easy browsing
Expanded sections: Configuration, Development, Testing, Peppol E-Invoicing, Deployment
Added production deployment checklist with Nginx/Apache configurations
Enhanced Contributing section with step-by-step process
Better organized documentation links categorized by Getting Started, Development, Advanced, Operations
Professional presentation suitable for 100s of production users

Removed Redundant Files:

Deleted .github/QUICKSTART.md (content merged into README)
Deleted .github/SETUP.md (content merged into README)

This consolidation provides a single entry point for all documentation, reducing confusion and improving the onboarding experience.

All Format Handlers Restored with Test Coverage ✅

FormatHandlerFactory:

All 11 Peppol format handlers are now supported:
- CII (Cross Industry Invoice - UN/CEFACT standard)
- EHF 3.0 (Norwegian e-invoice format)
- Factur-X (French/German hybrid PDF+XML format)
- Facturae 3.2 (Spanish e-invoice - mandatory for public administration)
- FatturaPA 1.2 (Italian e-invoice - mandatory for all invoices)
- OIOUBL (Danish e-invoice format)
- PEPPOL BIS 3.0 (Default Peppol format)
- UBL 2.1/2.4 (Universal Business Language)
- ZUGFeRD 1.0/2.0 (German e-invoice PDF+XML format)

PHPUnit Test Coverage:

Added comprehensive tests for all format handlers
Individual test methods for each format (EHF, Factur-X, Facturae, FatturaPA, OIOUBL, ZUGFeRD 1.0/2.0)
Updated registry validation tests to verify all 11 handlers
Handler existence checks for complete format support
Test for unsupported format exception handling

Documentation & Logging Improvements ✅

PeppolService Validation:

Added clear documentation explaining dual validation:
- validateInvoice(): Basic requirements (customer, invoice_number, items)
- formatHandler->validate(): Format-specific rules (UBL, CII standards)
Both validations are necessary and serve different purposes

FormatHandlerFactory Logging:

Logging Added: Three fallback scenarios now logged:
1. Customer's preferred format unavailable → Info level
2. Mandatory format for country unavailable → Warning level (configuration issue)
3. Recommended format unavailable → Info level
Logs include context (invoice_id, country_code, format, error details)

Key Changes Summary

ApiClient ✅

Uses ->throw() to automatically throw on all 4xx/5xx errors
Throws RequestException for all HTTP errors
Throws ConnectionException for network issues

HttpClientExceptionHandler ✅

Catches all exceptions from ApiClient
Logs request, response, and error details
Re-throws exception after logging

DocumentsClient ✅

Catches specific exceptions and returns responses for client inspection:
- submitDocument(): Returns response for 422, 429, 500 (validation, rate limit, server errors)
- getDocument(): Returns response for 404 (not found)
- Other errors (401, 403, etc.) propagate as exceptions
This allows tests to inspect error responses while still throwing for auth failures

PeppolService ✅

Dual validation with clear documentation of purpose
Checks if response is successful after DocumentsClient call
Throws RequestException for unsuccessful responses
Maintains special 404 handling for cancelDocument

FormatHandlerFactory ✅

All 11 format handlers supported with full documentation
Comprehensive PHPUnit test coverage for all handlers
Comprehensive logging for fallback scenarios
Graceful degradation with proper alerting for administrators

Documentation ✅

Consolidated main README with comprehensive user guide
Removed duplicate documentation files for clarity
Professional presentation with table of contents and organized sections
Single entry point for all documentation reducing confusion
Updated AI guidelines (.junie/guidelines.md and .github/copilot-instructions.md) with complete Peppol format handler information

This architecture satisfies all requirements:

ApiClient uses ->throw() as user requested
HttpClientExceptionHandler catches and logs exceptions
DocumentsClient selectively returns error responses for inspection
PeppolService throws when needed for proper error propagation
All Peppol format handlers restored with comprehensive test coverage
Fallback scenarios are logged for debugging and monitoring
Documentation consolidated for excellent user experience suitable for production use
AI assistant guidelines updated with complete format handler documentation for better code assistance

Original prompt

You are fixing a Laravel 10 application with PHPUnit tests.
Your task is to make ALL failing tests pass WITHOUT weakening assertions.

Follow these rules strictly:

GENERAL RULES

Never disable tests.

Never loosen assertions (do not change expected values unless logic demands it).

Prefer fixing application code over modifying tests unless tests are incorrect.

Always use Http::fake() in unit tests that touch HTTP.

Never allow real HTTP requests in tests.

Always call ->throw() on HTTP responses where tests expect RequestException.

Wrap connection errors consistently.

PART 1: HTTP CLIENT & EXCEPTION HANDLING

Ensure ApiClient and HttpClientExceptionHandler:

Convert connection errors into ConnectionException

Convert 4xx/5xx into RequestException when ->throw() is expected

Log errors using Log::error() exactly once per failure

Preserve response status and JSON body

Update HttpClientExceptionHandler so:

RequestException is rethrown for HTTP errors

ConnectionException is handled separately

Unexpected exceptions are logged and rethrown

PART 2: DOCUMENTS CLIENT (PEPPOL)

Ensure DocumentsClient:

Returns proper JSON responses

Uses correct HTTP verbs and URLs

Calls ->throw() for 401, 403, 404 when tests expect exceptions

Returns unsuccessful() = false for 4xx/5xx without throw

Preserves status codes (201, 204, 422, 429, 500)

Ensure Http::fake() responses include:

Correct status code

Correct JSON structure

PART 3: PEPPOL SERVICE & ACTIONS

Ensure validation errors throw InvalidArgumentException

Ensure HTTP errors throw RequestException

Ensure invoice validation happens before HTTP only when tests expect it

Ensure returned document statuses match test expectations

Ensure Http::assertSent() passes

PART 4: FORMAT HANDLER FACTORY

Do NOT resolve handlers via container blindly

Maintain an explicit supported format map

hasHandler() must return false for unsupported formats

Throw RuntimeException for unsupported formats

Never leak BindingResolutionException

PART 5: SERVICE PROVIDER CONFIG

Provide defaults or test-safe fallbacks for:

peppol.api_key

peppol.base_url

Allow overriding via config() in tests

Prevent null injection into BasePeppolClient

PART 6: DATABASE / NUMBERING LOGIC

Fix factories to always generate:

invoice_number

quote_number

valid foreign keys

Replace DB constraint failures with explicit domain checks

Throw RuntimeException for duplicate numbers BEFORE insert

Ensure SQLite foreign keys pass in tests

FINAL CHECK

Run PHPUnit and ensure:

No ConnectionException from real HTTP

No uncaught QueryException

All Log::shouldHaveReceived assertions pass

All RequestException expectations are met

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Summary by CodeRabbit

Bug Fixes
- Enhanced error handling and validation for invoice operations
- Improved authentication error detection in Peppol invoice service
- Strengthened error handling for duplicate invoice and quote detection
Changes
- Removed support for legacy invoice formats (EHF 3.0, Factur-X, Facturae 3.2, FatturaPA 1.2, OIOUBL, ZUGFeRD 1.0, ZUGFeRD 2.0)

_{✏️ Tip: You can customize this high-level summary in your review settings.}

…ect FormatHandlerFactory Co-authored-by: nielsdrost7 <[email protected]>

…re returning Co-authored-by: nielsdrost7 <[email protected]>

…ully fallback Co-authored-by: nielsdrost7 <[email protected]>

…te numbers Co-authored-by: nielsdrost7 <[email protected]>

nielsdrost7

@copilot make sure my phpunit tests are up to date

Modules/Invoices/Http/Clients/ApiClient.php

coderabbitai

Actionable comments posted: 5

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

Modules/Invoices/Peppol/Services/PeppolService.php (1)

23-26: Add LogsPeppolActivity trait and dispatch transmission event after successful submission.

PeppolService is missing the LogsPeppolActivity trait and does not dispatch events after transmission. Per coding guidelines, Peppol services must use both LogsApiRequests and LogsPeppolActivity traits, and events must be dispatched for all major operations like transmission. Other Peppol services (SendInvoiceToPeppolJob, PeppolManagementService, RetryFailedTransmissions) correctly implement both patterns.

🤖 Fix all issues with AI agents

In @Modules/Invoices/Config/config.php:
- Line 37: The config uses a test default for 'api_key' via
env('PEPPOL_E_INVOICE_BE_API_KEY', 'test-default-key'), which allows production
to run with test credentials; remove the test default and add a startup
validation (e.g., in the module's service provider boot method like
InvoiceServiceProvider::boot or in app bootstrap) that checks
env('PEPPOL_E_INVOICE_BE_API_KEY') is present and not equal to
'test-default-key', throwing/aborting startup with a clear error message if
invalid or missing in production (APP_ENV === 'production'); also update
deployment docs to require setting PEPPOL_E_INVOICE_BE_API_KEY in production.

In @Modules/Invoices/Observers/InvoiceObserver.php:
- Line 25: The throw in InvoiceObserver that uses a hardcoded English string
should be reverted to use the translation helper (e.g., trans() or __()) to
restore i18n support and match QuoteObserver; replace the literal message in the
throw new \RuntimeException(...) with a translated message using the same
translation key/placeholder format used by QuoteObserver (pass invoice_number
and company_id as parameters), and if the hardcoded change is required for
tests, document the decision and apply the same pattern to QuoteObserver for
consistency.
- Line 25: In InvoiceObserver.php replace the inline globally-qualified
exception with an imported one: add "use RuntimeException;" to the top of the
file and update the throw site in InvoiceObserver (the line that currently does
throw new \RuntimeException("Duplicate invoice number
'{$invoice->invoice_number}' for company ID {$invoice->company_id}");) to throw
new RuntimeException(...) without the leading backslash so the class is resolved
via the import.

🧹 Nitpick comments (4)

Modules/Invoices/Observers/InvoiceObserver.php (1)
5-5: Remove unused ValidationException import.

The ValidationException import is no longer used after switching to RuntimeException for duplicate detection.
♻️ Proposed cleanup
-use Illuminate\Validation\ValidationException;
 use Modules\Core\Observers\AbstractObserver;
 use Modules\Invoices\Models\Invoice;
+use RuntimeException;
Modules/Invoices/Peppol/Services/PeppolService.php (2)
84-90: Consider checking response status before parsing JSON.

The current pattern calls $response->json() before verifying $response->successful(). While Laravel's HTTP client handles this gracefully, the logical flow would be clearer if success is checked first—especially since error responses may have different JSON structures or non-JSON bodies.
♻️ Suggested refactor
         try {
             $response     = $this->documentsClient->submitDocument($documentData);
-            $responseData = $response->json();

             // Throw for non-successful responses
             if ( ! $response->successful()) {
                 $response->throw();
             }

+            $responseData = $response->json();
+
             $this->logResponse('Peppol', 'POST /documents', $response->status(), $responseData);
129-140: Same pattern issue: JSON parsing before success check.

Same observation as in sendInvoiceToPeppol—consider checking success before parsing the response body.
Modules/Invoices/Peppol/FormatHandlers/FormatHandlerFactory.php (1)
158-167: Consider including original exception in make() for debugging.

The make() method discards the original ValueError exception context. For consistency with create() (line 59), consider chaining the original exception.
♻️ Suggested fix
         } catch (ValueError $e) {
-            throw new RuntimeException("Invalid format: {$formatString}");
+            throw new RuntimeException("Invalid format: {$formatString}", 0, $e);
         }

📜 Review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 42f1f15 and 141c12a.

📒 Files selected for processing (7)

Modules/Invoices/Config/config.php
Modules/Invoices/Http/Clients/ApiClient.php
Modules/Invoices/Observers/InvoiceObserver.php
Modules/Invoices/Peppol/Clients/EInvoiceBe/DocumentsClient.php
Modules/Invoices/Peppol/FormatHandlers/FormatHandlerFactory.php
Modules/Invoices/Peppol/Services/PeppolService.php
Modules/Quotes/Observers/QuoteObserver.php

🧰 Additional context used

📓 Path-based instructions (4)

**/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Always use full namespace imports instead of short imports (e.g., use Illuminate\Support\Facades\Log not Log)

Files:

Modules/Invoices/Http/Clients/ApiClient.php
Modules/Invoices/Observers/InvoiceObserver.php
Modules/Invoices/Peppol/Clients/EInvoiceBe/DocumentsClient.php
Modules/Quotes/Observers/QuoteObserver.php
Modules/Invoices/Peppol/FormatHandlers/FormatHandlerFactory.php
Modules/Invoices/Config/config.php
Modules/Invoices/Peppol/Services/PeppolService.php

**/Services/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

Services must not build DTOs manually; they must use Transformers directly

Files:

Modules/Invoices/Peppol/Services/PeppolService.php

**/Peppol/{Services,Handlers}/*.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

**/Peppol/{Services,Handlers}/*.php: Peppol service follows Strategy Pattern for format handlers (UBL, FatturaPA, ZUGFeRD, etc.)
All Peppol API calls must go through the ApiClient with exception handling
Peppol logging is done via LogsApiRequests and LogsPeppolActivity traits
Peppol events are dispatched for all major operations (transmission, validation, etc.)

Files:

Modules/Invoices/Peppol/Services/PeppolService.php

**/Peppol/Services/PeppolService.php

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

PeppolService coordinates invoice transformation and transmission

Files:

Modules/Invoices/Peppol/Services/PeppolService.php

🧠 Learnings (9)

📓 Common learnings

Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-08T10:38:10.370Z
Learning: Applies to **/Peppol/{Services,Handlers}/*.php : All Peppol API calls must go through the ApiClient with exception handling

Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-08T10:38:10.370Z
Learning: Applies to **/Peppol/{Services,Handlers}/*.php : Peppol events are dispatched for all major operations (transmission, validation, etc.)

Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-08T10:38:10.370Z
Learning: Applies to **/Peppol/{Services,Handlers}/*.php : Peppol service follows Strategy Pattern for format handlers (UBL, FatturaPA, ZUGFeRD, etc.)

Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-08T10:38:10.370Z
Learning: Applies to **/Peppol/Services/PeppolService.php : PeppolService coordinates invoice transformation and transmission

Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-08T10:38:10.370Z
Learning: Applies to **/Peppol/Handlers/*.php : Peppol format handlers must implement validation, transformation, and format-specific logic

Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-08T10:38:10.370Z
Learning: Applies to **/Peppol/{Services,Handlers}/*.php : Peppol logging is done via LogsApiRequests and LogsPeppolActivity traits

Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-08T10:38:10.370Z
Learning: Applies to **/Peppol/Factories/*.php : Peppol Provider Factory creates provider-specific clients (e.g., EInvoiceBe)

Learnt from: CR
Repo: InvoicePlane/InvoicePlane-v2 PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2026-01-08T10:38:10.370Z
Learning: Applies to **/Peppol/Services/PeppolManagementService.php : PeppolManagementService handles integration lifecycle (create, test, validate, send)