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

Top Earning Pages widget cannot determine adsense linked status in view-only mode for non-admin user #5493

Closed
techanvil opened this issue Jul 1, 2022 · 7 comments
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Type: Bug Something isn't working

Comments

@techanvil
Copy link
Collaborator

techanvil commented Jul 1, 2022

Bug Description

When AdSense and Analytics are both connected and shared, and the user is on the view-only Dashboard, the following call to getAdsenseLinked results in a console error due to the call to the Analytics settings endpoint not being allowed. This results in the widget rendering normally even when AdSense is not linked.

isAdSenseLinked: select( MODULES_ANALYTICS ).getAdsenseLinked(),

See #5310 (comment)

Steps to reproduce

  1. Connect and share both AdSense and Analytics.
  2. Navigate to the view-only Dashboard as a user whose rule the above modules are shared with.
  3. The error should appear in the JS console.

Screenshots

image.png

Additional Context

  • PHP Version: any
  • OS: any
  • Browser: any
  • Plugin Version: 1.77.0
  • Device: any

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

Acceptance criteria

  • The view only dashboard should have awareness of the adsenseLinked state normally provided on load via the Analytics settings without having access to the module's settings
  • No error should be raised due to a failing request for module settings
  • When AdSense is linked with Analytics, no error should be raised on the view only dashboard when both modules are connected
    • When AdSense is not linked, it is expected that a restricted metrics error would be raised by the report request as there is no way to know proactively that the two are not linked.

Implementation Brief

In assets/js/modules/adsense/components/dashboard/DashboardTopEarningPagesWidget.js:

  • Modify the useSelect hook that returns the getAdsenseLinked selector to return only if the user is not on the view-only dashboard AND the getReport call for Analytics is done using hasFinishedResolution. In other words, return undefined early if the user is on the view-only dashboard AND the getReport call for Analytics is still loading; otherwise, return the getAdsenseLinked selector.

Test Coverage

  • No new tests are to be added.

QA Brief

According to AC the TypeError on the screenshot above should not appear in the DashBoard any more. All functionality should remain the same.

Note for the QA team: One little detail that we've discovered while code reviewing the PR for this issue is that in order to replicate the error in the console reported in this issue, besides the replication steps mentioned in the issue description, the Analytics and Adsense modules need to be shared with a non-administrator role (e.g. editor) and the view only dashboard should be accessed with that non-administrator role.

Changelog entry

  • Ensure the AdSense Linked status is correctly available in the Top Earning Pages widget when in view-only mode.
@techanvil techanvil added the Type: Bug Something isn't working label Jul 1, 2022
@techanvil techanvil changed the title Call to getAdsenseLinked selector in view-only mode results in a console error. Top Earning Pages widget cannot determine adsense linked status in view-only mode for non-admin user Jul 1, 2022
@aaemnnosttv aaemnnosttv self-assigned this Jul 1, 2022
@aaemnnosttv aaemnnosttv added P1 Medium priority Module: Analytics Google Analytics module related issues labels Jul 1, 2022
@aaemnnosttv aaemnnosttv removed their assignment Aug 31, 2022
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Sep 7, 2022
@aaemnnosttv aaemnnosttv self-assigned this Sep 16, 2022
@aaemnnosttv
Copy link
Collaborator

@hussain-t the current IB does not satisfy the AC which does not call for hiding the widget on the view only dashboard. The widget should work on the view only dashboard, the main problem here is how a view-only user wouldn't have access to the adsenseLinked value which is stored in module settings (even though it isn't really a setting). Perhaps what we should do here is make this value available somewhere else that a view-only user would have access to, and remove this from the module's settings. There isn't a clear best place for it to go that I can think of so we should brainstorm some solutions here.

@aaemnnosttv aaemnnosttv assigned hussain-t and unassigned aaemnnosttv Sep 16, 2022
@hussain-t
Copy link
Collaborator

Thanks, @aaemnnosttv. I have updated the IB to have a new REST endpoint to expose the data. Apart from that, we could create a new option for adsenseLinked instead of storing it in settings, that way, we avoid the situation where this one setting is visible to everyone whereas the rest are restricted. That could potentially be done in a follow-up issue.

@hussain-t hussain-t assigned aaemnnosttv and unassigned hussain-t Sep 21, 2022
@aaemnnosttv
Copy link
Collaborator

  • Add a new REST endpoint GET:adsense-linked, which returns the casted bool value of the googlesitekit_analytics_adsense_linked property from the Analytics options:

@hussain-t I think adding a REST endpoint to return a single boolean is overkill for this situation. Also, the googlesitekit_analytics_adsense_linked option is a legacy option that we only reference when there is no saved Analytics settings. It isn't updated like the current setting is when we detect the two are linked.

After thinking about this a bit more, I think all we have to do is delay the calling of the getAdsenseLinked selector until the Analytics getReport call is done loading in the DashboardTopEarningPagesWidget. This is where the restricted metrics are requested and requesting these will populate the adsenseLinked state after which selecting it will not trigger the fetch for Analytics settings in the resolver. The fact that we cache this error response for a short time when they are not linked should prevent this from being triggered on every request.

We can apply this change only when view-only to preserve the normal behavior for signed in users.

If that makes sense and sounds good to you, then let's do that 👍

@aaemnnosttv aaemnnosttv assigned hussain-t and unassigned aaemnnosttv Sep 21, 2022
@hussain-t
Copy link
Collaborator

Thanks, @aaemnnosttv, that makes perfect sense 👍

It could be a 3-pointer; however, I have set it to 7 to have more space for testing. We can also add the GFI label to this ticket.

@hussain-t hussain-t assigned aaemnnosttv and unassigned hussain-t Sep 22, 2022
@aaemnnosttv
Copy link
Collaborator

IB ✅

@aaemnnosttv aaemnnosttv removed their assignment Sep 22, 2022
@sashadoes sashadoes self-assigned this Sep 22, 2022
@sashadoes sashadoes removed their assignment Sep 24, 2022
@nfmohit nfmohit self-assigned this Sep 24, 2022
@nfmohit
Copy link
Collaborator

nfmohit commented Sep 24, 2022

Note

One little detail that I've discovered while code reviewing the PR for this issue is that in order to replicate the error in the console reported in this issue, the Analytics and Adsense modules need to be shared with a non-administrator role (e.g. editor) and the view only dashboard should be accessed with that non-administrator role. I've added this bit to the QA brief so that it doesn't get missed.

@nfmohit nfmohit removed their assignment Sep 24, 2022
@techanvil techanvil assigned techanvil and unassigned techanvil Sep 26, 2022
@wpdarren wpdarren assigned wpdarren and mohitwp and unassigned wpdarren Sep 26, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Sep 28, 2022

QA Update ✅

  • Verified on develop environment using tester plugin.
  • I'm able to reproduce issue on latest environment while accessing 'View only dashboard' with the user role having access to Analytics and AdSense.
  • On dev branch - I'm not getting any console errors while accessing 'View only dashboard' with the user role having access to Analytics and AdSense.
  • If AdSense not linked to Analytics, then restricted metrics error showing in console for both view only and admin dashboard, and it is same as latest environment.
  • Verified for default WP role as well as for Custom user role.

Error on latest environment

image

When AdSense not linked to Analytics -

image

Develop branch -

Error in console not showing when AdSense linked to Analytics-

image

image

When AdSense not linked to Analytics -

image

image

@mohitwp mohitwp removed their assignment Sep 28, 2022
@felixarntz felixarntz self-assigned this Oct 6, 2022
@felixarntz felixarntz removed their assignment Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Module: Analytics Google Analytics module related issues P1 Medium priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

8 participants