Skip to content

Commit

Permalink
Delay PairedBrowsing so its registration can be conditional on user a…
Browse files Browse the repository at this point in the history 
…uth state
  • Loading branch information
@westonruter
westonruter committed Aug 10, 2021
1 parent 4148e13 commit 35bed57
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 46 deletions.
28 changes: 19 additions & 9 deletions src/Admin/PairedBrowsing.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
use AMP_Validation_Manager;
use AMP_Validated_URL_Post_Type;
use AmpProject\AmpWP\Infrastructure\Conditional;
use AmpProject\AmpWP\Infrastructure\Delayed;
use AmpProject\AmpWP\Infrastructure\HasRequirements;
use AmpProject\AmpWP\Infrastructure\Registerable;
use AmpProject\AmpWP\Infrastructure\Service;
Expand All @@ -30,7 +31,7 @@
* @since 2.1
* @internal
*/
final class PairedBrowsing implements Service, Registerable, Conditional, HasRequirements {
final class PairedBrowsing implements Service, Registerable, Conditional, Delayed, HasRequirements {

/**
* Query var for requests to open the app.
Expand All @@ -53,6 +54,18 @@ final class PairedBrowsing implements Service, Registerable, Conditional, HasReq
*/
public $paired_routing;

/**
* Get the action to use for registering the service.
*
* This action needs to run late enough in the frontend and the backend for the user to be logged-in and for
* AMP dev mode to be opted-in to.
*
* @return string Registration action to use.
*/
public static function get_registration_action() {
return 'wp_loaded';
}

/**
* Check whether the conditional object is currently needed.
*
Expand All @@ -71,6 +84,10 @@ public static function is_needed() {
get_stylesheet() === AMP_Options_Manager::get_option( Option::READER_THEME )
)
)
&&
amp_is_dev_mode()
&&
is_user_logged_in()
);
}

Expand Down Expand Up @@ -136,7 +153,7 @@ public function filter_validated_url_status_actions( $actions, WP_Post $post ) {
* Initialize frontend.
*/
public function init_frontend() {
if ( ! amp_is_available() || ! amp_is_dev_mode() || ! is_user_logged_in() ) {
if ( ! amp_is_available() ) {
return;
}

Expand Down Expand Up @@ -291,13 +308,6 @@ public function ensure_app_location() {
* @return string Custom template if in paired browsing mode, else the supplied template.
*/
public function filter_template_include_for_app() {
if ( ! amp_is_dev_mode() ) {
wp_die(
esc_html__( 'Paired browsing is only available when AMP dev mode is enabled (e.g. when logged-in and admin bar is showing).', 'amp' ),
esc_html__( 'AMP Paired Browsing Unavailable', 'amp' ),
[ 'response' => 403 ]
);
}

/** This action is documented in includes/class-amp-theme-support.php */
do_action( 'amp_register_polyfills' );
Expand Down
31 changes: 22 additions & 9 deletions src/MobileRedirection.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ public function is_mobile_request() {
* @return bool True if mobile redirection should be done, false otherwise.
*/
public function is_using_client_side_redirection() {
if ( is_customize_preview() || ( amp_is_dev_mode() && is_user_logged_in() ) ) {
if ( is_customize_preview() || Services::has( 'admin.paired_browsing' ) ) {
return true;
}

Expand Down Expand Up @@ -591,19 +591,32 @@ public function add_mobile_version_switcher_link() {
</a>
</div>

<?php if ( amp_is_dev_mode() && is_user_logged_in() && ( ! is_customize_preview() || AMP_Theme_Support::READER_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT ) ) ) : ?>
<?php
// Note that the switcher link is disabled in Reader mode because there is a separate toggle to switch versions.
<?php
// Note that the switcher link is disabled in Reader mode because there is a separate toggle to switch versions,
// and because there are controls which are AMP-specific which don't apply when switching between versions.
$is_amp_reader_customizer = (
is_customize_preview()
&&
AMP_Theme_Support::READER_MODE_SLUG === AMP_Options_Manager::get_option( Option::THEME_SUPPORT )
);

$is_possibly_paired_browsing = (
Services::has( 'admin.paired_browsing' )
&&
! is_customize_preview()
);

if ( $is_amp_reader_customizer || $is_possibly_paired_browsing ) :
$exports = [
'containerId' => $container_id,
'isCustomizePreview' => is_customize_preview(),
'notApplicableMessage' => __( 'This link is not applicable in this context. It remains here for preview purposes only.', 'amp' ),
'containerId' => $container_id,
'isReaderCustomizePreview' => $is_amp_reader_customizer,
'notApplicableMessage' => __( 'This link is not applicable in this context. It remains here for preview purposes only.', 'amp' ),
];
?>
<script data-ampdevmode>
(function( { containerId, isCustomizePreview, notApplicableMessage } ) {
(function( { containerId, isReaderCustomizePreview, notApplicableMessage } ) {
addEventListener( 'DOMContentLoaded', () => {
if ( isCustomizePreview || [ 'paired-browsing-non-amp', 'paired-browsing-amp' ].includes( window.name ) ) {
if ( isReaderCustomizePreview || [ 'paired-browsing-non-amp', 'paired-browsing-amp' ].includes( window.name ) ) {
const link = document.querySelector( `#${containerId} a[href]` );
link.style.cursor = 'not-allowed';
link.addEventListener( 'click', ( event ) => {
Expand Down
45 changes: 17 additions & 28 deletions tests/php/src/Admin/PairedBrowsingTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
use AMP_Theme_Support;
use AmpProject\AmpWP\Admin\PairedBrowsing;
use AmpProject\AmpWP\Infrastructure\Conditional;
use AmpProject\AmpWP\Infrastructure\Delayed;
use AmpProject\AmpWP\Infrastructure\HasRequirements;
use AmpProject\AmpWP\Infrastructure\Registerable;
use AmpProject\AmpWP\Infrastructure\Service;
Expand All @@ -20,7 +21,6 @@
use AmpProject\AmpWP\Tests\DependencyInjectedTestCase;
use AmpProject\AmpWP\Tests\Helpers\AssertContainsCompatibility;
use AmpProject\DevMode;
use WPDieException;
use WP_Admin_Bar;

/** @coversDefaultClass \AmpProject\AmpWP\Admin\PairedBrowsing */
Expand All @@ -44,12 +44,21 @@ public function test_is_needed() {
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::READER_MODE_SLUG );
$this->assertFalse( PairedBrowsing::is_needed() );

$admin_user_id = self::factory()->user->create( [ 'role' => 'administrator' ] );
wp_set_current_user( $admin_user_id );
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );
$this->assertSame( Services::get( 'dependency_support' )->has_support(), PairedBrowsing::is_needed() );
wp_set_current_user( 0 );
$this->assertFalse( PairedBrowsing::is_needed() );
wp_set_current_user( $admin_user_id );
add_filter( 'amp_dev_mode_enabled', '__return_false' );
$this->assertFalse( PairedBrowsing::is_needed() );
remove_filter( 'amp_dev_mode_enabled', '__return_false' );
$this->assertTrue( PairedBrowsing::is_needed() );

// Case where Reader theme is set to be the same as the active theme.
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::READER_MODE_SLUG );
AMP_Options_Manager::update_option( Option::READER_THEME, get_stylesheet() );

$this->assertSame( Services::get( 'dependency_support' )->has_support(), PairedBrowsing::is_needed() );
}

Expand All @@ -58,12 +67,18 @@ public function test_get_requirements() {
$this->assertSame( [ 'dependency_support' ], PairedBrowsing::get_requirements() );
}

/** @covers ::get_registration_action() */
public function test_get_registration_action() {
$this->assertSame( 'wp_loaded', PairedBrowsing::get_registration_action() );
}

/** @covers ::__construct() */
public function test__construct() {
$this->assertInstanceOf( PairedBrowsing::class, $this->instance );
$this->assertInstanceOf( Service::class, $this->instance );
$this->assertInstanceOf( Registerable::class, $this->instance );
$this->assertInstanceOf( Conditional::class, $this->instance );
$this->assertInstanceOf( Delayed::class, $this->instance );
$this->assertInstanceOf( HasRequirements::class, $this->instance );
}

Expand Down Expand Up @@ -104,26 +119,10 @@ public function test_init_frontend_short_circuited() {

// Check first short-circuit condition.
add_filter( 'amp_skip_post', '__return_true' );
add_filter( 'amp_dev_mode_enabled', '__return_true' );
$this->assertFalse( amp_is_available() );
$this->assertTrue( amp_is_dev_mode() );
$this->instance->init_frontend();
$assert_short_circuited();
remove_all_filters( 'amp_skip_post' );
remove_all_filters( 'amp_dev_mode_enabled' );

// Check second short-circuit condition.
add_filter( 'amp_skip_post', '__return_false' );
add_filter( 'amp_dev_mode_enabled', '__return_false' );
$this->assertTrue( amp_is_available() );
$this->assertFalse( amp_is_dev_mode() );
$this->instance->init_frontend();
$assert_short_circuited();
remove_all_filters( 'amp_skip_post' );
remove_all_filters( 'amp_dev_mode_enabled' );

// Check condition for
$this->assertTrue( amp_is_available() );
}

/**
Expand All @@ -137,7 +136,6 @@ public function test_init_frontend_app() {
$this->go_to( add_query_arg( PairedBrowsing::APP_QUERY_VAR, '1', get_permalink( $post ) ) );

add_filter( 'amp_skip_post', '__return_false' );
add_filter( 'amp_dev_mode_enabled', '__return_true' );
$this->instance->init_frontend();

// Check that init_app() was called.
Expand All @@ -159,7 +157,6 @@ public function test_init_frontend_client() {
AMP_Options_Manager::update_option( Option::THEME_SUPPORT, AMP_Theme_Support::TRANSITIONAL_MODE_SLUG );

add_filter( 'amp_skip_post', '__return_false' );
add_filter( 'amp_dev_mode_enabled', '__return_true' );
$this->go_to( $this->instance->paired_routing->add_endpoint( get_permalink( $post ) ) );
$this->assertTrue( amp_is_request() );
$this->instance->init_frontend();
Expand Down Expand Up @@ -233,16 +230,8 @@ function () use ( &$redirected ) {
$this->assertTrue( $redirected );
}

/** @covers ::filter_template_include_for_app() */
public function test_filter_template_include_for_app_when_no_dev_mode() {
add_filter( 'amp_dev_mode_enabled', '__return_false' );
$this->setExpectedException( WPDieException::class, 'Paired browsing is only available when AMP dev mode is enabled (e.g. when logged-in and admin bar is showing).' );
$this->instance->filter_template_include_for_app();
}

/** @covers ::filter_template_include_for_app() */
public function test_filter_template_include_for_app_when_allowed() {
add_filter( 'amp_dev_mode_enabled', '__return_true' );
$this->assertEquals( 0, did_action( 'amp_register_polyfills' ) );

$include_path = $this->instance->filter_template_include_for_app();
Expand Down

0 comments on commit 35bed57

Please sign in to comment.