Page MenuHomePhabricator

getEditCountBucket() should always receive number, not a `null` when the user is logged out
Closed, ResolvedPublic

Description

From T168371#3530967:

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.

[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

Event Timeline

Piotr questioned whether we needed to do this.
null kinda makes sense for an anon as we don't know their edit count and this is not causing any issues in the code.
Let's chat about this before committing to it.

Sure, we'll have to change the getEditCountBucket function to accept nulls then.

Sure, we'll have to change the getEditCountBucket function to accept nulls then.

@phuedx as a result of T171853 we don't log events for logged in users. Is this code even needed anymore? Shouldn't we adjust the schema and remove?

Current flow is:

  1. When Popups initialize src/actions.js::boot() is called which retrieves wgUserEditCount from mw.config and stores user edit count under user.editCount property. For anonymous users editCount is null.
  2. On BOOT state we pass that array and create a state with baseData which is a result of calling src/reducers/eventLogging.js::getBaseData(). The getBaseData() function on line 28 first checks if user is not an anon (the editCount would be null in that case) and calls counts.getEditCountBucket() when we have logged-in user.

With that scenario there is no possibility to call counts.getEditCountBucket(null), as the check from line 28 prevents that. We can return null for sanity, but this path will not be executed.

phuedx claimed this task.

Per T173515#3545213, right?!