Page MenuHomePhabricator

Build AMC opt-in toggle
Closed, ResolvedPublic13 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 page reloads.
  • When a user opts in to (or out of) 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!

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.


Removed from AC:

  • FeaturesManager::isFeatureAvailableInContext will need to be updated to take a $user object and check the stored value of AMC. This is needed for the talk tab task (T212216).

☝️ Declined per T211197#4936296.

Signoff steps

  • setup task to turn on amc mode with talk feature in production
  • unstall T212516

QA Results

ACStatusDetails
1✅ PassedT211197#4946212
2✅ PassedT211197#4946212
3✅ PassedT211197#4946212
4✅ PassedT211197#4946212
5✅ PassedT211197#4951773

QA Results : Production

ACStatusDetails
1✅ PassedT211197#5047330
2✅ PassedT211197#5047330
3✅ PassedT211197#5047330
4✅ PassedT211197#5047330
5✅ PassedT211197#5047330

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
phuedx added a subscriber: phuedx.Jan 2 2019, 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)Jan 2 2019, 7:03 PM
Niedzielski updated the task description. (Show Details)Jan 2 2019, 7:23 PM
Niedzielski updated the task description. (Show Details)Jan 3 2019, 8:51 PM
alexhollender updated the task description. (Show Details)Jan 5 2019, 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)Jan 9 2019, 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.

Edtadros added a subscriber: Edtadros.EditedJan 22 2019, 11:12 PM

Worked with Piotr and we were able to test the first 4 items in the acceptance criteria and verified they worked as described on:

OS: macOS Mojave 10.14.1
Browser: Safari 12.0.1

The 4th item will need to be verified by an engineer. I can retest in Chrome if requested.

Tbayer updated the task description. (Show Details)Jan 22 2019, 11:13 PM
Tbayer changed the edit policy from "Custom Policy" to "All Users".
Edtadros updated the task description. (Show Details)Jan 23 2019, 12:21 AM

FeaturesManager::isFeatureAvailableInContext will need to be updated to take a $user object and check the stored value of AMC

@pmiazga this work is incomplete since https://gerrit.wikimedia.org/r/484729 is still open...

Jdlrobson updated the task description. (Show Details)Jan 24 2019, 6:17 PM

Change 484729 merged by jenkins-bot:
[mediawiki/extensions/MobileFrontend@master] Feature management should be aware of user modes

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

Jdlrobson claimed this task.

Either I or @pmiazga needs to write down some QA steps and update staging environment before handing this over.

Jdlrobson reassigned this task from Jdlrobson to pmiazga.EditedFeb 5 2019, 10:49 PM

Worked with Piotr and we were able to test the first 4 items in the acceptance criteria and verified they worked as described on:

@pmiazga and @Edtadros do you want to run through the QA again now the patch has landed and verify the final acceptance criteria? I've updated staging http://reading-web-staging.wmflabs.org to have the latest code.

Edtadros updated the task description. (Show Details)Feb 7 2019, 6:55 AM
Edtadros reassigned this task from pmiazga to Jdlrobson.Feb 7 2019, 7:11 AM

@Jdlrobson I checked and verified that the AMC toggle appears when logged in, and doesn't appear when not logged in. I can pass it if those are the only QA criteria. My concern is that the acceptance criteria list a few more items. Do you have steps you can provide for the rest of the criteria?

Seems like the last two acceptance criteria need to be tested by an engineer (and perhaps someone other than @pmiazga)

@ovasileva, Just in case my network continues misbehaving during today's standup, I think (and @phuedx agrees) that it would be helpful for me to work with the engineer for verifying the 5th acceptance criteria (When a user opts in to (or out of) the AMC mode we need to ensure it is logged and compatible with Schema:PrefUpdate in WikimediaEvents ...we also get T212516 fixed for free!). If @Jdlrobson is up for it we can do a hangout later today.

@ovasileva, Just in case my network continues misbehaving during today's standup, I think (and @phuedx agrees) that it would be helpful for me to work with the engineer for verifying the 5th acceptance criteria (When a user opts in to (or out of) the AMC mode we need to ensure it is logged and compatible with Schema:PrefUpdate in WikimediaEvents ...we also get T212516 fixed for free!). If @Jdlrobson is up for it we can do a hangout later today.

Sounds good to me!

phuedx updated the task description. (Show Details)Feb 7 2019, 6:12 PM
phuedx added a comment.Feb 7 2019, 6:22 PM

☝️ I pulled out the very technical AC from the AC section and into the Developer notes section as:

  • It's something that neither @ovasileva and @Edtadros can verify easily; and
  • It should probably have been verified during code design and review.
phuedx updated the task description. (Show Details)Feb 7 2019, 6:26 PM
phuedx added a comment.Feb 7 2019, 6:43 PM

FeaturesManager::isFeatureAvailableInContext will need to be updated to take a User object and check the stored value of AMC. This is needed for the talk tab task (T212216).

As stated, this isn't done. AIUI this does actually work in practice /cc @pmiazga

FeaturesManager::isFeatureAvailableInContext will go away, there were no changes to the isFeatureAvailableInContext, only deprecation. It's deprecated because Minerva is still using it, after we're done with Minerva changes we should be able to remove isFeatureAvailableInContext method.

phuedx updated the task description. (Show Details)Feb 8 2019, 9:45 AM
pmiazga updated the task description. (Show Details)Feb 8 2019, 10:36 PM
phuedx claimed this task.Feb 11 2019, 6:14 PM

Test Result (Acceptance Criteria 1-4 of 5)

Status: ✅ PASS
OS: macOS Mojave
Browser: Chrome DevTools Device Emulator

NOTE: This testing only covers the first four testing criteria listed in the description and listed (numbered) here for convenience
  1. Toggle is on the mobile settings page for logged in users server and client side
  2. For anonymous users, no toggle appears server or client side
  3. The feature is available as a separate setting (not a part of beta)
  4. When opting into the new mode page reloads.

Test Artifact(s):

AC 1:

A logged in user will see the following:

AC 2:

An anonymous in user will see the following:

AC 3:

Please see AC 1 and AC2. Both images show that the toggle doesn't appear in the Beta section.

AC 4:

The page reloads when opting in and opting out. Below is a screenshot of opting in.

Edtadros updated the task description. (Show Details)Feb 12 2019, 3:54 AM
Edtadros updated the task description. (Show Details)Feb 12 2019, 3:57 AM
phuedx reassigned this task from phuedx to Edtadros.Feb 13 2019, 2:31 PM
phuedx added a comment.EditedFeb 13 2019, 5:55 PM

@Edtadros and I worked through testing the 4th 5th acceptance criterion together during our 1:1 today.

Here's are the server-side EventLogging events that I captured while opting in and out of AMC mode on http://reading-web-staging.wmflabs.org/wiki/Special:MobileOptions:

Opt in

{
  "event": {
    "property": "mf_amc_optin",
    "value": "\"1\"",
    "isDefault": false,
    "version": "1",
    "userId": 1,
    "saveTimestamp": "20190213173528"
  },
  "schema": "PrefUpdate",
  "revision": 5563398,
  "clientValidated": true,
  "wiki": "wiki",
  "webHost": "reading-web-staging.wmflabs.org",
  "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36"
}

Opt out

{
  "event": {
    "property": "mf_amc_optin",
    "value": "\"0\"",
    "isDefault": true,
    "version": "1",
    "userId": 1,
    "saveTimestamp": "20190213173533"
  },
  "schema": "PrefUpdate",
  "revision": 5563398,
  "clientValidated": true,
  "wiki": "wiki",
  "webHost": "reading-web-staging.wmflabs.org",
  "userAgent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/71.0.3578.98 Safari/537.36"
}

When a user opts in to (or out of) the AMC mode we need to ensure it is logged and compatible with Schema:PrefUpdate in WikimediaEvents per T212516.

To the "and compatible" part of the AC: note well the "clientValidated": true in the events above.

ovasileva closed this task as Resolved.Feb 15 2019, 8:37 AM

And we're done! Thank you everybody.

@Edtadros and I worked through testing the 4th 5th acceptance criterion together during our 1:1 today.

Here's are the server-side EventLogging events that I captured while opting in and out of AMC mode on http://reading-web-staging.wmflabs.org/wiki/Special:MobileOptions:

[...]

To the "and compatible" part of the AC: note well the "clientValidated": true in the events above.

Thanks! This looks good enough for now, although it still seems worthwhile to run a query later - as soon as this is live in production - to verify that these events show up in the PrefUpdate EL table in Hive or MariaDB. (IIRC events from reading-web-staging are not logged there, correct?)

Tbayer added a comment.EditedWed, Mar 20, 7:48 PM

@Edtadros and I worked through testing the 4th 5th acceptance criterion together during our 1:1 today.

Here's are the server-side EventLogging events that I captured while opting in and out of AMC mode on http://reading-web-staging.wmflabs.org/wiki/Special:MobileOptions:

[...]

To the "and compatible" part of the AC: note well the "clientValidated": true in the events above.

Thanks! This looks good enough for now, although it still seems worthwhile to run a query later - as soon as this is live in production - to verify that these events show up in the PrefUpdate EL table in Hive or MariaDB. (IIRC events from reading-web-staging are not logged there, correct?)

Done - we are seeing plausible-looking events after today's deployment (T217643):

SELECT * FROM event.prefupdate WHERE year = 2019 AND month = 3 AND day >= 20 AND event.property = 'mf_amc_optin' LIMIT 1000;
...
Time taken: 1.596 seconds, Fetched: 7 row(s)

Note though that there seem to be some general concerns about the data quality of the PrefUpdate schema, regarding duplicate events. I have filed T218835 for this. Right now it seems that this problem could be limited to a small number of other preferences (i.e. not the AMC one we're relying on here), but it seems worth checking this later after a larger number of signups have occurred.

Test Result: Production

Status: ✅ PASS
OS: iOS
Browser: Chrome for iOS

Test Artifact(s)/Step(s):

NOTE: This testing only covers the first four testing criteria listed in the description and listed (numbered) here for convenience. The 5th criteria is not in scope for Production testing.

Test Artifact(s):

AC 1: Toggle is on the mobile settings page for logged in users server and client side



AC 2: For anonymous users, no toggle appears server or client side



AC 3:The feature is available as a separate setting (not a part of beta)
Please see AC 1 and AC2. Both images show that the toggle doesn't appear in the Beta section.

AC 4: When opting into the new mode page reloads.
The page reloads when opting in and opting out.

Edtadros updated the task description. (Show Details)Fri, Mar 22, 8:04 AM

@ovasileva This is tested and Passed.

Edtadros updated the task description. (Show Details)Sat, Mar 23, 12:11 AM