From d5ca31d7815a6930e3272c42e33f1ae394f8db87 Mon Sep 17 00:00:00 2001 From: Douglas Lindsay Date: Wed, 18 Mar 2026 09:55:15 +0000 Subject: [PATCH 1/6] ABN-287/feat: add PHPUnit test infrastructure and unit tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Bootstrap test infrastructure without composer install — uses a PSR-4 autoloader and Magento stub classes so tests run with just a phpunit phar on a bare PHP image. - 38 tests (65 assertions) covering getErrorFromResponse(), getFieldNameFromLoc(), getCheckoutApiUrl(), getCheckoutPageUrl() - Docker-based `make test` target and CI workflow (PHP 7.4/8.1/8.2) - Fix return type bug: SCHEMA_ERROR/ORDER_INVALID now returns Phrase Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/phpunit.yml | 28 ++ Makefile | 8 +- Model/Two.php | 2 +- Test/Stubs/Phrase.php | 49 ++++ Test/Stubs/ScopeConfigInterface.php | 11 + Test/Stubs/ScopeInterface.php | 13 + Test/Stubs/State.php | 22 ++ Test/Unit/Model/Config/RepositoryUrlTest.php | 177 ++++++++++++ Test/Unit/Model/TwoErrorHandlingTest.php | 288 +++++++++++++++++++ Test/bootstrap.php | 62 ++++ phpunit.xml | 12 + 11 files changed, 670 insertions(+), 2 deletions(-) create mode 100644 .github/workflows/phpunit.yml create mode 100644 Test/Stubs/Phrase.php create mode 100644 Test/Stubs/ScopeConfigInterface.php create mode 100644 Test/Stubs/ScopeInterface.php create mode 100644 Test/Stubs/State.php create mode 100644 Test/Unit/Model/Config/RepositoryUrlTest.php create mode 100644 Test/Unit/Model/TwoErrorHandlingTest.php create mode 100644 Test/bootstrap.php create mode 100644 phpunit.xml diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml new file mode 100644 index 0000000..e3c59ea --- /dev/null +++ b/.github/workflows/phpunit.yml @@ -0,0 +1,28 @@ +name: PHPUnit + +on: + push: + branches: [main] + pull_request: + branches: [main] + +jobs: + test: + runs-on: ubuntu-latest + strategy: + matrix: + php-version: ['7.4', '8.1', '8.2'] + + steps: + - uses: actions/checkout@v4 + + - name: Set up PHP ${{ matrix.php-version }} + uses: shivammathur/setup-php@v2 + with: + php-version: ${{ matrix.php-version }} + + - name: Download PHPUnit + run: php -r "copy('https://phar.phpunit.de/phpunit-9.6.phar', 'phpunit.phar');" + + - name: Run tests + run: php phpunit.phar diff --git a/Makefile b/Makefile index 352f15f..a0a6ed4 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,7 @@ TWO_API_BASE_URL ?= https://api.staging.two.inc TWO_CHECKOUT_BASE_URL ?= https://checkout.staging.two.inc TWO_STORE_COUNTRY ?= NO -.PHONY: help install configure compile run stop clean logs archive patch minor major format +.PHONY: help install configure compile run stop clean logs archive patch minor major format test .DEFAULT_GOAL := help @@ -94,6 +94,12 @@ patch: bumpver-patch minor: bumpver-minor ## Bump major version major: bumpver-major +## Run PHPUnit tests +test: + docker run --rm -v $(CURDIR):/app -w /app php:8.1-cli bash -c \ + "php -r \"copy('https://phar.phpunit.de/phpunit-9.6.phar', '/tmp/phpunit.phar');\" \ + && php /tmp/phpunit.phar" + ## Format frontend assets with Prettier format: prettier -w view/frontend/web/js/ diff --git a/Model/Two.php b/Model/Two.php index b5e6760..404708c 100755 --- a/Model/Two.php +++ b/Model/Two.php @@ -325,7 +325,7 @@ public function getErrorFromResponse(array $response): ?Phrase $reason = __('The buyer and the seller are the same company.'); } if ($isClientError && in_array($errorCode, ['SCHEMA_ERROR', 'SAME_BUYER_SELLER_ERROR', 'ORDER_INVALID'])) { - return $reason; + return $reason instanceof Phrase ? $reason : __($reason); } // System errors — include trace ID diff --git a/Test/Stubs/Phrase.php b/Test/Stubs/Phrase.php new file mode 100644 index 0000000..613cc86 --- /dev/null +++ b/Test/Stubs/Phrase.php @@ -0,0 +1,49 @@ +text = $text; + $this->arguments = $arguments; + } + + public function render(): string + { + $result = $this->text; + foreach ($this->arguments as $index => $value) { + $result = str_replace('%' . ($index + 1), (string)$value, $result); + } + return $result; + } + + public function __toString(): string + { + return $this->render(); + } + + public function getText(): string + { + return $this->text; + } + + public function getArguments(): array + { + return $this->arguments; + } +} diff --git a/Test/Stubs/ScopeConfigInterface.php b/Test/Stubs/ScopeConfigInterface.php new file mode 100644 index 0000000..5f47b9b --- /dev/null +++ b/Test/Stubs/ScopeConfigInterface.php @@ -0,0 +1,11 @@ +mode; + } +} diff --git a/Test/Unit/Model/Config/RepositoryUrlTest.php b/Test/Unit/Model/Config/RepositoryUrlTest.php new file mode 100644 index 0000000..8536a69 --- /dev/null +++ b/Test/Unit/Model/Config/RepositoryUrlTest.php @@ -0,0 +1,177 @@ +scopeConfig = $this->createMock(ScopeConfigInterface::class); + $encryptor = $this->createMock(EncryptorInterface::class); + $urlBuilder = $this->createMock(UrlInterface::class); + $productMetadata = $this->createMock(ProductMetadataInterface::class); + $this->appState = $this->createMock(State::class); + + $this->repository = new Repository( + $this->scopeConfig, + $encryptor, + $urlBuilder, + $productMetadata, + $this->appState + ); + } + + protected function tearDown(): void + { + foreach ($this->envVarsToClear as $var) { + putenv($var); + } + $this->envVarsToClear = []; + } + + private function setEnv(string $name, string $value): void + { + putenv("$name=$value"); + $this->envVarsToClear[] = $name; + } + + private function configureMode(string $mode): void + { + $this->scopeConfig + ->method('getValue') + ->willReturn($mode); + } + + // ── getCheckoutApiUrl ─────────────────────────────────────────────── + + public function testApiUrlProductionMode(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_PRODUCTION); + $this->configureMode('production'); + + $this->assertEquals('https://api.two.inc', $this->repository->getCheckoutApiUrl()); + } + + public function testApiUrlSandboxMode(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_PRODUCTION); + $this->configureMode('sandbox'); + + $this->assertEquals('https://api.sandbox.two.inc', $this->repository->getCheckoutApiUrl()); + } + + public function testApiUrlDeveloperModeWithEnvVar(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_DEVELOPER); + $this->setEnv('TWO_API_BASE_URL', 'http://localhost:8000'); + + $this->assertEquals('http://localhost:8000', $this->repository->getCheckoutApiUrl()); + } + + public function testApiUrlDeveloperModeEmptyEnvVar(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_DEVELOPER); + $this->setEnv('TWO_API_BASE_URL', ''); + $this->configureMode('sandbox'); + + $this->assertEquals('https://api.sandbox.two.inc', $this->repository->getCheckoutApiUrl()); + } + + public function testApiUrlNonDeveloperModeIgnoresEnvVar(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_PRODUCTION); + $this->setEnv('TWO_API_BASE_URL', 'http://localhost:8000'); + $this->configureMode('production'); + + $this->assertEquals('https://api.two.inc', $this->repository->getCheckoutApiUrl()); + } + + public function testApiUrlExplicitModeParameter(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_PRODUCTION); + + $this->assertEquals( + 'https://api.staging.two.inc', + $this->repository->getCheckoutApiUrl('staging') + ); + } + + // ── getCheckoutPageUrl ────────────────────────────────────────────── + + public function testPageUrlProductionMode(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_PRODUCTION); + $this->configureMode('production'); + + $this->assertEquals('https://checkout.two.inc', $this->repository->getCheckoutPageUrl()); + } + + public function testPageUrlSandboxMode(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_PRODUCTION); + $this->configureMode('sandbox'); + + $this->assertEquals('https://checkout.sandbox.two.inc', $this->repository->getCheckoutPageUrl()); + } + + public function testPageUrlDeveloperModeWithEnvVar(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_DEVELOPER); + $this->setEnv('TWO_CHECKOUT_BASE_URL', 'http://localhost:3000'); + + $this->assertEquals('http://localhost:3000', $this->repository->getCheckoutPageUrl()); + } + + public function testPageUrlDeveloperModeEmptyEnvVar(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_DEVELOPER); + $this->setEnv('TWO_CHECKOUT_BASE_URL', ''); + $this->configureMode('sandbox'); + + $this->assertEquals('https://checkout.sandbox.two.inc', $this->repository->getCheckoutPageUrl()); + } + + public function testPageUrlNonDeveloperModeIgnoresEnvVar(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_PRODUCTION); + $this->setEnv('TWO_CHECKOUT_BASE_URL', 'http://localhost:3000'); + $this->configureMode('production'); + + $this->assertEquals('https://checkout.two.inc', $this->repository->getCheckoutPageUrl()); + } + + public function testPageUrlExplicitModeParameter(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_PRODUCTION); + + $this->assertEquals( + 'https://checkout.staging.two.inc', + $this->repository->getCheckoutPageUrl('staging') + ); + } +} diff --git a/Test/Unit/Model/TwoErrorHandlingTest.php b/Test/Unit/Model/TwoErrorHandlingTest.php new file mode 100644 index 0000000..afec5af --- /dev/null +++ b/Test/Unit/Model/TwoErrorHandlingTest.php @@ -0,0 +1,288 @@ +model = $this->getMockBuilder(Two::class) + ->disableOriginalConstructor() + ->onlyMethods([]) // we test real implementations + ->getMock(); + + // Inject a configRepository so that PROVIDER constant is accessible. + $configRepo = $this->createMock(ConfigRepository::class); + $ref = new \ReflectionClass(Two::class); + $prop = $ref->getProperty('configRepository'); + $prop->setAccessible(true); + $prop->setValue($this->model, $configRepo); + } + + // ── getErrorFromResponse ──────────────────────────────────────────── + + public function testSuccessfulResponseReturnsNull(): void + { + $response = ['status' => 'APPROVED', 'id' => 'abc-123']; + $this->assertNull($this->model->getErrorFromResponse($response)); + } + + public function testEmptyResponseReturnsGeneralError(): void + { + $result = $this->model->getErrorFromResponse([]); + $this->assertInstanceOf(Phrase::class, $result); + $rendered = $result->render(); + $this->assertStringContainsString('Something went wrong', $rendered); + $this->assertStringContainsString('Two', $rendered); + } + + // ── Validation errors (400 + error_json) ──────────────────────────── + + public function testValidationErrorWithKnownFieldAndMessage(): void + { + $response = [ + 'http_status' => 400, + 'error_json' => [ + [ + 'loc' => ['buyer', 'representative', 'phone_number'], + 'msg' => 'Invalid phone number', + ], + ], + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringContainsString('Phone Number', $rendered); + $this->assertStringContainsString('Invalid phone number', $rendered); + // Validation errors must NOT include a trace ID + $this->assertStringNotContainsString('Trace ID', $rendered); + } + + public function testValidationErrorFieldOnlyNoMessage(): void + { + $response = [ + 'http_status' => 400, + 'error_json' => [ + ['loc' => ['buyer', 'representative', 'email']], + ], + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringContainsString('Email Address', $rendered); + $this->assertStringContainsString('is not valid', $rendered); + } + + public function testValidationErrorUnknownLocWithMessage(): void + { + $response = [ + 'http_status' => 400, + 'error_json' => [ + ['loc' => ['some', 'unknown', 'field'], 'msg' => 'Bad value'], + ], + ]; + $result = $this->model->getErrorFromResponse($response); + $this->assertStringContainsString('Bad value', $result->render()); + } + + public function testValidationErrorMultipleErrors(): void + { + $response = [ + 'http_status' => 400, + 'error_json' => [ + ['loc' => ['buyer', 'representative', 'first_name'], 'msg' => 'Required'], + ['loc' => ['buyer', 'representative', 'last_name'], 'msg' => 'Required'], + ], + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringContainsString('First Name', $rendered); + $this->assertStringContainsString('Last Name', $rendered); + } + + public function testValidationErrorCleansPydanticPrefix(): void + { + $response = [ + 'http_status' => 400, + 'error_json' => [ + ['loc' => ['some', 'field'], 'msg' => 'Value error, bad input'], + ], + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringNotContainsString('Value error,', $rendered); + $this->assertStringContainsString('bad input', $rendered); + } + + public function testValidationErrorCleansPydanticSuffix(): void + { + $response = [ + 'http_status' => 400, + 'error_json' => [ + ['loc' => ['some', 'field'], 'msg' => 'bad value [type=value_error]'], + ], + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringNotContainsString('[type=', $rendered); + $this->assertStringContainsString('bad value', $rendered); + } + + public function testValidationErrorNoDoublePeriods(): void + { + $response = [ + 'http_status' => 400, + 'error_json' => [ + [ + 'loc' => ['buyer', 'representative', 'phone_number'], + 'msg' => 'Invalid phone number.', + ], + ], + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + // rtrim(msg, '.') then append '.' → single period + $this->assertStringNotContainsString('..', $rendered); + } + + // ── User errors (400 + error_code) ────────────────────────────────── + + public function testUserErrorSchemaError(): void + { + $response = [ + 'http_status' => 400, + 'error_code' => 'SCHEMA_ERROR', + 'error_message' => 'Missing required field: company_id', + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringContainsString('Missing required field', $rendered); + $this->assertStringNotContainsString('Trace ID', $rendered); + } + + public function testUserErrorSameBuyerSeller(): void + { + $response = [ + 'http_status' => 400, + 'error_code' => 'SAME_BUYER_SELLER_ERROR', + 'error_message' => 'original api message', + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringContainsString('buyer and the seller are the same', $rendered); + $this->assertStringNotContainsString('Trace ID', $rendered); + } + + public function testUserErrorOrderInvalid(): void + { + $response = [ + 'http_status' => 400, + 'error_code' => 'ORDER_INVALID', + 'error_message' => 'Order amount too low', + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringContainsString('Order amount too low', $rendered); + $this->assertStringNotContainsString('Trace ID', $rendered); + } + + // ── System errors (non-400 + error_code) ──────────────────────────── + + public function testSystemErrorWithTraceId(): void + { + $response = [ + 'http_status' => 500, + 'error_code' => 'INTERNAL_ERROR', + 'error_message' => 'Something broke', + 'error_trace_id' => 'abc-123-trace', + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringContainsString('failed', $rendered); + $this->assertStringContainsString('Something broke', $rendered); + $this->assertStringContainsString('abc-123-trace', $rendered); + $this->assertStringContainsString('Trace ID', $rendered); + } + + public function testSystemErrorWithoutTraceId(): void + { + $response = [ + 'http_status' => 500, + 'error_code' => 'INTERNAL_ERROR', + 'error_message' => 'Something broke', + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + $this->assertStringContainsString('failed', $rendered); + $this->assertStringNotContainsString('Trace ID', $rendered); + $this->assertStringNotContainsString('[', $rendered); + } + + public function testNon400WithErrorJsonSkipsValidationPath(): void + { + $response = [ + 'http_status' => 500, + 'error_json' => [ + ['loc' => ['buyer', 'representative', 'email'], 'msg' => 'bad'], + ], + 'error_code' => 'INTERNAL_ERROR', + 'error_message' => 'Server error', + ]; + $result = $this->model->getErrorFromResponse($response); + $rendered = $result->render(); + // Should hit the system-error path, not validation + $this->assertStringContainsString('failed', $rendered); + $this->assertStringNotContainsString('Email Address', $rendered); + } + + // ── getFieldNameFromLoc ───────────────────────────────────────────── + + /** + * @dataProvider knownFieldMappingsProvider + */ + public function testKnownFieldMappings(string $locJson, string $expectedField): void + { + $result = $this->model->getFieldNameFromLoc($locJson); + $this->assertNotNull($result); + $this->assertEquals($expectedField, $result->render()); + } + + public function knownFieldMappingsProvider(): array + { + return [ + 'phone' => ['["buyer","representative","phone_number"]', 'Phone Number'], + 'org_num' => ['["buyer","company","organization_number"]', 'Company ID'], + 'fname' => ['["buyer","representative","first_name"]', 'First Name'], + 'lname' => ['["buyer","representative","last_name"]', 'Last Name'], + 'email' => ['["buyer","representative","email"]', 'Email Address'], + 'street' => ['["billing_address","street_address"]', 'Street Address'], + 'city' => ['["billing_address","city"]', 'City'], + 'country' => ['["billing_address","country"]', 'Country'], + 'postcode' => ['["billing_address","postal_code"]', 'Zip/Postal Code'], + ]; + } + + public function testUnknownLocReturnsNull(): void + { + $this->assertNull($this->model->getFieldNameFromLoc('["unknown","field"]')); + } + + public function testWhitespaceInLocIsNormalised(): void + { + $locWithSpaces = '[ "buyer" , "representative" , "phone_number" ]'; + $result = $this->model->getFieldNameFromLoc($locWithSpaces); + $this->assertNotNull($result); + $this->assertEquals('Phone Number', $result->render()); + } +} diff --git a/Test/bootstrap.php b/Test/bootstrap.php new file mode 100644 index 0000000..0588e1d --- /dev/null +++ b/Test/bootstrap.php @@ -0,0 +1,62 @@ + + + + + Test/Unit/ + + + From 019a6889609ef30cc0c16bf4d4b63635f940e84f Mon Sep 17 00:00:00 2001 From: Douglas Lindsay Date: Wed, 18 Mar 2026 10:13:13 +0000 Subject: [PATCH 2/6] ABN-287/test: document env var precedence over explicit mode parameter Add tests asserting that in developer mode, TWO_API_BASE_URL and TWO_CHECKOUT_BASE_URL env vars take precedence even when an explicit $mode parameter is passed to getCheckoutApiUrl/getCheckoutPageUrl. Co-Authored-By: Claude Opus 4.6 (1M context) --- Test/Unit/Model/Config/RepositoryUrlTest.php | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/Test/Unit/Model/Config/RepositoryUrlTest.php b/Test/Unit/Model/Config/RepositoryUrlTest.php index 8536a69..3673ebd 100644 --- a/Test/Unit/Model/Config/RepositoryUrlTest.php +++ b/Test/Unit/Model/Config/RepositoryUrlTest.php @@ -121,6 +121,15 @@ public function testApiUrlExplicitModeParameter(): void ); } + public function testApiUrlDeveloperModeEnvVarOverridesExplicitMode(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_DEVELOPER); + $this->setEnv('TWO_API_BASE_URL', 'http://localhost:8000'); + + // Env var takes precedence over explicit $mode in developer mode + $this->assertEquals('http://localhost:8000', $this->repository->getCheckoutApiUrl('sandbox')); + } + // ── getCheckoutPageUrl ────────────────────────────────────────────── public function testPageUrlProductionMode(): void @@ -174,4 +183,13 @@ public function testPageUrlExplicitModeParameter(): void $this->repository->getCheckoutPageUrl('staging') ); } + + public function testPageUrlDeveloperModeEnvVarOverridesExplicitMode(): void + { + $this->appState->method('getMode')->willReturn(State::MODE_DEVELOPER); + $this->setEnv('TWO_CHECKOUT_BASE_URL', 'http://localhost:3000'); + + // Env var takes precedence over explicit $mode in developer mode + $this->assertEquals('http://localhost:3000', $this->repository->getCheckoutPageUrl('sandbox')); + } } From 779565e98351b49284a86f213510c774a0f50097 Mon Sep 17 00:00:00 2001 From: Douglas Lindsay Date: Wed, 18 Mar 2026 10:21:02 +0000 Subject: [PATCH 3/6] ABN-287/fix: pin PHPUnit phar version and verify SHA256 checksum Pin to phpunit-9.6.34 with hardcoded SHA256 hash in both CI workflow and Makefile to prevent supply chain attacks via compromised CDN. Co-Authored-By: Claude Opus 4.6 (1M context) --- .github/workflows/phpunit.yml | 7 ++++++- Makefile | 6 +++++- 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/.github/workflows/phpunit.yml b/.github/workflows/phpunit.yml index e3c59ea..f37feb1 100644 --- a/.github/workflows/phpunit.yml +++ b/.github/workflows/phpunit.yml @@ -22,7 +22,12 @@ jobs: php-version: ${{ matrix.php-version }} - name: Download PHPUnit - run: php -r "copy('https://phar.phpunit.de/phpunit-9.6.phar', 'phpunit.phar');" + env: + PHPUNIT_VERSION: '9.6.34' + PHPUNIT_SHA256: 'e7264ae61fe58a487c2bd741905b85940d8fbc2b32cf4a279949b6d9a172a06a' + run: | + php -r "copy('https://phar.phpunit.de/phpunit-${PHPUNIT_VERSION}.phar', 'phpunit.phar');" + echo "${PHPUNIT_SHA256} phpunit.phar" | sha256sum -c - - name: Run tests run: php phpunit.phar diff --git a/Makefile b/Makefile index a0a6ed4..d3808dc 100644 --- a/Makefile +++ b/Makefile @@ -94,10 +94,14 @@ patch: bumpver-patch minor: bumpver-minor ## Bump major version major: bumpver-major +PHPUNIT_VERSION := 9.6.34 +PHPUNIT_SHA256 := e7264ae61fe58a487c2bd741905b85940d8fbc2b32cf4a279949b6d9a172a06a + ## Run PHPUnit tests test: docker run --rm -v $(CURDIR):/app -w /app php:8.1-cli bash -c \ - "php -r \"copy('https://phar.phpunit.de/phpunit-9.6.phar', '/tmp/phpunit.phar');\" \ + "php -r \"copy('https://phar.phpunit.de/phpunit-$(PHPUNIT_VERSION).phar', '/tmp/phpunit.phar');\" \ + && echo '$(PHPUNIT_SHA256) /tmp/phpunit.phar' | sha256sum -c - \ && php /tmp/phpunit.phar" ## Format frontend assets with Prettier From 51c17f30c96344a236b115823413d473738b7c69 Mon Sep 17 00:00:00 2001 From: Douglas Lindsay Date: Wed, 18 Mar 2026 16:34:22 +0000 Subject: [PATCH 4/6] ABN-287/test: add E2E test suite for Adapter against staging API Adds Test/E2E/ with a RealCurl HTTP implementation and three tests covering valid key, invalid key (401), and bad payload. make test runs Unit only (defaultTestSuite); make test-e2e forwards TWO_API_KEY/TWO_API_BASE_URL into Docker and runs the E2E suite. Missing key fails naturally at the assertion rather than skipping. --- Makefile | 12 +++++- Test/E2E/ApiAdapterTest.php | 69 +++++++++++++++++++++++++++++ Test/E2E/Http/RealCurl.php | 86 +++++++++++++++++++++++++++++++++++++ phpunit.xml | 6 ++- 4 files changed, 171 insertions(+), 2 deletions(-) create mode 100644 Test/E2E/ApiAdapterTest.php create mode 100644 Test/E2E/Http/RealCurl.php diff --git a/Makefile b/Makefile index d3808dc..129a671 100644 --- a/Makefile +++ b/Makefile @@ -14,7 +14,7 @@ TWO_API_BASE_URL ?= https://api.staging.two.inc TWO_CHECKOUT_BASE_URL ?= https://checkout.staging.two.inc TWO_STORE_COUNTRY ?= NO -.PHONY: help install configure compile run stop clean logs archive patch minor major format test +.PHONY: help install configure compile run stop clean logs archive patch minor major format test test-e2e .DEFAULT_GOAL := help @@ -104,6 +104,16 @@ test: && echo '$(PHPUNIT_SHA256) /tmp/phpunit.phar' | sha256sum -c - \ && php /tmp/phpunit.phar" +## Run end-to-end API tests (requires TWO_API_KEY) +test-e2e: + docker run --rm -v $(CURDIR):/app -w /app \ + -e TWO_API_KEY=$(TWO_API_KEY) \ + -e TWO_API_BASE_URL=$(TWO_API_BASE_URL) \ + php:8.1-cli bash -c \ + "php -r \"copy('https://phar.phpunit.de/phpunit-$(PHPUNIT_VERSION).phar', '/tmp/phpunit.phar');\" \ + && echo '$(PHPUNIT_SHA256) /tmp/phpunit.phar' | sha256sum -c - \ + && php /tmp/phpunit.phar --testsuite E2E" + ## Format frontend assets with Prettier format: prettier -w view/frontend/web/js/ diff --git a/Test/E2E/ApiAdapterTest.php b/Test/E2E/ApiAdapterTest.php new file mode 100644 index 0000000..53b174d --- /dev/null +++ b/Test/E2E/ApiAdapterTest.php @@ -0,0 +1,69 @@ +createMock(ConfigRepository::class); + $config->method('getCheckoutApiUrl')->willReturn($baseUrl); + $config->method('addVersionDataInURL')->willReturnArgument(0); + $config->method('getApiKey')->willReturn($apiKey); + + $log = $this->createMock(LogRepository::class); + + $this->adapter = new Adapter($config, new RealCurl(), $log); + } + + public function testApiKeyIsValid(): void + { + $result = $this->adapter->execute('/v1/merchant/verify_api_key', [], 'GET'); + + $this->assertIsArray($result); + $this->assertArrayNotHasKey('error_code', $result); + } + + public function testInvalidApiKeyReturns401WithStructuredError(): void + { + $baseUrl = getenv('TWO_API_BASE_URL') ?: 'https://api.staging.two.inc'; + + $config = $this->createMock(ConfigRepository::class); + $config->method('getCheckoutApiUrl')->willReturn($baseUrl); + $config->method('addVersionDataInURL')->willReturnArgument(0); + $config->method('getApiKey')->willReturn('invalid-key'); + + $log = $this->createMock(LogRepository::class); + + $adapter = new Adapter($config, new RealCurl(), $log); + $result = $adapter->execute('/v1/merchant/verify_api_key', [], 'GET'); + + $this->assertEquals(401, $result['http_status']); + } + + public function testBadOrderPayloadReturnsStructuredError(): void + { + $result = $this->adapter->execute('/v1/order', []); + + $this->assertArrayHasKey('http_status', $result); + $this->assertGreaterThanOrEqual(400, $result['http_status']); + } +} diff --git a/Test/E2E/Http/RealCurl.php b/Test/E2E/Http/RealCurl.php new file mode 100644 index 0000000..9ab60a7 --- /dev/null +++ b/Test/E2E/Http/RealCurl.php @@ -0,0 +1,86 @@ +headers[] = "$name: $value"; + } + + public function setOption(int $option, $value): void + { + $this->options[$option] = $value; + } + + public function post(string $url, $params): void + { + $this->doRequest($url, [ + CURLOPT_POST => true, + CURLOPT_POSTFIELDS => $params, + ]); + } + + public function get(string $url): void + { + $this->doRequest($url, [ + CURLOPT_HTTPGET => true, + ]); + } + + public function getBody(): string + { + return $this->body; + } + + public function getStatus(): int + { + return $this->status; + } + + public function getHeaders(): array + { + return $this->responseHeaders; + } + + private function doRequest(string $url, array $extraOptions): void + { + $ch = curl_init($url); + + $responseHeaders = []; + $curlOptions = $this->options + $extraOptions + [ + CURLOPT_HTTPHEADER => $this->headers, + CURLOPT_HEADERFUNCTION => function ($ch, $header) use (&$responseHeaders) { + $len = strlen($header); + $parts = explode(':', $header, 2); + if (count($parts) === 2) { + $responseHeaders[strtolower(trim($parts[0]))] = trim($parts[1]); + } + return $len; + }, + ]; + + curl_setopt_array($ch, $curlOptions); + + $this->body = (string)(curl_exec($ch) ?: ''); + $this->status = (int)curl_getinfo($ch, CURLINFO_HTTP_CODE); + $this->responseHeaders = $responseHeaders; + + curl_close($ch); + } +} diff --git a/phpunit.xml b/phpunit.xml index 7cd5b2a..ac53752 100644 --- a/phpunit.xml +++ b/phpunit.xml @@ -3,10 +3,14 @@ xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.6/phpunit.xsd" bootstrap="Test/bootstrap.php" colors="true" - verbose="true"> + verbose="true" + defaultTestSuite="Unit"> Test/Unit/ + + Test/E2E/ + From 11a800a99ec83435ed27fc4f6dfde09c910a6956 Mon Sep 17 00:00:00 2001 From: Douglas Lindsay Date: Wed, 18 Mar 2026 16:36:48 +0000 Subject: [PATCH 5/6] ABN-287/test: commit stubs, unit tests and docs from previous session Stubs for Curl, LocalizedException, BundlePrice, ProductType; unit tests for Adapter, OrderShouldSkip, and addVersionDataInURL; bootstrap wiring; session summary and plan docs. --- Test/Stubs/BundlePrice.php | 14 ++ Test/Stubs/Curl.php | 45 +++++ Test/Stubs/LocalizedException.php | 29 ++++ Test/Stubs/ProductType.php | 13 ++ Test/Unit/Model/Config/RepositoryUrlTest.php | 35 ++++ Test/Unit/Service/Api/AdapterTest.php | 172 +++++++++++++++++++ Test/Unit/Service/OrderShouldSkipTest.php | 132 ++++++++++++++ Test/bootstrap.php | 13 ++ docs/plans/improve-api-error-handling.md | 73 ++++++++ docs/session-summary-2026-03-17.md | 100 +++++++++++ 10 files changed, 626 insertions(+) create mode 100644 Test/Stubs/BundlePrice.php create mode 100644 Test/Stubs/Curl.php create mode 100644 Test/Stubs/LocalizedException.php create mode 100644 Test/Stubs/ProductType.php create mode 100644 Test/Unit/Service/Api/AdapterTest.php create mode 100644 Test/Unit/Service/OrderShouldSkipTest.php create mode 100644 docs/plans/improve-api-error-handling.md create mode 100644 docs/session-summary-2026-03-17.md diff --git a/Test/Stubs/BundlePrice.php b/Test/Stubs/BundlePrice.php new file mode 100644 index 0000000..a315cec --- /dev/null +++ b/Test/Stubs/BundlePrice.php @@ -0,0 +1,14 @@ +phrase = $phrase; + parent::__construct($phrase->render(), $code, $cause); + } + + public function getLogMessage(): string + { + return $this->phrase->render(); + } +} diff --git a/Test/Stubs/ProductType.php b/Test/Stubs/ProductType.php new file mode 100644 index 0000000..39dbf95 --- /dev/null +++ b/Test/Stubs/ProductType.php @@ -0,0 +1,13 @@ +assertEquals('http://localhost:3000', $this->repository->getCheckoutPageUrl('sandbox')); } + + // ── addVersionDataInURL ───────────────────────────────────────────── + + public function testAddVersionDataAppendsQueryStringToPlainUrl(): void + { + $this->scopeConfig->method('getValue')->willReturn('1.2.3'); + + $result = $this->repository->addVersionDataInURL('https://api.two.inc/v1/order'); + + $this->assertStringStartsWith('https://api.two.inc/v1/order?', $result); + $this->assertStringContainsString('client=Magento', $result); + $this->assertStringContainsString('client_v=1.2.3', $result); + } + + public function testAddVersionDataAppendsWithAmpersandWhenQueryExists(): void + { + $this->scopeConfig->method('getValue')->willReturn('1.2.3'); + + $result = $this->repository->addVersionDataInURL('https://api.two.inc/v1/order?foo=bar'); + + $this->assertStringContainsString('?foo=bar&', $result); + $this->assertStringContainsString('client=Magento', $result); + $this->assertStringContainsString('client_v=1.2.3', $result); + } + + public function testAddVersionDataWithNullVersionOmitsVersionParam(): void + { + $this->scopeConfig->method('getValue')->willReturn(null); + + $result = $this->repository->addVersionDataInURL('https://api.two.inc/v1/order'); + + $this->assertStringContainsString('client=Magento', $result); + // http_build_query omits null values entirely + $this->assertStringNotContainsString('client_v', $result); + } } diff --git a/Test/Unit/Service/Api/AdapterTest.php b/Test/Unit/Service/Api/AdapterTest.php new file mode 100644 index 0000000..3e922f3 --- /dev/null +++ b/Test/Unit/Service/Api/AdapterTest.php @@ -0,0 +1,172 @@ +configRepository = $this->createMock(ConfigRepository::class); + $this->curl = $this->createMock(Curl::class); + $this->logRepository = $this->createMock(LogRepository::class); + + $this->configRepository->method('getCheckoutApiUrl')->willReturn('https://api.two.inc'); + $this->configRepository->method('addVersionDataInURL')->willReturnArgument(0); + $this->configRepository->method('getApiKey')->willReturn('test-key'); + + $this->adapter = new Adapter( + $this->configRepository, + $this->curl, + $this->logRepository + ); + } + + // ── 2xx responses ─────────────────────────────────────────────────── + + public function testSuccessfulPostReturnsDecodedJson(): void + { + $this->curl->method('getStatus')->willReturn(200); + $this->curl->method('getBody')->willReturn('{"id":"abc"}'); + + $result = $this->adapter->execute('/v1/order', ['amount' => 100]); + + $this->assertEquals(['id' => 'abc'], $result); + } + + public function testGetRoutesThoughGetMethod(): void + { + $this->curl->method('getStatus')->willReturn(200); + $this->curl->method('getBody')->willReturn('{"ok":true}'); + + $this->curl->expects($this->once())->method('get'); + $this->curl->expects($this->never())->method('post'); + + $result = $this->adapter->execute('/v1/order/123', [], 'GET'); + + $this->assertEquals(['ok' => true], $result); + } + + public function testSuccessEmptyBodyNonTokenEndpoint(): void + { + $this->curl->method('getStatus')->willReturn(200); + $this->curl->method('getBody')->willReturn(''); + + $result = $this->adapter->execute('/v1/order', ['foo' => 'bar']); + + $this->assertEquals([], $result); + } + + public function testSuccessEmptyBodyTokenEndpointReturnsHeaders(): void + { + $this->curl->method('getStatus')->willReturn(200); + $this->curl->method('getBody')->willReturn(''); + $this->curl->method('getHeaders')->willReturn([ + 'X-Delegation-Token' => 'abc123', + 'Content-Type' => 'application/json', + ]); + + $result = $this->adapter->execute('/registry/v1/delegation'); + + $this->assertArrayHasKey('x-delegation-token', $result); + $this->assertEquals('abc123', $result['x-delegation-token']); + } + + // ── Non-2xx responses (ABN-287 critical) ──────────────────────────── + + public function testNon2xxWithBodyReturnsJsonPlusHttpStatus(): void + { + $this->curl->method('getStatus')->willReturn(422); + $this->curl->method('getBody')->willReturn( + '{"error_code":"VALIDATION_ERROR","error_message":"Invalid field"}' + ); + + $result = $this->adapter->execute('/v1/order', ['amount' => 100]); + + $this->assertEquals('VALIDATION_ERROR', $result['error_code']); + $this->assertEquals('Invalid field', $result['error_message']); + $this->assertEquals(422, $result['http_status']); + } + + public function testNon2xxWithMalformedJsonBody(): void + { + $this->curl->method('getStatus')->willReturn(500); + $this->curl->method('getBody')->willReturn('not json'); + + $result = $this->adapter->execute('/v1/order', ['amount' => 100]); + + $this->assertEquals(500, $result['http_status']); + } + + public function testNon2xxWithEmptyBodyReturnsCaughtException(): void + { + $this->curl->method('getStatus')->willReturn(500); + $this->curl->method('getBody')->willReturn(''); + + $result = $this->adapter->execute('/v1/order', ['amount' => 100]); + + $this->assertEquals(400, $result['error_code']); + $this->assertStringContainsString('Invalid API response from Two.', $result['error_message']); + } + + // ── Edge cases ────────────────────────────────────────────────────── + + public function testPostWithEmptyPayloadSendsEmptyString(): void + { + $this->curl->method('getStatus')->willReturn(200); + $this->curl->method('getBody')->willReturn('[]'); + + $this->curl->expects($this->once()) + ->method('post') + ->with($this->anything(), ''); + + $this->adapter->execute('/v1/order', []); + } + + public function testPutRoutesThoughPostBranch(): void + { + $this->curl->method('getStatus')->willReturn(200); + $this->curl->method('getBody')->willReturn('{}'); + + $this->curl->expects($this->once())->method('post'); + $this->curl->expects($this->never())->method('get'); + + $this->adapter->execute('/v1/order/123', ['status' => 'fulfilled'], 'PUT'); + } + + public function testExceptionDuringRequestReturnsCaughtError(): void + { + $this->configRepository = $this->createMock(ConfigRepository::class); + $this->configRepository->method('getCheckoutApiUrl') + ->willThrowException(new \RuntimeException('Connection failed')); + + $adapter = new Adapter( + $this->configRepository, + $this->curl, + $this->logRepository + ); + + $result = $adapter->execute('/v1/order'); + + $this->assertEquals(400, $result['error_code']); + $this->assertEquals('Connection failed', $result['error_message']); + } +} diff --git a/Test/Unit/Service/OrderShouldSkipTest.php b/Test/Unit/Service/OrderShouldSkipTest.php new file mode 100644 index 0000000..343b3d6 --- /dev/null +++ b/Test/Unit/Service/OrderShouldSkipTest.php @@ -0,0 +1,132 @@ +orderService = $this->getMockForAbstractClass( + Order::class, + [], + '', + false // don't call constructor + ); + } + + /** + * Create a mock item with getProductType() and getProduct()->getPriceType(). + */ + private function makeItem(string $productType, ?int $priceType = null): object + { + $item = new \stdClass(); + $item->productType = $productType; + $item->priceType = $priceType; + + // We need an object with getProductType() and getProduct() methods. + // Use an anonymous class for clean mocking. + return new class($productType, $priceType) { + private $productType; + private $priceType; + + public function __construct(string $productType, ?int $priceType) + { + $this->productType = $productType; + $this->priceType = $priceType; + } + + public function getProductType(): string + { + return $this->productType; + } + + public function getProduct(): object + { + $priceType = $this->priceType; + return new class($priceType) { + private $priceType; + + public function __construct(?int $priceType) + { + $this->priceType = $priceType; + } + + public function getPriceType(): ?int + { + return $this->priceType; + } + }; + } + }; + } + + // ── Bundle items (no parent) ──────────────────────────────────────── + + public function testBundleDynamicPricingIsSkipped(): void + { + $item = $this->makeItem(Type::TYPE_BUNDLE, Price::PRICE_TYPE_DYNAMIC); + + $this->assertTrue($this->orderService->shouldSkip(null, $item)); + } + + public function testBundleFixedPricingIsNotSkipped(): void + { + $item = $this->makeItem(Type::TYPE_BUNDLE, Price::PRICE_TYPE_FIXED); + + $this->assertFalse($this->orderService->shouldSkip(null, $item)); + } + + // ── Simple items (no parent) ──────────────────────────────────────── + + public function testSimpleItemNoParentIsNotSkipped(): void + { + $item = $this->makeItem('simple'); + + $this->assertFalse($this->orderService->shouldSkip(null, $item)); + } + + // ── Child items with non-bundle parent ────────────────────────────── + + public function testChildOfConfigurableIsSkipped(): void + { + $parent = $this->makeItem('configurable'); + $item = $this->makeItem('simple'); + + $this->assertTrue($this->orderService->shouldSkip($parent, $item)); + } + + public function testChildOfGroupedIsSkipped(): void + { + $parent = $this->makeItem('grouped'); + $item = $this->makeItem('simple'); + + $this->assertTrue($this->orderService->shouldSkip($parent, $item)); + } + + // ── Child items with bundle parent ────────────────────────────────── + + public function testChildOfBundleFixedPriceIsSkipped(): void + { + $parent = $this->makeItem(Type::TYPE_BUNDLE, Price::PRICE_TYPE_FIXED); + $item = $this->makeItem('simple'); + + $this->assertTrue($this->orderService->shouldSkip($parent, $item)); + } + + public function testChildOfBundleDynamicPriceIsNotSkipped(): void + { + $parent = $this->makeItem(Type::TYPE_BUNDLE, Price::PRICE_TYPE_DYNAMIC); + $item = $this->makeItem('simple'); + + $this->assertFalse($this->orderService->shouldSkip($parent, $item)); + } +} diff --git a/Test/bootstrap.php b/Test/bootstrap.php index 0588e1d..aebfbd3 100644 --- a/Test/bootstrap.php +++ b/Test/bootstrap.php @@ -34,6 +34,19 @@ require_once __DIR__ . '/Stubs/ScopeConfigInterface.php'; } +if (!class_exists(\Magento\Framework\HTTP\Client\Curl::class)) { + require_once __DIR__ . '/Stubs/Curl.php'; +} +if (!class_exists(\Magento\Framework\Exception\LocalizedException::class)) { + require_once __DIR__ . '/Stubs/LocalizedException.php'; +} +if (!class_exists(\Magento\Bundle\Model\Product\Price::class)) { + require_once __DIR__ . '/Stubs/BundlePrice.php'; +} +if (!class_exists(\Magento\Catalog\Model\Product\Type::class)) { + require_once __DIR__ . '/Stubs/ProductType.php'; +} + // Catch-all autoloader for remaining Magento classes/interfaces. // Creates empty stubs so that type hints, extends, and implements resolve. spl_autoload_register(function ($class) { diff --git a/docs/plans/improve-api-error-handling.md b/docs/plans/improve-api-error-handling.md new file mode 100644 index 0000000..044b970 --- /dev/null +++ b/docs/plans/improve-api-error-handling.md @@ -0,0 +1,73 @@ +# Improve API validation error handling + +## Problem + +When the Two API returns a 400 with validation errors (e.g. invalid phone number), the plugin shows a generic message like "Phone Number is not valid." or "Something went wrong with your request to Two." instead of surfacing the actual detail from the API. + +Example API response: +``` +1 validation error for CreateOrderRequestSchema: buyer: + Value error, Invalid phone number for GB: 00123456789 + [type=value_error, input_value={...}, input_type=dict] +``` + +User sees: "Something went wrong with your request to Two. Please try again later." + +## Root cause + +`Model/Two.php` has two methods that handle API errors: + +### `getErrorFromResponse()` (line 296-330) + +- Iterates `error_json` array from the API response +- Calls `getFieldFromLocStr()` for each error using only the `loc` field +- Ignores the `msg` field entirely — this is where the useful detail lives +- Falls back to generic error if `loc` doesn't match the hardcoded mapping + +### `getFieldFromLocStr()` (line 342-360) + +- Hardcoded mapping of `loc` arrays to generic messages: + - `["buyer","representative","phone_number"]` -> "Phone Number is not valid." + - `["buyer","company","organization_number"]` -> "Company ID is not valid." + - etc. +- Returns `null` for any `loc` not in the mapping, triggering the generic fallback +- The mapping is incomplete and the messages discard the API's actual explanation + +## Proposed fix + +Use the `msg` field from `error_json` entries to build user-facing messages. Each entry has: + +```json +{ + "loc": ["buyer", "representative", "phone_number"], + "msg": "Value error, Invalid phone number for GB: 00123456789", + "type": "value_error" +} +``` + +### Changes to `Model/Two.php` + +1. **`getErrorFromResponse()`**: When iterating `error_json`, extract `msg` from each error and use it in the displayed message. Use the field mapping for the field name prefix, but append the API message for detail. + +2. **`getFieldFromLocStr()`**: Refactor to return just the field name (e.g. "Phone Number") rather than a full sentence, so it can be composed with the API message. + +3. **Fallback**: If `msg` is missing, fall back to the current generic field messages. If `loc` is unrecognised, use the `msg` directly without a field prefix. + +### Example output after fix + +- "Phone Number: Invalid phone number for GB: 00123456789" +- "Company ID: Organization number is not valid for country GB" + +Instead of: + +- "Phone Number is not valid." +- "Something went wrong with your request to Two." + +## Files to modify + +- `Model/Two.php` — `getErrorFromResponse()` and `getFieldFromLocStr()` + +## Notes + +- The `msg` from pydantic often starts with "Value error, " — consider stripping that prefix for cleaner display +- The JS-side error handling (`view/frontend/web/js/view/payment/method-renderer/two_payment.js` line 287) has a separate `processOrderIntentErrorResponse` for Order Intent calls — may want to align that too diff --git a/docs/session-summary-2026-03-17.md b/docs/session-summary-2026-03-17.md new file mode 100644 index 0000000..0fb0f08 --- /dev/null +++ b/docs/session-summary-2026-03-17.md @@ -0,0 +1,100 @@ +# Session Summary: ABN-287 — Local Dev Setup & Error Handling + +## Branches + +### `doug/ABN-287` — Local development environment setup +Base: `main` + +**Changes:** +- `Model/Config/Repository.php` — env var overrides (`TWO_API_BASE_URL`, `TWO_CHECKOUT_BASE_URL`) gated behind Magento developer mode via `State::MODE_DEVELOPER` +- `Makefile` — full local dev setup with targets: `help`, `install`, `configure`, `compile`, `run`, `stop`, `clean`, `logs` plus existing release targets +- `.env.local.example` — template for local config (TWO_API_BASE_URL, TWO_CHECKOUT_BASE_URL, TWO_API_KEY) +- `.gitignore` — added `.env.local` +- `dev/configure` — PHP helper script (no `.php` extension to avoid Magento DI compile scanner) that sets payment config with encrypted API key, shows updated/unchanged status for each config path, requires real API key if current key is still `dummy-dev-key` + +### `doug/ABN-287-fix-compile-error` — DI compile fix +Base: `doug/ABN-287` (merged via PR #76) + +**The problem:** Magento's `setup:di:compile` scans all `.php` files under the module root. `dev/configure.php` contained `$obj->get(...)` calls that the DI compiler misinterpreted as class references, causing "Class get does not exist" on Magento 2.3.7 / PHP 7.3. + +**The fix:** Renamed `dev/configure.php` to `dev/configure` (with shebang, no `.php` extension) so the DI scanner skips it. Verified on both PHP 7.3/Magento 2.3.7 and PHP 8.2/Magento 2.4.6. + +### `doug/ABN-287-error-handling` — API error handling improvements +Base: includes both above branches (merged) + +**Changes to `Model/Two.php`:** + +1. **Error categorisation** — errors are classified as user or system: + - **User errors**: HTTP 400 + `error_json` (validation) or recognised `error_code` (`SCHEMA_ERROR`, `SAME_BUYER_SELLER_ERROR`, `ORDER_INVALID`) + - **System errors**: everything else + - User errors: clean message, no trace ID, no "Your request to Two failed" prefix + - System errors: "Your request to Two failed. Reason: ..." prefix with trace ID + +2. **`getErrorFromResponse()`** refactored: + - Checks `$response['http_status'] == 400` to gate user error treatment + - Validation errors (`error_json`): extracts `msg` field from pydantic errors, composes with field name from `loc` + - Uses `cleanValidationMessage()` to strip pydantic prefixes ("Value error, ") and suffixes ("[type=...") + +3. **`getFieldFromLocStr()` replaced with `getFieldNameFromLoc()`**: + - Returns just the field name (e.g. "Phone Number") not a full sentence + - Uses `static` cache for the translation array + - Mapping: phone_number, organization_number, first_name, last_name, email, street_address, city, country, postal_code + +4. **New `cleanValidationMessage()` private method**: + - Strips "Value error, " prefix (case-insensitive) + - Strips "[type=..." suffix + - Trims whitespace + +**Changes to `Service/Api/Adapter.php`:** +- Added `$result['http_status']` to error response arrays so `getErrorFromResponse()` can distinguish 400 from other status codes + +**Translation files** (nb_NO, sv_SE, nl_NL): +- Replaced full-sentence entries ("Phone Number is not valid.") with standalone field names ("Phone Number") +- Added `"%1 is not valid."` and `"%1: %2."` format strings + +**Example user-facing messages after changes:** +- Validation: `Phone Number: Invalid phone number for GB: 1234567.` +- Validation (unknown loc): `Invalid phone number for GB: 1234567` +- System: `Your request to Two failed. Reason: X-API-Key is incorrect or has expired [Trace ID: abc123]` + +## Key decisions & gotchas + +1. **Dev script must NOT have `.php` extension** — Magento DI compiler scans all `.php` files in the module root and chokes on non-class PHP files with `->get()` calls +2. **`State` injection is safe** — the DI compile error was caused by `dev/configure.php`, not the `State` constructor param. Confirmed by testing after renaming the script. +3. **`getenv('MAGE_MODE')` does NOT work** — `deploy:mode:set developer` writes to `app/etc/env.php`, not the process environment. Must use `Magento\Framework\App\State::getMode()` instead. +4. **API key must be encrypted** — `config:set` stores plaintext but `getApiKey()` calls `decrypt()`. The `dev/configure` script handles encryption via Magento's `EncryptorInterface`. +5. **User vs system error classification** — user errors (no trace ID) only for HTTP 400 + known error codes. Default to system error (with trace ID) to avoid losing diagnostic info. +6. **`docker restart` after configure** — config changes require a container restart to take effect. +7. **PSR-4 autoload maps `Two\Gateway\` to `""`** — the entire package root is scanned, so any `.php` file anywhere in the repo is fair game for the DI compiler. + +## Makefile targets + +``` +help Show this help +install Create Magento container, install plugin, configure payment method +configure Update payment config: TWO_API_KEY=xxx make configure +compile Recompile DI and restart +run Start the Magento container +stop Stop the Magento container +clean Remove the Magento container +logs Tail Two plugin logs +archive Create a versioned zip archive +patch/minor/major Bump version +format Format frontend assets with Prettier +``` + +## Files modified (across all branches) + +``` +.env.local.example — NEW: template for local dev config +.gitignore — added .env.local +Makefile — full rewrite with dev targets +Model/Config/Repository.php — State injection, env var URL overrides in dev mode +Model/Two.php — error handling refactor +Service/Api/Adapter.php — http_status in error responses +dev/configure — NEW: PHP helper for encrypted config (no .php extension) +i18n/nb_NO.csv — updated translations +i18n/nl_NL.csv — updated translations +i18n/sv_SE.csv — updated translations +docs/plans/improve-api-error-handling.md — plan doc +``` From 8ac0ceca6e948a761aaec9d16661bfdeafe84d66 Mon Sep 17 00:00:00 2001 From: Douglas Lindsay Date: Thu, 26 Mar 2026 14:45:30 +0000 Subject: [PATCH 6/6] chore: remove session artifacts from public repo Remove session summaries and implementation plans that were accidentally committed. Add Session Artifacts section to AGENTS.md clarifying that session-specific content must not be committed to this public repository. Co-Authored-By: Claude Opus 4.6 (1M context) --- AGENTS.md | 9 ++ docs/plans/improve-api-error-handling.md | 73 ----------------- docs/session-summary-2026-03-17.md | 100 ----------------------- 3 files changed, 9 insertions(+), 173 deletions(-) delete mode 100644 docs/plans/improve-api-error-handling.md delete mode 100644 docs/session-summary-2026-03-17.md diff --git a/AGENTS.md b/AGENTS.md index 5ce01ac..465beba 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -98,6 +98,15 @@ bin/magento cache:flush # Create pub/opcache-clear.php or restart PHP-FPM ``` +## Session Artifacts + +This is a **public repository**. Do not commit session-specific content such as: +- Session summaries or transcripts +- Implementation plans or review notes +- Any file under `docs/` that contains conversation context + +Use agent memory (e.g. `~/.claude/projects/` or equivalent) for session persistence instead. Plans can be saved locally and stashed but must not be committed. + ### Common Issues 1. **Template not found error**: Run `setup:di:compile` and clear opcache diff --git a/docs/plans/improve-api-error-handling.md b/docs/plans/improve-api-error-handling.md deleted file mode 100644 index 044b970..0000000 --- a/docs/plans/improve-api-error-handling.md +++ /dev/null @@ -1,73 +0,0 @@ -# Improve API validation error handling - -## Problem - -When the Two API returns a 400 with validation errors (e.g. invalid phone number), the plugin shows a generic message like "Phone Number is not valid." or "Something went wrong with your request to Two." instead of surfacing the actual detail from the API. - -Example API response: -``` -1 validation error for CreateOrderRequestSchema: buyer: - Value error, Invalid phone number for GB: 00123456789 - [type=value_error, input_value={...}, input_type=dict] -``` - -User sees: "Something went wrong with your request to Two. Please try again later." - -## Root cause - -`Model/Two.php` has two methods that handle API errors: - -### `getErrorFromResponse()` (line 296-330) - -- Iterates `error_json` array from the API response -- Calls `getFieldFromLocStr()` for each error using only the `loc` field -- Ignores the `msg` field entirely — this is where the useful detail lives -- Falls back to generic error if `loc` doesn't match the hardcoded mapping - -### `getFieldFromLocStr()` (line 342-360) - -- Hardcoded mapping of `loc` arrays to generic messages: - - `["buyer","representative","phone_number"]` -> "Phone Number is not valid." - - `["buyer","company","organization_number"]` -> "Company ID is not valid." - - etc. -- Returns `null` for any `loc` not in the mapping, triggering the generic fallback -- The mapping is incomplete and the messages discard the API's actual explanation - -## Proposed fix - -Use the `msg` field from `error_json` entries to build user-facing messages. Each entry has: - -```json -{ - "loc": ["buyer", "representative", "phone_number"], - "msg": "Value error, Invalid phone number for GB: 00123456789", - "type": "value_error" -} -``` - -### Changes to `Model/Two.php` - -1. **`getErrorFromResponse()`**: When iterating `error_json`, extract `msg` from each error and use it in the displayed message. Use the field mapping for the field name prefix, but append the API message for detail. - -2. **`getFieldFromLocStr()`**: Refactor to return just the field name (e.g. "Phone Number") rather than a full sentence, so it can be composed with the API message. - -3. **Fallback**: If `msg` is missing, fall back to the current generic field messages. If `loc` is unrecognised, use the `msg` directly without a field prefix. - -### Example output after fix - -- "Phone Number: Invalid phone number for GB: 00123456789" -- "Company ID: Organization number is not valid for country GB" - -Instead of: - -- "Phone Number is not valid." -- "Something went wrong with your request to Two." - -## Files to modify - -- `Model/Two.php` — `getErrorFromResponse()` and `getFieldFromLocStr()` - -## Notes - -- The `msg` from pydantic often starts with "Value error, " — consider stripping that prefix for cleaner display -- The JS-side error handling (`view/frontend/web/js/view/payment/method-renderer/two_payment.js` line 287) has a separate `processOrderIntentErrorResponse` for Order Intent calls — may want to align that too diff --git a/docs/session-summary-2026-03-17.md b/docs/session-summary-2026-03-17.md deleted file mode 100644 index 0fb0f08..0000000 --- a/docs/session-summary-2026-03-17.md +++ /dev/null @@ -1,100 +0,0 @@ -# Session Summary: ABN-287 — Local Dev Setup & Error Handling - -## Branches - -### `doug/ABN-287` — Local development environment setup -Base: `main` - -**Changes:** -- `Model/Config/Repository.php` — env var overrides (`TWO_API_BASE_URL`, `TWO_CHECKOUT_BASE_URL`) gated behind Magento developer mode via `State::MODE_DEVELOPER` -- `Makefile` — full local dev setup with targets: `help`, `install`, `configure`, `compile`, `run`, `stop`, `clean`, `logs` plus existing release targets -- `.env.local.example` — template for local config (TWO_API_BASE_URL, TWO_CHECKOUT_BASE_URL, TWO_API_KEY) -- `.gitignore` — added `.env.local` -- `dev/configure` — PHP helper script (no `.php` extension to avoid Magento DI compile scanner) that sets payment config with encrypted API key, shows updated/unchanged status for each config path, requires real API key if current key is still `dummy-dev-key` - -### `doug/ABN-287-fix-compile-error` — DI compile fix -Base: `doug/ABN-287` (merged via PR #76) - -**The problem:** Magento's `setup:di:compile` scans all `.php` files under the module root. `dev/configure.php` contained `$obj->get(...)` calls that the DI compiler misinterpreted as class references, causing "Class get does not exist" on Magento 2.3.7 / PHP 7.3. - -**The fix:** Renamed `dev/configure.php` to `dev/configure` (with shebang, no `.php` extension) so the DI scanner skips it. Verified on both PHP 7.3/Magento 2.3.7 and PHP 8.2/Magento 2.4.6. - -### `doug/ABN-287-error-handling` — API error handling improvements -Base: includes both above branches (merged) - -**Changes to `Model/Two.php`:** - -1. **Error categorisation** — errors are classified as user or system: - - **User errors**: HTTP 400 + `error_json` (validation) or recognised `error_code` (`SCHEMA_ERROR`, `SAME_BUYER_SELLER_ERROR`, `ORDER_INVALID`) - - **System errors**: everything else - - User errors: clean message, no trace ID, no "Your request to Two failed" prefix - - System errors: "Your request to Two failed. Reason: ..." prefix with trace ID - -2. **`getErrorFromResponse()`** refactored: - - Checks `$response['http_status'] == 400` to gate user error treatment - - Validation errors (`error_json`): extracts `msg` field from pydantic errors, composes with field name from `loc` - - Uses `cleanValidationMessage()` to strip pydantic prefixes ("Value error, ") and suffixes ("[type=...") - -3. **`getFieldFromLocStr()` replaced with `getFieldNameFromLoc()`**: - - Returns just the field name (e.g. "Phone Number") not a full sentence - - Uses `static` cache for the translation array - - Mapping: phone_number, organization_number, first_name, last_name, email, street_address, city, country, postal_code - -4. **New `cleanValidationMessage()` private method**: - - Strips "Value error, " prefix (case-insensitive) - - Strips "[type=..." suffix - - Trims whitespace - -**Changes to `Service/Api/Adapter.php`:** -- Added `$result['http_status']` to error response arrays so `getErrorFromResponse()` can distinguish 400 from other status codes - -**Translation files** (nb_NO, sv_SE, nl_NL): -- Replaced full-sentence entries ("Phone Number is not valid.") with standalone field names ("Phone Number") -- Added `"%1 is not valid."` and `"%1: %2."` format strings - -**Example user-facing messages after changes:** -- Validation: `Phone Number: Invalid phone number for GB: 1234567.` -- Validation (unknown loc): `Invalid phone number for GB: 1234567` -- System: `Your request to Two failed. Reason: X-API-Key is incorrect or has expired [Trace ID: abc123]` - -## Key decisions & gotchas - -1. **Dev script must NOT have `.php` extension** — Magento DI compiler scans all `.php` files in the module root and chokes on non-class PHP files with `->get()` calls -2. **`State` injection is safe** — the DI compile error was caused by `dev/configure.php`, not the `State` constructor param. Confirmed by testing after renaming the script. -3. **`getenv('MAGE_MODE')` does NOT work** — `deploy:mode:set developer` writes to `app/etc/env.php`, not the process environment. Must use `Magento\Framework\App\State::getMode()` instead. -4. **API key must be encrypted** — `config:set` stores plaintext but `getApiKey()` calls `decrypt()`. The `dev/configure` script handles encryption via Magento's `EncryptorInterface`. -5. **User vs system error classification** — user errors (no trace ID) only for HTTP 400 + known error codes. Default to system error (with trace ID) to avoid losing diagnostic info. -6. **`docker restart` after configure** — config changes require a container restart to take effect. -7. **PSR-4 autoload maps `Two\Gateway\` to `""`** — the entire package root is scanned, so any `.php` file anywhere in the repo is fair game for the DI compiler. - -## Makefile targets - -``` -help Show this help -install Create Magento container, install plugin, configure payment method -configure Update payment config: TWO_API_KEY=xxx make configure -compile Recompile DI and restart -run Start the Magento container -stop Stop the Magento container -clean Remove the Magento container -logs Tail Two plugin logs -archive Create a versioned zip archive -patch/minor/major Bump version -format Format frontend assets with Prettier -``` - -## Files modified (across all branches) - -``` -.env.local.example — NEW: template for local dev config -.gitignore — added .env.local -Makefile — full rewrite with dev targets -Model/Config/Repository.php — State injection, env var URL overrides in dev mode -Model/Two.php — error handling refactor -Service/Api/Adapter.php — http_status in error responses -dev/configure — NEW: PHP helper for encrypted config (no .php extension) -i18n/nb_NO.csv — updated translations -i18n/nl_NL.csv — updated translations -i18n/sv_SE.csv — updated translations -docs/plans/improve-api-error-handling.md — plan doc -```