Problem/Motivation

In an install with Views not installed (i.e. minimal install), the 'Last access' column in the default People list shows the years since UNIX timestamp 0 for newly created users (or anybody that has never logged in).

Steps to reproduce:

  • Do minimal install
  • Create a new user
  • Go to admin/people

Proposed resolution

Show 'never' like in the corresponding View.

Remaining tasks

User interface changes

Show something proper.

Before

After


CommentFileSizeAuthor
#20 2609504-19.png32.2 KBmohit_aghera
#19 users_show_never-2609504-19.patch1.79 KBlendude
#16 People-D8-2609504-16.png33.76 KBmohit_aghera
#16 interdiff-2609504-5-16.txt864 bytesmohit_aghera
#16 users_show_never-2609504-16.patch1.86 KBmohit_aghera
#11 People_D8_2609504_5.png33.62 KBmohit_aghera
#10 People D8.png34.34 KBmohit_aghera
#9 users_show_never-2609504-9.patch1.17 KBibustos
#9 Evidence 1 - Default date formatter #9.png12.25 KBibustos
#7 After patch #5.png27.42 KBlendude
#5 users_show_never-2609504-5.patch1.87 KBlendude
#5 users_show_never-2609504-5-TEST_ONLY.patch1.04 KBlendude
#2 users_show_never-2609504-2.patch837 byteslendude
people list with new user.png21.71 KBlendude

Comments

Lendude created an issue. See original summary.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue tags: +D8UX usability, +Needs tests
StatusFileSize
new users_show_never-2609504-2.patch837 bytes

This patch should fix it, but doesn't, because currently user.access and user.login default to 0 and not NULL. And since unix timestamp 0 is a perfectly valid non-empty timestamp, it's (rightly) not considered empty.

Will open a new issue to discuss the default values that need to be set to NULL.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Related issues: +#2609922: User access and login fields should default to NULL not 0

Defaults issue #2609922: User access and login fields should default to NULL not 0

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Active » Needs review
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue tags: -Needs tests
StatusFileSize
new users_show_never-2609504-5-TEST_ONLY.patch1.04 KB
new users_show_never-2609504-5.patch1.87 KB

Failing test and fix.

Still feel the fix in #2 is the way to go, but don't know if setting access to NULL has any other implications and how likely that is to actually make it in. The Views version of the people list deals with the 0 value, so just making the non-views version deal with it also makes sense.

The last submitted patch, 5: users_show_never-2609504-5-TEST_ONLY.patch, failed testing.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue summary: View changes
StatusFileSize
new After patch #5.png27.42 KB
lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue summary: View changes
ibustos’s picture

ibustos
he/him
Location Bogotá D.C.
commented
StatusFileSize
new Evidence 1 - Default date formatter #9.png12.25 KB
new users_show_never-2609504-9.patch1.17 KB

I confirm the issue still exists when using the default date formatter. Attached an evidence.

mohit_aghera’s picture

mohit_aghera commented
Issue summary: View changes
Status: Needs review » Needs work
StatusFileSize
new People D8.png34.34 KB

@ibustos
It's failing after applying patch. I've tested it with latest D8 head
see attached file.

Patched in comment #5 is working fine.

mohit_aghera’s picture

mohit_aghera commented
StatusFileSize
new People_D8_2609504_5.png33.62 KB

Testing patch #5
It seems appearing fine with latest D8 head

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented

@ibustos and @mohit_aghera thanks for looking at this!

About #9, as I pointed out in #1 and #2, 0 is a perfectly valid timestamp so making the default formatter interpret that as 'never' seems wrong to me (since it's Jan 1ste 1970). The patch in #5 is more of a work around to get the non-Views People display in line with the Views version.

I still think that actual fix for this should be what is outlined in #2, but that also feel a little like overkill for this issue, and not something that can be put into 8.0.x

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review

putting this back on need review for #5

mohit_aghera’s picture

mohit_aghera commented
Status: Needs review » Reviewed & tested by the community

@Lendude
Thanks for your input.
As per patch in comment #5 things are appearing fine.
See this comment https://www.drupal.org/node/2609504#comment-10837674
Hence, moving this into RTBC.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/user/src/UserListBuilder.php
@@ -152,7 +152,7 @@ public function buildRow(EntityInterface $entity) {
+    $row['access'] = $entity->access->getValue()[0]['value'] ? $this->t('@time ago', array('@time' => $this->dateFormatter->formatTimeDiffSince($entity->getLastAccessedTime()))) : t('never');

This looks odd to me. Why not getLastAccessedTime()

mohit_aghera’s picture

mohit_aghera commented
Status: Needs work » Needs review
StatusFileSize
new users_show_never-2609504-16.patch1.86 KB
new interdiff-2609504-5-16.txt864 bytes
new People-D8-2609504-16.png33.76 KB

Thanks @Catch for pointing out the issue.
Adding updated patch with changes mentioned by you.
Uploaded relevant screenshot as well.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

RTBC again assuming it comes back green.

catch’s picture

catch
he/him
Primary language English
commented
Version: 8.0.x-dev » 8.1.x-dev
Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

This needs a re-roll for 8.1.x

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new users_show_never-2609504-19.patch1.79 KB

Reroll for 8.1.x

Handled a little differently in 8.1, but still bugged.

mohit_aghera’s picture

mohit_aghera commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new 2609504-19.png32.2 KB

Tested #19 locally by applying patch.
Working fine. Screenshot attached.

  • catch committed 68e9f22 on 8.1.x 
    Issue #2609504 by Lendude, mohit_aghera, ibustos: Default People display...

  • catch committed 095431b on 8.0.x 
    Issue #2609504 by Lendude, mohit_aghera, ibustos: Default People display...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.1.x and 8.0.x thanks!

Status: Fixed » Closed (fixed)

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