Ping @Tbayer!

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.

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

In T171287#3486947, @bmansurov wrote:

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:

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!

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

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.

In T171287#3500537, @pmiazga wrote:

@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

In T171287#3511385, @phuedx wrote:

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

In T171287#3511403, @phuedx wrote:

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

🎉🎉🎉

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