Skip to content
This repository has been archived by the owner on May 10, 2024. It is now read-only.

Fix #3177: Update pre-populated search engine list #3189

Merged
merged 2 commits into from
Jan 13, 2021
Merged

Conversation

iccub
Copy link
Contributor

@iccub iccub commented Jan 12, 2021

This also replaces Yahoo with Ecosia on search onboarding screen.

Summary of Changes

This pull request fixes #3177

Submitter Checklist:

  • Unit Tests are updated to cover new or changed functionality
  • User-facing strings use NSLocalizableString()

Test Plan:

  1. New user
  • Set region with Ecosia enabled, United States for example
  • Fresh install the app
  • Verify that Ecosia is one of search options showed during onboarding
  • Verify that Yahoo is not listed as one of search options
  • Select it, make sure search queries use this search engine
  • Verify that correct search parameters are passed, you can refer to https://github.com/brave/brave-browser/wiki/Search-engine-integrations
  1. Existing user
  • Install previous app build, select some search engine
  • Update to build with Ecosia, make sure eligible region(like US) is set
  • Go to Brave Settings-> search engines
  • Verify that Ecosia is on the list, select it
  • Verify that correct search parameters are passed
  1. Existing Yahoo user
  • Install previous app build, select Yahoo as default search engine
  • Update to build with Ecosia, make sure eligible region(like US) is set
  • Verify that default search is still Yahoo

Screenshots:

Zrzut ekranu 2021-01-12 o 15 21 20

Reviewer Checklist:

  • Issues include necessary QA labels:
    • QA/(Yes|No)
    • release-notes/(include|exclude)
    • bug / enhancement
  • Necessary security reviews have taken place.
  • Adequate unit test coverage exists to prevent regressions.
  • Adequate test plan exists for QA to validate (if applicable).
  • Issue is assigned to a milestone (should happen at merge time).

@iccub iccub requested review from bsclifton and a team January 12, 2021 13:44
This also replaces Yahoo with Ecosia on search onboarding screen.
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look great to me! We'll keep Yahoo as an available engine (not in onboarding of course) until we can support having it migrated off (maybe after merging OpenSearch code)

@iccub
Copy link
Contributor Author

iccub commented Jan 12, 2021

@bsclifton yes, i tried to move yahoo out, but found no simple way to do it, we will revisit it later

case google, bing, duckduckgo, yandex, qwant, startpage, yahoo
case google, bing, duckduckgo, yandex, qwant, startpage, yahoo, ecosia

var excludedFromOnboarding: Bool {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property is temporary right until we remove the yahoo engine totally ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can keep it, if we add a lot of predefined search engines in the future we can choose which of them to show during onboarding, to avoid having a long list of search engines

@iccub iccub merged commit 9dc6c2d into development Jan 13, 2021
@iccub iccub deleted the bugfix/3177 branch January 13, 2021 19:02
iccub added a commit that referenced this pull request Jan 13, 2021
This also replaces Yahoo with Ecosia on search onboarding screen.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Updated pre-populated search engine list
4 participants