#8858 add custom providers to the providers list on successful setup

8 files changed, 81 insertions, 14 deletions

diff --git a/app/assets/urls/calyx.net.url b/app/assets/urls/calyx.net.url
index 8de04fe9..807e9e18 100644
--- a/app/assets/urls/calyx.net.url
+++ b/app/assets/urls/calyx.net.url
@@ -1,3 +1,3 @@
 {
-	"main_url" : "https://calyx.net/"
+	"main_url" : "https://calyx.net"
 }
diff --git a/app/assets/urls/demo.bitmask.net.url b/app/assets/urls/demo.bitmask.net.url
index 1a412055..0c4de648 100644
--- a/app/assets/urls/demo.bitmask.net.url
+++ b/app/assets/urls/demo.bitmask.net.url
@@ -1,3 +1,3 @@
 {

-	"main_url" : "https://demo.bitmask.net/"

+	"main_url" : "https://demo.bitmask.net"

 }

diff --git a/app/assets/urls/riseup.net.url b/app/assets/urls/riseup.net.url
index 4548b433..42cdb979 100644
--- a/app/assets/urls/riseup.net.url
+++ b/app/assets/urls/riseup.net.url
@@ -1,3 +1,3 @@
 {
-	"main_url" : "https://riseup.net/"
+	"main_url" : "https://riseup.net"
 }
diff --git a/app/src/main/java/se/leap/bitmaskclient/Provider.java b/app/src/main/java/se/leap/bitmaskclient/Provider.java
index 7104143c..f4e2c477 100644
--- a/app/src/main/java/se/leap/bitmaskclient/Provider.java
+++ b/app/src/main/java/se/leap/bitmaskclient/Provider.java
@@ -297,8 +297,6 @@ public final class Provider implements Parcelable {
         try {
             json.put(Provider.MAIN_URL, mainUrl);
             //TODO: add other fields here?
-            //this is used to save custom providers as json. I guess this doesn't work correctly
-            //TODO 2: verify that
         } catch (JSONException e) {
             e.printStackTrace();
         }
@@ -446,5 +444,4 @@ public final class Provider implements Parcelable {
         allowRegistered = false;
         allowAnonymous = false;
     }
-
 }
diff --git a/app/src/main/java/se/leap/bitmaskclient/ProviderListBaseActivity.java b/app/src/main/java/se/leap/bitmaskclient/ProviderListBaseActivity.java
index e961b0a2..3bf51a8c 100644
--- a/app/src/main/java/se/leap/bitmaskclient/ProviderListBaseActivity.java
+++ b/app/src/main/java/se/leap/bitmaskclient/ProviderListBaseActivity.java
@@ -207,7 +207,8 @@ public abstract class ProviderListBaseActivity extends ConfigWizardBaseActivity
 
     void handleProviderSetUp(Provider handledProvider) {
         this.provider = handledProvider;
-
+        adapter.add(provider);
+        adapter.saveProviders();
         if (provider.allowsAnonymous()) {
             mConfigState.putExtra(SERVICES_RETRIEVED, true);
             downloadVpnCertificate();
diff --git a/app/src/main/java/se/leap/bitmaskclient/ProviderManager.java b/app/src/main/java/se/leap/bitmaskclient/ProviderManager.java
index ed41be67..b625c336 100644
--- a/app/src/main/java/se/leap/bitmaskclient/ProviderManager.java
+++ b/app/src/main/java/se/leap/bitmaskclient/ProviderManager.java
@@ -31,6 +31,8 @@ public class ProviderManager implements AdapteeCollection<Provider> {
     private File externalFilesDir;
     private Set<Provider> defaultProviders;
     private Set<Provider> customProviders;
+    private Set<URL> defaultProviderURLs;
+    private Set<URL> customProviderURLs;
 
     private static ProviderManager instance;
 
@@ -52,11 +54,20 @@ public class ProviderManager implements AdapteeCollection<Provider> {
     private void addDefaultProviders(AssetManager assets_manager) {
         try {
             defaultProviders = providersFromAssets(URLS, assets_manager.list(URLS));
+            defaultProviderURLs = getProviderUrlSetFromProviderSet(defaultProviders);
         } catch (IOException e) {
             e.printStackTrace();
         }
     }
 
+    private Set<URL> getProviderUrlSetFromProviderSet(Set<Provider> providers) {
+        HashSet<URL> providerUrls = new HashSet<>();
+        for (Provider provider : providers) {
+            providerUrls.add(provider.getMainUrl().getUrl());
+        }
+        return providerUrls;
+    }
+
     private Set<Provider> providersFromAssets(String directory, String[] relativeFilePaths) {
         Set<Provider> providers = new HashSet<>();
 
@@ -89,6 +100,7 @@ public class ProviderManager implements AdapteeCollection<Provider> {
         customProviders = externalFilesDir != null && externalFilesDir.isDirectory() ?
                 providersFromFiles(externalFilesDir.list()) :
                 new HashSet<Provider>();
+        customProviderURLs = getProviderUrlSetFromProviderSet(customProviders);
     }
 
     private Set<Provider> providersFromFiles(String[] files) {
@@ -132,6 +144,8 @@ public class ProviderManager implements AdapteeCollection<Provider> {
         allProviders.addAll(defaultProviders);
         if(customProviders != null)
             allProviders.addAll(customProviders);
+        //add an option to add a custom provider
+        //TODO: refactor me?
         allProviders.add(new Provider());
         return allProviders;
     }
@@ -153,30 +167,56 @@ public class ProviderManager implements AdapteeCollection<Provider> {
 
     @Override
     public boolean add(Provider element) {
-        return !defaultProviders.contains(element) || customProviders.add(element);
+        return element != null &&
+                !defaultProviderURLs.contains(element.getMainUrl().getUrl()) &&
+                customProviders.add(element) &&
+                customProviderURLs.add(element.getMainUrl().getUrl());
     }
 
     @Override
     public boolean remove(Object element) {
-        return customProviders.remove(element);
+        return element instanceof Provider &&
+                customProviders.remove(element) &&
+                customProviderURLs.remove(((Provider) element).getMainUrl().getUrl());
     }
 
     @Override
     public boolean addAll(Collection<? extends Provider> elements) {
-        return customProviders.addAll(elements);
+        Iterator iterator = elements.iterator();
+        boolean addedAll = true;
+        while (iterator.hasNext()) {
+            Provider p = (Provider) iterator.next();
+            addedAll = customProviders.add(p) &&
+                    customProviderURLs.add(p.getMainUrl().getUrl()) &&
+                    addedAll;
+        }
+        return addedAll;
     }
 
     @Override
     public boolean removeAll(Collection<?> elements) {
-        if(!elements.getClass().equals(Provider.class))
+        Iterator iterator = elements.iterator();
+        boolean removedAll = true;
+        try {
+            while (iterator.hasNext()) {
+                Provider p = (Provider) iterator.next();
+                removedAll = ((defaultProviders.remove(p) && defaultProviderURLs.remove(p.getMainUrl().getUrl())) ||
+                        (customProviders.remove(p) && customProviderURLs.remove(p.getMainUrl().getUrl()))) &&
+                        removedAll;
+            }
+        } catch (ClassCastException e) {
             return false;
-        return defaultProviders.removeAll(elements) || customProviders.removeAll(elements);
+        }
+
+        return removedAll;
     }
 
     @Override
     public void clear() {
         defaultProviders.clear();
         customProviders.clear();
+        customProviderURLs.clear();
+        defaultProviderURLs.clear();
     }
 
     void saveCustomProvidersToFile() {
diff --git a/app/src/production/java/se/leap/bitmaskclient/ProviderListActivity.java b/app/src/production/java/se/leap/bitmaskclient/ProviderListActivity.java
index b6e67331..cf1d1aa6 100644
--- a/app/src/production/java/se/leap/bitmaskclient/ProviderListActivity.java
+++ b/app/src/production/java/se/leap/bitmaskclient/ProviderListActivity.java
@@ -39,11 +39,10 @@ public class ProviderListActivity extends ProviderListBaseActivity {
         setUpProvider();
     }
 
+    @Override
     public void showAndSelectProvider(String provider_main_url) {
         try {
             provider = new Provider(new URL((provider_main_url)));
-            adapter.add(provider);
-            adapter.saveProviders();
             autoSelectProvider(provider);
         } catch (MalformedURLException e) {
             e.printStackTrace();
diff --git a/app/src/test/java/se/leap/bitmaskclient/ProviderTest.java b/app/src/test/java/se/leap/bitmaskclient/ProviderTest.java
index 794c3087..495d5b3f 100644
--- a/app/src/test/java/se/leap/bitmaskclient/ProviderTest.java
+++ b/app/src/test/java/se/leap/bitmaskclient/ProviderTest.java
@@ -1,9 +1,15 @@
 package se.leap.bitmaskclient;
 
+import org.json.JSONException;
 import org.junit.Test;
 
+import java.io.IOException;
+import java.util.HashSet;
+import java.util.Set;
+
 import se.leap.bitmaskclient.testutils.TestSetupHelper;
 
+import static junit.framework.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 /**
@@ -18,4 +24,28 @@ public class ProviderTest {
         assertTrue("Providers should be same:", p1.equals(p2));
     }
 
+    @Test
+    public void testEquals_sameFields_returnsFalse() throws Exception {
+        Provider p1 = TestSetupHelper.getConfiguredProvider();
+        Provider p2 = TestSetupHelper.getConfiguredProvider();
+        p2.setMainUrl("http://somethingsdiffer.org");
+        assertFalse("Providers should be same:", p1.equals(p2));
+    }
+
+    // see ProviderManagerTest testing add(...)
+    @Test
+    public void testEqualsThroughSetContains_differentFields_returnsFalse() throws Exception {
+        Provider p1 = TestSetupHelper.getConfiguredProvider();
+        Provider p2 = TestSetupHelper.getConfiguredProvider();
+        p2.setMainUrl("http://somethingsdiffer.org");
+        Provider p3 = new Provider("https://anotherprovider.net");
+
+        Set<Provider> defaultProviders = new HashSet<>();
+        defaultProviders.add(p1);
+        defaultProviders.add(p2);
+
+        assertTrue(defaultProviders.contains(p1));
+        assertTrue(defaultProviders.contains(p2));
+        assertFalse(defaultProviders.contains(p3));
+    }
 }