Validate and handle a survey's platform
Closed, ResolvedPublic

Description

I took a quick pass over the QuickSurveys codebase and noticed that neither the back- or front-ends deals with the "platform" part of the config.

  • Rename platform to platforms
  • When a platform is set to something other than an array e.g. true it throws a PHP exception
  • When mobile platform is set to array( 'alpha' ) it throws an exception as alpha is not a valid mode.
  • When desktop platform is set to array( 'stable',' beta' ) it throws an exception as beta is not a valid mode in desktop.
  • When platforms mobile is not set or is set to an empty array but desktop is set to array( 'stable', 'beta' ) a survey only appears in desktop.
  • When platforms desktop is not set or is set to an empty array but mobile is set to array( 'stable', 'beta' ) a survey only appears in mobile.
  • When mobile is set to array( 'beta' ) a survey only appears in mobile beta
There are a very large number of changes, so older changes are hidden. Show Older Changes
leila set Security to None.Nov 23 2015, 7:56 PM
jhobs added a subscriber: jhobs.EditedNov 23 2015, 8:03 PM

We should consider allowing config params to be customized per platform, either in this task or as a new one.
e.g.

$survey = array(
    "name" => "name-key",
    "description" => "description-key",
    "link" => array(
        "desktop" => "link-desktop-key",
        "mobile" => array(
            "stable" => "link-mobile-stable-key",
            "beta" => "link-mobile-beta-key",
        ),
    ),
    "coverage" => array(
        "desktop" => 0.0005,
        "mobile" => 0.001, // Values could be defined at any point in the tree and get applied to all children
    ),
    "platform" => array(
        "desktop" => array( "stable" ),
        "mobile" => array( "stable", "beta" ),
    ),
);
phuedx renamed this task from QS: Validate and handle a survey's platform to Validate and handle a survey's platform.Nov 24 2015, 3:37 PM
phuedx added a project: QuickSurveys.

@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.

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.

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.

phuedx added a comment.Dec 2 2015, 5:42 PM

... 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 updated the task description. (Show Details)Dec 2 2015, 11:00 PM
Jdlrobson removed a project: Patch-For-Review.
Jdlrobson removed phuedx as the assignee of this task.
phuedx added a subscriber: phuedx.Dec 3 2015, 1:04 PM

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

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

254402 needs rebasing atop dev.

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

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

I've been working on this (my bad, forgot to move to Doing and apparently claim it, which I thought I had already done...), but I'm not certain I'm using the correct approach. For now, I'm simply hiding the survey if the user's platform (determined by skin only atm) is not the targeted one. However, this means all the overhead of bucketing and creating the survey are still done for said user. This may be a moot point if we decide to use Central Notice for QuickSurveys in the future[0], since CN can take care of platform, but I figured I should bring it up since we should have a short-term solution.

[0] I thought I had already sent out the email about this but I guess that was just something I dreamt in a medically-induced haze last week. Expect one soon.

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).

jhobs removed jhobs as the assignee of this task.Jun 16 2016, 8:31 PM

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

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.

Jdlrobson changed the task status from Open to Stalled.Jun 22 2016, 5:57 PM
phuedx added a comment.EditedJun 23 2016, 8:29 AM

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

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

Elitre added a subscriber: Elitre.Jan 27 2017, 7:20 PM
Jdlrobson changed the task status from Stalled to Open.Mar 6 2017, 8:06 PM
Jdlrobson lowered the priority of this task from Normal to Low.

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

Jdlrobson updated the task description. (Show Details)Jun 28 2017, 9:44 PM
Jdlrobson closed this task as Resolved.
Jdlrobson claimed this task.