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.

Sign up for GitHub

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

test for trim action name #17266

Merged
merged 2 commits into from Mar 15, 2021
Merged

test for trim action name #17266

merged 2 commits into from Mar 15, 2021

Conversation

flamisz
Copy link
Contributor

@flamisz flamisz commented Feb 25, 2021 •
edited

Description:

ref #15501

Turned out it's not fixing the original issue. For more details see the comments on that issue.

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

@flamisz flamisz self-assigned this Feb 25, 2021
@flamisz flamisz marked this pull request as ready for review March 2, 2021 22:27
@flamisz flamisz added Needs Review PRs that need a code review not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Mar 2, 2021
@tsteur tsteur mentioned this pull request Mar 8, 2021
Closed
@flamisz flamisz added this to the 4.4.0 milestone Mar 9, 2021
sgiehl
sgiehl requested changes Mar 9, 2021
View reviewed changes
Copy link
Member

@sgiehl sgiehl left a comment •
edited

Choose a reason for hiding this comment

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

@flamisz Looks fine. Maybe you could add a new test for that case where the delimiter is set empty and an actions (url) with pre- or appending spaces is tracked. Guess this could be added here: https://github.com/matomo-org/matomo/blob/4.x-dev/tests/PHPUnit/Integration/Tracker/ActionTest.php

@sgiehl sgiehl added Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. and removed Needs Review PRs that need a code review labels Mar 9, 2021
@flamisz
Copy link
Contributor Author

flamisz commented Mar 10, 2021

@tsteur and @sgiehl I found something interesting with this action_name while I was writing a test.
I modified back my changes to see the test a made is failing, but it's not. So I checked and we already trim the action name, but not there where I modified the code.

matomo/core/Tracker/ActionPageview.php

Lines 28 to 29 in b176160

$actionName = $this->cleanupActionName($actionName);
$this->setActionName($actionName);

and then:

matomo/core/Tracker/Action.php

Lines 212 to 215 in b176160

protected function setActionName($name)
{
$this->actionName = PageUrl::cleanupString((string)$name);
}

So this couldn't be the original issue. I can add the tests and not modifying the code, so we will see actually it is trimmed.
But then what was the issue for the user?

@sgiehl
Copy link
Member

sgiehl commented Mar 10, 2021

@flamisz Had a closer look at the code myself now and it seems the action name is trimmed in all cases already. I guess there are actually no changes needed. Will comment the original issue as well.

@flamisz flamisz changed the title trim action name test for trim action name Mar 10, 2021
@flamisz flamisz removed Pull Request WIP Indicates the current pull request is still work in progress and not ready yet for a review. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Mar 10, 2021
@sgiehl sgiehl added c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. labels Mar 15, 2021
@sgiehl
Copy link
Member

sgiehl commented Mar 15, 2021

Merging this one as an additional test can't hurt ;-)

@sgiehl sgiehl merged commit d38da1a into 4.x-dev Mar 15, 2021
@sgiehl sgiehl deleted the 15501-tracker-api-trim-spaces branch March 15, 2021 09:45
@mattab mattab modified the milestones: 4.5.0, 4.4.0 May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sgiehl sgiehl sgiehl requested changes

Assignees

@flamisz flamisz

Labels
c: Tests & QA For issues related to automated tests or making it easier to QA & test issues. not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Milestone
4.4.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@flamisz @sgiehl @mattab