site-kit-wp icon indicating copy to clipboard operation
site-kit-wp copied to clipboard

Store User Input answers in Site Database

Open kuasha420 opened this issue 2 years ago • 2 comments

Feature Description

Currently, the answers to the User Input questions are being stored in the Site Kit Service and then being synced and cached. Going forward, the answers should be stored on the WordPress Site's Database.


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

Acceptance criteria

  • Answers to both user specific and site specific User Input questions must be persistently saved in the WordPress database.
  • The related REST route should be updated to retrieve and store User Input answers from the database.

Implementation Brief

In includes/Core/Util/User_Input_Settings.php, add the following changes:

  • Extend the User_Input_Settings with User_Setting abstract class.
  • Create a new constant OPTION with value googlesitekit_user_input_settings.
  • In REST:get_routes method update the following changes in the user-input-settings REST route:
    • Use the User_Input_Settings::get method to retrieve the user input settings instead of the get_settings method.
    • Use the User_Input_Settings::set method to update the user input settings instead of the set_settings method.
  • Update any other references to the get_settings and set_settings methods to use the User_Input_Settings::get and User_Input_Settings::set methods respectively.
  • Update the Authentication::verify_user_input_settings method with the following changes:
    • Use the User_Input_Settings::get method to retrieve the user input settings instead of the User_Input_State::get method.
    • Use the User_Input_Settings::has method to check if the user input settings are set instead of the User_Input_Settings::are_settings_empty method.
  • Removing the legacy code (unused properties and methods) will be implemented in #5899.

Test Coverage

  • Fix any failing tests.
  • Update the tests for the above changes.

QA Brief

Changelog entry

kuasha420 avatar Sep 25 '22 21:09 kuasha420

  • Update the User_Input_Settings::get_settings with the following logic:

    • Remove the existing logic.
    • Retrieve the settings from the parent::get method.
    • Return the settings if they are not empty otherwise, return the default value from the get_default method.
  • Update the User_Input_Settings::set_settings with the following logic:

    • Remove the existing logic.
    • Get the current settings from the get_settings method and merge the new settings with the current settings.
    • Set the merged settings using the set method, inherited from the User_Setting class.

@hussain-t this is not needed because the parent class User_Setting already implements get and set methods that do everything what we need. We just need to remove these methods and update the user input settings usage to use get and set methods instead of get_settings and set_settings.

The are_settings_empty method should be update to be the has method. The usage of the are_settings_empty method should be updated accordingly.

eugene-manuilov avatar Oct 18 '22 09:10 eugene-manuilov

Thanks, @hussain-t. IB ✔️

eugene-manuilov avatar Oct 19 '22 13:10 eugene-manuilov

@hussain-t Quick question for a little confirmation, so that I don't spend time in the wrong direction. Do you think it'd be wise not to store the site specific questions in User_Setting and store it in Setting instead? If so, we could then re-purpose the User_Input_Settings class to be a wrapper above underlying classes for user specific questions and site specific questions. What do you think?

CC: @eugene-manuilov

nfmohit avatar Nov 16 '22 09:11 nfmohit

@nfmohit, it wasn’t mentioned where to store data in the User Input V2 docs. However, based on this point, your suggestion could be right:

The check for whether a user has completed the questionnaire can be determined by checking the associated Setting and User_Setting.

@kuasha420, WDYT?

hussain-t avatar Nov 16 '22 09:11 hussain-t

@hussain-t That is correct and was the intention. Nice.

cc @nfmohit

kuasha420 avatar Nov 16 '22 14:11 kuasha420

@kuasha420 please assign to me for MR once you've completed your review. Thanks!

aaemnnosttv avatar Nov 22 '22 17:11 aaemnnosttv

QA:Eng :white_check_mark:

  • Went through the User Input flow and verified the settings were correctly saved in the DB:

image

techanvil avatar Dec 09 '22 12:12 techanvil

QA Update: ❌

@nfmohit I have a few observations, which might be expected but would like to check.

  1. When I have more than one admin, the message Your answers will apply to the entire WordPress site: any other admins with access to Site Kit can see them and edit them. appears on the first question on load. When I click one of the radio buttons to answer the question, the message disappears. Is this expected? It's a weird UX/UI experience in my opinion.

  2. The notice above also only appearing on the first question. I am assuming is expected?

  3. If Google Analytics is not connected, and I submit the user input questions, when I am redirected to the Site Kit Dashboard, an error appears in the console. Is this expected?

Google Site Kit API Error method:GET datapoint:report type:modules identifier:analytics error:"Module must be active to request data."

image

  1. As Admin 1, I answer all the questions and submit them. They appear in the admin settings as expected. I then login as Admin 2 and connect Site Kit with a different Google account. I am redirected to the the user input questions, but I do not see the message This question has been answered by: and the first question isn't selected on Admin 1 response by default. Is this expected at this stage?

This leads me to that at the moment, when you login with different users, using different Google accounts, each user is able to submit their own separate answers, is this expected too?

wpdarren avatar Dec 12 '22 14:12 wpdarren

Thank you for sharing your findings, @wpdarren.

  1. When I have more than one admin, the message Your answers will apply to the entire WordPress site: any other admins with access to Site Kit can see them and edit them. appears on the first question on load. When I click one of the radio buttons to answer the question, the message disappears. Is this expected? It's a weird UX/UI experience in my opinion.

That is very odd and I can't seem to be able to replicate it. Here's a screencast:

1

  1. The notice above also only appearing on the first question. I am assuming is expected?

That is expected. Only the first question is a site-scoped question. Answers to the other questions vary and are stored separately for each user.

3. If Google Analytics is not connected, and I submit the user input questions, when I am redirected to the Site Kit Dashboard, an error appears in the console. Is this expected?

Google Site Kit API Error method:GET datapoint:report type:modules identifier:analytics error:"Module must be active to request data."

Interesting. I was able to replicate this. However, upon further investigation of the code, it looks like this is happening due to the changes made in #5895. Could we raise it as a potential regression there?

7. As Admin 1, I answer all the questions and submit them. They appear in the admin settings as expected. I then login as Admin 2 and connect Site Kit with a different Google account. I am redirected to the the user input questions, but I do not see the message This question has been answered by: and the first question isn't selected on Admin 1 response by default. Is this expected at this stage?

This leads me to that at the moment, when you login with different users, using different Google accounts, each user is able to submit their own separate answers, is this expected too?

This is expected at this stage. This happens due to my last point in the QA Brief:

Note: Currently, the user input answers will be wiped out if a user signs into Site Kit. This should be resolved once https://github.com/google/site-kit-wp/issues/5900 is merged.

nfmohit avatar Dec 12 '22 16:12 nfmohit

QA Update: ⚠️

@nfmohit for point 1, on a fresh new site (InstaWP and TasteWP) I am still seeing the issue. Would you be able to try and recreate it on a new site, rather than on your local. Have been able to recreate this a few times.

https://user-images.githubusercontent.com/73545194/207268972-b6ab225e-4616-45bd-ab23-026142015075.mp4

wpdarren avatar Dec 13 '22 08:12 wpdarren

Thank you for the re-confirmation, @wpdarren.

I was able to replicate this. Apparently, this only happens when the user has not completed the user input flow yet, which is why I wasn't able to replicate it as I had done it once and was updating it to try and replicate.

For more technical context, when the user input is not at a completed state, the user responses are stored in the cache (session storage). When storing the cache, only the answers were saved, not the scope. This makes the UI lose the scope state and turn it to undefined, causing the reported unexpected behaviour.

Just to clarify, this isn't a regression from this particular issue. I believe this existed from the initiation.

I have opened #6321 to address this.

nfmohit avatar Dec 13 '22 10:12 nfmohit

QA Update: ✅

Verified:

  • Went through the User Input flow with different admins multiple times and the completed answers are correctly displayed after completion.
  • A second admin sees the site scoped question (currently only the Purpose question) as answered.
  • There are no network request failures, console errors, or debug.log errors.
  • Confirmed that the issue reported here has been fixed. When going through the user input flow for the first time for a user, verify that this issue no longer exists.

Currently, the user input answers will be wiped out if a user signs into Site Kit. This should be resolved once https://github.com/google/site-kit-wp/issues/5900 is merged.

Screenshots

image image image

wpdarren avatar Dec 13 '22 13:12 wpdarren