Page MenuHomePhabricator

[EPIC] Remove SkinMinervaBeta class
Closed, ResolvedPublic

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.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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)

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.

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

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

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

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

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

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

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

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

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

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

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 20 2017, 11:24 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 - 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.
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 Analysis to Upcoming on the Readers-Web-Backlog board.
Jdlrobson raised the priority of this task from Low to Normal.May 26 2017, 6:15 PM
Jdlrobson updated the task description. (Show Details)

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

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