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

Improve the configuration of excluded User Agents #8482

Closed
wants to merge 13 commits into from
Closed

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Aug 3, 2015

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 12 commits June 6, 2015 22:22
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
…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
* The following API methods are now returning an array of strings instead of a list encoded in CSV:
    * `SitesManager.getExcludedQueryParametersGlobal`
    * `SitesManager.getExcludedUserAgentsGlobal`
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
# 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 3, 2015
@mnapoli mnapoli added this to the 2.15.0 milestone Aug 3, 2015
@mnapoli
Copy link
Contributor Author

mnapoli commented Aug 3, 2015

Tests are OK, the only fails are for UI screenshots where the translation has been changed: http://builds-artifacts.piwik.org/piwik/piwik/bug-7978/14619/

# Conflicts:
#	core/Version.php
## Piwik 2.15.0

### Breaking Changes
* The following API methods are now returning an array of strings instead of a list encoded in CSV:
Copy link
Member

Choose a reason for hiding this comment

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

Is there maybe an easy way to keep BC?

@tsteur
Copy link
Member

tsteur commented Aug 13, 2015

Is this PR similar to #8368 ? Also needs a rebase.

Re HTTP API break maybe @mattab can have a look

@mattab
Copy link
Member

mattab commented Aug 13, 2015

@mnapoli can you target this PR for 3.0 branch? we will break the API there and keep 2.15.0 without API breakage 👍

@mnapoli
Copy link
Contributor Author

mnapoli commented Aug 13, 2015

Is this PR similar to #8368?

Not really it's a more global refactoring and improvement to fix 4 different issues (see the PR description).

Will target 3.0.

@mnapoli mnapoli modified the milestones: 3.0.0, 2.15.0 Aug 13, 2015
@mnapoli mnapoli added the Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. label Aug 13, 2015
@mnapoli mnapoli self-assigned this Aug 13, 2015
@tsteur
Copy link
Member

tsteur commented Aug 13, 2015

So we still need to merge #8368 on top?

@mnapoli
Copy link
Contributor Author

mnapoli commented Aug 13, 2015

No, this PR fixes #7978.

@mnapoli
Copy link
Contributor Author

mnapoli commented Aug 17, 2015

Closed in favor of #8586 that is targetting 3.0.

@mnapoli mnapoli closed this Aug 17, 2015
@mattab mattab modified the milestones: 3.0.0, 3.0.0-b1 Sep 23, 2015
@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
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 Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants