fix: an inactive offer can still receive traffic

src/Admin/Repository/OfferRepository.php

@@ -16,6 +16,37 @@ public function findById(string $id): ?Offer
         return $row === null ? null : Offer::fromRow($row);
     }
 
+    /**
+     * Load active offers by ID, keyed by their ID for routing lookups.
+     *
+     * @param list<string> $ids
+     * @return array<string,Offer>
+     */
+    public function findActiveByIds(array $ids): array
+    {
+        $ids = array_values(array_unique(array_filter($ids, static fn (string $id): bool => $id !== '')));
+        if ($ids === []) return [];
+
+        $placeholders = [];
+        $params = [];
+        foreach ($ids as $i => $id) {
+            $key = 'id' . $i;
+            $placeholders[] = ':' . $key;
+            $params[$key] = $id;
+        }
+
+        $rows = $this->db->fetchAll(
+            'SELECT * FROM core.offers WHERE is_active AND id IN (' . implode(',', $placeholders) . ')',
+            $params,
+        );
+        $offers = [];
+        foreach ($rows as $row) {
+            $offer = Offer::fromRow($row);
+            $offers[$offer->id] = $offer;
+        }
+        return $offers;
+    }
+
     public function findByToken(string $token): ?Offer
     {
         $row = $this->db->fetchOne('SELECT * FROM core.offers WHERE postback_token = :t', ['t' => $token]);

src/Engine/ClickHandler.php

@@ -20,6 +20,14 @@
 
 final class ClickHandler
 {
+    /**
+     * Offer ids reach `core.offers.id`, which is a `uuid` column: a malformed id
+     * is not a miss, it aborts the whole lookup. Anything that does not match is
+     * dropped from the candidate list instead, the way OfferPicker already drops
+     * an empty id.
+     */
+    private const OFFER_ID = '/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i';
+
     public function __construct(
         private readonly CampaignRepository $campaigns,
         private readonly OfferRepository $offers,
@@ -84,9 +92,18 @@ public function handle(ServerRequestInterface $request, ResponseInterface $respo
         if ($flow !== null && $flow->targetType === 'offers' && $flow->targetOffers !== []) {
             // Flow override wins; null = inherit the campaign default.
             $sticky = $flow->stickyOffer ?? $campaign->stickyOffer;
-            $offerId = $this->picker->pick($flow->targetOffers, $ctx, $sticky);
+            $targetIds = array_map(
+                self::candidateOfferId(...),
+                $flow->targetOffers,
+            );
+            $activeOffers = $this->offers->findActiveByIds($targetIds);
+            $activeTargets = array_values(array_filter(
+                $flow->targetOffers,
+                static fn (array $target): bool => isset($activeOffers[self::candidateOfferId($target)]),
+            ));
+            $offerId = $this->picker->pick($activeTargets, $ctx, $sticky);
             if ($offerId !== null) {
-                $offer = $this->offers->findById($offerId);
+                $offer = $activeOffers[$offerId] ?? null;
                 if ($offer !== null) {
                     $outUrl = $this->macros->expand($offer->url, $ctx);
                 }
@@ -196,14 +213,22 @@ private function resolveTrashUrl(Campaign $c, Context $ctx): string
         return $this->macros->expand($raw, $ctx);
     }
 
-    /** Resolve an {offer:X} reference by UUID id, falling back to exact name. */
+    /** @param array<string,mixed> $target */
+    private static function candidateOfferId(array $target): string
+    {
+        $offerId = $target['offer_id'] ?? null;
+        return is_string($offerId) && preg_match(self::OFFER_ID, $offerId) ? $offerId : '';
+    }
+
+    /** Resolve an active {offer:X} reference by UUID id, falling back to exact name. */
     private function resolveOffer(string $ref): ?Offer
     {
-        if (preg_match('/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/i', $ref)) {
+        if (preg_match(self::OFFER_ID, $ref)) {
             $byId = $this->offers->findById($ref);
-            if ($byId !== null) return $byId;
+            if ($byId !== null) return $byId->isActive ? $byId : null;
         }
-        return $this->offers->findByName($ref);
+        $byName = $this->offers->findByName($ref);
+        return $byName !== null && $byName->isActive ? $byName : null;
     }
 
     private function trash(?Campaign $c, string $url, ResponseInterface $response): ResponseInterface

tests/Integration/Engine/ClickHandlerTest.php

@@ -34,6 +34,7 @@
     $cRepo = new CampaignRepository($this->db, new CampaignIdGenerator());
     $oRepo = new OfferRepository($this->db);
     $fRepo = new FlowRepository($this->db);
+    $this->offers = $oRepo;
     $compiler = new FilterCompiler();
     $this->handler = new ClickHandler(
         $cRepo, $oRepo,
@@ -53,7 +54,7 @@
 
     $this->camp = $cRepo->create(['name' => 'Click test', 'slug' => 'clkt01', 'is_active' => '1']);
     $this->offer = $oRepo->create(['name' => 'O', 'url' => 'https://example.com/?c={country}&cid={click_id}', 'is_active' => '1']);
-    $fRepo->create($this->camp->id, [
+    $this->flow = $fRepo->create($this->camp->id, [
         'name' => 'all → offer',
         'filters' => [],
         'target_type' => 'offers',
@@ -86,6 +87,99 @@
     expect($resp->getStatusCode())->toBe(404);
 });
 
+test('inactive offer does not receive new traffic', function (): void {
+    $this->db->execute(
+        'UPDATE core.offers SET is_active = false WHERE id = :id',
+        ['id' => $this->offer->id],
+    );
+
+    $resp = $this->handler->handle(
+        (new ServerRequestFactory())->createServerRequest('GET', '/clkt01'),
+        new Response(),
+        'clkt01',
+    );
+
+    expect($resp->getStatusCode())->toBe(204);
+    expect($resp->getHeaderLine('Location'))->toBe('');
+});
+
+test('inactive offer is removed before active offer weights are evaluated', function (): void {
+    $activeOffer = $this->offers->create([
+        'name' => 'Active fallback',
+        'url' => 'https://active.example/?cid={click_id}',
+        'is_active' => '1',
+    ]);
+    $this->db->execute(
+        'UPDATE core.offers SET is_active = false WHERE id = :id',
+        ['id' => $this->offer->id],
+    );
+    $this->db->execute(
+        'UPDATE core.flows SET target_offers = :targets::jsonb WHERE id = :id',
+        [
+            'id' => $this->flow->id,
+            'targets' => json_encode([
+                ['offer_id' => $this->offer->id, 'weight' => 100],
+                ['offer_id' => $activeOffer->id, 'weight' => 1],
+            ], JSON_THROW_ON_ERROR),
+        ],
+    );
+
+    $req = (new ServerRequestFactory())
+        ->createServerRequest('GET', '/clkt01')
+        ->withCookieParams(['vu' => '019dc137-724a-756c-923a-a392001e3d79']);
+    $resp = $this->handler->handle($req, new Response(), 'clkt01');
+
+    expect($resp->getStatusCode())->toBe(302);
+    expect($resp->getHeaderLine('Location'))->toStartWith('https://active.example/');
+});
+
+test('a malformed offer_id does not take the rest of the flow down with it', function (): void {
+    // core.offers.id is a uuid column, so a non-uuid candidate aborts the lookup
+    // for every visitor — not just the share of them the picker would have sent
+    // to that entry. The healthy offer next to it must still be served.
+    $this->db->execute(
+        'UPDATE core.flows SET target_offers = :targets::jsonb WHERE id = :id',
+        [
+            'id' => $this->flow->id,
+            'targets' => json_encode([
+                ['offer_id' => 'not-a-uuid', 'weight' => 100],
+                ['offer_id' => $this->offer->id, 'weight' => 1],
+            ], JSON_THROW_ON_ERROR),
+        ],
+    );
+
+    $resp = $this->handler->handle(
+        (new ServerRequestFactory())->createServerRequest('GET', '/clkt01'),
+        new Response(),
+        'clkt01',
+    );
+
+    expect($resp->getStatusCode())->toBe(302);
+    expect($resp->getHeaderLine('Location'))->toStartWith('https://example.com/');
+});
+
+test('an offer_id that no longer exists is skipped in favour of a live one', function (): void {
+    $this->db->execute(
+        'UPDATE core.flows SET target_offers = :targets::jsonb WHERE id = :id',
+        [
+            'id' => $this->flow->id,
+            'targets' => json_encode([
+                ['offer_id' => '019dc137-724a-756c-923a-a39200000000', 'weight' => 100],
+                ['offer_id' => $this->offer->id, 'weight' => 1],
+            ], JSON_THROW_ON_ERROR),
+        ],
+    );
+
+    $resp = $this->handler->handle(
+        (new ServerRequestFactory())->createServerRequest('GET', '/clkt01'),
+        new Response(),
+        'clkt01',
+    );
+
+    expect($resp->getStatusCode())->toBe(302);
+    expect($resp->getHeaderLine('Location'))->toStartWith('https://example.com/');
+});
+
 test('trash {offer:name} redirects to the offer url, macro-expanded', function (): void {
     $cRepo = new CampaignRepository($this->db, new CampaignIdGenerator());
     $c2 = $cRepo->create(['name' => 'Trash offer', 'slug' => 'trsh01', 'is_active' => '1']);
@@ -107,6 +201,52 @@
     expect($loc)->not->toContain('{offer:');  // reference resolved, not passed through literally
 });
 
+test('trash {offer:name} does not redirect to an inactive offer', function (): void {
+    $this->db->execute(
+        'UPDATE core.offers SET is_active = false WHERE id = :id',
+        ['id' => $this->offer->id],
+    );
+    $cRepo = new CampaignRepository($this->db, new CampaignIdGenerator());
+    $campaign = $cRepo->create(['name' => 'Inactive trash offer', 'slug' => 'trsh04', 'is_active' => '1']);
+    $this->db->execute(
+        'UPDATE core.campaigns SET trash_mode = 1, trash_url = :url WHERE id = :id',
+        ['url' => '{offer:O}', 'id' => $campaign->id],
+    );
+
+    $resp = $this->handler->handle(
+        (new ServerRequestFactory())->createServerRequest('GET', '/trsh04'),
+        new Response(),
+        'trsh04',
+    );
+
+    expect($resp->getStatusCode())->toBe(204);
+    expect($resp->getHeaderLine('Location'))->toBe('');
+});
+
+test('trash {offer:uuid} does not redirect to an inactive offer either', function (): void {
+    // The reference resolves by id before it falls back to the name, so the
+    // active-only check has to hold on both branches, not just the named one.
+    $this->db->execute(
+        'UPDATE core.offers SET is_active = false WHERE id = :id',
+        ['id' => $this->offer->id],
+    );
+    $cRepo = new CampaignRepository($this->db, new CampaignIdGenerator());
+    $campaign = $cRepo->create(['name' => 'Inactive trash offer by id', 'slug' => 'trsh05', 'is_active' => '1']);
+    $this->db->execute(
+        'UPDATE core.campaigns SET trash_mode = 1, trash_url = :url WHERE id = :id',
+        ['url' => '{offer:' . $this->offer->id . '}', 'id' => $campaign->id],
+    );
+
+    $resp = $this->handler->handle(
+        (new ServerRequestFactory())->createServerRequest('GET', '/trsh05'),
+        new Response(),
+        'trsh05',
+    );
+
+    expect($resp->getStatusCode())->toBe(204);
+    expect($resp->getHeaderLine('Location'))->toBe('');
+});
+
 test('trash {offer:missing} falls back to 204 (no leaked macro)', function (): void {
     $cRepo = new CampaignRepository($this->db, new CampaignIdGenerator());
     $c2 = $cRepo->create(['name' => 'Trash miss', 'slug' => 'trsh02', 'is_active' => '1']);