Page MenuHomePhabricator

Remove Page's View functionality
Closed, ResolvedPublic5 Story Points

Description

The concept of a "page model" is a common subject in MobileFrontend. It may be its most significant model. It should be easy to create, reason about, and manipulate such an important model.

However for historic reasons the Page is also a View (it used to be used to re-render a page after an edit). It doesn't need to be, in fact, a page is never rendered, it is only ever passed around. As a result, I recommend we make this a pure model.

Acceptance Criteria

  • Before writing any patches have a slack conversation outlining your plans to avoid the problems we've been hitting in our refactor work and ensure you have buy in. Make sure to share your work early and check in that everyone understands it.
  • Page / Page.test.js is a plain model. No or very minimal work in its constructor except assignment. No inheritance. There will no decrease in the amount of existing code or major API refactors so any model methods that existed previously will still be there.
  • Pull out PageJSONParser and PageJSONParser.test.js which parses JSON and returns a Page model. It clarifies in the comments what gateways or response formats it works with given the flexibility of the MW API. It exists in a distinct file.
  • The preRender is removed/renamed from the Page class to avoid confusion.

Developer notes

  • Avoid renames - they are not worth the hassle - simply work towards removing the View inheritance
  • All construction parameters including those passed via "options.prop" are well documented with JSDocs including typing and whether parameters are optional.

PageJSONParser looks like the easiest to tackle and may be a good place to start. PageView should probably come next so that it's easy to test the Page model without a DOM.

Sign off notes

The Page object is used in various places, so breaking changes to its API are going to be tricky and risky and should be avoided. As part of completing this task, follow up work should be proposed in the comments and follow up tasks created as part of sign off

QA instructions

This is one of those tickets where QA becomes 🙏 because it touched so many different parts of the mobile site. Here are probably the main ones to look out for and it might be helpful to keep the JS console open as you are browsing the site and see if you notice any out of the ordinary JS errors:

Editing

  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain and make sure you can edit using the visual editor and source editor while logged in and anonymous. I'm not sure if tests rely on this page or not so you might want to undo any of the edits you make to preserve its original text.

SearchOverlay

  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain while anonymous or logged in (shouldn't matter) and click on the search box. Search for anything making sure that the list populates like normal and that you are able to click on an item and go to that page.

Watchstar/Watchlist

  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain while logged in and click on the watchstar icon to add it to your watchlist.
  2. Click on the hamburger menu to open the main menu and click the Watchlist link and verify the page shows up in that list
  3. Go back to https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain while logged in and click on the watchstar icon to remove it from your watchlist.
  4. Click on the hamburger menu to open the main menu and click the Watchlist link and verify the page is removed from the list

Table of contents/section toggling

  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama while anon or logged in (shouldn't matter) and make sure the table of contents toggling works.
  2. Click on a section and verify that the section toggling works as well

Reference Drawer

  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama while anon or logged in (shouldn't matter) and click on a reference link. Make sure the black drawer appears with the reference

Page issues

  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Pharmacovigilance while anon (logged in is doing weird stuff with new user page) and verify that you see page issues present. Also verify that clicking on the "Learn more" link on one of them opens up the page issue overlay.

QA Results

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson renamed this task from Decouple Page's view and model to Remove Page's View functionality.Apr 2 2019, 11:46 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)

I've updated this task to remove the View part of Page entirely as it's not used anywhere. I think this fits in with our general work of moving away from inheritance and might be less painful than the overlay refactors given the resulting code doesn't touch any View related code.

Jdlrobson raised the priority of this task from Normal to High.Apr 9 2019, 7:11 PM
ovasileva set the point value for this task to 5.Apr 10 2019, 4:12 PM

Before writing any patches have a slack conversation outlining your plans to avoid the problems we've been hitting in our refactor work and ensure you have buy in. Make sure to share your work early and check in that everyone understands it.

Here's my initial plan of attack:

  • Split out PageJSONParser from Page and split out PageJSONParser.test.js from Page.test.js.
  • Replace Page.test.js helper functions with ES6 template strings or test fixture files (maybe like CtaDrawer.test.html.js or actual HTML if we can do that). The current approach is imperative and difficult to conceptualize and reason about.
  • Exercise all Page APIs in use in tests. If any APIs are unused, remove them.
  • Move Page.getRedLinks(), Page.getThumbnails(), Page.getLeadSectionElement(), and findChildInSectionLead / findSectionHeadingByIndex / this.$headings to PageHTMLParser and relevant tests from Page.test.js to PageHTMLParser.test.js. Provide temporary proxy APIs in Page.js as needed. Be mindful of properties used for caching.
  • Remove temporary proxies and migrate clients like mobile.init.js from Page to PageHTMLParser. The goal is to remove View as a parent and Page.$el. Be mindful of properties used for caching.

I hope that each bullet is 1-2 patches per repo. There will be much more work to do but I'm getting lost looking further out and I think this is a good start to dividing Page.

Change 504768 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: split JSON parsing from Page

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

Change 504979 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/MobileFrontend@master] Hygiene: replace Page.test.js helpers with ES6 strings

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

Change 504768 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: split JSON parsing from Page

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

Change 504979 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Hygiene: replace Page.test.js helpers with ES6 strings

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

Niedzielski removed Niedzielski as the assignee of this task.May 1 2019, 5:24 PM

Moving back to to-do since I'm not working on this at the moment.

Change 519699 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove isTalkPage() method from Page.js

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

Change 519699 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove isTalkPage() method from Page.js

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

Change 520146 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove getId() method from Page.js

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

Change 520477 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove isBorderBox and languageUrl options from Page.js

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

Change 520478 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove Page.js inheritance of View.js

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

Change 520146 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove getId() method from Page.js

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

Change 520477 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove isBorderBox and languageUrl options from Page.js

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

Change 520804 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Consolidate Page.js instance variables

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

Change 520478 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove Page.js inheritance of View.js

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

Change 520804 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Consolidate Page.js instance variables and get rid of this.options

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

Change 521891 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Convert Page.js to ES6 class

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

Change 521892 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Move parsing responsibilities from Page.js into PageHTMLParser.js

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

Change 521891 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Convert Page.js to ES6 class

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

Change 522136 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/skins/MinervaNeue@master] Make minerva use PageHTMLParser.js and Page.js

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

Change 522139 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Revise API to use pageHTMLParser.js

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

Change 522210 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove backwards compatibility with old Page.js parsing logic

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

Change 522544 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Clean up PageHTMLParser.js .find usage

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

Change 521892 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Move parsing responsibilities from Page.js into PageHTMLParser.js

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

Change 522136 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Make Minerva use new PageHTMLParser.js and refactored Page.js

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

Change 523346 had a related patch set uploaded (by Nray; owner: Nray):
[mediawiki/extensions/MobileFrontend@master] Remove lazyReferencesLoader 'references-loaded' event/eventBus usage

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

Change 522139 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Refactor MF to use PageHTMLParser / clean up Page.js usage

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

Change 522210 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove backwards compatibility with old Page.js parsing logic

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

Change 523346 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Remove lazyReferencesLoader 'references-loaded' event/eventBus usage

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

nray reassigned this task from nray to Edtadros.Jul 18 2019, 4:49 PM
nray updated the task description. (Show Details)
nray added a subscriber: nray.
nray updated the task description. (Show Details)Jul 18 2019, 5:00 PM
nray updated the task description. (Show Details)

Change 522544 merged by Jdlrobson:
[mediawiki/extensions/MobileFrontend@master] Clean up PageHTMLParser.js .find usage

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

Test Result

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome
Device: MBP (AC5-iPhoneX)
Emulated Device: iPhoneX

Test Artifact(s):

QA instructions
✅AC1: Editing
  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain and make sure you can edit using the visual editor and source editor while logged in and anonymous. I'm not sure if tests rely on this page or not so you might want to undo any of the edits you make to preserve its original text.
AnonymousAMC OnAMC Off
✅AC2: SearchOverlay
  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain while anonymous or logged in (shouldn't matter) and click on the search box. Search for anything making sure that the list populates like normal and that you are able to click on an item and go to that page.
Anonymous
✅AC3: Watchstar/Watchlist
  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain while logged in and click on the watchstar icon to add it to your watchlist.
  2. Click on the hamburger menu to open the main menu and click the Watchlist link and verify the page shows up in that list
Logged In
  1. Go back to https://en.m.wikipedia.beta.wmflabs.org/wiki/Spain while logged in and click on the watchstar icon to remove it from your watchlist.
  2. Click on the hamburger menu to open the main menu and click the Watchlist link and verify the page is removed from the list
Logged In
✅AC4: Table of contents/section toggling
  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama while anon or logged in (shouldn't matter) and make sure the table of contents toggling works.
  2. Click on a section and verify that the section toggling works as well
AnonymousAMC OnAMC Off
✅AC5: Reference Drawer
  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Barack_Obama while anon or logged in (shouldn't matter) and click on a reference link. Make sure the black drawer appears with the reference
Anonymous
✅AC6: Page issues
  1. Go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Pharmacovigilance while anon (logged in is doing weird stuff with new user page) and verify that you see page issues present. Also verify that clicking on the "Learn more" link on one of them opens up the page issue overlay.
Anonymous
Edtadros reassigned this task from Edtadros to ovasileva.Thu, Jul 25, 2:06 AM
Edtadros updated the task description. (Show Details)
Edtadros added a subscriber: Edtadros.
nray removed ovasileva as the assignee of this task.Thu, Jul 25, 5:05 PM
Niedzielski closed this task as Resolved.Thu, Jul 25, 5:50 PM
Niedzielski updated the task description. (Show Details)