Page MenuHomePhabricator

eventLogging exception in Popups "Value "undefined previews" for property "previewCountBucket" is not one of ..."
Closed, ResolvedPublic3 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

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJun 20 2017, 9:46 AM
ovasileva moved this task from Backlog to Next Up on the Page-Previews board.
Jdlrobson added a subscriber: Jdlrobson.EditedJun 21 2017, 4:32 PM

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 triaged this task as Low priority.

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 Normal.
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 updated the task description. (Show Details)Aug 2 2017, 7:29 AM
phuedx added a comment.Aug 2 2017, 7:44 AM

@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.

ovasileva set the point value for this task to 3.Aug 2 2017, 4:29 PM
pmiazga claimed this task.

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

pmiazga added a comment.EditedAug 15 2017, 2:25 PM

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 removed pmiazga as the assignee of this task.
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 removed bmansurov as the assignee of this task.
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.

bmansurov added a comment.EditedAug 17 2017, 3:34 PM

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 closed this task as Resolved.Aug 22 2017, 5:12 PM
Jdlrobson claimed this task.

Calling this done.