Ads Module `Ads Conversion ID` Field Value Migration

[10upsimon]

Feature Description

As part of the newly introduced Ads module, there is a requirement that the existing Ads Conversion ID field value (currently residing within the GA4 module settings) be migrated to it's new location within the Ads module settings. Said migration can happen when the user first visits the GA4 settings screen after plugin update, if the user was previously making use of said field.

The migration will be accompanied by an inline notice indicating to the user that said new field has moved to the Ads settings, see the relevant Figma design here: https://www.figma.com/file/THG1FJw5SaUxmiq38Mkf1x/Ads?type=design&node-id=3-2941&mode=design&t=bSNuYLajFDiSm8sD-0

See the relevant area of the Ads Module Design Doc here: https://docs.google.com/document/d/1APuSv95bf62uhzlaFlW6jPrzPKy1avpRYd9W1MSAAJo/edit?resourcekey=0-UuynlcUz9CoubgldR6Z5sg#heading=h.on4pl4vqwjes

---

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

Acceptance criteria

• For users who were previously using the value of the GA4 Ads Conversion ID label (indicated below):
  • 
  • Upon visiting the legacy settings field for the first time following plugin update, the value should be migrated to the new location applicable to the settings defined in the Ads module (SettingsEdit, SettingsView etc) - see #8225
  • User should be shown the applicable inline notice indicating that the field has been moved to the Ads settings, as indicated in the Figma file at https://www.figma.com/file/THG1FJw5SaUxmiq38Mkf1x/Ads?type=design&node-id=3-2941&mode=design&t=bSNuYLajFDiSm8sD-0
    • This inline notice/banner should be dismissible via the standardised pattern of dismissible notices and banners within the Site Kit codebase
    • If not dismissed, the inline notice/banner should be displayed for a period of 28 (twenty eight) days from when the user first sees the notice, i.e at the time of value migration
    • A timestamp of this moment in time should be stored as a setting against the GA4 module
    • This new setting should be used as the timestamp value comparison when calculating if within the 28 day window
• The SettingsEdit and SettingsView components of the GA4 module should be updated to reflect the above requirements
• The SettingsView component of the GA4 module should be updated to employ the necessary migration logic that moves the field value to the new location relative to the new Ads module.
  • This should be performed only once, and migration should happen prior to the SettingsView children being rendered
  • The migration notice should appear immediately thereafter
  • This process can and should happen in an async isLoading manner

Implementation Brief

• [ ] Add assets/js/modules/analytics-4/components/settings/AdsConversionIDSettingsNotice.js
  • Render SettingsNotice component, with similar props as here
    • Pass dismiss prop
    • For icon use info-circle.svg
    • type is the same as in referenced example
    • In the content, as outlined in figma design, Ads settings part is linked. To format a link, first obtain the settings admin url https://github.com/google/site-kit-wp/blob/b19bdd5bf6c3e0b53d3a157022927672c9df88b3/assets/js/components/notifications/SetupSuccessBannerNotification.js#L81-L83 and link it to the ads module settings - ${settingsAdminURL}#/connected-services/${ads_module_slug}
• [ ] Update assets/js/modules/analytics-4/components/settings/SettingsView.js
  • Pull the adsConversionID setting from MODULES_ADS datastore, and from MODULES_ANALYTICS_4 datastore
  • In useEffect for example, check if ads store setting is empty '', analytics store settings has a value and not isDoingSaveSettings
    • Activate the ads module using activateModule action from CORE_MODULES store
    • Migrate the adsConversionID setting from analytics-4 datastore to the ads
    • After ads store settings are saved, clear the setting value in analytics-4 datastore and also dispatch the saveSettings to save the empty value. So migration can happen only once as later on user can save an empty value
    • using setValues of CORE_FORMS, save, say ADS_CONVERSION_ID_MIGRATED form to mark that migration happend
  • Conditionally render AdsConversionIDSettingsNotice component, if ADS_CONVERSION_ID_MIGRATED is true. So it can persist during the session, and be shown only to the users after the migration, if they had the setting previously saved,
• [ ] Update assets/js/modules/analytics-4/components/settings/SettingsEdit.js
  • Conditionally render AdsConversionIDSettingsNotice component like above

Test Coverage

• Add AdsConversionIDSettingsNotice tests and stories

QA Brief

• Set up Site Kit, but do not turn on the adsModule feature flag yet.
• Set up the Analytics module.
• In Analytics settings, set the ads conversion ID (e.g. AW-1234).
• Go to the SK dashboard and turn on the adsModule feature flag.
• Go to the SK settings and open the Analytics settings view.
• Verify that the setting for Ads Conversion ID no longer appears, instead, a notice appears according to the Figma designs.
• Verify that the Ads module now appears connected in the list of connected modules.
• Open the Ads module settings view. Verify that the Ads Conversion ID setting there shows the original ID that you had set in the Analytics module.

Changelog entry

• Migrate the Ads Conversion ID field from the Analytics module to the Ads module.

[eugene-manuilov]

AC 🌶️

[zutigrm]

Hi @10upsimon , in this case, the field will be updated only if/when user visits the settings for Analytics module. In case user Has the ads ID value, and sets up the Ads module without visiting the Analytics module settings, he will need to input the field value again, is that excepted? We can still use the backend migration script to ensure field is always migrated without relying on user visiting the settings, what do you think?

[10upsimon]

@zutigrm the idea is that in the component logic for when we need this value, we would continue to use the old stored value if it exists and if not yet migrated, so yes it should migrate in settings view. This is also because the user is most likely to return there to update the value, and that is where we make them aware that the location has changed. Does this make sense?

[zutigrm]

@10upsimon Got it, thanks

[zutigrm]

Module activation (for users already having this setting as part of Analytics) is included as part of this issue, after syncing with @10upsimon , we decided it made most sense, since updating the setting will already satisfy the is_connected criteria for ads module

[eugene-manuilov]

IB ✔️

[10upsimon]

The following updates to the AC have been main IRO the inline notice/banner shown to the user after field value migration:

• This inline notice/banner should be dismissible via the standardised pattern of dismissible notices and banners within the Site Kit codebase
• If not dismissed, the inline notice/banner should be displayed for a period of 28 (twenty eight) days from when the user first sees the notice, i.e at the time of value migration
• A timestamp of this moment in time should be stored as a setting against the GA4 module
• This new setting should be used as the timestamp value comparison when calculating if within the 28 day window

These changes to AC have been implemented based on upon internal discussions and have been approved by @marrrmarrr and @sigal-teller. IB updates to follow suite.

[nfmohit]

@10upsimon Based on the above additions, do you think it is valid to escalate the estimate of this issue by another notch? It will not likely need 30 hours IMO, but it will most likely go over 19. Besides, we have a few additional challenges:

1. Aligning the SettingsNotice component with the design.
2. When we use activateModule to activate the Ads module, it doesn't show up in the list of "Connected Services" until you refresh the page. As a result, the link in the notice doesn't work immediately. I will have to find a way to address that.

As I have already started the execution and am almost halfway, I don't think we should re-write the IB at this point as it will just require additional time.

Please let me know what you think of the estimate, thanks!

CC: @bethanylang @ivonac4 @eugene-manuilov

[10upsimon]

@nfmohit I think it makes sense to bump it for this matter, even if we come under. cc @bethanylang @ivonac4

[nfmohit]

@bethanylang @ivonac4 Based on the above discussion, I'm bumping up the estimate to a 30 pointer. Thanks!

[kelvinballoo]

QA Update :x:

• Tested on dev environment WP 6.4.3, PHP 8.0
• QAB test steps are as expected
• There are mainly issues around the message/box not matching figma and these are all detailed further down
• I tested to ensure that the notice appeared only once. After clicking on 'Got it', it never reappeared, even if site-kit or the Analytics module was disconnected and reconnected
• There is one clarification related to the AC. One bullet says:

  "If not dismissed, the inline notice/banner should be displayed for a period of 28 (twenty eight) days from when the user first sees the notice, i.e at the time of value migration"

  • There is no mentioned of testing this 28 days in the QAB. I want to confirm if this is in the testing scope. If yes, please provide an updated brief on how to test this as we won't be able to wait 28 days for this testing. Maybe we could have constant that can be easily updated for testing purposes?

ISSUE 1: The 'Got it' button margin at the top and bottom should be 10px. Currently it's 8px

Screenshots

Figma: 10px

Implementation: 8 px

ISSUE 2: The icon size is currently 20x 20. It should be 22 x 22

Screenshots

Figma: 22 x 22

Implementation: 20 x 20

ISSUE 3: The space between 'All logged in users' to the notice box is currently 24px. It should be 27px.

Screenshots

Figma: 27px

Implementation: 24px

ISSUE 4: The space from below the notice box to 'See full details in analytics' should be 33px Currently it's 2 x 24 = 48 px

Screenshots

Figma:

Implementation:

ISSUE 5: Currently, in our implementation, we have the external outbound box next to 'See full details in Analytics'. But this is not present on figma. Question is: Should we be removing that box icon?

Screenshots

Figma:

Implementation:

ISSUE 6: In the edit mode, 2 things to highlight:

• Spacing above the notice should be 44px and that below until the CTA button 'Confirm changes' should be 81px. Currently, it's 24 and 48 px respectively.
• The CTA button on figma says 'Confirm changes' while our implementation says 'Save'

Screenshots

Figma:

Implementation:

[nfmohit]

Thank you for sharing your observations, @kelvinballoo! I have responded to all your findings. I also have opened a follow-up PR.

@sigal-teller If it is okay with you, I have a question for you at the response for "ISSUE 2" below, thank you!

There is one clarification related to the AC. One bullet says:

"If not dismissed, the inline notice/banner should be displayed for a period of 28 (twenty eight) days from when the user first sees the notice, i.e at the time of value migration"

• There is no mentioned of testing this 28 days in the QAB. I want to confirm if this is in the testing scope. If yes, please provide an updated brief on how to test this as we won't be able to wait 28 days for this testing. Maybe we could have constant that can be easily updated for testing purposes?

I've updated the QAB so that this can be verified.

ISSUE 1: The 'Got it' button margin at the top and bottom should be 10px. Currently it's 8px

We're using a reusable button component here that has slightly different dimensions than what the design depicts. The height of the button is still the same in both places (40px), hence, in my opinion, it is fine to keep this as it is.

ISSUE 2: The icon size is currently 20x 20. It should be 22 x 22

Same as the former one, we are using a reusable SettingsNotice component which has a fixed icon size (20 x 20) for the icon.

As Site Kit has grown, a lot of design differences have emerged in the usages of notices in settings screens. This has, in my instances, required us to add additional custom CSS to match the difference in the design. In my opinion, we should open an issue to work with @sigal-teller and the UX team to unify their designs so that we can have a one-component-fits-all scenario, if possible.

@sigal-teller: For the icon size here, do you think this is a significant difference that would require us to address this right now, or do you think this is fine to use until we've implemented a unified design for the SettingsNotice component as part of the new issue as described above?

ISSUE 3: The space between 'All logged in users' to the notice box is currently 24px. It should be 27px.

I've included a fix for this in the follow-up PR. Thank you!

ISSUE 4: The space from below the notice box to 'See full details in analytics' should be 33px Currently it's 2 x 24 = 48 px

It uses the same templated spacing between the last setting item and the footer for settings view screens, the same as all other modules. We wouldn't want to change the padding for the Analytics module specifically as this would look inconsistent.

ISSUE 5: Currently, in our implementation, we have the external outbound box next to 'See full details in Analytics'. But this is not present on figma. Question is: Should we be removing that box icon?

No. We use the external icon for all modules, and that is out of the scope of this issue.

ISSUE 6: In the edit mode, 2 things to highlight:

• Spacing above the notice should be 44px and that below until the CTA button 'Confirm changes' should be 81px. Currently, it's 24 and 48 px respectively.
• The CTA button on figma says 'Confirm changes' while our implementation says 'Save'

While that is a little odd as it should always say Confirm changes I think. It should also be disabled when there have been no changes made. It might be worth creating a new issue about it as that is outside the scope of this issue.

[nfmohit]

ISSUE 2: The icon size is currently 20x 20. It should be 22 x 22

Same as the former one, we are using a reusable SettingsNotice component which has a fixed icon size (20 x 20) for the icon.

As Site Kit has grown, a lot of design differences have emerged in the usages of notices in settings screens. This has, in my instances, required us to add additional custom CSS to match the difference in the design. In my opinion, we should open an issue to work with @sigal-teller and the UX team to unify their designs so that we can have a one-component-fits-all scenario, if possible.

@sigal-teller: For the icon size here, do you think this is a significant difference that would require us to address this right now, or do you think this is fine to use until we've implemented a unified design for the SettingsNotice component as part of the new issue as described above?

Based on the internal discussion here, I've opened #8546 that aims to unify designs for all notices that we use across Site Kit. I'm moving this forward to CR. Thanks!

[kuasha420]

hI @kelvinballoo - The follow up PR that fixes some of your observations above has been merged so I've assigned it back to you for another pass. Please make note of @nfmohit's comment above, as only some of the observations are fixed, others are expected (difference between Figma design and plugin) or will be tackled in a follow up issue.

Cheers.

[kelvinballoo]

QA Update ✅

Thanks all.

• Noted that we won't be fixing anything related to no. 1,2,4,5 because other modules are set up as such, unlike the figma, and we will keep those consistent.
• For Issue 3, I confirm that the spacing as been updated to 27px:
• For Issue 6, I have reviewed this across other modules and it seems it's behaving similarly. As such, there is no need to create another ticket here.
• Tested the 28 days timeframe check for the message to disappear and it's working as expected

Moving this ticket to approved.

Screenshot for issue 3 as fixed

The image depicts the section for 24px but there is also another 3 px margin below it, thus adding to 27px

Screenshots for 28 day verification

At 28 days, the message doesn't appear

At 27 days, the message appears

[aaemnnosttv]

@nfmohit @eugene-manuilov I realize we may change the implementation to go back to the DB migration but this issue as-is would be a good candidate for using the reference date rather than Date.now. In most cases this is what we should be using when the unit of time is in days or longer. If we keep this for whatever reason, let's refactor it to use this selector. Aside from that, I would say we should perhaps even open another issue to add an ESlint rule to warn about this when using Date directly.