Enhancement/4925 survey timeouts #5003

eugene-manuilov · 2022-03-30T14:44:47Z

Summary

Addresses issue:

Make timeout for view_dashboard surveys more reliable and extend it #4925

Relevant technical choices

PR Author Checklist

My code is tested and passes existing unit tests.
My code has an appropriate set of unit tests which all pass.
My code is backward-compatible with WordPress 4.7 and PHP 5.6.
My code follows the WordPress coding standards.
My code has proper inline documentation.
I have added a QA Brief on the issue linked above.
I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

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

Code Reviewer Checklist

Run the code.
Ensure the acceptance criteria are satisfied.
Reassess the implementation with the IB.
Ensure no unrelated changes are included.
Ensure CI checks pass.
Check Storybook where applicable.
Ensure there is a QA Brief.

Merge Reviewer Checklist

Ensure the PR has the correct target branch.
Double-check that the PR is okay to be merged.
Ensure the corresponding issue has a ZenHub release assigned.
Add a changelog message to the issue.

…urvey-timeouts.

tofumatt

This largely looks good, just a few questions and small comments. I think after that this can get merged 👍🏻

I tested it locally and it worked well 👍🏻

tofumatt · 2022-04-12T19:26:10Z

assets/js/components/DashboardMainApp.js

-			<SurveyViewTrigger triggerID="view_dashboard" ttl={ 3600 } />
+			<SurveyViewTrigger
+				triggerID="view_dashboard"
+				ttl={ DAY_IN_SECONDS }


includes/Core/User_Surveys/Survey_Timeouts.php

includes/Core/User_Surveys/User_Surveys.php

tests/phpunit/includes/RestTestTriat.php

tofumatt · 2022-04-12T19:56:49Z

tests/phpunit/integration/Core/User_Surveys/REST_User_Surveys_ControllerTest.php

+			array(
+				'data' => array(
+					'slug'    => 'bar',
+					'timeout' => 100, // phpcs:ignore WordPressVIPMinimum.Performance.RemoteRequestTimeout.timeout_timeout


Hah, is this getting triggering incorrectly? Maybe we should add a comment highlighting this, in case someone sees this later and doesn't know why we're (legitimately) disabling this rule here.

Yep. Added a comment .

assets/js/googlesitekit/datastore/user/surveys.js

tofumatt · 2022-04-12T20:15:06Z

assets/js/googlesitekit/datastore/user/surveys.js

 			);

-			if ( false === cacheHit ) {
+			if ( ! isTimedOut && ! isTimingOut ) {


Should these be checks for isTimedOut === false rather than ! isTimedOut? Because isTimedOut could be undefined… do we want this to behave the same way if it's false and undefined?

Suggested change

if ( ! isTimedOut && ! isTimingOut ) {

if ( ! isTimedOut && ! isTimingOut ) {

Those variables are always resolved by the time we get them. Added a comment that explains it.

Ah, awesome, thanks! I think in that case too it's fine to be less explicit with the checks as anyone reading this for prior art will see the comment and know why we're doing a false-y rather than === false check. 👍🏻

Co-authored-by: Matthew Riley MacPherson <hi@tofumatt.com>

…urvey-timeouts.

tofumatt

Awesome, thanks for the comments. Works great when tested locally 🚢

tofumatt · 2022-04-14T22:51:18Z

assets/js/googlesitekit/datastore/user/surveys.js

 			);

-			if ( false === cacheHit ) {
+			if ( ! isTimedOut && ! isTimingOut ) {


Ah, awesome, thanks! I think in that case too it's fine to be less explicit with the checks as anyone reading this for prior art will see the comment and know why we're doing a false-y rather than === false check. 👍🏻

felixarntz

@eugene-manuilov @tofumatt One post-merge question here.

felixarntz · 2022-04-25T17:24:06Z

assets/js/components/surveys/SurveyViewTrigger.js

+	const isTimedOut = useSelect( ( select ) =>
+		select( CORE_USER ).isSurveyTimedOut( triggerID )
+	);
+	const isTimingOut = useSelect( ( select ) =>
+		select( CORE_USER ).isTimingOutSurvey( triggerID )
+	);
+
 	const { triggerSurvey } = useDispatch( CORE_USER );

-	useMount( () => {
-		if ( usingProxy ) {
+	const shouldTriggerSurvey = isTimedOut === false && isTimingOut === false;
+
+	useEffect( () => {
+		if ( shouldTriggerSurvey && usingProxy ) {
 			triggerSurvey( triggerID, { ttl } );
 		}
-	} );
+	}, [ shouldTriggerSurvey, usingProxy, triggerSurvey, triggerID, ttl ] );


Why is all this needed? It looks like the triggerSurvey action already includes these checks and also ensures the API-based bits are resolved. Why duplicate these conditions in here?

I'm not sure why it's needed, and it makes me slightly worried relying on useEffect instead of useMount, since this really should only happen once.

Ah, that's a fair point. With triggerSurvey making sure everything is resolved this isn't needed, but I tend to think about building components like this to protect against unresolved selectors… in this case all of the selectors themselves could return undefined so waiting on them to fulfill like this seemed best.

I don't think this would ever "trigger twice" unintentionally, but in this case replacing it with the action on-mount with a commend saying "The action waits for all relevant selectors to resolve so this is fine to call." would work.

@tofumatt I'll leave it to you and @eugene-manuilov to make a decision on this, but my thinking is that the additional code here is somewhat of a duplicate since the action is already resilient enough to "make sure it works", plus the point about useEffect theoretically being more error-prone to calling the action multiple times - while I agree in that I cannot see a point where this happens using the current logic, it feels more error-prone than using useMount.

If you still feel the current approach is more resilient, we can leave it, but if you agree, please open a follow-up PR to address this.

eugene-manuilov added 18 commits March 30, 2022 17:37

Add the new Survey_Timeouts class.

b1e2304

Add the User_Surveys class.

dcf42b8

Merge remote-tracking branch 'origin/develop' into enhancement/4925-s…

62400af

…urvey-timeouts.

Add survey timeout endpoints.

2219b05

Add the survey-timeouts endpoint to preload filter.

89651e9

Merge remote-tracking branch 'origin/develop' into enhancement/4925-s…

2dafa88

…urvey-timeouts.

Add fetch stores to the user survey partial datastore.

3fd83b4

Merge remote-tracking branch 'origin/develop' into enhancement/4925-s…

1786958

…urvey-timeouts.

Add selectors to receive timeout information.

63cc09f

Merge remote-tracking branch 'origin/develop' into enhancement/4925-s…

ea95338

…urvey-timeouts.

Update the survey trigger function.

e3f296a

Fix broken phpunit tests.

79d3f56

Update js tests.

2b43eb3

Merge remote-tracking branch 'origin/develop' into enhancement/4925-s…

0527365

…urvey-timeouts.

Add phpunit tests for the Survey_Timeouts class.

cf365b5

Add phpunit tests for survey-timeout and survey-timeouts endpoints.

f7d9cf9

Fix minor eslint warning.

bb41c74

Fix isTimingOut selector issue.

31b4942

eugene-manuilov marked this pull request as ready for review April 11, 2022 06:44

Fix permissions check issue for survey endpoints.

8527627

This comment was marked as resolved.

Sign in to view

eugene-manuilov added 2 commits April 11, 2022 13:57

Fix broken e2e tests.

5d81f98

Fix typo.

e0cf51f

tofumatt reviewed Apr 12, 2022

View reviewed changes

eugene-manuilov and others added 5 commits April 13, 2022 16:11

Apply suggestions from code review

f52a110

Co-authored-by: Matthew Riley MacPherson <hi@tofumatt.com>

Merge remote-tracking branch 'origin/develop' into enhancement/4925-s…

befba24

…urvey-timeouts.

Add comments.

dad289f

Fix js tests.

b2cc2db

Fix e2e tests.

549b29c

tofumatt approved these changes Apr 14, 2022

View reviewed changes

tofumatt merged commit 3615194 into develop Apr 14, 2022

tofumatt deleted the enhancement/4925-survey-timeouts branch April 14, 2022 22:52

felixarntz reviewed Apr 25, 2022

View reviewed changes

felixarntz mentioned this pull request Apr 25, 2022

Make timeout for view_dashboard surveys more reliable and extend it #4925

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Enhancement/4925 survey timeouts #5003

Enhancement/4925 survey timeouts #5003

eugene-manuilov commented Mar 30, 2022 •

edited

Loading

This comment was marked as resolved.

tofumatt left a comment

tofumatt Apr 12, 2022

tofumatt Apr 12, 2022

eugene-manuilov Apr 13, 2022

tofumatt Apr 12, 2022

eugene-manuilov Apr 13, 2022

tofumatt Apr 14, 2022

tofumatt left a comment

tofumatt Apr 14, 2022

felixarntz left a comment

felixarntz Apr 25, 2022

tofumatt Apr 25, 2022

felixarntz Apr 25, 2022

	if ( ! isTimedOut && ! isTimingOut ) {
	if ( ! isTimedOut && ! isTimingOut ) {

Enhancement/4925 survey timeouts #5003

Enhancement/4925 survey timeouts #5003

Conversation

eugene-manuilov commented Mar 30, 2022 • edited Loading

Summary

Relevant technical choices

PR Author Checklist

Code Reviewer Checklist

Merge Reviewer Checklist

This comment was marked as resolved.

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tofumatt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

felixarntz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugene-manuilov commented Mar 30, 2022 •

edited

Loading