Page MenuHomePhabricator

eventLogging exception in Popups "Value "undefined previews" for property "previewCountBucket" is not one of ..."
Closed, ResolvedPublic3 Estimated Story Points

Description

Seen locally:

[Popups] Value "undefined previews" for property "previewCountBucket" is not one of ["0 previews","1-4 previews","5-20 previews","21+ previews","unknown"]

Doesn't happen on a fresh session on an incognito window.

Steps to reproduce

  • Unknown

There's some codepath that ends up putting NaN in storage for the previewCount.

More info

mw.storage.store.get('ext.popups.core.previewCount')
//> "NaN"

AC

  • When setting things in storage the types and existence of values is validated before storing (review all storage setters and make assertions)
  • Wherever previewCount can become NaN is fixed
  • Discard previewCount on storage read if it doesn't validate as a not-NaN number
  • Return "unknown" in getPreviewCountBucket when count isn't a number or is -1 (see T168371#3367852).

Event Timeline

Looks like

function getPreviewCountBucket( count ) {

is getting called with count undefined.
We should add a test case and handle this.

exports.getPreviewCountBucket = function getPreviewCountBucket( count ) {
	var bucket;

	if ( count === -1 ) {
		return 'unknown';
	}

	if ( count === 0 ) {
		bucket = '0';
	} else if ( count >= 1 && count <= 4 ) {
		bucket = '1-4';
	} else if ( count >= 5 && count <= 20 ) {
		bucket = '5-20';
	} else if ( count >= 21 ) {
		bucket = '21+';
	}

	return bucket + ' previews';
};
Jdlrobson moved this task from Incoming to Triaged but Future on the Readers-Web-Backlog board.

Not necessarily undefined I guess, maybe it is being called with NaN or 'NaN' or something like that.

Jdlrobson raised the priority of this task from Low to Medium.Aug 1 2017, 9:13 PM
Jdlrobson moved this task from Triaged but Future to Upcoming on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: phuedx.

@phuedx if we're going to work on this, I'm guessing we should work on this now or never?

@phuedx if we're going to work on this, I'm guessing we should work on this now or never?

Let's get it estimated.

Change 372056 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] getPreviewCountBucket should return unknown when no bucket is found

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

I couldn't reproduce this issue. I decided to apply a fix that should solve eventLogging exception without understanding the root cause. When previewCount is not -1 or a natural number it will always return unknown - where previously it was returning unknown only when preview_count === -1

Jdlrobson added a subscriber: pmiazga.

skipping qa - this is a technical edge case. Signer off should review the unit tests to confirm this is fixed given this is hard to reproduce.

Change 372056 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] getPreviewCountBucket should return unknown when no bucket is found

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

bmansurov added a subscriber: bmansurov.

When the user is logged out the editCount will be null [1], which is then passed as the property of the user object [2] to getEditCountBucket [3]. I suggest we either adjust the signature of exports.getEditCountBucket to accept null or pass in a -1 as before. We should also remove the new test cases (possibly keeping only one of them) from [4].

[1] https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/actions.js#L68
[2] https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/actions.js#L84
[3] https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/reducers/eventLogging.js#L29
[4] https://gerrit.wikimedia.org/r/#/c/372056/1/tests/node-qunit/counts.test.js

@bmansurov this task is about previewCountBucket. I was going to create a second patch that fixes editCountBucket but there is no need as in getBaseData() [1] we verify if ( !bootAction.user.isAnon ) {. With that check, getEditCountBucket() will not be called with null.

[1] https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/reducers/eventLogging.js#L28

@bmansurov sounds like a good find! Can you create a task around the edit count bucket and how it's impacting EventLogging? I'll take a look tomorrow and make sure we get that scheduled for next sprint.

Can you verify the fix for previewCountBucket?

My bad, I've mixed up two kinds of buckets. I'll verify the solution properly this time. ;)

no worries, I had the same problem as getPreviewCountBucket() and getEditCountBucket() are very similar.

The problem seems to be happening at [1]. Since parseInt can return NaN, the function should return 0 in such cases. Can we guard against this possibility and remove the newly introduced tests as mentioned in T168371#3529067? This should keep our code cleaner and help us not be defensive without knowing the cause of the problem.

[1] https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/userSettings.js#L83

Change 372411 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] if preview count stored in localStorage is not a number override it

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

Change 372411 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] if preview count stored in localStorage is not a number override it

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

Jdlrobson claimed this task.

Calling this done.