fix(carriers): fix errors in another postnl carrier configuration (#292)

- occurs when no custom postnl contract is present but there are still multiple enabled postnl carriers returned from the api
myparcelnl · Aug 15, 2024 · 9996991 · 9996991
1 parent dd0c199
commit 9996991
Show file tree

Hide file tree

Showing 2 changed files with 78 additions and 7 deletions.
diff --git a/src/Account/Response/GetShopCarrierOptionsResponse.php b/src/Account/Response/GetShopCarrierOptionsResponse.php
@@ -51,6 +51,10 @@ protected function parseResponseBody(): void
     }
 
     /**
+     * If multiple PostNL carriers are enabled it can mean there's a custom PostNL contract enabled in the account and
+     * that one needs to be used. Otherwise, use the first PostNL Carrier. There should never be multiple PostNL
+     * carriers enabled or shown in our app.
+     *
      * @param  \MyParcelNL\Pdk\Carrier\Collection\CarrierCollection $collection
      *
      * @return \MyParcelNL\Pdk\Carrier\Collection\CarrierCollection
@@ -61,14 +65,14 @@ private function createCarrierCollection(CarrierCollection $collection): Carrier
             ->where('id', Carrier::CARRIER_POSTNL_ID)
             ->where('enabled', true);
 
-        // If multiple PostNL carriers are enabled it means there's a custom PostNL contract enabled in the account.
         if ($enabledPostNlCarriers->count() > 1) {
-            $customPostNlCarrier = $enabledPostNlCarriers->firstWhere('type', Carrier::TYPE_CUSTOM);
+            $preservedCarrier = $enabledPostNlCarriers->firstWhere('type', Carrier::TYPE_CUSTOM)
+                ?? $enabledPostNlCarriers->first();
 
-            // Remove all PostNL carriers except the custom one.
-            return $collection->reject(static function (Carrier $carrier) use ($customPostNlCarrier) {
-                return $carrier->id === Carrier::CARRIER_POSTNL_ID && $carrier->contractId !== $customPostNlCarrier->contractId;
-            });
+            // Remove all PostNL carriers except the chosen one, including disabled ones.
+            return $collection
+                ->where('id', '!=', Carrier::CARRIER_POSTNL_ID)
+                ->push($preservedCarrier);
         }
 
         return $collection;

diff --git a/tests/Unit/App/Action/Backend/Account/UpdateAccountActionTest.php b/tests/Unit/App/Action/Backend/Account/UpdateAccountActionTest.php
@@ -188,6 +188,73 @@ function executeUpdateAccount(
         ->pluck('externalIdentifier')
         ->all();
 
-    // If custom PostNL carrier is present, all other PostNL entries should be removed
+    // If multiple PostNL carriers are present, the custom contract should be used.
     expect($externalIdentifiers)->toBe(['postnl:23991']);
 });
+
+it('maps carriers correctly with multiple non-contract postnl entries', function () {
+    executeUpdateAccount(['apiKey' => 'test-api-key'], null, [
+        [
+            'id'         => 1,
+            'label'      => null,
+            'carrier_id' => 1,
+            'carrier'    => [
+                'id'   => 1,
+                'name' => 'postnl',
+            ],
+            'enabled'    => 1,
+            'optional'   => 1,
+            'primary'    => 1,
+            'type'       => 'main',
+        ],
+        [
+            'id'         => 1,
+            'label'      => 'postnl_package_small_nl',
+            'carrier_id' => 1,
+            'carrier'    => [
+                'id'   => 1,
+                'name' => 'postnl',
+            ],
+            'enabled'    => 1,
+            'optional'   => 0,
+            'primary'    => 1,
+            'type'       => 'main',
+        ],
+        [
+            'id'         => 1,
+            'label'      => 'postnl_flespakket_on_myparcel',
+            'carrier_id' => 1,
+            'carrier'    => [
+                'id'   => 1,
+                'name' => 'postnl',
+            ],
+            'enabled'    => 0,
+            'optional'   => 1,
+            'primary'    => 1,
+            'type'       => 'main',
+        ],
+        // Add a dhl for you carrier to make sure it's not removed.
+        [
+            'id'          => 12424,
+            'carrier_id'  => 9,
+            'carrier'     => [
+                'id'   => 9,
+                'name' => 'dhlforyou',
+            ],
+            'enabled'     => 1,
+            'optional'    => 1,
+            'primary'     => 0,
+            'type'        => 'custom',
+            'contract_id' => 677,
+        ],
+    ]);
+
+    $firstShop = AccountSettings::getAccount()->shops->first();
+
+    $externalIdentifiers = $firstShop->carriers
+        ->pluck('externalIdentifier')
+        ->all();
+
+    // If multiple PostNL carriers are present, but no custom contract, only the first one should be kept.
+    expect($externalIdentifiers)->toBe(['dhlforyou:12424', 'postnl']);
+});