Change 336718 had a related patch set uploaded (by Jdlrobson):
Only add Popups code to output page where needed

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

The above patch should leave us in a position where we can make this merge happen.

@Jdlrobson, @bmansurov, @pmiazga: Firstly, apologies for splitting the conversation between Gerrit and Phab, but I find it easier to write here.

Now, I agree with @bmansurov that the name of PopupsContext#isEnabled{By,For}User is wrong. Instead, I think we should be talking about whether or not the ext.popups RL module should be added to the output. This might be why we're having trouble deciding which way to cut it.

How about renaming it to PopupsContext#shouldSendModuleToUser?

PopupsContext#shouldSendModuleToUser sounds good to me.

+1 to PopupsContext#shouldSendModuleToUser. The idea of passing "isEnabled" from backend to frontend evolved a lot and currently, we have different flow. I think that shouldSendModuleToUser is much more descriptive -> +1

The rename is done in https://gerrit.wikimedia.org/r/336945

@Jdlrobson, I suppose the following requirement is true only when the Beta Feature mode is enabled, right?

• Ensure Popups code does not load for anonymous users.

Change 336718 merged by jenkins-bot:
Only add Popups code to output page where needed

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

Change 337064 had a related patch set uploaded (by Jdlrobson):
Disable Hungarian Popups A/B test

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

In T156290#3017892, @bmansurov wrote:

@Jdlrobson, I suppose the following requirement is true only when the Beta Feature mode is enabled, right?

• Ensure Popups code does not load for anonymous users.

$wgPopupsBetaFeature is true on all sites except loginwiki (not sure what that is). Note we are apparently still A/B testing Hungarian wiki so I'm not sure how to interpret "Make sure beta is on for huwiki" other than writing a patch to disable the A/B test.

Open questions that we need answers for to continue the work

• @ovasileva am I right to think we want to disable the Hungarian A/B test in production prior to rolling out the rewrite?
• @ovasileva can we merge to master? if not what needs to happen
• @Jhernandez Is webpack migration a blocker?
• devs: Do we want to squash or fast forward?

In T156290#3017954, @Jdlrobson wrote:

• devs: Do we want to squash or fast forward?

git checkout master
git pull
git merge mpga --no-ff

Will:

• Ensure that there's an object for Gerrit to accept – I see no problem with self-merging here either.
• Preserve history, which squashing wouldn't.

In T156290#3017954, @Jdlrobson wrote:

In T156290#3017892, @bmansurov wrote:

@Jdlrobson, I suppose the following requirement is true only when the Beta Feature mode is enabled, right?

• Ensure Popups code does not load for anonymous users.

$wgPopupsBetaFeature is true on all sites except loginwiki (not sure what that is). Note we are apparently still A/B testing Hungarian wiki so I'm not sure how to interpret "Make sure beta is on for huwiki" other than writing a patch to disable the A/B test.

Open questions that we need answers for to continue the work

• @ovasileva am I right to think we want to disable the Hungarian A/B test in production prior to rolling out the rewrite?

or at the same time, turn a/b test off, turn on beta feature for hungarian

• @ovasileva can we merge to master? if not what needs to happen

I'd like to see T156800: Display Page Preview images when using the RESTBase endpoint resolved first. After that, no blockers

• @Jhernandez Is webpack migration a blocker?

I think we decided this wasn't a blocker

• devs: Do we want to squash or fast forward?

We talked about this in standup. Nothing blocking from @ovasileva 's end but @Jhernandez wants to add a few things (he will add blocking tasks) so please don't merge until he's given the green light to do so.

Change 337064 merged by jenkins-bot:
Disable Hungarian Popups A/B test

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

As far as I'm concerned, the tooling change hasn't broken anything, so I'm good.

I've noticed something though, opened T158044: As an anon, Popups remain disabled after navigating (mpga). Not sure if that is the behavior we want. Ping @ovasileva

@Jhernandez - checked behavior, it's okay. Since we're merging with the beta option, technically the enable previews link should not appear. It was put there I believe by @bmansurov (or maybe @phuedx) so that we can test anon behavior after the merge.

The Popups extension on the staging server had been checked out to a branch that wasn't tracking an upstream branch so it was missing (amongst other things) the changes related to T156333: Technical: Use bundler to generate Popups JavaScript code. I've checked out the extension to the mpga branch and git pulled.

Thanks @phuedx, I've re-done a mini QA and everything seems fine now as a beta feature. As far as I'm concerned, branch is mergeable.

I think https://gerrit.wikimedia.org/r/337636 is the issue so should only impact beta cluster. Going to see if I can find some one to SWAT that to confirm.

Also for those interested.. this is what happens on Wikidata due to TextExtracts not being installed:

I don't understand the description well enough to test this. If anyone does know how to test this, please post a list of steps I can use to test the functionality all by myself, and I will try to recreate the scenario.

@Nicholas.tsg - for now @ABorbaWMF and I will be testing through these tasks - we will need your help when testing in production next week - I will ping you separately for that. Sorry for the confusion!

tested through all non-preferences test cases on Catalan and Hebrew wikipedias from https://docs.google.com/spreadsheets/d/1FTegYsAfpfsID66nhyfcfGB9N8LzXk7jfNkTehuR29A/edit#gid=1550308107, opened T158858: Horizontal gradient appearing on right side of text for RTL page previews
@ABorbaWMF - could you review the results? I've added the names of articles and hovers I test through in the sheet

@ovasileva I took a look at the testcases you ran and I see the same behaviors for the most part.

• There's still some problems with bolded text. This is a known issue.
• Page previews reopen after closing a new tab. https://phabricator.wikimedia.org/T158631
• On the star trek page, hovering over star trek enterprise does not show page image. Should this be a new bug?
• I was able to find a missing page preview on https://ca.wikipedia.org/wiki/Lee_Perry hovering over '2003'. In this case, it passed.
• I was not able to find a page with no-include text. Do you have an example?
• RTL language previews have an incorrect horizontal gradient. https://phabricator.wikimedia.org/T158858

The main difference I noticed was the lack of previews specific dialog(s). I have to login to turn on page previews and if I click the settings cog within a preview, the preferences page appears. This seems correct, but I don't know how to test the case for the page previews settings dialog(s).

In T156290#3054063, @ABorbaWMF wrote:

@ovasileva I took a look at the testcases you ran and I see the same behaviors for the most part.

• There's still some problems with bolded text. This is a known issue.
• Page previews reopen after closing a new tab. https://phabricator.wikimedia.org/T158631
• On the star trek page, hovering over star trek enterprise does not show page image. Should this be a new bug?

yup. logged as T158632: Previews not displaying non-free images

• I was able to find a missing page preview on https://ca.wikipedia.org/wiki/Lee_Perry hovering over '2003'. In this case, it passed.

good choice of test page :)

• I was not able to find a page with no-include text. Do you have an example?

No, but it's okay for now since we're expecting that case to fail

• RTL language previews have an incorrect horizontal gradient. https://phabricator.wikimedia.org/T158858

The main difference I noticed was the lack of previews specific dialog(s). I have to login to turn on page previews and if I click the settings cog within a preview, the preferences page appears. This seems correct, but I don't know how to test the case for the page previews settings dialog(s).

This is due to preview being in beta mode. Actual behavior can be tested on https://se.wikimedia.org/wiki/Huvudsida where previews are live. It seems to be working fine, but I will be testing it more thoroughly tomorrow.

After two grooming sessions, I'm checking "Review existing Hovercards bugs and resolve any bugs fixed by the refactor and take immense pleasure in seeing the backlog shrink" as complete.

@ovasileva: Do you want to resolve this or should I? 😄

:) @phuedx done and done.