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

SIT-2708: Save sophi responses in CPT #298

Merged
merged 18 commits into from
Jun 13, 2022
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions includes/blocks/site-automation-block/register.php
Original file line number Diff line number Diff line change
Expand Up @@ -40,8 +40,6 @@ function render_block_callback( $attributes, $content, $block ) {
$page_name = sanitize_title( $attributes['pageName'] );
$widget_name = sanitize_title( $attributes['widgetName'] );

$curated_posts_transient_key = 'sophi_curated_posts_' . $page_name . '_' . $widget_name;

/**
* Whether to bypass caching.
*
Expand All @@ -56,9 +54,27 @@ function render_block_callback( $attributes, $content, $block ) {
*/
$bypass_cache = apply_filters( 'sophi_bypass_curated_posts_cache', false, $page_name, $widget_name );

$curated_posts = get_transient( $curated_posts_transient_key );
$sophi_cached_response = new \WP_Query(
Copy link
Contributor

Choose a reason for hiding this comment

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

When $bypass_cache === true, the if statement at 76 will not check for $curated_posts at all. I think we are safe to skip 57:74 in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In this case I think we should still do the query. The most important check is if $bypass_cache === true, and the next one should be if there's $curated_posts does it makes sense @cadic ?

Copy link
Contributor

Choose a reason for hiding this comment

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

@oscarssanchez thank you for explaining. I mean we could make the query optional.

  1. When $bypass_cache === true, we will execute contents of if statement in 78:90 regardless the $curated_posts initial value, it will be overwrited with get_posts.
  2. When $bypass_cache === false, this if statement will only be executed if $curate_posts is negative.

This means to me that we only need to take care about the initial value for $curated_posts when $bypass_cache === false

$curated_posts = false;

if ( false === $bypass_cache ) {
	$sophi_cached_response = new \WP_Query(
		[
			'post_name__in'          => [ "sophi-site-automation-data-{$page_name}-{$widget_name}" ],
			'post_type'              => 'sophi-response',
			'posts_per_page'         => 1,
			'fields'                 => 'ids',
			'no_found_rows'          => true,
			'update_post_term_cache' => false
		]
	);

	if ( $sophi_cached_response->found_posts ) {
		$last_update = get_post_meta( $sophi_cached_response->posts[0], 'sophi_site_automation_last_updated', true );

		if ( $last_update + 5 * MINUTE_IN_SECONDS > time() ) {
			$curated_posts = get_post_meta( $sophi_cached_response->posts[0], 'sophi_site_automation_data' );
		}
	}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

@oscarssanchez was ^ this addressed in your latest commits?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cadic Thanks for clarifying, that makes sense to me! I have addressed this here: c40cf6e

[
's' => "sophi_site_automation_data_{$page_name}_{$widget_name}",
'post_type' => 'sophi-response',
'posts_per_page' => 1,
'fields' => 'ids',
'no_found_rows' => true,
'update_post_term_cache' => false
]
);

if ( $sophi_cached_response->found_posts ) {
$last_update = get_post_meta( $sophi_cached_response->posts[0], 'sophi_site_automation_last_updated', true );

if ( $last_update + 5 * MINUTE_IN_SECONDS > time() ) {
$curated_posts = get_post_meta( $sophi_cached_response->posts[0], "sophi_site_automation_data_{$page_name}_{$widget_name}" );
}
}

if ( $bypass_cache || ! $curated_posts ) {
cadic marked this conversation as resolved.
Show resolved Hide resolved

if ( $bypass_cache || false === $curated_posts ) {
// phpcs:ignore WordPressVIPMinimum.Functions.RestrictedFunctions.get_posts_get_posts
$curated_posts = get_posts(
[
Expand All @@ -71,7 +87,6 @@ function render_block_callback( $attributes, $content, $block ) {
return '';
}

set_transient( $curated_posts_transient_key, $curated_posts, 5 * MINUTE_IN_SECONDS );
}

ob_start();
Expand Down
50 changes: 42 additions & 8 deletions includes/classes/SiteAutomation/Request.php
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ public function get( $page, $widget, $timeout = 3 ) {

$this->status = $this->get_status();
$site_automation_data = false;
$post_id = false;

/**
* Whether to bypass caching.
Expand All @@ -96,8 +97,26 @@ public function get( $page, $widget, $timeout = 3 ) {
*/
$bypass_cache = apply_filters( 'sophi_bypass_get_cache', false, $page, $widget );

if ( ! $bypass_cache ) {
$site_automation_data = get_option( "sophi_site_automation_data_{$page}_{$widget}" );
if ( ! $bypass_cache && post_type_exists('sophi-response' ) ) {
cadic marked this conversation as resolved.
Show resolved Hide resolved
$query = new \WP_Query(
[
's' => "sophi_site_automation_data_{$page}_{$widget}",

Choose a reason for hiding this comment

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

Did you explore using post_name__in here instead? Just wondering if someone has ElasticPress and Protected Content enabled (just as an example) if this wouldn't bring the wrong result.

Choose a reason for hiding this comment

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

(Obviously, this would require an adjustment in the wp_insert_post call below)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it is a more reliable approach indeed, I have addressed it here 39094a0 and here c737ab6

'post_type' => 'sophi-response',
'posts_per_page' => 1,
'fields' => 'ids',
'no_found_rows' => true,
'update_post_term_cache' => false
]
);

if ( $query->found_posts ) {
$post_id = $query->posts[0];
$last_update = get_post_meta( $post_id->posts[0], 'sophi_site_automation_last_updated', true );
oscarssanchez marked this conversation as resolved.
Show resolved Hide resolved

if ( $last_update + 5 * MINUTE_IN_SECONDS > time() ) {
$site_automation_data = get_post_meta( $post_id->posts[0], 'sophi_site_automation_data', true );
oscarssanchez marked this conversation as resolved.
Show resolved Hide resolved
}
}
}

if ( $site_automation_data && ! empty( $this->status['success'] ) ) {
Expand Down Expand Up @@ -125,7 +144,7 @@ public function get( $page, $widget, $timeout = 3 ) {
}

$this->set_status( [ 'success' => true ] );
return $this->process( $response, $bypass_cache );
return $this->process( $response, $bypass_cache, $post_id );
}

/**
Expand Down Expand Up @@ -254,18 +273,33 @@ private function request( $timeout ) {
/**
* Process response from Sophi.
*
* @param array $response Response of Site Automation API.
* @param bool $bypass_cache Whether to bypass cache or not.
* @param array $response Response of Site Automation API.
* @param bool $bypass_cache Whether to bypass cache or not.
* @param int|bool $post_id The post id to update the post meta with response or false.
*
* @return array
*/
private function process( $response, $bypass_cache ) {
private function process( $response, $bypass_cache, $post_id ) {
if ( ! $response ) {
return [];
}

if ( ! $bypass_cache ) {
update_option( "sophi_site_automation_data_{$this->page}_{$this->widget}", $response );
if ( ! $bypass_cache && post_type_exists('sophi-response') ) {

Choose a reason for hiding this comment

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

Can we make post_type_exists('sophi-response') to post_type_exists( 'sophi-response')? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure addressed it here 8089267

if ( $post_id ) {
update_post_meta( $post_id, "sophi_site_automation_data_{$this->page}_{$this->widget}", $response );
update_post_meta( $post_id, 'sophi_site_automation_last_updated', time() );
} else {
$post_id = wp_insert_post(
[
'post_type' => 'sophi-response',
'post_title' => "sophi_site_automation_data_{$this->page}_{$this->widget}",
]
);
if ( $post_id ) {
update_post_meta( $post_id, "sophi_site_automation_data_{$this->page}_{$this->widget}", $response );
update_post_meta( $post_id, 'sophi_site_automation_last_updated', time() );
}
}

Choose a reason for hiding this comment

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

Realizing this is much more of a personal preference than anything, can't we check for ! $post_id and create the post, and only then call the update_post_meta lines (only once)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that simplifies the logic and makes it more readable, I've addressed it here: bd0b247

}
return $response;
}
Expand Down
25 changes: 25 additions & 0 deletions includes/functions/post-type.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,25 @@
<?php
/**
* The Sophi Post type register file.
*/
namespace SophiWP\PostType;

/**
* Registers the sophi-response post type.
*/
function post_type() {
$args = [
'description' => esc_html__( 'Sophi Responses', 'sophi-wp' ),
'public' => false,
'publicly_queryable' => false,
'show_ui' => false,
'show_in_menu' => false,
'query_var' => true,
'capability_type' => 'post',
'has_archive' => false,
'hierarchical' => false,
'supports' => [ 'title' ],
];

register_post_type( 'sophi-response', $args );
}
2 changes: 2 additions & 0 deletions sophi.php
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
require_once SOPHI_WP_INC . 'functions/tracking.php';
require_once SOPHI_WP_INC . 'functions/content-sync.php';
require_once SOPHI_WP_INC . 'functions/blocks.php';
require_once SOPHI_WP_INC . 'functions/post-type.php';

// phpcs:enable WordPressVIPMinimum.Files.IncludingFile.UsingCustomConstant

Expand All @@ -60,6 +61,7 @@ function() {
// Bootstrap.
SophiWP\Core\setup();
SophiWP\Settings\setup();
SophiWP\PostType\post_type();

Choose a reason for hiding this comment

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

To keep consistency, we probably should have a setup() wrapping this function as well @oscarssanchez . What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good @felipeelia i've addressed it here 9b104eb


if ( ! SophiWP\Utils\is_configured() ) {
return add_action( 'admin_notices', 'sophi_setup_notice' );
Expand Down