Page MenuHomePhabricator

AMC feature flag can be superseded by a development query parameter
Open, LowPublic

Description

We would like to be able to test the AMC mode even when it's off, in order to do that we need to provide a query parameter that overloads AMC settings and enables/disables the feature.

Open questions:

  • what is the best paramter name? amc_action?
  • what is the expected behavior - just change AMC flag to enabled and allow the user to decide whether to enable/disable the feature?
  • what to do if the AMC mode is disabled via config (should we enable that) ?
  • if we decide to disable AMC because of some issues, what if users share the link that enables the AMC mode (if we decide to implement the global overhaul that skips the config variable)

Event Timeline

Niedzielski triaged this task as High priority.Jan 2 2019, 7:02 PM
Niedzielski created this task.
Niedzielski created this object with edit policy "Custom Policy".
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJan 2 2019, 7:02 PM
Jdlrobson renamed this task from Add AMC feature flag to [Subtask] Add AMC feature flag.Jan 2 2019, 7:21 PM

I need to talk to Piotr about whether this is covered by the task he's working on or something new.

most of the stuff is covered by T211197, only remaining things is:

  • Feature flag can be superseded by a development query parameter

@pmiazga, @Jdlrobson - what do we want to do here? Add that to the current task? Leave this as a separate task and remove the rest of the acceptance criteria?

I'll repurpose this task to handle only Feature flag can be superseded by a development query parameter, as this is a bit more complicated.

pmiazga renamed this task from [Subtask] Add AMC feature flag to [Subtask] AMC feature flag can be superseded by a development query parameter.Jan 15 2019, 4:37 PM
pmiazga updated the task description. (Show Details)
Jdlrobson renamed this task from [Subtask] AMC feature flag can be superseded by a development query parameter to AMC feature flag can be superseded by a development query parameter.Jan 22 2019, 4:53 PM
Jdlrobson lowered the priority of this task from High to Normal.
Jdlrobson added a comment.EditedJan 22 2019, 4:56 PM

Not sure how high a priority this is given opting in is so straight forward.

We could piggy back off mobileaction query string parameter which allows us to share a link to a page in beta. However how to handle those uris shared when the user is logged out?

I suspect we can get away without doing this and avoid ourselves some implementation headaches.

What would the value of having such a query parameter be?

If I understand this task right (the AMC mode has to be disabled on production for now - not visible on Special:MobileOptions page), this development query parameter has to supersede the MFAdvancedMobileContributions config flag (enable the AMC mode and enable AMC for currently logged-in user). It looks bit scary, as the beta mode is available on production, and the mobileaction only temporarily enables the beta mode for the current user.

If I understand this task right (the AMC mode has to be disabled on production for now - not visible on Special:MobileOptions page), this development query parameter has to supersede the MFAdvancedMobileContributions config flag (enable the AMC mode and enable AMC for currently logged-in user).

This is my understanding as well. I think it will be very useful for testing changes on wikis where the feature is otherwise disabled.

I'll chat to Olga in our next sync about how testing requirements.

Jdlrobson lowered the priority of this task from Normal to Low.Jan 24 2019, 9:23 PM

Marking as low until we have a pressing need. Adding such a flag would complicate our configuration code and I'm not seeing a strong reason to do that just yet when we can use specific accounts of testing these things. Hopefully we can chat about this next week.

@Jdlrobson - can we close this?

Do consider that this change might be useful for setting up automated browser tests targetting AMC mode available/unavailable, since the Node.js stack doesn't support a local LocalSettings.php file.

I think it's valuable to do this thing, but IMHO we shouldn't allow users to enable things, that are disabled via config. I see the point of having ?amc=true to force AMC mode to be on, but only when AMC mode is available. But then it makes it
bit useless when it comes to testing because the automated browser test can just go and enable the AMC mode by itself.

The only thing that comes to my mind is to have the AMC mode enabled by default in our test bench, and then having a flag to disable the AMC mode, sth like disable_amc=true, and this would make the AMC mode not available.
Thanks to that we don't create security loophole (allowing users to override config) and it will allow us to test both available and unavailable modes.