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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Issue tags: +VDC

.

oadaeh’s picture

FileSize
730 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

Status: Active » Needs review

Forgot the Status update.

dawehner’s picture

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

FileSize
2.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

@@ -0,0 +1,61 @@
+class AreaResultTest extends ViewUnitTestBase {

Can I hazza unit test, plz?

damiankloip’s picture

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

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

damiankloip’s picture

FileSize
3.14 KB
4.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

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

Status: Needs work » Needs review
FileSize
6.18 KB
6.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

Issue tags: -Needs tests
FileSize
740 bytes
6.51 KB

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

dawehner’s picture

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

FileSize
1.26 KB
6.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

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’s picture

That got committed already.

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

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @damiankloip for great work.

alexpott’s picture

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.