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

Improve the configuration of excluded User Agents #8586

Closed
wants to merge 13 commits into from
Closed

Improve the configuration of excluded User Agents #8586

wants to merge 13 commits into from

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Aug 17, 2015

Replaces #8482 that was opened against master.

Fixes #7978, replaces #8050

Problems:

  • Excluded User Agents is a comma-separated list, but user agents can contain commas
  • we want to let users wrap user agents in quotes (see Excluded User Agent still tracked #7978 (comment)) which is currently not supported
  • the inline help for Excluded User Agents doesn't say how to format user agent strings (Excluded User Agent still tracked #7978)
  • other fields in the page ask for one item per line:
    • it's not consistent with Excluded User Agents (which is comma-separated), so it's confusing
    • in the code it's actually comma-separated, so it's clearly misleading and broken for some users

These problems apply to global and site-specific settings.

Goals:

  • consistent formatting for all fields (and both global and site-specific settings)
    • excluded user agents
    • excluded query parameters
    • excluded IPs
  • it needs to be "one per line" because of user agents (that can contain commas)
  • enclosing quotes need to be accepted (and trimmed) for user agents
  • field helps need to match the actual behavior
  • migration script
  • tests
  • update changelog regarding API changes

Notes:

  • enclosing quotes for user agents are trimmed (for the other fields I didn't think it would make sense)
  • the help text for user agents has also been updated (only the english version), it now explicitly says that user agents should be separated by new lines. What is the best course of action to ensure translations are updated? New translation key?
  • The following API methods are now returning an array of strings instead of a list encoded in CSV:
    • SitesManager.getExcludedIpsGlobal
    • SitesManager.getExcludedQueryParametersGlobal
    • SitesManager.getExcludedUserAgentsGlobal
  • The following API methods now accept values separated by new lines instead of commas:
    • SitesManager.setGlobalExcludedIps
    • SitesManager.setGlobalExcludedQueryParameters
    • SitesManager.setGlobalExcludedUserAgents

saleemkce and others added 13 commits June 6, 2015 22:22
@saleemkce
#7978 - fix for user agent quotes issue
b8cafee
@mnapoli
Merge branch 'bugfix-user-agent-quotes-issue' of https://github.com/s…
d9db5ed 
…aleemkce/piwik into bug/7978
@mnapoli
#7978 Improve the configuration of excluded User Agents
980160d 
Before this commit, users can configure excluded user agents by separating them with commas. Enclosing quotes are not trimmed.

With this commit, users can configure excluded user agents one per line. Enclosing quotes are trimmed. This is meant to be simpler and more consistent. It also allows to exclude user agents containing both commas and quotes. This change applies both to global settings as well as site-specific settings.

This commit also changes the help text, which now explicitly says that user agents should be separated by new lines.

A migration script is included: it will replace commas by new lines.

Related: #7978, #8050
@mnapoli
#8448 Users can now configure excluded query parameters using line re…
0ac9804 
…turns

Before this commit, users can configure excluded query parameters by separating them with commas.

With this commit, users can configure them one per line. This is meant to be simpler and more consistent (see #8448). This is now also consistent with form inline helps. This change applies both to global settings as well as site-specific settings.

A migration script is included: it will replace commas by new lines.

Related: #7978, #8050
@mnapoli
Make the update script for #8448 use raw SQL queries
51f9b39 
See #8448 (comment) and #8448 (comment)
@mnapoli
Changelog update for #8448
1a2bec6 
* The following API methods are now returning an array of strings instead of a list encoded in CSV:
    * `SitesManager.getExcludedQueryParametersGlobal`
    * `SitesManager.getExcludedUserAgentsGlobal`
@mnapoli
Fix SitesManager API tests
3ac7b1b
@mnapoli
#8448 Users can now configure excluded IPs using line returns
9f8d0e7 
Before this commit, users can configure excluded IPs by separating them with commas.

With this commit, users can configure them one per line. This is meant to be simpler and more consistent (see #8448). This is now also consistent with form inline helps. This change applies both to global settings as well as site-specific settings.

A migration script is included: it will replace commas by new lines.

Related: #7978, #8050
@mnapoli
Update the changelog and documentation for #8448
4a01807
@mnapoli
Merge branch 'master' into bug/7978
35de86c 
# Conflicts:
#	core/Version.php
@mnapoli
Fix test and update script
3875412
@mnapoli
Fix tests
55519c3
@mnapoli
Merge branch 'master' into bug-7978
9452f77 
# Conflicts:
#	core/Version.php
@mnapoli mnapoli added Bug For errors / faults / flaws / inconsistencies etc. c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Needs Review PRs that need a code review labels Aug 17, 2015
@mnapoli mnapoli added this to the 3.0.0 milestone Aug 17, 2015
@mnapoli
Copy link
Contributor Author

mnapoli commented Aug 17, 2015

This pull request contains commits from master that were not merged in 3.0. Master needs to be merged in 3.0 so that this branch can be merged (rebasing this branch with the intertwined history would be way too long, too complicated, and wouldn't fix the root issue).

This was referenced Aug 17, 2015
Closed
Open
@sgiehl sgiehl force-pushed the 3.0 branch 10 times, most recently from fd504e4 to 6725a57 Compare October 4, 2015 19:54
@tsteur
Copy link
Member

tsteur commented Aug 30, 2016

@mattab I have to close this one. We need to have this fix in Piwik 3 but there all this logic completely changed as they are now all part of the "Plugin Settings / Fields API". There we can eventually make a change to the API and it will be applied everywhere a textarea with multiple entries is expected.

@tsteur tsteur closed this Aug 30, 2016
@tsteur tsteur deleted the bug-7978 branch August 30, 2016 22:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug For errors / faults / flaws / inconsistencies etc. c: Usability For issues that let users achieve a defined goal more effectively or efficiently. Needs Review PRs that need a code review
Projects
None yet
Milestone
Backlog (Help wanted)
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@mnapoli @tsteur @mattab @saleemkce