GrowthExperiments shouldn't be setting the AMC mode
Closed, ResolvedPublic
Actions

Description

HomePageHooks in the GrowthExtension reuses the talk_on_top feature to show link to the "My homepage" page. To make it happen the SkinOptions::OPTIONS_TALK_AT_TOP and SkinOptions::OPTIONS_TABS_ON_SPECIALS has to be set. Sadly, because of some reason, HomepageHooks also enables the OPTION_AMC, which enables the Advanced Mobile Contributions mode on mobile.

The SkinOptions::OPTION_AMC should not be enabled, as it additionally enables new features like UserMenu (visible UI changes).

Referenced code: HomepageHooks::onSkinMinervaOptionsInit()

Details

Subject	Repo	Branch	Lines +/-
Split getSkinStyles into separate class, add unit test	mediawiki/skins/MinervaNeue	master	+146 -42
Remove setting of OPTION_AMC to true	mediawiki/extensions/GrowthExperiments	master	+0 -2
Enable AMC styles if only one of the AMC properties is enabled	mediawiki/skins/MinervaNeue	master	+11 -1

Customize query in gerrit

Related Objects

Mentioned In: rSKIN5aba58b97697: Update git submodules
Mentioned Here: T229295: Cleanup AMC feature flagging

Event Timeline

pmiazga created this task.Jul 25 2019, 4:39 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 25 2019, 4:39 PM

Jdlrobson claimed this task.Jul 25 2019, 4:51 PM

Jdlrobson moved this task from Incoming to Needs Prioritization on the Web-Team-Backlog board.

pmiazga triaged this task as High priority.Jul 25 2019, 5:09 PM

MMiller_WMF subscribed.Jul 25 2019, 5:36 PM

Needs some analysis and then an estimation.

The reason is in the line directly above the one referenced in this task description, namely that this results in correct styles being applied to the AMC menu.

As far as I can tell the fix would be that if one is rendering the AMC menu, the necessary styles should be loaded and those should not be behind the AMC enabled flag.

• Niedzielski renamed this task from GrowthExtension shouldn't be setting the AMC mode to GrowthExperiments shouldn't be setting the AMC mode.Jul 25 2019, 6:53 PM

Change 525630 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Enable AMC styles if only one of the AMC properties is enabled

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

gerritbot added a project: Patch-For-Review.Jul 25 2019, 7:17 PM

@kostajh I think something like https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/525630 would be best here - can you confirm with that change you wouldn't need to enable AMC mode?

Also can you confirm this only applies for logged in users?

Jdlrobson reassigned this task from Jdlrobson to kostajh.Jul 25 2019, 8:21 PM

Jdlrobson subscribed.

Change 525656 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/extensions/GrowthExperiments@master] Remove setting of OPTION_AMC to true

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

can you confirm with that change you wouldn't need to enable AMC mode?

Yes something like that would work, but I commented on the patch with a change request.

Also can you confirm this only applies for logged in users?

That's right, this is just for logged-in users.

kostajh edited projects, added Growth-Team (Sprint 0 (Growth Team)); removed Growth-Team.Jul 25 2019, 9:21 PM

kostajh moved this task from Incoming to Code Review on the Growth-Team (Sprint 0 (Growth Team)) board.

Change 525670 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/skins/MinervaNeue@master] Split getSkinStyles into separate class, add unit test

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

Jdlrobson moved this task from Blocked on Others to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-2019-20-Q1) board.Jul 25 2019, 11:05 PM

Change 525630 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Enable AMC styles if only one of the AMC properties is enabled

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

Change 525656 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Remove setting of OPTION_AMC to true

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

Restricted Repository Identity mentioned this in rSKIN5aba58b97697: Update git submodules.Jul 25 2019, 11:26 PM

ReleaseTaggerBot added a project: MW-1.34-notes (1.34.0-wmf.16; 2019-07-30).Jul 26 2019, 12:00 AM

Jdlrobson moved this task from Needs Code Review to Blocked on Others on the Web-Team-Backlog (Readers-Web-Kanbanana-2019-20-Q1) board.Jul 26 2019, 3:06 PM

Jdlrobson moved this task from Blocked on Others to Needs More Work on the Web-Team-Backlog (Readers-Web-Kanbanana-2019-20-Q1) board.Jul 26 2019, 9:49 PM

The SkinOptions::OPTION_AMC should not be enabled, as it additionally enables new features like UserMenu (visible UI changes).

@Etonkovidova the relevant bit for QA is this (users with homepage enabled and _not_ opted into AMC should not see UserMenu) and that the tabs on Special:Homepage display with the AMC styles, as they did before.

Jdlrobson moved this task from Needs More Work to Needs Code Review on the Web-Team-Backlog (Readers-Web-Kanbanana-2019-20-Q1) board.Jul 29 2019, 9:51 PM

@kostajh related discussion kicked off on T229295

@pmiazga will take a look at this today.

Jdlrobson moved this task from Needs Code Review to Blocked on Others on the Web-Team-Backlog (Readers-Web-Kanbanana-2019-20-Q1) board.Jul 30 2019, 10:37 PM

Tested in betalabs - enabling Homepage (from Special:Preferences or by default for a new user) doesn't enable AMC. I checked the absence of AMC according to list AMC-related features T229295.

Change 525670 abandoned by Jdlrobson:
[mediawiki/skins/MinervaNeue@master] Split getSkinStyles into separate class, add unit test

Reason:
Code review queue cleanup: Closing due to inactivity.