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

Add upgrade notice if property ID starts with "UA" #321

Merged
merged 3 commits into from
Oct 31, 2023
Merged

Add upgrade notice if property ID starts with "UA" #321

merged 3 commits into from
Oct 31, 2023

Conversation

martynmjones
Copy link
Contributor

@martynmjones martynmjones commented Oct 27, 2023 •
edited
Loading

Changes proposed in this Pull Request:

Adds an admin notice if the stored property ID begins with "UA" prompting merchants to use Google's setup assistant and then update their WooCommerce settings.

Screenshots:

Screenshot 2023-10-27 at 17 04 01

Detailed test instructions:

  1. Checkout add/ua-update-notice on a site with the Google Analytics Tracking ID set to UA-.....
  2. Visit other admin areas to confirm the notice is displayed
  3. Confirm notice is clear and links work
  4. Update property ID to start with G-
  5. Confirm notice is no longer displayed

Changelog entry

Add - Update notice for merchants using a Universal Analytics Property ID

@github-actions github-actions bot added changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement. labels Oct 27, 2023
@martynmjones martynmjones marked this pull request as ready for review October 30, 2023 05:38
@martynmjones martynmjones requested a review from a team October 30, 2023 05:39
@martynmjones martynmjones self-assigned this Oct 30, 2023
@martynmjones martynmjones mentioned this pull request Oct 30, 2023
Merged
9 tasks
Base automatically changed from remove/ua-code to remove-universal-analytics October 30, 2023 12:30
ianlin
ianlin reviewed Oct 31, 2023
View reviewed changes
Copy link
Member

@ianlin ianlin left a comment

Choose a reason for hiding this comment

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

Thanks for the change, it worked as expected. I can see the notice being shown when the property ID set to UA-xxx. I just left some questions that I wasn't very sure for discussion.

includes/class-wc-google-analytics.php Outdated
'notice notice-error',
sprintf(
/* translators: 1) URL for Google documentation on upgrading from UA to GA4 2) URL to WooCommerce Google Analytics settings page */
__( 'Your website is configured to use Universal Analytics which Google retired in July of 2023. Update your account using the <a href="%1$s" target="_blank">setup assistant</a> and then update your <a href="%2$s">WooCommerce settings</a>.', 'woocommerce-google-analytics-integration' ), // phpcs:ignore WordPress.Security.EscapeOutput
Copy link
Member

Choose a reason for hiding this comment

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

I'm thinking if using esc_html__ would be better even though we are not using any variables in the string. I slightly lean towards to escape it like the code below to show privacy policy so it would be more consistent and also it could keep the string safe if we were to use the variables inside it in the future.

woocommerce-google-analytics-integration/includes/class-wc-google-analytics.php

Lines 432 to 437 in fd97569

$policy_text = sprintf(
/* translators: 1) HTML anchor open tag 2) HTML anchor closing tag */
esc_html__( 'By using this extension, you may be storing personal data or sharing data with an external service. %1$sLearn more about what data is collected by Google and what you may want to include in your privacy policy%2$s.', 'woocommerce-google-analytics-integration' ),
'<a href="https://support.google.com/analytics/answer/7318509" target="_blank">',
'</a>'
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in 1f0b6fc

includes/class-wc-google-analytics.php Outdated
sprintf(
/* translators: 1) URL for Google documentation on upgrading from UA to GA4 2) URL to WooCommerce Google Analytics settings page */
__( 'Your website is configured to use Universal Analytics which Google retired in July of 2023. Update your account using the <a href="%1$s" target="_blank">setup assistant</a> and then update your <a href="%2$s">WooCommerce settings</a>.', 'woocommerce-google-analytics-integration' ), // phpcs:ignore WordPress.Security.EscapeOutput
'https://support.google.com/analytics/answer/9744165?sjid=9632005471070882766-EU#zippy=%2Cin-this-article',
Copy link
Member

Choose a reason for hiding this comment

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

❔ Does the parameter sjid=9632005471070882766-EU has a special meaning? If not I think we could just use the link https://support.google.com/analytics/answer/9744165.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for picking up on that!

@martynmjones
Copy link
Contributor Author

Hey @ianlin, many thanks for the review! I've made the changes you suggested so this is ready for another look 👀

ianlin
ianlin approved these changes Oct 31, 2023
View reviewed changes
Copy link
Member

@ianlin ianlin left a comment

Choose a reason for hiding this comment

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

Thanks for the extra changes, LGTM 👍

@martynmjones martynmjones merged commit 27ba6d9 into remove-universal-analytics Oct 31, 2023
5 checks passed
@martynmjones martynmjones deleted the add/ua-update-notice branch October 31, 2023 09:24
@jorgemd24 jorgemd24 mentioned this pull request Mar 5, 2024
Merged
19 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ianlin ianlin ianlin approved these changes

Assignees

@martynmjones martynmjones

Labels
changelog: add A new feature, function, or functionality was added. type: enhancement The issue is a request for an enhancement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@martynmjones @ianlin