1125131 - Ensure the correct partner code is sent for searches

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Search, defect)

Comment 1

[Dale Harvey (:daleharvey)]

Reposting from discussion about the implementation: 

1) The partner must define a "partner" variable in addition to providing valid MCC-MNC sets. (in variant.json, I'd assume).  In the case of an OEM, they would not provide MCC-MNC sets.

2) If a SIM is inserted that matches a provided MCC-MNC pair, we'll use the values for that pair.

3) If no SIM is inserted, or the SIM does not match one of the provided sets, we would use the "partner" value to determine the correct search URL set to provide.  In the case of someone like TEF, we could use either a generic value, or just assign to one of the subs (this is likely to be an edge case).  The OEM would have a single set.  We'd use these values unless/until a valid (as in #2) SIM is inserted.

4) If nothing matches, we'll use the Mozilla defaults.

I'll note that we _may_ want to tweak this for languages depending on partner, so that's an extra bit of complexity on the master file side.  (As in, there should be a possibility to define locale-specific defaults for a given partner entry.)

(Non-technical people can ignore the rest here.)

For format, this would look something like this:

{
  "default": {
      "en-US": {... },
      "es-AR": {... },
  },
  "tef": {
    ...
  },
  "434-343": {
    ...
  },
}

Comment 2

[Dale Harvey (:daleharvey)]

>  We'd use these values unless/until a valid (as in #2) SIM is inserted.

Peter, Francis

I wanted to check with this requirement, this means in theory the user could have used the search app / settings once, insert a sim and have a completely different set of options show up. I believe this is something we very specifically avoid (having customizations change things under the users eyes).

Is this a valid requirement or does it need fixed?

Comment 3

[Mike Connor [:mconnor]]

It's not ideal, but I think it's a requirement.  I'd be surprised if the options were completely different.

Comment 4

[Peter Dolanjski [:pdol]]

Dale, if you think think of another approach, certainly bring it up.  In practice I highly doubt that there would be a difference between the search urls for the "partner" defaults for a given build (scenario 1) and the search urls based on a valid SIM (scenario 2).

For example, if the build specifies that the build is a TEF build with search engines A, B and C and the user uses it without a SIM for while, then they'll get engines A, B and C with certain URLs used (user doesn't really care about the URLs).  If they then put in a valid (as specified in the build) SIM for Vivo Brazil, it's quite unlikely that their list of search engines will differ from A, B and C.  The main difference will be the URLs used.

If there is a way to add to the list without duplication such that the URL used is the latest one applied, then that might be ideal.  You can tell me if that's possible.
For example: the default TEF build may have Engine A with URL enginea.com/abc123 and Engine B with URL engineb.com/abc123.  Then the Vivo SIM is inserted (as above) which has Engine A with URL enginea.com/xyz789 and Engine C with URL enginec.com/xyz789.
The result will be that the user has Engines A (enginea.com/xyz789), B (engineb.com/abc123), and C (enginec.com/xyz789).  Does that make sense?

Comment 5

[Dale Harvey (:daleharvey)]

The most reliable way to do this would be

// search_providers.json
{
  'en-us' { 
    'google': {
      'searchUrl': 'http://url.com?partnerCode={partnerCode}'
    }
  } 
}

// partner_codes.json
{
  '123-456': 'tef'
}

Then we fill in the partner code based on the mnc-mcc configuration, falling back to the one provided in the build, falling back to a default one for mozilla. Ideally we leave the mnc-mcc tracking to the partner and we can just put that in directly, but will defer that to Mike

This means there is no chance of switching sims changing the list out from under users, we can happily apply the code based on the current sim configuration instead of just the first sim entered, its generally much easier and more flexible.

Are there any search providers we have a partnership with that use entirely different urls just to be able to track a code? otherwise adding complexity to build entirely different lists seems overly complex

Comment 6

[Mike Connor [:mconnor]]

Okay, so... this is too simple, sadly.  And it means partners have to manage codes, and one goal is to avoid all of that.

To answer a key question, yes, we have different URLs for the same partner. Yahoo has per-market URLs still, and even without that there's some partners of theirs (like Mozilla!) who use YHS instances instead of their base search experience, which means a different base URL.  There are also cases where there are different or additional params.  So your example is doomed to fail.

Part of the reason it took me a while to reply here is that I wanted to make sure I had a workable design in my head.  I think I've got that (but you'll probably hate it until you read the whole thing).

We _do_ want to avoid duplicating everything in the file (on our side), but that means we end up with a more complex approach in code.  I think that's the right tradeoff, especially if we write some solid tests.

Rules:
# If MCC-MNC matches, use that config
# Otherwise fall back to partner.
# If no match for partner, use defaultConfig
# within a config, use the correct locale.  If no locale match is found, use defaultLocale for that config.
# all base URLs/params are replaceable by the config-specific version
# partners must be able to specify an arbitrary set of search partners

// search_engine_definitions.json
{
  'engines': {
    'aPartner': {
      'baseURL': 'https://url.com/search',
      'params': {
         'e':  'UTF-8',
         'pc': 'mozilla',
         's':  '{searchTerms}'
      }
    },
    'anotherPartner': {
      'baseURL': 'https://differenturl.com/s',
      'params': {
         'appid': 'ffos',
         'hpart': 'moz',
         't': '{searchTerms}'
      }
    },
    'oemPartner': {
       'baseURL': 'https://oempartner.net/webSearch',
       'params': {
         'q': '{searchTerms}',
       }
    } 
  }
  'configs': {
    'defaultConfig': {
      'defaultLocale': {
        'defaultEngine': 'aPartner',
        'engines': {
          'aPartner': {
          },
          'anotherPartner': {
          }
        }
      }
    },
    '123-456': {
      'es-ES': {
        'defaultEngine': 'anotherPartner',
        'engines': {
          'aPartner': {
            'params': {
              'pc': 'abc-002',
              'src': 'ffos'
            }
          }
          'anotherPartner': {
            'params': {
              'hpart': 'abc'
            }
          },
          'oemPartner': {
            'baseURL': 'https://es-ES.oempartner.net/s',
            'params': {
              'tsrc': 'abc'
            }
          }
        }
      }
    },
    'anOEM': {
      'en-US': {
        'defaultEngine': 'anotherPartner',
        'engines': {
          'anotherPartner': {
            'params': {
              'hpart': 'abc'
            }
          },
          'aPartner': {
            'params': {
              'pc': 'abc-001'
            }
          },
          'oemPartner': {
            'params': {
              'tsrc': 'abc',
            }
          }
        }
      }
    }
  }
}

The code would find the correct config (MNC-MCC > base partner > default), and assemble via the following heuristics:

* for each entry in the 'engines' object for the config, look for a matching entry in the top level engines object and clone that object.
* if config's engines object has baseURL or params properties, those should replace or be added to the object.
* Once there's a set of engine objects, the final URLs are generated by appending parameter pairs to the URL

Yes, this is complicated, but it's necessary to handle all of the variants without excessive duplication, and still providing fallbacks.

case #1: MCC-MNC is 123-456, partner code is anOEM, locale es-ES

* default is anotherPartner
* URLs are:
** anotherPartner: https://differenturl.com/s?appid=ffos&hpart=abc&t={searchTerms}
** aPartner: https://url.com/search?e=UTF-8&pc=abc-002&src=ffos&s={searchTerms}
** oemPartner: https://es-ES.oempartner.net/s?tsrc=abc&q={searchTerms}

case #2: no MCC-MNC match, partner code is anOEM, locale en-US

* default is anotherPartner
* URLs are:
** anotherPartner: https://differenturl.com/s?appid=ffos&hpart=abc&t={searchTerms}
** aPartner: https://url.com/search?e=UTF-8&pc=abc-001&s={searchTerms}
** oemPartner: https://oempartner.net/s?tsrc=abc&q={searchTerms}

case #3: no match, fall back to default, no locales specified, so using 

* default is aPartner
* URLs are:
** aPartner: https://url.com/search?e=UTF-8&pc=mozilla&s={searchTerms}
** anotherPartner: https://differenturl.com/s?appid=ffos&hpart=moz&t={searchTerms}

One file to rule them all, minimal duplication, near-infinite flexibility.

If we want to add icons, suggestion URLs, etc, the same rules would apply for default values with config-specific overrides still available.

For a partner such as Yahoo, I'd probably recommend we create distinct engine entries for each "market URL" (i.e. en-GB is uk.search.yahoo.com for both search and suggestions), and simply include "yahooUK" instead of "yahoo" in the appropriate configs.

Hopefully this all makes sense.

Comment 7

[Dale Harvey (:daleharvey)]

> Okay, so... this is too simple, sadly.  And it means partners have to manage codes, and one goal is 
> to avoid all of that.

The partner wouldnt have to manage codes, aside from the single partner code that they are going to have to put into their build in all of these choices (we would maintain partner_codes.json)

> We _do_ want to avoid duplicating everything in the file (on our side), but that means we end up 
> with a more complex approach in code.  I think that's the right tradeoff, especially if we 
> write some solid tests.

I am less worried about duplications than the fact that this may at runtime underwrite a configuration that user has already been exposed to

Been trying to clear up other user facing blockers up till now, but taking a look at this now to see how viable it is

Comment 8

[Dale Harvey (:daleharvey)]

I think this is feasible, the naming conventions are confusing so may change them up a bit but will try an initial implementation as specified

Comment 9

[Dale Harvey (:daleharvey)]

So how about this, it increases the duplication, but in my opinion is far more readable, and it codifies the fact that a user cant open the application, see search engines a,b,c then insert a sim and see them replaced with d,e,f

{
  "search_providers": {
    "default": {
      "google": {
        "title": "Google",
        "searchUrl": "google.com/search?q={searchTerms}"
        "suggestUrl": "google.com/suggest?client=firefox&q={searchTerms}"
      },
      "yahoo": {
        "title": "Yahoo",
        "searchUrl": "yahoo.com/search?q={searchTerms}"
        "suggestUrl": "yahoo.com/suggest?client=firefox&q={searchTerms}"
      },
    },
    "en-FR": {
      "google": {
        "searchUrl": "google.fr/?q="
      }
    }
  },

  "sim-customisations": {
    "123-456": {
      "default": {
        "google": {
          "searchUrl": "custom.google.com/"
        },
        "custom": {
          "title": "A Partners own search engine",
        }
      },
      "en-FR": {
        "google": {
          "searchUrl": "google.fr/?q"
        }
      }
    }
  },

  "partner-codes": {
    "tef": {
      // Save format as sim-customisations
    }
  }
}

We start with the search providers object (in the appropriate locale), we pick up the correct customisation as previously mentioned (Sim > Partner) and if found (again with appropriate locale), clone it on top of the default, this means the we have the ability to change the urls for any customisation and they can provide their own oem agreed engines. It gives us pretty much the same power of customisation as before but without the ability for changing sim to delete existing entries.

Technically its still possible to change the url + title to an entirely different engine, however the main point is that if the user has previous picked 'ddg' as the key of their selected engine then that key will never disappear, the configuration is additive and tt also has a lot less complexity. I think if the size becomes a problem we can look at how to deduplicate similiar to the previous proposal.

Comment 10

[Dale Harvey (:daleharvey)]

Hey Mike, how does this look?

Comment 11

[Mike Connor [:mconnor]]

The main problem I have with this is that it uses full URLs everywhere, which makes it a pain to add/change things on a global basis.  If we needed to add a parameter (i.e. an API key for search suggestions) this would require a lot of find/replace.  It also means it's harder to track blame for changes, because the same line will change every time.  We've switched to params instead of full URLs on desktop and mobile to avoid this pain. I'd like to skip straight to the end state instead of having to learn everything from scratch.

Comment 12

[Dale Harvey (:daleharvey)]

Ok, extended this to match the earlier format that split out parameters from urls

https://github.com/daleharvey/gaia/commit/0f0a1a729bb613480387aaeb3b797341b6c21147#diff-11d13ab71433169b886dc2adf2b08beaR1

Now the config can be fully defined by the sim / partner match, one difference is we start with a base config, extend the locale on top of that, then extend the sim / partner customisation on top of that. This means we can add things to the locale without replacing the entire thing (ie they just have a different url)

I think the end result is as you specified mike, pretty much the same format as well, just took a while playing with it before understanding, needinfoing so you get a chance to see it if you have any issues. Going to work on plugging it in now

Comment 13

[Mike Connor [:mconnor]]

This looks pretty good, I think you've figured out what I couldn't quite explain correctly.  Only thought is whether we want to have inheritance all the way down: default + locale + partner + sim.  That would probably have the absolute minimum of duplication of a given partner value (as in we'd almost never specify the same param key/value in multiple places, thus minimizing future errors).

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] daleharvey:1125131 > mozilla-b2g:master

Comment 15

[Dale Harvey (:daleharvey)]

> Only thought is whether we want to have inheritance all the way down: default + locale + partner + sim. 

That sounds reasonable but I would like to address this in and when its needed in a follow up, I would like to get the base functionality landed since we are pushing the FC now.

Comment 16

[Dale Harvey (:daleharvey)]

Comment on attachment 8568215 [details] [review]
[gaia] daleharvey:1125131 > mozilla-b2g:master

So this is now working, functionally everything should work the same but allows us to configure partner code based on sim or predefined partner code. 

George, we need to access the search_provider.json file in both the search app and the settings app which is why its a json file in the shared directory, if you can suggest a better implementation then would be happy to hear, I dont love the current implementation but couldnt think of something better.

Comment 18

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8568215 [details] [review]
[gaia] daleharvey:1125131 > mozilla-b2g:master

Sorry, man, it should not work.
As my comments in github, coping shared resource happens earlier than app's own buildscript, and I don't suggest you to modify a common resource in app's buildscript.

My suggestion would be, to create a new build module that only do modified the search_providers.json and execute it before webapp-shared (see pre-app), or do it in contacts-import-services.js and rename it since I think they are all trying to modify shared resource.

Comment 19

[Dale Harvey (:daleharvey)]

Comment on attachment 8568215 [details] [review]
[gaia] daleharvey:1125131 > mozilla-b2g:master

Addressed the build issues and comments on github, Cheers

Comment 20

[Kevin Grandon :kgrandon]

Comment on attachment 8568215 [details] [review]
[gaia] daleharvey:1125131 > mozilla-b2g:master

I will leave some comments on github, but I think the tests are failing, and they look like actual problems. I'm not sure why they're firing socket recv() timeout messages though =/

Comment 21

[(Inactive after June) George Duan [:gduan] [:喬智]]

Comment on attachment 8568215 [details] [review]
[gaia] daleharvey:1125131 > mozilla-b2g:master

r=gduan, I'm ok with build part.
However, as Kevin said, please use utils.getFile(opts.VARIANT_PATH) in your code, because GAIA_DISTRIBUTION_DIR may be null. And please make sure all the tests are green.

Comment 22

[Dale Harvey (:daleharvey)]

George, hey, I have the rest of the test fixes working, however I cant figure out the Gaia Integration Build Failure

https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=4adc34002268

The remake tests fails, the timestamp of the generated application does change, however none of them include the changed shared resources, so not sure why, any ideas?

Comment 23

[(Inactive after June) George Duan [:gduan] [:喬智]]

Oh man, I forgot the rebuild rule in our build script.
Currently, if you change any file of shared folder, it will rebuild all the apps and that's what we don't want.

Perhaps you can consider to keep search_providers.json in shared and modify it only when there's diff (see as below)
https://github.com/mozilla-b2g/gaia/blob/master/build/contacts-import-services.js#L44

Comment 24

[Dale Harvey (:daleharvey)]

Comment on attachment 8568215 [details] [review]
[gaia] daleharvey:1125131 > mozilla-b2g:master

Thats great, thanks George, this should be green now, reflagging review

Comment 25

[Kevin Grandon :kgrandon]

Comment on attachment 8568215 [details] [review]
[gaia] daleharvey:1125131 > mozilla-b2g:master

Please fix the jshint nit, but this looks good to me. R=me, nice work!

Comment 26

[Dale Harvey (:daleharvey)]

All green @ https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=bfb1585e9338

Landed in https://github.com/mozilla-b2g/gaia/commit/664438c29898e857ff0cef29f115793b4a4a61e7

Cheerrs George, Kevin

Comment 27

[Hermes Cheng (inactive after July 27, 2015)]

Please uplift to v2.2. Thanks.

Comment 28

[Hermes Cheng (inactive after July 27, 2015)]

(In reply to Hermes Cheng[:hermescheng] from comment #27)
> Please uplift to v2.2. Thanks.

Dale, could you please help us uplift? Thanks.

Comment 29

[Dale Harvey (:daleharvey)]

Comment on attachment 8568215 [details] [review]
[gaia] daleharvey:1125131 > mozilla-b2g:master

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Feature work landed late
[User impact] if declined: We wont have the ability to correctly setup search partnerships
[Testing completed]: automated tests, baked in master
[Risk to taking this patch] (and alternatives if risky): Its not a minor patch, but its been well used and stable so far
[String changes made]:

Comment 30

[Ryan VanderMeulen [:RyanVM]]

v2.2: https://github.com/mozilla-b2g/gaia/commit/6c8cdceac24e31a857216c7c5d68cebd62cfda03