Page MenuHomePhabricator

Feature flag the 'Expand all sections' feature in Special:MobileOptions
Closed, ResolvedPublic1 Estimated Story Points


This will help us to enable/disable the feature without relying on the beta mode. This will also make the toggling code not depend on the beta mode. We can replace the beta mode check in with the value of the feature flag.

Event Timeline

bmansurov updated the task description. (Show Details)
bmansurov moved this task from Uncategorized to Clean-Up on the Technical-Debt (RW-Tech-Debt) board.
phuedx set the point value for this task to 1.

Not sure I understand the motivation for this task.
Is the idea that we want MFCollapseSectionsByDefault to be passed in as an option to the Toggler class (this value never changes)?
Or are we actually talking about the expand sections beta feature that overrides it ?
I do not understand the link to removing SkinMinervaBeta right now so would help if someone made the description more explicit.

ovasileva lowered the priority of this task from High to Medium.Jan 25 2017, 7:56 PM

@Jdlrobson, the idea is to simplify the code that enables mobile toggling. Right now we test the config variable and test the beta mode. If we make the config variable take different values for stable and beta modes, then we can create a function in the front-end (similar to getConfigVariable in MobileContext.php) and use it to enable the toggling functionality.

It's a little more complicated than this.
wgMFCollapseSectionsByDefault does not vary in stable/beta mode - it's always the value you give it (I'm not sure if there is much value in allowing it to be changed in beta)

However in beta mode (see mobile.special.mobileoptions.scripts) a checkbox is added to Special:MobileOptions that allows a user to override the default via localStorage. There is no feature flag for this and there probably should be (suggested value wgMFCollapseSectionsByDefaultAllowUserOverride )

Hence my confusion.

Needs a little discussion before it can be considered ready to be worked on.

@Jdlrobson I was talking about this part of the code:, where you can see that the code to collapse sections depends on either collapseSectionsByDefault or the beta mode. So, rather than depending on the beta mode, we could make the collapseSectionsByDefault config variable take different values depending on the mobile mode, then we can get rid of the bet mode check on the line linked above.

collapseSectionsByDefault is a sysadmin setting
The beta flag relates to a client side user specific setting restricted to beta.
We could drop the beta flag entirely here to make this less confusing if necessary but we still need to check both and neither would benefit imo from having different stable/beta flags.

If we drop the beta flag, wouldn't we be enabling the feature in stable regardless of the value of collapseSectionsByDefault? What I'm suggesting is that if we have different values for collapseSectionsByDefault depending on the mode, we won't need the isBeta check any more. This way we can also keep the behavior the same - no spill over to stable.

I'm not sure I follow.

expandSections = !collapseSectionsByDefault ||
			( context.isBetaGroupMember() && settings.get( 'expandSections', true ) === 'true' );

The beta flag here is a precursor to settings.get( 'expandSections', true ) which is a setting that can be changed when in beta on Special:MobileOptions.
We don't need to check the beta flag here, we could add a feature flag EnableMobileOptionsSectionPreference on Special:MobileOptions instead and drop the beta check here.

OK, I see where my confusion is coming from. I think I was trying to combine two different settings. Your suggestion of creating an additional config variable seems reasonable as the goal of these tasks is to feature flag features.

bmansurov renamed this task from Make toggling code not depend on the beta context to Feature flag the 'Expand all sections' feature in Special:MobileOptions.Mar 7 2017, 7:37 PM
bmansurov updated the task description. (Show Details)

@bmansurov this makes sense now! Thanks for chatting through the details <3

Change 356763 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/MobileFrontend@master] Feature flag expand all sections

Change 356763 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Feature flag expand all sections

Feature flag is written, although I must point out it was tempting to kill this feature altogether... :)