From 0dd7d4aad74b4f7a6041471087d7c8e1b4931248 Mon Sep 17 00:00:00 2001
From: Nikolay Strikhar <nikolay@learndash.com>
Date: Thu, 13 Aug 2026 15:38:18 +0200
Subject: [PATCH] Prove the notice was there before asserting it is gone

Three clear-then-assertFalse tests never established the row existed, so a
store that wrote nothing passed them. The renderer's not-called assertion had
its control in a sibling test. The replacement-store test overrode all(), put()
and clear() but not option_name(), so a writer composing the name itself --
instead of delegating, which is the documented seam -- passed.

Adds the multisite scope neither option had: the queue is written, a second
site is created, and switch_to_blog() finds it still there. Swapping the store
to get_option()/update_option() now fails the multisite leg, where before it
was green on both.

Also pins that the capability is checked before the store is read. The check
guards the clearing as much as the drawing, and nothing said so.
---
 tests/unit/Notices/PresenterTest.php |  74 +++++++++++++++-
 tests/unit/Notices/RendererTest.php  |  22 +++++
 tests/unit/Notices/StoreTest.php     | 123 +++++++++++++++++++++++++++
 tests/unit/Notices/WriterTest.php    |  38 ++++++++-
 4 files changed, 254 insertions(+), 3 deletions(-)

diff --git a/tests/unit/Notices/PresenterTest.php b/tests/unit/Notices/PresenterTest.php
index 0348c57..4bb9c2d 100644
--- a/tests/unit/Notices/PresenterTest.php
+++ b/tests/unit/Notices/PresenterTest.php
@@ -132,6 +132,11 @@ public function test_render_keeps_a_link_but_not_an_event_handler(): void {
 	public function test_render_clears_the_queue(): void {
 		$this->queue_notice( 'queue_merge_notice' );
 
+		// The queue has to be there before rendering can be what took it away: a writer that never
+		// wrote, or an option name the two halves disagree about, would satisfy the assertion below
+		// without a presenter having cleared anything.
+		$this->assertTrue( $this->queue_exists(), 'The queue must exist before it is rendered.' );
+
 		$presenter = $this->make_presenter();
 		$this->render_to_string( $presenter );
 
@@ -202,16 +207,83 @@ public function render( array $queue ): void {
 			}
 		};
 
+		$presenter = $this->make_presenter( null, $renderer );
+
+		// The control belongs in this test rather than in a sibling: a presenter that reached no
+		// renderer at all, or a renderer this one never received, would satisfy the assertion at the
+		// end for a reason that has nothing to do with the capability.
+		$this->queue_notice( 'queue_merge_notice' );
+
+		$this->render_to_string( $presenter );
+
+		$this->assertTrue( $renderer->called, 'This renderer must be reachable for someone who may consume the queue.' );
+
+		$renderer->called = false;
+
 		$this->queue_notice( 'queue_merge_notice' );
 
 		wp_set_current_user( $this->create_user( 'subscriber' ) );
 
-		$this->render_to_string( $this->make_presenter( null, $renderer ) );
+		$this->render_to_string( $presenter );
 
 		$this->assertFalse( $renderer->called );
 		$this->assertTrue( $this->queue_exists() );
 	}
 
+	/**
+	 * The capability is asked before the queue is read, not after.
+	 *
+	 * The gate gets its authority from being in front of both halves: reading is what the clearing
+	 * follows from, so a presenter that read the queue and then decided who may see it would already
+	 * have taken the notice out of the store by the time it turned the subscriber away. Asserting on
+	 * the output alone cannot see that difference, so the store here counts the calls it receives.
+	 */
+	public function test_the_capability_is_checked_before_the_queue_is_read(): void {
+		$store = new class() extends Store {
+			/**
+			 * @var int
+			 */
+			public $reads = 0;
+
+			/**
+			 * @var int
+			 */
+			public $clears = 0;
+
+			/**
+			 * @return array<string,string>
+			 */
+			public function all(): array {
+				++$this->reads;
+
+				return [ 'give-recurring:merge' => 'Bundled now.' ];
+			}
+
+			/**
+			 * @return void
+			 */
+			public function clear(): void {
+				++$this->clears;
+			}
+		};
+
+		$presenter = $this->make_presenter( $store );
+
+		wp_set_current_user( $this->create_user( 'subscriber' ) );
+
+		$this->assertSame( '', $this->render_to_string( $presenter ) );
+		$this->assertSame( 0, $store->reads, 'The queue must not be read at all for a user who may not consume it.' );
+		$this->assertSame( 0, $store->clears, 'And it must certainly not be cleared.' );
+
+		// The recorder has to be shown working, or "never read" and "never wired to anything" are the
+		// same result: the same store, in the same presenter, for a user who does have the capability.
+		$this->become_plugin_administrator();
+
+		$this->assertStringContainsString( 'Bundled now.', $this->render_to_string( $presenter ) );
+		$this->assertSame( 1, $store->reads );
+		$this->assertSame( 1, $store->clears );
+	}
+
 	/**
 	 * Rendering consumes the queue, so a user who cannot act on the notice must neither see it
 	 * nor destroy it. The merge notice is raised once and never re-queued.
diff --git a/tests/unit/Notices/RendererTest.php b/tests/unit/Notices/RendererTest.php
index a1d516a..04f69a7 100644
--- a/tests/unit/Notices/RendererTest.php
+++ b/tests/unit/Notices/RendererTest.php
@@ -3,6 +3,8 @@
  * @package Nexcess\PluginAbsorber
  */
 
+declare( strict_types=1 );
+
 namespace Nexcess\PluginAbsorber\Tests\Unit\Notices;
 
 use Codeception\TestCase\WPTestCase;
@@ -163,6 +165,26 @@ public static function empty_messages(): Generator {
 		yield 'a message that is only disallowed markup' => [ '<script></script>' ];
 	}
 
+	/**
+	 * Skipping is per message, not for the rest of the queue: an entry a host left empty — or one
+	 * `wp_kses_post()` emptied for it — must not take the notices behind it off the screen with it.
+	 * That is the difference between a `continue` and a `return`, and every case above holds a single
+	 * message, so none of them can see it.
+	 */
+	public function test_an_empty_message_does_not_stop_the_ones_behind_it(): void {
+		$output = $this->render(
+			[
+				'a:merge'      => 'First.',
+				'b:merge'      => '<script></script>',
+				'c:dependency' => 'Second.',
+			]
+		);
+
+		$this->assertStringContainsString( 'First.', $output );
+		$this->assertStringContainsString( 'Second.', $output );
+		$this->assertSame( 2, substr_count( $output, '<div class="notice' ), 'Two messages in, two notices out.' );
+	}
+
 	public function test_an_empty_queue_prints_nothing(): void {
 		$this->assertSame( '', $this->render( [] ) );
 	}
diff --git a/tests/unit/Notices/StoreTest.php b/tests/unit/Notices/StoreTest.php
index 2e5f32b..274f73a 100644
--- a/tests/unit/Notices/StoreTest.php
+++ b/tests/unit/Notices/StoreTest.php
@@ -3,6 +3,8 @@
  * @package Nexcess\PluginAbsorber
  */
 
+declare( strict_types=1 );
+
 namespace Nexcess\PluginAbsorber\Tests\Unit\Notices;
 
 use Codeception\TestCase\WPTestCase;
@@ -11,6 +13,9 @@
 use Nexcess\PluginAbsorber\Exceptions\Config_Exception;
 use Nexcess\PluginAbsorber\Notices\Store;
 use Nexcess\PluginAbsorber\Tests\Support\Config_State;
+use RuntimeException;
+use WP_Error;
+use WP_Network;
 
 /**
  * The storage half of the queue, exercised without going through Queue.
@@ -28,6 +33,13 @@ class StoreTest extends WPTestCase {
 
 	private const OPTION_NORMALISED = 'give_core_plugin_absorber_notices';
 
+	/**
+	 * The second site the network-scope test reads the queue from, once it has one.
+	 *
+	 * @var int|null
+	 */
+	private $second_site_id = null;
+
 	public function setUp(): void {
 		parent::setUp();
 
@@ -37,6 +49,7 @@ public function setUp(): void {
 	}
 
 	public function tearDown(): void {
+		$this->delete_second_site();
 		delete_site_option( self::OPTION );
 		delete_site_option( self::OPTION_WOO );
 		delete_site_option( self::OPTION_NORMALISED );
@@ -83,6 +96,10 @@ public function test_clear_removes_the_row_entirely(): void {
 		$store = new Store();
 		$store->put( 'give-recurring:merge', 'Bundled now.' );
 
+		// The row has to be there before it can be taken away: a store that wrote nothing at all
+		// would satisfy every assertion below without ever having cleared anything.
+		$this->assertNotFalse( get_site_option( self::OPTION, false ), 'The queue must exist before it is cleared.' );
+
 		$store->clear();
 
 		$this->assertFalse( get_site_option( self::OPTION, false ), 'The option must be gone, not emptied.' );
@@ -179,4 +196,110 @@ public function test_the_queue_is_not_autoloaded(): void {
 
 		$this->assertNotContains( self::OPTION, array_keys( wp_load_alloptions() ) );
 	}
+
+	/**
+	 * The queue is one option for the whole network, not one per site.
+	 *
+	 * `get_site_option()`/`update_site_option()` rather than the plain pair is the only thing making
+	 * that true, and outside multisite the two are the same function — so nothing on the singlesite
+	 * leg can tell them apart, and swapping them would leave every other test in this class green.
+	 * The scope has to match the act it reports: `deactivate_plugins()` takes the standalone out of
+	 * the *network's* active plugins, and the merge notice explaining that is raised exactly once, so
+	 * a per-site queue would file it against whichever site the request happened to land on and no
+	 * administrator elsewhere would ever be told.
+	 */
+	public function test_the_queue_is_one_option_for_the_whole_network(): void {
+		if ( ! is_multisite() ) {
+			$this->markTestSkipped( 'Outside multisite there is one site, so there is no scope to cross.' );
+		}
+
+		( new Store() )->put( 'give-recurring:merge', 'Bundled now.' );
+
+		$this->assertSame(
+			[ 'give-recurring:merge' => 'Bundled now.' ],
+			( new Store() )->all(),
+			'The queue must be readable where it was written, or reading it elsewhere proves nothing.'
+		);
+
+		$this->second_site_id = $this->create_second_site();
+
+		switch_to_blog( $this->second_site_id );
+
+		try {
+			$elsewhere = ( new Store() )->all();
+		} finally {
+			// In a finally block so a read that throws cannot leave the rest of the suite running
+			// against the second site.
+			restore_current_blog();
+		}
+
+		$this->assertSame(
+			[ 'give-recurring:merge' => 'Bundled now.' ],
+			$elsewhere,
+			'The queue must reach every site on the network, not only the one that wrote it.'
+		);
+	}
+
+	/**
+	 * A second site on the current network, created with the same domain and a path of its own so
+	 * that it is unique on a subdomain install and a subdirectory one alike.
+	 *
+	 * @throws RuntimeException When there is no network, or the site cannot be created.
+	 *
+	 * @return int
+	 */
+	private function create_second_site(): int {
+		$network = get_network();
+
+		if ( ! $network instanceof WP_Network ) {
+			throw new RuntimeException( 'The multisite env has no current network.' );
+		}
+
+		// Creating a site runs core's populate_options(), which calls delete_expired_transients(), whose
+		// DELETE self-joins the options table under two aliases. The suite runs inside a transaction on
+		// TEMPORARY tables, and MySQL cannot open one of those twice in a statement -- so the query
+		// fails, harmlessly, on a site that has no transients to expire. Suppressed for the one call
+		// rather than left to print a WordPress database error into every CI log.
+		global $wpdb;
+
+		$suppressing = $wpdb->suppress_errors( true );
+
+		$site_id = wpmu_create_blog(
+			$network->domain,
+			$network->path . 'absorber-queue-scope/',
+			'Queue scope',
+			0,
+			[],
+			get_current_network_id()
+		);
+
+		$wpdb->suppress_errors( $suppressing );
+
+		// `wpmu_create_blog()` puts WordPress into installing mode and never takes it back out, so
+		// without this every later test in the process runs as though the site were mid-install.
+		// Core's own blog factory ends with the same line, for the same reason.
+		wp_installing( false );
+
+		if ( $site_id instanceof WP_Error ) {
+			throw new RuntimeException( 'Could not create a second site: ' . $site_id->get_error_message() );
+		}
+
+		return $site_id;
+	}
+
+	/**
+	 * Creating a site is DDL, which MySQL commits outside the transaction the suite rolls back, so
+	 * the tables it made have to be dropped by hand rather than left to the rollback.
+	 *
+	 * @return void
+	 */
+	private function delete_second_site(): void {
+		if ( $this->second_site_id === null ) {
+			return;
+		}
+
+		wp_delete_site( $this->second_site_id );
+
+		$this->second_site_id = null;
+	}
 }
diff --git a/tests/unit/Notices/WriterTest.php b/tests/unit/Notices/WriterTest.php
index 602a202..73ee61d 100644
--- a/tests/unit/Notices/WriterTest.php
+++ b/tests/unit/Notices/WriterTest.php
@@ -207,6 +207,11 @@ public function test_different_slugs_do_not_collide(): void {
 	 * Where notices are kept is a constructor argument, so a host can move the queue somewhere else
 	 * without also taking on how notices are worded. It is a required argument and it is bound by
 	 * `Provider`, so a host rebinds the store rather than subclassing the writer.
+	 *
+	 * The double answers `option_name()` too, and the writer is asked for it here, because that is
+	 * the whole of what a host reading the queue itself has to go on. A writer composing the name
+	 * from the hook prefix instead of delegating would agree with the default store on every other
+	 * test in this class and send that host to an option nothing writes.
 	 */
 	public function test_a_replacement_store_is_used_instead_of_the_option(): void {
 		$store = new class() extends Store {
@@ -215,6 +220,20 @@ public function test_a_replacement_store_is_used_instead_of_the_option(): void {
 			 */
 			public $written = [];
 
+			/**
+			 * Deliberately nothing the hook prefix could produce.
+			 *
+			 * @var string
+			 */
+			public $name = 'host_managed_notice_queue';
+
+			/**
+			 * @return string
+			 */
+			public function option_name(): string {
+				return $this->name;
+			}
+
 			/**
 			 * @return array<string,string>
 			 */
@@ -240,12 +259,18 @@ public function clear(): void {
 			}
 		};
 
-		$this->make_writer( $store )->queue_merge_notice(
+		$writer = $this->make_writer( $store );
+		$writer->queue_merge_notice(
 			$this->make_sub_plugin( [ 'conflict_notice_message' => static fn() => 'Bundled now.' ] )
 		);
 
 		$this->assertSame( [ 'give-recurring:merge' => 'Bundled now.' ], $store->written );
 		$this->assertFalse( $this->queue_exists(), 'The default option must not have been written to.' );
+		$this->assertSame(
+			$store->name,
+			$writer->option_name(),
+			'The writer must report where its store keeps the queue, not compose a name of its own.'
+		);
 	}
 
 	/**
@@ -280,6 +305,15 @@ public function test_a_corrupted_queue_heals_on_the_next_write(): void {
 	}
 
 	public function test_the_option_is_keyed_by_the_hook_prefix(): void {
+		// Under the host's own prefix the queue lands in the option this class asserts against. Shown
+		// first, and then taken away again, because otherwise its absence at the end of the test says
+		// only that setUp deleted it — which is true of an option nothing ever writes.
+		$this->make_writer()->queue_merge_notice( $this->make_sub_plugin() );
+
+		$this->assertTrue( $this->queue_exists(), 'The give-prefixed option is where this writer writes.' );
+
+		$this->clear_queue();
+
 		Config_State::reset();
 		Config::set_hook_prefix( 'woo' );
 
@@ -288,7 +322,7 @@ public function test_the_option_is_keyed_by_the_hook_prefix(): void {
 		$this->make_writer()->queue_merge_notice( $this->make_sub_plugin() );
 
 		$this->assertIsArray( get_site_option( self::OPTION_FOR_OTHER_PREFIX, false ) );
-		$this->assertFalse( $this->queue_exists() );
+		$this->assertFalse( $this->queue_exists(), 'Nothing may be left under the previous prefix.' );
 	}
 
 	public function test_queueing_needs_a_hook_prefix(): void {