-
Notifications
You must be signed in to change notification settings - Fork 100
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
Add UI to select an offline page #28
Changes from 1 commit
314c472
596e44c
225ba95
53f6533
8d7aa1c
7d7da59
c683eaf
e76b7f1
83c5604
98c6f82
7e6574b
c403da5
6a7a720
cc997ef
33c461c
c7c96b4
c48b36c
05b655b
7104c53
cc40e00
846ce07
11b49fc
40f8737
7ef0dea
eddeb84
d1dbaf9
f8c3137
ab4cbc6
03e78ba
9669026
af9aedd
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,170 @@ | ||
<?php | ||
/** | ||
* Tests for class WP_Offline_Page. | ||
* | ||
* @package PWA | ||
*/ | ||
|
||
/** | ||
* Tests for class WP_Offline_Page. | ||
*/ | ||
class Test_WP_Offline_Page extends WP_UnitTestCase { | ||
|
||
/** | ||
* Tested instance. | ||
* | ||
* @var WP_Offline_Page | ||
*/ | ||
public $instance; | ||
|
||
/** | ||
* Setup. | ||
* | ||
* @inheritdoc | ||
*/ | ||
public function setUp() { | ||
parent::setUp(); | ||
$this->instance = new WP_Offline_Page(); | ||
} | ||
|
||
/** | ||
* Test init. | ||
* | ||
* @covers WP_Offline_Page::init() | ||
*/ | ||
public function test_init() { | ||
$this->instance->init(); | ||
$this->assertEquals( 10, has_action( 'admin_init', array( $this->instance, 'register_setting' ) ) ); | ||
$this->assertEquals( 10, has_action( 'admin_init', array( $this->instance, 'settings_field' ) ) ); | ||
} | ||
|
||
/** | ||
* Test register_setting. | ||
* | ||
* @covers WP_Offline_Page::register_setting() | ||
*/ | ||
public function test_register_setting() { | ||
global $new_whitelist_options, $wp_registered_settings; | ||
|
||
$this->instance->register_setting(); | ||
$this->assertTrue( in_array( WP_Offline_Page::OPTION_NAME, $new_whitelist_options['reading'], true ) ); | ||
$this->assertEquals( | ||
array( | ||
'type' => 'integer', | ||
'group' => WP_Offline_Page::OPTION_GROUP, | ||
'description' => '', | ||
'sanitize_callback' => array( $this->instance, 'sanitize_callback' ), | ||
'show_in_rest' => false, | ||
), | ||
$wp_registered_settings[ WP_Offline_Page::OPTION_NAME ] | ||
); | ||
} | ||
|
||
/** | ||
* Test sanitize_callback. | ||
* | ||
* @covers WP_Offline_Page::sanitize_callback() | ||
*/ | ||
public function test_sanitize_callback() { | ||
global $wp_settings_errors; | ||
|
||
$settings_error = array( | ||
'setting' => WP_Offline_Page::OPTION_NAME, | ||
'code' => WP_Offline_Page::OPTION_NAME, | ||
'type' => 'error', | ||
); | ||
|
||
// The option isn't a post, so this should add a settings error. | ||
$this->assertEquals( null, $this->instance->sanitize_callback( true ) ); | ||
$this->assertEquals( | ||
array_merge( | ||
$settings_error, | ||
array( | ||
'message' => 'The current offline page does not exist. Please select or create one.', | ||
) | ||
), | ||
reset( $wp_settings_errors ) | ||
); | ||
$wp_settings_errors = array(); // WPCS: global override OK. | ||
|
||
// The option isn't a post, so this should add a settings error. | ||
$trashed_post_id = $this->factory()->post->create( array( 'post_status' => 'trash' ) ); | ||
$this->assertEquals( null, $this->instance->sanitize_callback( $trashed_post_id ) ); | ||
$this->assertEquals( | ||
array_merge( | ||
$settings_error, | ||
array( | ||
'message' => 'The current offline page is in the trash. Please select or create one.', | ||
) | ||
), | ||
reset( $wp_settings_errors ) | ||
); | ||
$wp_settings_errors = array(); // WPCS: global override OK. | ||
|
||
// The argument passed to the sanitize_callback() is a valid page ID, so it should return it. | ||
$valid_post_id = $this->factory()->post->create( array( 'post_type' => 'page' ) ); | ||
$this->assertEquals( $valid_post_id, $this->instance->sanitize_callback( $valid_post_id ) ); | ||
$this->assertEquals( array(), $wp_settings_errors ); | ||
} | ||
|
||
/** | ||
* Test settings_field. | ||
* | ||
* @covers WP_Offline_Page::settings_field() | ||
*/ | ||
public function test_settings_field() { | ||
global $wp_settings_fields; | ||
|
||
$this->instance->settings_field(); | ||
$this->assertEquals( | ||
array( | ||
'id' => WP_Offline_Page::SETTING_ID, | ||
'title' => 'Progressive Web App Offline Page', | ||
'callback' => array( $this->instance, 'settings_callback' ), | ||
'args' => array(), | ||
), | ||
$wp_settings_fields[ WP_Offline_Page::OPTION_GROUP ]['default'][ WP_Offline_Page::SETTING_ID ] | ||
); | ||
} | ||
|
||
/** | ||
* Test settings_callback. | ||
* | ||
* @covers WP_Offline_Page::settings_callback() | ||
*/ | ||
public function test_settings_callback() { | ||
$number_pages = 10; | ||
$page_ids = array(); | ||
for ( $i = 0; $i < $number_pages; $i++ ) { | ||
$page_ids[] = $this->factory()->post->create( array( 'post_type' => 'page' ) ); | ||
} | ||
ob_start(); | ||
$this->instance->settings_callback(); | ||
$output = ob_get_clean(); | ||
$this->assertContains( 'Select an existing page:', $output ); | ||
|
||
// All of the pages should appear in the <select> element from wp_dropdown_pages(). | ||
foreach ( $page_ids as $page_id ) { | ||
$this->assertContains( strval( $page_id ), $output ); | ||
} | ||
} | ||
|
||
/** | ||
* Test has_pages. | ||
* | ||
* @covers WP_Offline_Page::has_pages() | ||
*/ | ||
public function test_has_pages() { | ||
$this->assertFalse( $this->instance->has_pages() ); | ||
|
||
// There is a post, but this needs a post type of 'page'. | ||
$this->factory()->post->create(); | ||
$this->assertFalse( $this->instance->has_pages() ); | ||
|
||
// There's a post with the type of 'page,' so this should be true. | ||
$this->factory()->post->create( array( | ||
'post_type' => 'page', | ||
) ); | ||
$this->assertTrue( $this->instance->has_pages() ); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,157 @@ | ||
<?php | ||
/** | ||
* WP_Offline_Page class. | ||
* | ||
* @package PWA | ||
*/ | ||
|
||
/** | ||
* WP_Offline_Page class. | ||
* Much of this is taken from wp-admin/privacy.php. | ||
*/ | ||
class WP_Offline_Page { | ||
|
||
/** | ||
* The option group for the offline page option. | ||
* | ||
* @var string | ||
*/ | ||
const OPTION_GROUP = 'reading'; | ||
|
||
/** | ||
* The option name. | ||
* | ||
* @var string | ||
*/ | ||
const OPTION_NAME = 'wp_offline_page'; | ||
|
||
/** | ||
* The ID of the settings section. | ||
* | ||
* @var string | ||
*/ | ||
const SETTING_ID = 'pwa_offline_page'; | ||
|
||
/** | ||
* Inits the class. | ||
*/ | ||
public function init() { | ||
add_action( 'admin_init', array( $this, 'register_setting' ) ); | ||
add_action( 'admin_init', array( $this, 'settings_field' ) ); | ||
} | ||
|
||
/** | ||
* Registers the offline page setting. | ||
*/ | ||
public function register_setting() { | ||
register_setting( | ||
self::OPTION_GROUP, | ||
self::OPTION_NAME, | ||
array( | ||
'type' => 'integer', | ||
'sanitize_callback' => array( $this, 'sanitize_callback' ), | ||
) | ||
); | ||
} | ||
|
||
/** | ||
* Sanitize callback for the setting. | ||
* Mainly taken from wp-admin/privacy.php. | ||
* | ||
* @todo: Ensure this is stored with a custom post status. | ||
* @param string $raw_setting The setting before sanitizing it. | ||
* @return string|null The sanitized setting, or null if it's invalid. | ||
*/ | ||
public function sanitize_callback( $raw_setting ) { | ||
$sanitized_post_id = sanitize_text_field( $raw_setting ); | ||
$offline_page = get_post( $sanitized_post_id ); | ||
if ( ! $offline_page instanceof WP_Post ) { | ||
add_settings_error( | ||
self::OPTION_NAME, | ||
self::OPTION_NAME, | ||
__( 'The current offline page does not exist. Please select or create one.', 'pwa' ) | ||
); | ||
} elseif ( 'trash' === $offline_page->post_status ) { | ||
add_settings_error( | ||
self::OPTION_NAME, | ||
self::OPTION_NAME, | ||
__( 'The current offline page is in the trash. Please select or create one.', 'pwa' ) | ||
); | ||
} else { | ||
return $sanitized_post_id; | ||
} | ||
} | ||
|
||
/** | ||
* Adds a settings field to the 'Reading Settings' page. | ||
*/ | ||
public function settings_field() { | ||
add_settings_field( | ||
self::SETTING_ID, | ||
__( 'Progressive Web App Offline Page', 'pwa' ), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Instead of “Progressive Web App Offline Page” I think this should be something like:
This would align more with the existing:
Then there can be a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @westonruter Is this what you are thinking? |
||
array( $this, 'settings_callback' ), | ||
self::OPTION_GROUP | ||
); | ||
} | ||
|
||
/** | ||
* Outputs the settings section. | ||
* Mainly taken from wp-admin/privacy.php. | ||
*/ | ||
public function settings_callback() { | ||
if ( $this->has_pages() ) : | ||
?> | ||
<label for="<?php echo esc_attr( self::OPTION_NAME ); ?>"> | ||
<?php esc_html_e( 'Select an existing page:', 'pwa' ); ?> | ||
</label> | ||
<?php | ||
wp_dropdown_pages( | ||
array( | ||
'name' => esc_html( self::OPTION_NAME ), | ||
/* Translators: %1$s: A long dash */ | ||
'show_option_none' => sprintf( esc_html__( '%1$s Select %1$s', 'pwa' ), '—' ), | ||
'option_none_value' => '0', | ||
'selected' => intval( get_option( self::OPTION_NAME ) ), | ||
'post_status' => array( 'draft', 'publish' ), | ||
) | ||
); | ||
endif; | ||
?> | ||
<p> | ||
<span> | ||
<?php | ||
// @todo: Add the href value, to allow creating a new page. | ||
if ( $this->has_pages() ) { | ||
printf( | ||
// Translators: %s: A link to create a new page. | ||
esc_html__( 'or %s a new one.', 'pwa' ), | ||
sprintf( '<a href="%s">%s</a>', '#', esc_html__( 'create', 'pwa' ) ) | ||
); | ||
} else { | ||
esc_html_e( 'There are no pages.', 'pwa' ); | ||
} | ||
?> | ||
</span> | ||
</p> | ||
<?php | ||
} | ||
|
||
/** | ||
* Whether the there are pages to display in the <select> | ||
* Mainly taken from wp-admin/privacy.php | ||
* | ||
* @todo: Handle a case where there are no pages. | ||
* @return boolean | ||
*/ | ||
public function has_pages() { | ||
$query = new WP_Query( array( | ||
'post_type' => 'page', | ||
'posts_per_page' => 1, | ||
'post_status' => array( | ||
'publish', | ||
'draft', | ||
), | ||
) ); | ||
return $query->found_posts > 0; | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This class is pretty small. Would it make more sense to have
WP_Offline_Page_UI
andWP_Offline_Page_Excluder
be just integrated into this one class? It could be calledWP_Offline_Manager
perhaps.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that too @westonruter. But the approach that I took was to separate out by focus and functionality. The
WP_Offline_Page
is a manager. You are right that it is a better representation of what it does and the reason it exists.Why did I break out UI and Excluder from the Manager? Great question @westonruter.
We have these distinct concerns in the Offline Page:
We could combine the serving and excluding concerns into one class, as both are focused on routing and queries. But I think that the UI portion is a separate concern. In time, I'm betting that the UX will grow as we think through how to empower users and make it even easier for them to leverage the plugin.
Where did the manager come from? It serves as a mediator to create the dependent objects, determine when to initialize them, and share commonality between them. It also provides the plugin (later core) bootstrap with one object to create and initialize.
@westonruter What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds fine with me. We can always revisit later if it makes sense to merge into a single class for the core merge. But for now we can continue as it is.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@westonruter I think we can rename and refocus the
excluder
class into a more inclusive responsibility of handing the request whether for serving or excluding,WP_Offline_Page_Request
.