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

Remove AdSense functionality related to tag permission checks #4627

Closed
aaemnnosttv opened this issue Jan 7, 2022 · 5 comments
Closed

Remove AdSense functionality related to tag permission checks #4627

aaemnnosttv opened this issue Jan 7, 2022 · 5 comments
Labels
Module: AdSense Google AdSense module related issues P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jan 7, 2022

Feature Description

The functionality around existing tag checks for AdSense is being simplified along with other modules. Compared to the other modules, the behavior for AdSense is the simplest regarding existing tags.


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

Acceptance criteria

  • The following selectors and related AdSense data store infrastructure (fixtures, tests, etc) should be removed:
    • hasExistingTagPermission
    • hasTagPermission
    • getTagPermission.
      Essentially everything not provided by createExistingTagStore
  • All use of hasExistingTagPermission in AdSense components should be replaced with a simple check whether the client ID from an existing tag matches the selected client ID
  • The AdSense GET:tag-permission datapoint should be removed entirely (including the has_access_to_client method only used there)

The PR for this issue should target the feature/existing-tag-simplification branch

Implementation Brief

In assets/js/modules/adsense/datastore/tags.js:

  • Rename existingTagStore to store and export it .
  • Remove the combineStore and everything else related to it such as fetchGetTagPermissionStore, baseSelectors, baseResolvers etc.
  • Remove all the unused imports and variables.

In assets/js/modules/adsense/datastore/tags.test.js:

  • Remove tests for hasExistingTagPermission, hasTagPermission and getTagPermission.
  • Remove unused imports and variables.

In assets/js/modules/adsense/datastore/__fixtures__.

  • Delete the folder and everything in it.

In Google\Site_Kit\Modules\AdSense class:

  • Remove GET:tag-permission from the get_datapoint_definitions and create_data_request.
  • Remove the has_access_to_client protected function from the class.
  • Remove tests for GET:tag-permission from AdSenseTest.

In assets/js/modules/adsense/components/setup/SetupAccountApproved.js:

  • Get the current clientID using the getClientID selector of MODULES_ADSENSE store.
  • For hasExistingTagPermission, remove the usage of hasExistingTagPermission selector. Use simple check to see if existingTag === clientID.

Test Coverage

  • Fix any broken tests.

QA Brief

  • Check if tests pass.
  • Check the AdSense setup flow if anything is broken.

Changelog entry

  • Remove functionality related to checking for existing AdSense tag permission.
@aaemnnosttv aaemnnosttv added P1 Medium priority Type: Enhancement Improvement of an existing feature Module: AdSense Google AdSense module related issues labels Jan 7, 2022
@felixarntz
Copy link
Member

ACs LGTM 👍

@felixarntz felixarntz removed their assignment Jan 14, 2022
@kuasha420 kuasha420 assigned kuasha420 and unassigned kuasha420 Jan 18, 2022
@tofumatt tofumatt self-assigned this Jan 26, 2022
@tofumatt
Copy link
Collaborator

Works for me! I guess for now it makes sense to leave the check inline, so IB ✅

@tofumatt tofumatt removed their assignment Jan 26, 2022
@asvinb asvinb self-assigned this Jan 31, 2022
@asvinb asvinb assigned asvinb and unassigned asvinb Feb 4, 2022
@kuasha420 kuasha420 assigned kuasha420 and unassigned kuasha420 Feb 8, 2022
@asvinb asvinb assigned eugene-manuilov and unassigned asvinb Feb 14, 2022
@FlicHollis FlicHollis added the Rollover Issues which role over to the next sprint label Feb 14, 2022
@eugene-manuilov eugene-manuilov removed their assignment Feb 14, 2022
@mohitwp mohitwp self-assigned this Feb 16, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Mar 1, 2022

QA Update: ⚠️

@asvinb @kuasha420 we need more information on the QAB here. Please could you give more detail to the tests you want us to do. The AC/IB is quite technical so needs more information for QA to pass this. cc @mohitwp @eclarke1

@asvinb
Copy link
Collaborator

asvinb commented Mar 1, 2022

Hey @wpdarren we are mostly removing the checks for tags for AdSense. Basically you need to go through the AdSense setup and check if everything is works, i.e nothing is broken. Let me know if that makes sense 😁

@mohitwp
Copy link
Collaborator

mohitwp commented Mar 14, 2022

QA Update ✅

Verified Ad sense functionality with existing tag simplification
Adsense flow working as expected.
Verified when analytics and tag manager module not active.
Verified after adding GTA module manually through header.php
Verified after adding GA analytics module manually through header.php

@mohitwp mohitwp removed their assignment Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: AdSense Google AdSense module related issues P1 Medium priority Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants