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

Store User Input answers in Site Database #5898

Closed
kuasha420 opened this issue Sep 25, 2022 · 12 comments
Closed

Store User Input answers in Site Database #5898

kuasha420 opened this issue Sep 25, 2022 · 12 comments
Labels
P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer Rollover Issues which role over to the next sprint Type: Enhancement Improvement of an existing feature

Comments

@kuasha420
Copy link
Contributor

kuasha420 commented Sep 25, 2022

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 Remove infrastructure related to storage and synchronization of user input settings between service and plugin #5899.

Test Coverage

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

QA Brief

  • This shouldn't introduce any visual changes.
  • Go through the User Input flow with different admins multiple times and verify that the completed answers are correctly displayed after completion.
  • Verify that a second admin sees the site scoped question (currently only the Purpose question) as answered, but the others as not, by a first admin.
  • Verify that there are no network request failures, console errors, or debug.log errors.
  • Note: Currently, the user input answers will be wiped out if a user signs into Site Kit. This should be resolved once Simplify logic around user input completion state #5900 is merged.
  • Update addressing Store User Input answers in Site Database #5898 (comment):
    • When going through the user input flow for the first time for a user, verify that this issue no longer exists.

QA:Eng

  • Go through the User Input flow and verify that the settings are correctly saved in the respective _options and _usermeta tables.

Changelog entry

  • Update User Input answers to be stored in WP.
@kuasha420 kuasha420 added the Type: Enhancement Improvement of an existing feature label Sep 25, 2022
@kuasha420 kuasha420 self-assigned this Sep 25, 2022
@kuasha420 kuasha420 assigned aaemnnosttv and unassigned kuasha420 Oct 12, 2022
@aaemnnosttv aaemnnosttv added the P0 High priority label Oct 12, 2022
@aaemnnosttv aaemnnosttv removed their assignment Oct 12, 2022
@hussain-t hussain-t self-assigned this Oct 14, 2022
@eclarke1 eclarke1 added P1 Medium priority and removed P0 High priority labels Oct 14, 2022
@hussain-t hussain-t removed their assignment Oct 18, 2022
@eugene-manuilov eugene-manuilov self-assigned this Oct 18, 2022
@eugene-manuilov
Copy link
Collaborator

  • 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
Copy link
Collaborator

Thanks, @hussain-t. IB ✔️

@nfmohit
Copy link
Collaborator

nfmohit commented Nov 16, 2022

@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

@hussain-t
Copy link
Collaborator

@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?

@kuasha420
Copy link
Contributor Author

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

cc @nfmohit

@nfmohit nfmohit added the QA: Eng Requires specialized QA by an engineer label Nov 18, 2022
@nfmohit nfmohit removed their assignment Dec 8, 2022
@aaemnnosttv aaemnnosttv removed their assignment Dec 8, 2022
@wpdarren wpdarren self-assigned this Dec 9, 2022
@techanvil techanvil self-assigned this Dec 9, 2022
@techanvil
Copy link
Collaborator

QA:Eng ✅

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

image

@techanvil techanvil removed their assignment Dec 9, 2022
@wpdarren
Copy link
Collaborator

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 wpdarren assigned nfmohit and unassigned wpdarren Dec 12, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Dec 12, 2022

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 #5900 is merged.

@nfmohit nfmohit assigned wpdarren and nfmohit and unassigned nfmohit Dec 12, 2022
@wpdarren
Copy link
Collaborator

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.

ui-1.mp4

@nfmohit
Copy link
Collaborator

nfmohit commented Dec 13, 2022

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 nfmohit removed their assignment Dec 13, 2022
@nfmohit nfmohit assigned techanvil and unassigned nfmohit Dec 13, 2022
techanvil added a commit that referenced this issue Dec 13, 2022
…cache

Fix User Input question missing scope
@techanvil techanvil assigned wpdarren and unassigned techanvil Dec 13, 2022
@wpdarren
Copy link
Collaborator

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 #5900 is merged.

Screenshots

image
image
image

@wpdarren wpdarren removed their assignment Dec 13, 2022
@felixarntz felixarntz self-assigned this Dec 15, 2022
@felixarntz felixarntz removed their assignment Dec 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 Medium priority PHP QA: Eng Requires specialized QA by an engineer 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

10 participants