Page MenuHomePhabricator
Create Task
Maniphest T147944

[EPIC] Remove SkinMinervaBeta class
Closed, ResolvedPublic
Actions

Assigned To
Jdlrobson
Authored By
Jdlrobson
Oct 12 2016, 5:42 PM
Tags
Referenced Files
None
Subscribers
Aklapper
AndyRussG
• bmansurov
gerritbot
Jdlrobson
• Jhernandez
• jhobs
View All 9 Subscribers
Tokens
"Love" token, awarded by jhobs.

Description

Given our new feature flagged approach it is not helpful to use whether a user is in stable or beta as an indication that a feature is enabled. We have done a lot of work to remove the SkinMinervaBeta class and its friends and introduce feature flagging which allows us to control which features are enabled on stable or beta via a config change.

Work so far

Beta differs from stable in several ways let's clean this up:

  • We currently load a talk icon for the talk button . Make a decision on T142976 (In meantime I suggest moving the existing beta only flagged code to stable) (no longer valid)
  • We currently disable CentralNotice on beta. Let's remove that logic and enable it in beta too.
  • Fontchanger should be feature flagged and enabled inside SkinMinerva.php (see the subtask T148193)
  • Back to top should be feature flagged and enabled inside SkinMinerva.php

Wait for the outcome of T148199:

  • remove the skins.minerva.beta.scripts

Let's complete the transition

  • Remove SkinMinervaBeta.php
  • Remove MinervaTemplateBeta

Remaining work

  • Resolve the 2 remaining subtasks
  • With subtasks resolved, submit a patch associated with this task that drops isBetaGroupMember from resources/mobile.startup/context.js from the codebase

Estimation notes

If estimating this task do so for the final subtask. It doesn't seem helpful at this time to create another subtask just for that.

Details

SubjectRepoBranchLines +/-
Remove SkinMinervaBeta.phpmediawiki/extensions/MobileFrontendmaster+4 -24
Get rid of the `mode` property of SkinMinerva.phpmediawiki/extensions/MobileFrontendmaster+6 -15
Hygiene: rename a variablemediawiki/extensions/MobileFrontendmaster+4 -4
Remove unneeded padding from Special:Mobiledifmediawiki/extensions/MobileFrontendmaster+1 -2
Feature flag "BackToTop"mediawiki/extensions/MobileFrontendmaster+38 -14
Do not disable CentralNotice in betamediawiki/extensions/MobileFrontendmaster+0 -15
Remove dead code in SkinMinervaBetamediawiki/extensions/MobileFrontendmaster+0 -69
Customize query in gerrit

Related Objects
Search...

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
• Jhernandez subscribed.Oct 18 2016, 1:19 PM

This is good.

Redoing beta as a different default for the feature flags is a great idea I believe.

I also think we should talk about what @phuedx mentioned above and not just increase the size/complexity of SkinMinerva/MinervaTemplate.

Jdlrobson updated the task description. (Show Details)Oct 18 2016, 4:52 PM

@phuedx it's my feeling that on the longer term we should remove some of the experiments, maybe restricting the number in flight at a time. Given my analysis suggests we don't have many features in flight at current time now seems a great time to do this. Yes there is overlap with T141087 and this could inform / help drive that. Should we make this a subtask?

phuedx added a comment.Oct 19 2016, 5:54 PM
In T147944#2725915, @Jdlrobson wrote:

@phuedx it's my feeling that on the longer term we should remove some of the experiments, maybe restricting the number in flight at a time. Given my analysis suggests we don't have many features in flight at current time now seems a great time to do this. Yes there is overlap with T141087 and this could inform / help drive that. Should we make this a subtask?

To be clear, I wasn't suggesting that we don't do this. I was suggesting that removing {SkinMinerva,MinervaTemplate}Beta might only be half of the solution if a goal is to reduce technical debt.

Jdlrobson renamed this task from Remove SkinMinervaBeta to Remove SkinMinervaBeta class.Oct 20 2016, 12:27 AM
Jdlrobson updated the task description. (Show Details)
gerritbot added a comment.Oct 20 2016, 8:43 PM

Change 315540 merged by jenkins-bot:
Remove dead code in SkinMinervaBeta

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

ReleaseTaggerBot added a project: MW-1.28-release (WMF-deploy-2016-10-25_(1.28.0-wmf.23)).Oct 20 2016, 9:00 PM
phuedx mentioned this in T148197: Unify the logic to brand the search functionality across mobile modes.Oct 26 2016, 11:11 AM
phuedx mentioned this in T145517: Minerva feature classes leak into Vector skin.Oct 27 2016, 7:58 AM
phuedx closed subtask T148196: Remove the donation link feature as Resolved.Nov 1 2016, 10:42 AM
phuedx closed subtask T148197: Unify the logic to brand the search functionality across mobile modes as Resolved.Nov 1 2016, 4:03 PM
Jdlrobson removed a project: Patch-For-Review.Nov 8 2016, 11:43 AM
• bmansurov mentioned this in T148199: [Spike: 1hr] Identify differences between SkinMinervaBeta and SkinMinerva.Nov 23 2016, 11:27 PM
• bmansurov closed subtask T148199: [Spike: 1hr] Identify differences between SkinMinervaBeta and SkinMinerva as Resolved.
• bmansurov updated the task description. (Show Details)
ovasileva closed subtask T142976: Talk icon behaviour in beta different to stable. In stable it isn't loaded in non-RL-capable UAs as Resolved.Nov 28 2016, 11:56 AM
ovasileva closed subtask T148195: Feature flag categories feature as Resolved.Nov 28 2016, 11:59 AM
gerritbot added a comment.Dec 12 2016, 11:01 PM

Change 326840 had a related patch set uploaded (by Bmansurov):
Feature flag disabling of CentralNotice

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

gerritbot added a project: Patch-For-Review.Dec 12 2016, 11:01 PM
gerritbot added a comment.Dec 12 2016, 11:14 PM

Change 326841 had a related patch set uploaded (by Bmansurov):
Feature flag "BackToTop"

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

• bmansurov updated the task description. (Show Details)Dec 12 2016, 11:15 PM
gerritbot added a comment.Dec 12 2016, 11:51 PM

Change 326850 had a related patch set uploaded (by Bmansurov):
Remove SkinMinervaBeta.php

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

gerritbot added a comment.Dec 12 2016, 11:51 PM

Change 326851 had a related patch set uploaded (by Bmansurov):
Get rid of the mode property of SkinMinerva.php

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

• bmansurov added a subscriber: ovasileva.Dec 13 2016, 4:29 PM

@ovasileva in beta we disable CentralNotice. Do you think we should enable it there too? I don't know how that decision was made, but it doesn't look right as users in beta won't be getting any notices.

• bmansurov added projects: Unplanned-Sprint-Work, Reading-Web-Sprint-87-♨️😭.Dec 14 2016, 1:52 PM
ovasileva added a comment.Dec 14 2016, 3:12 PM

@bmansurov - I agree, let's enable it. Could we do this here or under a separate card?

• bmansurov updated the task description. (Show Details)Dec 14 2016, 3:19 PM

@ovasileva we can do it here.

Jdlrobson updated the task description. (Show Details)Dec 14 2016, 6:15 PM
ovasileva set the point value for this task to 5.Dec 14 2016, 6:24 PM
ovasileva moved this task from Needs Analysis to Needs More Work on the Reading-Web-Sprint-87-♨️😭 board.
• bmansurov claimed this task.Dec 14 2016, 6:31 PM
AndyRussG subscribed.Dec 16 2016, 4:49 PM
• bmansurov moved this task from Needs More Work to Code Review on the Reading-Web-Sprint-87-♨️😭 board.Dec 16 2016, 5:33 PM
gerritbot added a comment.Dec 16 2016, 8:27 PM

Change 326840 merged by jenkins-bot:
Do not disable CentralNotice in beta

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

gerritbot added a comment.Dec 16 2016, 8:28 PM

Change 326841 merged by jenkins-bot:
Feature flag "BackToTop"

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

Jdlrobson added a comment.Dec 16 2016, 8:31 PM

My remaining concern on the last patchset is with the future of T125588 and opening ourselves up to issues with varying on context. There's a little risk with this one - we should be careful that beta functionality doesn't leak to stable.

Jdlrobson updated the task description. (Show Details)Dec 16 2016, 8:32 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)
ReleaseTaggerBot added a project: MW-1.29-release (WMF-deploy-2017-01-03_(1.29.0-wmf.7)).Dec 16 2016, 9:00 PM
• bmansurov added a comment.Dec 16 2016, 9:19 PM
In T147944#2882358, @Jdlrobson wrote:

My remaining concern on the last patchset is with the future of T125588 and opening ourselves up to issues with varying on context. There's a little risk with this one - we should be careful that beta functionality doesn't leak to stable.

We can ignore the last patchset for now then. I think the task can be considered done without it too.

Jdlrobson moved this task from Code Review to Needs More Work on the Reading-Web-Sprint-87-♨️😭 board.Dec 16 2016, 9:51 PM
• bmansurov added a project: Reading-Web-Sprint-88-¯\_(ツ)_/¯.Dec 19 2016, 5:09 PM
• bmansurov moved this task from Needs Analysis to Needs More Work on the Reading-Web-Sprint-88-¯\_(ツ)_/¯ board.
MBinder_WMF removed a project: Unplanned-Sprint-Work.Dec 19 2016, 5:21 PM
• bmansurov created subtask T153971: Feature-flag media viewer swipe functionality.Dec 22 2016, 8:20 PM
gerritbot added a comment.Dec 22 2016, 8:25 PM

Change 328721 had a related patch set uploaded (by Bmansurov):
Hygiene: rename a variable

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

• bmansurov created subtask T153973: Deploy flicker-free search for all users.Dec 22 2016, 8:38 PM
gerritbot added a comment.Dec 22 2016, 8:45 PM

Change 328723 had a related patch set uploaded (by Bmansurov):
Remove unneeded padding from Special:Mobiledif

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

• bmansurov created subtask T153975: Feature flag the 'Expand all sections' feature in Special:MobileOptions.Dec 22 2016, 8:49 PM
gerritbot added a comment.Dec 23 2016, 6:54 AM

Change 328723 merged by jenkins-bot:
Remove unneeded padding from Special:Mobiledif

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

gerritbot added a comment.Dec 23 2016, 9:19 AM

Change 328721 merged by jenkins-bot:
Hygiene: rename a variable

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

ovasileva closed subtask T148193: Feature flag font-changer as Resolved.Jan 3 2017, 4:06 PM
• bmansurov added a comment.Jan 3 2017, 7:36 PM

This task is not actionable until the new subtasks are resolved. I suggested we put it back to backlog and prioritize the subtasks. @ovasileva what do you say?

• bmansurov removed • bmansurov as the assignee of this task.Jan 4 2017, 6:04 PM
MBinder_WMF removed a project: Patch-For-Review.Jan 17 2017, 6:11 PM
gerritbot added a comment.Jan 26 2017, 4:16 PM

Change 326851 abandoned by Bmansurov:
Get rid of the mode property of SkinMinerva.php

Reason:
Not working on it.

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

gerritbot added a comment.Jan 26 2017, 4:17 PM

Change 326850 abandoned by Bmansurov:
Remove SkinMinervaBeta.php

Reason:
not working on it now

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

Jdlrobson updated the task description. (Show Details)Apr 13 2017, 11:52 PM
Jdlrobson added a subtask: T107943: Improve/Move ImageOverlay Swipe function (to stable).
Jdlrobson updated the task description. (Show Details)Apr 20 2017, 11:24 PM
Jdlrobson added a subtask: T143416: Remove mobile.mediaviewer swipe feature in beta .Apr 26 2017, 8:24 PM
Jdlrobson mentioned this in T143416: Remove mobile.mediaviewer swipe feature in beta .
Jdlrobson closed subtask T153973: Deploy flicker-free search for all users as Resolved.Apr 27 2017, 11:18 PM
Jdlrobson updated the task description. (Show Details)May 5 2017, 7:44 AM

So SkinMinervaBeta no longer exists. Probably need to rename this task as there's a little more to it apparently.

Jdlrobson moved this task from Epics/Goals to Needs Prioritization on the Web-Team-Backlog board.May 5 2017, 7:44 AM
ovasileva added a comment.May 9 2017, 3:14 PM

@Jdlrobson - could you expand the description or add any necessary subtasks?

Jdlrobson lowered the priority of this task from High to Low.May 11 2017, 12:11 PM
Jdlrobson updated the task description. (Show Details)May 11 2017, 12:14 PM
Jdlrobson removed the point value for this task.
Krinkle removed a project: MW-1.29-release (WMF-deploy-2017-01-03_(1.29.0-wmf.7)).May 25 2017, 2:15 PM
Jdlrobson renamed this task from Remove SkinMinervaBeta class to [EPIC] Remove SkinMinervaBeta class.May 26 2017, 6:08 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Needs Prioritization to Upcoming on the Web-Team-Backlog board.
Jdlrobson raised the priority of this task from Low to Medium.May 26 2017, 6:15 PM
Jdlrobson moved this task from Upcoming to 2016-17 Q4 on the Web-Team-Backlog board.Jun 2 2017, 4:14 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson added projects: Unplanned-Sprint-Work, Readers-Web-Kanbanana-Board-Old.

https://gerrit.wikimedia.org/r/#/c/356769/ completes the work here so I'm moving into current sprint to ensure we resolve it.

Jdlrobson moved this task from To Do to Needs Code Review on the Readers-Web-Kanbanana-Board-Old board.Jun 2 2017, 4:15 PM
• bmansurov moved this task from Needs Code Review to Ready for Signoff on the Readers-Web-Kanbanana-Board-Old board.Jun 5 2017, 2:38 PM

The above patch has been merged.

Jdlrobson closed this task as Resolved.Jun 5 2017, 11:58 PM
Jdlrobson claimed this task.
Jdlrobson updated the task description. (Show Details)

Let's continue the conversation in https://phabricator.wikimedia.org/T141087

Jdlrobson closed subtask T143416: Remove mobile.mediaviewer swipe feature in beta as Resolved.Jun 6 2017, 12:00 AM
Jdlrobson closed subtask T153975: Feature flag the 'Expand all sections' feature in Special:MobileOptions as Resolved.Jun 6 2017, 12:04 AM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL · Credits