fix(sticky): filter mutator union takes order by out of its clause caung trouble

Signed-off-by: Sami Mazouz <ilyasmazouz@gmail.com>

.github/workflows/flarum-sticky-backend.yml

@@ -6,6 +6,6 @@ jobs:
   run:
     uses: ./.github/workflows/REUSABLE_backend.yml
     with:
-      enable_backend_testing: false
+      enable_backend_testing: true
 
       backend_directory: ./extensions/sticky

composer.json

@@ -86,7 +86,6 @@
     "require": {
         "ext-json": "*",
         "php": ">=7.3",
-        "axy/sourcemap": "^0.1.4",
         "components/font-awesome": "^5.14.0",
         "composer/composer": "^2.0",
         "dflydev/fig-cookies": "^3.0.0",
@@ -127,6 +126,8 @@
         "psr/http-server-middleware": "^1.0",
         "pusher/pusher-php-server": "^2.2",
         "s9e/text-formatter": "^2.3.6",
+        "sycho/json-api": "^0.5.0",
+        "sycho/sourcemap": "^2.0.0",
         "symfony/config": "^5.2.2",
         "symfony/console": "^5.2.2",
         "symfony/event-dispatcher": "^5.2.2",
@@ -134,7 +135,6 @@
         "symfony/polyfill-intl-messageformatter": "^1.22.0",
         "symfony/translation": "^5.1.5",
         "symfony/yaml": "^5.2.2",
-        "tobscure/json-api": "^0.3.0",
         "wikimedia/less.php": "^3.0"
     },
     "require-dev": {

extensions/mentions/tests/integration/api/ListPostsTest.php

@@ -14,7 +14,7 @@ use Flarum\Testing\integration\RetrievesAuthorizedUsers;
 use Flarum\Testing\integration\TestCase;
 use Illuminate\Support\Arr;
 
-class ListTest extends TestCase
+class ListPostsTest extends TestCase
 {
     use RetrievesAuthorizedUsers;
 
@@ -85,4 +85,26 @@ class ListTest extends TestCase
         $ids = Arr::pluck($data, 'id');
         $this->assertEqualsCanonicalizing(['4'], $ids, 'IDs do not match');
     }
+
+    /**
+     * @test
+     */
+    public function mentioned_filter_works_with_sort()
+    {
+        $response = $this->send(
+            $this->request('GET', '/api/posts')
+                ->withQueryParams([
+                    'filter' => ['mentioned' => 1],
+                    'sort' => '-createdAt'
+                ])
+        );
+
+        $data = json_decode($response->getBody()->getContents(), true)['data'];
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        // Order-independent comparison
+        $ids = Arr::pluck($data, 'id');
+        $this->assertEqualsCanonicalizing(['2', '3'], $ids, 'IDs do not match');
+    }
 }

extensions/sticky/composer.json

@@ -51,7 +51,7 @@
                 "prettier": true,
                 "typescript": false,
                 "bundlewatch": false,
-                "backendTesting": false,
+                "backendTesting": true,
                 "editorConfig": true,
                 "styleci": true
             }
@@ -64,5 +64,28 @@
         }
     ],
     "minimum-stability": "dev",
-    "prefer-stable": true
+    "prefer-stable": true,
+    "autoload-dev": {
+        "psr-4": {
+            "Flarum\\Sticky\\Tests\\": "tests/"
+        }
+    },
+    "scripts": {
+        "test": [
+            "@test:unit",
+            "@test:integration"
+        ],
+        "test:unit": "phpunit -c tests/phpunit.unit.xml",
+        "test:integration": "phpunit -c tests/phpunit.integration.xml",
+        "test:setup": "@php tests/integration/setup.php"
+    },
+    "scripts-descriptions": {
+        "test": "Runs all tests.",
+        "test:unit": "Runs all unit tests.",
+        "test:integration": "Runs all integration tests.",
+        "test:setup": "Sets up a database for use with integration tests. Execute this only once."
+    },
+    "require-dev": {
+        "flarum/testing": "^1.0.0"
+    }
 }

extensions/sticky/src/PinStickiedDiscussionsToTop.php

@@ -60,7 +60,6 @@ class PinStickiedDiscussionsToTop
             $query->orderByRaw('is_sticky and not exists ('.$read->toSql().') and last_posted_at > ? desc')
                 ->addBinding(array_merge($read->getBindings(), [$filterState->getActor()->read_time ?: 0]), 'union');
 
-            $query->unionOrders = array_merge($query->unionOrders, $query->orders);
             $query->unionLimit = $query->limit;
             $query->unionOffset = $query->offset;
 

extensions/sticky/tests/integration/api/ListDiscussionsTest.php

@@ -0,0 +1,90 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Sticky\tests\integration\api;
+
+use Carbon\Carbon;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Illuminate\Support\Arr;
+
+class ListDiscussionsTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->extension('flarum-sticky');
+
+        $this->prepareDatabase([
+            'users' => [
+                ['id' => 1, 'username' => 'Muralf', 'email' => 'muralf@machine.local', 'is_email_confirmed' => 1],
+                $this->normalUser()
+            ],
+            'discussions' => [
+                ['id' => 1, 'title' => __CLASS__, 'created_at' => Carbon::now(), 'last_posted_at' => Carbon::now(), 'user_id' => 1, 'first_post_id' => 1, 'comment_count' => 1, 'is_sticky' => true],
+                ['id' => 2, 'title' => __CLASS__, 'created_at' => Carbon::now()->addMinute(), 'last_posted_at' => Carbon::now(), 'user_id' => 1, 'first_post_id' => 1, 'comment_count' => 1, 'is_sticky' => false],
+                ['id' => 3, 'title' => __CLASS__, 'created_at' => Carbon::now()->addMinutes(2), 'last_posted_at' => Carbon::now(), 'user_id' => 1, 'first_post_id' => 1, 'comment_count' => 1, 'is_sticky' => true],
+                ['id' => 4, 'title' => __CLASS__, 'created_at' => Carbon::now()->addMinutes(3), 'last_posted_at' => Carbon::now(), 'user_id' => 1, 'first_post_id' => 1, 'comment_count' => 1, 'is_sticky' => false],
+            ],
+            'discussion_user' => [
+                ['discussion_id' => 1, 'user_id' => 1, 'last_read_post_number' => 1],
+                ['discussion_id' => 3, 'user_id' => 1, 'last_read_post_number' => 1],
+            ]
+        ]);
+    }
+
+    /** @test */
+    public function list_discussions_shows_sticky_first_as_guest()
+    {
+        $response = $this->send(
+            $this->request('GET', '/api/discussions')
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        $data = json_decode($response->getBody()->getContents(), true);
+
+        $this->assertEqualsCanonicalizing([1, 3, 2, 4], Arr::pluck($data['data'], 'id'));
+    }
+
+    /** @test */
+    public function list_discussions_shows_sticky_unread_first_as_user()
+    {
+        $response = $this->send(
+            $this->request('GET', '/api/discussions', [
+                'authenticatedAs' => 2
+            ])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        $data = json_decode($response->getBody()->getContents(), true);
+
+        $this->assertEqualsCanonicalizing([1, 3, 2, 4], Arr::pluck($data['data'], 'id'));
+    }
+
+    /** @test */
+    public function list_discussions_shows_normal_order_when_all_read_as_user()
+    {
+        $response = $this->send(
+            $this->request('GET', '/api/discussions', [
+                'authenticatedAs' => 1
+            ])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        $data = json_decode($response->getBody()->getContents(), true);
+
+        $this->assertEqualsCanonicalizing([1, 2, 3, 4], Arr::pluck($data['data'], 'id'));
+    }
+}

extensions/sticky/tests/integration/setup.php

@@ -0,0 +1,16 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+use Flarum\Testing\integration\Setup\SetupScript;
+
+require __DIR__.'/../../vendor/autoload.php';
+
+$setup = new SetupScript();
+
+$setup->run();

extensions/sticky/tests/phpunit.integration.xml

@@ -0,0 +1,25 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd"
+    backupGlobals="false"
+    backupStaticAttributes="false"
+    colors="true"
+    convertErrorsToExceptions="true"
+    convertNoticesToExceptions="true"
+    convertWarningsToExceptions="true"
+    processIsolation="true"
+    stopOnFailure="false"
+>
+    <coverage processUncoveredFiles="true">
+        <include>
+            <directory suffix=".php">../src/</directory>
+        </include>
+    </coverage>
+    <testsuites>
+        <testsuite name="Flarum Integration Tests">
+            <directory suffix="Test.php">./integration</directory>
+             <exclude>./integration/tmp</exclude>
+        </testsuite>
+    </testsuites>
+</phpunit>

extensions/sticky/tests/phpunit.unit.xml

@@ -0,0 +1,27 @@
+<?xml version="1.0" encoding="UTF-8"?>
+<phpunit
+    xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:noNamespaceSchemaLocation="https://schema.phpunit.de/9.3/phpunit.xsd"
+    backupGlobals="false"
+    backupStaticAttributes="false"
+    colors="true"
+    convertErrorsToExceptions="true"
+    convertNoticesToExceptions="true"
+    convertWarningsToExceptions="true"
+    processIsolation="false"
+    stopOnFailure="false"
+>
+    <coverage processUncoveredFiles="true">
+        <include>
+            <directory suffix=".php">../src/</directory>
+        </include>
+    </coverage>
+    <testsuites>
+        <testsuite name="Flarum Unit Tests">
+            <directory suffix="Test.php">./unit</directory>
+        </testsuite>
+    </testsuites>
+    <listeners>
+        <listener class="\Mockery\Adapter\Phpunit\TestListener" />
+    </listeners>
+</phpunit>

extensions/tags/src/Api/Controller/ShowTagController.php

@@ -49,11 +49,26 @@ class ShowTagController extends AbstractShowController
         $slug = Arr::get($request->getQueryParams(), 'slug');
         $actor = RequestUtil::getActor($request);
         $include = $this->extractInclude($request);
+        $setParentOnChildren = false;
 
-        return $this->tags
+        if (in_array('parent.children.parent', $include, true)) {
+            $setParentOnChildren = true;
+            $include[] = 'parent.children';
+            $include = array_unique(array_diff($include, ['parent.children.parent']));
+        }
+
+        $tag = $this->tags
             ->with($include, $actor)
             ->whereVisibleTo($actor)
             ->where('slug', $slug)
             ->firstOrFail();
+
+        if ($setParentOnChildren && $tag->parent) {
+            foreach ($tag->parent->children as $child) {
+                $child->parent = $tag->parent;
+            }
+        }
+
+        return $tag;
     }
 }

extensions/tags/src/TagRepository.php

@@ -14,7 +14,7 @@ use Illuminate\Database\Eloquent\Builder;
 
 class TagRepository
 {
-    private const TAG_RELATIONS = ['children', 'parent'];
+    private const TAG_RELATIONS = ['children', 'parent', 'parent.children'];
 
     /**
      * Get a new query builder for the tags table.

extensions/tags/tests/integration/api/tags/ListTest.php

@@ -83,15 +83,16 @@ class ListTest extends TestCase
     }
 
     /**
+     * @dataProvider listTagsIncludes
      * @test
      */
-    public function user_sees_where_allowed_with_included_tags()
+    public function user_sees_where_allowed_with_included_tags(string $include, array $expectedIncludes)
     {
         $response = $this->send(
             $this->request('GET', '/api/tags', [
                 'authenticatedAs' => 2,
             ])->withQueryParams([
-                'include' => 'children'
+                'include' => $include
             ])
         );
 
@@ -106,7 +107,7 @@ class ListTest extends TestCase
         // 6, 7, 8 aren't included because child access shouldnt work unless parent
         // access is also given.
         $this->assertEquals(['1', '2', '3', '4', '9', '10', '11'], Arr::pluck($data, 'id'));
-        $this->assertEquals(['3', '4'], Arr::pluck($included, 'id'));
+        $this->assertEquals($expectedIncludes, Arr::pluck($included, 'id'));
     }
 
     /**
@@ -125,4 +126,12 @@ class ListTest extends TestCase
         $ids = Arr::pluck($data, 'id');
         $this->assertEquals(['1', '2', '3', '4', '9', '10'], $ids);
     }
+
+    public function listTagsIncludes(): array
+    {
+        return [
+            ['children', ['3', '4']],
+            ['parent', ['2']],
+        ];
+    }
 }

extensions/tags/tests/integration/api/tags/ShowTest.php

@@ -0,0 +1,75 @@
+<?php
+
+/*
+ * This file is part of Flarum.
+ *
+ * For detailed copyright and license information, please view the
+ * LICENSE file that was distributed with this source code.
+ */
+
+namespace Flarum\Tags\Tests\integration\api\tags;
+
+use Flarum\Group\Group;
+use Flarum\Tags\Tests\integration\RetrievesRepresentativeTags;
+use Flarum\Testing\integration\RetrievesAuthorizedUsers;
+use Flarum\Testing\integration\TestCase;
+use Illuminate\Support\Arr;
+
+class ShowTest extends TestCase
+{
+    use RetrievesAuthorizedUsers;
+    use RetrievesRepresentativeTags;
+
+    /**
+     * @inheritDoc
+     */
+    protected function setUp(): void
+    {
+        parent::setUp();
+
+        $this->extension('flarum-tags');
+
+        $this->prepareDatabase([
+            'tags' => $this->tags(),
+            'users' => [
+                $this->normalUser(),
+            ],
+            'group_permission' => [
+                ['group_id' => Group::MEMBER_ID, 'permission' => 'tag8.viewForum'],
+                ['group_id' => Group::MEMBER_ID, 'permission' => 'tag11.viewForum']
+            ]
+        ]);
+    }
+
+    /**
+     * @dataProvider showTagIncludes
+     * @test
+     */
+    public function user_sees_tag_relations_where_allowed(string $include, array $expectedIncludes)
+    {
+        $response = $this->send(
+            $this->request('GET', '/api/tags/primary-2-child-2', [
+                'authenticatedAs' => 2,
+            ])->withQueryParams([
+                'include' => $include
+            ])
+        );
+
+        $this->assertEquals(200, $response->getStatusCode());
+
+        $responseBody = json_decode($response->getBody()->getContents(), true);
+
+        $included = $responseBody['included'] ?? [];
+        $this->assertEqualsCanonicalizing($expectedIncludes, Arr::pluck($included, 'id'));
+    }
+
+    public function showTagIncludes(): array
+    {
+        return [
+            ['children', []],
+            ['parent', ['2']],
+            ['parent.children', ['3', '2']],
+            ['parent.children.parent', ['3', '2']],
+        ];
+    }
+}

framework/core/less/forum/Composer.less

@@ -190,6 +190,7 @@
       }
 
       .normal &:first-child {
+        height: var(--header-height-phone);
         margin: calc(~"0px - var(--header-height-phone)") 50px 0;
         text-align: center;
         position: relative;
@@ -198,6 +199,9 @@
         white-space: nowrap;
         overflow: hidden;
         text-overflow: ellipsis;
+        display: flex;
+        align-items: center;
+        justify-content: center;
 
         h3 {
           color: var(--header-control-color);

framework/core/less/forum/NotificationList.less

@@ -68,14 +68,17 @@
     color: var(--heading-color) !important;
     padding: 8px 16px;
     white-space: nowrap;
-    overflow: hidden;
-    text-overflow: ellipsis;
 
     display: flex;
     align-items: center;
 
     // Prevent outline overflowing parent
     .add-keyboard-focus-ring-offset(-1px);
+
+    &, span {
+      overflow: hidden;
+      text-overflow: ellipsis;
+    }
   }
 
   &-badges {

framework/core/src/Frontend/Content/Assets.php

@@ -53,7 +53,7 @@ class Assets
         return $this;
     }
 
-    public function __invoke(Document $document, Request $request): void
+    public function __invoke(Document $document, Request $request)
     {
         $locale = $request->getAttribute('locale');
 

framework/core/src/Query/ApplyQueryParametersTrait.php

@@ -33,12 +33,14 @@ trait ApplyQueryParametersTrait
             $sort($query->getQuery());
         } else {
             foreach ((array) $sort as $field => $order) {
+                $field = $query->getQuery()->from.'.'.Str::snake($field);
+
                 if (is_array($order)) {
                     foreach ($order as $value) {
-                        $query->getQuery()->orderByRaw(Str::snake($field).' != ?', [$value]);
+                        $query->getQuery()->orderByRaw($field.' != ?', [$value]);
                     }
                 } else {
-                    $query->getQuery()->orderBy(Str::snake($field), $order);
+                    $query->getQuery()->orderBy($field, $order);
                 }
             }
         }

framework/core/src/User/User.php

@@ -650,7 +650,7 @@ class User extends AbstractModel
      */
     public function assertAdmin()
     {
-        $this->assertCan($this, 'administrate');
+        $this->assertCan('administrate');
     }
 
     /**