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

desktop-options.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 1083 matching lines @@
       acceptableAdsPrivacy.setAttribute("aria-disabled", true);
       acceptableAdsPrivacy.setAttribute("tabindex", -1);
     }
   }
 
   function isAcceptableAds(url)
   {
     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];
     let privacyList = null;
     for (let url in subscriptionsMap)
     {
       let subscription = subscriptionsMap[url];
       if (subscription.recommended == "privacy")
       {
         privacyList = subscription;
         break;
       }
     }
     return acceptableAdsList && acceptableAdsList.disabled == false &&
       privacyList && privacyList.disabled == false;
+  }
+
+  function setPrivacyConflict()
+  {
+    let acceptableAdsForm = E("acceptable-ads");
+    if (hasPrivacyConflict())
+    {
+      getPref("ui_warn_tracking", (showTrackingWarning) =>
+      {
+        acceptableAdsForm.classList.toggle("show-warning", showTrackingWarning);
+      });
+    }
+    else
+    {
+      acceptableAdsForm.classList.remove("show-warning");
+    }
   }
 
   function populateLists()
   {
     subscriptionsMap = Object.create(null);
     filtersMap = Object.create(null);
 
     // Empty collections and lists
     for (let property in collections)
       collections[property].clearAll();
@@ skipping 131 matching lines @@
           knownSubscription.originalTitle = subscription.title;
         else
           knownSubscription[property] = subscription[property];
       }
       subscription = knownSubscription;
     }
     switch (action)
     {
       case "disabled":
         updateSubscription(subscription);
+        setPrivacyConflict();
         break;
       case "downloading":
       case "downloadStatus":
       case "homepage":
       case "lastDownload":
       case "title":
         updateSubscription(subscription);
         break;
       case "added":
         let {url, recommended} = subscription;
         // Handle custom subscription
         if (/^~user/.test(url))
         {
           loadCustomFilters(subscription.filters);
           return;
         }
         else if (url in subscriptionsMap)
           updateSubscription(subscription);
         else
           addSubscription(subscription);
 
         if (isAcceptableAds(url))
           setAcceptableAds();
 
-        if (isConflictingSubscription(subscription) && hasPrivacyConflict())
-        {
-          getPref("ui_warn_tracking", (showTrackingWarning) =>
-          {
-            if (showTrackingWarning)
-              E("acceptable-ads").classList.add("show-warning");
-          });
-        }
-
         collections.filterLists.addItem(subscription);
+        setPrivacyConflict();
         break;
       case "removed":

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

It's a shame we missed that in our last review so it's great that we're tackling
this now.

However, note that the conflict can also be resolved by disabling/enabling one
of the filter lists so we also need to react to those messages.

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

Right.
Good point, done.

         if (subscription.recommended)
         {
           subscription.disabled = true;
           onSubscriptionMessage("disabled", subscription);
         }
         else
         {
           delete subscriptionsMap[subscription.url];
           if (isAcceptableAds(subscription.url))
           {
             setAcceptableAds();
           }
           else
           {
             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);
+        setPrivacyConflict();
         break;
     }
   }
 
   function hidePref(key, value)
   {
     let element = document.querySelector("[data-pref='" + key + "']");
     if (element)
       element.setAttribute("aria-hidden", value);
   }
@@ skipping 34 matching lines @@
     switch (key)
     {
       case "notifications_ignoredcategories":
         value = value.indexOf("*") == -1;
         break;
 
       case "notifications_showui":
         hidePref("notifications_ignoredcategories", !value);
         break;
       case "ui_warn_tracking":
-        let showWarning = (value && hasPrivacyConflict());
-        E("acceptable-ads").classList.toggle("show-warning", showWarning);
+        setPrivacyConflict();
         break;
     }
 
     let checkbox = document.querySelector(
       "[data-pref='" + key + "'] button[role='checkbox']"
     );
     if (checkbox)
       checkbox.setAttribute("aria-checked", value);
   }
 
@@ skipping 67 matching lines @@
   });
   browser.runtime.sendMessage({
     type: "subscriptions.listen",
     filter: ["added", "disabled", "homepage", "lastDownload", "removed",
              "title", "downloadStatus", "downloading"]
   });
 
   window.addEventListener("DOMContentLoaded", onDOMLoaded, false);
   window.addEventListener("hashchange", onHashChange, false);
 }