update CappedPostRanker constructor args to pass optional params last (#42)

Co-authored-by: lucilastancato <lucilastancato@automattic.com>

Author: lucila

.github/workflows/mt.yml

@@ -55,4 +55,4 @@ jobs:
         if: github.event_name == 'pull_request'
         run: |
           git fetch --depth=1 origin $GITHUB_BASE_REF
-          vendor/bin/infection --coverage=build/logs --threads=$(nproc) --show-mutations --no-interaction --only-covered --only-covering-test-cases --skip-initial-tests --git-diff-lines --git-diff-base=origin/$GITHUB_BASE_REF
+          vendor/bin/infection --coverage=build/logs --threads=$(nproc) --show-mutations --no-interaction --only-covering-test-cases --only-covering-test-cases --skip-initial-tests --git-diff-lines --git-diff-base=origin/$GITHUB_BASE_REF

Makefile

@@ -35,7 +35,7 @@ COMPOSER=$(PHP) $(shell which composer)
 INFECTION=vendor/bin/infection
 MIN_MSI=50
 MIN_COVERED_MSI=50
-INFECTION_ARGS=--min-msi=$(MIN_MSI) --min-covered-msi=$(MIN_COVERED_MSI) --threads=$(JOBS) --coverage=build/logs --show-mutations --no-interaction --only-covered --only-covering-test-cases
+INFECTION_ARGS=--min-msi=$(MIN_MSI) --min-covered-msi=$(MIN_COVERED_MSI) --threads=$(JOBS) --coverage=build/logs --show-mutations --no-interaction --only-covering-test-cases --only-covering-test-cases
 
 all: test
 

lib/Tumblr/StreamBuilder/StreamRankers/CappedPostRanker.php

@@ -61,20 +61,20 @@ class CappedPostRanker extends StreamRanker
      * CappedPostRanker constructor.
      * @param User $user User
      * @param string $identity The identity of this ranker
-     * @param bool $debug Need extra debug infos or not. e.g. Ranking score
      * @param bool $cap_desc Whether we are looking for the first violated blog post in descending order (top-down)
      * @param int $cap The cap applied (higher values are making the reranking less strict)
      * @param string $ranking_context The context that ranking is being applied to e.g. dashboard
      * @param bool $panel_allow_ranking Whether the meta key on panel regarding ranking is enabled or not
+     * @param bool $debug Need extra debug infos or not. e.g. Ranking score
      */
     public function __construct(
         User $user,
         string $identity,
-        ?bool $debug = null,
         bool $cap_desc,
         int $cap,
         string $ranking_context,
-        bool $panel_allow_ranking
+        bool $panel_allow_ranking,
+        ?bool $debug = null
     ) {
         parent::__construct($identity);
         $this->user = $user;
@@ -115,11 +115,11 @@ public static function from_template(StreamContext $context)
         return new self(
             $context->get_meta_by_key('user'),
             $context->get_current_identity(),
-            $context->get_meta_by_key('client_meta')->is_panel(),
             $context->get_optional_property('cap_desc', true),
             $context->get_optional_property('cap', 2),
             $context->get_optional_property('ranking_context', 'dashboard'),
             $context->get_meta_by_key('allow_ranking'),
+            $context->get_meta_by_key('client_meta')->is_panel()
         );
     }
 

tests/unit/Tumblr/StreamBuilder/StreamRankers/CappedPostRankerTest.php

@@ -76,12 +76,12 @@ protected function setUp(): void
         $this->invalid_stream_elements = $this->setup_capped_invalid_stream_elements($blog_ids);
 
         $this->enabled_ranker_instance = $this->getMockBuilder(CappedPostRanker::class)
-            ->setConstructorArgs([$this->mock_user, $this->identity, false, true, 2, 'dashboard', false])
+            ->setConstructorArgs([$this->mock_user, $this->identity, true, 2, 'dashboard', false, false])
             ->getMock();
         $this->enabled_ranker_instance->method('can_rank')->willReturn(true);
 
         $this->disabled_ranker_instance = $this->getMockBuilder(CappedPostRanker::class)
-            ->setConstructorArgs([$this->mock_user, $this->identity, false, true, 2, 'dashboard', false])
+            ->setConstructorArgs([$this->mock_user, $this->identity, true, 2, 'dashboard', false, false])
             ->getMock();
         $this->disabled_ranker_instance->method('can_rank')->willReturn(false);
     }
@@ -165,7 +165,7 @@ public function test_disabled_rank(): void
     public function test_invalid_elements(): void
     {
         $this->expectException(TypeMismatchException::class);
-        $this->invokeMethod($this->enabled_ranker_instance, 'pre_fetch', $this->invalid_stream_elements);
+        $this->invokeMethod($this->enabled_ranker_instance, 'pre_fetch', [$this->invalid_stream_elements]);
     }
 
     /**
@@ -176,7 +176,7 @@ public function test_get_blog_dictionaries(): void
         $dictionaries = $this->invokeMethod(
             $this->enabled_ranker_instance,
             'get_blog_dictionaries',
-            $this->stream_elements
+            [$this->stream_elements]
         );
         $post_to_element = $dictionaries[0];
         $blog_to_posts_ids = $dictionaries[1];
@@ -186,28 +186,101 @@ public function test_get_blog_dictionaries(): void
             $this->assertSame($post_id, $original_element->get_post_id());
         }
         # Which posts belong to which blogs
-        $expected_blog_to_posts_ids = [111 => [1, 2, 3, 4, 5], 222 => [6, 7], 333 => [8]];
-        $this->assertEquals($blog_to_posts_ids, $expected_blog_to_posts_ids);
+        $expected_blog_to_posts_ids = [111 => ['1', '2', '3', '4', '5'], 222 => ['6', '7'], 333 => ['8']];
+        $this->assertSame($expected_blog_to_posts_ids, $blog_to_posts_ids);
         $expected_stats_per_blog = [111 => [5, 0], 222 => [2, 0], 333 => [1, 0]];
-        $this->assertEquals($stats_per_blog, $expected_stats_per_blog);
+        $this->assertSame($expected_stats_per_blog, $stats_per_blog);
+    }
+
+    /**
+     * Test that invalidate_post_id properly removes posts from blog_to_posts_ids
+     */
+    public function test_invalidate_post_id_removes_posts(): void
+    {
+        $ranker = new CappedPostRanker($this->mock_user, $this->identity, true, 2, 'dashboard', false);
+
+        // Get initial blog dictionaries using reflection
+        $dictionaries = $this->invokeMethod($ranker, 'get_blog_dictionaries', [$this->stream_elements]);
+        $blog_to_posts_ids = $dictionaries[1];
+
+        // Verify initial state
+        $expected_initial = [111 => ['1', '2', '3', '4', '5'], 222 => ['6', '7'], 333 => ['8']];
+        $expected_post_counts = [111 => [5, 0], 222 => [2, 0], 333 => [1, 0]];
+        $this->assertSame($expected_initial, $blog_to_posts_ids);
+        $this->assertSame($expected_post_counts, $dictionaries[2]);
+
+        // Test invalidating a specific post using reflection
+        $violated_post_id = $this->invokeMethod($ranker, 'invalidate_post_id', ['111', '3', &$blog_to_posts_ids]);
+        // The method returns null when a specific post ID is provided, but the post should be removed
+        $this->assertNull($violated_post_id);
+
+        // Verify post was removed from blog 111
+        $this->assertCount(4, $blog_to_posts_ids[111]); // Should have 4 posts instead of 5
+        $this->assertContains('1', $blog_to_posts_ids[111]);
+        $this->assertContains('2', $blog_to_posts_ids[111]);
+        $this->assertContains('4', $blog_to_posts_ids[111]);
+        $this->assertContains('5', $blog_to_posts_ids[111]);
+    }
+
+    /**
+     * Test that invalidate_post_id returns the first available post when no specific post is provided
+     */
+    public function test_invalidate_post_id_returns_first_available(): void
+    {
+        $ranker = new CappedPostRanker($this->mock_user, $this->identity, true, 2, 'dashboard', false);
+
+        $dictionaries = $this->invokeMethod($ranker, 'get_blog_dictionaries', [$this->stream_elements]);
+        $blog_to_posts_ids = $dictionaries[1];
+
+        // Test getting first available post from blog 111 using reflection
+        $first_post = $this->invokeMethod($ranker, 'invalidate_post_id', ['111', null, &$blog_to_posts_ids]);
+        $this->assertSame(1, $first_post); // Returns integer, not string
+
+        // Verify first post was removed
+        $this->assertNotContains('1', $blog_to_posts_ids[111]);
+        $this->assertCount(4, $blog_to_posts_ids[111]);
+        $this->assertContains('2', $blog_to_posts_ids[111]);
+        $this->assertContains('3', $blog_to_posts_ids[111]);
+        $this->assertContains('4', $blog_to_posts_ids[111]);
+        $this->assertContains('5', $blog_to_posts_ids[111]);
+    }
+
+    /**
+     * Test that invalidate_post_id handles non-existent posts gracefully
+     */
+    public function test_invalidate_post_id_nonexistent_post(): void
+    {
+        $ranker = new CappedPostRanker($this->mock_user, $this->identity, true, 2, 'dashboard', false, false);
+
+        $dictionaries = $this->invokeMethod($ranker, 'get_blog_dictionaries', [$this->stream_elements]);
+        $blog_to_posts_ids = $dictionaries[1];
+
+        // Try to invalidate a non-existent post using reflection
+        $result = $this->invokeMethod($ranker, 'invalidate_post_id', ['111', '999', &$blog_to_posts_ids]);
+        $this->assertNull($result);
+
+        // Verify no changes were made
+        $this->assertCount(5, $blog_to_posts_ids[111]);
+        $this->assertContains('1', $blog_to_posts_ids[111]);
+        $this->assertContains('2', $blog_to_posts_ids[111]);
+        $this->assertContains('3', $blog_to_posts_ids[111]);
+        $this->assertContains('4', $blog_to_posts_ids[111]);
+        $this->assertContains('5', $blog_to_posts_ids[111]);
     }
 
     /**
      * Helper method to enable testing private and protected methods
-     * @param object $object The object that the private method belongs to
+     * @param CappedPostRanker $object The object that the private method belongs to
      * @param string $method_name The name of the method we want to test
      * @param array $parameters List of parameters passed to the method
      * @return mixed Whatever the method would return
      * @throws \ReflectionException Something went wrong with reflection
      */
-    public function invokeMethod(object &$object, string $method_name, array $parameters = [])
+    public function invokeMethod(CappedPostRanker &$object, string $method_name, array $parameters = [])
     {
-        $reflection = new \ReflectionClass(get_class($object));
+        $reflection = new \ReflectionClass(CappedPostRanker::class);
         $method = $reflection->getMethod($method_name);
         $method->setAccessible(true);
-        if ($method_name = 'get_blog_dictionaries') {
-            $parameters = [$parameters];
-        }
         return $method->invokeArgs($object, $parameters);
     }
 }