Odd UX/UI on the 'Confirm changes' button and enabled state is incorrect

[wpdarren]

Bug Description

As reported by @kelvinballoo on the Asana bug bash board.

Issue 1: When the user toggles the 'Enable enhanced conversion tracking', the 'Save' button doesn't change to 'Confirm changes'. This is the overall usual behaviour whenever we change data on a module.

Issue 2: Linked to the above, if the user enables the toggle without clicking 'Save', the module will show as 'Enabled' after clicking Cancel. By right it should be disabled. It's only when we reload the page that the correct disable status is shown.

The image shows 'Enabled' even though I clicked 'Cancel' and not save.

Uploading Enable_disable.mov…

---

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

Acceptance criteria

• When the state of the "Enable enhanced conversion tracking" toggle is changed in the edit view of either Ads or GA4 settings edit views, the "Save" CTA button should be re-labelled to read as "Confirm changes"
  • If the "Cancel" CTA button is clicked in either of the Ads or GA4 settings edit views, the original state of the "Enable enhanced conversion tracking" toggle should be reverted to, i.e non-saved/discarded state should result in a rollback to the original state that was present when the edit view was first entered into.

Implementation Brief

Please see a POC PR here: #8854

• Within the data store at assets/js/googlesitekit/datastore/site/conversion-tracking.js
  • Define a new Redux style action constant, const ROLLBACK_CONVERSION_TRACKING_SETTINGS = 'ROLLBACK_CONVERSION_TRACKING_SETTINGS';
  • Create a new action to aid in settings rollback, say rollbackConversionTrackingSettings()
    • This should return an empty {} payload using the newly defined ROLLBACK_CONVERSION_TRACKING_SETTINGS type
  • Within the existing baseReducer, handle the case for ROLLBACK_CONVERSION_TRACKING_SETTINGS and set the current state to the savedSettings state, i.e state.conversionTracking.settings = state.conversionTracking.savedSettings;
• Within the SettingsView components of both the Ads & GA4 modules:
  • Import the rollbackConversionTrackingSettings action via the stores useDispatch hook
  • Define a helper function in the form of useCallback, i.e handleConversionTrackingSettingsRollback()
    • This should call the rollbackConversionTrackingSettings() action
  • Within a useEffect hook, call the handleConversionTrackingSettingsRollback() function if conversionInfra feature flag is enabled, i,.e if ( iceEnabled ) { ... }
    • Add iceEnabled and handleConversionTrackingSettingsRollback as deps of the useEffect hook

Test Coverage

• No additional tests or extensions of existing test coverage is required.

QA Brief

• Follow the steps from description
• Verify that issue is fixed

Changelog entry

• N/A

[zutigrm]

AC LGTM ✅

[zutigrm]

Thanks @10upsimon looks good overall, just two comments:

Within the existing baseReducer, handle the case for ROLLBACK_CONVERSION_TRACKING_SETTINGS and set the current state to the savedSettings state, i.e state.conversionTracking.settings = state.conversionTracking.savedSettings

If we are trying to rollback changers to the original state, we should be resetting it to the state.conversionTracking.settings as this holds the original values fetched from the DB. savedSettings are storing the latest changed value after you interact with the option

Within a useSelect hook, call the handleConversionTrackingSettingsRollback() function
Add iceEnabled and handleConversionTrackingSettingsRollback as deps of the useSelect hook

It is probably a typo, I think you meant useEffect hook? Since we shouldn't be dispatching actions from useSelect, and it doesn't have dependencies

Also, please add an estimate

[10upsimon]

If we are trying to rollback changers to the original state, we should be resetting it to the state.conversionTracking.settings as this holds the original values fetched from the DB. savedSettings are storing the latest changed value after you interact with the option

@zutigrm this is not correct. We set both the settings and savedSettings values on initial API fetch, but we update the settings state when a change is made via the toggle, therefore reverting the settings state to savedSettings resets it to the API retrieved value. This can be seen and tested in the aforementioned POC. Change it to the suggested settings as per your comment instead of savedSettings, it has the opposite (and undesired) effect.

It is probably a typo, I think you meant useEffect hook? Since we shouldn't be dispatching actions from useSelect, and it doesn't have dependencies

Correct, good spot :) I updated this.

Estimate added.

[zutigrm]

@10upsimon Ah yes, indeed, I mistaken it for something else. Thanks, IB LGTM

[mohitwp]

QA Update ⚠️

• Tested on dev environment.
• Verified when Ads module set up in NON PAX manner.
• Verified when Ads module setup in PAX manner.

@zutigrm

Issue > When Ads module setup in PAX manner then 'Confirm Changes' CTA is appearing disable and not working. Will you able to fix the issue in follow up PR or should I create new ticket?

Recording.1081.mp4

PASS CASES -

Analytics Settings "Enhanced Conversion Tracking" toggle

Recording.1077.mp4

Ads Module Settings "Enhanced Conversion Tracking" toggle ( NON PAX manner)

Recording.1078.mp4

[zutigrm]

@mohitwp Better to check it in follow up issue

[zutigrm]

@benbowler It seems you forgot to unassign yourself. I unassigned you so it can be picked up for MR

[mohitwp]

QA Update ✅

• Tested on dev environment.
• Verified when Ads module set up in NON PAX manner.
• Verified when Ads module setup in PAX manner.
• Verified issue of 'Confirm Changes' button not working when Ads module is setup through PAX manner is now resolved.

Ads Module Settings "Enhanced Conversion Tracking" toggle (PAX manner)

Recording.1082.mp4

Ads Module Settings "Enhanced Conversion Tracking" toggle (Non PAX manner)

Recording.1083.mp4

Analytics Settings "Enhanced Conversion Tracking" toggle

Recording.1084.mp4

[aaemnnosttv]

⚠️ Approval

@zutigrm the changes here deviate from a semantic convention we have: validation functions throw. This adds a new one which does not. See createValidationSelector and how this is used with validateCanSubmitChanges.

The added validateHaveSettingsChanged to createModuleStore and createSettingsStore is not tested either which is very important for these widely used utilities.

Please open a follow up issue to address this.