From e78bffee315087acc11c7424680ce7f59ba847d5 Mon Sep 17 00:00:00 2001 From: Filip Ilic Date: Fri, 18 Sep 2026 14:06:01 +0200 Subject: [PATCH 1/3] Add a short confirmation code to the test email Email delivery is the one check a site cannot verify for itself: WordPress knows wp_mail() returned true, not that anything arrived. The existing token solves that by living only in the delivered message, so presenting it back proves delivery. The token is delivered as the href of a "Click here" link, which works for a person who clicks and not for a person who wants to relay it. Reading it off the screen is impossible; copying it means right-clicking and pasting a 128-character URL. That matters now this question gets asked through an AI assistant rather than the dashboard. An assistant with mailbox access can read the token and needs nothing from the user. An assistant without one has to ask, and "tell it the code from the email" only works if the code is short enough to say out loud. So the email now carries both: the link, unchanged, and a four-character code shown as text. The code is deliberately weaker than the token, and the docblock on generate_task_confirmation_code() records why that is acceptable -- single use, 24-hour expiry, scoped to one user and one task, manage_options required, and rate limiting around it. Anyone who could satisfy all of that can complete the task from wp-admin anyway. The alphabet omits characters that are misread when spoken or retyped (0/O, 1/I/L, 5/S, 8/B), and comparison ignores case and surrounding whitespace, because a code that survives being read aloud is the entire point. A test asserts the alphabet holds across repeated generation. The email body was built in two places; it is now built once. Co-Authored-By: Claude Opus 5 (1M context) --- classes/class-suggested-tasks.php | 109 ++++++++++++++++++ .../providers/class-email-sending.php | 35 +++++- tests/phpunit/test-class-suggested-tasks.php | 87 ++++++++++++++ 3 files changed, 225 insertions(+), 6 deletions(-) diff --git a/classes/class-suggested-tasks.php b/classes/class-suggested-tasks.php index 70542077b..cd1791c97 100644 --- a/classes/class-suggested-tasks.php +++ b/classes/class-suggested-tasks.php @@ -244,12 +244,121 @@ public function maybe_complete_task() { } } + /** + * The alphabet used for confirmation codes. + * + * Excludes characters that are easily confused when read aloud or retyped: + * 0/O, 1/I/L, 5/S, 8/B. A code is only useful if a person relays it + * correctly on the first try. + * + * @var string + */ + const CONFIRMATION_CODE_ALPHABET = '234679ACDEFGHJKMNPQRTUVWXYZ'; + + /** + * The length of a confirmation code. + * + * @var int + */ + const CONFIRMATION_CODE_LENGTH = 4; + + /** + * Generate a short confirmation code for a task. + * + * WHY THIS EXISTS, alongside the 32-character token below: + * + * Both prove the same thing -- that the email actually arrived, because + * the value only exists inside the delivered message. They differ in who + * can realistically relay them. + * + * The token is delivered as the href of a "Click here" link. A person + * cannot read it off the screen: they would have to right-click, copy the + * link address, and paste a 128-character URL. That is fine for clicking + * and useless for telling someone. + * + * This matters because email delivery is the one check a site cannot + * verify for itself, and it is increasingly answered through an AI + * assistant rather than the dashboard. An assistant with mailbox access + * reads the token and needs nothing from the user. An assistant without it + * has to ask, and "tell it the code from the email" only works if the code + * is short enough to say out loud. + * + * The code is deliberately weaker than the token: four characters from a + * 27-character alphabet is about 19 bits. What makes that acceptable is + * everything around it -- the code is single-use, expires in 24 hours, is + * scoped to one user and one task, is only accepted from a caller who + * already holds manage_options, and failed attempts are rate limited. An + * attacker who could satisfy all of that can complete the task by simply + * clicking a button in wp-admin, so the code is not the weak link. + * + * Codes are stored uppercase and compared case-insensitively, because a + * person retyping one should not have to think about it. + * + * @param string $task_id The task ID. + * @param int $user_id The user ID. + * + * @return string The generated confirmation code. + */ + public function generate_task_confirmation_code( $task_id, $user_id ) { + $alphabet = self::CONFIRMATION_CODE_ALPHABET; + $max = \strlen( $alphabet ) - 1; + $code = ''; + + for ( $i = 0; $i < self::CONFIRMATION_CODE_LENGTH; $i++ ) { + $code .= $alphabet[ \wp_rand( 0, $max ) ]; + } + + \set_transient( + 'prpl_confirm_' . $task_id . '_' . $user_id, + $code, + DAY_IN_SECONDS + ); + + return $code; + } + + /** + * Verify a task confirmation code. + * + * @param string $task_id The task ID. + * @param int $user_id The user ID. + * @param string $provided_code The code supplied by the caller. + * + * @return bool + */ + public function verify_task_confirmation_code( $task_id, $user_id, $provided_code ) { + $stored_code = \get_transient( 'prpl_confirm_' . $task_id . '_' . $user_id ); + + if ( ! $stored_code ) { + return false; // Expired, already used, or never issued. + } + + // hash_equals to keep the comparison constant-time; strtoupper because + // the code is meant to be retyped by a person. + return \hash_equals( (string) $stored_code, \strtoupper( \trim( $provided_code ) ) ); + } + + /** + * Delete a task confirmation code after use. + * + * @param string $task_id The task ID. + * @param int $user_id The user ID. + * + * @return bool True if deleted, false otherwise. + */ + public function delete_task_confirmation_code( $task_id, $user_id ) { + return \delete_transient( 'prpl_confirm_' . $task_id . '_' . $user_id ); + } + /** * Generate a secure token for task completion via email link. * * This token prevents CSRF attacks by ensuring only legitimate email * links can mark tasks as complete. * + * See generate_task_confirmation_code() above for why a second, shorter + * value exists next to this one. + * * @param string $task_id The task ID. * @param int $user_id The user ID. * diff --git a/classes/suggested-tasks/providers/class-email-sending.php b/classes/suggested-tasks/providers/class-email-sending.php index 50f9e3064..a9ca3e86d 100644 --- a/classes/suggested-tasks/providers/class-email-sending.php +++ b/classes/suggested-tasks/providers/class-email-sending.php @@ -110,6 +110,34 @@ public function init() { // $this->email_content is not read anywhere else. } + /** + * Build the test email body. + * + * The body carries two ways to confirm the message arrived: a link, which + * carries the full token and is what a person clicks, and a short code, + * which is what a person can read out to an AI assistant that has no + * access to their mailbox. Both prove the same thing -- that the message + * was delivered -- because neither value exists anywhere the recipient + * could reach without receiving it. See + * Suggested_Tasks::generate_task_confirmation_code() for the reasoning. + * + * @param string $token The completion token. + * @param int $user_id The user the token and code belong to. + * + * @return string + */ + protected function get_email_content( $token, $user_id ) { + $code = \progress_planner()->get_suggested_tasks()->generate_task_confirmation_code( $this->get_task_id(), $user_id ); + + return \sprintf( + /* translators: %1$s: the admin URL, %2$s: the assistant's name, %3$s: a short confirmation code. */ + \__( 'You just used Progress Planner to verify if sending email works on your website.

The good news; it does! Click here to mark %2$s\'s Recommendation as completed.

Using an AI assistant? Give it this confirmation code instead: %3$s', 'progress-planner' ), + \admin_url( 'admin.php?page=progress-planner&prpl_complete_task=' . $this->get_task_id() . '&token=' . $token ), + \esc_html( \progress_planner()->get_ui__branding()->get_ravi_name() ), + \esc_html( $code ) + ); + } + /** * Get the troubleshooting guide URL. * @@ -273,12 +301,7 @@ public function ajax_test_email_sending() { $user_id = \get_current_user_id(); $token = \progress_planner()->get_suggested_tasks()->generate_task_completion_token( $this->get_task_id(), $user_id ); - $email_content = \sprintf( - // translators: %1$s the admin URL. - \__( 'You just used Progress Planner to verify if sending email works on your website.

The good news; it does! Click here to mark %2$s\'s Recommendation as completed.', 'progress-planner' ), - \admin_url( 'admin.php?page=progress-planner&prpl_complete_task=' . $this->get_task_id() . '&token=' . $token ), - \esc_html( \progress_planner()->get_ui__branding()->get_ravi_name() ) - ); + $email_content = $this->get_email_content( $token, $user_id ); $headers = [ 'Content-Type: text/html; charset=UTF-8' ]; diff --git a/tests/phpunit/test-class-suggested-tasks.php b/tests/phpunit/test-class-suggested-tasks.php index fec2a15d4..6be4baf68 100644 --- a/tests/phpunit/test-class-suggested-tasks.php +++ b/tests/phpunit/test-class-suggested-tasks.php @@ -102,4 +102,91 @@ public function test_task_cleanup() { \wp_cache_flush_group( \Progress_Planner\Suggested_Tasks_DB::GET_TASKS_CACHE_GROUP ); // Clear the cache. $this->assertEquals( \count( $tasks_to_keep ), \count( \progress_planner()->get_suggested_tasks_db()->get_tasks_by( [ 'post_status' => 'publish' ] ) ) ); } + + /** + * Test that a confirmation code round-trips. + * + * @return void + */ + public function test_confirmation_code_verifies() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertSame( 4, \strlen( $code ) ); + $this->assertTrue( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, $code ) ); + } + + /** + * Test that a code is accepted however a person retypes it. + * + * A code only has value if it survives being read aloud and typed back, + * so case and surrounding whitespace must not matter. + * + * @return void + */ + public function test_confirmation_code_is_forgiving_about_formatting() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertTrue( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, \strtolower( $code ) ) ); + $this->assertTrue( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, ' ' . $code . ' ' ) ); + } + + /** + * Test that a wrong code is rejected. + * + * @return void + */ + public function test_confirmation_code_rejects_a_wrong_code() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertFalse( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, 'ZZZZ' ) ); + } + + /** + * Test that a code belongs to one user and one task. + * + * @return void + */ + public function test_confirmation_code_is_scoped() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertFalse( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 2, $code ) ); + $this->assertFalse( $suggested_tasks->verify_task_confirmation_code( 'other-task', 1, $code ) ); + } + + /** + * Test that a code cannot be reused once consumed. + * + * @return void + */ + public function test_confirmation_code_is_single_use() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $suggested_tasks->delete_task_confirmation_code( 'sending-email', 1 ); + + $this->assertFalse( $suggested_tasks->verify_task_confirmation_code( 'sending-email', 1, $code ) ); + } + + /** + * Test that codes avoid characters that are misread. + * + * @return void + */ + public function test_confirmation_code_avoids_ambiguous_characters() { + $suggested_tasks = \progress_planner()->get_suggested_tasks(); + + for ( $i = 0; $i < 40; $i++ ) { + $code = $suggested_tasks->generate_task_confirmation_code( 'sending-email', 1 ); + + $this->assertSame( + 0, + \preg_match( '/[01OILSB58]/', $code ), + "Code {$code} contains an easily confused character." + ); + } + } } From ff2fafee5068e19913c5fb9b30d522b982c2129d Mon Sep 17 00:00:00 2001 From: Filip Ilic Date: Fri, 18 Sep 2026 14:15:53 +0200 Subject: [PATCH 2/3] Let an agent clear WordPress's placeholder content Adds hello-world and sample-page, bringing the applicable set to 19. These are the first entries that remove something rather than change a setting, and they are treated differently for it: - Annotated destructive. The annotation describes what the ability can do, not what a given call does, so complete-recommendation as a whole is now destructive and a client prompts before any of it runs. - Confirm-only, so next-mode skips them. A daily unattended run must never be the thing that deleted something; they can only be applied by naming the provider. - Trashed, not force-deleted. The dashboard's JavaScript passes force=true and removes the post outright. An agent should leave a way back, and the completion check passes either way because it looks for a published post. What makes them defensible at all is that the target is not ambiguous: the data collector resolves the specific post WordPress ships, by slug with a title fallback, and no ID is ever taken from the caller. The success message says the content was trashed and can be restored, rather than reusing the settings wording. The new tests create and trash posts, which writes to the activities table. That table is custom, so WP_UnitTestCase does not roll it back, and post IDs are reused across tests -- a leftover row was being seen by an unrelated test that asserts a fresh post has no activity. The class now clears the table in tearDown. Co-Authored-By: Claude Opus 5 (1M context) --- classes/abilities/class-abilities.php | 15 ++- .../abilities/class-recommendation-fixes.php | 106 +++++++++++++++++ classes/abilities/class-recommendations.php | 27 ++++- classes/abilities/class-schemas.php | 4 + tests/phpunit/test-class-abilities.php | 111 ++++++++++++++++++ 5 files changed, 253 insertions(+), 10 deletions(-) diff --git a/classes/abilities/class-abilities.php b/classes/abilities/class-abilities.php index 1e3298cb8..16af34762 100644 --- a/classes/abilities/class-abilities.php +++ b/classes/abilities/class-abilities.php @@ -157,6 +157,12 @@ public function register_abilities() { 'permission_callback' => [ $this, 'can_fix' ], 'execute_callback' => [ $this->recommendations, 'complete' ], 'readonly' => false, + // Most of what this applies is a settings change, but two + // recommendations trash WordPress's placeholder content. The + // annotation describes what the ability can do, not what a + // given call happens to do, so it is declared destructive and + // a client prompts before any of it runs. + 'destructive' => true, ] ) ); @@ -173,8 +179,9 @@ public function register_abilities() { * @return array */ private function ability_args( array $args ) { - $readonly = $args['readonly'] ?? true; - unset( $args['readonly'] ); + $readonly = $args['readonly'] ?? true; + $destructive = $args['destructive'] ?? false; + unset( $args['readonly'], $args['destructive'] ); return \array_merge( [ @@ -184,9 +191,7 @@ private function ability_args( array $args ) { 'show_in_rest' => true, 'annotations' => [ 'readonly' => $readonly, - // Nothing registered here deletes or overwrites content: - // the write ability changes settings from a fixed list. - 'destructive' => false, + 'destructive' => $destructive, 'idempotent' => true, ], ], diff --git a/classes/abilities/class-recommendation-fixes.php b/classes/abilities/class-recommendation-fixes.php index 0d4bcc8db..255fef69b 100644 --- a/classes/abilities/class-recommendation-fixes.php +++ b/classes/abilities/class-recommendation-fixes.php @@ -148,6 +148,37 @@ class Recommendation_Fixes { 'value' => false, 'summary' => 'Disable author feeds in All in One SEO.', ], + + /* + * Deleting WordPress's own placeholder content. + * + * These are the only entries that remove something rather than change a + * setting, so they are the only ones annotated destructive. Two things + * make them defensible anyway: the target is not ambiguous -- the data + * collector resolves the specific post WordPress ships, by slug with a + * title fallback -- and the recommendation exists precisely because the + * site owner is being asked to delete it. + * + * They are trashed, not force-deleted. The dashboard's own JavaScript + * passes force=true and removes the post outright; an agent acting + * unattended should leave a way back, and the task's completion check + * passes either way because it looks for a published post. + * + * confirm_only keeps them out of next-mode, so a daily unattended run + * never deletes anything: they can only be applied by naming the + * provider explicitly. + */ + 'hello-world' => [ + 'delete' => 'post', + 'confirm_only' => true, + 'summary' => 'Move the default "Hello world!" post to the trash.', + ], + 'sample-page' => [ + 'delete' => 'page', + 'confirm_only' => true, + 'summary' => 'Move the default "Sample Page" to the trash.', + ], + 'aioseo-media-pages' => [ 'seo' => 'aioseo', // Attachment redirection lives under dynamicOptions, not options, and @@ -225,6 +256,10 @@ public static function apply( $provider_id, $value = null ) { ); } + if ( isset( $fix['delete'] ) ) { + return self::apply_deletion( $provider_id ); + } + if ( isset( $fix['seo'] ) ) { return self::apply_seo_setting( $fix ); } @@ -246,6 +281,77 @@ public static function apply( $provider_id, $value = null ) { return true; } + /** + * Move a placeholder post to the trash. + * + * The post ID comes from the provider's own data collector, which resolves + * WordPress's default content by slug. Nothing here takes an ID from the + * caller, so an ability argument cannot point this at arbitrary content. + * + * @param string $provider_id The provider ID. + * + * @return true|\WP_Error + */ + private static function apply_deletion( $provider_id ) { + $provider = \progress_planner()->get_suggested_tasks()->get_tasks_manager()->get_task_provider( $provider_id ); + + if ( ! $provider || ! \method_exists( $provider, 'get_data_collector' ) ) { + return new \WP_Error( + 'progress_planner_no_target', + \__( 'That recommendation is not available on this site.', 'progress-planner' ) + ); + } + + $post_id = (int) $provider->get_data_collector()->collect(); + + if ( ! $post_id || ! \get_post( $post_id ) ) { + return new \WP_Error( + 'progress_planner_no_target', + \__( 'The default content this recommendation refers to is no longer there.', 'progress-planner' ) + ); + } + + if ( ! \wp_trash_post( $post_id ) ) { + return new \WP_Error( + 'progress_planner_delete_failed', + \__( 'The content could not be moved to the trash.', 'progress-planner' ) + ); + } + + return true; + } + + /** + * Whether applying a fix removes content rather than changing a setting. + * + * Drives the destructive annotation, which is what a client shows a person + * before calling. + * + * @param string $provider_id The provider ID. + * + * @return bool + */ + public static function is_destructive( $provider_id ) { + $fix = self::get( $provider_id ); + + return null !== $fix && isset( $fix['delete'] ); + } + + /** + * Whether a fix may only be applied by naming its provider explicitly. + * + * Keeps anything that removes content out of an unattended run. + * + * @param string $provider_id The provider ID. + * + * @return bool + */ + public static function is_confirm_only( $provider_id ) { + $fix = self::get( $provider_id ); + + return null !== $fix && ! empty( $fix['confirm_only'] ); + } + /** * Apply a setting owned by an SEO plugin. * diff --git a/classes/abilities/class-recommendations.php b/classes/abilities/class-recommendations.php index 0bfbfa26e..838985374 100644 --- a/classes/abilities/class-recommendations.php +++ b/classes/abilities/class-recommendations.php @@ -135,12 +135,23 @@ public function complete( $input = [] ) { // for work that did not happen. $completed = $this->is_satisfied( $provider, $task ); + // The wording distinguishes a settings change from a deletion: an agent + // relaying this to a person should not describe trashing a post as + // changing a setting. + if ( Recommendation_Fixes::is_destructive( $provider_id ) ) { + $message = $completed + ? \__( 'The content was moved to the trash and the recommendation is now satisfied. It can be restored from the trash if that was not intended.', 'progress-planner' ) + : \__( 'The content was moved to the trash, but the recommendation is not reported as satisfied yet.', 'progress-planner' ); + } else { + $message = $completed + ? \__( 'The setting was changed and the recommendation is now satisfied.', 'progress-planner' ) + : \__( 'The setting was changed, but the recommendation is not reported as satisfied yet.', 'progress-planner' ); + } + return $this->result( true, $completed ? 'completed' : 'applied_not_yet_complete', - $completed - ? \__( 'The setting was changed and the recommendation is now satisfied.', 'progress-planner' ) - : \__( 'The setting was changed, but the recommendation is not reported as satisfied yet.', 'progress-planner' ), + $message, $task ); } @@ -198,8 +209,13 @@ private function get_next_fixable_provider_id() { $provider_id = $task->get_provider_id(); // A fix needing a value cannot be chosen unattended: there is no - // correct tagline to invent on the site owner's behalf. - if ( ! Recommendation_Fixes::has_fix( $provider_id ) || Recommendation_Fixes::needs_value( $provider_id ) ) { + // correct tagline to invent on the site owner's behalf. Nor can one + // that removes content, however well scoped -- an unattended run + // should never be the thing that deleted something. + if ( ! Recommendation_Fixes::has_fix( $provider_id ) + || Recommendation_Fixes::needs_value( $provider_id ) + || Recommendation_Fixes::is_confirm_only( $provider_id ) + ) { continue; } @@ -285,6 +301,7 @@ private function prepare( $task ) { // recommendations it is allowed to apply. 'fixable' => Recommendation_Fixes::has_fix( $provider_id ), 'needs_value' => Recommendation_Fixes::needs_value( $provider_id ), + 'destructive' => Recommendation_Fixes::is_destructive( $provider_id ), ]; } } diff --git a/classes/abilities/class-schemas.php b/classes/abilities/class-schemas.php index d534ec944..e9ed0aeae 100644 --- a/classes/abilities/class-schemas.php +++ b/classes/abilities/class-schemas.php @@ -217,6 +217,10 @@ public static function recommendation() { 'type' => 'boolean', 'description' => \__( 'Whether applying it requires a value from the caller, such as the tagline text.', 'progress-planner' ), ], + 'destructive' => [ + 'type' => 'boolean', + 'description' => \__( 'Whether applying it removes content rather than changing a setting. These are only applied when named explicitly, never picked automatically.', 'progress-planner' ), + ], ], ]; } diff --git a/tests/phpunit/test-class-abilities.php b/tests/phpunit/test-class-abilities.php index 1fff245b8..2482bcc19 100644 --- a/tests/phpunit/test-class-abilities.php +++ b/tests/phpunit/test-class-abilities.php @@ -58,6 +58,24 @@ public function setUp(): void { $this->recommendations = new \Progress_Planner\Abilities\Recommendations(); } + /** + * Tear down test. + * + * Activities live in a custom table that WP_UnitTestCase does not roll + * back, and post IDs are reused across tests. A row left here would be seen + * by a later test that happens to be handed the same ID and asserts it has + * no activity, so this class clears what it caused. + * + * @return void + */ + public function tearDown(): void { + global $wpdb; + + $wpdb->query( 'TRUNCATE TABLE ' . $wpdb->prefix . 'progress_planner_activities' ); // phpcs:ignore WordPress.DB + + parent::tearDown(); + } + /** * Invoke a private or protected method on an object. * @@ -663,4 +681,97 @@ public function test_seo_fix_requires_its_plugin() { $this->assertWPError( $result ); $this->assertSame( 'progress_planner_seo_plugin_inactive', $result->get_error_code() ); } + + /** + * Test that the placeholder deletions are marked destructive. + * + * @return void + */ + public function test_placeholder_deletions_are_destructive() { + $this->assertTrue( Recommendation_Fixes::is_destructive( 'hello-world' ) ); + $this->assertTrue( Recommendation_Fixes::is_destructive( 'sample-page' ) ); + $this->assertFalse( Recommendation_Fixes::is_destructive( 'disable-comments' ) ); + } + + /** + * Test that anything destructive can only be applied by name. + * + * An unattended run must never be the thing that deleted something. + * + * @return void + */ + public function test_destructive_fixes_are_confirm_only() { + $this->assertTrue( Recommendation_Fixes::is_confirm_only( 'hello-world' ) ); + $this->assertTrue( Recommendation_Fixes::is_confirm_only( 'sample-page' ) ); + $this->assertFalse( Recommendation_Fixes::is_confirm_only( 'disable-comments' ) ); + } + + /** + * Test that applying the hello-world fix trashes the post rather than + * deleting it outright. + * + * @return void + */ + public function test_hello_world_fix_trashes_the_post() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $post_id = self::factory()->post->create( + [ + 'post_title' => 'Hello world!', + 'post_name' => 'hello-world', + 'post_status' => 'publish', + ] + ); + + $this->seed_task( 'hello-world' ); + + $result = $this->recommendations->complete( [ 'provider_id' => 'hello-world' ] ); + + $this->assertTrue( $result['applied'] ); + $this->assertSame( 'trash', \get_post_status( $post_id ), 'The post should be recoverable from the trash.' ); + } + + /** + * Test that next mode never picks a destructive fix. + * + * @return void + */ + public function test_next_mode_never_deletes() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $post_id = self::factory()->post->create( + [ + 'post_title' => 'Hello world!', + 'post_name' => 'hello-world', + 'post_status' => 'publish', + ] + ); + + $this->seed_task( 'hello-world' ); + + $result = $this->recommendations->complete( [] ); + + if ( null !== $result['task'] ) { + $this->assertNotSame( 'hello-world', $result['task']['provider_id'] ); + } + + $this->assertSame( 'publish', \get_post_status( $post_id ), 'Next mode must not trash anything.' ); + } + + /** + * Test that a deletion reports missing content rather than failing oddly. + * + * @return void + */ + public function test_deletion_reports_missing_target() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $result = Recommendation_Fixes::apply( 'hello-world' ); + + if ( \is_wp_error( $result ) ) { + $this->assertSame( 'progress_planner_no_target', $result->get_error_code() ); + } else { + $this->assertTrue( $result ); + } + } } From 4b10ec52c0c86467e38123268205b9c2461c4f23 Mon Sep 17 00:00:00 2001 From: Filip Ilic Date: Fri, 18 Sep 2026 16:21:28 +0200 Subject: [PATCH 3/3] Let a caller record which page serves the About, Contact or FAQ role Brings the applicable set to 22. These recommendations do not create a page. They ask whether the site has one, and record the answer plus which page it is. The interesting part is who decides. The plugin deliberately does not search for the page. Searching a real site for "about" returns everything whose content mentions the word: on the demo site that is eleven pages, including one called "Job opening", with the actual About page ranked ninth and titled "About Emilia" at /about-us/. Deciding which of those is the About page means weighing title, slug, hierarchy and navigation in whatever language the site is written in. A caller that can read the site's pages does that well; a title match in PHP would get it wrong quietly, and worse on non-English sites. So the division of labour is: the caller identifies the page, and this verifies the ID before writing. A missing ID, an ID that does not exist, a draft, or a post that is not a page are each a distinct error rather than a silent no-op, because "we recorded your About page" is worth being true. The page type comes from the table and never from the caller, and the allowed post types are derived from the hierarchical public ones rather than hardcoded to 'page', so a site serving these roles from a custom post type still works. They need a value, so next-mode skips them: there is no correct page to choose on the owner's behalf. One caveat is recorded in is_satisfied(). These providers do not override is_task_completed(), so satisfaction now falls back to should_add_task(). That can still read pre-write state within the same request, because page-type lookups are memoised in a static cache with no invalidation -- which is why the status distinguishes "applied" from "satisfied" instead of assuming they are the same. Co-Authored-By: Claude Opus 5 (1M context) --- .../abilities/class-recommendation-fixes.php | 116 ++++++++++++++++++ classes/abilities/class-recommendations.php | 21 +++- classes/abilities/class-schemas.php | 2 +- tests/phpunit/test-class-abilities.php | 90 ++++++++++++++ 4 files changed, 226 insertions(+), 3 deletions(-) diff --git a/classes/abilities/class-recommendation-fixes.php b/classes/abilities/class-recommendation-fixes.php index 255fef69b..9c1826ffc 100644 --- a/classes/abilities/class-recommendation-fixes.php +++ b/classes/abilities/class-recommendation-fixes.php @@ -149,6 +149,39 @@ class Recommendation_Fixes { 'summary' => 'Disable author feeds in All in One SEO.', ], + /* + * Telling the plugin which existing page serves a given role. + * + * These do not create anything. The recommendation asks whether the site + * has an About page; this records the answer and, when the answer is + * yes, which page it is. + * + * The plugin deliberately does not try to find the page itself. Deciding + * that "About Emilia" at /about-us/ is the About page -- and that + * "Job opening" is not -- is a judgement about titles, slugs, navigation + * and content in whatever language the site is written in. A caller that + * can read the site's pages is far better at that than a title match + * would be, and a wrong guess made in PHP would be silent. + * + * So the division is: the caller identifies the page, and this verifies + * the ID names a real published page of an allowed type before writing. + */ + 'set-page-about' => [ + 'page_type' => 'about', + 'input' => 'value', + 'summary' => 'Record which existing page is the About page.', + ], + 'set-page-contact' => [ + 'page_type' => 'contact', + 'input' => 'value', + 'summary' => 'Record which existing page is the Contact page.', + ], + 'set-page-faq' => [ + 'page_type' => 'faq', + 'input' => 'value', + 'summary' => 'Record which existing page is the FAQ page.', + ], + /* * Deleting WordPress's own placeholder content. * @@ -256,6 +289,10 @@ public static function apply( $provider_id, $value = null ) { ); } + if ( isset( $fix['page_type'] ) ) { + return self::apply_page_type( $fix, $value ); + } + if ( isset( $fix['delete'] ) ) { return self::apply_deletion( $provider_id ); } @@ -281,6 +318,85 @@ public static function apply( $provider_id, $value = null ) { return true; } + /** + * Record which page serves a given role. + * + * The page type comes from the table; only the page ID comes from the + * caller, and it is checked before anything is written: it must name a post + * that exists, is published, and is of a post type the site treats as a + * page. An ID that does not pass is an error rather than a silent no-op, + * because "we recorded your About page" is worth being true. + * + * @param array $fix The fix definition. + * @param mixed $value The page ID supplied by the caller. + * + * @return true|\WP_Error + */ + private static function apply_page_type( array $fix, $value ) { + $page_id = \is_numeric( $value ) ? (int) $value : 0; + + if ( 1 > $page_id ) { + return new \WP_Error( + 'progress_planner_missing_page_id', + \__( 'This recommendation needs the ID of the page that serves this role.', 'progress-planner' ) + ); + } + + $page = \get_post( $page_id ); + + if ( ! $page ) { + return new \WP_Error( + 'progress_planner_no_such_page', + \__( 'There is no page with that ID.', 'progress-planner' ) + ); + } + + if ( 'publish' !== $page->post_status ) { + return new \WP_Error( + 'progress_planner_page_not_published', + \__( 'That page is not published, so it cannot serve this role yet.', 'progress-planner' ) + ); + } + + if ( ! \in_array( $page->post_type, self::page_post_types(), true ) ) { + return new \WP_Error( + 'progress_planner_not_a_page', + \__( 'That post is not a page.', 'progress-planner' ) + ); + } + + \progress_planner()->get_admin__page_settings()->set_page_values( + [ + (string) $fix['page_type'] => [ + 'id' => $page_id, + 'have_page' => 'yes', + ], + ] + ); + + return true; + } + + /** + * The post types that can serve a page role. + * + * Hierarchical public post types, which is what WordPress means by a page, + * rather than a hardcoded 'page' -- a site may serve these roles from a + * custom post type. + * + * @return array + */ + private static function page_post_types() { + $types = \get_post_types( + [ + 'public' => true, + 'hierarchical' => true, + ] + ); + + return \array_values( $types ); + } + /** * Move a placeholder post to the trash. * diff --git a/classes/abilities/class-recommendations.php b/classes/abilities/class-recommendations.php index 838985374..cd8a90570 100644 --- a/classes/abilities/class-recommendations.php +++ b/classes/abilities/class-recommendations.php @@ -185,8 +185,25 @@ private function result( $applied, $status, $message, $task ) { * @return bool */ private function is_satisfied( $provider, $task ) { - return \method_exists( $provider, 'is_task_completed' ) - && (bool) $provider->is_task_completed( $task->get_task_id() ); + if ( \method_exists( $provider, 'is_task_completed' ) + && (bool) $provider->is_task_completed( $task->get_task_id() ) + ) { + return true; + } + + // Some providers answer through should_add_task() instead: the task + // exists precisely while the condition is unmet, so "would not be added + // now" means satisfied. The set-page providers are the case in point -- + // they do not override is_task_completed() at all. + // + // This can still report false immediately after a write. Page-type + // lookups are memoised in a static cache with no invalidation, so within + // one request the check may read state from before the change. That is + // why the status distinguishes "applied" from "satisfied" rather than + // assuming the two are the same: the next request sees it correctly, and + // a caller is told what actually happened either way. + return \method_exists( $provider, 'should_add_task' ) + && false === (bool) $provider->should_add_task(); } /** diff --git a/classes/abilities/class-schemas.php b/classes/abilities/class-schemas.php index e9ed0aeae..d90809a8e 100644 --- a/classes/abilities/class-schemas.php +++ b/classes/abilities/class-schemas.php @@ -75,7 +75,7 @@ public static function complete_recommendation_input() { ], 'value' => [ 'type' => 'string', - 'description' => \__( 'The value to set, for recommendations that need one: the tagline text, a timezone identifier such as "Europe/Amsterdam", or a date format string. Recommendations with only one correct outcome ignore this.', 'progress-planner' ), + 'description' => \__( 'The value to set, for recommendations that need one: the tagline text, a timezone identifier such as "Europe/Amsterdam", a date format string, or -- for the recommendations that ask which page serves a role -- the ID of an existing published page. Recommendations with only one correct outcome ignore this. Check needs_value on a recommendation to see whether one is required.', 'progress-planner' ), ], ], ]; diff --git a/tests/phpunit/test-class-abilities.php b/tests/phpunit/test-class-abilities.php index 2482bcc19..59f68e78e 100644 --- a/tests/phpunit/test-class-abilities.php +++ b/tests/phpunit/test-class-abilities.php @@ -774,4 +774,94 @@ public function test_deletion_reports_missing_target() { $this->assertTrue( $result ); } } + + /** + * Test that the page-role recommendations need a page ID. + * + * @return void + */ + public function test_page_role_fixes_need_a_value() { + foreach ( [ 'set-page-about', 'set-page-contact', 'set-page-faq' ] as $provider_id ) { + $this->assertTrue( Recommendation_Fixes::has_fix( $provider_id ) ); + $this->assertTrue( Recommendation_Fixes::needs_value( $provider_id ) ); + $this->assertFalse( Recommendation_Fixes::is_destructive( $provider_id ) ); + } + } + + /** + * Test that a valid page is recorded as serving the role. + * + * @return void + */ + public function test_page_role_records_the_page() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $page_id = self::factory()->post->create( + [ + 'post_title' => 'About Us', + 'post_type' => 'page', + 'post_status' => 'publish', + ] + ); + + $this->assertTrue( Recommendation_Fixes::apply( 'set-page-about', (string) $page_id ) ); + + $slugs = \wp_get_object_terms( $page_id, 'progress_planner_page_types', [ 'fields' => 'slugs' ] ); + + $this->assertContains( 'about', (array) $slugs ); + } + + /** + * Test that an unusable page ID is refused rather than recorded. + * + * "We recorded your About page" is worth being true, so each of these is an + * error rather than a silent no-op. + * + * @return void + */ + public function test_page_role_refuses_unusable_ids() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + + $cases = [ + 'progress_planner_missing_page_id' => null, + 'progress_planner_no_such_page' => '99999999', + 'progress_planner_page_not_published' => (string) self::factory()->post->create( + [ + 'post_type' => 'page', + 'post_status' => 'draft', + ] + ), + 'progress_planner_not_a_page' => (string) self::factory()->post->create( + [ + 'post_type' => 'post', + 'post_status' => 'publish', + ] + ), + ]; + + foreach ( $cases as $expected_code => $value ) { + $result = Recommendation_Fixes::apply( 'set-page-about', $value ); + + $this->assertWPError( $result ); + $this->assertSame( $expected_code, $result->get_error_code() ); + } + } + + /** + * Test that next mode never picks a page-role fix. + * + * There is no correct page to choose on the owner's behalf. + * + * @return void + */ + public function test_next_mode_skips_page_role_fixes() { + \wp_set_current_user( self::factory()->user->create( [ 'role' => 'administrator' ] ) ); + $this->seed_task( 'set-page-about' ); + + $result = $this->recommendations->complete( [] ); + + $picked = $result['task']['provider_id'] ?? null; + + $this->assertNotSame( 'set-page-about', $picked ); + } }