Page MenuHomePhabricator

event.linkInteractionToken can be undefined
Closed, ResolvedPublic2 Estimated Story Points

Description

Background

Current Behavior

https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/changeListeners/eventLogging.js#L71

For example, with the pageLoaded event. Probably with others.

Guard the token deduplication logic with the token existence.

Reported originally here T167273#3364463 :

@phuedx - there is one tiny issue with tracking PageLoaded events.

We create a pageLoaded event here - https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/reducers/eventLogging.js#L170 - event doesn't contain linkInteractionToken property.

Then in https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/changeListeners/eventLogging.js#L71 we read linkInteractionToken which is undefined.

It would be nice to fix it so our code is consistent.

AC/Expected Behaviour

  • Events with null or undefined link interaction tokens aren't deduplicated.

Notes

  1. It may be useful to have a whitelist for events that can have null or undefined link interaction tokens associated with them e.g. disabled
  2. This should be done in [the eventLogging change listener](https://github.com/wikimedia/mediawiki-extensions-Popups/blob/38970b488d2c02a9ef2c3f7e9d7e2adb7601f2c3/src/changeListeners/eventLogging.js#L71-L77).

Event Timeline

To be clear, the linkInteractionToken field is marked with required = false in the schema description; it can (and should) be NULL for pageLoaded events (but yeah, not undefined).

Thanks @Tbayer, good to know.

Right now this doesn't have any effects that I can see, but...

Given the deduplication mechanism we still have, we could be blocking logging of other events that don't have a linkInteractionToken (because they all get stored as tokenToSeenMap[ undefined ] and thus the next ones would not logged).

Right now there are no other events without a linkInteractionToken because the settings cog event only gets logged from the popup cog that has an interaction and thus a token.

But for example if we started logging clicks on the footer as *settings cog* events (as I believe we intend to do) without an interaction, they'd be deduplicated based on undefined token and not be logged.

As said, no immediate effects right now, but this is wrong code and needs fixing.

Thanks for the detailed write-up, @Jhernandez!

Here's what I said in the Hangout not 5 minutes ago:

  • User loads a page with PP disabled – pageLoaded event is logged with no Link Interaction Token (LIT)
  • User clicks on the footer link, opening the settings dialog
  • User enables PP – enabled event is logged with no LIT and we see a collision

And here's why I'm wrong: there's no enabled event. I'm not sure why I thought there was.

As I said, I think that this should be fixed but now I think it's Normal or Low priority.

phuedx triaged this task as Medium priority.Jun 21 2017, 4:46 PM
phuedx updated the task description. (Show Details)

Change 362315 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] Allow events without linkInteractionToken to be logged

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

Change 362315 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Allow events without linkInteractionToken to be logged

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

phuedx added a subscriber: Jdlrobson.

This is a purely technical task. I'll sign off on it /cc @Jdlrobson

@phuedx with this one you can wait and verify the T167365 (the JS part). If JS part works it means this task also works.