site-kit-wp
site-kit-wp copied to clipboard
Simplify logic around user input completion state
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 returnstrue
when user has completed the user input questions, otherwise it should returnfalse
. 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
- The usage of
User_Input_Settings
should implement a simpler replacement forUser_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
andrequired
state should be replaced byincomplete
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 I have updated the wordings and merged #5901 with this one. It it looks good to you. feel free to close #5901. Cheers!
Thanks @kuasha420 ā LGTM š
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!
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.
Thank you for the clarification, @kuasha420!
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 theuser-input
page instead of calling therequire_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
Thank you for pointing that out, @techanvil. I have updated the IB to remove the condition and method call entirely.
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.
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.
Thanks, @nfmohit. LGTM!
IB :white_check_mark:
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
@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.
QA Update: ā
I have an observation and a question.
- 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, theKey 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
- 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 Thanks for the thorough testing as always!
- 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.
- 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.
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 thePersonalize 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
@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 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!
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!