Enhance mechanism for dashboard sharing request context

[aaemnnosttv]

Feature Description

In #5711 we added a new is_using_shared_credentials property to the Module class for storing state to enable logic that should be conditionally applied for a request that is made on behalf of a module owner rather than the current user via dashboard sharing.

This approach works but should ideally be a bit more reliable to avoid logic being potentially applied improperly due to this property getting out of sync. See #5711 (comment)

---

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

Acceptance criteria

• Sharing request context (i.e. whether a given request will use shared credentials or not) and behavior based on it should be determined using the Data_Request and not stored in a way which is not specific to the request
  • Said another way, any behavior related to whether a request is made as a shared one or not should be derived from the current user together with the Data_Request
• The Data_Request class should not be changed

Implementation Brief

• Extract the logic in Module::execute_data_request that is used to determine if a request should use shared credentials into a new protected method is_shared_data_request( Data_Request ) : bool (perhaps final as well)
  • Refactor execute_data_request to use the new method (this introduces some duplicate calls but that should be acceptable – if that's a concern, we could consider caching the result by the given Data_Request. Ideally we could use a Weak_Map for this but that doesn't exist until PHP 8, so we'd need to use something like SplObjectStorage)
• Rework usage of Module::$is_using_shared_credentials to use the new Module::is_shared_data_request method instead
  • The only usage of this property is in create_data_request -> validate_report_metrics | validate_report_dimensions for Analytics(_4) and AdSense modules
  • This can be reworked by renaming validate_report_* to validate_shared_report_* (which is more accurate to what it does as well) and conditionally calling that if is_shared_data_request from the calling create_data_request where the current Data_Request is in scope
• Remove Module::$is_using_shared_credentials property

Test Coverage

• Add more robust PHPUnit coverage of data requests that covers shared requests
  • Current module owner with/without sharing with their role
  • Non-module owner with/without sharing with their role

QA Brief

• Perform a regression test of the Dashboard Sharing feature for shared AdSense and Analytics widgets, verifying they still display as expected with shared data.
• On the Shared Dashboard, check that the AdSense and Analytics report metrics and dimensions are still validated correctly.
  • Make some requests with invalid AdSense report metrics and dimensions and verify that they return a 400 error with an error message in the format Unsupported (metric|dimension)/s requested: SOME_(METRIC|DIMENSION).
  • In order to make the requests, invoke as follows in the JS Console.
    • Note the valid metrics are: ESTIMATED_EARNINGS, IMPRESSIONS, PAGE_VIEWS_CTR, PAGE_VIEWS_RPM
    • And the only valid dimension is: DATE

googlesitekit.api.get("modules", "adsense", "report", {
  startDate: "2022-07-26",
  endDate: "2022-08-22",
  metrics: ["FOO", "BAR"],
  dimensions: ["BAZ"],
});

• Perform similar checks for Analytics.
  • Note the valid metrics are:
    • ga:sessions
    • ga:users
    • ga:pageviews
    • ga:uniquePageviews
    • ga:bounceRate
    • ga:avgSessionDuration
    • ga:adsenseRevenue
    • ga:adsenseECPM
    • ga:adsensePageImpressions
    • ga:goalCompletionsAll
  • And the valid dimensions:
    • ga:date
    • ga:pagePath
    • ga:pageTitle
    • ga:channelGrouping
    • ga:country
    • ga:deviceCategory
    • ga:hostname

googlesitekit.api.get("modules", "analytics", "report", {
  startDate: "2022-07-26",
  endDate: "2022-08-22",
  metrics: ["FOO", "BAR"],
  dimensions: ["BAZ"],
});

• Note that the Analytics 4 report endpoint can't currently be QA'd in the same manner, as sharing is not working correctly for it. This will be remedied in a future issue, Ensure GA4 widgets can display data on the Shared Dashboard. #6687.

Changelog entry

• Enhance mechanism for dashboard sharing request context.

[FlicHollis]

@aaemnnosttv if you are not actively working on this, can this be unassigned so another team member can pick this up? Thanks!

[mxbclang]

@aaemnnosttv Following up on this one – can this be unassigned so another team member can pick up, or do you want to move it forward? Thanks!

[aaemnnosttv]

Updated with AC + IB. @techanvil @eugene-manuilov I'd be interested to get your thoughts on this one. Once this is approved, this is one I'd like to work on.

[eugene-manuilov]

@aaemnnosttv IB looks good to me. I'll let Tom to take a look as well.

[techanvil]

@aaemnnosttv, LGTM to me too. The only change I would consider is possibly making is_shared_data_request a property/accessor on Data_Request itself, seeing as the value is relevant to that specific instance. This would solve the caching problem, and allow Data_Request to be passed to an external utility if necessary including that piece of information.

The logic to set it would still need to reside in Module, though, so might look a little unwieldy.

Please see what you think - if you'd prefer to keep as-is, then consider the IB approved from me too.

[aaemnnosttv]

The only change I would consider is possibly making is_shared_data_request a property/accessor on Data_Request itself, seeing as the value is relevant to that specific instance. This would solve the caching problem, and allow Data_Request to be passed to an external utility if necessary including that piece of information.

@techanvil that's something I had considered as well, although I don't think this state fits well into Data_Request as its intended to be immutable and represents the request itself whereas the "is shareable" state is entirely contingent on internal conditions involving the current user, sharing settings, etc.

The ability to pass the object off elsewhere isn't a real need yet but we can always modify things later if it did. It would be nice to have it available on the object itself, but it isn't too different to get the same data using the object as the key for getting it especially as the Module class would need to be involved there anyways.

Even the performance concern should be more/less a non-issue since these requests are always async and only handle one at a time.

I'll move this forward for implementation now – thanks both for your thoughts. Feel free to raise anything else if you think of it 👍

[mohitwp]

QA Update ✅

• Tested on dev.
• Done regression testing of Dashboard sharing.
• Verified AdSense and Analytics shared widget on View only dashboard.
• Verified AdSense and Analytics shared widget For another admin.
• Ran code in console and validated analytics and AdSense metrics and dimensions,
• Tested different combinations of valid and invalid metrics and dimensions.

Valid metrics and invalid dimensions-

Invalid metrics and valid dimensions-

Analytics

Valid metrics and dimensions-

Invalid metrics and valid dimensions-

Valid metrics and invalid dimension-