issue 6075 - Hide Acceptable Ads notification when corresponding subscription is removed

desktop-options.js

Index: desktop-options.js
===================================================================
--- a/desktop-options.js
+++ b/desktop-options.js
@@ -1101,6 +1101,12 @@
     return url == acceptableAdsUrl || url == acceptableAdsPrivacyUrl;
   }
 
+  function isConflictingSubscription(subscription)
+  {
+    return subscription.url == acceptableAdsUrl ||

[Thomas Greiner, 2017/11/23 14:46:12]

Suggestion: If you want you can pass an optional `subscription` parameter to
`setPrivacyConflict()` if you want to keep this logic around.

[saroyanm, 2017/11/23 16:41:37]

Not sure if it will help much and it might complicate the logic IMO as we will
have both logic there the one that extract from "known" subscriptions and the
other one which check which subscription was trigger (we might need another
argument to know what triggered the subscription addition/removal to make use of
it).

+      subscription.recommended == "privacy"
+  }
+
   function hasPrivacyConflict()

[Thomas Greiner, 2017/11/23 14:46:12]

I wouldn't mind keeping the privacy conflict check separate to make
`setPrivacyConflict()` easier to read. Because right now there's quite a lot
happening in there.

[saroyanm, 2017/11/23 16:41:37]

Agree, done.

   {
     let acceptableAdsList = subscriptionsMap[acceptableAdsUrl];
@@ -1291,8 +1297,7 @@
         if (isAcceptableAds(url))
           setAcceptableAds();
 
-        if ((url == acceptableAdsUrl || recommended == "privacy") &&
-          hasPrivacyConflict())
+        if (isConflictingSubscription(subscription) && hasPrivacyConflict())
         {
           getPref("ui_warn_tracking", (showTrackingWarning) =>
           {
@@ -1321,6 +1326,11 @@
             collections.more.removeItem(subscription);
           }
         }
+        if (isConflictingSubscription(subscription))
+        {
+          E("acceptable-ads").classList.remove("show-warning");
+        }

[Thomas Greiner, 2017/11/23 12:53:39]

Why not instead introduce a function similar to `setAcceptableAds()` where we
simply update the state of the warning? Thereby we don't have to duplicate the
element's ID, its class name and this logic throughout the code.

[saroyanm, 2017/11/23 14:06:51]

Good point, done.

+
         collections.filterLists.removeItem(subscription);
         break;
     }