988867 - Enhance search engines so they will give mockup suggestions

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Infrastructure, defect)

Description

[Cosmin Malutan, [:cosmin-malutan]]

We have to enhance search engines so they will give suggestion. This is need for bug 942737.

Comment 1

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v1.0

Here is the patch for this.

Comment 2

[Cosmin Malutan, [:cosmin-malutan]]

Comment on attachment 8397846 [details] [diff] [review]
patch_v1.0

I change the review flag to Andreea and Andrei, I thought it was reviewed by Henrink but it wasn't.

Comment 3

[Andrei Eftimie]

Comment on attachment 8397846 [details] [diff] [review]
patch_v1.0

Review of attachment 8397846 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/search/mozsearch_suggestions.json
@@ +1,1 @@
> +["mozilla",["Mozsearch suggestion 0","Mozsearch suggestion 1","Mozsearch suggestion 2","Mozsearch suggestion 3","Mozsearch suggestion 4","Mozsearch suggestion 5","Mozsearch suggestion 6","Mozsearch suggestion 7","Mozsearch suggestion 8","Mozsearch suggestion 9"],[],[]]

Please properly indent these json responses.
It will make them easier to maintain.

Why do we have 2 empty Arrays at the end? I don't think

::: firefox/search/opensearch.xml
@@ +7,2 @@
>    <Url type="text/html" method="get"
>         template="http://hg.mozilla.org/qa/litmus-data/raw-file/tip/firefox/search/searchresults.html?q={searchTerms}"></Url>

This url is no longer valid.
Please update it to use mozqa.com

::: firefox/search/opensearch_suggestions.json
@@ +1,1 @@
> +["mozilla",["Openserach suggestion 0","Openserach suggestion 1","Openserach suggestion 2","Openserach suggestion 3","Openserach suggestion 4","Openserach suggestion 5","Openserach suggestion 6","Openserach suggestion 7","Openserach suggestion 8","Openserach suggestion 9"],[],[]]

typo: Opensearch

We should also be consistent with the casing: either `opensearch` or `OpenSearch`

Comment 4

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.0

Thanks for review Andrei, here are the updates.

Comment 5

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8398516 [details] [diff] [review]
patch_v2.0

Review of attachment 8398516 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/search/mozsearch_suggestions.json
@@ +1,4 @@
> +["Moz",
> +  [
> +    "MozSearch suggestion 0",
> +    "MozSearch suggestion 1",

I believe we would need better names for these suggestions. On the other side, they're general and suits for other projects that might use this.
Henrik, what do you think?

Comment 6

[Andrei Eftimie]

Comment on attachment 8398516 [details] [diff] [review]
patch_v2.0

Review of attachment 8398516 [details] [diff] [review]:
-----------------------------------------------------------------

This has a r+ from me with the mentioned nit fixed.

::: firefox/search/mozsearch_suggestions.json
@@ +8,5 @@
> +    "MozSearch suggestion 5",
> +    "MozSearch suggestion 6",
> +    "MozSearch suggestion 7",
> +    "MozSearch suggestion 8",
> +    "MozSearch suggestion 9"]

Just one nit from me: please keep this consistent and move the closing bracket to a newline as it is in the other json file.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8398516 [details] [diff] [review]
patch_v2.0

Review of attachment 8398516 [details] [diff] [review]:
-----------------------------------------------------------------

It's good to see that it is not that hard to actually do the switch over to json. Please see my inline comment.

::: firefox/search/mozsearch_suggestions.json
@@ +1,4 @@
> +["Moz",
> +  [
> +    "MozSearch suggestion 0",
> +    "MozSearch suggestion 1",

Yes, I vote for better names here too, but we should also return descriptions, and search urls. Also I don't think we need 10 suggestions. About 5 should be enough.

So what should we return:
* Shorter suggestions
* Variations - not all suggestions should start with Moz, but also should contain it in the middle or at the end of the word
* I would add "Mozi" as search term too, and return different values
* We have to update the test case so it mentions which search terms are supported

Comment 8

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.1

I added description and URLs.
As for suggestions I took random suggestion from different already existing engines(Google, Wikipedia and Yahoo).

Comment 9

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8399428 [details] [diff] [review]
patch_v2.1

Review of attachment 8399428 [details] [diff] [review]:
-----------------------------------------------------------------

Looks better to me.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8399428 [details] [diff] [review]
patch_v2.1

Review of attachment 8399428 [details] [diff] [review]:
-----------------------------------------------------------------

::: firefox/search/mozsearch_suggestions.json
@@ +1,1 @@
> +["Mozi",

It still misses a second search term. Also I don't see an update for the appropriate test page, which tells what search terms are supported. If you ask me I would use 'm' and 'mo'.

::: firefox/search/opensearch_suggestions.json
@@ +1,1 @@
> +["Mozi",

Same here as for the mozsearch engine.

Comment 11

[Cosmin Malutan, [:cosmin-malutan]]

(In reply to Henrik Skupin (:whimboo) from comment #10)
> ::: firefox/search/mozsearch_suggestions.json
> @@ +1,1 @@
> > +["Mozi",
> 
> It still misses a second search term. Also I don't see an update for the
> appropriate test page, which tells what search terms are supported. If you
> ask me I would use 'm' and 'mo'.
No matter of the search term I will retrieve the same answer because the response is hardcoded.
What I can do is to give the search term in the path for the json file and offer something like this:
> <Url type="application/x-suggestions+json" method="GET" template="http://mozqa.com/data/firefox/search/mozsearch_suggestions_{searchterm}.json"/>
and create two json files:
>mozsearch_suggestions_m.json
>mozsearch_suggestions_mo.json

If this is what you have in mind it will update the patch.
Thanks

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

If that works, yeah lets do that.

Comment 13

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.2

I added suggestions for "m" and "mo" keywords.

Comment 14

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8401277 [details] [diff] [review]
patch_v2.2

Review of attachment 8401277 [details] [diff] [review]:
-----------------------------------------------------------------

Way better this way :)

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 8401277 [details] [diff] [review]
patch_v2.2

Review of attachment 8401277 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Please fix the one nit, and we are good to get this landed.

::: firefox/search/mozsearch.xml
@@ +8,3 @@
>    <Url type="text/html" method="GET"
>         template="http://mozqa.com/data/firefox/search/searchresults.html?q={searchTerms}"/>
>    <SearchForm>http://mozqa.com/data/firefox/search/searchresults.html</SearchForm> 

nit: please kill this trailing white-space.

Comment 16

[Cosmin Malutan, [:cosmin-malutan]]

Attachment: patch_v2.3

Thanks for review Henrik, I pass this to Andreea with the nit fixed!

Comment 17

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 8401760 [details] [diff] [review]
patch_v2.3

Review of attachment 8401760 [details] [diff] [review]:
-----------------------------------------------------------------

Landed:
http://hg.mozilla.org/qa/testcase-data/rev/f96a01c5508c

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Something went wrong here. This check-in is not listed:
http://hg.mozilla.org/qa/testcase-data/

Comment 19

[Henrik Skupin [:whimboo][⌚️UTC+1]]

My bad. Not sure what went wrong, but now the entry appeared in the pushlog. Let me see what's wrong with mozqa.com in fetching those updates.