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

Simplify logic around user input completion state

Open kuasha420 opened this issue 2 years ago ā€¢ 3 comments

Feature Description

Going forward, user input questions will no longer be mandatory to answer and the given answers will live on the sites WordPress database. Therefore, there will be only two states for a given user regarding User Input completion: complete and incomplete.

Also, it will be easier to infer this state from the saved values and can be represented as a boolean. This will significantly simplify logic around User_Input_State and User_Input_Settings.

Going forward, the User Input is optional for all users, therefore, there should no longer be any automatic redirection to user-input page based on the completion state of User Input Survey.


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

Acceptance criteria

  • The User Input should no longer have a REQUIRED (mandatory) state.
  • The User_Input_Settings class should implement a method that returns true when user has completed the user input questions, otherwise it should return false. The User Input questions should be considered complete for a user when,
    • All the Site Specific questions are answered and
    • All the User specific questions are answered by the current user.
  • The User_Input_State class should be removed and its usage should be refactored to make use of the above method.
  • Users should not automatically be redirected from dashboard to user-input page when they have not completed the user input questions.

Implementation Brief

Test Coverage

QA Brief

Changelog entry

kuasha420 avatar Sep 25 '22 22:09 kuasha420

  • The usage of User_Input_Settings should implement a simpler replacement for User_Input_State with only two state - complete (true) and incomplete (false).

@kuasha420 this is worded in a way that could be misinterpreted. I think the intention here is that User_Input_Settings should implement a new method for checking the completion status? Currently it reads as if the code that uses User_Input_Settings should change.

The usage of User_Input_State should be replaced with the above check.

  • Both missing and required state should be replaced by incomplete state.

This part is a bit confusing as well. Above we're saying the completion status can be represented as a boolean now, but here we're referring to new named user input states. AFAIU, we should be able to remove User_Input_State entirely as part of this simplification. If that's the case then let's say that and then make it a bit more clear as to what should be done where it was used before.

aaemnnosttv avatar Oct 12 '22 21:10 aaemnnosttv

@aaemnnosttv I have updated the wordings and merged #5901 with this one. It it looks good to you. feel free to close #5901. Cheers!

kuasha420 avatar Oct 14 '22 04:10 kuasha420

Thanks @kuasha420 ā€“ LGTM šŸ‘

aaemnnosttv avatar Oct 14 '22 20:10 aaemnnosttv

Hi @kuasha420! šŸ‘‹

Quick question regarding the ACs here. Iā€™m working on the IB and it strikes me, do we really need a method to check the completion state of user input?

I ask this because since we no longer need to redirect the user to the user input page based on the completion state (we will only redirect once after authorisation, if user input settings are not set, which can be checked using User_Input_Settings::has being introduced in #5898), where would this method actually be used?

I'm sure I'm missing something and just in need of a little clarification.

Thank you!

nfmohit avatar Oct 20 '22 05:10 nfmohit

Hi @nfmohit !

While brainstorming, my thought was that we will store the answers in separate Options field, that this function would return true if all the options are set ie. all questions answered. Which will be used in Key Metrics CTA in Dashboard and settings to determine whether the user has completed User Input questions. If we can easily determine that without this method, we can safely skip the method.

Hope that clears it up. Cheers.

kuasha420 avatar Oct 20 '22 09:10 kuasha420

Thank you for the clarification, @kuasha420!

nfmohit avatar Oct 22 '22 07:10 nfmohit

Hi @nfmohit, thanks for the IB.

I am not sure about this point:

  • Update the action added to the googlesitekit_authorize_user hook so that if $previous_scopes is empty, it redirects the user to the user-input page instead of calling the require_user_input() method.

I might have misunderstood something here but this does not appear needed to fulfil the AC and seems counter to the stated aim of the User Input V2 Design Doc, which in the Overview states:

The User Input questions should not be asked immediately after Site Kit Setup for new users.

It looks to me like implementing the above point would result in new users being redirected to the User Input page.

Please could you take a look and either clarify or update/remove this point as necessary?

cc @kuasha420

techanvil avatar Oct 25 '22 13:10 techanvil

Thank you for pointing that out, @techanvil. I have updated the IB to remove the condition and method call entirely.

nfmohit avatar Oct 25 '22 16:10 nfmohit

Thanks @nfmohit!

I've spotted something else, sorry for not realising yesterday. Bear in mind this AC point:

The User_Input_State class should be removed and its usage should be refactored to make use of the above method.

The IB removes User_Input_State and adds the is_complete method, but it doesn't do anything with is_complete. The front-end code that relies on userInputState will no longer work.

We should in fact, rather than simply removing userInputState from the inlined user data, be replacing it with something like isUserInputComplete => is_complete(), and updating the front-end code accordingly, to keep things working.

techanvil avatar Oct 26 '22 09:10 techanvil

Thank you for spotting that, @techanvil! I have updated the IB and have bumped the estimate to 15. Let me know if it looks good now.

nfmohit avatar Oct 26 '22 11:10 nfmohit

Thanks, @nfmohit. LGTM!

IB :white_check_mark:

techanvil avatar Oct 26 '22 11:10 techanvil

Note: #5898 adds a new action googlesitekit_user_input_set that helps set the User_Input_State so that is is set to the correct state once a user submits their answers. When the User_Input_State class is removed, this new action needs to be removed as well.

More information here. Action added here.

CC: @jimmymadon @aaemnnosttv @kuasha420 @techanvil

nfmohit avatar Dec 08 '22 07:12 nfmohit

@jimmymadon, I have closed #5899 since most of the infrastructure code related to storing has been deleted in #5898. The remaining part is the are_settings_empty method, its usage, and test cases you will have to remove as part of this issue. Please refer to my comment.

hussain-t avatar Dec 20 '22 09:12 hussain-t

QA Update: āŒ

I have an observation and a question.

  1. After completing the questions and saving the Congrats! You set your site goals banner appears, and if you close that banner and then refresh the dashboard, the Key metrics banner reappears for a split second. On the screencast below if you pause it at 7 seconds, you'll see the banner. Note: when you navigate to the dashboard (rather than refreshing) the banner does not reappear from what I could see.

https://user-images.githubusercontent.com/73545194/215770991-2a451594-1eb4-4d2f-a73d-4ae51b9dc280.mp4

  1. Question: should the key metrics banner appear underneath the zero data state banner notification? I suspect so, but wanted to check because that is the current behaviour.

wpdarren avatar Jan 31 '23 13:01 wpdarren

@wpdarren Thanks for the thorough testing as always!

  1. The Key metrics banner appearing in the issue above is still using the old User Input banner code which will completely be replaced in #6209 and #6210. So we should tackle the flicker (if it still exists then) as part of those issues.
  2. The KM banner/widget area should not appear when the site is in Gathering Data or Zero Data states. However, this is still not implemented. The gathering data check will be implemented as part of #6209 and the Zero Data state check in #6512.

jimmymadon avatar Feb 01 '23 11:02 jimmymadon

QA Update: āœ…

Thanks @jimmymadon for clarifying those issues will be fixed in other tickets.

Verified:

  • Set up Site Kit with Analytics. Previously the user input questions automatically appeared on the dashboard but they no longer do. šŸ‘ Instead, I see the Key metrics banner instead.
  • When I click on the Personalize your metrics link I am redirected to the user input form.
  • I didn't complete the questions but navigated back to the dashboard and the Key metrics banner still appears.
  • When I complete the questions and submit the responses, I am redirected back to the dashboard and the Congrats! You set your site goals banner appears.
  • When I go to Settings > Admin Settings, the Key metrics banner appears and when I click on the Personalize your metrics link I am redirected to the user input questions. I am able to complete and save the questions.
  • Tested to make sure that the Key metrics banner does not appear when the site is in gathering state and/or does not have analytics connected.

https://user-images.githubusercontent.com/73545194/216032845-c3b0c178-d28c-4a10-a36e-dc910e9f46bc.mp4

wpdarren avatar Feb 01 '23 11:02 wpdarren

@aaemnnosttv @kuasha420 @jimmymadon Apologies, looking at this for approval, I'm lacking some context here. I have tried to find the pull request that addresses this issue but don't see it anywhere in the issue history, and somehow most references in here appear to be from December 2022 or older.

What is the context here? If this was implemented such a long time ago, why is it only in a release now? Or am I just missing something? :)

felixarntz avatar Feb 10 '23 00:02 felixarntz

@felixarntz Here is the PR that addresses this issue. Not sure why it didn't show up as a linked issue - I've linked it in Zenhub and via the comment template on the PR issue. Hope this is what you were looking for - thanks!

jimmymadon avatar Feb 13 '23 13:02 jimmymadon

I have tried to find the pull request that addresses this issue but don't see it anywhere in the issue history, and somehow most references in here appear to be from December 2022 or older.

@felixarntz the PR is linked above but that event was in a folded group by GitHub so you wouldn't see it without expanding that.

Note: I just amended the AC here to remove a reference to a specific class that no longer exists. There was already a relevant requirement, so that part was more of an implementation detail anyways.

This LGTM though so, closing it out!

aaemnnosttv avatar Feb 13 '23 16:02 aaemnnosttv