Bugfix FXIOS-14056 [Trending Searches] google trending url

[cyndichin]

📜 Tickets

Jira ticket
Github issue

💡 Description

The trending url for google was only showing a list of terms related to search terms and not the actual trending searches. We should be using /complete/search?q=&client=firefox&channel=ftr instead. Added an override bool overrideSearchArg to avoid appending searchTerms to the search query argument.

🎥 Demos

Before	After

📝 Checklist

• I filled in the ticket numbers and a description of my work
• I updated the PR name to follow our PR naming guidelines
• I ensured unit tests pass and wrote tests for new code
• If working on UI, I checked and implemented accessibility (Dynamic Text and VoiceOver)
• If adding telemetry, I read the data stewardship requirements and will request a data review
• If adding or modifying strings, I read the guidelines and will request a string review from l10n
• If needed, I updated documentation and added comments to complex code

[mobiletest-ci-bot]

	Messages
📖	Project coverage: 38.63%

🧹 Tidy commit

Just 1 file(s) touched. Thanks for keeping it clean and review-friendly!

💬 Description craftsman

Great PR description! Reviewers salute you 🫡

✅ Per-file coverage

All changed files meet the threshold of 35.0%.

Client.app: Coverage: 37.15

File	Coverage
OpenSearchEngine.swift	82.72%	✅

Generated by 🚫 Danger Swift against 1297c13

[mattreaganmozilla]

@cyndichin If we're always wanting this behavior for trending URLs, why are we having to override this on the client. Shouldn't this be fixed in the source data (on Remote Settings)? Apologies if I'm misunderstanding or missing something obvious. 👀

[cyndichin]

Thanks for calling this out @mattreaganmozilla !!

Summarizing what we discussed on slack:

why are we having to override this on the client.

We get this configuration from remote settings:

Optional<SearchEngineUrl>
  ▿ some : SearchEngineUrl
    - base : "https://www.google.com/complete/search"
    - method : "GET"
    ▿ params : 2 elements
      ▿ 0 : SearchUrlParam
        - name : "client"
        ▿ value : Optional<String>
          - some : "firefox"
        - enterpriseValue : nil
        - experimentConfig : nil
      ▿ 1 : SearchUrlParam
        - name : "channel"
        ▿ value : Optional<String>
          - some : "ftr"
        - enterpriseValue : nil
        - experimentConfig : nil
    ▿ searchTermParamName : Optional<String>
      - some : "q"
    - displayName : nil
    - isNewUntil : nil
    - excludePartnerCodeFromTelemetry : false
    - acceptedContentTypes : nil

However, on our side we are parsing the details and formatting into a url via convertASSearchURLToOpenSearchURL. It seems that for our searchTermParamName we are always appending {searchTerms}, but we don't want to do that for trending searches. Therefore, we added an override here.

For trending searches, we want the following url - https://www.google.com/complete/search?client=firefox&channel=ftr&q=

Shouldn't this be fixed in the source data (on Remote Settings)?

We discussed that maybe it makes more sense to not have q under searchTermParamName and instead maybe have it under base or param. Pinged Mark on the consolidated search channel and will wait to hear back on this before proceeding next steps.

[mattreaganmozilla]

Thanks Cyndi for documenting this 🙏

[cyndichin]

So instead of trying to parse the URL differently from remote settings, it was recommended that we should still pass through {searchTerms}, which for the other urls should be replaced by the actual search term. However, for trending searches should be the empty string q=. Updated PR with this commit: 1297c13

[mattreaganmozilla]

🚀

[github-actions]

🚀 PR merged to main, targeting version: 145.2