site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Update WP Consent API plugin detection to account for non-standard location

Open techanvil opened this issue 1 year ago • 2 comments

Feature Description

As mentioned in this https://github.com/google/site-kit-wp/pull/8305#discussion_r1499937157, it's possible for the WP Consent API plugin entry point to reside in a location other than wp-consent-api/wp-consent-api.php.

https://github.com/google/site-kit-wp/blob/b19bdd5bf6c3e0b53d3a157022927672c9df88b3/includes/Core/Consent_Mode/REST_Consent_Mode_Controller.php#L143-L144

We should update the above code to account for alternative locations for the plugin.


Do not alter or remove anything below. The following sections will be managed by moderators only.

Acceptance criteria

  • The Consent Mode controller should detect that the Consent API plugin is installed, even if it was installed via Composer or into a custom WordPress plugin folder and use the correct path to the plugin file.

Implementation Brief

  • [ ] Update the core/site/data/consent-mode REST route includes/Core/Consent_Mode/REST_Consent_Mode_Controller.php:
    • [ ] Replace the $slug and $plugin variables with a single new variable called $plugin_uri with the value of https://wordpress.org/plugins/wp-consent-api. This is the URI of the plugin which won't change even if the plugin folder or main file changes. https://github.com/google/site-kit-wp/blob/cc356a6a97916db3f2eac54987e886917cee916e/includes/Core/Consent_Mode/REST_Consent_Mode_Controller.php#L155-L156
    • [ ] Create the $slug and $plugin variables as empty strings to set later based on dynamic plugin values.
    • [ ] Update the install check block: https://github.com/google/site-kit-wp/blob/cc356a6a97916db3f2eac54987e886917cee916e/includes/Core/Consent_Mode/REST_Consent_Mode_Controller.php#L166-L171
      • [ ] Get the key and value in the foreach get_plugins() as $plugin_file => $installed_plugin.
      • [ ] Remove the array_keys function so that the loop gets the full plugin objects.
      • [ ] Update the check condition to $installed_plugin["PluginURI"] === $plugin_uri.
        • [ ] Set the $slug variable using $installed_plugin["TextDomain"].
        • [ ] Set the $plugin variable using $plugin_file.
    • The response block will now use the $slug and $plugin variables set based on the WordPress plugin data above, no changes required: https://github.com/google/site-kit-wp/blob/cc356a6a97916db3f2eac54987e886917cee916e/includes/Core/Consent_Mode/REST_Consent_Mode_Controller.php#L177-L184

Test Coverage

  • Confirm existing phpunit tests pass.

QA Brief

Changelog entry

techanvil avatar Feb 23 '24 10:02 techanvil

AC ✔️, I have adjusted it a bit to add a note that the controller should use the right path to the plugin.

eugene-manuilov avatar Jun 12 '24 11:06 eugene-manuilov

IB ✅

tofumatt avatar Jun 12 '24 19:06 tofumatt

QA Update ✅

  • Tested on dev environment.
  • Tested using File manager plugin.
  • Tested by creating custom folder as mentioned in QAB.
  • Verified that the Consent Mode controller detect that the Consent API plugin is installed, even if it was installed via Composer or into a custom WordPress plugin folder and use the correct path to the plugin file.
  • Verified that plugin installation and activation is working as expected in case of installation through custom folder or directly through Site Kit > Settings > Admin Settings.

image

image

image

https://github.com/google/site-kit-wp/assets/94359491/c5afaa8f-1a33-4306-95e7-d9e930ad1436

mohitwp avatar Jul 09 '24 12:07 mohitwp

@mohitwp as per my conversation on Slack. I am a little paranoid with this feature due to past issues 😄 so could you test the sceario in the QAB, but also check that the snippet still appears in the source code and ensure that tracking is working as expected for EU and Non EU traffic. More info on the snippet can be found in the testing instructions. Let me know if you have any questions.

wpdarren avatar Jul 10 '24 11:07 wpdarren

QA Update ✅

    • Tested on dev environment.
    • Tested using File manager plugin.
    • Tested by creating custom folder as mentioned in QAB.
    • Verified that the Consent Mode controller detect that the Consent API plugin is installed, even if it was installed via Composer or into a custom WordPress plugin folder and use the correct path to the plugin file.
    • Verified that plugin installation and activation is working as expected in case of installation through custom folder or directly through Site Kit > Settings > Admin Settings.
  • Verified that tracking is working as expected for Non EU and EU members including UK.
  • Done E2E testing for consent mode functionality as per test instructions.

image

image

image

https://github.com/google/site-kit-wp/assets/94359491/c5afaa8f-1a33-4306-95e7-d9e930ad1436

  • Verified that if consent mode is enabled snippet is showing under source code.

https://github.com/google/site-kit-wp/assets/94359491/f58c36a7-6421-4d60-b39c-630adcdc51f5

image

  • Verified that consent mode snippet is not showing if consent mode is not enabled.

https://github.com/google/site-kit-wp/assets/94359491/f1b1e21f-be10-4990-836c-4e8c8e8c90cd

  • Verified GA events related to WP constant API plugin activation and other Consent mode GA events listed under testing instructions.

image

image

image

image

image

image

image

image

image

mohitwp avatar Jul 10 '24 13:07 mohitwp

⚠️ Approval

There is a small change we should make to the implementation here. See https://github.com/google/site-kit-wp/pull/8959#pullrequestreview-2175494399

I can raise a quick follow up here if needed.

aaemnnosttv avatar Jul 12 '24 18:07 aaemnnosttv

⚠️ Approval

There is a small change we should make to the implementation here. See #8959 (review)

I can raise a quick follow up here if needed.

I've created a follow-up PR.

nfmohit avatar Jul 12 '24 20:07 nfmohit

Thanks @nfmohit, this has been merged and tested so this is good to go 👍

It's worth noting that this issue's changes were somehow reverted (no longer in main or develop) in https://github.com/google/site-kit-wp/commit/9ab0412a6047e43781a011d06293adfedea7001e after it had gone through QA, so the follow-up PR actually re-introduced the change while addressing the points I raised.

aaemnnosttv avatar Jul 12 '24 21:07 aaemnnosttv