When clicking on any AMP link in the "Most Popular Content" tab it opens a canonical "Detailed Page Stats" - which excludes AMP traffic

[jamesozzie]

Bug Description

In a users "Most Popular Content" table both AMP and non AMP URLs may be listed. When clicking on an AMP URL to see further stats via the "Detailed Page Stats" only the canonical stats appear, with the resulting data excluding AMP traffic.

Any fix would also apply to using the admin toolbar, to display AMP URL specific insights.

Steps to reproduce

1. Setup Site Kit on a new site with paired AMP (via the AMP plugins transitional or reader mode, preferably transitional mode in case your homepage shows a list of posts)
2. Connect the Analytics module and ensure logged in users are counted in reporting
3. Visit both your AMP homepage and your non AMP homepage 5 times each
4. Both the AMP version of your homepage and canonical AMP version appear in the "Most popular content"
5. When clicking on the AMP version it will show the canonical version data

Screenshots

Additional Context

• Video demonstrating issue
• SK 1.29.0
• When clicking on the SK link to check further insights at Analytics traffic is broken up between AMP and non AMP (see below)

---

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

Acceptance criteria

• Secondary AMP URLs should be properly identified as a distinct entity and viewable in the single URL view.
• The Google\Site_Kit\Core\Util\Entity class should be enhanced:
  • It should receive a new mode string class property that can be set in the constructor with the other arguments. This property should by default be empty and allow to specify an identifier for a certain "sub-variant" of an entity URL (this could be for example the secondary AMP version of the entity, or the embed version of the entity).
  • A public get_mode method should also be present.
• The Google\Site_Kit\Core\Util\Entity_Factory class should be enhanced:
  • The from_context and from_url methods should receive secondary AMP URL support, as follows:
    • If the AMP plugin is active and the current URL (in from_context) / the passed URL (in from_url) includes a non-empty ?amp query parameter or if its untrailingslashed path ends in /amp, the URL should be considered a URL for a secondary AMP version.
    • In that case, the resulting entity should have its mode property set to amp_secondary, and the entity's URL should also include the respective suffix found in the original URL (i.e. if there's an amp query parameter, have that in the entity URL, or if there's an /amp path suffix, have that in the entity URL). It has to be ensured the latter respects the way trailing slashes are configured on the site (i.e. if the regular URL ends in a trailing slash, add an amp/ suffix; if it doesn't end in a trailing slash, add an /amp suffix).

Implementation Brief

• In the Google\Site_Kit\Core\Util\Entity class:
  • Add a new private property mode.
  • Add a new get_mode method that returns the mode property.
  • Add mode option to the $args parameters in the constructor and assign it to the mode property as we do it for type and title properties.
• In the Google\Site_Kit\Core\Util\Entity_Factory class:
  • Update the from_url method:
    • Assign the result of the self::from_wp_query function call to the $entity variable instead of returning it immediately.
    • Check whether the AMP plugins is active using the defined( 'AMP__VERSION' ) function. If the AMP plugin is not active, return the $entity variable immediately.
    • Otherwise check whether the incoming $url ends with /amp or has the non-empty amp query parameter. If not, return the $entity variable immediately.
    • Otherwise clone the entity (see an example below) setting amp_secondary as a new mode value and adding corresponding amp query parameter or suffix to the entity URL (see the last point in AC for details):

      $new_url = $entity->get_url();
      $new_url = $new_url . '...' ; // add ?amp=... query parameter or /amp suffix
      
      $new_entity = new Entity( $new_url, array(
          'id'    => $entity->get_id(),
          'type'  => $entity->get_type(),
          'title' => $entity->get_title(),
          'mode'  => 'amp_secondary',
      ) );
      
      return $new_entity;

    • Return the cloned entity.
  • The similar thing we need to do in the from_context method:
    • Restructure the logic to assign an entity to a variable instead of returning it immediately.
    • Return NULL if the entity is empty
    • Then check whether the current URL (using $_SERVER or $_GET) ends with /amp or has the non-empty amp query parameter. If not, return the $entity variable immediately.
    • Otherwise clone the entity as it is described for the from_url method and return the cloned entity.

Test Coverage

• N/A

Visual Regression Changes

• N/A

QA Brief

• Detailed Page Stats of a AMP url should show the stats for AMP version of the page instead of showing stats of canonical url.

Changelog entry

• Update the Detailed Page Stats page to display AMP traffic for an AMP version of a page.

[jamesozzie]

@felixarntz Assigning as discuss earlier.

[eugene-manuilov]

@felixarntz how can we get the current URL in the from_context method? Do you mean to use the permaLink parameter from the current URL query args? In this case, the from_context method will not be used because the Context class will use the from_url method instead.

[eclarke1]

@felixarntz @aaemnnosttv could either of you chime in here to help get this one pushed through to EB please? Thank you.

[felixarntz]

@eugene-manuilov

how can we get the current URL in the from_context method? Do you mean to use the permaLink parameter from the current URL query args? In this case, the from_context method will not be used because the Context class will use the from_url method instead.

The from_context method needs to look at the current URL itself, i.e. $_SERVER or $_GET vars etc like WordPress core does for its routing. This is relevant to support the frontend where that is the only way. For example when you look at an AMP page in the frontend, the stats that the Site Kit admin bar shows need to be for that AMP version, and this information is determined via from_context.

Regarding the IB: You need to make one addition related to the above: A similar change you're already describing for from_url needs to happen for from_context too, but here you need to look at the current request ($_SERVER or $_GET) instead of a provided URL string.

[eugene-manuilov]

Thanks, @felixarntz. IB is updated.

[felixarntz]

IB ✅

[cole10up]

QA ✅

Confirmed with @jamesozzie that this is functioning properly.

See recording here: https://recordit.co/JJ7Aqqjrsh

Sending to approval