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

AdSense module shows simultaneously as "unable to set up" and "connected" in banner notification #4014

Closed
marrrmarrr opened this issue Sep 9, 2021 · 6 comments
Labels
Module: AdSense Google AdSense module related issues P1 Medium priority Type: Bug Something isn't working
Milestone

Comments

@marrrmarrr
Copy link
Collaborator

marrrmarrr commented Sep 9, 2021

Bug Description

The AdSense module is shown as simultaneously "unable to set up" and "connected" in success notices in certain situations.

Steps to reproduce

  1. Ensure AdSense is connected
  2. Enable any ad blocker
  3. Complete setup for another service (e.g. Analytics, Idea Hub) in order to get the "success" notification on the dashboard
  4. AdSense shows up in the list as simultaneously "connected" and "impossible to set up"

Screenshots

adsense-adblocker

Additional Context

  • PHP Version:
  • OS: [e.g. iOS]
  • Browser [e.g. chrome, safari]
  • Plugin Version [e.g. 22]
  • Device: [e.g. iPhone6]

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

Acceptance criteria

  • For cases where AdSense is already successfully connected, don't display the ad blocker warning in the success notice.

Implementation Brief

in assets/js/components/ModulesListItem.js,

  • Expand the logic to not disable module here and the so an already connected (ie. setupComplete) module is not disabled.
  • Only render ModuleSettingsWarning compoment here when setupComplete is falsy. ie. { ! setupComplete && <ModuleSettingsWarning slug={ slug } context="modules-list" /> }.

Test Coverage

  • Not needed.

Visual Regression Changes

  • Not needed.

QA Brief

  • Follow steps to reproduce.
  • Adblock module should only show up as connected (not disabled/impossible to set up).
  • Test with Adblock Plus

Changelog entry

  • Update the modules list banner not to disable connected modules.
@marrrmarrr marrrmarrr added P1 Medium priority Type: Bug Something isn't working Module: AdSense Google AdSense module related issues labels Sep 9, 2021
@marrrmarrr
Copy link
Collaborator Author

@felixarntz could you have a look and if the ACs LGTY, move to IB?

@felixarntz
Copy link
Member

@marrrmarrr Thanks, looks great!

@kuasha420 kuasha420 assigned kuasha420 and unassigned kuasha420 Sep 15, 2021
@aaemnnosttv aaemnnosttv self-assigned this Sep 15, 2021
@aaemnnosttv
Copy link
Collaborator

@kuasha420 I think we can simplify this to be specific to ModulesListItem only.

We only should only render ModuleSettingsWarning if the module setup isn't completed since this displays the module requirements error which was raised by the module's checkRequirements function.

Then the only other change I think we need is the logical change in when we add the disabled class, as you already identified 🙂

Regarding the variable name and other specific details of the code, this is getting into code review territory so it's best to describe the changes a little more abstractly and focus more on the logic 👍

@aaemnnosttv aaemnnosttv assigned kuasha420 and unassigned aaemnnosttv Sep 15, 2021
@kuasha420
Copy link
Contributor

@aaemnnosttv I've updated the IB accordingly. Please have a look.

@kuasha420 kuasha420 assigned aaemnnosttv and unassigned kuasha420 Sep 16, 2021
@fhollis fhollis added this to the Sprint 59 milestone Sep 22, 2021
@aaemnnosttv
Copy link
Collaborator

IB ✅

@cole10up
Copy link

cole10up commented Oct 6, 2021

QA ✅

Tested

Confirmed the bug here:
image

Reset and ran the same test to pop the issue above. Confirmed the fix:
image

Continued to setup all modules to confirm no other issues occurred with an ad blocker configured.

Sending to approval

@cole10up cole10up removed their assignment Oct 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: AdSense Google AdSense module related issues P1 Medium priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

7 participants