Site-kit-wp: Implement `modules/tagmanager` datastore with setup/settings-related capabilities

[felixarntz]

Feature Description

The 3rd module to refactor the setup and settings panels is Tag Manager. This is the first part of that - we'll need a datastore with the relevant selectors and actions.

---

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

Acceptance criteria

• Introduce datastore modules/tagmanager that is registered on googlesitekit.data and exported from assets/js/modules/tagmanager/datastore.
• Rename assets/js/modules/tagmanager/index.js to assets/js/modules/tagmanager/index.legacy.js and update relevant import references.
• Add new assets/js/modules/tagmanager/index.js that for now simply imports from ./datastore.
• Introduce JS entrypoint asset googlesitekit-modules-tagmanager.js with similar handle in PHP. It should simply import from ./modules/tagmanager.
• At the moment, we only store a container's publicId (for us containerID / ampContainerID), but not its actual containerId (see https://developers.google.com/tag-manager/api/v2/reference/accounts/containers#resource for reference). To make API calls, we need to have the containerId too:
  
  
  
  • Whenever we set containerID, we should also set internalContainerID.
  
  
  • Whenever we set ampContainerID, we should also set internalAMPContainerID.
  
  
• The datastore should expose the following public selectors:
  
  
  
  • getAccounts(): resolver should issue API request to GET:accounts datapoint (we'll phase out GET:accounts-containers for this one - contrary to GA, this is really not needed here)
  
  
  • getContainers( accountID, usageContext ): resolver should issue API request to GET:containers. When making the API request, set usageContext to 'both', so all containers are returned, but filter out the results based on the usageContext supplied (it should be either 'web' or 'amp'.
  
  
  • getLiveContainerVersion( accountID, internalContainerID ): resolver should issue API request to a new GET:live-container-version datapoint (should basically pass through to https://developers.google.com/tag-manager/api/v2/reference/accounts/containers/versions/live)
  
  
  • setting selectors getAccountID(), getContainerID(), getInternalContainerID(), getAMPContainerID(), getInternalAMPContainerID(), getUseSnippet(): should be implemented via createSettingsStore; to get the correct name for the AMP container selector we'll need to do some manual tweaking in that specific datastore file, but that should be okay
  
  
• The datastore should expose the following public actions:
  
  
  
  • createContainer( accountID, usageContext ): should issue API request to a new POST:create-container datapoint (should run same logic that is currently internalized in POST:settings and then return the new container object as response)
  
  
  • setting actions for the same four settings above, of course also via createSettingsStore, and with similar tweak to the AMP container one to get the name right
  
  

Implementation Brief

• Create a new module datastore in assets/js/modules/tagmanager/datastore. This file should
  automatically register the datastore on our main registry (googlesitekit.data).
• Move assets/js/modules/tagmanager/index.js to assets/js/modules/tagmanager/index.legacy.js, and update imports. The new index.js should import modules/tagmanager/datastore.
• Create a new entrypoint at assets/js/googlesitekit-modules-tagmanager.js that loads the content from assets/js/modules/tagmanager/index.js. This should also be registered in Assets.php.

modules/tagmanager selectors

• getAccounts() should call GET:accounts (https://github.com/google/site-kit-wp/blob/af32da2ebb1fda7d69508efae68d69b71e8b34b8/includes/Modules/Tag_Manager.php#L460) and return a list of Tag Manager accounts
• getContainers( accountID, usageContext ) should call GET:containers (https://github.com/google/site-kit-wp/blob/af32da2ebb1fda7d69508efae68d69b71e8b34b8/includes/Modules/Tag_Manager.php#L542) with the passed accountID. usageContext should be 'amp' or 'web'; it should only return containers that match the usageContext supplied. When we make the request to the server we should use 'both' as the usageContext so we get all data in a single request—filtering of the data should be done client-side, by the selector.
• getLiveContainerVersion( accountID, internalContainerID ) should call GET:live-container-version. (This PHP endpoint also needs to be implemented; see below)
• This module will have settings regarding which account, container, and AMP container the site should use. We'll also want to be able to toggle the useSnippet setting. We should use createSettingsStore() for:
  
  
  
  • accountID
  
  
  • containerID
  
  
  • ampContainerID
  
  
  • internalContainerID
  
  
  • internalAMPContainerID
  
  
  • useSnippet
  
  
• Update createSettingsStore() to convert an allow list of settingSlugs to uppercase when appropriate:
  
  
  
  • if the first part of the slug is amp, url, id, json, html—and is immediately followed by an uppercase letter—the entire first "section" of the slug (eg. "amp" in "ampContainerID") should be uppercase. (This is only necessary because the selector/action gets prefixed with get / set, so there it should be getAMPContainerID rather than getAmpContainerID or getampContainerID.)
  
  

modules/tagmanager actions

• createContainer( accountID, usageContext ) should call POST:create-container (new; see below) and have the creation of a new Tag Manager container.

PHP Settings

• Add settings for internalContainerID and internalAMPContainerID.
• Don't assume their existence anywhere, since current versions of the plugin don't have them.

PHP Endpoints

To be added to includes/Modules/Tag_Manager.php:

• Add GET:live-container-version; it should expect accountID and internalContainerID and pass through arguments given to the getLiveContainerVersion() selector and call https://developers.google.com/tag-manager/api/v2/reference/accounts/containers/versions/live
• Add POST:create-container; this should be similar to the logic in POST:settings (https://github.com/google/site-kit-wp/blob/af32da2ebb1fda7d69508efae68d69b71e8b34b8/includes/Modules/Tag_Manager.php#L548)

QA Brief

Changelog entry

• Introduce modules/tagmanager datastore to enable JS-based access to Tag Manager data.

[tofumatt]

I've moved this over to IB review but there are a few things I'm not quite clear about and would love clarification on. 😄

There's mention of tweaking the AMP container name here but I'm not clear on how that should be tweaked or what needs changing from the AC/IBs here. Is this something we're already doing and we can reference for the IB?

passing both contexts web and amp, to avoid the potential need to make an additional request; containers should be grouped by usage context when receiving them so that the selector can easily return the correct ones based on the parameter

I'm not clear on what this means from the AC and I've left it out of the IB for now. I couldn't find a reference to usageContext in the REST API code so I'm not sure where/how to pass this value. If it's just about filtering the results that's fine, but it mentions passing the value to the request to Google's API, which I didn't see happening in the code. Maybe I missed something.

But if it's just about filtering returned results, the IB covers it.

[felixarntz]

@tofumatt We already have support for both a web and an amp container. Our REST API datapoint for GET:containers supports a usageContext parameter, which should be either web or amp or both (and web is the default, but here we should pass both). So this is nothing new, what I'm saying here is that we should fire a single request to get _all_ containers and then categorize them on the client by web vs amp, rather than issuing two requests to the server, which would be unnecessary.

I don't think there's a need to have any feature flags here. We can simply introduce the datastore like we have introduced other datastores and left them unused. Then #1386 will be the follow-up PR that will actually start using the datastore, so that's all encapsulated already in a way that will avoid side effects. So let's avoid the feature flag and implement it similarly like we have for Analytics and AdSense.

[felixarntz]

@tofumatt IB mostly looks good to me, a few comments below:

getContainers( accountID, usageContext ) should call GET:containers (https://github.com/google/site-kit-wp/blob/af32da2ebb1fda7d69508efae68d69b71e8b34b8/includes/Modules/Tag_Manager.php#L542) with the passed accountID. usageContext should be 'amp' or 'web'; it should only return containers that match the usageContext supplied.

Maybe add a note here that the resolver though should issue a request with _both_ amp and web to minimize API requests necessary - this way it will technically "resolve" for both possible values of usageContext. Also, note that you have this bullet point twice here, probably accidentally 😄

AMPContainerID

The setting name is ampContainerID, so that needs to be specified there - however it will result in e.g. the selector being getAmpContainerID when it should actually be getAMPContainerID. I think we have two options to address this:

1. Either we manually "hack-fix" this in the GTM datastore definition, like selectors.getAMPContainerID = selectors.getAmpContainerID and delete selectors.getAmpContainerID (same for actions and resolvers).
2. Or we add a capability to createSettingsStore directly to recognize certain abbreviations as such and capitalize them (I think amp, url, id, json, html would be a fairly comprehensive list to start with). Still a bit hacky, but I think a lot cleaner than option 1.

Add PHP endpoint for the calls made by createSettingsStore()

This shouldn't be necessary, GET:settings and POST:settings automatically exist for every module (handled in PHP Modules class).

[tofumatt]

Of those two options: I'm not a big fan of either 😆. I would prefer the consistency of getAmpContainerID, because it's what one would expect out of this API.

That said, if we wanted to _alias_ getAMPContainerID to getAmpContainerID I think that's fine; same goes for any actions. There's no need to delete the original ones, but creating an alias will be easier to reason about in the code and doesn't require solutions that feel as "hacky" or "fragile". This one is a lot more intentional and easier-to-follow, so it's what I suggested here. Let me know what you think 🙂

[felixarntz]

IB ✅

Added a note to the IB on why this abbreviation handling is needed. Generally if we find at some point that we use another abbreviation that isn't covered by this function, we should just add it.

[aaemnnosttv]

@felixarntz one note here regarding GET:containers

When we make the request to the server we should use 'both' as the usageContext so we get all data in a single request—filtering of the data should be done client-side, by the selector.

We updated this datapoint to accept an array of usageContexts to support querying both. The Google API doesn't support this filtering as a parameter but we have it implemented in the response parsing. Basically, we don't need to add a special both parameter because we can provide an array of both which we're already doing now 😄

Should we maybe also have selectors for getWebContainers and getAMPContainers to match the selectors for container ID?

[felixarntz]

@aaemnnosttv I hadn't actually noticed the both being a string - yes it should absolutely be an array. 😄

I'd say let's stick with a single getContainers for now that accepts usageContext. The resolver for that selector will query all containers anyway from the server (by providing both web and app), and it's closer to how the API handles it.

[aaemnnosttv]

Update createSettingsStore() to convert an allow list of settingSlugs to uppercase when appropriate

@felixarntz @tofumatt I'm not sure we need (or should) bake this into createSettingsStore as this is pretty specific to our conventions and could be confusing for users if they assumed it was simply camelcase as it is with all others. We can work around this for now by passing the slug pre-capitalized (e.g. AMPContainerID) so that no special handling is needed internally.

If we really wanted to include something to handle this in the settings store utility, I would think it should create both. Alternatively we could extend createSettingsStore to be a bit more flexible in its API (potentially supporting a different value for settingsSlugs) but I don't think that's necessary.

[tofumatt]

Hah, not sure how we missed that but that's totally true. Let's just go with calling it AMPContainerID. If @felixarntz is happy with that I'll update the IB (or feel free to do it 😄).

[felixarntz]

@aaemnnosttv @tofumatt I think we thought about this before - we cannot pass slugs pre-capitalized because they also are used for the real setting slug, so this wouldn't work.

An alternative I'd be okay with would be to expand the createSettingsStore arguments to accept "override" values for the function names somehow. It would make usage for the function less intuitive for those cases, but it would be more consistent. I think the problem with the current approach is not that we capitalize abbreviations, it's only that we wouldn't be able to automatically capitalize all abbreviations anyone could possibly use.

[aaemnnosttv]

Following up here after talking this over with @felixarntz that we're going to manually "rename" the selector and action since there's only one that is affected by this. When another one comes up we can address the best way to solve this going forward but it might be as simple as creating a generic renameKey utility for objects.

[aaemnnosttv]

@felixarntz while working on the IB for #1381 I realized that we don't actually need datapoints or selectors for workspaces or tags (yet). We only need to request versions:live which returns all the relevant data for the live/published version of a given container. We don't care about all the tags and workspaces in the account, just what the current version of the container is using, which is what this endpoint provides in a single response using just the account and container IDs (I was soo happy to find this considering the alternative which I don't think would work anyways due to container versioning).

In that IB I reference using a getLiveContainer( accountID, internalContainerID ) selector to get access to this data for the other selectors I propose adding, which I think would make sense to implement here (just this one) instead of those for tags and workspaces defined here. I've only added the datapoints for those so far, not the datastore parts, so nothing would really be lost here yet.

I'm aiming to get back to work on this as soon as I can. Let me know what you think!

[felixarntz]

@aaemnnosttv That's awesome you found that, a much better and simpler alternative! So big 👍

Can we name the selector getLiveContainerVersion, to be more in line with how the API treats this? Technically the object returned is a "container version" object, not a "container".

[aaemnnosttv]

Can we name the selector getLiveContainerVersion, to be more in line with how the API treats this? Technically the object returned is a "container version" object, not a "container".

Sure that sounds good to me 👍

[aaemnnosttv]

@felixarntz we'll also need a datapoint for calling versions:live - what would you like that to be called? GET:container-version-live ?

[felixarntz]

@aaemnnosttv That sounds good, maybe we should do GET:live-container-version so that makes it similar to the selector.

[felixarntz]

@aaemnnosttv Updated the ACs to match the new requirements.

[eugene-manuilov]

@felixarntz @EvanHerman @tofumatt while reviewing this ticket I have noticed that a new error store has been introduced for the tagmanager module and it is 100% identical to error datastores available in optimize and adsense modules. What do you think if we create a factory function (similar to createFetchStore) to create generic error stores for modules where it is needed? It will help us to get rid of 3 identical files that we have in the codebase as of now. I can create a ticket for it.

[eugene-manuilov]

QA :heavy_check_mark: - everything seems to be in place.

[aaemnnosttv]

@eugene-manuilov there are a few places where there is some duplication currently occurring in datastore definitions as well as some components. I think this warrants its own issue to DRY up once all of the datastores have been merged so we can address them all at once with an abstraction based on the full context of the current usage.