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

Refactor the Feature Tours *setLastDismissedAt() action to use *setCacheItem(). #6473

Closed
techanvil opened this issue Jan 27, 2023 · 12 comments
Closed
Labels
Exp: SP Good First Issue Good first issue for new engineers P2 Low priority Type: Enhancement Improvement of an existing feature

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jan 27, 2023

Feature Description

Following on from #6472, we should refactor the Feature Tours *setLastDismissedAt() action to use the new *setCacheItem() action.


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

Acceptance criteria

  • The Feature Tours *setLastDismissedAt() action should be updated to reuse the new *setCacheItem() action.
  • The CACHE_LAST_DISMISSED_AT control that calls setItem() should be removed as it will no longer be needed.

Implementation Brief

  • In assets/js/googlesitekit/datastore/user/feature-tours.js:
    • Update the setLastDismissedAt() action with the following:
    • Remove the CACHE_LAST_DISMISSED_AT control.

Test Coverage

  • Update the setLastDismissedAt() action tests to use the *setCacheItem() action instead of the setItem (setItemSpy).

QA Brief

  • Go to tester settings
  • Under the Force initial Site Kit plugin version option input 1.29.0
  • Go to dashboard, you will see feature tour for help menu
  • Click on dismiss
  • Verify that on reload of the page tour is not poping up again
  • Open chrome dev tools, and in the console tab, add: console.log(window.googlesitekit.data.select('core/user').getLastDismissedAt()) it should show number, for example 1693995426179
  • Then enter: console.log(window.googlesitekit.data.select('core/user').areFeatureToursOnCooldown()) it should display true

Changelog entry

  • N/A
@techanvil techanvil added P2 Low priority Type: Enhancement Improvement of an existing feature labels Jan 27, 2023
@techanvil techanvil changed the title Refactor the Feature Tours setLastDismissedAt action to use *setCacheItem(). Refactor the Feature Tours *setLastDismissedAt() action to use *setCacheItem(). Jan 27, 2023
@derweili derweili assigned derweili and unassigned derweili Feb 7, 2023
@hussain-t hussain-t self-assigned this Feb 7, 2023
@hussain-t hussain-t removed their assignment Feb 8, 2023
@eugene-manuilov eugene-manuilov self-assigned this Feb 8, 2023
@eugene-manuilov
Copy link
Collaborator

  • Update the setLastDismissedAt() action with the following:

  • Remove the CACHE_LAST_DISMISSED_AT control.

@hussain-t, no need to change anything in that action, we can replace it completely with the setCacheItem call since it is used only in the dismissTour action. So, what we need to do is to delete the setLastDismissedAt completely and update the dismissTour action to use setCacheItem instead.

Also, for the estimates, I think that 11 is way too much for this simple task. Let's reduce it to 7 at least.

@hussain-t
Copy link
Collaborator

hussain-t commented Feb 9, 2023

@eugene-manuilov that makes sense, but the AC says to update the setLastDismissedAt() action:

The Feature Tours *setLastDismissedAt() action should be updated to reuse the new *setCacheItem() action.

In that case, do we need to update the AC to avoid any confusion down the road?

Also, for the estimates, I think that 11 is way too much for this simple task. Let's reduce it to 7 at least.

I set it to 11 keeping in mind test changes and the Exp: SP label.

@eugene-manuilov
Copy link
Collaborator

@hussain-t

but the AC says to update the setLastDismissedAt() action:

The Feature Tours *setLastDismissedAt() action should be updated to reuse the new *setCacheItem() action.

In that case, do we need to update the AC to avoid any confusion down the road?

yeah, AC is updated.

Also, for the estimates, I think that 11 is way too much for this simple task. Let's reduce it to 7 at least.

I set it to 11 keeping in mind test changes

We won't need to change tests, we will need to remove existing tests for the setLastDismissedAt action.

and the Exp: SP label.

Replacing one action with another and removing some unused code doesn't take long. So this ticket can be easily considered as 3, but since we have that label, we can bump it to 7.

@eugene-manuilov
Copy link
Collaborator

Oups, apologies, @hussain-t, you are right, I checked the codebase and looks like what we need to do is what you mentioned in IB. Sorry, about confusing you 🤦. IB ✔️ + I'll update it to be 11 again since it makes sense to me know.

@hussain-t
Copy link
Collaborator

No problem. Thanks, @eugene-manuilov 👍

@techanvil techanvil added the Good First Issue Good first issue for new engineers label Aug 31, 2023
@techanvil techanvil assigned techanvil and unassigned techanvil Sep 1, 2023
@zutigrm zutigrm self-assigned this Sep 1, 2023
@zutigrm zutigrm removed their assignment Sep 6, 2023
@tofumatt tofumatt self-assigned this Sep 6, 2023
@tofumatt tofumatt removed their assignment Sep 7, 2023
@mohitwp mohitwp self-assigned this Sep 11, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Sep 15, 2023

QA Update ⚠️

@wpdarren @zutigrm I tried to trigger 'help' feature tour on 2 different sites. But it's not triggered for me. Cleared cache and waited for 2 hours but feature tour not appearing.
Note : Tested this on both latest and develop environment.
Let me know if you 'help' feature tour appearing for you.

  • Tested on plugin version 1.29.0.
  • Verified by clearing session and local cache.
  • UnifiedDashboard and DashboardSharing Feature tour are appearing but feature tour for help menu is not appearing.

image

image

@zutigrm
Copy link
Collaborator

zutigrm commented Sep 15, 2023

Hi @mohitwp, thank you for checking. Yes, it appears for me on development, locally and on instawp.

Screen.Recording.2023-09-15.at.12.16.57.mov

Can you try to spin a new instance on instaWP, and then check the 1.29.0 version override?

Or you can remove tours from your existing instance, by removing it from DB, here is the guide:

Screen.Recording.2023-09-15.at.12.24.50.mov

@mohitwp
Copy link
Collaborator

mohitwp commented Sep 18, 2023

@zutigrm I saw your video and you are highlighting UnifiedDashboard Feature tour. But, QAB says 'Feature tour for help menu'.

Go to dashboard, you will see feature tour for help menu

As confirmed with @wpdarren we have intro tip for dashboard Help menu. See this - Figma link

So, can you please confirm that testing UnifiedDashboard feature tour will be good enough ?

@mohitwp
Copy link
Collaborator

mohitwp commented Sep 18, 2023

QA Update ❌

@zutigrm

As discussed on huddle. I followed QAB and use the snippet but getting undefined for both the snippets.

Recording.546.mp4

@zutigrm
Copy link
Collaborator

zutigrm commented Sep 18, 2023

@mohitwp hm, I can't seem to replicate this. I tried locally and on instaWP, here is the video from instaWP:

Screen.Recording.2023-09-18.at.16.23.16.mov

@techanvil techanvil self-assigned this Sep 18, 2023
@techanvil
Copy link
Collaborator Author

techanvil commented Sep 18, 2023

Hey @mohitwp, I think I've realised what's up here. Looking at your screencast, you've got the "errors" filter in the console selected, which means it's only going to display console.error() messages and not console.log() messages which are at the Info level. Please can you retry with the top "messages" option selected which removes the filter (note the "user messages" and "info" options should also work). Alternatively, you could simply run the QAB snippets without wrapping them in console.log() and their value should be logged directly out.

image

@techanvil techanvil assigned mohitwp and unassigned zutigrm and techanvil Sep 18, 2023
@mohitwp
Copy link
Collaborator

mohitwp commented Sep 19, 2023

QA Update ✅

Thanks @techanvil @zutigrm ! Apologies ! I completely forgot that filter is active under console.

  • Tested on dev environment.
  • Verified on version 1.29.
  • Followed the QAB and getting the expected output for the snippets.
Recording.549.mp4

@mohitwp mohitwp removed their assignment Sep 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Exp: SP Good First Issue Good first issue for new engineers P2 Low priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

9 participants