Page MenuHomePhabricator

[wmf.14-regression] Echo: Unread count color won't stay gray
Closed, ResolvedPublicBug

Description

Steps to Reproduce:

  1. Mark all alerts as read
  2. Give yourself an alert (e.g. a successful or failed mention)
  3. Click on the alerts (bell) button, now showing "1" in red
  4. Close the alerts popup without marking the new notification as read
  5. Refresh the page (or open any page on the site)

or

  1. Mark all alerts as read
  2. Mark an alert as unread (by clicking the circle at the right top)
  3. Refresh the page (or open any page on the site)

Actual Results:
The number "1" is shown on a red background

Expected Results:
The number is shown on a gray background

SkinActualExpected
Vector
Mobile

Same with notices (except for blue instead of red, of course).

Not sure if this is a bug or a change by design, but I believe the background of the unread count used to stay gray after seeing, but not marking as read, a new notification. (WP:VPT)

Event Timeline

Nardog created this task.Jul 23 2019, 12:37 PM
Restricted Application added a project: Growth-Team. · View Herald TranscriptJul 23 2019, 12:37 PM
Restricted Application added a subscriber: Aklapper. · View Herald Transcript
Jeff_G added a subscriber: Jeff_G.Jul 23 2019, 5:10 PM
JTannerWMF added subscribers: Etonkovidova, JTannerWMF.

@Etonkovidova will see if it is reproducible

According to SharabSalam on Wikipedia VPT:

The "expected" after clicking on the notification button and refreshing is that the number disappear and the gray bell icon appears but what happens right now is that the notification appears again as if I haven't clicked on the notification button.

When you click on the notification button with the number it changes to gray which is as usual and the normal behaviour the problem appears when you refresh the page, what is expected is that the bell icon appears like if there was no notification but what happens is that the notification appears again. The photos that you added suggest a different problem. The expected photos should not contain the number of notifications in gray but the bell icon.

The way I remember is that opening and closing the alerts panel without marking all alerts as read would have made the background of the unread count gray but didn't make the count disappear entirely, but I may be mistaken.

Etonkovidova added a comment.EditedJul 24 2019, 10:37 PM

It's a regression bug (exists in wmf.14)
Merged T228946 into this task;

  1. Have some echo notifications present for a user.
  2. Click on a badge, the color will be changed to grey.
  3. Go to a different page or refresh the page - the colors will be back to red and blue (unseen)

Etonkovidova renamed this task from Echo: Unread count color won't stay gray to [wmf.14-regression] Echo: Unread count color won't stay gray.Jul 24 2019, 11:03 PM
Etonkovidova added a project: Regression.
Catrope claimed this task.Jul 24 2019, 11:33 PM
Catrope added a subscriber: Catrope.

Something's definitely wrong here...

This appears to be happening because MainStash / Redis has broken handling of $ttl = 0. It's supposed to mean "forever", but it looks like it's being taken to mean "expire immediately":

catrope@mwmaint1002:~$ mwscript eval.php testwiki
> $stash = MediaWiki\MediaWikiServices::getInstance()->getMainObjectStash();

> $key = $stash->makeKey('roantest');

> $stash->set($key, 'foobarbaz', 3600);

> echo $stash->get($key);
foobarbaz

> $key = $stash->makeKey('roantest2');

> $stash->set($key, 'foobarbazquux');

> echo $stash->get($key);

>

The TTL gets set to 1 somehow...

I've figured it out: rMW383460afa3b7: objectcache: normalize BagOStuff method overriding pattern for *Multi() methods changed the behavior of convertToRelative(), such that convertToRelative( 0 ) used to return 0 but now returns 1. This is incorrect (or at least unhelpful), because RedisBagOStuff does $ttl = $this->convertToRelative( $exptime );, then checks whether or not $ttl is zero to determine whether to set a TTL or not. But as currently written, convertToRelative() can never return 0, so in code like if ( $ttl ) { ... } else { ... } the else branch is never taken.

I'm going to dinner now, but will work on a patch later this evening.

@Trizek-WMF Re: Tech/News -- Is this is a significant enough bug that it deserves a mention? AFAICT it doesn't impact the ability to read notifications or do any editing work, it's just a minor confusion/annoyance that will be fixed very soon. I'd suggest not including it.
However, if you do strongly believe it should be added, how would you like the mention to be phrased? I would guess at something like:
"Problems: There is a bug with the color of the New Notifications indicators. The colors do not stay grey once you have opened the new Notifications. This is being worked on."

Change 525697 had a related patch set uploaded (by Catrope; owner: Catrope):
[mediawiki/core@master] MediumSpecificBagOStuff: Make convertToRelative(0) return 0, not 1

https://gerrit.wikimedia.org/r/525697

Change 525697 merged by jenkins-bot:
[mediawiki/core@master] MediumSpecificBagOStuff: Make convertToRelative(0) return 0, not 1

https://gerrit.wikimedia.org/r/525697

@Trizek-WMF Re: Tech/News -- Is this is a significant enough bug that it deserves a mention? AFAICT it doesn't impact the ability to read notifications or do any editing work, it's just a minor confusion/annoyance that will be fixed very soon. I'd suggest not including it.

Makes sense.

Etonkovidova closed this task as Resolved.Tue, Jul 30, 8:54 PM