-
Notifications
You must be signed in to change notification settings - Fork 7
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
Changes from 3 commits
b9bfbb2
9b653fc
89140d2
9b104eb
8089267
bd0b247
39094a0
c737ab6
df93c80
0c73766
b2f0456
92f97fd
b944508
b5137e2
5325a42
c40cf6e
bc1ddb6
5485f6d
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 |
---|---|---|
|
@@ -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. | ||
|
@@ -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}", | ||
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. Did you explore using 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. (Obviously, this would require an adjustment in the wp_insert_post call below) 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. |
||
'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'] ) ) { | ||
|
@@ -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 ); | ||
} | ||
|
||
/** | ||
|
@@ -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') ) { | ||
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. Can we make 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. 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() ); | ||
} | ||
} | ||
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. Realizing this is much more of a personal preference than anything, can't we check for 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. I think that simplifies the logic and makes it more readable, I've addressed it here: bd0b247 |
||
} | ||
return $response; | ||
} | ||
|
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 ); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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 | ||
|
||
|
@@ -60,6 +61,7 @@ function() { | |
// Bootstrap. | ||
SophiWP\Core\setup(); | ||
SophiWP\Settings\setup(); | ||
SophiWP\PostType\post_type(); | ||
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. To keep consistency, we probably should have a setup() wrapping this function as well @oscarssanchez . What do you think? 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. Sounds good @felipeelia i've addressed it here 9b104eb |
||
|
||
if ( ! SophiWP\Utils\is_configured() ) { | ||
return add_action( 'admin_notices', 'sophi_setup_notice' ); | ||
|
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.
When
$bypass_cache === true
, theif
statement at 76 will not check for$curated_posts
at all. I think we are safe to skip 57:74 in this case.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.
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 ?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.
@oscarssanchez thank you for explaining. I mean we could make the query optional.
$bypass_cache === true
, we will execute contents of if statement in 78:90 regardless the$curated_posts
initial value, it will be overwrited withget_posts
.$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
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.
@oscarssanchez was ^ this addressed in your latest commits?
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.
@cadic Thanks for clarifying, that makes sense to me! I have addressed this here: c40cf6e