Skip to content
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

Merged
merged 23 commits into from Oct 28, 2020

Conversation

sgiehl
Copy link
Member

@sgiehl sgiehl commented Oct 20, 2020

refs #16259

@sgiehl sgiehl added this to the 4.0.0-RC milestone Oct 20, 2020
plugins/Live/API.php Outdated Show resolved Hide resolved
plugins/Live/API.php Outdated Show resolved Hide resolved
plugins/Live/Controller.php Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member Author

sgiehl commented Oct 21, 2020

@tsteur did the suggested adjustments.

@sgiehl sgiehl marked this pull request as ready for review October 21, 2020 15:47
plugins/Live/Live.php Outdated Show resolved Hide resolved
plugins/Live/Live.php Outdated Show resolved Hide resolved
Copy link
Member

@tsteur tsteur left a 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
image 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".

@sgiehl
Copy link
Member Author

sgiehl commented Oct 22, 2020

  • Realtime map is now disabled together with visits log
  • Button to test segments is also hidden then
  • Realtime map won't try to open visitor profile if it's disabled

@tsteur
Copy link
Member

tsteur commented Oct 22, 2020

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?

  • Disable Visits profile
  • Disable Visits log and visitor profile

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

@tsteur
Copy link
Member

tsteur commented Oct 23, 2020

Works otherwise.

The settings still need a description though.

And the userId feature is still showing the visitor profile feature:
image

plugins/Live/Live.php Outdated Show resolved Hide resolved
plugins/Live/Live.php Outdated Show resolved Hide resolved
@tsteur
Copy link
Member

tsteur commented Oct 23, 2020

I think also the link from the privacy -> anonymise data page to this setting is still missing I think?

@sgiehl
Copy link
Member Author

sgiehl commented Oct 23, 2020

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

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.
On the other side would that open up the possibility to get parts of the visits log data for users with admin access even if its disabled... Any suggestion on that?

@tsteur
Copy link
Member

tsteur commented Oct 25, 2020

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.
On the other side would that open up the possibility to get parts of the visits log data for users with admin access even if its disabled... Any suggestion on that?

Just like we whitelisted the API suggested segment values could we also white list this method?

Below some more needed changes in other plugins: They should also apply if the Live plugin is disabled ideally.

For Media Analytics: Could you add a PR to disable the real time map widget when visitor log is disabled and also the "audience log widget"?

In Heatmap & Session Recording there is an icon with a link to the profile popup that will need to be removed:
image

In form analytics will need to remove these links:
image

in funnels: There is also a link to segmented visitor log:
image

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

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.

I'm thinking the visitor profile should only be possible to be deactivated when also deactivating the visitor log. While the feature could be used individually, you could argue that the visitor profile uses features of the visitor log. Also while there are few edge case places where it could be used individually, most normal ways be blocked and it would cause in the end more confusion. Meaning visitor profile can be only disabled together with the visitor log.

@sgiehl
Copy link
Member Author

sgiehl commented Oct 26, 2020

Just like we whitelisted the API suggested segment values could we also white list this method?

Actually not, as the JavaScript directly calls the Live.getLastVisitsDetails method. So the only possibility I see is to add a new proxy method in the controller, that returns the relevant content of the API result that is needed for the GDPR tools.

@tsteur
Copy link
Member

tsteur commented Oct 26, 2020

@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 deleteDataSubjects and exportDataSubjects API so be good to also add a findDataSubjects API. It could also make sure to only return the needed information so less data will be transferred plus it can't be used to still get visitor log details etc.

@sgiehl
Copy link
Member Author

sgiehl commented Oct 27, 2020

@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 = [
Copy link
Member

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

@tsteur
Copy link
Member

tsteur commented Oct 28, 2020

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...

sounds good to remove the feature from there for now when it is disabled.

image

I wonder if we should remove it to rather use Disable Visitor Log & Visitor profile and then "check" it to disable it? Cause see screenshot above it's not clear when disabling both that visitor profile is also disabled. It becomes invisible. Would then need to name the other setting "Disable visitor profile".

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

@sgiehl
Copy link
Member Author

sgiehl commented Oct 28, 2020

@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 🤷

@tsteur
Copy link
Member

tsteur commented Oct 28, 2020

@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

@tsteur tsteur merged commit 0e910f0 into 4.x-dev Oct 28, 2020
@tsteur tsteur deleted the disableprofilemethod branch October 28, 2020 20:46
@mattab mattab added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants