Test coverage: multiple sorts

Hello @e0ipso,

No code in progress yet, I have been on other subjects.

Thanks

Hello,

After seeing #2921892: fix(Sort): Expanded form sort parameters should respect order, I have retested with the last dev version of JSONAPI, and it is fixed now.

I requeued patch from dawehner, maybe it can be merged to have more tests.

@e0ipso: ok, assigning to myself. I will make the re-roll tomorrow.

Here is the rebased patch. I also fixed some coding standards at the same time.

But the tests fails:

There was 1 failure:

1) Drupal\Tests\jsonapi\Functional\JsonApiFunctionalTest::testRead
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
Array (
- 0 => 55
- 1 => 60
- 2 => 59
- 3 => 58
- 4 => 57
- 5 => 56
+ 0 => 1
+ 1 => 2
+ 2 => 3
+ 3 => 4
+ 4 => 5
+ 5 => 6
)

/project/www/core/tests/Drupal/Tests/BrowserTestBase.php:1240
/project/www/modules/contrib/jsonapi/tests/src/Functional/JsonApiFunctionalTest.php:294

I will see why. I have uploaded this failing patch to be able to make an interdiff.

Here is the test fixed.

But to fix it, there is no sort in DESC. So I don't know if the node samples should be modified to keep having "field_sort1,-field_sort2".

Thanks for the review.

Hello,

@Wim Leers, thanks for your feedbacks and for updating the patch.

I am triggering the tests locally and yes, the output count not matching the limit is strange. I am investigating.

I am verifying but I think I found why, at the beginning of the test method:

  // Unpublish the last entity, so we can check access.
  $this->nodes[60]->setUnpublished()->save();

But if the user can't access the last entity it should display another one published, no?

OK.

I confirm that it is this line that provoked the fact that the node 61 is excluded from the result but included in the query limit.

Otherwise the expect nid order would be:

    // 24. Test sort criteria on multiple fields: first DESC, second ASC.
...
    $this->assertEquals([61, 60, 59, 58, 57, 56], $output_nids);
...
    // 25. Test sort criteria on multiple fields: both DESC.
...
    $this->assertEquals([61, 56, 57, 58, 59, 60], $output_nids);

I quickly reproduced the behavior:

With 3 articles: article 1, article 2, article 3

The article 3 is unpublished.

On jsonapi/node/article as anonymous, I see two articles and a meta error message:

    "meta": {
        "errors": [
            {
                "title": "Forbidden",
                "status": 403,
                "detail": "The current user is not allowed to GET the selected resource.",
                "links": {
                    "info": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4"
                },
                "code": 0,
                "id": "/node--article/8ed78306-ff14-4ef2-9eb7-1091cf98bbf5",
                "source": {
                    "pointer": "/data"
                }
            }
        ]
    },

On jsonapi/node/article?page[limit]=1&sort=-title as anonymous, I only see with no data:

    "meta": {
        "errors": [
            {
                "title": "Forbidden",
                "status": 403,
                "detail": "The current user is not allowed to GET the selected resource.",
                "links": {
                    "info": "http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html#sec10.4.4"
                },
                "code": 0,
                "id": "/node--article/8ed78306-ff14-4ef2-9eb7-1091cf98bbf5",
                "source": {
                    "pointer": "/data"
                }
            }
        ]
    },

Also there is a need to add a stop at the end of the comment (for PHPCS):

      // Create values for the sort fields, to allow for testing complex
      // sorting:
      // - field_sort1 increments every 5 articles, starting at zero
      // - field_sort2 decreases every article, ending at zero.

@e0ipso: sorry, the manual test case is ambiguous.

There is no data because I limit the result to 1 item and with the sort criteria it is the unpublished item that is the first one. But as the anonymous user can't access this unpublished node, there is no data.

If there is no result limit, the anonymous user sees 2 articles (the two published) with a meta errors for the unpublished one.

Hello,

Thanks both for your replies and work.

But is it an expected behavior?

I mean, when having a view with a limit to only 5 results, if I have 100 published content and one unpublished. Even if the unpublished one should be in the results due to the sorts criteria, I want the visitors to see 5 results, so if other contents match the filters and the visitor has access to the other contents, its should be displayed.

Maybe I should open a new issue for that? Because this issue is about sorts, not filtering.

OK.

Thanks for the reply and the commit.