Page MenuHomePhabricator

Expose whether previews are enabled for third-party JavaScript
Closed, ResolvedPublic2 Story Points

Description

Background

The Fundraising team will be running tests every Wednesday, throughout August. Because their analysts don't believe that we should apply the results from the previous test on itwiki to other wikis, the Fundraising team have requested that Page Previews re-expose whether previews are enabled so that it can be included in the analysis of the new tests.

AC

If the Popups extension is loaded on the wiki, then:

  • mw.popups.isEnabled() method returns true when previews are enabled for the user; otherwise, it returns false.
  • This is the case for all users.
  • it returns false when popups conflicts with Navigational popups
  • it works properly when popups are enabled as a beta feature

Developer Notes

  • When popups are set up as a beta feature and they are disabled by user we do not ship Javascript code to the frontend. Because of that there is no possibility to setup mw.popups object.
  • We talked about implementing this as a ES5 property access with a getter but not a setter, so that it is clear that the global variable is read only and cannot be set to manipulate popups. Also we are keeping the mw.popups.enabled interface so that the fundraising team don't have to update their code. Pseudocode:
mw.popups = {
    isEnabled = function() { return ... }
}
  • Also @phuedx mentioned using the getter to return the data from the store directly, instead of having to implement a change listener.

Event Timeline

phuedx created this task.Jul 21 2017, 10:10 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 21 2017, 10:10 AM
ovasileva triaged this task as High priority.Jul 21 2017, 10:26 AM
phuedx updated the task description. (Show Details)Jul 24 2017, 3:46 PM
Jdlrobson set the point value for this task to 2.Jul 25 2017, 4:25 PM

We talked about implementing this as a member access with a getter but not a setter.

mw.popups = {}
Object.defineProperty(mw.popups, 'enabled', {
  get: function() { return this.a + 1; },
  set: function() { throw new Exception('You can\'t set mw.popups.enabled'); }
});

Also @phuedx mentioned using the getter to return the data from the store directly instead of having to implement a change listener.

phuedx updated the task description. (Show Details)Jul 25 2017, 4:30 PM
Jhernandez updated the task description. (Show Details)Jul 25 2017, 4:32 PM

Open question: when should this variable be set? Do we want to set it whenever we're ready to do so, or do we want to subscribe to an event and set it then. The former approach isn't desirable as the fundraising team will have to poll the status of this code periodically. The latter approach may also help us not complicate things with getters and setters.

@bmansurov How do they check for the variable now? Ideally we would emit a page-previews.ready with mw.hook but there was a concern about making the FR team change their code.

I think whatever method they had to check the variable before they already have implemented, but I'm not sure.

Not sure, do we know anyone from the FR team that we can ping here? cc @ovasileva

pmiazga claimed this task.

Not sure, do we know anyone from the FR team that we can ping here? cc @ovasileva

@Pcoombe: The question is: How does the banner code check for the variable now? Do you wait until the page has loaded and read the variable if it's set? Do you wait until the ext.popups RL module has loaded?

@phuedx We check for mw.popups.enabled within the mw.centralNotice.bannerData.alterImpressionData() function. CentralNotice banners usually load after the rest of the page, but if needed we can add a wait until after that module has loaded to be safe.

We do not use mw.popups namespace any more. Also, we do not pass enabled from the backend as there is some extra logic on the frontend side - anynomous users can disable page previews and configuration value is stored in localStorage), Once Popups module is loaded we can set up a mw.popups.enabled as a read-only property for you.

@pmiazga Yes, we're aware it has changed. This task is to restore a similar functionality (the name of the variable can of course change if needed) so that fundraising banners can again tell if page previews are enabled.

@Pcoombe last thing. This variable can change during one pageview. As I said the anonymous user can disable PagePreviews feature without page refresh. If banner interaction also works without page refresh the best approach would be to check mw.popups.enabled after user action, not after module load.

We send the log that a user has seen a banner before they interact though. Do you have any idea from previous testing how many people might disable the feature? Unless it's a lot, I think we'll be okay to just use the state at page load.

It isn't seemingly a problem to change the mw.popups.enabled interface for FR so I think it is a good idea exposing a function to get the status, something like mw.popups.isEnabled(), instead of a variable and dealing with getters and setters.

@Pcoombe I agree giving you are logging before interactions. The initial popups enabled state after the RL module has loaded seems good enough here.

Change 369711 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/Popups@master] Allow 3rd party to check Popups enabled state by accessing mw.popups object

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

I created a simple method mw.popups.isEnabled() which returns true or false.

Tested on chrome with following scenarios:

Testmw.popups.isEnabled()Pass
Logged in user, popups enabledtrue
logged in user, popups disabled via preference pagefalse
logged in user, Navigational Popups conflicts with PagePreviewsfalse
anonymous user, popups enabledtrue
anonymous user, popups disabledfalse
anomouse user, visited page with popups enabled, disabled popupsfalse
Popups as beta feature, logged in user, popups enabledtrue
Popups as beta feature, logged in user, popus disabledfailure
Popups extension is not installedfailure

Last two test scenario fails because when Popups are a beta feature and they are disabled we do not send JS code to the user. Same happens when the extension is not present. When there is no popups JS code in frontend there is nothing that could initialize mw.popups object.

To be sure that getter works in all possible scnearios please verify if mw.popups object exists first.
Sample code to retreive popups isEnabled state

mw.popups && mw.popups.isEnabled()

@Pcoombe please let me know if this solution fits your needs.

@pmiazga That's great, thanks!

pmiazga updated the task description. (Show Details)Aug 2 2017, 7:58 PM
pmiazga updated the task description. (Show Details)Aug 2 2017, 8:00 PM
pmiazga moved this task from Doing to Needs Code Review on the Readers-Web-Kanbanana-Board-Old board.
pmiazga added a comment.EditedAug 4 2017, 11:21 AM

@Jhernandez I added simple unit tests, but there are two questions bouncing in my head:

  • is it better to add it as an integration test and spin whole Popups structure to retrieve store instead of mocking store? Looks like not necessary thing because there is big integration.test.js and it should pick up the error, but if we change preview.enabled and fix the integration.test.js the mwpopups test won't catch it as it's mocking everything. It's an edge case but I'm more than sure no one is going to test mw.popups.isEnabled() any more and integration test sounds more appropriate.
  • if we still stick to mocking I don't think we need redux-mock-store to test functionality around Redux.Store. We're not dispatching stuff, not listening to changes, the only thing we require is to take the store and call getState() to retrieve current state. Using an external library for that sounds teasing but personally, I don't see a valid reason to use it. Do you see any value in bringing the redux-mock-store lib?
phuedx added a comment.EditedAug 9 2017, 8:53 AM

I've +1'd @pmiazga's change. If we're going to run a test in August, both this and T171853: Create A/B test strategy for en- and dewiki tests need to be done (merged, QA'd, and deployed). @pmiazga's approach is consistent with the rest of the codebase, so I'm inclined to say that we should investigate webpack's library and libraryTarget config variables later.

Edit

… and by "investigate", I mean finding out whether the alternative approach works and weighing up the pros and cons of both.

phuedx added a comment.Aug 9 2017, 8:59 AM

@Jhernandez I added simple unit tests, but there are two questions bouncing in my head:

  • is it better to add it as an integration test and spin whole Popups structure to retrieve store instead of mocking store? Looks like not necessary thing because there is big integration.test.js and it should pick up the error, but if we change preview.enabled and fix the integration.test.js the mwpopups test won't catch it as it's mocking everything. It's an edge case but I'm more than sure no one is going to test mw.popups.isEnabled() any more and integration test sounds more appropriate.

Given that the integration tests mostly test the preview reducer and that that reducer holds the enabled state (for better or worse), this shouldn't be too difficult. Per T171287#3511385, though, please consider submitting this change as a follow-up.

Change 370790 had a related patch set uploaded (by Jhernandez; owner: Jhernandez):
[mediawiki/extensions/Popups@master] Hygiene: make integrations/mwpopups pure

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

Change 369711 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Allow 3rd party to check Popups enabled state by accessing mw.popups object

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

@pmiazga's approach is consistent with the rest of the codebase

Kind of. Jon has a point, we've favored pure functions every where else using src/index.js as the place where globals are read/set.

, so I'm inclined to say that we should investigate webpack's library and libraryTarget config variables later.

Sure. I like the explicitness of registering the global in the source also.

I've submitted a followup hygiene that moves the global set to index.js and keeps integration/mwpopups pure which I think is a nice middle ground.

Given that the integration tests mostly test the preview reducer and that that reducer holds the enabled state (for better or worse), this shouldn't be too difficult. Per T171287#3511385, though, please consider submitting this change as a follow-up.

@pmiazga addressed my comments and added unit tests to his patch. So this is done. No followups needed.

It is better to not merge patches that don't have tests, specially in a code base where everything is tested.

@Jdlrobson @pmiazga If you can have a look at {T171287#3511447} before resolving this out that would be great.

Skipping QA as this is a technical task.

@Jhernandez any reason the webpack config bit didn't make sense (asking out of my own curiosity)?

Change 370790 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Hygiene: make integrations/mwpopups pure

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

@Jdlrobson I suppose they would be equivalent in the end, but the config work seemed too obfuscating for this use case:

  • Webpack library config:
    • Edit test and understand webpack config settings
      • output.library
      • output.libraryExport
      • output.libraryTarget
    • Add an export default in index.js
  • Export global in index.js
    • Add global registration in entry point

If we had a library that we had to export for npm and compile too for script usage it would be more worth it I guess. I haven't discarded it completely, just implemented what I knew is reasonable, since in this task there are comments about discussing in the future if we should use that config.

Btw, the documentation linked in the previous comments is for webpack v1. Be very careful to not go to webpack.github.io which shows up quite a lot on google searches.

The up to date documentation is here: https://webpack.js.org/configuration/output/#output-library

Jdlrobson removed pmiazga as the assignee of this task.
Jdlrobson added a subscriber: pmiazga.

Thanks for the summary! So my understanding is that it's possible, just an unreasonable amount of effort to support this particular task.

Who's going to sign this off..??

phuedx claimed this task.Aug 9 2017, 5:02 PM

I don't mind taking the time to sign this off.

phuedx added a comment.EditedAug 15 2017, 10:25 AM

🎉🎉🎉


Notes

I tested the logged in scenarios on the Beta Cluster and the logged out scenarios on my MediaWiki Vagrant instance.

Logged in

  • When Page Previews (PP) is configured as a beta feature:
    • With the beta feature enabled, console.log( mw.popups.isEnabled() ); // => true
    • With the beta feature disabled, console.log( mw.popups.isEnabled() ); // => false
    • With the beta feature enabled and the Navigational Popups gadget enabled, console.log( mw.popups.isEnabled() ); // => false
  • When PP isn't configured as a beta feature (it's a user preference for logged in users):
    • With the preference enabled, console.log( mw.popups.isEnabled() ); // => true
    • With the preference disabled, console.log( mw.popups.isEnabled() ); // => false

Logged out

  • When PP isn't configured as a beta feature (see above):
    • By default, console.log( mw.popups.isEnabled() ); // => true
    • After disabling the feature via the settings menu, console.log( mw.popups.isEnabled() ); // => false
phuedx updated the task description. (Show Details)Aug 15 2017, 10:25 AM
phuedx updated the task description. (Show Details)Aug 15 2017, 10:31 AM
phuedx closed this task as Resolved.