KM metric tile report errors on view-only dashboard about site-kit-wp HOT 17 OPEN

IB ✅

@jimmymadon – I removed the part about the CTA from the test coverage section since that has been moved to #7386

from site-kit-wp.

Thanks @hussain-t for these fixes.

I'll leave points 1 and 2 for a Code / Merge reviewer but that SGTM!

3. "Top performing keywords" and "Most popular products by pageviews" (others may also be affected) widgets' links are non-clickable in the view-only dashboard; these links appear as though they're clickable. This could mislead users. For a better user experience, could we implement non-interactive text for view-only users with the MetricTileTablePlainText component? This could be addressed in a separate issue.

Yes - this is definitely not a bug bash blocking issue and can be dealt separately as a know issue. I can create one later today and test this once the PR has been merged. Thanks again. Nice work!

from site-kit-wp.

• The Key Metrics Setup Banner CTA should not be displayed for non-authenticated admins (i.e. on the View Only dashboard. The Banner should never be shown regardless if the KM widget has been set up or not.

IIRC I thought view-only users would be able to pick their own metrics, this is still the case, correct? Is it only the setup CTA that we don't want to show here?

@aaemnnosttv Correct - the AC of #6259 states: "When the Key Metrics widget is active for any user (authenticated admin or view-only user), a "Change metrics" link should be created as show in the Figma mock."

from site-kit-wp.

Thanks @10upsimon ! I'm going to amend this quickly to unblock it in the interest of time, but basically we should apply the author-related changes to the response in the controller rather than in the get_answers method. Otherwise this should be good to go 👍

from site-kit-wp.

@hussain-t We discussed this on stand-up today and decided it's best to take care of this in your PR here as opposed to in a separate issue. I've closed #7421 (which @jimmymadon had created for this) so you can update your PR to address those non-clickable links. Thanks!

from site-kit-wp.

@hussain-t We discussed this on stand-up today and decided it's best to take care of this in your PR here as opposed to in a separate issue. I've closed #7421 (which @jimmymadon had created for this) so you can update your PR to address those non-clickable links. Thanks!

@bethanylang, I've fixed it as part of this issue in my PR.

from site-kit-wp.

• The Key Metrics Setup Banner CTA should not be displayed for non-authenticated admins (i.e. on the View Only dashboard. The Banner should never be shown regardless if the KM widget has been set up or not.

IIRC I thought view-only users would be able to pick their own metrics, this is still the case, correct? Is it only the setup CTA that we don't want to show here?

from site-kit-wp.

ACs are good, moving to IB 👍🏻

from site-kit-wp.

@aaemnnosttv This issue does not clearly specify what should happen if the non-authenticated user does not have shared access to modules whose metric tiles are displayed. We haven't explicitly discussed or concluded if we should "hide" individual widget tiles for a non-authenticated user without module access.

The design doc does mention that we should not show the entire KMW if more than 2 metric tiles do not have their modules shared. Since all but one tile currently relies on GA4, this would mean that if GA4 is not shared, then the KMW would be hidden.

I'm adding another AC point here to clarify.

from site-kit-wp.

There are a few more caveats to think about for when the view-only user does not have access. So might be worth discussing this in a separate issue which is definitely not "bug-bash blocking".

The design doc does mention that we should not show the entire KMW if more than 2 metric tiles do not have their modules shared.

This will create yet another "special case" for our widgets API which we can avoid if we decide to follow our current widget pattern and show the Widget area if at least one widget is available.

The final edge case here is if the 4 selected metrics are all from modules that are not shared, but there are other metric tiles in the list whose modules are shared, then having the widget hidden would mean that the user won't have access to the "Change metrics" link which would allow them to choose different metrics.

So we should re-think what we show to users when their metric tiles rely on modules they don't have access to.

from site-kit-wp.

Flagging that as per this Slack thread, the AC is main focus here and was changed once the issue already had the title and description.

from site-kit-wp.

Thanks @10upsimon – this looks great for the most part.

The one thing I think is missing is around the change to the permissions for the user input endpoint. It would be useful to know why this necessary to change, but if needed we should modify the response from that endpoint based on the user's capabilities as we don't expose the user information of other users unless the current user has the list_users capability. See REST_Modules_Controller::prepare_module_data_for_response

So in making this endpoint available to non-admins, we should apply the same.

from site-kit-wp.

@aaemnnosttv I've updated the IB to include:

• A brief sentence in the opening section of the IB about why the READABLE route callback permissions need updating.
• Renamed the second section within the IB to be called Update REST endpoint permissions & filtering of author key from response and added a set of steps that need to be taken in order to filter out the 'author' key from the response object.

from site-kit-wp.

@jimmymadon @aaemnnosttv, I've identified a few concerns while working on this issue:

1. GA4 setting endpoint permission error on view-only dashboard, specifically within the PopularProductsWidget component. The error emerges when the getServiceReportURL selector is utilized. - Resolved ✅

2. In the PopularProductsWidget component, links weren't opening. The problem occurred from the columns array directly returning the value from useSelect. Instead, it should have been used within the component. Feel free to check this commit for more details. Resolved ✅

3. "Top performing keywords" and "Most popular products by pageviews" (others may also be affected) widgets' links are non-clickable in the view-only dashboard; these links appear as though they're clickable. This could mislead users. For a better user experience, could we implement non-interactive text for view-only users with the MetricTileTablePlainText component? This could be addressed in a separate issue.

from site-kit-wp.