Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Break Locale(s) and Geographic Region(s) into their own required fields (out of the current Population Filtering box) #971

Closed
shell1 opened this issue Feb 25, 2019 · 11 comments
Assignees
Milestone

Comments

@shell1
Copy link

shell1 commented Feb 25, 2019

Today there is a natural language box for everything on population filtering. We want to pull out the extremely common and well-defined Locale(s) and Geographic Region(s) and into their own fields - with check boxes for all the options. Also add a new field for Platforms. All 3 should go above the current Population Filtering box.

  • Locale(s) and Geographic Region(s) should both be optional, but marked as incomplete if blank. Incomplete means that these fields shows as missing info that is needed when users click "ready to ship"). They should behave the way the object/analysis boxes do.
    • We want separate lines with multi-select check boxes generated for each option.
  • Platform(s) is completely optional (not incomplete if blank). The options should be "All Platforms, All Windows, All Mac, All Linux".
@shell1 shell1 changed the title Break Population and Locales into their own required fields (above current Population Filtering box) Break Locale(s) and Geographic Region(s) into their own required fields (out of the current Population Filtering box) Feb 25, 2019
@jaredlockhart jaredlockhart added this to the Backlog milestone Feb 27, 2019
@peterbe peterbe self-assigned this Mar 1, 2019
@peterbe
Copy link
Contributor

peterbe commented Mar 1, 2019

We want separate lines with multi-select check boxes generated for each option.

I don't understand. "check boxes" would get incredibly busy.
Thee widget for selecting multiple things is <select multiple> and looks like this:
https://codepen.io/peterbe/pen/NJxQJv?editors=1000#0

Also, since "All" isn't really an option, we can probably do something similar to what Delivery-Console/Normandy does which is that if NO locale is selected, it allows all locales. But DC/Normandy doesn't have the concept of "All" as an option. And since we're going to store this as a database many-to-many we can't allow in arbitrary strings. We could potentially have two fields in our database:

  1. locale (many-to-many)
  2. all_locales (boolean)

So, if you select "All" it sets all_locales=true and locales=[]. Then we have a record that the user chose "All" at least and that they didn't ignore this.

@peterbe
Copy link
Contributor

peterbe commented Mar 1, 2019

Actually, I don't know the license or the compatibility but another good looking option for a widget is: http://davidstutz.de/bootstrap-multiselect/

We could try that as a bonus.

@shell1
Copy link
Author

shell1 commented Mar 1, 2019

@peterbe I shouldn't have said checkboxes. We just need a way for multi-select from the provided list.

@mythmon @jaredkerim what logic does Normandy need for locales and geos? Looking at @peterbe comment - it sounds like Normandy only needs to know if there is an exception and defaults to no filtering on locales or geos (logical).

I thought Locales and Geos should be optional but required - but maybe they should be totally optional? I'd feel more confident if people specify locales and geos - since that is something that should be answered to avoid surprises from missed considerations. But maybe an "all" answer is just more work (see two comments above about needing a small workaround to store "all" since it's an arbitrary string not on the list).

@mythmon
Copy link
Contributor

mythmon commented Mar 2, 2019

@shell1 This is a place where Normandy's abstractions are leaking a bit. All of our filters are "opt-in" so to speak, in that if a Normandy recipe doesn't mention a field, it isn't included in the filter. That's a result of Normandy being very flexible.

Experimenter on the other hand is meant for people, and is meant to have a very narrow use case. It can be a little nicer. I think that a required field with an option of "All" is fine. We should design Experimenter's UI to meet the needs of humans and the experimenter program, and then translate those ideas to Normandy.

@shell1
Copy link
Author

shell1 commented Mar 4, 2019

@peterbe your suggestion above of all_locales (boolean) and similar for Geos is the best solution. Having that "all" choice as optional but incomplete if missing for those 2 helps avoid a pretty common human oversight when asking for studies. Asking those two (locale and geo) questions about their study forces working through with DS on the implications of limiting locales and geos or if there is need to translate any UI.

@jaredlockhart jaredlockhart modified the milestones: Backlog, 2.26.0 Mar 4, 2019
@jaredlockhart
Copy link
Collaborator

@shell1 @peterbe Why can't we use that bootstrap-multiselect widget and just have it look like

[] All Locales
[] ab (Abkhazian)
[] aa (Afar)
[] af (Afrikaans)

etc with the default being 'All Locales' checked, otherwise you can check any arbitrary selection of locales which then unchecks 'all locales'. We can use a single string field to store it, either as None for all locales, or as a comma separated list of locale codes. @peterbe Does that make sense?

@mythmon
Copy link
Contributor

mythmon commented Mar 6, 2019

Another interesting use case came up during Lightning Advising today. This experiment has a geolocation filter of "Global (excluding RU, TR, CN, KZ, BY)".

This isn't something we can do in filter objects, but its easy enough to do in JEXL. I think it might make sense to support this in the UI (and filter objects).

@peterbe
Copy link
Contributor

peterbe commented Mar 6, 2019

That's certainly interesting! I have spent an absurd amount of time now just get the Django form to integrate well with bootstrap-selectpicker and I don't see it being able to easily support this pattern.

But there can be ways. There can be a toggle in the UI that switches from "Locale(s):" to "Excluded Locale(s):". Or, we can have a second widget that only appears if you have selected "All" that'd look like this:

Locale(s): "ALL"
Excluded locale(s): "fo", "ba", "re"

@shell1
Copy link
Author

shell1 commented Mar 6, 2019

Potentially stupid question: How many locales and geos are there? Would it be unreasonable to expect that if they want to do exclusion - just check off all the locales/geos EXCEPT the ones they don't want?

@mythmon
Copy link
Contributor

mythmon commented Mar 6, 2019

There are 159 locales and 249 countries that Normandy knows how to filter on. I don't think asking people to select all the options is reasonable.

@peterbe
Copy link
Contributor

peterbe commented Mar 14, 2019

Regarding "Platform(s)" this is NOT something that Delivery Console supports.

I don't think it should stop us in Experimenter but at least for a while, it's going to have to become part of a JEXL expression.

jaredlockhart pushed a commit that referenced this issue Apr 2, 2019
* Rearrange order of population fields

Per #971, these fields should go *above* the current client_matching
field.

* Add platform field fixes #1048

* Add a section title for "Additional Filtering"

Thanks @jaredkerim for the suggestion.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants