Build AMC opt-in toggle
Open, HighPublic13 Story Points

Description

Note: T212516 needs to be fixed before turning this on.
Consider a feature branch to speed up development and encourage you to break this work into small progressive easily-reviewable chunks rather than one giant commit. Sync with whoever is working on T212216

Background

more details in T210653: [EPIC] AMC - Opting in, please read before starting this task. Note that the acceptance criteria in this task does not reflect the final version of the feature.

Acceptance criteria

  • Toggle is on the mobile settings page for logged in users server and client side
  • For anonymous users, no toggle appears server or client side
  • The feature is available as a separate setting (not a part of beta)
  • When opting into the new mode using the toggle switch, there's no product need to reload the page (as currently the UI doesn't change in any visible way) but we may want to reload it given in future changes may happen. Developer should decide what makes the most sense here.
  • When a user opts in to the AMC mode we need to ensure it is logged and compatible with Schema:PrefUpdate in WikimediaEvents per T212516. Make sure it uses either the WikimediaEventsHooks::onUserSaveOptions hook or the API. Note that if we use and update the hook, we also get T212516 fixed for free!

Open questions

  • FeaturesManager::isFeatureAvailableInContext will need to be updated to take a $user object and check the stored value of AMC. It's unclear if that needs to be part of this change or separated into a new task or the talk task (T212216)

Mockup

Prototype & workflow

https://wikimedia.invisionapp.com/share/RNO2HHBPK7M#/screens/331909359_Enabling-Disabling

Developer notes

Note: that the form currently works with and WITHOUT JavaScript


The form is a standard POST form with the post handled in SpecialMobileOptions::submitSettingsForm
This will need to set a user preference when the user is logged in.
This preference is likely needed to be registered in Special:Preferences (although can be added as a hidden preference). See MFEnableMobilePreferences for how we're managing mobile preferences currently.
Note, in the case of opting into beta mode when logged in, the setting of the mode is handled in setMobileMode. This code might be useful to look at.

The JS mode will be a little trickier.
That will need to inject a toggle box and HIDE the checkbox (see https://github.com/wikimedia/mediawiki-extensions-MobileFrontend/blob/master/resources/mobile.special.mobileoptions.scripts/mobileoptions.js#L128 and implementation of beta toggle)

Make sure it's possible to tab between the controls as a proxy to check that the form is accessible.

Note: we wil NOT brand the experience like we do with the beta symbol (see screenshot).


This was added to beta as we got lots of bug reports (screenshots) and didn't know what configuration the user was using so how to replicate, but will not be used in AMC as we are expected the UI changes to be much more obvious.

sign off steps

  • setup task to turn on amc mode with talk feature in production
  • unstall T212516
ovasileva triaged this task as Normal priority.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptDec 5 2018, 12:00 PM
ovasileva raised the priority of this task from Normal to High.Dec 7 2018, 12:35 PM
ovasileva moved this task from Needs Analysis to Upcoming on the Readers-Web-Backlog board.
Tbayer added a subscriber: Tbayer.Dec 12 2018, 1:47 AM
Jdlrobson moved this task from Upcoming to Needs Analysis on the Readers-Web-Backlog board.EditedDec 14 2018, 6:28 PM
Jdlrobson added subscribers: pmiazga, Jdlrobson.

Moving to needs analysis to remind us that we need to talk about the outcomes of @pmiazga spike (T211195) in next goals/team time. The direction we choose to take has big consequences on this task and we'll need to rewrite this task accordingly with some detailed "Developer notes".

Given, we are not planning to add anon mode from the start there are a few things we need;

  • What does this screen look like when visiting in anonymous mode
  • Settings is currently anonymous only, so we'll need to work out how to separate this for logged in users. I need to flesh this out with another developer to add some notes on the technical details of this task.
ovasileva updated the task description. (Show Details)Dec 18 2018, 5:19 PM
ovasileva updated the task description. (Show Details)

Given, we are not planning to add anon mode from the start there are a few things we need;

  • What does this screen look like when visiting in anonymous mode

I'll let @alexhollender confirm, but I think we just won't show the setting

  • Settings is currently anonymous only, so we'll need to work out how to separate this for logged in users. I need to flesh this out with another developer to add some notes on the technical details of this task.

I'm not sure what you mean here...settings can also be accessed while logged in

Given, we are not planning to add anon mode from the start there are a few things we need;

  • What does this screen look like when visiting in anonymous mode

I'll let @alexhollender confirm, but I think we just won't show the setting

just spoke to @alexhollender - we'll remove the item for anons for now. Later, we might also explore ways of encouraging people to log in as per @pmiazga's suggestion.

Jdlrobson updated the task description. (Show Details)Dec 19 2018, 4:24 PM
Jdlrobson updated the task description. (Show Details)
Jdlrobson updated the task description. (Show Details)Dec 19 2018, 4:27 PM

Addressing open questions:

  1. We will not be rebranding the experience
  2. We will reload the page - prototype for this is shown in the task description (ignore the question mark, https://wikimedia.invisionapp.com/share/RNO2HHBPK7M#/screens/331909359_Enabling-Disabling)
Jdlrobson updated the task description. (Show Details)Dec 20 2018, 5:45 PM
Jdlrobson updated the task description. (Show Details)

@pmiazga could you take a look at the developer notes and tell me if I've missed anything? If not, I think this can be moved to upcoming and estimated.

Ideally that user preference should only be set if the value has changed from the default (otherwise every save to this form is going to set a value unnecessarily)

This is already covered by user settings, it stores only non-default options, if the user picks the default option then the row from user_properties is removed

Because this is handled on the server side and logged-in users skip cache, we can render the form toggle only for logged in users.

Additionally I would suggest to move all setMobileBetaMode, isBetaGroupMember, all beta/stable constants from MobileContext into new class, something like MobileUserOptions, or MobileUserMode and keep all logic (reading/writing user options/cookies etc) there. It will be easier to maintain/rewrite this feature, plus if we decide to handle anons, the only place where changes will be required are Special::MobileOptions (for render and submit logic) and that new class.

Jdlrobson updated the task description. (Show Details)Dec 20 2018, 6:59 PM
Jdlrobson removed Jdlrobson as the assignee of this task.
Jdlrobson updated the task description. (Show Details)

Thank you @pmiazga ! Last question can be discussed during estimation.

Jdlrobson updated the task description. (Show Details)Fri, Dec 21, 6:33 PM
phuedx updated the task description. (Show Details)Wed, Jan 2, 5:29 PM
Jdlrobson set the point value for this task to 13.Wed, Jan 2, 6:07 PM

There was a lot of discussion about this task around 8 or 13 points. There was a good long talk about splitting this up and the scope of refactors (we need to minimise these but we don't want to add on top of what is already a fragile stack). While we all agree this was at least an 8, a couple of us pushed for a 13 given the risk associated with this task and in lack of general agreement, we have gone with the higher estimate.

We're providing a framework for which all of AMC will be based upon, so this work is important, however the biggest reason for a 13 is that touching user preferences has proved tricky in the past. Patches for T173527 which touch user preferences have had to be reverted twice due to how hooks are used in MobileFrontend/Minerva. The PHP code for beta/stable is already quite brittle and we'll adding on top of it so we'll need to be careful not to have unexpected side effects elsewhere (a change to add a featured flagged mobile skin preferences here broke the main menu on stable!). Refactoring would be a responsible outcome of this task.

phuedx added a subscriber: phuedx.Wed, Jan 2, 6:16 PM

There was a lot of back and forth between an 8 and a 13 with good discussion about:

In the end, we opted for 13 as @Jdlrobson highlighted a serious risk with introducing a feature flagged user preference and that this risk isn't due to one AC but a function of many of them and therefore can't be split out into a separate task.

We'll see if we can reduce the estimate to an 8.

Niedzielski updated the task description. (Show Details)Wed, Jan 2, 7:03 PM
Niedzielski updated the task description. (Show Details)Wed, Jan 2, 7:23 PM
Niedzielski updated the task description. (Show Details)Thu, Jan 3, 8:51 PM
alexhollender updated the task description. (Show Details)Sat, Jan 5, 2:12 AM

Change 483029 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Inroduce the AMC opt-in toggle and base AMC classes

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

Jdlrobson updated the task description. (Show Details)Wed, Jan 9, 10:08 PM

I see @alexhollender revised the mock on Friday. I've reverted to the old mock as this is a big scope increase, and will talk about it on T210653

@Jdlrobson @pmiazga apologies for the switch up / sneak attack here. When we initially discussed the idea of an incremental/progressive AMC release I mentioned that one counter-balance I'd like is an explicit call-out on the settings page to let users know what we're up to, roughly what's coming, and a place to submit feedback. I should've explicitly mentioned to y'all that I was following through with that idea here, with these updated mocks. It sounds like we've got things sorted now.

Change 484729 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] Feature management should be aware of user modes

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

Change 483029 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Introduce the AMC opt-in toggle and base AMC classes

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

Change 485075 had a related patch set uploaded (by Pmiazga; owner: Pmiazga):
[mediawiki/extensions/MobileFrontend@master] The AMC Mode should be available only in mobile mode

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

Change 485075 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] The AMC Mode should be available only in mobile mode

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

The AMC toggle seems to be working great! Please note: the mocks in the description of this task are currently outdated (but I didn't want to swap in the new ones for the sake of posterity). Please refer to T214195 when making any further design changes to the settings page.