Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
felixarntz opened this issue Mar 7, 2022 · 11 comments
Closed
Labels
P0 High priority PHP Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@felixarntz
Copy link
Member

felixarntz commented Mar 7, 2022

It is currently possible to see a few too many surveys in short time frame, which isn't that great from a user experience perspective. This is particularly related to the view_dashboard trigger, which several surveys get attached to. While there is already a 1 hour timeout, it is only stored on the client-side and it is regularly invalidated e.g. when activating a new module, so that can result in an overload of surveys being shown in little time.


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

Acceptance criteria

  • The implementation of the timeout mechanism for remote-controlled surveys should be modified to rely on a server-side storage (i.e. a storage which persists beyond user sessions in the browser). In practice, this only affects the view_dashboard trigger, which is the only survey trigger ID currently relying on a timeout, but the implementation should be generic enough to support other triggers in the future.
    • The storage should rely on user meta, so that this is stored per user.
    • The storage should support an undefined number of trigger IDs, so it shouldn't be a single meta key just for the view_dashboard trigger.
    • A REST API endpoint to set a trigger timeout should be added, which allows specifying the trigger ID and requested timeout.
    • The client has to then receive awareness of which trigger IDs are currently "on timeout" per user, replacing the session storage lookup for this (potentially with a preloaded REST API endpoint to get the list of trigger timeouts).
  • The timeout for the view_dashboard trigger specifically should be extended to 24 hours (instead of 1 hour as it is now).

Implementation Brief

Create new REST endpoints

Note: The Dismissed Items endpoints and related code under includes/Core/Dismissals can serve as a reference and broadly be copied here.

Working in the directory includes/Core/User_Surveys:

  • Create a new class called Survey_Timeouts to represent the survey timeouts as a user setting.
    • This can mostly be a copy of includes/Core/Dismissals/Dismissed_Items.php, renamed for survey timeouts.
    • OPTION should be googlesitekit_survey_timeouts.
    • get_dismissed_items and filter_dismissed_items should be renamed get_survey_timeouts and filter_survey_timeouts.
    • There is no equivalent need for the method is_dismissed and this can be ignored/removed in Survey_Timeouts.
  • Create a new class User_Surveys, as a container for Survey_Timeouts and REST_User_Surveys_Controller, following the same pattern as includes/Core/Dismissals/Dismissals.php.
    • Update includes/Plugin.php to create and register User_Surveys instead of REST_User_Surveys_Controller.
  • Add new REST routes to REST_User_Surveys_Controller:
    • GET core/user/data/survey-timeouts for retrieving the list of survey timeouts.
    • POST core/user/data/survey-timeout for setting a survey timeout.
    • These can be copied from the GET core/user/data/dismissed-items and POST core/user/data/dismiss-item routes in includes/Core/Dismissals/REST_Dismissals_Controller.php.
    • Other than renaming any "dismiss item" references to "survey timeout", these can be directly copied.
    • Ensure core/user/data/survey-timeouts is preloaded by adding it to the googlesitekit_apifetch_preload_paths filter in the register method.
      • See the corresponding use of googlesitekit_apifetch_preload_paths in REST_Dismissals_Controller for an example.

Create Redux interface to new endpoints

Note: The Redux store for Dismissed Items in assets/js/googlesitekit/datastore/user/dismissed-items.js can serve as a reference and broadly be copied here.

  • Create a new file assets/js/googlesitekit/datastore/user/survey-timeouts.js.
    • This can essentially be a copy of assets/js/googlesitekit/datastore/user/dismissed-items.js, renamed for survey timeouts.
    • The createFetchStore store for the GET .../survey-timeouts endpoint should have the baseName of getSurveyTimeouts.
    • The createFetchStore store for the POST .../survey-timeout endpoint should have the baseName of setSurveyTimeout.
    • dismissItem should be renamed setSurveyTimeout.
    • getDismissedItems should be renamed getSurveyTimeouts.
    • isItemDismissed should be renamed isSurveyTimedOut.
    • isDismissingItem should be renamed isTimingOutSurvey.
    • All other names should be updated to appropriate survey timeout related names.
  • Combine the new store in assets/js/googlesitekit/datastore/user/index.js.

Update triggerSurvey action to use backend data

Update the triggerSurvey action in assets/js/googlesitekit/datastore/user/surveys.js to use backend data via the new store actions and selectors, instead of checking local storage.

  • Replace the condition that checks for a cache hit with a condition that checks:
    • If the isSurveyTimedOut selector returns false and the isTimingOutSurvey selector returns false, for the given triggerID.
    • Note, isSurveyTimedOut must be checked for explicit === false, as undefined indicates the state is not resolved yet.
  • Within the setTimeout callback:
    • Replace the call to setItem with a dispatch of the setSurveyTimeout action, passing in the triggerID and ttl.
    • Await the above action. If successful, the response will be the updated set of survey timeouts.
    • With a successful response, dispatch the receiveGetSurveyTimeouts action with the received survey timeouts, thus updating the local state.
    • If the response has an error, it can be ignored.

Replace useMount with useEffect in SurveyViewTrigger

Within SurveyViewTrigger:

  • Create a variable, shouldTriggerSurvey, containing the boolean value of the condition as described above:
    • The condition is true if the isSurveyTimedOut selector returns false and the isTimingOutSurvey selector returns false, for the given triggerID.
    • Otherwise, it is false.
  • Use useSelect to access the selectors.

The survey should now be triggered in a useEffect with the value of shouldTriggerSurvey as a dependency. This is because the survey timeout state is not guaranteed to be available in the useMount hook, as it may still be resolving at this point.

  • Replace useMount with useEffect.
  • Include shouldTriggerSurvey in the condition for triggering the survey.
  • Ensure shouldTriggerSurvey is included in the useEffect dependencies.

Update the view_dashboard survey timeouts to one day

  • In DashboardApp and DashboardMainApp, update the view_dashboard TTL value to 86400 (i.e. one day, in seconds).

Test Coverage

  • Add PHP and JS tests for the new endpoints and store.
  • Update the tests for triggerSurvey to reflect the new behaviour.
  • Update any failing tests.

QA Brief

  • Install and activate the Always Surveys plugin
  • Go to the dashboard page and confirm that you see the survey. Don't do anything for a minute and then refresh the page.
  • Confirm that you don't see the survey anymore and it doesn't re-appear during the next 24 hours.

Changelog entry

  • Improve logic for handling timeouts for user surveys on the dashboard.
@felixarntz felixarntz added P0 High priority Type: Enhancement Improvement of an existing feature labels Mar 7, 2022
@felixarntz felixarntz assigned felixarntz and unassigned felixarntz Mar 7, 2022
@techanvil techanvil assigned techanvil and unassigned techanvil Mar 9, 2022
@eugene-manuilov eugene-manuilov self-assigned this Mar 15, 2022
@eugene-manuilov
Copy link
Collaborator

Nice work, @techanvil! IB ✔️

@eugene-manuilov eugene-manuilov removed their assignment Mar 15, 2022
@eugene-manuilov eugene-manuilov self-assigned this Mar 30, 2022
@eugene-manuilov eugene-manuilov removed their assignment Apr 11, 2022
@eclarke1 eclarke1 added the Rollover Issues which role over to the next sprint label Apr 11, 2022
@tofumatt tofumatt assigned tofumatt and eugene-manuilov and unassigned tofumatt Apr 12, 2022
@tofumatt tofumatt removed their assignment Apr 14, 2022
@wpdarren wpdarren self-assigned this Apr 15, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 15, 2022

QA Update: ✅

Verified:

  • Confirmed that the survey appeared on the Site Kit Dashboard.
  • When I refreshed the page after a minute, the survey disappeared.
  • Confirmed that you don't see the survey anymore and it doesn't re-appear during the next 24 hours.

image

@wpdarren wpdarren removed their assignment Apr 18, 2022
@felixarntz
Copy link
Member Author

@eugene-manuilov @tofumatt One question for you here: #5003 (review)

@eugene-manuilov
Copy link
Collaborator

@tofumatt do you mind reviewing #5129?

@eugene-manuilov eugene-manuilov removed their assignment Apr 26, 2022
@tofumatt tofumatt removed their assignment Apr 26, 2022
@wpdarren
Copy link
Collaborator

wpdarren commented Apr 26, 2022

QA Update: ❌

@tofumatt as per our Slack conversation, I tested this on develop branch and the survey is triggered but I see a console error which is mentioned below:

googlesitekit-vendor-1ab785b1746c84358448.js:1   POST https://nezapi.us4.instawp.xyz/index.php/wp-json/google-site-kit/v1/core/user/data/survey-event?_locale=user 400 react_devtools_backend.js:3973  Google Site Kit API Error method:POST datapoint:survey-event type:core identifier:user error:"invalid parameter: 'session'"

image

I also checked the main branch and the same error appears.

I don't know if this change also updated main but I do not remember seeing this issue when previously testing this.

@wpdarren wpdarren assigned tofumatt and unassigned wpdarren Apr 26, 2022
@tofumatt tofumatt assigned eugene-manuilov and unassigned tofumatt Apr 26, 2022
@tofumatt
Copy link
Collaborator

@wpdarren Odd, I didn't see that error when testing locally.

That said, the session param being missing would make sense if we're using the plugin to force a survey. @eugene-manuilov Is this just caused because of the plugin? I don't think it's related to the change, right?

@eugene-manuilov
Copy link
Collaborator

@wpdarren if you use the plugin that simulates surveys, then that's okay because you see fake surveys and there is no way to submit survey events for them.

@wpdarren
Copy link
Collaborator

QA update: ✅

Verified that the survey appears on Site Kit Dashboard.
After waiting for a minute and then refreshing the page, the survey does not reappear.
Tested this on develop and main branch.

image

@wpdarren wpdarren removed their assignment Apr 26, 2022
@felixarntz
Copy link
Member Author

@eugene-manuilov @tofumatt Just noting that #5129 was created based on develop, but since this is for the release, it needed to be against main. Please keep that in mind for the future.

Can you please approve #5134?

@eugene-manuilov
Copy link
Collaborator

@felixarntz, yes, you are right. Sorry about that. PR is approved.

@tofumatt
Copy link
Collaborator

Ah, sure thing. I actually figured since the issue was already QA'd as-shipped in main that it was safer to leave this one in develop-only, but fair enough: that confuses things. 😅

I've approved that main-targetting PR as well, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority PHP Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

6 participants