Added new setting to exclude server side libraries defined in device detector #19

matomo-org / plugin-TrackingSpamPrevention

GNU General Public License v3.0

12 stars 7 forks source link

Added new setting to exclude server side libraries defined in device detector #19 #21

Closed AltamashShaikh closed 2 years ago

AltamashShaikh commented 3 years ago

Description:

Added new setting to exclude server side libraries defined in device detector Issue: Fixes #19

Review

[ ] Functional review done
[ ] Potential edge cases thought about (behavior of the code with strange input, with strange internal state or possible interactions with other Matomo subsystems)
[ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
[ ] Security review done
[ ] Wording review done
[ ] Code review done
[ ] Tests were added if useful/possible
[ ] Reviewed for breaking changes
[ ] Developer changelog updated if needed
[ ] Documentation added if needed
[ ] Existing documentation updated if needed

tsteur commented 2 years ago

@AltamashShaikh the list is quite long (and there are no spaces). Maybe we could just mention a few most popular examples.

Also could we move this setting up next to the other 3 checkboxes?

For consistency could we name this setting something like Block tracking requests from server side libraries?

Then In the setting description we should explain to users when this can be disabled and when not similar to the other settings. Like we could say When this feature is enabled it will block tracking requests made from server side libraries like .... If you are only tracking using our JavaScript tracker then you can safely enable these features as any request from such server side libraries would not be expected and indicates spam. If you are using a server side SDK like our PHP tracking SDK, Java SDK, Python SDK, Android or iOS SDK, or others to track data then you should not enable this feature. Something like that.

AltamashShaikh commented 2 years ago

@AltamashShaikh the list is quite long (and there are no spaces). Maybe we could just mention a few most popular examples.

Also could we move this setting up next to the other 3 checkboxes?

For consistency could we name this setting something like Block tracking requests from server side libraries?

Then In the setting description we should explain to users when this can be disabled and when not similar to the other settings. Like we could say When this feature is enabled it will block tracking requests made from server side libraries like .... If you are only tracking using our JavaScript tracker then you can safely enable these features as any request from such server side libraries would not be expected and indicates spam. If you are using a server side SDK like our PHP tracking SDK, Java SDK, Python SDK, Android or iOS SDK, or others to track data then you should not enable this feature. Something like that.

Yes updated the text as discussed

AltamashShaikh commented 2 years ago

Nice @AltamashShaikh that works

Left only a few minor comments.

There is one problem though. The screenshot test you added was not added through GIT LFS but it should. I'm now always getting git errors like these:

You can see on https://github.com/matomo-org/plugin-TrackingSpamPrevention/blob/4.x-dev/tests/UI/expected-screenshots/TrackingSpamPreventionSettings_page.png it shows

but for https://github.com/matomo-org/plugin-TrackingSpamPrevention/blob/19-setting-to-block-UAs/tests/UI/expected-screenshots/TrackingSpamPreventionSettings_page.png it doesn't.

Might need to remove the screenshot and then install git lfs: https://github.com/git-lfs/git-lfs/wiki/Installation and add it again

Thanks, Updated via git lfs

tsteur commented 2 years ago

Nice 🎉 Let's maybe have a look later how to release a new version