Issue / 10036 Duplicate Survey Triggers view_kmw view_ga4_dashboard Surveys
#10161
Conversation
…w-kmw-view-ga4-dashboard-surveys.
…setup by current user. Add trigger for view_kmw survey.
|
Build files for 1b52106 are ready:
|
|
Size Change: -299 B (-0.01%) Total Size: 2 MB
ℹ️ View Unchanged
|
…, implement async on tests and waitForRegistry() in order to suppress act() errors.
There was a problem hiding this comment.
Thanks @10upsimon ! Apologies for the late feedback in the cycle here but it seems we're maybe trying to solve the problem in the wrong place.
A survey shouldn't really be possible to be triggered multiple times (regardless of where or how) because we "timeout" survey triggers and so only triggers that aren't timed out should hit the backend.
site-kit-wp/assets/js/googlesitekit/datastore/user/surveys.js
Lines 212 to 234 in f4f7360
Looking at the action, it seems we're not protecting against a survey from being triggered again if that happens before the timeout is set, which is delayed by 30 seconds as you can see above. I'm not familiar with why that is but I'm sure we can avoid concurrent triggers during this time by revisiting this behavior in the datastore and fix the issue everywhere at once rather than fixing it at each point where the trigger is invoked.
| @@ -69,7 +123,6 @@ export default function ChangeMetricsLink() { | |||
| > | |||
| { __( 'Change metrics', 'google-site-kit' ) } | |||
| </Link> | |||
| <SetupCompletedSurveyTrigger /> | |||
There was a problem hiding this comment.
I realize this is counter to the IB but I'd prefer if we didn't change this, but also I don't quite see the benefit of pulling so much of this component (which is now unused) into this one. If the in view state is beneficial, we could source that via useInView (which should source its state from the InViewProvider of the KMW area) although I'm not sure we need to implement an observer just for this button. Pushing the tracking into the render here allows us to use the same conditions for the trigger as for showing. I don't think it's particularly important that the button itself is viewed.
| currentUserID, | ||
| isKeyMetricsSetupCompletedBy, | ||
| ] ); | ||
|
|
||
| if ( ! renderChangeMetricLink ) { |
There was a problem hiding this comment.
The IB says
Include
renderChangeMetricLinkin the condition, surveys shouldn't be triggered if the value is falsy
this wouldn't be necessary though if we can keep the former render-based triggering.
|
Thanks @aaemnnosttv , just to chip in here, It is nice to fix the root problem, but since this issue is a launch blocker for an epic, IB proposed a fix for issue at hand, as working on a core logic will affect all possible surveys which means long and thorough QA to test everything, watch for regression, etc. I would suggest we do that in a separate issue since that can be done at any time, unlike the launch blocker. WDYT? |
Summary
Addresses issue:
Relevant technical choices
PR Author Checklist
Do not alter or remove anything below. The following sections will be managed by moderators only.
Code Reviewer Checklist
Merge Reviewer Checklist