Page MenuHomePhabricator

Merge mpga branch into master
Closed, ResolvedPublic5 Story Points

Description

Pre-merge

  • QA tested before merge (T156292: Page Previews full feature QA)
  • Evaluate and eradicate potential sources of caching issues.
    • See below. This feature MUST be enabled as a beta feature, which only logged-in users can enabled. Consequently, there'll be no caching issues.
  • Ensure that Popups is only enabled for beta feature users in production.
  • High five someone EVERYONE ๐Ÿ™

Signoff AC: Post-merge

  • Resolve T155386 since the documentation is up to date.
  • Review existing Hovercards bugs and resolve any bugs fixed by the refactor and take immense pleasure in seeing the backlog shrink.
  • High five someone EVERYONE ๐Ÿ™

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
phuedx removed phuedx as the assignee of this task.Feb 7 2017, 6:20 PM
Jdlrobson updated the task description. (Show Details)Feb 8 2017, 6:52 PM
phuedx updated the task description. (Show Details)Feb 8 2017, 7:05 PM

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

Jdlrobson updated the task description. (Show Details)Feb 8 2017, 10:16 PM

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

phuedx assigned this task to Jdlrobson.Feb 9 2017, 2:28 PM

@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

@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

@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?
Jdlrobson updated the task description. (Show Details)Feb 10 2017, 8:03 PM
phuedx added a comment.EditedFeb 13 2017, 6:30 AM
  • 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.
phuedx updated the task description. (Show Details)Feb 13 2017, 6:31 AM

@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

I think we decided this wasn't a blocker

  • devs: Do we want to squash or fast forward?
Jhernandez updated the task description. (Show Details)Feb 13 2017, 6:10 PM

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.

Jdlrobson updated the task description. (Show Details)Feb 13 2017, 7:41 PM

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

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

Jdlrobson removed Jdlrobson as the assignee of this task.Feb 14 2017, 1:34 AM
Jdlrobson removed a project: Patch-For-Review.
Jdlrobson updated the task description. (Show Details)
Jhernandez updated the task description. (Show Details)Feb 14 2017, 10:45 AM

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.

Olga - what's the Wikidata situation?

Jdlrobson removed ovasileva as the assignee of this task.Feb 14 2017, 8:12 PM

Left note on https://www.wikidata.org/wiki/MediaWiki_talk:Gadget-PopupsFix.js
The branch has been merged and will roll out on the train https://gerrit.wikimedia.org/r/337625 to replacing the existing beta feature.

Bizarrely it's not showing up in beta features despite wgPopupsBetaFeature being true when I check it in the console it's false.

> mw.popups.isEnabled(mw.user, mw.popups.createUserSettings( mw.storage ), mw.config)
> false
> mw.config.get( 'wgPopupsBetaFeature' )
> false
> mw.config.get( 'wgPopupsShouldSendModuleToUser' )
> false

We should debug...

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.

ANDDDDD we are live. if you view https://en.wikipedia.beta.wmflabs.org/wiki/Bear with the beta feature enabled you will see the nice shiny popups.

@ABorbaWMF are there any tests you can/want to run on the beta cluster.

This should roll out to all wikis starting the 21st February

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

Jdlrobson updated the task description. (Show Details)Feb 15 2017, 12:06 AM

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.

Prtksxna removed a subscriber: Prtksxna.Feb 16 2017, 3:45 AM

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

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

@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

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

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 updated the task description. (Show Details)Feb 26 2017, 12:03 PM

@ovasileva: Do you want to resolve this or should I? ๐Ÿ˜„

phuedx reassigned this task from ABorbaWMF to ovasileva.Feb 28 2017, 1:15 PM

:) @phuedx done and done.

ovasileva closed this task as Resolved.Feb 28 2017, 3:33 PM