matomo-org / matomo

Empowering People Ethically with the leading open source alternative to Google Analytics that gives you full control over your data. Matomo lets you easily collect data from websites & apps and visualise this data and extract insights. Privacy is built-in. Liberating Web Analytics. Star us on Github? +1. And we love Pull Requests!
https://matomo.org/
GNU General Public License v3.0
19.8k stars 2.64k forks source link

Unable to remove all URLs for Measurables #20057

Open Starker3 opened 1 year ago

Starker3 commented 1 year ago

This is present in 4.12.3 and the latest 4.13.0-rc2 release.

Steps to reproduce:

  1. Go to Administration > Measurable/Websites > Manage
  2. Click edit on a measurable that has a URL set
  3. Remove all lines from the setting (I.e. the field should be empty)
  4. Click Save
  5. Reload the page

The green prompt at the top of the page says the settings are saved, but when refreshing the page we can see that the URLs that were previously set are still present.

peterhashair commented 1 year ago

@Starker3 thanks for reporting this, I can reproduce this locally. Our product team will prioritize this.

peterhashair commented 1 year ago

maybe we should add a new test for that one.

    public function testUpdateSiteRemoveAllTheUrls()
    {
        $idSite = API::getInstance()->addSite("site1", ['http://main.url', 'http://main2.url']);

        $settings['WebsiteMeasurable'][0]['name'] = 'urls';
        $settings['WebsiteMeasurable'][0]['value'] = null;
        API::getInstance()->updateSite(
            $idSite,
            'site1',
            $urls = null,
            $ecommerce = null,
            $siteSearch = null,
            $searchKeywordParams = null,
            $searchCategoryParams = null,
            $excludedIps = null,
            $excludedQueryParameters = null,
            $timeZone = null,
            $currency = null,
            $group = null,
            $startDate = null,
            $excludedUserAgents = null,
            $keepUrlFragments = null,
            $type = null,
            $settings,
            $excludeUnknownUrls = true);

        $urls = API::getInstance()->getSiteUrlsFromId($idSite);
        $this->assertEmpty($urls);
    }
sgiehl commented 1 year ago

It should actually be required to set at least one url for a website measurable. So we first need to clarify what the expected behaviour would be. Either it should be possible to remove all urls, or if not, an error message should shown.

heurteph-ei commented 1 year ago

By default, when a new measurable is created, this field is not mandatory.

Stan-vw commented 1 year ago

I understand that the ability to create a website without a URL was introduced deliberately as a feature, so we're removing the regression label. However, it's not in line with the current design decision where you can't remove a URL after you've saved one before.

Since we allow people to track items that don't have a URL (such as an app), I suggest we adjust/fix the ability to save the deletion of the URL. I.e. if you previously had a URL saved, you can now delete and save it and it will successfully save without a URL.