Skip to content

fix(insurance): refresh stored carrier data so insurance keeps working - #624

Open
FreekVR wants to merge 5 commits into
mainfrom
fix/INT-1699-refresh-carrier-capabilities
Open

fix(insurance): refresh stored carrier data so insurance keeps working#624
FreekVR wants to merge 5 commits into
mainfrom
fix/INT-1699-refresh-carrier-capabilities

Conversation

@FreekVR

@FreekVR FreekVR commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

refreshes the stored carrier data on update so insurance keeps working when the API changes format, and stops unit tests calling the live API.

Carrier data stored before this release holds insurance limits in the nested wrapper the MyParcel API is removing (Core API story SB-2770). The PDK now reads the flat limits, which that older data does not have, so insurance would be unavailable in the shop until something refreshed it.

Adds a timestamped migration that fetches the contract definitions again and rewrites the stored carriers in the shape the PDK expects. It runs once per install, tracked by identity rather than plugin version. When the API cannot be reached the migration reports failure through the PDK's markFailed(), so it stays unrecorded and is retried on the next load without taking the page down. PrestaShop runs migrations in the request, with no background worker to fall back on, so throwing meant a merchant hitting the same fatal on every load until the API recovered.

Tested against both formats the API can send: the flat limits on their own, and the flat limits alongside the deprecated nested wrapper as it sends today. Both store the same thing, with no nested wrapper left behind. The test drives the real capabilities service rather than a stub, so it is the actual behaviour being exercised.

Separately, the first commit fixes something that turned up while writing that test. The bootstrapper mocks the client adapter, which covers the legacy API layer, but the SdkApi services built their own HTTP client and slipped past it — so any test touching capabilities, shipping rule implications or whoami sent a real request to the live API, surfacing as a 401 rather than a useful failure. Those services now take their transport through the constructor, so one binding points all of them at the mock queue, including any service added later. Worth a look on its own: it affects every test in the module, not just this migration.

Now on PDK 4.7.0, which carries the injectable transport (myparcelnl/pdk#512) and markFailed() (myparcelnl/pdk#519).

Still to do before this can be released: myparcelnl/pdk#511 for the flat insurance format has to merge and ship. Until it does, the flat limits alongside the deprecated nested wrapper test stays red, because the PDK does not drop the nested wrapper yet. With #511 merged locally the whole suite passes: 196 passing, 4 skipped, and no outbound host contacted.

Resolves INT-1699
Part of INT-1695

🤖 Generated with Claude Code

Copilot AI review requested due to automatic review settings July 29, 2026 12:32
@FreekVR
FreekVR requested a review from a team as a code owner July 29, 2026 12:32
@codecov

codecov Bot commented Jul 29, 2026

Copy link
Copy Markdown

❌ 1 Tests Failed:

Tests completed Failed Passed Skipped
202 1 201 4
View the top 1 failed test(s) by shortest run time
Tests.Unit.Migration.RefreshCarrierCapabilitiesMigrationTest::it stores only the flat insurance limits with data set "flat limits alongside the deprecated nested wrapper"
Stack Traces | 0.0439s run time
.../Unit/Migration/RefreshCarrierCapabilitiesMigrationTest.php::it stores only the flat insurance limits with data set "flat limits alongside the deprecated nested wrapper"
Failed asserting that MyParcelNL\Sdk\Client\Generated\CoreApi\Model\RefCapabilitiesSharedOptionsInsuranceBaseInsuranceV2InsuredAmount Object &00000000000032870000000000000000 (
    'openAPINullablesSetToNull' => Array &0 ()
    'container' => Array &1 (
        'default' => MyParcelNL\Sdk\Client\Generated\CoreApi\Model\RefTypesMoney Object &0000000000003b410000000000000000 (
            'openAPINullablesSetToNull' => Array &2 ()
            'container' => Array &3 (
                'currency' => 'EUR'
                'amount' => 3
            )
        )
        'min' => MyParcelNL\Sdk\Client\Generated\CoreApi\Model\RefTypesMoney Object &00000000000040930000000000000000 (
            'openAPINullablesSetToNull' => Array &4 ()
            'container' => Array &5 (
                'currency' => 'EUR'
                'amount' => 1
            )
        )
        'max' => MyParcelNL\Sdk\Client\Generated\CoreApi\Model\RefTypesMoney Object &0000000000003ff70000000000000000 (
            'openAPINullablesSetToNull' => Array &6 ()
            'container' => Array &7 (
                'currency' => 'EUR'
                'amount' => 2
            )
        )
    )
) is null.

.../pest/src/Expectation.php:704
.../Unit/Migration/RefreshCarrierCapabilitiesMigrationTest.php:142
.../src/Factories/TestCaseFactory.php:151
.../src/Concerns/Testable.php:301
.../src/Support/ExceptionTrace.php:29
.../src/Concerns/Testable.php:300
.../src/Concerns/Testable.php:278
.../src/Console/Command.php:119
.../pest/bin/pest:62
.../pest/bin/pest:63

To view more test analytics, go to the Test Analytics Dashboard
📋 Got 3 mins? Take this short survey to help us improve Test Analytics.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR ensures insurance remains available after the MyParcel API removes the deprecated nested insuredAmount wrapper by forcing a one-time refresh of stored carrier capabilities, and it hardens the test bootstrap so unit tests don’t accidentally call the live API via SdkApi services.

Changes:

  • Add a timestamped migration that re-fetches contract definitions and stores carriers in the flat insurance-limits format expected by PDK 4.x.
  • Add unit coverage for the migration across both API response shapes (flat-only vs flat + deprecated nested wrapper).
  • Update the PrestaShop test bootstrapper to bind SdkClientFactoryInterface to a mock factory so SdkApi services consistently use the mock transport.

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
src/Migration/2026_07_29_113506_refresh_carrier_capabilities.php New timestamped migration to refresh stored carrier capabilities and persist flat insurance limits.
tests/Unit/Migration/RefreshCarrierCapabilitiesMigrationTest.php New tests validating retry behavior and flat-limit storage across API response formats.
tests/Bootstrap/MockPsPdkBootstrapper.php Bind SdkApi client factory to a mock to prevent outbound requests during unit tests.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/Migration/2026_07_29_113506_refresh_carrier_capabilities.php Outdated
Comment thread src/Migration/2026_07_29_113506_refresh_carrier_capabilities.php Outdated
@myparcel-bot
myparcel-bot Bot force-pushed the fix/INT-1699-refresh-carrier-capabilities branch 3 times, most recently from 2f3a152 to 4c6bdb0 Compare August 4, 2026 09:09
} catch (Throwable $exception) {
// Re-throw so the migration is not marked as applied, letting it retry on the next
// load instead of leaving the carriers on the old shape.
Logger::warning('Failed to refresh carrier capabilities; migration will retry.', [

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, but does this possibly run the migration on every request, when failing? I gues this is an edge case and therefore alright? Just checking

@FreekVR FreekVR Aug 6, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it keeps retrying the migration until it passes.

However in Presta, this would block the page load (doesnt have any background cron process like Woo does), so I'm backporting a slightly more graceful markFailed() that would mark the migration as not-executed and would keep trying to run it on whatever hooks they usually run.

(I am not 100% sure what the trigger in Presta is, I think you have to press the "upgrade" button on the overview to actually execute the migration script)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now applies the PDK update with ->markFailed()

Comment thread src/Migration/2026_07_29_113506_refresh_carrier_capabilities.php
Comment thread src/Migration/2026_07_29_113506_refresh_carrier_capabilities.php
@myparcel-bot myparcel-bot Bot added the changes requested (Auto) label Aug 4, 2026
@myparcel-bot
myparcel-bot Bot force-pushed the fix/INT-1699-refresh-carrier-capabilities branch 2 times, most recently from fe23532 to 14b9d4d Compare August 5, 2026 11:55
FreekVR and others added 4 commits August 6, 2026 11:27
The test bootstrapper swaps the client adapter for a mock, which covers the legacy API layer. The SdkApi services used to build their own HTTP client and slipped past it, so any test touching capabilities, shipping rule implications or whoami sent a real request to the live API. It surfaced as a 401 rather than a useful failure.

Those services now take their transport through the constructor, so binding it once points every one of them at the mock queue, including any service added later.

Resolves INT-1699

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Carrier data stored before this release holds insurance limits in the nested wrapper the MyParcel API is removing. The PDK now reads the flat limits, which that older data does not have, so insurance would be unavailable in the shop until something refreshed it.

Adds a migration that fetches the contract definitions again and rewrites the stored carriers in the shape the PDK expects. It runs once per install. When the API cannot be reached the migration fails on purpose so it is retried on the next load, rather than leaving the old data in place.

Resolves INT-1699

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Matches the context Migration5_3_0 already logs for the same failure, so both refresh paths report a failure the same way.

Resolves INT-1699

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…h the api

The carrier refresh now reports failure through the PDK's markFailed() instead of
throwing. It is still retried on the next load, but a briefly unreachable API no
longer takes the page down with it.

Requires the PDK release containing markFailed().

Resolves INT-1699
Part of INT-1695

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@myparcel-bot
myparcel-bot Bot force-pushed the fix/INT-1699-refresh-carrier-capabilities branch from 0f065d6 to 7438681 Compare August 6, 2026 11:28
4.7.0 is the first release carrying markFailed(), which the carrier refresh
migration calls. The flat insurance format is not in it yet, so the nested
wrapper test stays red until myparcelnl/pdk#511 ships.

Part of INT-1695

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.

Comment on lines +22 to +27
/** @var PdkAccountRepositoryInterface $accountRepository */
$accountRepository = Pdk::get(PdkAccountRepositoryInterface::class);
$account = $accountRepository->getAccount(true);
// PHPStan types Account::$shops as a non-null ShopCollection, but the guard is kept
// intentionally to stay safe against partial/corrupted account data during upgrade.
$shop = $account && $account->shops ? $account->shops->first() : null;
Comment on lines +141 to +145
// Same stored result either way: the flat limits, and no nested wrapper left behind.
expect($stored->getInsuredAmount())->toBeNull()
->and($stored->getMin()->getAmount())->toBe(0)
->and($stored->getMax()->getAmount())->toBe(500_000)
->and($stored->getDefault()->getAmount())->toBe(0);
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Development

Successfully merging this pull request may close these issues.

5 participants