Skip to content
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

Merged
merged 31 commits into from
Jul 27, 2018
Merged

Add UI to select an offline page #28

merged 31 commits into from
Jul 27, 2018

Conversation

kienstra
Copy link
Contributor

@kienstra kienstra commented Jul 11, 2018

Update: The state of the UI as merged in this PR is as follows:

image

The subsequent screenshots are snapshots during development.


WIP Pull Request

  • Mainly taken from the privacy policy page UI in wp-admin/privacy.php.
  • @todos include adding the custom post status and adding the href value

Closes #23

offline page ui

There's no "Use This Page" element as shown in Issue 23 (comment).

That's an <input type="submit">, which submits the <form>. The entire "Reading Settings" page is a form, which should probably only submit with the "Save Changes" element at the bottom.

If you try to save a post that doesn't exist (error mainly taken from the Privacy Page UI):

error-page-does-not-exist

If the post is in the trash (also mainly taken from wp-admin/privacy.php:

page-in-trash

Mainly taken from the privacy policy page
UI in wp-admin/privacy.php.
Some items remain,
including adding the custom post status,
and adding the href value.
public function settings_field() {
add_settings_field(
self::SETTING_ID,
__( 'Progressive Web App Offline Page', 'pwa' ),
Copy link
Collaborator

Choose a reason for hiding this comment

The 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:

Page displays when offline

This would align more with the existing:

Your homepage displays

Then there can be a p.description element which explains that this is is for PWA, and how provide more details about how it works, maybe mentioning it is “like a 404 page but it shows when someone's internet is down (e.g. during flight or going through tunnel), as opposed to when content is not found.”

Copy link
Contributor

@hellofromtonya hellofromtonya Jul 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter Is this what you are thinking?

screen shot 2018-07-11 at 4 52 32 pm

@kienstra
Copy link
Contributor Author

kienstra commented Jul 11, 2018

So far, this PR doesn't actually serve the offline page. It only saves the post ID as an option, with the name of WP_Offline_Page::OPTION_NAME. This should also serve the page, per #23.

@westonruter
Copy link
Collaborator

That's fine. I think it's good to work on the UX for selecting the offline page separately from the logic from actually serving the offline page. The serving of the offline page should probably incorporate Workbox (#5) and also it will probably involve precaching (#25) since we'll need to make sure the offline page and its required assets get added to the cache.

New option name is consistent with the naming structure for static
special pages.
Refactored to move the `add_settings_error` code to a separate method.  Hooked that method to `admin_notices` action.  Now settings errors appear both when saving and upon first loading the Reading page.

Modified the "in the trash" error to link a link to the Page's Trash UI.
This new strategy works for both the classic and Gutenberg editors.
Instead of registering multiple callbacks to the `admin_init`, this commit registers `init_admin()`.  Then the new callback method handles the order of calling each of the tasks.

This commit is more performant by O(n-1):

1. Decreases the processing time to do `add_action` as it only have to invoke it once.
2. Decreases the processing time when `do_action` as it only has to invoke one callback.

It's more readable, as we can quickly read that the callback intent is to initialize the admin.
This comment handles excluding special static pages ( i.e. `page_on_front`, `page_for_posts`, and `wp_page_for_privacy_policy`, and `page_for_offline` ) from `wp_dropdown_pages`:

1. When selected, the offline page is excluded from the other dropdowns, preventing it from being selected as anything else but the offline page.
2. The homepage, posts page, and privacy page are all excluded from the Offline Page's dropdown.

This commit also includes slight performance improvements by:

1. Initializing static pages on init.
2. Caching the offline page ID within the object itself, while also providing for a hard request (i.e. when true, run `get_option`).
Using the Privacy code, this commit adds the HTML and create new page functionality.  A unique action is added to the Reading URL.  Upon clicking the "create new page" link, the new functionality inserts a new page and assigns it as the Offline Page.
@kienstra
Copy link
Contributor Author

Offline Page

Hi @hellofromtonya,
It's nice how you excluded the 'offline page' from the homepage <select> elements:

offline-page-select

Also, the "create a new one" link works well:

page-created-offline-link

@hellofromtonya
Copy link
Contributor

hellofromtonya commented Jul 13, 2018

At this point, we have the following functionality in place with much more to do.

Create New Offline Page

The create new page link is now working. It follows the privacy policy's button strategy while using the hyperlink inside of a form button:

  • inserts a new page with the title of "Offline Page" and "draft" status.
  • assigns that page as the page_for_offline
  • redirects you to that new page

Tagging the Offline Page in the List Table

screen shot 2018-07-13 at 3 28 21 pm copy

Exclude Static Pages from Selection

As @kienstra showed above, the new changes exclude static pages from the page pick dropdowns.

  • The offline page cannot be selected as the homepage, posts page, or privacy page. To ensure, it is removed from those dropdowns.
  • Similarly the homepage, posts page, and privacy page cannot be selected as the offline page. All of those are excluded from the offline page dropdown.

Error Messaging

Error handling occurs on:

  • on saving settings
  • loading the Reading page

Currently errors include:

  1. when the selected offline page is in the trash:

screen shot 2018-07-13 at 3 48 43 pm

  1. when no offline page is selected

screen shot 2018-07-13 at 3 52 20 pm

  1. when the selected offline page has been deleted

screen shot 2018-07-13 at 3 53 08 pm

  1. when an error occurs creating a new offline page

To be consistent with the other special pages (homepage, posts page, and privacy), this commit re-enables the page attributes.
This commit excludes the offline page from all search and the menu builders including Appearance > Menus and Customizer's Menus.
This commit excludes the offline page from rendering on the frontend.  A 404 is served up.

When using plain permalinks, e.g. https://example.com?page_id=28, it will find and render the offline page.

This is an incompletion strategy per conversations in Issue #23 @see #23 (comment).
Grouped the tasks and split into separate classes, as the previous class design was doing too much.
This commit improves the excluder by:

1. Using parse_query.
2. Using $query->is_admin.
3. Checking for the offline page's query first.  If yes, setting a 404 on the frontend.
@westonruter westonruter changed the title [WIP] Add UI to select an offline page Add UI to select an offline page Jul 25, 2018
}

$page_id = wp_insert_post(
array(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The array() needs to be wrapped in wp_slash(), unfortunately.


if ( ! is_wp_error( $page_id ) ) {
update_option( WP_Offline_Page::OPTION_NAME, $page_id );
if ( wp_redirect( admin_url( 'post.php?post=' . $page_id . '&action=edit' ) ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think think using wp_redirect() in the conditional is right, since it will only return false if you don't provide a location, which one is being provided. Also, it should be wp_safe_redirect() for good measure.


if ( ! is_wp_error( $page_id ) ) {
update_option( WP_Offline_Page::OPTION_NAME, $page_id );
if ( wp_redirect( admin_url( 'post.php?post=' . $page_id . '&action=edit' ) ) ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should use get_edit_post_link() instead of manually constructing the URL.


$page_id = wp_insert_post(
array(
'post_title' => __( 'Offline Page', 'pwa' ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this title is going to be displayed to the end user, it should just be “Offline”.

*/
public function add_post_state( array $post_states, $post ) {
if ( $this->manager->get_offline_page_id() === $post->ID ) {
$post_states[] = __( 'Offline Page', 'pwa' );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent

* @return array
*/
public function get_static_pages( $hard = false ) {
if ( ! $hard ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since these options are autoloaded, they will aways be loaded from the cache and not from the DB. So there is no need to add another cache.

protected $static_pages = array(
'page_on_front' => 0,
'page_for_posts' => 0,
'wp_page_for_privacy_policy' => 0,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be the same as self::OPTION_NAME?

Copy link
Contributor

@hellofromtonya hellofromtonya Jul 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter This one is for the privacy page and not the offline page. It's used to exclude each of the special pages from the Offline Page's select dropdown.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your right. I totally missed this.

// Check that it excludes the configured static pages.
update_option( 'page_on_front', (int) $page_ids[0] );
update_option( 'page_for_posts', (int) $page_ids[1] );
update_option( 'wp_page_for_privacy_policy', (int) $page_ids[2] );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This string should instead be \WP_Offline_Page::OPTION_NAME.

Copy link
Contributor

@hellofromtonya hellofromtonya Jul 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Privacy policy page is a special page too. In this test we want to ensure that the page_on_front, page_for_posts, and wp_page_for_privacy_policy are excluded from the offline page dropdown.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hellofromtonya Oh, my mistake! I mistakenly read wp_page_for_privacy_policy as wp_page_for_offline_page or something. I actually hadn't seen the wp_page_for_privacy_policy option before, so I mistakenly glossed over it.

*
* @return array|string
*/
protected function get_static_pages_ids( $join = false ) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See note below about how $join isn't necessary since we can just implode( ',', ... ) when calling.

Also, I don't think we need get_static_pages_ids either. We can just do:

array_filter( $this->manager->get_static_pages() )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better refactor. Thank you @westonruter.

'option_none_value' => '0',
'selected' => intval( $this->manager->get_offline_page_id() ),
'post_status' => array( 'draft', 'publish' ),
'exclude' => esc_html( $this->get_static_pages_ids( true ) ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs to not exclude the offline page, or else it won't be selectable.

/**
* This class manages the Offline Page.
*/
class WP_Offline_Page {
Copy link
Collaborator

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 and WP_Offline_Page_Excluder be just integrated into this one class? It could be called WP_Offline_Manager perhaps.

Copy link
Contributor

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:

  1. Routing
    • Serving the page
    • Excluding the page from queries, searches, dropdowns, etc.
  2. UI - including user interfaces, settings, workflow, and messaging.

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?

Copy link
Collaborator

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.

Copy link
Contributor

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.

$page_id = wp_insert_post(
wp_slash( array(
'post_title' => __( 'Offline', 'pwa' ),
'post_status' => 'draft',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As discussed, let's have this create a publish'ed page. If we allow selecting a draft page then it wouldn't be accessible to the user.

$query->is_singular = false;
$query->set( 'page_id', 0 );
} elseif ( $this->is_okay_to_exclude( $query ) ) {
$query->set( 'post__not_in', array( $this->manager->get_offline_page_id() ) );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The post__not_in query var is advised against for performance reasons: https://vip.wordpress.com/documentation/performance-improvements-by-removing-usage-of-post__not_in/

Maybe that doesn't apply here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter I consider that in my research. Let's talk through it to see if we agree that this use case does not apply or if there is a better way to comply but also be performant.

The Concerns with post__not_in var

The concerns about the usage of post__not_in var include:

the query cache hitrate will often be a lot lower

True. But in this use case, we do not want the offline page included the caching as it should not be included or rendered on the frontend.

make the query slower if the exclusion list is large.

Notice that slower occurs when the list is large. In this use case, our list consists of one (1) item, i.e. one page ID.

The VIP Proposed Solution - Iterate.

In almost all cases you can gain great speed improvements by requesting more posts and skipping the posts in PHP.

The proposed alternative solution then is to include the offline page in the query and then remove it or skip over it when iterating through the collection of posts.

In our use case, this solution would require us to provide the means to exclude the offline page from the collection of posts after we get it back from the database. Let's think about what this means.

  1. The offline page would be cached.
    That's not desirable in our use case.

  2. We'd need to remove it from the array of posts.
    We could hook into one of the posts filters, such as posts_results. However, iterating over the collection to remove one item is not performant either.

My Thoughts

I think our use case is the perfect use case for post__not_in. Why?

  1. PWA is proposed to be merged into core.
  2. We need to exclude the offline page ID from all frontend queries and all searches.
  3. It's more performant to exclude in the SQL query than to iterate over the collection of posts for removal.
  4. We do not want the offline page to ever be in the cache.

IMO I think using post__not_in is the best strategy overall for this use case.

@westonruter What do you think? Are there other ways we could explore for excluding it from the query that do not involve using post__not_in?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your reasoning resonates with me! ✅

$query->is_404 = true;
$query->is_page = false;
$query->is_singular = false;
$query->set( 'page_id', 0 );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these being done in anticipation of there being an is_offline?

I don't think that we should necessarily make these changes here. The offline page is a page, so is_page() should return true. But what we could do here is $query->is_offline = $this->is_offline_page_query( $query ) to dynamically define a new member variable that can be looked at similarly to the others. We can then introduce a new global function:

function is_offline() {
	global $wp_query;
	if ( ! isset( $wp_query ) ) {
		_doing_it_wrong( __FUNCTION__, __( 'Conditional query tags do not work before the query is run. Before then, they always return false.' ), '3.1.0' );
		return false;
	}
	return ! empty( $wp_query->is_offline );
}

And this is_offline() function can be used in the template to conditionally show content for offline responses.

Copy link
Contributor

@hellofromtonya hellofromtonya Jul 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these being done in anticipation of there being an is_offline?

@westonruter No, it's meant to prevent the offline page from being displayed on the frontend. It's forcing a 404. It was added to provide the functionality discussed here in Issue #23.

I don't think that we should necessarily make these changes here.

Okay. Let's think about this. Maybe we don't care if the default offline page is requestable or viewable on the frontend.

  • We're already excluding it from the query via post__not_in.
  • We exclude it from the menus.

What's the harm of viewing it on the frontend? If no or little, then there's no need to set a 404 here.

And this is_offline() function can be used in the template to conditionally show content for offline responses.

Agreed. I was thinking about adding that functionality in a separate PR. We'll want pages/content be marked as offline and then set a var to note it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think there is no need to prevent the offline page from being accessible if someone knows the URL. If the user can access it directly by the URL, that is fine. In fact, that should be actually desirable because it would allow for someone to easily preview their offline page and make changes. The main thing is just that we don't expose the offline page in menus, page lists, searches, etc. In case someone links to the offline page then we could add wp_no_robots() to run in wp_head for that specific page.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bam, done in commit ab4cbc6. The default offline page will now render on the frontend. That makes it easier.

In case someone links to the offline page then we could add wp_no_robots() to run in wp_head for that specific page.

Included in this commit 9669026.

return false;
}

return ( $this->manager->get_offline_page_id() === $query->get( 'get_id' ) );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is get_id right here?

I think this method should largely just do:

return $query->is_singular() && $this->manager->get_offline_page_id() === (int) $query->get_queried_object_id();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter It is checking is_singular above in the guard clause:

		if ( ! $query->is_page || ! $query->is_singular ) {
			return false;
		}

I'll switch that to $query->is_singular() instead of $query->is_singular.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that is_singular implies is_page, so only one of the conditions is needed.

But I was more drawing attention to $query->get( 'get_id' ). I haven't seen get_id before. I don't believe that is right, and get_queried_object_id() is what you intend?

'show_option_none' => sprintf( esc_html__( '%1$s Select %1$s', 'pwa' ), '&mdash;' ),
'option_none_value' => '0',
'selected' => intval( $this->manager->get_offline_page_id() ),
'post_status' => array( 'draft', 'publish' ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per above, let's only include publish here.


wp_dropdown_pages(
array(
'name' => esc_html( WP_Offline_Page::OPTION_NAME ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should actually be esc_attr().

$query = new WP_Query( array(
'post_type' => 'page',
'posts_per_page' => 1,
'post_status' => array( 'publish', 'draft' ),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Per above, let's only include publish here.

$query->is_singular = false;
$query->set( 'page_id', 0 );
} elseif ( $this->is_okay_to_exclude( $query ) ) {
$query->set( 'post__not_in', array( $this->manager->get_offline_page_id() ) );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👉 This is not accounting for whether there is any existing post__not_in query var set. So at the very least this should merge with any existing value.

We want to include all special pages, as we use this collection to remove these special pages from the page select dropdowns.
Tonya Mork added 3 commits July 26, 2018 14:22
…e page dropdown

and that the offline page is included and selected.
This change was requested by Alberto. He's right as the previous label could be confusing as more than one page can be served in offline.  But this specific page is the default offline status page when the one requested is not available.
return array(
'page_on_front' => (int) get_option( 'page_on_front', 0 ),
'page_for_posts' => (int) get_option( 'page_for_posts', 0 ),
self::OPTION_NAME => $this->get_offline_page_id(),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The wp_page_for_privacy_policy needs to be restored here.

@@ -176,8 +176,7 @@ public function test_render_settings() {
// Check that it excludes the configured static pages.
update_option( 'page_on_front', (int) $page_ids[0] );
update_option( 'page_for_posts', (int) $page_ids[1] );
update_option( 'wp_page_for_privacy_policy', (int) $page_ids[2] );
$this->manager->get_static_pages( true );
update_option( WP_Offline_Page::OPTION_NAME, (int) $page_ids[2] );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I mistakenly equated wp_page_for_privacy_policy with WP_Offline_Page::OPTION_NAME here.

Tonya Mork added 6 commits July 26, 2018 16:30
Being mindful of performance, this commit checks if the "post__not_in" has a value.  If yes, then it merges with the offline page ID and then filters for only the unique values.

Else, it sets the offline ID only, thereby skipping the `array_merge` and `array_unique`.

The code is more complex.  But it serves to only run the the array functions when needed.
return false;
}

return $this->manager->get_offline_page_id() === (int) $query->get_queried_object_id();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing but we could combine these:

return (
    ! $query->is_admin
    &&
    $query->is_singular()
    &&
    $this->manager->get_offline_page_id() === (int) $query->get_queried_object_id()
);

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could. I tend to break them out separately. But we can definitely combine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter Changes added in commit af9aedd.

*/
public function exclude_from_query( WP_Query $query ) {
if ( $this->is_offline_page_query( $query ) ) {
add_action( 'wp_head', 'wp_no_robots' );
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is quite right because it could result in wp_no_robots being added unintentionally. Consider, for example, a request to a normal post on a site which at init or some other early hook does (for some reason):

$offline_query = new WP_Query( array(
    'page_id' => get_option( WP_Offline_Page::OPTION_NAME )
) );

This will result in the page getting wp_no_robots added since the parse_query action runs for every instance of WP_Query whether it is the main query or not.

It would be better to separate the concerns here to add a new method like:

function show_no_robots_on_offline_page() {
    global $wp_query;
    if ( $this->is_offline_page_query( $wp_query ) ) {
        wp_no_robots();
    }
}

And then add this method to the wp_head action unconditionally in WP_Offline_Page_Excluder::init().

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is giving a side effect. I was trying to avoid hooking in again and rerunning the check. But I do like the separation. Agree on your points.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@westonruter Changes added in commit af9aedd.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add ability to select a default offline page to be served when the user or site is offline
3 participants