1022053 - Add notification in search app for users to configure search provider

Status: VERIFIED FIXED

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

Comment 1

[Dale Harvey (:daleharvey)]

Francis, this is the bug tracking the implementation of https://mozilla.app.box.com/s/0i2j11d3odruziews0r7

I realised that we probably need some visuals for having suggestions disabled, as there will be no icon, if we hide the suggestions row / dont display something we will be hiding a way we have taught the user to configure that.

Comment 2

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/daleharvey/gaia/commit/de913292fd382ba42ec6d24c24a97d326cfe0c6b

Implements the type notice that tells the user how to configure search providers and the other UX tweaks as specified in https://bugzilla.mozilla.org/show_bug.cgi?id=948320

I have left the 'icon' to be a white circle so it will be easier to see in testing, that will change on the parent bug landing (and based on feedback from francis)

Pavel, also not keen on this styling being in here, I was wondering if there was any shared styles I should pull in for this / cleaner implementation

I figured the functionality can be reviewed prior to fixing up the icon logic as that will be a small change to follow up.

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8436256 [details]
https://github.com/daleharvey/gaia/commit/de913292fd382ba42ec6d24c24a97d326cfe0c6b

I would like to see Pavel or Francis respond first. The white dot looks a bit weird, and this close to FL I think we can't land incomplete things to master. The patch generally looks good, but I'm hesitant to R+ it because I'm not sure about landing order.

I'm also not seeing the notification 100% of the time for some reason. Am I supposed to?

Comment 4

[Ben Francis [:benfrancis]]

I think there is some confusion here.

The visual design at https://mozilla.app.box.com/s/0i2j11d3odruziews0r7 is based on the interaction design at https://mozilla.app.box.com/s/5dy7gllr9k3xaro44d07 but that interaction design was the best-case scenario of three different design options (https://mozilla.app.box.com/s/bb95jlbdk9l0robxluxc/1/1955330942) where we support multiple search suggestion providers (a stretch goal for 2.0).

1) Ideal scenario - multiple search suggestions providers, icon in settings app shows the current provider and acts as a shortcut to settings.
2) EverythingMe suggestions - Search suggestions are always provided by EverythingMe, no icon is shown in the search app because it would be confusing (search results come from one provider, suggestions from another).
3) No suggestions - Failing 2, no suggestions should be shown at all if the provider is anything other than EverythingMe.

The actual interaction spec that was implemented was 2) https://mozilla.app.box.com/s/bb95jlbdk9l0robxluxc/1/1955330942/17595207603/1 where we don't show the provider icon next to suggestions as a shortcut to settings.

Comment 5

[Stephany Wilkes]

Flagging Eric since he owns VxD here.

Comment 6

[Dale Harvey (:daleharvey)]

The notification is shown one (after typing 3 characters), once its shown once it is not shown again.

Based on this mornings meetings, I think we mostly just want an icon that isnt specific to any search provider so the user can still get the notice and navigate to configure their providers

Comment 7

[Dale Harvey (:daleharvey)]

Attachment: https://github.com/mozilla-b2g/gaia/pull/20248

New patch based on updated specs, the notification is only to be shown one time and when dismissed it will not show again.

Comment 8

[Dale Harvey (:daleharvey)]

Running a build @ https://travis-ci.org/daleharvey/gaia/builds/27168179

Seems to be a preexisting intermittent with the vertical homescreen (icon_retriever)

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8437200 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20248

Looks good! Going to leave some nits on github. Would like to see a rebased and green travis run before giving an R+.

Comment 10

[Dale Harvey (:daleharvey)]

Comment on attachment 8437200 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20248

Patch has been updated with nits addressed

Comment 11

[Dale Harvey (:daleharvey)]

Theres a travis run @ https://travis-ci.org/daleharvey/gaia/builds/27417275 which should kick off shortly

Comment 12

[Dale Harvey (:daleharvey)]

Some genuine failing tests, taking a look

Comment 13

[Kevin Grandon :kgrandon]

Comment on attachment 8437200 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20248

Sounds good. Flag me again when you think it's ready. Thanks!

Comment 14

[Dale Harvey (:daleharvey)]

Comment on attachment 8437200 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20248

Fixed tests, we now use and confirm the notice during integration tests, so this is very well tested

`apps/search/test/marionette/app_search_test.js` has extra code to switch the frame + focus so we can use the same triggerFirstRun whether we are in full rocketbar or hoomescreen.

https://travis-ci.org/daleharvey/gaia/builds/27452219 is pretty much a green run, the failing integration test is | 1) Contacts > Delete  > Edit menu is not visible on search mode:| which is definitely unrelated

Comment 15

[Kevin Grandon :kgrandon]

Comment on attachment 8437200 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20248

I left a few comments on github. Assuming you address them, and have a green build somewhere, R+ from me. Thanks a lot!

Comment 16

[Jason Smith [:jsmith]]

Candice - Give that this is a feature, can you tag this with feature-b2g?

Comment 17

[Candice Serran (:cserran)]

yep...done

Comment 18

[Dale Harvey (:daleharvey)]

Addressed the comments, green build @ https://travis-ci.org/daleharvey/gaia/builds/27522253

landed in https://github.com/mozilla-b2g/gaia/commit/18e592f53add20d36e6d40937e2714d8be9d067d

Comment 19

[Francesco Lodolo [:flod]]

"Ok" is not correct in English, it should be spelled OK.
No need to change the string ID for this kind of change.

Comment 20

[Dale Harvey (:daleharvey)]

Apologies, was as specified, but now you point it out its obviously wrong, minor fix so followed it up

https://github.com/mozilla-b2g/gaia/commit/c588bdd1b6e02d294d2b33fe28487e325f2167c9

Note for uplifting, both the following commits need uplifted

https://github.com/mozilla-b2g/gaia/commit/18e592f53add20d36e6d40937e2714d8be9d067d
https://github.com/mozilla-b2g/gaia/commit/c588bdd1b6e02d294d2b33fe28487e325f2167c9

Comment 21

[Francesco Lodolo [:flod]]

(In reply to Dale Harvey (:daleharvey) from comment #20)
> Note for uplifting, both the following commits need uplifted

If this bug is going to be uplifted, it's missing late-l10n in the keywords and approval? flag for 2.0

Comment 22

[Dale Harvey (:daleharvey)]

Comment on attachment 8437200 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/20248

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): New Feature
[User impact] if declined: Inadequate 
[Testing completed]: Used in hot path of integration tests
[Risk to taking this patch] (and alternatives if risky): minor
[String changes made]: 2 new strings

Comment 23

[Kevin Grandon :kgrandon]

Attachment: Follow-up commit (also uplift)

Comment 24

[Kevin Grandon :kgrandon]

Attachment: Github commit

Obsoleting the pull request in favor of the commit that actually landed to ease uplifting. 

Carrying 2.0 approval request.

Comment 25

[Johan Lorenzo [:jlorenzo]]

On master:
* Saw the notification after typing 3 characters.
* Notification is still present while keeping to type after it pop'd.
* After tapping OK, the notification doesn't show up anymore after typing more character or performing anther search. 
* Pop-up is not shown anymore after hitting the close or home button.

It's OK for me on these points.

Still an issue after hitting Enter, the notification is not dismissed as per specs: https://mozilla.app.box.com/s/bb95jlbdk9l0robxluxc/1/1955330942/18050739494/1

Bug 1026007 has been filed.

Comment 26

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/565c7a905a109fc8d5039618caac6e693b43fe5c
v2.0: https://github.com/mozilla-b2g/gaia/commit/23701b639bcb726bfaec460532c9eaa671681a9b

Comment 27

[Lancy(Leave from Mozilla)]

Attachment: Verify_Video_Flame.MP4

This issue has been verified successfully on Flame 2.0 & 2.1.
See attachment: Verify_Video_Flame.MP4
Reproducing rate: 0/5

Flame v2.0 version:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141202000201
Version         32.0

Flame v2.1 version:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141202001201
Version         34.0