Fix website measurable type was defining settings and properties for all types (#10991)

* Fix website measurable type was defining settings and properties for all types

The settings specified in website measurable type should not apply to any other type by default.

* add possibility to extend website settings

* fix comparison was wrong

* fix comparison was wrong

* trying to fix the tests

* fix test

* tweak code

* add test to make sure validation works with any type

* add possibility to update group via settings

7 files changed, 183 insertions, 95 deletions

diff --git a/core/Measurable/Type/TypeManager.php b/core/Measurable/Type/TypeManager.php
index a514abbbdc..8a886d29d5 100644
--- a/core/Measurable/Type/TypeManager.php
+++ b/core/Measurable/Type/TypeManager.php
@@ -29,6 +29,17 @@ class TypeManager
         return $instances;
     }
 
+    public function isExistingType($typeId)
+    {
+        foreach ($this->getAllTypes() as $type) {
+            if ($type->getId() === $typeId) {
+                return true;
+            }
+        }
+
+        return false;
+    }
+
     /**
      * @param string $typeId
      * @return Type|null
diff --git a/core/Settings/Measurable/MeasurableProperty.php b/core/Settings/Measurable/MeasurableProperty.php
index b49ee04698..ebf03635ca 100644
--- a/core/Settings/Measurable/MeasurableProperty.php
+++ b/core/Settings/Measurable/MeasurableProperty.php
@@ -34,7 +34,7 @@ class MeasurableProperty extends \Piwik\Settings\Setting
         'ecommerce', 'sitesearch', 'sitesearch_keyword_parameters',
         'sitesearch_category_parameters',
         'exclude_unknown_urls', 'excluded_ips', 'excluded_parameters',
-        'excluded_user_agents', 'keep_url_fragment', 'urls'
+        'excluded_user_agents', 'keep_url_fragment', 'urls', 'group'
     );
 
     /**
diff --git a/plugins/MobileAppMeasurable/MeasurableSettings.php b/plugins/MobileAppMeasurable/MeasurableSettings.php
new file mode 100644
index 0000000000..100784d61d
--- /dev/null
+++ b/plugins/MobileAppMeasurable/MeasurableSettings.php
@@ -0,0 +1,17 @@
+<?php
+/**
+ * Piwik - free/libre analytics platform
+ *
+ * @link http://piwik.org
+ * @license http://www.gnu.org/licenses/gpl-3.0.html GPL v3 or later
+ */
+
+namespace Piwik\Plugins\MobileAppMeasurable;
+
+class MeasurableSettings extends \Piwik\Plugins\WebsiteMeasurable\MeasurableSettings
+{
+    protected function shouldShowSettingsForType($type)
+    {
+        return $type === Type::ID;
+    }
+}
diff --git a/plugins/SitesManager/API.php b/plugins/SitesManager/API.php
index 736cf1fc9c..49bda812f0 100644
--- a/plugins/SitesManager/API.php
+++ b/plugins/SitesManager/API.php
@@ -13,8 +13,6 @@ use Piwik\Access;
 use Piwik\Common;
 use Piwik\Container\StaticContainer;
 use Piwik\Date;
-use Piwik\Db;
-use Piwik\Metrics\Formatter;
 use Piwik\Network\IPUtils;
 use Piwik\Option;
 use Piwik\Piwik;
@@ -74,10 +72,16 @@ class API extends \Piwik\Plugin\API
      */
     private $settingsMetadata;
 
-    public function __construct(SettingsProvider $provider, SettingsMetadata $settingsMetadata)
+    /**
+     * @var Type\TypeManager
+     */
+    private $typeManager;
+
+    public function __construct(SettingsProvider $provider, SettingsMetadata $settingsMetadata, Type\TypeManager $typeManager)
     {
         $this->settingsProvider = $provider;
         $this->settingsMetadata = $settingsMetadata;
+        $this->typeManager = $typeManager;
     }
 
     /**
@@ -555,40 +559,22 @@ class API extends \Piwik\Plugin\API
 
         $this->checkName($siteName);
 
-        if (empty($settingValues)) {
+        if (!isset($settingValues)) {
             $settingValues = array();
         }
 
-        if (isset($urls)) {
-            $settingValues = $this->setSettingValue('urls', $urls, $settingValues);
-        }
-        if (isset($ecommerce)) {
-            $settingValues = $this->setSettingValue('ecommerce', $ecommerce, $settingValues);
-        }
-        if (isset($siteSearch)) {
-            $settingValues = $this->setSettingValue('sitesearch', $siteSearch, $settingValues);
-        }
-        if (isset($searchKeywordParameters)) {
-            $settingValues = $this->setSettingValue('sitesearch_keyword_parameters', explode(',', $searchKeywordParameters), $settingValues);
-        }
-        if (isset($searchCategoryParameters)) {
-            $settingValues = $this->setSettingValue('sitesearch_category_parameters', explode(',', $searchCategoryParameters), $settingValues);
-        }
-        if (isset($keepURLFragments)) {
-            $settingValues = $this->setSettingValue('keep_url_fragment', $keepURLFragments, $settingValues);
-        }
-        if (isset($excludeUnknownUrls)) {
-            $settingValues = $this->setSettingValue('exclude_unknown_urls', $excludeUnknownUrls, $settingValues);
-        }
-        if (isset($excludedIps)) {
-            $settingValues = $this->setSettingValue('excluded_ips', explode(',', $excludedIps), $settingValues);
-        }
-        if (isset($excludedQueryParameters)) {
-            $settingValues = $this->setSettingValue('excluded_parameters', explode(',', $excludedQueryParameters), $settingValues);
-        }
-        if (isset($excludedUserAgents)) {
-            $settingValues = $this->setSettingValue('excluded_user_agents', explode(',', $excludedUserAgents), $settingValues);
-        }
+        $coreProperties = array();
+        $coreProperties = $this->setSettingValue('urls', $urls, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('ecommerce', $ecommerce, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('group', $group, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('sitesearch', $siteSearch, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('sitesearch_keyword_parameters', explode(',', $searchKeywordParameters), $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('sitesearch_category_parameters', explode(',', $searchCategoryParameters), $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('keep_url_fragment', $keepURLFragments, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('exclude_unknown_urls', $excludeUnknownUrls, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('excluded_ips', explode(',', $excludedIps), $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('excluded_parameters', explode(',', $excludedQueryParameters), $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('excluded_user_agents', explode(',', $excludedUserAgents), $coreProperties, $settingValues);
 
         $timezone = trim($timezone);
         if (empty($timezone)) {
@@ -620,7 +606,12 @@ class API extends \Piwik\Plugin\API
             $bind['group'] = "";
         }
 
-        $allSettings = $this->setAndValidateMeasurableSettings(0, $bind['type'], $settingValues);
+        $allSettings = $this->setAndValidateMeasurableSettings(0, 'website', $coreProperties);
+
+        // any setting specified in setting values will overwrite other setting
+        if (!empty($settingValues)) {
+            $this->setAndValidateMeasurableSettings(0, $bind['type'], $settingValues);
+        }
 
         foreach ($allSettings as $settings) {
             foreach ($settings->getSettingsWritableByCurrentUser() as $setting) {
@@ -640,7 +631,12 @@ class API extends \Piwik\Plugin\API
 
         $idSite = $this->getModel()->createSite($bind);
 
-        $this->saveMeasurableSettings($idSite, $bind['type'], $settingValues);
+        if (!empty($coreProperties)) {
+            $this->saveMeasurableSettings($idSite, 'website', $coreProperties);
+        }
+        if (!empty($settingValues)) {
+            $this->saveMeasurableSettings($idSite, $bind['type'], $settingValues);
+        }
 
         // we reload the access list which doesn't yet take in consideration this new website
         Access::getInstance()->reloadAccess();
@@ -657,27 +653,34 @@ class API extends \Piwik\Plugin\API
         return (int) $idSite;
     }
 
-    private function setSettingValue($fieldName, $value, $settingValues)
+    private function setSettingValue($fieldName, $value, $coreProperties, $settingValues)
     {
         $pluginName = 'WebsiteMeasurable';
-        if (empty($settingValues[$pluginName])) {
-            $settingValues[$pluginName] = array();
-        }
 
-        $found = false;
-        foreach ($settingValues[$pluginName] as $key => $setting) {
-            if ($setting['name'] === $fieldName) {
-                $setting['value'] = $value;
-                $found = true;
-                break;
+        if (isset($value)) {
+
+            if (empty($coreProperties[$pluginName])) {
+                $coreProperties[$pluginName] = array();
             }
-        }
 
-        if (!$found) {
-            $settingValues[$pluginName][] = array('name' => $fieldName, 'value' => $value);
+            $coreProperties[$pluginName][] = array('name' => $fieldName, 'value' => $value);
+
+        } elseif (!empty($settingValues[$pluginName])) {
+            // we check if the value is defined in the setting values instead
+            foreach ($settingValues[$pluginName] as $key => $setting) {
+                if ($setting['name'] === $fieldName) {
+
+                    if (empty($coreProperties[$pluginName])) {
+                        $coreProperties[$pluginName] = array();
+                    }
+
+                    $coreProperties[$pluginName][] = array('name' => $fieldName, 'value' => $setting['value']);
+                    return $coreProperties;
+                }
+            }
         }
 
-        return $settingValues;
+        return $coreProperties;
     }
 
     public function getSiteSettings($idSite)
@@ -1206,41 +1209,26 @@ class API extends \Piwik\Plugin\API
             $bind['name'] = $siteName;
         }
 
-        if (empty($settingValues)) {
+        if (!isset($settingValues)) {
             $settingValues = array();
         }
 
-        if (isset($urls)) {
-            $settingValues = $this->setSettingValue('urls', $urls, $settingValues);
-        }
-        if (isset($ecommerce)) {
-            $settingValues = $this->setSettingValue('ecommerce', $ecommerce, $settingValues);
-        }
-        if (isset($siteSearch)) {
-            $settingValues = $this->setSettingValue('sitesearch', $siteSearch, $settingValues);
-        }
-        if (isset($searchKeywordParameters)) {
-            $settingValues = $this->setSettingValue('sitesearch_keyword_parameters', explode(',', $searchKeywordParameters), $settingValues);
-        }
-        if (isset($searchCategoryParameters)) {
-            $settingValues = $this->setSettingValue('sitesearch_category_parameters', explode(',', $searchCategoryParameters), $settingValues);
-        }
-        if (isset($keepURLFragments)) {
-            $settingValues = $this->setSettingValue('keep_url_fragment', $keepURLFragments, $settingValues);
-        }
-        if (isset($excludeUnknownUrls)) {
-            $settingValues = $this->setSettingValue('exclude_unknown_urls', $excludeUnknownUrls, $settingValues);
-        }
-        if (isset($excludedIps)) {
-            $settingValues = $this->setSettingValue('excluded_ips', explode(',', $excludedIps), $settingValues);
-        }
-        if (isset($excludedQueryParameters)) {
-            $settingValues = $this->setSettingValue('excluded_parameters', explode(',', $excludedQueryParameters), $settingValues);
-        }
-        if (isset($excludedUserAgents)) {
-            $settingValues = $this->setSettingValue('excluded_user_agents', explode(',', $excludedUserAgents), $settingValues);
+        if (empty($coreProperties)) {
+            $coreProperties = array();
         }
 
+        $coreProperties = $this->setSettingValue('urls', $urls, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('group', $group, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('ecommerce', $ecommerce, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('sitesearch', $siteSearch, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('sitesearch_keyword_parameters', explode(',', $searchKeywordParameters), $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('sitesearch_category_parameters', explode(',', $searchCategoryParameters), $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('keep_url_fragment', $keepURLFragments, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('exclude_unknown_urls', $excludeUnknownUrls, $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('excluded_ips', explode(',', $excludedIps), $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('excluded_parameters', explode(',', $excludedQueryParameters), $coreProperties, $settingValues);
+        $coreProperties = $this->setSettingValue('excluded_user_agents', explode(',', $excludedUserAgents), $coreProperties, $settingValues);
+
         if (isset($currency)) {
             $currency = trim($currency);
             $this->checkValidCurrency($currency);
@@ -1264,6 +1252,10 @@ class API extends \Piwik\Plugin\API
             $bind['type'] = $this->checkAndReturnType($type);
         }
 
+        if (!empty($coreProperties)) {
+            $this->setAndValidateMeasurableSettings($idSite, $idType = 'website', $coreProperties);
+        }
+
         if (!empty($settingValues)) {
             $this->setAndValidateMeasurableSettings($idSite, $idType = null, $settingValues);
         }
@@ -1272,6 +1264,10 @@ class API extends \Piwik\Plugin\API
             $this->getModel()->updateSite($bind, $idSite);
         }
 
+        if (!empty($coreProperties)) {
+            $this->saveMeasurableSettings($idSite, $idType = 'website', $coreProperties);
+        }
+
         if (!empty($settingValues)) {
             $this->saveMeasurableSettings($idSite, $idType = null, $settingValues);
         }
diff --git a/plugins/SitesManager/tests/Integration/ApiTest.php b/plugins/SitesManager/tests/Integration/ApiTest.php
index 6606015395..c7061f2639 100644
--- a/plugins/SitesManager/tests/Integration/ApiTest.php
+++ b/plugins/SitesManager/tests/Integration/ApiTest.php
@@ -9,12 +9,11 @@
 namespace Piwik\Plugins\SitesManager\tests\Integration;
 
 use Piwik\Container\StaticContainer;
-use Piwik\Settings\Measurable\MeasurableSetting;
-use Piwik\Settings\Measurable\MeasurableSettings;
 use Piwik\Piwik;
 use Piwik\Plugin;
 use Piwik\Plugins\MobileAppMeasurable;
 use Piwik\Plugins\MobileAppMeasurable\Type;
+use Piwik\Plugins\WebsiteMeasurable\Type as WebsiteType;
 use Piwik\Plugins\SitesManager\API;
 use Piwik\Plugins\SitesManager\Model;
 use Piwik\Plugins\UsersManager\API as APIUsersManager;
@@ -83,6 +82,19 @@ class ApiTest extends IntegrationTestCase
      */
     public function test_addSite_WithExcludedIps_AndTimezone_AndCurrency_AndExcludedQueryParameters_SucceedsWhenParamsAreValid()
     {
+        $this->addSiteTest($expectedWebsiteType = 'mobile-\'app');
+    }
+
+    /**
+     * @dataProvider getDifferentTypesDataProvider
+     */
+    public function test_addSite_WhenTypeIsKnown($expectedWebsiteType)
+    {
+        $this->addSiteTest($expectedWebsiteType);
+    }
+
+    private function addSiteTest($expectedWebsiteType, $settingValues = null)
+    {
         $ips = '1.2.3.4,1.1.1.*,1.2.*.*,1.*.*.*';
         $timezone = 'Europe/Paris';
         $currency = 'EUR';
@@ -90,12 +102,11 @@ class ApiTest extends IntegrationTestCase
         $expectedExcludedQueryParameters = 'p1,P2,P33333';
         $excludedUserAgents = " p1,P2, \nP3333 ";
         $expectedExcludedUserAgents = "p1,P2,P3333";
-        $expectedWebsiteType = 'mobile-\'app';
         $keepUrlFragment = 1;
         $idsite = API::getInstance()->addSite("name", "http://piwik.net/", $ecommerce = 1,
             $siteSearch = 1, $searchKeywordParameters = 'search,param', $searchCategoryParameters = 'cat,category',
             $ips, $excludedQueryParameters, $timezone, $currency, $group = null, $startDate = null, $excludedUserAgents,
-            $keepUrlFragment, $expectedWebsiteType);
+            $keepUrlFragment, $expectedWebsiteType, $settingValues);
         $siteInfo = API::getInstance()->getSiteFromId($idsite);
         $this->assertEquals($ips, $siteInfo['excluded_ips']);
         $this->assertEquals($timezone, $siteInfo['timezone']);
@@ -111,6 +122,8 @@ class ApiTest extends IntegrationTestCase
         $this->assertEquals($searchCategoryParameters, $siteInfo['sitesearch_category_parameters']);
         $this->assertEquals($expectedExcludedQueryParameters, $siteInfo['excluded_parameters']);
         $this->assertEquals($expectedExcludedUserAgents, $siteInfo['excluded_user_agents']);
+
+        return $siteInfo;
     }
 
     /**
@@ -193,11 +206,11 @@ class ApiTest extends IntegrationTestCase
     /**
      * @expectedException \Exception
      * @expectedExceptionMessage SitesManager_OnlyMatchedUrlsAllowed
+     * @dataProvider getDifferentTypesDataProvider
      */
-    public function test_addSite_ShouldFailAndNotCreatedASite_IfASettingIsInvalid()
+    public function test_addSite_ShouldFailAndNotCreatedASite_IfASettingIsInvalid($type)
     {
         try {
-            $type = MobileAppMeasurable\Type::ID;
             $settings = array('WebsiteMeasurable' => array(array('name' => 'exclude_unknown_urls', 'value' => 'fooBar')));
             $this->addSiteWithType($type, $settings);
         } catch (Exception $e) {
@@ -212,7 +225,7 @@ class ApiTest extends IntegrationTestCase
 
     public function test_addSite_ShouldSavePassedMeasurableSettings_IfSettingsAreValid()
     {
-        $type = MobileAppMeasurable\Type::ID;
+        $type = WebsiteType::ID;
         $settings = array('WebsiteMeasurable' => array(array('name' => 'urls', 'value' => array('http://www.piwik.org'))));
         $idSite = $this->addSiteWithType($type, $settings);
 
@@ -867,7 +880,7 @@ class ApiTest extends IntegrationTestCase
         $idSite = $this->addSiteWithType($type, array());
 
         try {
-            $settings = array('WebsiteMeasurable' => array(array('name' => 'exclude_unknown_urls', 'value' => 'fooBar')));
+            $settings = array('MobileAppMeasurable' => array(array('name' => 'exclude_unknown_urls', 'value' => 'fooBar')));
             $this->updateSiteSettings($idSite, 'newSiteName', $settings);
 
         } catch (Exception $e) {
@@ -881,7 +894,7 @@ class ApiTest extends IntegrationTestCase
 
     public function test_updateSite_ShouldSavePassedMeasurableSettings_IfSettingsAreValid()
     {
-        $type = MobileAppMeasurable\Type::ID;
+        $type = WebsiteType::ID;
         $idSite = $this->addSiteWithType($type, array());
 
         $this->assertSame(1, $idSite);
@@ -915,6 +928,38 @@ class ApiTest extends IntegrationTestCase
     }
 
     /**
+     * @dataProvider getDifferentTypesDataProvider
+     */
+    public function test_updateSite_WithDifferentTypes($type)
+    {
+        $idSite = $this->addSiteWithType('website', array());
+
+        $site = API::getInstance()->getSiteFromId($idSite);
+        $this->assertEquals(0, $site['exclude_unknown_urls']);
+
+        API::getInstance()->updateSite($idSite, $siteName = 'new site name', $urls = null, $ecommerce = true, $siteSearch = false,
+            $searchKeywordParams = null, $searchCategoryParams = null, $excludedIps = null, $excludedQueryParameters = null,
+            $timzeone = null, $currency = 'NZD', $group = null, $startDate = null, $excludedUserAgents = null,
+            $keepUrlFragments = null, $type, $settings = null, $excludeUnknownUrls = true);
+
+        $site = API::getInstance()->getSiteFromId($idSite);
+        $this->assertEquals('new site name', $site['name']);
+        $this->assertEquals(1, $site['exclude_unknown_urls']);
+        $this->assertEquals(1, $site['ecommerce']);
+        $this->assertEquals(0, $site['sitesearch']);
+        $this->assertEquals('NZD', $site['currency']);
+    }
+
+    public function getDifferentTypesDataProvider()
+    {
+        return array(
+            array('website'),
+            array('mobileapp'),
+            array('notexistingtype'),
+        );
+    }
+
+    /**
      * @expectedException Exception
      * @expectedExceptionMessage SitesManager_ExceptionDeleteSite
      */
diff --git a/plugins/SitesManager/tests/System/expected/test_SitesManager__SitesManager.getSiteSettings.xml b/plugins/SitesManager/tests/System/expected/test_SitesManager__SitesManager.getSiteSettings.xml
index 4bdd947b68..3fafd8604e 100644
--- a/plugins/SitesManager/tests/System/expected/test_SitesManager__SitesManager.getSiteSettings.xml
+++ b/plugins/SitesManager/tests/System/expected/test_SitesManager__SitesManager.getSiteSettings.xml
@@ -67,7 +67,6 @@
 				<name>excluded_ips</name>
 				<title>Excluded IPs</title>
 				<value>
-					<row/>
 				</value>
 				<defaultValue>
 				</defaultValue>
@@ -88,7 +87,6 @@
 				<name>excluded_parameters</name>
 				<title>Excluded Parameters</title>
 				<value>
-					<row/>
 				</value>
 				<defaultValue>
 				</defaultValue>
@@ -109,7 +107,6 @@
 				<name>excluded_user_agents</name>
 				<title>Excluded User Agents</title>
 				<value>
-					<row/>
 				</value>
 				<defaultValue>
 				</defaultValue>
diff --git a/plugins/WebsiteMeasurable/MeasurableSettings.php b/plugins/WebsiteMeasurable/MeasurableSettings.php
index 95e4187f73..bcb3b5f228 100644
--- a/plugins/WebsiteMeasurable/MeasurableSettings.php
+++ b/plugins/WebsiteMeasurable/MeasurableSettings.php
@@ -8,6 +8,7 @@
 
 namespace Piwik\Plugins\WebsiteMeasurable;
 use Piwik\IP;
+use Piwik\Measurable\Type\TypeManager;
 use Piwik\Network\IPUtils;
 use Piwik\Piwik;
 use Piwik\Plugin;
@@ -16,7 +17,6 @@ use Piwik\Settings\Setting;
 use Piwik\Settings\FieldConfig;
 use Piwik\Plugins\SitesManager;
 use Exception;
-use Piwik\Url;
 
 /**
  * Defines Settings for ExampleSettingsPlugin.
@@ -74,16 +74,38 @@ class MeasurableSettings extends \Piwik\Settings\Measurable\MeasurableSettings
      */
     private $pluginManager;
 
-    public function __construct(SitesManager\API $api, Plugin\Manager $pluginManager, $idSite, $idMeasurableType)
+    /**
+     * @var TypeManager
+     */
+    private $typeManager;
+
+    public function __construct(SitesManager\API $api, Plugin\Manager $pluginManager, TypeManager $typeManager, $idSite, $idMeasurableType)
     {
         $this->sitesManagerApi = $api;
         $this->pluginManager = $pluginManager;
+        $this->typeManager = $typeManager;
 
         parent::__construct($idSite, $idMeasurableType);
     }
 
+    protected function shouldShowSettingsForType($type)
+    {
+        $isWebsite = $type === Type::ID;
+
+        if ($isWebsite) {
+            return true;
+        }
+
+        // if no such type exists, we default to website properties
+        return !$this->typeManager->isExistingType($type);
+    }
+
     protected function init()
     {
+        if (!$this->shouldShowSettingsForType($this->idMeasurableType)) {
+            return;
+        }
+
         $this->urls = new Urls($this->idSite);
         $this->addSetting($this->urls);