google / google/site-kit-wp

Enhance unique container name constraint detection when creating new containers

Open
#2,274 13 comments 0 reactions 2 assignees Claimed by @sigal-teller View on GitHub
Module: Tag Manager P2 Type: Enhancement UX
Dominant language
JavaScript
Stars
1.4k
Forks
383
Avg merge
4d 12h
Merged PRs (30d)
80

Description

## Feature Description

In #1817 we enhanced the logic used for Tag Manager when creating a new container to avoid a potential (cryptic) error from the API due to a requirement for containers to have unique names.

Currently we only require that the new container name is not already in the list of all containers, however for secondary AMP specifically where we create multiple containers in the same request this would fail to detect a non-unique name if both new containers had the same name. This is a bit of an edge case but is just as easily detectable and avoidable.

---------------

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

## Acceptance criteria

* When creating new web and AMP containers at the same time (i.e. in a secondary AMP setup), the user should be blocked from submitting if both containers have the same name
* If both containers have the same name, a validation error should appear indicating that both names cannot be the same
* The text for this error should be: "Web and AMP containers cannot have the same name"
* This validation error should be secondary to the first (current) error which requires the names are unique among the set of all current existing containers – e.g. if both inputs were set to an existing container name, there should only be the single validation error on each field that "A container with this name already exists" as today. This would only be shown if both containers are set to the same name that also does not already exist in the account by that name.

## Implementation Brief

* Implement `compareWith` prop in `ContainerNameTextField` component in `assets/js/modules/tagmanager/components/common/ContainerNameTextField.js`.
* * the prop will receive the fieldname of the other field.
* get the value of `compareWith` (ie. `otherContainerName`) using the useSelect hook similarly as done [here.
](https://github.com/google/site-kit-wp/blob/f54254f2bf2e71d6660c4f5f3510c89027a048bd/assets/js/modules/tagmanager/components/common/ContainerNameTextField.js#L46)
* Derive `isSameName` by checking if the normalized value of containerName and otherContainerName is same. ie. `getNormalizedContainerName(containerName) === getNormalizedContainerName(otherContainerName)`. It is advisable to create a util function similarly as `isUniqueContainerName` for this purpose.
* Expend the logic of `HelperText` variable [here](https://github.com/google/site-kit-wp/blob/f54254f2bf2e71d6660c4f5f3510c89027a048bd/assets/js/modules/tagmanager/components/common/ContainerNameTextField.js#L60) and include appropriate message when `isSameName` is `true`.
* Add `isSameName` to the `mdc-text-field-error` class logic [here](https://github.com/google/site-kit-wp/blob/f54254f2bf2e71d6660c4f5f3510c89027a048bd/assets/js/modules/tagmanager/components/common/ContainerNameTextField.js#L83) appropriately. ie. `! containerName || ! isUniqueName || isSameName`.
* In `assets/js/modules/tagmanager/components/common/WebContainerNameTextField.js`, pass `ampContainerName` as the `compareWith` prop value.
* Similarly, In `assets/js/modules/tagmanager/components/common/AMPContainerNameTextField.js`, pass `containerName` as the `compareWith` prop value.
* in `assets/js/modules/tagmanager/datastore/settings.js`, expend the logic of `validateCanSubmitChanges` to include the logic where `containerName` and `ampContainerName` can not be same when `containerID` and `ampContainerID` both are `CONTAINER_CREATE`.

### Test Coverage

* Add tests on `assets/js/modules/tagmanager/datastore/settings.test.js`.
* * Both name cannot be same when creating Both Web and AMP container.
* * add/update `canSubmitChanges` tests.

### Visual Regression Changes

* No changes is required.

## QA Brief

*

## Changelog entry

*

Contributor guide

Open the contributing guide

Assessment

This issue has not been assessed yet.

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.