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

For 'View Only dashboard' – CTAs that shared users can't use should not appear #5346

Closed
eclarke1 opened this issue Jun 15, 2022 · 8 comments
Labels
Good First Issue Good first issue for new engineers P0 High priority Type: Bug Something isn't working

Comments

@eclarke1
Copy link
Collaborator

eclarke1 commented Jun 15, 2022

Bug Description

CTAs in the dashboard that are intended for admins, without any useful info or actionable links/items when viewed in a shared context, should not be shown to "shared"/view-only Dashboard users.

Instead, we should show a different CTA informing the user that the admin needs to take steps before the module will be useable. Alternatively, we could show the user nothing as there is currently nothing really to show.

Bug bash issue: https://app.asana.com/0/1202258919887896/1202419867342280 please refer to Asana issue for background

Steps to reproduce

  1. Connect AdSense module and enable AdSense module sharing in Site Kit.
  2. Ensure AdSense property is in a state that would cause a CTA to appear for the admin user
  3. Sign in as a "shared dashboard" user
  4. See the CTA intended for the admin

Screenshots

image.png


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

Acceptance criteria

  • The AdSenseLinkCTA component should not be shown in a view-only context

Implementation Brief

  • In assets/js/modules/adsense/components/dashboard/DashboardTopEarningPagesWidget.js:
  • Using the useViewOnly hook, return the WidgetNull component if the user has view-only access AND Adsense isn't linked.

Test Coverage

  • In stories/module-adsense-components.stories.js, add a story to render the AdSenseLinkCTA component if it isn't view-only context AND Adsense isn't linked.

QA Brief

  • Start off with AdSense in a state that would cause the "Link Analytics and AdSense" CTA to appear for the admin user
  • Sign in with a user with permissions to view the module
  • The "Link Analytics and AdSense" CTA should not show up for user with view only access.

Changelog entry

  • Don't show the "Link Analytics and AdSense" CTA on the view-only Dashboard.
@eclarke1 eclarke1 added P0 High priority Type: Bug Something isn't working labels Jun 15, 2022
@tofumatt tofumatt self-assigned this Jun 16, 2022
@tofumatt tofumatt changed the title For 'View Only dashboard' – CTA links for which user don't have access should display inactive For 'View Only dashboard' – CTAs that shared users can't use should not appear Jun 16, 2022
@tofumatt tofumatt assigned aaemnnosttv and unassigned tofumatt Jun 16, 2022
@tofumatt
Copy link
Collaborator

@aaemnnosttv In the Asana task I saw you mentioned that instead of hiding the links we should show an entirely different CTA as the existing one isn't very actionable/usable to a "view-only" dashboard user.

In this case I think we ought not to show anything at all, as the wording and info shown will likely be unclear/confusing to a user not familiar with AdSense setup. I think until AdSense is ready it'd be best not to show these CTAs at all rather than creating new CTAs that basically just tell the "view-only" user that their admin needs to complete some AdSense steps.

What do you think?

@aaemnnosttv
Copy link
Collaborator

In this case I think we ought not to show anything at all, as the wording and info shown will likely be unclear/confusing to a user not familiar with AdSense setup. I think until AdSense is ready it'd be best not to show these CTAs at all rather than creating new CTAs that basically just tell the "view-only" user that their admin needs to complete some AdSense steps.

@tofumatt The problem with not showing anything at all is that you may be sending a view-only user to an empty dashboard if AdSense is the only shared module, in which case we'd need a different empty state 😄 We may still need this depending on how other issues go but having a placeholder widget would probably be the simplest solution here.

Alternatively, we could maybe address this on the server side – maybe AdSense should only be considered shareable if it's in a "ready" state? This would also mean that the sharing settings would be unavailable for AdSense until it was ready too.

If we knew the account or site isn't in a "blocked" state, we could treat it as a kind of gathering data state e.g. if AdSense is pending approval, etc. I'm not sure how we would handle things if the site wasn't approved though 🤔

Thoughts @felixarntz ?

@felixarntz
Copy link
Member

@aaemnnosttv @tofumatt We need to differentiate here:

  • We shouldn't show anything at all to view-only users when it comes to CTAs. The "Link Analytics and AdSense" one is a CTA, so this widget shouldn't appear at all in view-only mode.
  • However, we should still show zero data states to view-only users. That "No ad impressions yet" box indeed looks like a CTA, but it is something custom that we basically override a regular zero data state with, just in the case of AdSense.

In other words, can we avoid showing the "No ad impressions yet" box for view-only users and instead show a regular zero data state on the main AdSense widget? FWIW, this just makes me think that we should probably change this anyway in light of the adsenseSetupV2 where that CTA doesn't really make sense anymore. I'll create a separate issue for that.

Anyway, I think we need to keep some version of what is technically a zero data state showing for view-only users, but not the "Link Analytics and AdSense" one.

Alternatively, we could maybe address this on the server side – maybe AdSense should only be considered shareable if it's in a "ready" state? This would also mean that the sharing settings would be unavailable for AdSense until it was ready too.

What do you mean by "ready" state? Modules can't be shared unless they have their setup completed, no? Again, this CTA is a relic of the AdSense V1 limitations where we didn't really know from the Site Kit side whether the AdSense setup is really ready or not. That is different now with V2 though.

@aaemnnosttv
Copy link
Collaborator

What do you mean by "ready" state? Modules can't be shared unless they have their setup completed, no? Again, this CTA is a relic of the AdSense V1 limitations where we didn't really know from the Site Kit side whether the AdSense setup is really ready or not. That is different now with V2 though.

@felixarntz gotcha, I thought we had previously stored the account/site status before as well. You're saying with the v2 setup, AdSense will never be connected until the account + site statuses are in a ready state?

@aaemnnosttv
Copy link
Collaborator

aaemnnosttv commented Jun 21, 2022

@felixarntz the problem with showing the existing DashboardZeroData component is more about the site steps that we can't show for view-only users. These use deep links into the service that we can't build (due to lack of settings) but also because we shouldn't expect view-only users to be able to action them just like with other CTAs. I think we can simply make this part of the component conditional.

Edit: nevermind, I missed the part of your comment about doing away with this custom zero data state 😄

@aaemnnosttv aaemnnosttv removed their assignment Jun 21, 2022
@aaemnnosttv aaemnnosttv added the Good First Issue Good first issue for new engineers label Jun 21, 2022
@hussain-t hussain-t assigned hussain-t and unassigned hussain-t Jun 22, 2022
@tofumatt tofumatt self-assigned this Jun 22, 2022
@tofumatt
Copy link
Collaborator

The IB here will show a zero/gathering data state for users instead of the "Link AdSense and Analytics" CTA. Given the discussion above, I think it would be better to show nothing for AdSense + Analytics not being linked when a view-only user views AdSense widgets.

I think rendering nothing instead of the output here:

<Widget noPadding Footer={ Footer }>
<TableOverflowContainer>
<ReportTable
rows={ data?.[ 0 ]?.data?.rows || [] }
columns={ tableColumns }
zeroState={ ZeroDataMessage }
gatheringData={ isGatheringData }
/>
</TableOverflowContainer>
</Widget>
would be better in a view-only context if AdSense and Analytics aren't linked…

Otherwise the above render will likely happen with the IB suggested, and that means view-only users will ALWAYS see this widget in a zero/gathering data state even though we'll never gather data for it since it isn't setup.

Let's hide it entirely instead on a view-only dashboard.

@tofumatt tofumatt assigned hussain-t and unassigned tofumatt Jun 22, 2022
@hussain-t hussain-t assigned tofumatt and unassigned hussain-t Jun 23, 2022
@tofumatt
Copy link
Collaborator

IB ✅

@tofumatt tofumatt removed their assignment Jun 23, 2022
@maciejost maciejost self-assigned this Jun 24, 2022
@maciejost maciejost removed their assignment Jun 28, 2022
@techanvil techanvil assigned techanvil and unassigned techanvil Jun 28, 2022
@mohitwp mohitwp self-assigned this Jun 28, 2022
@mohitwp
Copy link
Collaborator

mohitwp commented Jun 28, 2022

QA update ✅

Verified

  • Verified for admin1.
  • Verified for editor with view access.
  • Verified for admin with view access.
  • Verified for admin2.
  • The "Link Analytics and AdSense" CTA is not showing for user with view only access.

admin 1

image

Editor with view access

image

Admin2 with view access

image

Admin 2 (signed in)

image

@mohitwp mohitwp removed their assignment Jun 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Good First Issue Good first issue for new engineers P0 High priority Type: Bug Something isn't working
Projects
None yet
Development

No branches or pull requests

8 participants