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

Jump to bottom

Add regular expression support to list of user agents to exclude #16766

Merged
merged 2 commits into from Jan 1, 2021
Merged

Add regular expression support to list of user agents to exclude #16766

merged 2 commits into from Jan 1, 2021

Conversation

nina-py
Copy link
Contributor

@nina-py nina-py commented Nov 22, 2020

Description:

  • Added regex support to Administration -> Websites -> Settings ->
    Global list of user agents to exclude
  • Made sure old tests that use stripos() pass
  • Added new tests
  • Added a sentence to the inline help area about regex support
  • Fixed a typo elsewhere as specified in the original issue.

Closes #14186.

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

@tsteur tsteur added the Needs Review PRs that need a code review label Nov 22, 2020
@tsteur tsteur added this to the 4.1.0 milestone Nov 22, 2020
sgiehl
sgiehl requested changes Nov 24, 2020
View reviewed changes
core/Tracker/VisitExcluded.php Outdated Show resolved Hide resolved
sgiehl
sgiehl requested changes Nov 30, 2020
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nina-py Sorry for not coming back earlier. Was a bit busy with various other stuff for the 4.0 release 🙈

I've added another small note. Besides that there are a few tests failing. Some system test files need to be updated.
See https://travis-ci.com/github/matomo-org/matomo/jobs/448352173#L813 and https://travis-ci.com/github/matomo-org/matomo/jobs/448352172#L805

Regarding the UI tests. There are a few failing due to your changes. You can view the changes on our build artifacts server:
https://builds-artifacts.matomo.org/matomo-org/matomo/4.x-dev/43882/

It also displays a command that allows you downloading the changed files. But only add/commit those files that had been changed through your PR. Some tests are sometimes failing randomly.

When commiting UI file you need to have git lfs installed, as those files are stored on git lfs.

If you have problems with updating those files let me know, and I can do that for you.

plugins/WebsiteMeasurable/MeasurableSettings.php Outdated Show resolved Hide resolved
@sgiehl
Copy link
Member

sgiehl commented Dec 15, 2020

@nina-py could you maybe merge in the latest changed from 4.x-dev? If tests are passing, guess this PR would be good to merge as well.

@nina-py
Copy link
Contributor Author

nina-py commented Dec 15, 2020

@sgiehl, there are some System tests that I need to update first, and go over the UI tests to see if any failures are relevant to changes in this PR. I'll get to it over the next couple of days!

@nina-py
Add regular expression support to list of user agents to exclude
d54edae 
- Added regex support to Administration -> Websites -> Settings ->
Global list of user agents to exclude
- Made sure old tests that use stripos() pass
- Added new tests
- Added a sentence to the inline help area about regex support
- Fixed a typo elsewhere as specified in the original issue.

Closes #14186.

Updated method and tests following code review

Update plugins/WebsiteMeasurable/MeasurableSettings.php

Co-authored-by: Stefan Giehl <stefan@matomo.org>

Update system tests
@nina-py
Update screenshots for UI tests affected by the copy changes
734268d
@nina-py
Copy link
Contributor Author

nina-py commented Dec 19, 2020

Hi @sgiehl, I've rebased the branch once again, updated the failing System and UI tests and finally most checks are green! The remaining three mismatched UI screenshots are not related to the changes in this branch. Please review.

sgiehl
sgiehl approved these changes Dec 21, 2020
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good now @nina-py

@tsteur Feel free to merge this one if we shall include it into the upcoming release

@nina-py
Copy link
Contributor Author

nina-py commented Dec 21, 2020

Thank you @sgiehl!

@mattab mattab modified the milestones: 4.1.0, 4.2.0 Dec 21, 2020
@diosmosis diosmosis merged commit df68fbc into matomo-org:4.x-dev Jan 1, 2021
@diosmosis
Copy link
Member

Thanks for another contribution @nina-py !

@nina-py nina-py deleted the 14186-user-agent-exclusion-supports-regular-expressions branch January 1, 2021 09:10
@mattab mattab added the Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. label Jan 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl approved these changes

Assignees
No one assigned
Labels
Enhancement For new feature suggestions that enhance Matomo's capabilities or add a new report, new API etc. Needs Review PRs that need a code review
Projects
None yet
Milestone
4.2.0
Development

Successfully merging this pull request may close these issues.

List of user agents to exclude could support regular expressions
5 participants
@nina-py @sgiehl @diosmosis @tsteur @mattab