Dashboard sharing GA change events are raised when no change is made

[aaemnnosttv]

Bug Description

In #5221 we added events for tracking interaction with the new dashboard sharing settings interface, including events for measuring the change to shared roles and sharing management.

These events are currently being triggered when their respective inputs are opened rather than by a change to the value.

Screenshots

Kapture.2022-06-30.at.16.32.36.mp4

---

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

Acceptance criteria

• GA events for changed dashboard sharing settings should only be triggered when the selected values are changed – i.e. selecting the same value or making no change should not trigger the event

Implementation Brief

In assets/js/googlesitekit/modules/datastore/sharing-settings.js, create a new selector haveModuleSharingSettingsChanged with the following changes:

• Receives moduleSlug as required param and keys as optional param defaults to null.
• Get sharingSettings and savedSharingSettings from the state.
• Return undefined if either one is not loaded yet.
• It should be similar to the haveSharingSettingsChanged selector, except that it should compare the sharingSettings and savedSharingSettings of the given moduleSlug.
• If the keys are provided, it should compare the sharingSettings and savedSharingSettings of the given moduleSlug for the given keys.
• Otherwise, it should compare the sharingSettings and savedSharingSettings of the given moduleSlug for all keys/properties.

In assets/js/components/dashboard-sharing/DashboardSharingSettings/Module.js:

• Get whether the user roles have changed using the haveModuleSharingSettingsChanged selector.
• Pass the moduleSlug and 'management' as the moduleSlug and keys params.
• Conditionally call the trackEvent function within a new useEffect hook if the haveModuleSharingSettingsChanged selector returns true.
• Set the management from the getSharingManagement selector to the track event name.
• Remove the trackEvent call from the handleOnChange callback and remove the unnecessary dependencies.

In assets/js/components/dashboard-sharing/UserRoleSelect.js:

• Get whether the user roles have changed using the haveModuleSharingSettingsChanged selector.
• Pass the moduleSlug and 'sharedRoles' as the moduleSlug and keys params.
• In the toggleEditMode callback, ensure trackEvent is called when the edit mode is being toggled off if the haveModuleSharingSettingsChanged selector returns true.
• Update the useCallback dependencies appropriately.
• Remove the initialSharedRoles state occurrences that are not used anymore, as the new selector will handle this.

Test Coverage

• Add test cases for the haveModuleSharingSettingsChanged selector.

QA Brief

• Go to dashboard sharing settings modal.
• Change any module's management setting and ensure the GA event for management setting changes should be triggered.
• Change it back to the previous setting (how it was in the server) and ensure GA event for management setting changes shouldn't be triggered.
• Change any module's user roles setting, close the toggle, and ensure the GA event for the user role setting changes should be triggered.
• Change the roles back to the previous setting (how it was in the server) and ensure GA event for the user role setting changes shouldn't be triggered.

Changelog entry

• Avoid tracking Google Analytics events for Dashboard Sharing settings when no change is made.

[eclarke1]

Following comms with @marrrmarrr confirming this is a 'Nice to Have' for launch, so dropping down to a P1

[aaemnnosttv]

In assets/js/components/dashboard-sharing/DashboardSharingSettings/Module.js:

• Get whether the user roles have changed using the haveSharingSettingsChanged selector.
• Pass the keys parameter with moduleSlug and management as a string value within an array.

@hussain-t the keys parameter limits the settings change check to the given modules (in this case keys are module slugs), it doesn't support a subset of specific sharing settings for a module (although this is somewhat what it is supposed to do). We might need a new selector to check if a specific module's sharing setting has changed. Otherwise, this may result in the event being raised on any settings change for the module, not specifically the management setting.

The same applies to the suggested changes to UserRoleSelect.

[hussain-t]

Thanks, @aaemnnosttv. I have updated the IB.

[aaemnnosttv]

In assets/js/googlesitekit/modules/datastore/sharing-settings.js, create a new selector haveSharingSettingsChangedByModule with the following changes:

• Receives moduleSlug and key as required params.
• To compare the management changes, have a condition to check if key is equal to management.
• Return true if the management setting for the given module has been changed.
• Similarly, to compare the sharedRoles changes, have a condition to check if key is equal to sharedRoles

@hussain-t We shouldn't need to have key-specific conditions. This can probably work similar to the existing haveSharingSettingsChanged selector, but for a specific module. By that I mean something like this would be more appropriate: haveModuleSharingSettingsChanged( moduleSlug, keys = null ). The rest of it would essentially work the same. keys would be optional and without it, it would check if any sharing settings had changed for the given module (same as using haveSharingSettingsChanged with a single key/slug). It could support multiple keys though in the same way, so that it would be just as simple to check multiple specific settings as it would be to check one, without needing additional changes/conditions/selectors later.

[aaemnnosttv]

@hussain-t let's try to prioritize this one as one of the top-priority non-blocking issues for DS.

[hussain-t]

Thanks, @aaemnnosttv. I have updated the IB.

[aaemnnosttv]

Thanks @hussain-t, this is almost there, just a few comments.

• Move the trackEvent call inside the condition, i.e., from the if block to the else block.

This instruction is a bit too low level and could produce in an incorrect result if the condition were to change between now and when the issue is executed. This would also be more resilient and easier to follow if described at a bit higher level, e.g. "ensure trackEvent is called when the edit mode is being toggled off"

Finally, there is some initialSharedRoles state that should no longer be necessary in the UserRoleSelect as this will be handled by the new selector.

[hussain-t]

Thanks for the suggestions, @aaemnnosttv 👍 The IB is updated.

[aaemnnosttv]

IB ✅

[mohitwp]

QA Update ✅

Verified below listed scenarios.

• Change any module's management setting and ensure the GA event for management setting changes should be triggered. - PASS
• Change it back to the previous setting (how it was in the server) and ensure GA event for management setting changes shouldn't be triggered. - PASS
• Change any module's user roles setting, close the toggle, and ensure the GA event for the user role setting changes should be triggered. - PASS
• Change the roles back to the previous setting (how it was in the server) and ensure GA event for the user role setting changes shouldn't be triggered. - PASS
• Verify Cancel event triggers on Cancel button. PASS
• Verify settings confirm event trigger on apply button. - PASS

Recording.128.mp4