1119820 - Updates to search providers arent applied and unintialised when accessed from search app

Status: RESOLVED FIXED

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

Description

[Dale Harvey (:daleharvey)]

So this isnt actually as simple as I was hoping, currently our search.providers are initialised via settings (https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/search/search.js#L24) 

However the user on a fresh build may not have gone to settings, which means this will not be initialised, I think we need to switch search.providers (and search.urlTemplate) to populate the settings as part of the build.

We should also take into account users upgrading, the easiest way to do this would be to just use a new key for the 'search.providers' setting

Comment 1

[Dale Harvey (:daleharvey)]

Ben, did you work on the original implementation of the settings, anything more we need to worry about?

Comment 2

[Dale Harvey (:daleharvey)]

As a note the current way we get round this is hardcoding the urlTemplate in search.js - https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/search.js#L26, Which is a bad thing.

Comment 3

[Ben Francis [:benfrancis]]

Is this a duplicate of bug 989964?

(In reply to Dale Harvey (:daleharvey) from comment #0)
> So this isnt actually as simple as I was hoping, currently our
> search.providers are initialised via settings
> (https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/panels/
> search/search.js#L24) 
> 
> However the user on a fresh build may not have gone to settings, which means
> this will not be initialised, I think we need to switch search.providers
> (and search.urlTemplate) to populate the settings as part of the build.

search.urlTemplate should already be set at build time from https://github.com/mozilla-b2g/gaia/blob/master/customization/settings.json

search.providers may only be initialised in the Settings app as you say because currently the Settings app is the only app which uses it. When we add the feature to change search engines from the search app this may need to change.

> We should also take into account users upgrading, the easiest way to do this
> would be to just use a new key for the 'search.providers' setting

Yes handling upgrades is tricky. We need to respect the user's choice when upgrading if they have already switched from the default so we should be careful not to overwrite that.

It would be good if we didn't need to change the key of search.providers when we update the list, but I agree that's the simplest solution. A better solution would be to have a database of search engines (for use when we finally add the ability for the user to add new search engines), and to add items to the database after an upgrade.

(In reply to Dale Harvey (:daleharvey) from comment #1)
> Ben, did you work on the original implementation of the settings, anything
> more we need to worry about?

Yes the way the search engines list and the default search engine are set is complicated.

The list of search engines can be defined:
1) In the default list in the settings app https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/resources/search/providers.json
2) In a build-time customisation if provided https://github.com/mozilla-b2g/gaia/blob/master/customization/search/providers.json
3) Based on MCC/MNC if provided, at the time a SIM card is entered https://github.com/mozilla-b2g/gaia/blob/master/customization/mobizilla/mobizilla_expected_search.json

The default search engine can be defined:
1) The hard-coded one in the search app (this is just a fallback) https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/search.js#L26
1) At build time from the default list of settings https://github.com/mozilla-b2g/gaia/blame/master/customization/settings.json
3) Based on MCC/MNC if provided, at the time a SIM card is entered https://github.com/mozilla-b2g/gaia/blob/master/customization/mobizilla/mobizilla_default_search.json


(In reply to Dale Harvey (:daleharvey) from comment #2)
> As a note the current way we get round this is hardcoding the urlTemplate in
> search.js -
> https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/search.js#L26,
> Which is a bad thing.

Yeah I agree it's bad this is hard coded, that was the way it was implemented when the search app was first created. We have since added the ability to customise it at build time and run time, but this hard coded string remains as a fallback.

I hope I haven't missed anything, this is complex and I find it hard to remember all the details.

Comment 4

[Dale Harvey (:daleharvey)]

> Is this a duplicate of bug 989964?

Will change the title since adding ddg was just one small part of what needs to be fixed

> search.providers may only be initialised in the Settings app as you say because currently the Settings app is the only app which uses it. When we add the feature to change search engines from the search app this may need to change.

It needs to change now since https://bugzilla.mozilla.org/show_bug.cgi?id=1119515 picks up the data from the settings, I think we can just use a new key for 'search.providers' and have it populated at built time, with the ability for mnc/mcc to alter it, We also need to fix the Yahoo suggestions provider as the current url is broken.

Firefox Desktop doesnt handle these via settings, the data that defines the default providers lives in the source tree so it can be updated at will, I think the best option is likely to remove 'search.providers' completely and have a the providers.json file end up in a /shared/ location so it can be accessed by settings and the search app, user defined search engines can then live in settings or datastore or wherever.

Comment 5

[Dale Harvey (:daleharvey)]

Ok, so the problem to fix here is we have duplicated settings that get instansiated at different times, urlTemplate is defined in 

https://github.com/mozilla-b2g/gaia/blob/master/apps/search/js/search.js#L26
https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/resources/search/providers.json#L4
https://github.com/mozilla-b2g/gaia/blame/master/customization/settings.json
(similiarly with suggestionsUrlTemplate)

search.providers only gets set when the search page of the settings app is opened which means the search app cant provide the ability to switch settings, the settings are currently being set individually which means they can easily be out of sync, when we update the code (add a new provider or fix existing ones) it only applies in a reset-gaia which means all upgraded users are broken.

Just stating the problem so I can get my head round a solution

Comment 6

[Dale Harvey (:daleharvey)]

So we could do a providers.json file, that can be customised at build time, gets put into `shared/js/providers.json`, both the search app and the settings app would include this file in the format

[
 {'google': {
   'searchUrl': 'htt...'
 }
]
 
Have a `search.selectedProvider = "google"' set as a configurable part of the build

mnc / mcc configurations can introduce new 'search.customProviders' that both the search app / settings can pick up, user configured providers can also be done in the same manner.

By putting the configuration in source code and seperating the configured / dynamic providers we can update the build in providers at any point without worrying about overwriting user data etc

Comment 7

[Dale Harvey (:daleharvey)]

Just as a heads up, this would allow mnc / mcc configuration to specify new search providers and would allow that configuration to change the default provider, but it will not let mnc / mcc configurations change existing provider settings, they would exist alongside the configured defaults. I cant think of any sensible solution to allow overriding built in defaults

Comment 8

[Marcelino Veiga Tuimil [:sonmarce]]

If I understand, it would only be possible for MCC/MNC customizations to add new search providers, but not to overwrite search providers defined in default list. If that is the case, I do not see a problem on this approach.

Comment 9

[Dale Harvey (:daleharvey)]

So clarifying after a discussion with Mike / Peter

Mozilla wants to do the carrier code customisation itself, since its an agreement between (at least) 3 parties (search provider, mozilla, carrier) the current operator variant mechanism requires the carriers to do a lot of things in the right way to ensure they are correctly paid.

So we have a list of mnc/mcc combinations that refer to a 'partner code'

{
  '123-321': 'tef',
  '321-123': 'o2',
  '987-789': 'vodaphone'
}

A (potentially huge) list of carrier customisations, keyed by the 'partner code'

{
  'default': [{
    name: 'yahoo',
    title: 'Yahoo', 
    searchUrl: 'http://yahoo.com?q={searchTerms}',
    suggestionsUrl: 'http://suggest.yahoo.com?q={searchTerms}'
  }, {
    name: 'askjeeves',
    ....
  }],
  'tef': [{
    name: 'yahoo',
    title: 'Yahoo', 
    searchUrl: 'http://yahoo.com?q={searchTerms}&partner=tef',
    suggestionsUrl: 'http://suggest.yahoo.com?q={searchTerms}&partner=tef'
  }]
}

We also want the carrier to put their 'partner code' in the build customisation so we can match whether sim  provider matches who provided the build.

Before I flesh out the implementation, Mike can you clarify some questions for how the implementation should work

1. So no sim, we use the 'default' configuration, all is well

2. The user starts up with a sim, if and only if the partner code indicated by the mnc / mcc matches the partner code provided in the build then those become the defaults yes?

3. This customisation will apply in the same way as our other customisations, in that it will only ever apply once when the first sim is inserted into the device yes?

Cheers

Comment 10

[Dale Harvey (:daleharvey)]

A mostly done patch is @ https://github.com/mozilla-b2g/gaia/pull/27625, this skips out the partner customisation as that discussion keeps going back and forth, but it can land without it, I just need to fix the tests / settings bug.

Arthur, if I load my shared scripts in settings/index.html everything works fine - https://github.com/mozilla-b2g/gaia/pull/27625/files#diff-137e8d2850b5b7e6862f5fcf403bcd96R114, however we dont want to do that, if I 'require' I try to require('search_provider') then I get SearchProvider is undefined, I commented out the code https://github.com/mozilla-b2g/gaia/pull/27625/files#diff-78cd492003464b2691dd9365dddb4729R5, the search provider is defined @ https://github.com/mozilla-b2g/gaia/pull/27625/files#diff-4c0e32780e42a7a697395a9a4c47fb17R1

Any idea whats going on?

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Because shared/search_provider.js does not define an AMD module, so before we load it using require, we should define a shim for it [1]. The shim simply specifies which object(variable) is going to be returned from the require function when loading a non-AMD module.

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/js/config/require.js#L8

Comment 12

[Dale Harvey (:daleharvey)]

Thanks arthur, fixed

Comment 13

[Dale Harvey (:daleharvey)]

Attachment: Implement shared search provider library

Arthur, if you could look at the settings changes that would be great, thanks.

Ok, so I have tried getting these patches reviewed one at a time but that is too unweildy at this point. 

This introduces /shared/search_provider.js to provides a shared api between the search application and the settings app. 

It removes the operator variant customisation which the implementation is being specified in https://bugzilla.mozilla.org/show_bug.cgi?id=1125131, its currently implemented by a stub which will always use the build default but allows us to be flexible with the upcoming implementation.

It fixes upgrades so if we change the configuration data or add new providers they are applied.

There are 3 follow up bugs
 * allow user to switch from search app - https://bugzilla.mozilla.org/show_bug.cgi?id=1117970
 * Implement the new carrier configuration - https://bugzilla.mozilla.org/show_bug.cgi?id=1125131
 * Reenable search notification - https://bugzilla.mozilla.org/show_bug.cgi?id=1125794

I cant really make much more progress at this point as this patch is a large patch touching a lot of different things. 

(as a note, the default yahoo suggestions url doesnt work, but will be fixed along with carrier customisation)

Comment 14

[Kevin Grandon :kgrandon]

Comment on attachment 8554501 [details] [review]
Implement shared search provider library

This is looking pretty good to me. I left a few comments on github, and here are a few things I'd like some clarification on:

- I'm still not too happy about disabling *all* of the tests. I think we should remove the ones we have no current plans for (like bookmarking as it seems that's going away). Ideally we would also fix the others before landing. At a minimum let's file a new bug and reference that in the blacklist.json.

- What makes the search notice hard to keep? I just imagine it easier to keep it working if possible so we can request uplift on fewer patches.

- Why do we want to load the initial search provider config at runtime instead of buildtime?

Comment 15

[Dale Harvey (:daleharvey)]

> At a minimum let's file a new bug and reference that in the blacklist.json

Sure will do that, I tried fixing the tests but with so much moving around it was causing a lot of busy work

> What makes the search notice hard to keep? I just imagine it easier to keep it working if possible so we can request uplift on fewer patches.

The search notice is going to behave differently (on first run, not depending on results), its will hopefully be easy to reenable, but this is a 34 file change right now, I cant just carry on working on it and land it at once.

> Why do we want to load the initial search provider config at runtime instead of buildtime?

As mentioned in github, this is a potentially very large file and I dont want it sitting in 2 applications permanently consuming memory

Comment 16

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8554501 [details] [review]
Implement shared search provider library

EJ, could you help review this patch? Thanks!

Comment 17

[Ben Francis [:benfrancis]]

Comment on attachment 8554501 [details] [review]
Implement shared search provider library

f+ from me on at least the search app parts.

I've left some comments on GitHub.

Francis, should there be a maximum number of search suggestions? The current number provided is quite high.

Dale, it's not entirely clear to me how all the types of customizations would be applied? That includes customization at build time and customization on SIM entry based on MCC/MNC. If the user chooses something other than the default provider, will their choice be preserved on an upgrade? All of this will need documenting here https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/Market_customizations_guide

Comment 18

[Dale Harvey (:daleharvey)]

There are no customisation of the provider list, the correct search engine config is picked at runtime (and will be implemented in https://bugzilla.mozilla.org/show_bug.cgi?id=1125131) we will have a list of the configurations based on the partner agreements / contracts in place.

User selection is preserved on upgrade.

Comment 19

[Dale Harvey (:daleharvey)]

I filed a new bug for the related tests, other issues have been addressed.

I want to land this now, its a fairly clear self contained bit of work that fixes a bunch of bugs and gets us pretty close to the specification. the patch is large and every time I add something to it I touch another part which leads to more 'well we should fix this as well'. I am having a hard time basing other changes on top of this.

Comment 20

[Ben Francis [:benfrancis]]

Yeah this patch covers a lot of separate bugs, it's a shame it couldn't have been more divided up, but the end result looks good.

I also noticed that the search heading appears even if there are no suggestions for the current search terms (e.g. if it's a URL), I think it should be hidden. Would you prefer to file a follow-up for this too?

Comment 21

[Dale Harvey (:daleharvey)]

> I also noticed that the search heading appears even if there are no suggestions for the current search terms (e.g. if it's a URL), I think it should be hidden. Would you prefer to file a follow-up for this too?

Yeh, I once the follow up functionality is fixed I will be going through a walkthrough and ensuring everything is to spec and then ask for a ux review, its one of the various things that can be fixed then

Comment 22

[Kevin Grandon :kgrandon]

Comment on attachment 8554501 [details] [review]
Implement shared search provider library

Normally this would be ab R- due to the disabled tests, but I'll go ahead and leave an R+ here so you can land this. I'm under the impression that we should always fix the existing tests when landing features where possible, otherwise we just make more work for ourselves in the long run. (More patches to uplift etc). I disagree with the idea that just because the patch is getting a bit big we can fix the tests later. We've landed *much* larger patches into gaia, 30x as big for example, without disabling tests.

Comment 23

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8554501 [details] [review]
Implement shared search provider library

Hi @Dale, This patch looks nice to me !

I think there is one problem may happen and I just left some comments on Github, please take a look !

Thanks !

Comment 24

[Dale Harvey (:daleharvey)]

Green try run @ https://github.com/mozilla-b2g/gaia/pull/27625

I fixed up the review comments and removed the tests that were definitely not going to be used, 1 test that was disabled was functionality that will be redone in https://bugzilla.mozilla.org/show_bug.cgi?id=1117972, the other 2 are based on functionality we lost but I may be able to revive them. Will get that done shortly.

Green try run @ https://treeherder.mozilla.org/ui/#/jobs?repo=gaia-try&revision=317c186305b1, merged in https://github.com/mozilla-b2g/gaia/commit/782d371263911a526870263bffcb419b52c7c88a

Comment 25

[Dale Harvey (:daleharvey)]

Clearing old needinfos

Comment 26

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

Could you please uplift to v2.2? Thanks.

Comment 27

[Dale Harvey (:daleharvey)]

Pushed to 2.2 in https://github.com/mozilla-b2g/gaia/commit/9cc67d59629ead09a577a3e22f39193f24c79aca

Green try run @ https://treeherder.mozilla.org/#/jobs?repo=gaia-try&revision=4a6c9879b64e

Comment 28

[Dale Harvey (:daleharvey)]

Comment on attachment 8554501 [details] [review]
Implement shared search provider library

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): 2.2 Feature Development, accidentally already landed without approval, proactively asking for approval
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Comment 29

[bhavana bajaj [:bajaj]]

I really hope we have no fallouts from these landings that went in without approval...Also, please make sure to give us all the details requested in the approval request comment especially the risk..

Comment 30

[Francis Djabri [:djabber]]

(In reply to Ben Francis [:benfrancis] from comment #17)
> Comment on attachment 8554501 [details] [review]
> Implement shared search provider library
> 
> f+ from me on at least the search app parts.
> 
> I've left some comments on GitHub.
> 
> Francis, should there be a maximum number of search suggestions? The current
> number provided is quite high.
> 
> Dale, it's not entirely clear to me how all the types of customizations
> would be applied? That includes customization at build time and
> customization on SIM entry based on MCC/MNC. If the user chooses something
> other than the default provider, will their choice be preserved on an
> upgrade? All of this will need documenting here
> https://developer.mozilla.org/en-US/Firefox_OS/Developing_Gaia/
> Market_customizations_guide

I see no compelling reason to limit the number of suggestions as these will rapidly decrease as the user types.