Page MenuHomePhabricator

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

Description

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 https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.toggle/toggle.js#L247 with the value of the feature flag.

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 22 2016, 8:49 PM
bmansurov removed bmansurov as the assignee of this task.Jan 3 2017, 4:40 PM
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 Normal.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: https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.toggle/toggle.js#L247, 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

Restricted Application added a subscriber: TerraCodes. · View Herald TranscriptApr 17 2017, 11:52 PM

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

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

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

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

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

Jdlrobson removed Jdlrobson as the assignee of this task.Jun 5 2017, 5:06 PM
Jdlrobson closed this task as Resolved.Jun 6 2017, 12:04 AM
Jdlrobson claimed this task.

LGTM