Kind of blocking #2020399: Convert "Who's online" block to a View

The result area allows you to configure to display the total amount of results, though under some circumstances, like when no pager exists, this number is not available.

We could look in the query method whether @total is part of the text and set $view->get_total_rows otherwise.

CommentFileSizeAuthor
#17 2063461-17.patch6.58 KBdamiankloip
#17 interdiff-2063461-17.txt1.32 KBdamiankloip
#14 vdc-2063461-14.patch6.73 KBdawehner
#14 interdiff.txt1.26 KBdawehner
#12 2063461-12.patch6.51 KBdamiankloip
#12 interdiff-2063461-12.txt740 bytesdamiankloip
#11 2063461-11.patch6.5 KBdamiankloip
#11 interdiff-2063461-11.txt6.18 KBdamiankloip
#9 2063461-9.patch4.76 KBdamiankloip
#9 interdiff-2063461-9.txt3.14 KBdamiankloip
#7 2063461-7.patch3.76 KBdamiankloip
#7 interdiff-2063461-7.txt4.1 KBdamiankloip
#5 2063461-5.patch2.91 KBdamiankloip
#2 get-total-rows-2063461-2.patch730 bytesoadaeh

Comments

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +VDC

.

oadaeh’s picture

oadaeh commented
StatusFileSize
new get-total-rows-2063461-2.patch730 bytes

Here's my first stab at correcting this. I get the desired results, I'm just not 100% positive it's the correct way to do it.

oadaeh’s picture

oadaeh commented
Status: Active » Needs review

Forgot the Status update.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +Needs tests

Thanks for fixing that bug!

@@ -54,6 +54,14 @@ public function buildOptionsForm(&$form, &$form_state) {
+   * Implements \Drupal\views\Plugin\views\area\AreaPluginBase::query().

This should be {@inheritdoc}

It would be also cool to have some sort of test for it (personally a unit test would be the test thing).

damiankloip’s picture

damiankloip commented
StatusFileSize
new 2063461-5.patch2.91 KB

@oadaeh, here is a start on the tests, I've just added the first assert for the '@label' token.

This line also needed fixing in the render() method, this is why having test coverage is a good idea. This has been broken for a long time!

-    if (!isset($this->options['content']) || $this->view->plugin_name == 'default_summary') {
+    if (!isset($this->options['content']) || $this->view->style_plugin instanceof DefaultSummary) {
dawehner’s picture

dawehner
Primary language German
commented 
@@ -0,0 +1,61 @@
+class AreaResultTest extends ViewUnitTestBase {

Can I hazza unit test, plz?

damiankloip’s picture

damiankloip commented
StatusFileSize
new interdiff-2063461-7.txt4.1 KB
new 2063461-7.patch3.76 KB

ok ok, here is the same patch as #5 but for phpunit instead. I've just added the same assertion for the @label token.

And yes, the interdiff is bigger than the patch :)

dawehner’s picture

dawehner
Primary language German
commented

Just to be sure, we should test the get_total_rows setting, not just the @label.

damiankloip’s picture

damiankloip commented
StatusFileSize
new interdiff-2063461-9.txt3.14 KB
new 2063461-9.patch4.76 KB

Yes, the intention was to test all of them.

I think this makes sense to switch to using a data provider too.

dawehner’s picture

dawehner
Primary language German
commented
Status: Needs review » Needs work

Please try to run the test individually ... there is a check_plain() call in the code.

      $page_count = (int) ceil($total / $per_page);
      $total_count = $current_page * $per_page;
      if ($total_count > $total) {
        $total_count = $total;
      }
      $start = ($current_page - 1) * $per_page + 1;
      $end = $total_count;

This piece of the code is not executed at all, so it has to be tested.

Additional the actual bug in this issue has to be tested as well.

damiankloip’s picture

damiankloip commented
Status: Needs work » Needs review
StatusFileSize
new interdiff-2063461-11.txt6.18 KB
new 2063461-11.patch6.5 KB

Thanks, this class should be pretty well tested now, as the items per page are also passed in from the data provider.

Also, testing the actual bug in this issue is a great idea! Lets do that ;)

So bootstrap.inc is included for unit tests? meh.

damiankloip’s picture

damiankloip commented
Issue tags: -Needs tests
StatusFileSize
new interdiff-2063461-12.txt740 bytes
new 2063461-12.patch6.51 KB

I should not call destroy; it calls viewsHandlerTypes, which in turn uses t() all over the place.

dawehner’s picture

dawehner
Primary language German
commented

There are a few lines which are not 100% tested but this already is so much of a progress!

+++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/area/ResultTest.php
@@ -0,0 +1,143 @@
+    $this->view = new ViewExecutable($storage);

Do we really need an actual viewExecutable or can we just mock it?

dawehner’s picture

dawehner
Primary language German
commented
StatusFileSize
new interdiff.txt1.26 KB
new vdc-2063461-14.patch6.73 KB

Mh we can't as we still have directly access to all this properties.

The constant is needed to order to run it properly (just this class instead of the full phpunit test suite).

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented

We can ignore these issue it is pretty much RTBC for me.

  1. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/area/Result.php
@@ -8,6 +8,9 @@
 use Drupal\Component\Annotation\PluginID;
+use Drupal\views\Plugin\views\style\DefaultSummary;
+use Drupal\Component\Utility\Xss;
+use Drupal\Component\Utility\String;

    Sorting order. 

  2. +++ b/core/modules/views/tests/Drupal/views/Tests/Plugin/area/ResultTest.php
@@ -0,0 +1,150 @@
+// @todo Remove this once the constant got converted.

    issues id would be nice.

damiankloip’s picture

damiankloip commented

#2067017: Remove usage of DRUPAL_CORE_COMPATIBILITY/VERSION constants is the DRUPAL_CORE_COMPATIBILITY issue

damiankloip’s picture

damiankloip commented
StatusFileSize
new interdiff-2063461-17.txt1.32 KB
new 2063461-17.patch6.58 KB

That got committed already.

jibran, I think that takes care of your comments now?

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs review » Reviewed & tested by the community

Thank you @damiankloip for great work.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 8ebd5dc and pushed to 8.x. Thanks!

Automatically closed -- issue fixed for 2 weeks with no activity.