From 9b72130fdf9da44ba9266916b091ef3bba7b15d5 Mon Sep 17 00:00:00 2001
From: Arpit Jalan <arpit@arpitjalan.com>
Date: Thu, 27 Jun 2024 22:13:54 +0530
Subject: [PATCH] FIX: show non-restricted ads instead of not showing ads at
 all (#213)

In some cases where there were category restricted house ads we were not
showing ads on reload. This commit filter out all the ads that should
not be shown on current page, leaving only allowed ads. So now we'll
show ads on every reload in all the cases.

Internal ticket: t130920
---
 .../discourse/components/house-ad.js          | 39 ++++++++++++-------
 test/javascripts/acceptance/house-ad-test.js  | 39 +++++++++++++++++--
 2 files changed, 60 insertions(+), 18 deletions(-)

diff --git a/assets/javascripts/discourse/components/house-ad.js b/assets/javascripts/discourse/components/house-ad.js
index 7360aec..399295c 100644
--- a/assets/javascripts/discourse/components/house-ad.js
+++ b/assets/javascripts/discourse/components/house-ad.js
@@ -72,22 +72,21 @@ export default AdComponent.extend({
       placement = this.get("placement").replace(/-/g, "_"),
       adNames = this.adsNamesForSlot(placement);
 
-    if (adNames.length > 0) {
+    // filter out ads that should not be shown on the current page
+    const filteredAds = adNames.filter((adName) => {
+      const ad = houseAds.creatives[adName];
+      return (
+        !ad.category_ids?.length ||
+        ad.category_ids.includes(this.currentCategoryId)
+      );
+    });
+    if (filteredAds.length > 0) {
       if (!adIndex[placement]) {
         adIndex[placement] = 0;
       }
-      let ad = houseAds.creatives[adNames[adIndex[placement]]] || "";
-      adIndex[placement] = (adIndex[placement] + 1) % adNames.length;
-      // check if the ad includes the current category, or if no category restrictions are set for the ad
-      // otherwise don't show it
-      if (
-        !ad.category_ids?.length ||
-        ad.category_ids.includes(this.currentCategoryId)
-      ) {
-        return ad.html;
-      }
-    } else {
-      return "";
+      let ad = houseAds.creatives[filteredAds[adIndex[placement]]] || "";
+      adIndex[placement] = (adIndex[placement] + 1) % filteredAds.length;
+      return ad.html;
     }
   },
 
@@ -120,9 +119,21 @@ export default AdComponent.extend({
 
     if (adIndex.topic_list_top === null) {
       // start at a random spot in the ad inventory
+      const houseAds = this.site.get("house_creatives");
       Object.keys(adIndex).forEach((placement) => {
         const adNames = this.adsNamesForSlot(placement);
-        adIndex[placement] = Math.floor(Math.random() * adNames.length);
+        if (adNames.length === 0) {
+          return;
+        }
+        // filter out ads that should not be shown on the current page
+        const filteredAds = adNames.filter((adName) => {
+          const ad = houseAds.creatives[adName];
+          return (
+            !ad.category_ids?.length ||
+            ad.category_ids.includes(this.currentCategoryId)
+          );
+        });
+        adIndex[placement] = Math.floor(Math.random() * filteredAds.length);
       });
     }
 
diff --git a/test/javascripts/acceptance/house-ad-test.js b/test/javascripts/acceptance/house-ad-test.js
index 2a35eb0..de78a25 100644
--- a/test/javascripts/acceptance/house-ad-test.js
+++ b/test/javascripts/acceptance/house-ad-test.js
@@ -197,7 +197,7 @@ acceptance(
       assert
         .dom(".h-topic-list")
         .doesNotExist(
-          "ad is not displayed because the current category id is included in the ad category_ids"
+          "ad is not displayed because the current category id is not included in the ad category_ids"
         );
     });
   }
@@ -229,7 +229,7 @@ acceptance(
       assert
         .dom(".h-topic-list")
         .doesNotExist(
-          "ad is not displayed because the current category id is included in the ad category_ids"
+          "ad is not displayed because the current category id is not included in the ad category_ids"
         );
     });
   }
@@ -261,7 +261,7 @@ acceptance(
       assert
         .dom(".h-topic-list")
         .doesNotExist(
-          "ad is not displayed because the current category id is included in the ad category_ids"
+          "ad is not displayed because the current category id is not included in the ad category_ids"
         );
     });
   }
@@ -288,7 +288,7 @@ acceptance(
       },
     });
 
-    test("hides ad to anon users when current category id is not included in ad category_ids", async (assert) => {
+    test("shows ad to anon users when current category id is included in ad category_ids", async (assert) => {
       await visit("/c/bug/1");
       assert
         .dom(".h-topic-list")
@@ -298,3 +298,34 @@ acceptance(
     });
   }
 );
+
+acceptance(
+  "House Ads | Category and Group Permissions | Anonymous | Show non-restricted ads",
+  function (needs) {
+    needs.settings({
+      no_ads_for_categories: "",
+    });
+    needs.site({
+      house_creatives: {
+        settings: {
+          topic_list_top: "Topic List Top One|Topic List Top Two",
+        },
+        creatives: {
+          "Topic List Top One": {
+            html: "<div class='h-topic-list-one'>TOPIC LIST TOP ONE</div>",
+            category_ids: [2],
+          },
+          "Topic List Top Two": {
+            html: "<div class='h-topic-list-two'>TOPIC LIST TOP TWO</div>",
+            category_ids: [],
+          },
+        },
+      },
+    });
+
+    test("shows non-restricted ad to anon users", async (assert) => {
+      await visit("/c/bug/1");
+      assert.dom(".h-topic-list-two").exists("non-restricted ad is displayed");
+    });
+  }
+);