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

Fix for HTML entity decoding of single quotes in campaigns and csv/xml exports #18170

Merged
merged 8 commits into from Nov 5, 2021

Conversation

bx80
Copy link
Contributor

@bx80 bx80 commented Oct 18, 2021

Description:

Fixes #14695

Tweaked the campaign referrer name decoding so that HTML entity single quotes are properly rendered when displayed on the visitor log.

Also changed the XML and CSV datatable render to use HTML entity decode with the ENT_QUOTES option instead of the ENT_COMPAT option so that exported data with entity encoded quotes is correctly converted.

Review

@bx80 bx80 added not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. Needs Review PRs that need a code review labels Oct 18, 2021
@bx80 bx80 added this to the 4.6.0 milestone Oct 18, 2021
@bx80 bx80 self-assigned this Oct 18, 2021
@sgiehl
Copy link
Member

sgiehl commented Oct 20, 2021

@bx80 Are you still working on that one? Had a quick look, but it didn't seem to work as expected. Feel free to readd the Needs Review label, once you are done.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Oct 20, 2021
@bx80
Copy link
Contributor Author

bx80 commented Oct 21, 2021

@sgiehl I've added an additional commit to fix issue #18190 where outlinks with single quotes are not decoded on the visits log. I've checked both the campaign referrer name and outlinks and they both correctly show the single quotes for me. If it's still not working for you, could you provide a screenshot or the test string used? Then I can try to recreate. Maybe character set is a factor?

My test steps for the campaign referrer:

  1. Create a new campaign with a name containing a single quote
  2. Visit a tracked page using the campaign URL.
  3. Checked the visit log campaign name. (contained ' before the fix, single quote after)
    visit log campaigns single quote fix

For the outlinks:

  1. On my test tracked website, added a new link to the wikipedia article reported in the issue.
  2. Followed the link to record a tracked visit.
  3. Checked the visit log actions outlink. (contained ' before the fix, single quote after)
    visit log outlink single quote fix

Thanks!

@bx80 bx80 added the Needs Review PRs that need a code review label Oct 21, 2021
@sgiehl
Copy link
Member

sgiehl commented Oct 25, 2021

@bx80 How did you track the campaign? I have a local site that simply tracks a page view. Adding ?pk_campaign=te%27%22st to the url, triggers tracking of a campaign with the name te'"st.

Visitor log shows this for the visit:
image

The outlink seems to work correct now.

@bx80
Copy link
Contributor Author

bx80 commented Oct 25, 2021

@sgiehl That's pretty much how I did my tests, a local test site tracking page views with a campaign url. I've just copied and pasted your example campaign url from the comment above and repeated the test - it still works fine for me. It seems like there is some other factor here. I wonder if it could be database collation / character set dependent? SELECT @@character_set_database, @@collation_database; shows that I'm using utf8mb4 / utf8mb4_general_ci, how about you?

@sgiehl
Copy link
Member

sgiehl commented Oct 28, 2021

My database collation ist actually still latin, but all tables are utf8mb4. But I don't think that should be the issue.
Maybe you could extend/change the UI tests of the Live plugin by some data containing such special chars, so we can see in the tests if it works or not.

@sgiehl sgiehl removed the Needs Review PRs that need a code review label Oct 28, 2021
@justinvelluppillai
Copy link
Contributor

@sgiehl it'd be good to know if you see any issues left on this needing fixed or what needs to be done to get it merged as well as the useful smaller tips.

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.

Seems that actually works as expected. It took me a while to figure out why it doesn't work for campaigns locally. 🤯 It's actually the MarketingCampaignsReporting plugin that I have active, as it overwrites the campaign in that case. Would you mind checking how to fix that for that plugin as well, and maybe create a pull request there as well? Guess it might need to be added here: https://github.com/matomo-org/plugin-MarketingCampaignsReporting/blob/c7d437e9bf4232b29ec5e1f16dcd21dd457bec7e/VisitorDetails.php#L52

@bx80 bx80 added Needs Review PRs that need a code review and removed Needs Review PRs that need a code review labels Nov 5, 2021
@sgiehl
Copy link
Member

sgiehl commented Nov 5, 2021

@bx80 be good not to update a submodule to a specific branch without mentioning it. After merging the plugin PR, the submodule would now be broken as the reference doesn't exist anymore. Will update the submodule again and merge the PR if tests are passing afterwards

@sgiehl
Copy link
Member

sgiehl commented Nov 5, 2021

failing tests seem to be random failures. Will merge this now 👍

@sgiehl sgiehl merged commit e31d620 into 4.x-dev Nov 5, 2021
@sgiehl sgiehl deleted the m-14695-campaign-apostrophe-encoding branch November 5, 2021 14:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Special character apostrophe is not getting printed properly in visits log (campaign url)
3 participants