@jhobs: Where's our spec for the platforms spec property? I thought we'd changed it throughout the course of the review?

I don't think that is a good idea since it increases the complexity of the config parsing by a whole bunch.

I would argue that if you want different values for a survey then you probably want different surveys.

So for that use case, you would just split the config into two, "font-size-desktop" and "font-size-mobile", and you can even use array_merge for overriding values of font-size-desktop in the mobile version and avoid duplication in the configuration.

That's my 2 cents. I just think that feature is already available even though more explicit.

In T119367#1834966, @Jhernandez wrote:

I don't think that is a good idea since it increases the complexity of the config parsing by a whole bunch.

I would argue that if you want different values for a survey then you probably want different surveys.

So for that use case, you would just split the config into two, "font-size-desktop" and "font-size-mobile", and you can even use array_merge for overriding values of font-size-desktop in the mobile version and avoid duplication in the configuration.

That's my 2 cents. I just think that feature is already available even though more explicit.

My main thinking was primarily for bucketing. I could see scenarios where desktop and mobile run the exact same survey but want different coverage rates. The rest was kind of just a "let's do it the more general way." I do see what you're saying though; maybe we should just allow coverage to be a Number or Array?

Also, I think for the messages it's more useful if you wanted to differentiate some wording between stable & beta but both within mobile. However, in that case you could still just create two surveys ("font-size-beta," "font-size-stable", e.g.), so your point still stands.

There is an issue with the patch - it doesn't seem to blacklist the surveys - they still show up (unless I'm misunderstanding how it's supposed to work). - QuickSurvys::getEnabledSurveys needs configuring if I'm not mistaken
return $survey->isEnabled(); and return isEnabledForPlatform( 'desktop', 'beta' )

I would appreciate some discussion around my comment - as it might also simplify this and allow easier testing (as using skin would not need to depend on MobileFrontend concepts):
https://gerrit.wikimedia.org/r/#/c/254402/

"I actually would like to kill platform in favour of skin and browser width and I said this right at the beginning. The existence of platform adds to the idea that MobileFrontend is a silo when it's not.
Beta could be handled by using minerva-beta as the skin value.
There is currently no way to distinguish between tablet and mobile Minerva which is also bad.
If I'm using Vector on a mobile phone it makes no sense that my platform is mobile.... although some might say it does."

There is an issue with the patch - it doesn't seem to blacklist the surveys - they still show up (unless I'm misunderstanding how it's supposed to work).

I've updated the commit message of 254402 to correctly define the scope the change: it's only meant for validating the platforms property of the configuration, which is half of the battle.

I'm not going to increase the scope of change given that we don't have a clear definition of how exactly it's supposed to act.

In T119367#1844452, @phuedx wrote:

There is an issue with the patch - it doesn't seem to blacklist the surveys - they still show up (unless I'm misunderstanding how it's supposed to work).

I've updated the commit message of 254402 to correctly define the scope the change: it's only meant for validating the platforms property of the configuration, which is half of the battle.

I'm not going to increase the scope of change given that we don't have a clear definition of how exactly it's supposed to act.

That being said, we need to either update this task's title/description to reflect that (and create a new task with this as a blocker), or note that this is a two-patch task via a checklist in the description.

... or note that this is a two-patch task via a checklist in the description.

A task can be completed by as many patches as we see fit.

I'm down with the checklist though.

@Jdlrobson: Something odd happened in Gerrit, you +2'd the patch but it's still -1'd.

In T119367#1847925, @phuedx wrote:

@Jdlrobson: Something odd happened in Gerrit, you +2'd the patch but it's still -1'd.

254402 needs rebasing atop dev.

Done.

Change 254402 merged by jenkins-bot:
Validate and forward survey's platforms

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

post a WIP patch and I can take a look :)

What's going on with this @jhobs? I have seen no movement here all week so not sure how I can help...

Change 261212 had a related patch set uploaded (by Jhobs):
Implement platform-dependent surveys

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

To keep history on this thread from off-phab discussions: I had been waiting for more discussion around CentralNotice implementation before continuing work on this. Since not much really happened (and we should make platforms work in the meantime anyways), I finished it up.

I talked to @jhobs and we felt it might make more sense to do this on the server side.

To follow up from this morning's prioritization meeting, this task should not prevent work on instrumentation.

If it makes it easier for the instrumentation, concatenation of the form factor, a hyphen, and the skin is sufficient for the presentation field in https://meta.wikimedia.org/wiki/Schema:QuickSurveysResponses (as opposed to concatenating form factor, a hyphen, and one of "stable|beta|alpha|prototype"). As I understand it, for example 'minerva' and 'minerva-beta' are distinct things, so it will be obvious when it's stable versus beta on the mobile web (and in some glorious future, maybe even on the desktop).

What is left to do with this? @jhobs / @phuedx do you remember?

In T119367#2399864, @Jdlrobson wrote:

What is left to do with this? @jhobs / @phuedx do you remember?

I don't. I know there was some discussion of doing it on the server side instead of the client, but not sure where that went.

In T119367#2399864, @Jdlrobson wrote:

What is left to do with this? @jhobs / @phuedx do you remember?

Get @jhobs' change reviewed, tested, and merged.

Thx @phuedx

For reference: https://gerrit.wikimedia.org/r/#/c/261212/

Change 362073 had a related patch set uploaded (by Jdlrobson; owner: Jhobs):
[mediawiki/extensions/QuickSurveys@master] Implement platform-dependent surveys

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

Change 362073 merged by jenkins-bot:
[mediawiki/extensions/QuickSurveys@master] Implement platform-dependent surveys

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

Change 261212 abandoned by Jdlrobson:
Implement platform-dependent surveys

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

Change 599905 had a related patch set uploaded (by Thiemo Kreuz (WMDE); owner: Thiemo Kreuz (WMDE)):
[mediawiki/extensions/QuickSurveys@master] Remove duplicate $name property from subclasses

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

Change 599905 merged by jenkins-bot:
[mediawiki/extensions/QuickSurveys@master] Remove duplicate $name property from subclasses

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

I hadn't really thought about this feature until a moment ago, but now some questions are bothering me:

• Would be good to include documentation on the extension help page. I made a start here. I couldn't find much information about $wgMFMode or the anonymous-mobile beta option, linking to that might be useful.
• Can we provide a default?—for consistency with the other filters, I'd suggest that the default is to allow the survey on all platforms.

wgMFMode is for internal usage and the beta mode has been disabled for some time now, so may likely be removed in future. Use of it should be considered an anti-pattern.

As for defaulting a survey to all platforms that sounds like a good idea,