Added site detection for cloudflare and updated noData page to mention cloudflare #20566

AltamashShaikh · 2023-04-06T02:42:13Z

Description:

Added site detection for cloudflare and updated noData page to mention cloudflare.
Fiixes: #PG-2726.

Review

…n cloudflare, #PG-2726

snake14

Looks pretty good. I only had a few minor comments. When I viewed the UI, things looked good. I noticed that it showed as detecting Cloudflare without me having to set the headers or anything, but it looks like it was showing as detecting GA as well. Is that part of being in development mode?

plugins/CoreAdminHome/templates/trackingCodeGenerator.twig

plugins/SitesManager/templates/_siteWithoutDataTabs.twig

snake14 · 2023-04-06T05:29:50Z

plugins/SitesManager/templates/_siteWithoutDataTabs.twig

+            <p>&nbsp;&nbsp;&nbsp;&nbsp;Matomo URL: <code vue-directive="CoreHome.SelectOnFocus">{{ piwikUrl }}</code></p>
+            <p>&nbsp;&nbsp;&nbsp;&nbsp;{{ 'SitesManager_EmailInstructionsYourSiteId'|translate('<code vue-directive="CoreHome.SelectOnFocus">' ~ idSite ~ '</code>')|raw }}</p>
+            <p>8. {{ 'SitesManager_SiteWithoutDataCloudflareFollowStep8'|translate }}</p>
+            <p>9. {{ 'SitesManager_SiteWithoutDataCloudflareFollowStep9'|translate }}</p>


Would using an ordered list (<ol>) instead of manual numbering format things a little more nicely here? I think it might make it so that you don't have to use   to indent the extra lines under line 7 as well.

@snake14 spacing and all was breaking and that was the reason I didn't use that

Keep in mind that we als have rtl languages where such spacing might not make sense. Also using an ordered list would make more sense for those languages, as the numbers might need to be placed on the other side or would be written with Romanian numbers. If you have problems with the layout its better to use css instead of trying to "fix" something using spaces

Changed to ol, please check again

AltamashShaikh · 2023-04-06T10:01:41Z

ew minor comments. When I viewed the UI, things looked good. I noticed that it showed as detecting Cloudflare without me having to set the headers or anything, but it lo

@snake14 no its not by default, can you check the URL of your idsite, and if you CURL that url will you get those required headers?

plugins/SitesManager/templates/_displayJavascriptCode.twig

core/SiteContentDetector.php

sgiehl · 2023-04-06T11:58:23Z

plugins/SitesManager/templates/_siteWithoutDataTabs.twig

+            <p>&nbsp;&nbsp;&nbsp;&nbsp;Matomo URL: <code vue-directive="CoreHome.SelectOnFocus">{{ piwikUrl }}</code></p>
+            <p>&nbsp;&nbsp;&nbsp;&nbsp;{{ 'SitesManager_EmailInstructionsYourSiteId'|translate('<code vue-directive="CoreHome.SelectOnFocus">' ~ idSite ~ '</code>')|raw }}</p>
+            <p>8. {{ 'SitesManager_SiteWithoutDataCloudflareFollowStep8'|translate }}</p>
+            <p>9. {{ 'SitesManager_SiteWithoutDataCloudflareFollowStep9'|translate }}</p>


Keep in mind that we als have rtl languages where such spacing might not make sense. Also using an ordered list would make more sense for those languages, as the numbers might need to be placed on the other side or would be written with Romanian numbers. If you have problems with the layout its better to use css instead of trying to "fix" something using spaces

AltamashShaikh · 2023-04-11T02:39:00Z

@snake14 @sgiehl Updated to use ordered list instead of using <p> tag with numbers manually added

sgiehl

Left some more comments. In addition to that it might be helpful to also add a UI test around that. There are already a couple of tests around empty site detection. E.g. https://github.com/matomo-org/matomo/blob/71dcd54e1b70916cc61f5815352234308a4b8823/tests/UI/specs/EmptySite_GA4_spec.js

plugins/SitesManager/images/icons

core/SiteContentDetector.php

snake14

The changes look alright to me. The links have been updated to the actual FAQ URL.

bx80 · 2023-04-11T23:42:40Z

plugins/SitesManager/lang/en.json

+        "SiteWithoutDataCloudflare": "Cloudflare",
+        "SiteWithoutDataCloudflareDescription": "You can use the Matomo App in Cloudflare to track data, follow the instructions from this %1$sguide%2$s.",
+        "SiteWithoutDataCloudflareIntro": "If you use Cloudflare, you can start tracking data in Matomo seamlessly using the Matomo app available on Cloudflare.",
+        "SiteWithoutDataCloudflareFollowStepsIntro": "Follow this steps to set it up:",


Suggested change

"SiteWithoutDataCloudflareFollowStepsIntro": "Follow this steps to set it up:",

"SiteWithoutDataCloudflareFollowStepsIntro": "Follow these steps to set it up:",

@Stan-vw This change looks good to you ?

'steps' is a plural noun, therefore requires a plural demonstrative, 'this step' vs 'these steps'.

Small tweaks:
"SiteWithoutDataCloudflare": "Cloudflare",
"SiteWithoutDataCloudflareDescription": "You can use the Matomo App in Cloudflare to track data, follow the instructions from %1$sthis guide%2$s.",
"SiteWithoutDataCloudflareIntro": "If you use Cloudflare, you can start tracking data in Matomo seamlessly using the Matomo App available in Cloudflare.",
"SiteWithoutDataCloudflareFollowStepsIntro": "Follow these steps to set it up:",

whoops, you gotta teach me how to make the text so beautiful 😅

updated as suggested

@Stan-vw If you find the lines to tweak on the 'Files Changed' tab up top, then click the blue + by the line number (or drag for multiple lines) it will add a comment against those lines, in the comment box that appears, on the toolbar, there is a page icon with a plus/minus symbol which will add a copy of the lines as a suggestion in the comment edit box, you can then edit this copy of the lines to what you think they should be.

This way they get formatted nicely, can be seen as a 'diff' on the files changes list, can be committed in a single click with attribution and it also creates a comment on the main conversation page here 🙂

AltamashShaikh · 2023-04-12T02:36:41Z

@sgiehl Added UI test and deleted unwanted file

sgiehl · 2023-04-12T08:06:02Z

plugins/SitesManager/templates/_trackingCodeEmail.twig

 {% if ga4Used %}
-{{ 'SitesManager_GA4DetectedEmail'|translate('Google Analytics 4', 'GA', 'https://matomo.org/faq/how-to/migrate-from-google-analytics-4-to-matomo/')|raw }}
+{{ 'SitesManager_GADetectedEmail'|translate('Google Analytics 4', 'GA', 'https://matomo.org/faq/how-to/migrate-from-google-analytics-4-to-matomo/')|raw }}
 {{ 'CoreAdminHome_JSTracking_ConsentManagerEmailNote'|translate }}
 {% endif %}

 {% if gtmUsed %}
-{{ 'SitesManager_GTMDetectedEmail'|translate('Google Tag Manager', 'GTM', 'https://matomo.org/faq/tag-manager/migrating-from-google-tag-manager/')|raw }}
+{{ 'SitesManager_GTMDetectedEmail'|translate('https://matomo.org/faq/tag-manager/migrating-from-google-tag-manager/')|raw }}
+{{ 'CoreAdminHome_JSTracking_ConsentManagerEmailNote'|translate }}
+{% endif %}
+
+
+{% if cloudflare %}
+{{ 'SitesManager_CloudflareDetectedEmail'|translate('https://matomo.org/faq/new-to-piwik/how-do-i-install-the-matomo-tracking-code-on-my-cloudflare-setup/')|raw }}
 {{ 'CoreAdminHome_JSTracking_ConsentManagerEmailNote'|translate }}
 {% endif %}


This might actually look a bit weird in the email, as if multiple stuff is detected each will be displayed with the sentence Or you could directly embed the tracking code in your website instead: in the end.
But I guess improving this generated email (content) might be part of another issue maybe?
ping @Stan-vw

@Stan-vw This is what Stefan meant

@sgiehl Updated the logic

I'd love to improve this email but since we want to ship these changes as quickly as possible I'm ok with the last email format.
@AltamashShaikh, give me a call when you're up, would be good for me to understand how we can iterate this email 👍
cc: @Javi-Ormaechea

sgiehl

Left another minor comment, that might not be part of this issue.
Otherwise this looks good to merge for me.

…n cloudflare (#20566) * Added site detection for cloudflare and updated noData page to mention cloudflare, #PG-2726 * UI fixes * PR ceomments resolution * Changed to ol from p * Spacing for url and idsite fixed * Updated with FAQ url * Updated logic for updatedTab * Created UI test for cloudflare * UI test case updated * Updated UI screenshots * textual changes * Updated UI screenshot * Updated UI screenshot * Tracking email code fixed

…n cloudflare (#20573) * Added site detection for cloudflare and updated noData page to mention cloudflare (#20566) * Added site detection for cloudflare and updated noData page to mention cloudflare, #PG-2726 * UI fixes * PR ceomments resolution * Changed to ol from p * Spacing for url and idsite fixed * Updated with FAQ url * Updated logic for updatedTab * Created UI test for cloudflare * UI test case updated * Updated UI screenshots * textual changes * Updated UI screenshot * Updated UI screenshot * Tracking email code fixed * Cherrypicked Cloudflare changes from 5.x-dev * Removed uneanted lines added due to merge

Added site detection for cloudflare and updated noData page to mentio…

58fa0f6

…n cloudflare, #PG-2726

AltamashShaikh added the BHAG-Tracking-OKR-Q2 label Apr 6, 2023

AltamashShaikh requested a review from sgiehl April 6, 2023 02:58

AltamashShaikh marked this pull request as ready for review April 6, 2023 02:58

AltamashShaikh added the Needs Review PRs that need a code review label Apr 6, 2023

AltamashShaikh requested a review from snake14 April 6, 2023 03:04

UI fixes

4c46149

snake14 approved these changes Apr 6, 2023

View reviewed changes

PR ceomments resolution

975f435

sgiehl reviewed Apr 6, 2023

View reviewed changes

Changed to ol from p

f6bdf5d

AltamashShaikh requested review from snake14 and sgiehl April 11, 2023 01:14

AltamashShaikh added 2 commits April 11, 2023 06:56

Spacing for url and idsite fixed

44f93fa

Updated with FAQ url

80ebb74

Updated logic for updatedTab

8710253

sgiehl requested changes Apr 11, 2023

View reviewed changes

plugins/SitesManager/images/icons Outdated Show resolved Hide resolved

core/SiteContentDetector.php Show resolved Hide resolved

snake14 approved these changes Apr 11, 2023

View reviewed changes

bx80 reviewed Apr 11, 2023

View reviewed changes

AltamashShaikh added 2 commits April 12, 2023 07:14

Created UI test for cloudflare

418a001

UI test case updated

f3c6c2f

AltamashShaikh requested a review from sgiehl April 12, 2023 02:36

AltamashShaikh added 4 commits April 12, 2023 08:16

Updated UI screenshots

a706a09

textual changes

2ad15b0

Updated UI screenshot

2ee0414

Updated UI screenshot

0b15c0c

sgiehl reviewed Apr 12, 2023

View reviewed changes

sgiehl added this to the 5.0.0 milestone Apr 12, 2023

sgiehl approved these changes Apr 12, 2023

View reviewed changes

Tracking email code fixed

56c023b

sgiehl merged commit 8917e26 into 5.x-dev Apr 12, 2023

sgiehl deleted the PG-2726-cloudflare-no-data branch April 12, 2023 08:45

sgiehl added the not-in-changelog For issues or pull requests that should not be included in our release changelog on matomo.org. label Apr 12, 2023

mattab mentioned this pull request Apr 24, 2023

Create & place a cloudflare installation guide #20477

Closed

	"SiteWithoutDataCloudflareFollowStepsIntro": "Follow this steps to set it up:",
	"SiteWithoutDataCloudflareFollowStepsIntro": "Follow these steps to set it up:",

Uh oh!

Added site detection for cloudflare and updated noData page to mention cloudflare #20566

Added site detection for cloudflare and updated noData page to mention cloudflare #20566

Uh oh!

Conversation

AltamashShaikh commented Apr 6, 2023

Description:

Review

Uh oh!

snake14 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AltamashShaikh commented Apr 6, 2023

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AltamashShaikh commented Apr 11, 2023

Uh oh!

sgiehl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

snake14 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AltamashShaikh commented Apr 12, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Stan-vw Apr 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sgiehl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Stan-vw Apr 12, 2023 •

edited

Loading