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

Improve Behaviour of 'Maybe Later' link on Connect GA CTA for Key Metrics Widget #7285

Closed
wpdarren opened this issue Jul 11, 2023 · 12 comments
Closed
Labels
P0 High priority Type: Enhancement Improvement of an existing feature

Comments

@wpdarren
Copy link
Collaborator

wpdarren commented Jul 11, 2023

Bug Description

While testing #6265 I discovered that if you click on the 'Maybe later' link on the new Key Metrics Connect Google Analytics CTA, it does disappear, but when you connect Analytics again, and then disconnect later, the CTA no longer appears. There was a discussion on the best way to implement this in the comments but due to its complexity it was decided that we should create a ticket.


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

Acceptance criteria

  • The 'Connect Google Analytics CTA' banner within the Key Metrics widget area should reappear in the below scenario even when GA4 is connected and disconnected again, after dismissing the banner on the first disconnection.
    • In other words, any existing dismissal of this CTA should be invalidated when GA4 is connected
  • As per the AC of Implement “Connect GA” CTA after Key Metrics widget setup with GA4 disconnected/unavailable #6265, the banner should continue to be rendered only if more than 2 of the selected metrics are dependent on GA4 and GA4 was disconnected after the widget's setup.

Implementation Brief

Implementing the remove Method

In includes/Core/Dismissals/Dismissed_Items.php:

  • Introduce a new method remove, which receives an argument $item (string). This method is responsible for removing the provided $item from the dismissed_items option.
    • Retrieve the currently dismissed items via the Dismissed_Items::get method, resulting in an array of dismissed items.
    • Remove the provided $item from the array of dismissed items.
    • Persist the updated array of dismissed items using the User_Setting::set method.

Invocation of the remove Method During Module Activation

In includes/Modules/Analytics_4.php:

  • Implement the Module_With_Activation interface in the Analytics_4 class.
  • Implement a new method on_activation. This method should invoke the Dismissed_Items::remove method, passing key-metrics-connect-ga4-cta-widget as the item to remove.

Test Coverage

  • In tests/phpunit/integration/Core/Dismissals/Dismissed_ItemsTest.php, add a new test test_remove, that tests the remove method.
  • In tests/phpunit/integration/Modules/Analytics_4Test.php, add a new test test_on_activation that tests the on_activation method.

QA Brief

Changelog entry

  • N/A
@wpdarren wpdarren added the Type: Bug Something isn't working label Jul 11, 2023
@jimmymadon jimmymadon assigned jimmymadon and unassigned jimmymadon Jul 11, 2023
@mxbclang mxbclang added the P0 High priority label Jul 11, 2023
@aaemnnosttv aaemnnosttv self-assigned this Jul 13, 2023
@aaemnnosttv aaemnnosttv added Type: Enhancement Improvement of an existing feature and removed Type: Bug Something isn't working labels Jul 13, 2023
@aaemnnosttv
Copy link
Collaborator

Thanks @jimmymadon – I added a sub-point to clarify the first – is that correct?

Regarding the second point, is this a change or only here to ensure that this behavior is preserved?

@jimmymadon
Copy link
Collaborator

@aaemnnosttv - The sub point look good - I had written something similar but it sounded like a possible "implementation" to achieve the main point, so I left it out.

The second point just needs to be preserved. I felt like adding it only because that point was technically not fully met in #6265 - i.e., the banner was not showing up when "GA4 was disconnected (again) after the Key Metrics widget was setup". So it can be skipped but added here just for context, completeness and to ensure no regression comes up.

@jimmymadon jimmymadon assigned aaemnnosttv and unassigned jimmymadon Jul 13, 2023
@aaemnnosttv aaemnnosttv removed their assignment Jul 18, 2023
@aaemnnosttv
Copy link
Collaborator

The sub point look good - I had written something similar but it sounded like a possible "implementation" to achieve the main point, so I left it out.

I avoided using implementation specific language for that reason but it is a bit closer than usual. It's okay to add clarity in cases like this I think.

AC 👍

@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Jul 19, 2023
@tofumatt tofumatt self-assigned this Jul 21, 2023
@tofumatt
Copy link
Collaborator

IB ✅

@tofumatt tofumatt removed their assignment Jul 21, 2023
@kuasha420 kuasha420 self-assigned this Jul 24, 2023
@kuasha420 kuasha420 removed their assignment Jul 26, 2023
@tofumatt tofumatt assigned tofumatt and unassigned tofumatt Jul 27, 2023
@wpdarren wpdarren self-assigned this Jul 31, 2023
@wpdarren
Copy link
Collaborator Author

wpdarren commented Aug 3, 2023

QA Update: ❌

@kuasha420 I have tried this on two sites and when I disconnect and reconnect Analytics after clicking on the 'Maybe later' link, the CTA still does not appear asking me to complete the user survey. I tested this on the develop branch.

I added a screencast below. In this scenario, I disconnected and reset Site Kit and then set up Analytics, but the same issue occurs if you simply go to settings and disconnect/reconnect Analytics.

cta.mp4

@wpdarren wpdarren assigned kuasha420 and unassigned wpdarren Aug 3, 2023
@kuasha420
Copy link
Contributor

@wpdarren, sorry for the confusion, but this issue is about the Connect GA full Width CTA, not the Key metrics CTA. Thank you.

@kuasha420 kuasha420 assigned wpdarren and unassigned kuasha420 Aug 7, 2023
@wpdarren
Copy link
Collaborator Author

wpdarren commented Aug 8, 2023

@kuasha420 apologies too for not spotting this, bit slack on my part.

@wpdarren
Copy link
Collaborator Author

wpdarren commented Aug 8, 2023

QA Update: ⚠️

@kuasha420 On the Connect GA CTA, the maybe later no longer appears. See screenshot.

image

I connected Site Kit with Analytics. Went through user input survey, could see the KMW tiles on the widget. I then disconnected Analytics and the CTA appeared above. I did notice this ticket in QA, and wonder if that's overwriting this ticket, and this no longer applies. A little confused here. c.c. @jimmymadon into this too.

@jimmymadon
Copy link
Collaborator

@wpdarren #7337 does remove the use of the full-width banner. The Banner will be shown again in #7278 so this can be QA'd when we re-introduce the banner then. This will be post bug-bash though.

@wpdarren
Copy link
Collaborator Author

wpdarren commented Aug 8, 2023

@jimmymadon the issue is, this ticket is merged and part of 1.107.0. 🤔

@wpdarren
Copy link
Collaborator Author

wpdarren commented Aug 8, 2023

QA Update: ✅

As per conversation with @jimmymadon on Slack. We need to move this to approved. Unable to test until #7278 is in QA. I have added a comment to ensure that we test this scenario in that ticket which will be post-bug bash.

@aaemnnosttv
Copy link
Collaborator

Will revisit in #7278

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P0 High priority Type: Enhancement Improvement of an existing feature
Projects
None yet
Development

No branches or pull requests

7 participants