GTM and Analytics Create Account or Create New Container Buggy about site-kit-wp HOT 11 CLOSED

@ivankristianto

So to fix it, it needs to add conditional in settings-modules.js handleButtonAction, to catch error and set it from props, so the setup component can show the error response.

Can you clarify this in more detail in how you plan to set the error on the TagmanagerSetup component using this technique?

It would be even better if this didn't use filters here. It's something we need to review as part of the JS refactoring.

from site-kit-wp.

@felixarntz
Me and Lucy find out that the components is always remounting (created and destroyed) on any user action on settings page.
here is the ticket: #132

This will make the implementation more complex since, 1) the save button is outside the component and handled through filter. 2) it destroyed the component and create again.

The not so ideal solution to fix this with current architecture is, TagmanagerSetup pass the error message back to the parent component SettingsModules with props, and when recreating the component, pass back the error message back with props. Then in TagmanagerSetup handle the error message.

But if we are planning to refactor this later soon, I don't think this worth to pursuit. And I like to hear more feedback from you.

cc: @adamsilverstein @aaemnnosttv and @lucymtc

from site-kit-wp.

Assigning to @felixarntz for feedback, per Ivan's last comment. Many thanks!

from site-kit-wp.

@ivankristianto I reviewed this together with @swissspidy and we included a plan to address the problems in the current architecture in the JS refactoring.

So to summarize: The error is currently caught in the TagManager component. Wouldn't it be easier to throw an error from there? The SettingsModules component catches errors on the promise, so I assume it would catch this?

from site-kit-wp.

@felixarntz
Yes I think that would work. When it cached by the modulePromise here:

from site-kit-wp.

@felixarntz - Ivan has updated the implementation brief based on the latest comments in the thread, and reassigned to you for review. Many thanks!

from site-kit-wp.

Implementation brief looks good!

from site-kit-wp.

@lucymtc Noticed two problems during QA:

1. If I create a new container during module setup and a container with the name already exists, no feedback error message is shown. Also, the existing container with that name is not selected.
2. If I have change the container in module settings and choose to create a new one, and a container with the name already exists, the feedback error message is shown, but it still shows the previously selected container (to reproduce, select a different container during setup that does not have the correct name).

In other words, this acceptance criteria is not met:

If the same container name already exist, select that container and give a notice message back to the user.

Selecting the correct container generally doesn't work, and the notice is only displayed in settings, but not in setup.

from site-kit-wp.

@felixarntz @marrrmarrr first issue from comment #66 (comment) has a fix on this PR #354

Second issue is a bit trickier and there are also a couple of questions around it:

• When the user selects from the settings page "Setup New Container", creating a new container is triggered after clicking on "Confirm Changes". If the container already exists, should we save the correct container in the settings instead of only preselecting it in the dropdown as user has confirmed the changes?

• If user confirms the changes and we preselect (or save) the account in the dropdown, it isn't obvious the container in the dropdown has changed as the containers are shown by their publicID and not the name. Does it make sense to throw an error at this point or should we preselect/save the correct container warning the user we found an existing account with same name and saved it?

from site-kit-wp.

Code review and QA on the fix for the aforementioned 1. is good!

@marrrmarrr I suggest we create a follow-up issue for the following acceptance criteria:

• If a user chooses "Setup new container" and confirms, but then a container of that name already exists (there is already an error message displayed to indicate that), the existing container with this name should be selected.
• The error message should be expanded to indicate that the existing container of that name has been selected (e.g. "We've preselected the existing container for you.").
• This should be the case in both the module setup flow and the module settings panel.

That means we should adjust the one acceptance criteria on this issue here:

If the same container name already exist, select that container and give a notice message back to the user.

Does that work with you?

from site-kit-wp.

SGTM
@felixarntz let's follow up on the outstanding corner cases in a new issue

from site-kit-wp.