Convert "Who's online" block to a View

i work on it

I Created a new View.

views.view.user_online_block.yml

The View list a maximum off 10 Active User.

Following Fields are selected:

• User: Name

Selected Filter Criteria

• User: is Active: True
• User: last Access: is greater or equal then -15 minutes

Header Field:

• Custom Result Field
• Currently is @total user online

No Results Behavior:

• Custom Text Area
• Currently are 0 users online

Also Removed the old UserOnlineBlock.php

There are currently two settings on the block:

• Seconds since the last login. This one really does not seem to be important, so I personally think this is okay to be moved to force the configuration on the view level.
• The amount of users. This feature is added in #1957346: Add some settings on the block display to allow overrides on the block instance configuration so this issue is kind of blocked on that one.

We can now override the items per page, let's get this issue up to speed again.

This doesn't properly account for more than 1 user. I also think this may need to be a handler.

In an ideal world we could just use #1958538: Improve math expression engine

Rerolled the patch and fixed nearly all your points.

One problem here is that there is no count query executed so we have to fix that as well beside of the single/plural camps.

+++ b/core/modules/user/config/views.view.who_s_online.yml
@@ -0,0 +1,196 @@
+      footer: {  }
+      empty:
+        text_custom:
+          id: text_custom
+          table: views
+          field: text_custom
+          relationship: none
+          group_type: group

I assumed this was a typo because looking at the view this was showing up as 'broken/mission handler'.

I had a look at the code in views.view.user_admin_people.yml and it uses area_text_custom instead of text_custom. That seems to work.

+++ b/core/modules/user/config/views.view.who_s_online.yml
@@ -0,0 +1,196 @@
+      header:
+        result:
+          id: result
+          table: views
+          field: result
+          relationship: none
+          group_type: group
+          admin_label: ''
+          empty: '0'
+          content: 'There is currently @total users online.'
+          plugin_id: result

This code seems to be correct, but for some reason no kind of field placed in the header region of the view is being output by the block.

I don't know why that is.

I'm pretty sure this patch is going to fail the tests, but I'll submit it anyway..

The count of items are just count by default if a pager is existant, why otherwise you would need to count all the results.
What you though can do is $view->get_total_rows = TRUE ... something (probably the result area plugin, should set that), if @total is part of the output.

Here is a parallel issue to fix that: #2063461: Automatically fetch the count of items, if there is @total part of the result area.

Please also share the screenshot of block configuration page.

Here are the before and after screenshots for block config:

I'ved debugged why the test fails for UserRoleUpgradePathTest.

It looks like in the DefaultPluginManager class when trying to create a block instance the following code is run:

<?php
  public function getDefinition($plugin_id) {
    // Fetch definitions if they're not loaded yet.
    if (!isset($this->definitions)) {
      $this->getDefinitions();
    }
    // Avoid using a ternary that would create a copy of the array.
    if (isset($this->definitions[$plugin_id])) {
      return $this->definitions[$plugin_id];
    }
    return array();
  }
?>

$this->getDefinitions() returns an array which is attached in the screenshot. The plugin_id that is being looked for is 'user_online_block' however, the plugin_id in the getDefinitions array is 'views_block:who_s_online-who_s_online_block'.

This is as far as I've gotten...

@yan
You don't often see a backtrace in an issue, cool stuff. We do replace the old plugin ('user_online_block') with the one based upon views,
so it seems totally valid that we have to change the test and replace this call

    $this->drupalGet('admin/structure/block/add/user_online_block/bartik');

with the one pointing to the views one.

No problem!

Going to try and tackle this during the DrupalCon Prague Code Sprint.

The tests were failing since ViewsDefaultTest->testDefaultViews() was asserting on number of results returned > 0, and the test did not have any user logged in (so nobody "online"). I've expanded the ViewsDefaultTest->setUp() to log a user in, and the tests now pass.

Forgot to include the new view's yml file in previous commit. Also made two additional changes:

• Moved the block from the default "Views" category, to the "User" category.
• Set the description of the block to "A list of users that are currently logged in.".

The test is failing since the Views module is not enabled in the test database. It's not trivial to enable the Views module, since UpgradePathTestBase::setUp() does not enable additional modules.

Rather than enabling modules to get blocks, etc. we can just change the block that is being tested. All the test wants is a block that is available to check that the role visibility settings work, so I propose we use a different block (perhaps the 'powered by drupal' block).

@yannoboi, good idea!

Patch attached that changes UserRoleUpgradePathTest to test role visibility during upgrade against the Powered By block.

Rather than enabling modules to get blocks, etc. we can just change the block that is being tested. All the test wants is a block that is available to check that the role visibility settings work, so I propose we use a different block (perhaps the 'powered by drupal' block).

+2 for this idea!

Patch works, looks good to me. If the test turns green, I'd say RTBC.

ITS GREEN :D

Added testing to the scope of the issue.

Updated issue summary.

updated issue summary

Great catch @oadaeh!

+++ b/core/modules/user/user.module
@@ -562,9 +562,6 @@ function user_preprocess_block(&$variables) {
-      case 'user_online_block':
-        $variables['attributes']['role'] = 'complementary';
-        break;

Should we be setting the role attribute to 'complementary' on the new block?

removed double numbers

Given that jbeach said that these are wrong anyway.

The patch was just a straight rerole.

47: vdc-2020399.patch queued for re-testing.

Re-roll because of #2134951: Remove upgrade path tests.

If its just a straight re-roll then I think we can RTBC this.

Argh sorry. I still haven't gotten my head around the new issue queue.... I hadn'e noticed that the patch hadn't come back from testing yet...

@yanniboi

If the testbot fails it will be pushed back anyway, so it is kind of safe to set it RTBC

First, a simple one:

+++ b/core/modules/user/config/views.view.who_s_online.yml
@@ -0,0 +1,197 @@
+      block_category: 'User'

+++ /dev/null
@@ -1,110 +0,0 @@
- *   category = @Translation("Lists (Views)")

This was formerly in "Lists (Views)" category, and now shows up under 'User'. In #2085201: Use sensible block categories (rather than module-name categories) the UX team tried to deliberately simplify/English-ify the list of categories away from stock module names, so this needs to be switched back.

Next, a before/after comparison with the HEAD version of the block. I always do these without reading the issue first so I can come in "fresh," so apologies as always if I'm repeating stuff that was already said.

NOTE! In the past, I know I have burnt out the VDC team by doing these kinds of detailed UX reviews. My intention is NOT to burn anyone out! :( I am simply documenting what I see, which is my job as a patch reviewer/core committer. If it is unreasonable to fix some of these things (at least in this patch), then please push back and we can talk about it! Getting all of these blocks converted to Views would be a HUGE win not only for the VDC initiative, but also for D8, and I really, really want to see it happen! But I also need to give this the same scrutiny as I do other UX patches (ask anyone: I'm an equal-opportunity hard-ass :P) so we don't introduce regressions. I do this in order to make you guys look really effing good when new people encounter D8. :) Hugs, kisses, unicorns, ponies, <3 webchick. :D

Ok. With that out of the way....

HEAD's Who's Online block

This patch's Who's Online block

Differences

- The most obvious one is that the block name is missing back on the block listing page. That's a commit blocker, unfortunately. There's no way for someone to determine what block that is short of hovering over the Configure link.
- That may be related to another obvious regression on the config form, where before we used a standard Name / Machine name pattern and now we just have the machine name sitting there glaring out at us menacingly. That's a pretty big regression, so I would love to see that fixed.
- Second, that field (once fixed) should really be at the top of the form, since that's the standard place for "namey" things.
- We also lost the ability to filter the block on the activity level of users from this form. While this is still possible to do by clicking into "Edit view" from contextual links and mucking about with the filters, that's definitely a LOT harder than it used to be. Do we have any provision for exposing this field like we do the count of records (which is also a views thing)?

Once the blocks are placed on the page, they look identical, so great work there. Needs work for the above though. Let's discuss.

Probably also this.

+++ b/core/modules/user/user.module
@@ -571,9 +571,6 @@ function user_preprocess_block(&$variables) {
-      case 'user_online_block':
-        $variables['attributes']['role'] = 'complementary';
-        break;

Well, hold up now. What happened to this? This would cause an accessibility regression, no?

NOTE! In the past, I know I have burnt out the VDC team by doing these kinds of detailed UX reviews. My intention is NOT to burn anyone out! :( I am simply documenting what I see, which is my job as a patch reviewer/core committer. If it is unreasonable to fix some of these things (at least in this patch), then please push back and we can talk about it! Getting all of these blocks converted to Views would be a HUGE win not only for the VDC initiative, but also for D8, and I really, really want to see it happen! But I also need to give this the same scrutiny as I do other UX patches (ask anyone: I'm an equal-opportunity hard-ass :P) so we don't introduce regressions. I do this in order to make you guys look really effing good when new people encounter D8. :) Hugs, kisses, unicorns, ponies, <3 webchick. :D

Thank you!!

This was formerly in "Lists (Views)" category, and now shows up under 'User'. In #2085201: Needs documentation: Use sensible block categories (rather than module-name categories) the UX team tried to deliberately simplify/English-ify the list of categories away from stock module names, so this needs to be switched back.

Right ... you seemed to have picked a bad example. You probably to know that I aggressively set these issues to RTBC, because no one will care anyway/otherwise, but I did tested them so the actual block rendering functionality works.

- Second, that field (once fixed) should really be at the top of the form, since that's the standard place for "namey" things.

Sure, well we just need yet another drupalcon to discuss things, another maybe skipped and finally one to fix it. This will be an endless story.

- We also lost the ability to filter the block on the activity level of users from this form. While this is still possible to do by clicking into "Edit view" from contextual links and mucking about with the filters, that's definitely a LOT harder than it used to be. Do we have any provision for exposing this field like we do the count of records (which is also a views thing)?

First note: There is one issue which provides an edit view link directly in the block interface (or at least allows to add one in the future).
This would certainly improve the usability a LOT here.

Second note: Let's shake our heads for a long time, let's make a long facepalm, and last but not least shout out loud. And again, and again ...
When we ran into the items per page problem like 9 months ago. There we decided that we don't need a generic override system.
Let me try to argue again why I think this filter is pointless anyway:

There are different people.

• There is a hobbyist which just installs drupal, places some blocks and creates some content. That person
  gives a shit about when a user is considered to be online or not.
• There is the hobbyist which builds some small websites for other
  people, like sport clubs, maybe some local NGOs. This is the region where drupal shines: you create content types, add some fields, create
  a view and you have immediately an event listing. No damn theming, no damn Yes, they maybe have logged in users, but they know what views is and at least can configure
  it to do simple things. This block already has the filter, so changing it is trivial for that person.
• There is the professional site builder ... that person gives a shit about existing views and just start from scratch anyway

So if you still want to have that filter, feel free to mark just this issue as closed (works as designed), the other ones don't need that (and yes I used that word often) shit.

Well, hold up now. What happened to this? This would cause an accessibility regression, no?

See https://drupal.org/comment/8173469#comment-8173469 why we can remove it. I asked her to comment in one issue, even there are for sure multiple issues on which those problems apply.

- The most obvious one is that the block name is missing back on the block listing page. That's a commit blocker, unfortunately. There's no way for someone to determine what block that is short of hovering over the Configure link.

Yes, I know that this is kind of problem but I just ignored that in order to make progress on like 6 issues at a time. If we don't convert th

PS: Let's see whether I will translate "List (Views)" with just 'Views' in German to start a war just for fun.

My intention is NOT to burn anyone out!

Maybe you just manage to move people to more low level code. If you would ask me honest I would prefer to fix things instead of rerolling
all the patches all the time. The rerolling time can be invested in actually improving stuff.

Well, I'll just be honest and say that comment feels really hurtful. :( I genuinely am coming here from a position of respect for your work, and a genuine desire to help get it into core, and tried to make this extremely clear in my review. But now I'm just feeling utterly demoralized and attacked for simply doing my job. Thanks for your honesty, though, I guess...?

Probably best for another core maintainer to deal with these issues then. It doesn't make sense for me to continue to spend 3+ hours doing in-depth reviews on VDC conversion patches if this is the way those reviews are going to be received.

However, you did make the point (as non-constructively as possible) that changing the activity time filter most likely falls under the site builder persona vs. the content editor persona, so exposing a filter for it in the block configuration isn't necessary. So I withdraw that from my review.

And, it seems like #2135341: Views should provide a means to add arbitrary attributes (such as aria-* attributes) to a view's wrapping element will handle the concerns about the ARIA roles being lost, so site builders can put those back if they feel they're necessary. Which, in this case, it sounds like they're not, so we're fine there.

So that leaves fixing the category, fixing the block title on the block listing page, fixing the machine name field to be a normal name / machine name pairing, and moving the field to the top of the form (I've no idea why we need a DrupalCon to figure this out; check any form in core, it's the same).

And with that, I'm out. Still hope this makes it into core. But don't ping me about it again.

Well, I'll just be honest and say that comment feels really hurtful. :( I genuinely am coming here from a position of respect for your work, and a genuine desire to help get it into core, and tried to make this extremely clear in my review. But now I'm just feeling utterly demoralized and attacked for simply doing my job. Thanks for your honesty, though, I guess...?

Damn, I am sorry. It is sadly so hard to express thoughts in a non native language, and not piss of you. Sorry! I did not intended it.

:( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :(
:( :( :( :( :( :( :( :( :( :( :( /me hugs :( :( :( :( :( :( :( :(
:( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :( :(

dawehner and I subsequently hugged and made up on IRC. :) I'd also love to get all of these views conversions in in the next alpha, so tagging.

Now that #1957276: Let users set the block instance title for Views blocks in the Block UI is in, my main point of feedback was addressed, and #2151731: Views blocks' machine names do not fit the standard Drupal pattern will address the second. The only other thing was the View category was incorrect, but simply by removing the block_category line from the YAML file, that's fixed up to. So!

Committed and pushed to 8.x. YEAH!

webchick++