@sgiehl opened this Pull Request on October 20th 2020 Member

refs #16259

@sgiehl commented on October 21st 2020 Member

@tsteur did the suggested adjustments.

@sgiehl commented on October 22nd 2020 Member
  • 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 commented on October 22nd 2020 Member

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 commented on October 23rd 2020 Member

Works otherwise.

The settings still need a description though.

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

@tsteur commented on October 23rd 2020 Member

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

@sgiehl commented on October 23rd 2020 Member

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 commented on October 25th 2020 Member

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 commented on October 26th 2020 Member

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 commented on October 26th 2020 Member

@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 commented on October 27th 2020 Member

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

@tsteur commented on October 28th 2020 Member

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 commented on October 28th 2020 Member

@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 commented on October 28th 2020 Member

@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

This Pull Request was closed on October 28th 2020
Powered by GitHub Issue Mirror