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

Follow API naming conventions for ID field names #6484

Closed
felixarntz opened this issue Jan 27, 2023 · 6 comments
Closed

Follow API naming conventions for ID field names #6484

felixarntz opened this issue Jan 27, 2023 · 6 comments
Labels
Exp: SP Module: Analytics Google Analytics module related issues P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Jan 27, 2023

For consistency with older Site Kit code (e.g. our Analytics internalWebPropertyID in the API is just the "property ID", while our Tag Manager containerID in the API is the "public container ID"), #6319 (comment) led us to calling a GTE "container ID" internalContainerID. This is good for consistency with that existing Site Kit convention, but it's also worth considering that in all of those cases we are being inconsistent with the Google API naming conventions.

I believe in the long term it makes more sense to align with the API naming conventions than to keep following our own derived convention, which is more confusing than helpful particularly when referring to e.g. API docs.

This issue is for simply following the API naming conventions in the scope of the new GTE work. For example internalContainerID should be renamed to containerID, since that's what it is in the API. If we ever need the public container ID for GTE (which we probably don't), we would need to call it publicContainerID.

In the future, we can also align the older Site Kit code that is not following the API naming conventions, as follows:

  • The old UA Analytics code will be removed later this year anyway, as UA is being sunsetted, so we can just wait for that to happen - not worth changing the code.
  • For the Tag Manager module, we can at some point open an issue to rename containerID to publicContainerID ("GTM-...") and internalContainerID to containerID (numeric ID), since that's what those IDs really are. This would also need to involve some migration routine so that the older setting value in the database is maintained correctly.

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

Acceptance criteria

  • Within the Analytics 4 module logic (on server-side and client-side), field referring to the numeric Google Tag container ID should use a name like containerID instead of internalContainerID. This is partially already the case, but e.g. in the GET:container-destinations REST data point it is not.
    • For the GTE work, we can easily change that, since nothing has launched yet. We should also keep in mind to follow that convention for any other ongoing GTE work.
    • This should however not be changed anywhere where it is part of the Tag Manager module. Doing that would break existing sites that use one of those settings, and a renaming there should happen in a separate issue later.

Implementation Brief

  • In Google\Site_Kit\Modules\Analytics_4 class:
    • In the GET:container-destinations route, change the internalContainerID parameter to containerID.
  • In assets/js/modules/analytics-4/datastore/containers.js
    • Account for the updated parameter made above in fetchGetGoogleTagContainerDestinationsStore.

Test Coverage

  • Update any affected tests that are failing due to the change.

QA Brief

  • There is no visible change expected.
  • The Analytics setup and settings should continue to work correctly with gteSupport flag.
  • The GTE tag should be kept being placed correctly when available in the property.

Changelog entry

  • Follow API naming conventions for Analytics 4 field names.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature Module: Analytics Google Analytics module related issues labels Jan 27, 2023
@aaemnnosttv
Copy link
Collaborator

If we ever need the public container ID for GTE (which we probably don't), we would need to call it publicContainerID

@felixarntz (from the description) – would this not still be a deviation from the API names? If we need to disambiguate the setting from others without getting into a multi-dimensional structure it might be better to do something like container_publicID and container_containerID to keep the suffix 1-1 with the API name (only using our preferred casing). For parameters that are already specific to a container (e.g. GET:container-destinations), I think that prefix could be omitted, but it makes more sense as a setting – especially for the GTM module which has web and AMP variants that would compete for the same name.

@felixarntz
Copy link
Member Author

@aaemnnosttv I don't think we need to be that strict. For example container_containerID is a bit unnecessary duplication, and it would become more confusing with something like container_accountID (why would we call it that when it is also the account ID of an account struct elsewhere?).

We can use a different name, but my point is more that the terminology should be the same. It should be clear when seeing the Google API documentation and looking at our settings what is what. For example, for https://developers.google.com/tag-platform/tag-manager/api/v2/reference/accounts/containers#resource-representations:

  • accountID is container.accountId
  • containerID is container.containerId
  • publicContainerID is container.publicId

With the above 3 examples, I don't think there is any ambiguity. Where we are today this is far more confusing:

  • accountID is container.accountId
  • internalContainerID is container.containerId
  • containerID is container.publicId

@felixarntz felixarntz assigned aaemnnosttv and unassigned felixarntz Jan 27, 2023
@aaemnnosttv
Copy link
Collaborator

AC SGTM 👍

  • publicContainerID is container.publicId

@felixarntz for this one, while it doesn't affect this issue, it would be good to align on the convention going forward – why not use publicID instead? Adding Container in the middle also deviates from the convention as a special case.

@aaemnnosttv aaemnnosttv removed their assignment Feb 14, 2023
@felixarntz
Copy link
Member Author

felixarntz commented Feb 14, 2023

@aaemnnosttv

  • publicContainerID is container.publicId

@felixarntz for this one, while it doesn't affect this issue, it would be good to align on the convention going forward – why not use publicID instead? Adding Container in the middle also deviates from the convention as a special case.

I think we should always ensure that it is clear what the ID is referring to. If we just use publicID, we don't know if it's the account public ID or container public ID or whatever other related entity's public ID.

Essentially, the convention I'm thinking about is:

  • Refer to the entity in every field name (e.g. account or container).
  • Stick to the entity's property name where possible, but avoid a duplicate reference (e.g. accountAccountID is not more clear over accountID in any way, while accountPublicID is more clear than accountID or publicID).
  • If an entity's property refers to another entity's ID that is also available on that other entity, go with the simpler version (e.g. use accountID which refers to account.accountId instead of using containerAccountID which refers to container.accountId --> both are the same data).

@kuasha420 kuasha420 self-assigned this Feb 21, 2023
@kuasha420 kuasha420 removed their assignment Feb 25, 2023
@tofumatt tofumatt self-assigned this Mar 1, 2023
@tofumatt
Copy link
Collaborator

tofumatt commented Mar 1, 2023

IB ✅

@tofumatt tofumatt removed their assignment Mar 1, 2023
@kuasha420 kuasha420 self-assigned this Mar 13, 2023
@kuasha420 kuasha420 removed their assignment Mar 13, 2023
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Mar 13, 2023
@techanvil techanvil assigned techanvil and unassigned techanvil Mar 14, 2023
@mohitwp mohitwp self-assigned this Mar 14, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Mar 22, 2023

QA Update ✅

  • Tested on dev environment.
  • Tested analytics set up when gteSupport feature flag is enabled.
  • Verified analytics set up is working as expected with gteSupport feature.

@mohitwp mohitwp removed their assignment Mar 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP Module: Analytics Google Analytics module related issues P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants