Adds measuarable settings to disable visits log and visitor profile #16561
Conversation
sgiehl
added
the
Pull Request WIP
sgiehl
force-pushed
the
disablevlogprofile
branch
2 times, most recently
from
517a5d0
to
6e88855
Compare
|
@tsteur Do we also need a possibility to define the default value for new websites, or maybe the possibility to disable the features in config, so it's not possible to enable them in UI? |
|
Haven't looked at the PR so not sure what is implemented. I think a global system setting to disable the feature for all sites be needed. When the feature is disabled globally then it should not be present in measurable settings. If it's enabled, then we would show it in measurable settings. If we use a system setting for the global flag then it'll be automatically configurable through the config file as well and would be then removed in the UI. I think that should do. |
sgiehl
force-pushed
the
disablevlogprofile
branch
2 times, most recently
from
5eac8bd
to
04f0d3a
Compare
sgiehl
added
Needs Review
sgiehl
force-pushed
the
disablevlogprofile
branch
2 times, most recently
from
bfd5dea
to
70cd195
Compare
|
Seems due to the new UI test file, some tests moved between the splitting causing some tests to have other results. Will fix those later. But everything else can already be reviewed... |
sgiehl
force-pushed
the
disablevlogprofile
branch
2 times, most recently
from
29bb2fa
to
7d91f75
Compare
sgiehl
force-pushed
the
disablevlogprofile
branch
from
7d91f75
to
94097ad
Compare
| @@ -90,6 +92,7 @@ public function goalConversionsOverview() | |||
| $view->conversion_rate_returning = $this->formatConversionRate($goalMetrics, 'conversion_rate_returning_visit'); | |||
| $view->conversion_rate_new = $this->formatConversionRate($goalMetrics, 'conversion_rate_new_visit'); | |||
| $view->idGoal = $idGoal; | |||
| $view->visitorLogEnabled = Manager::getInstance()->isPluginActivated('Live') && Live::isVisitorLogEnabled($this->idSite); | |||
There was a problem hiding this comment.
Can we check for Manager::getInstance()->isPluginActivated('Live') in Live::isVisitorLogEnabled or do the checks need to be separate?
There was a problem hiding this comment.
In theory guess we could do that. But might be better if we always check if a plugin is installed & activated before using it.
Not sure if Matomo would even run correctly if the Live plugin would be deleted, but if that would work, the code would break here if we directly use a part of the plugin without checking if it's available...
plugins/Live/Live.php
Outdated
| @@ -19,6 +19,9 @@ | |||
| */ | |||
| class Live extends \Piwik\Plugin | |||
| { | |||
| protected static $visitorProfileEnabled = null; | |||
| protected static $visitorLogEnabled = null; | |||
| protected static $siteIdLoaded = null; | |||
There was a problem hiding this comment.
Note: if adding static variables like this, we'd need to make sure they are unset in Fixture::clearInMemoryCaches() (though these won't affect any existing tests I think, but they might in the future). If using the transient cache, then nothing needs to change.
There was a problem hiding this comment.
using transient cache instead sound fine... will change that
| @@ -189,6 +189,10 @@ public function getVisitorProfile($idSite, $visitorId = false, $segment = false, | |||
| { | |||
| Piwik::checkUserHasViewAccess($idSite); | |||
|
|
|||
| if (!Live::isVisitorProfileEnabled($idSite)) { | |||
| throw new Exception('Visitor profile has been disabled in website settings'); | |||
| } | |||
There was a problem hiding this comment.
Should this be translated? Since the user could see it.
There was a problem hiding this comment.
He should actually only see this when calling the API directly. The UI shouldn't try to load the visitor profile at all when it's disabled
There was a problem hiding this comment.
I can't remember how, but I was able to see the error in the UI a couple times. Once in widgets, once in the visitor log page. This was while switching the setting on/off in one tab and testing in another, however, so it could have been an edge case users wouldn't notice.
| $idSite = Common::getRequestVar('idSite', 0, 'int'); | ||
| } | ||
|
|
||
| $cache = Cache::getTransientCache(); |
There was a problem hiding this comment.
fyi @sgiehl in future we probably wouldn't need to cache these settings. If so, we could add a generic transient cache here to all settings: https://github.com/matomo-org/matomo/blob/4.0.0-b3/core/Settings/Storage/Factory.php#L134 but they shouldn't be majorly slow and partially already transient cached in the backend potentially
fixes #16259