New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improve possibility to disable visits log / visitor profile #16598
Conversation
6561ee0
to
f8fb48c
Compare
@tsteur did the suggested adjustments. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@sgiehl could we add some description to the settings?
the real time map needs to be disabled as well when visitor log is disabled? And when visitor profile is disabled the bubbles in real time map need to be disabled if possible (haven't checked this one yet, might be already done)
would we also need to remove the "test" button in the segment editor? or would we still show it and people see the error message that the visitor log is disabled? currently seeing this
would maybe at least need to be bit more specific and mention visitor log was disabled
Be great if we could also add a sentence to intro of the Privacy -> Anonymise data
page as mentioned in the issue I think with a link to the Live
settings (if the Live plugin is enabled).
Noticed an edge case re Roll Up Reporting that need a documentation or maybe we mention it as part of the system setting or not sure... If there is a Roll Up that has the features enabled and includes a children site where this feature is disabled, then the data will be still shown for this children site because of the event Live.API.getIdSitesString
. Or maybe ideally we could adjust roll up reporting to not add sites in the addIdSitesToLiveQueries
where this feature is disabled? Then we wouldn't need to explain anything in the UI and we would maybe have an FAQ explaining "Why the logs of some sites might not appear in the roll up visitor log".
|
BTW @sgiehl haven't checked yet but do we need to have a check that visit log also requires visit profile to be disabled? Would we need to rename the settings to like below to make this more clear?
Like technically, none of the profile features work without the visits log except for maybe the API. In any way it could be misleading maybe to have visitor profile enabled but not the visits log because they wouldn't find a way through the UI to get there |
I think also the link from the privacy -> anonymise data page to this setting is still missing I think? |
There is actually still the visitor profile widget that can be added to the dashboard. And the UserId report still has the possibility to open a profile for the user. Another thing I just noticed is that disabling the visitor log currently breaks the GDPR Tools, as they use Live.getLastVisitsDetails to search for visitors. Not sure how to handle that, as imho I guess those tools should actually work even if visits log / profile are disabled. Could maybe proxy the API call through a controller method, so I'm able to circumvent the check somehow. |
Actually not, as the JavaScript directly calls the |
@sgiehl Instead of a controller method be better to create a new API method in the privacy manager. It would then not even take any date/period as input but only the siteIds and segment filter. It's better to use API so it can be used by other consumers, it supports different formats, automatically has the token check etc. Also we already have |
@tsteur implemented the new api method. Should we hide the link to the visitor profile in the GDPR tools if the visits log or profile is disabled? Might be hard to temporarily allow opening a visitor profile for that case... |
'segment' => $segment, | ||
'idSite' => $idSite, | ||
'period' => 'range', | ||
'date' => '1998-01-01,today', | ||
'filter_limit' => 401, | ||
'doNotFetchActions' => 1 | ||
]); | ||
|
||
$columnsToKeep = [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
btw I suppose showColumns
parameter doesn't work here? Not important though
sounds good to remove the feature from there for now when it is disabled. I wonder if we should remove it to rather use There are still descriptions missing for the settings. Can you add them and then if tests pass merge the PR? Feel free to then also merge all the related PRs |
@tsteur could you check if the descriptions are fine that way? Will update the tests and merge tomorrow. Waiting for the travis build since an hour already 🤷 |
@sgiehl looks good! I'll merge now so we can include the feature in the next beta. Maybe you can create a PR to fix the ui tests tomorrow? Just fixed some system/integration test now but there might be still one left also |
refs #16259