@flamisz opened this Pull Request on February 24th 2021 Contributor

Description:

fixes #16776

Review

  • [ ] Functional review done
  • [ ] Usability review done (is anything maybe unclear or think about anything that would cause people to reach out to support)
  • [ ] Security review done see checklist
  • [ ] 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
@sgiehl commented on February 24th 2021 Member

Looks good so far. We could maybe add a UI test to make sure this will keep working. @flamisz it's possible to manipulate the useragent used in UI tests (see https://github.com/matomo-org/matomo/blob/4.x-dev/tests/UI/specs/OptOutForm_spec.js)

@Findus23 commented on February 24th 2021 Member

Do I understand it correctly that this runs the users user-agent through device-detector on any single request?

I can imagine that there are quite a few slow servers out there (or ones without regex-caching) where this could add more than just a few milliseconds to every request.

Maybe just adding it to the login should be enough (unless that's what the code already does).

@tsteur commented on February 24th 2021 Member

BTW only running it on login could work for regular requests but for widgets it would always need to run (eg whenPiwik::getModule()===Widgetize), it would also need to always run for anonymous users, and for API module/plugin we wouldn't want it to run (for performance etc since it wouldn't be "rendered" in the browser)

@flamisz commented on February 24th 2021 Contributor

@tsteur I modified the logic: only checks the browser if the user is anonymous and the module is not API. This way even the widgets won't be checked if you are in the same browser, and of course before you log in, it will be checked.

@flamisz commented on March 1st 2021 Contributor

Hey @tsteur I modified the PR according to your comments. Please view it again when you have some time.

Thanks

@tsteur commented on March 2nd 2021 Member

@flamisz overall looks good from my perspective 👍 but didn't test it

@flamisz commented on March 8th 2021 Contributor

@tsteur I did my best to test on a couple of browsers and a couple of use cases with BrowserStack.
And of course, we have some test cases as well.

@tsteur commented on March 8th 2021 Member

👍 feel free to merge if the tests pass (didn't check)

This Pull Request was closed on March 28th 2021
Powered by GitHub Issue Mirror